refactor(lookupRecord): harden JXA escaping using shared helpers#35
Open
ebowman wants to merge 2 commits into
Open
refactor(lookupRecord): harden JXA escaping using shared helpers#35ebowman wants to merge 2 commits into
ebowman wants to merge 2 commits into
Conversation
added 2 commits
May 25, 2026 15:04
…itted Previously, when `databaseName` was not supplied, `lookup_record` silently scoped to `theApp.currentDatabase()`. This caused lookups to miss records that exist in other open databases, with no signal to the caller that the scope had been narrowed — `success: true` with zero results. DEVONthink's `lookupRecordsWith*` AppleScript APIs (unlike `search`) don't natively span all open databases when `in:` is omitted, so the fix iterates `theApp.databases()` and unions the per-database results, deduplicating by UUID. The `x-devonthink-item://` URL shortcut still resolves globally via `getRecordWithUuid` and short-circuits the loop. Schema and tool descriptions updated to document the new default. Tests: 11 unit tests verifying script generation across all six `lookupType` branches, scoping behavior with and without `databaseName`, URL shortcut, matchAnyTag, and UUID dedup.
The previous lookupRecord script template interpolated user-controlled
strings (value, databaseName, lookupType, tags) raw — e.g.
`"${value}"`. Any value containing a double quote, backslash, or newline
would break the JXA at runtime, and there was no boundary check for
control characters that can't be safely escaped at all. This mirrors the
issue addressed for create/update by PR dvcrn#32 (executeJxa shell-escape)
and issue dvcrn#26 (lost apostrophes), but at the script-template layer.
Changes:
- Pre-validate string inputs with `isJXASafeString`; reject control
characters (0x00-0x08, 0x0B, 0x0C, 0x0E-0x1F, 0x7F) with a structured
error before any JXA is built.
- Build all interpolated values as safely-escaped JXA literals up front
using `formatValueForJXA` / `escapeStringForJXA` and declare them as
`const p*` at the top of the script. The body references `pValue`,
`pDatabaseName`, `pLookupType`, `pTags`, `pMatchAnyTag`, `pLimit`
rather than re-interpolating raw input. Mirrors the pattern in
`src/tools/search.ts` and `src/tools/importFile.ts`.
- The `x-devonthink-item://` URL shortcut, the `matchAnyTag` flag, the
cross-database UUID dedup, and the default-scope semantics from PR1
are preserved unchanged.
Tests: 7 new unit tests covering quote/backslash/newline escaping in
value, databaseName, and tags; plus control-character rejection on each
of those inputs.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the lookup_record tool by implementing input validation for JXA safety, improving string escaping, and expanding the default search scope to include all open databases. It also adds result deduplication and a specialized shortcut for x-devonthink-item:// URLs, supported by a new suite of unit tests. Feedback focuses on adhering to the project's style guide regarding JXA interpreter limitations, specifically requiring the use of bracket notation for object property assignments and returns within the script templates to avoid potential runtime errors.
Comment on lines
106
to
110
| return JSON.stringify({ | ||
| success: false, | ||
| error: "Database not found: ${databaseName}" | ||
| error: "Database not found: " + pDatabaseName | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
The JXA interpreter has known limitations with object literal syntax when generated via template literals. According to the repository style guide, objects should be built using bracket notation before being stringified or returned to avoid potential ReferenceError issues.
Suggested change
| return JSON.stringify({ | |
| success: false, | |
| error: "Database not found: ${databaseName}" | |
| error: "Database not found: " + pDatabaseName | |
| }); | |
| } | |
| const errorRes = {}; | |
| errorRes["success"] = false; | |
| errorRes["error"] = "Database not found: " + pDatabaseName; | |
| return JSON.stringify(errorRes); |
References
- JXA cannot return object literals directly. Always build objects using bracket notation before returning. (link)
|
|
||
| switch (pLookupType) { | ||
| case "filename": | ||
| dbResults = theApp.lookupRecordsWithFile(pValue, { in: searchDatabase }); |
Contributor
There was a problem hiding this comment.
Avoid using inline object literals as arguments in JXA scripts. It is safer to define an options object using bracket notation first.
Suggested change
| dbResults = theApp.lookupRecordsWithFile(pValue, { in: searchDatabase }); | |
| const options = {}; | |
| options["in"] = searchDatabase; | |
| dbResults = theApp.lookupRecordsWithFile(pValue, options); |
References
- Use bracket notation for object property assignment to avoid JXA interpreter limitations. (link)
Comment on lines
+165
to
+168
| const tagOptions = { in: searchDatabase }; | ||
| if (pMatchAnyTag) { | ||
| tagOptions.any = true; | ||
| } |
Contributor
There was a problem hiding this comment.
Refactor tagOptions to use bracket notation for property assignment to ensure compatibility with the JXA interpreter.
Suggested change
| const tagOptions = { in: searchDatabase }; | |
| if (pMatchAnyTag) { | |
| tagOptions.any = true; | |
| } | |
| const tagOptions = {}; | |
| tagOptions["in"] = searchDatabase; | |
| if (pMatchAnyTag) { | |
| tagOptions["any"] = true; | |
| } |
References
- Use bracket notation for object property assignment. (link)
|
|
||
| // Include URL if available | ||
| if (record.url && record.url()) { | ||
| result.url = record.url(); |
Contributor
There was a problem hiding this comment.
Use bracket notation for property assignment.
Suggested change
| result.url = record.url(); | |
| result["url"] = record.url(); |
References
- Use bracket notation for object property assignment. (link)
Comment on lines
221
to
223
| if (record.comment && record.comment()) { | ||
| result.comment = record.comment(); | ||
| } |
Contributor
There was a problem hiding this comment.
Use bracket notation for property assignment.
Suggested change
| if (record.comment && record.comment()) { | |
| result.comment = record.comment(); | |
| } | |
| if (record.comment && record.comment()) { | |
| result["comment"] = record.comment(); | |
| } |
References
- Use bracket notation for object property assignment. (link)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
lookup_recordscript template interpolates user-controlled strings raw:A value containing a double quote, backslash, or newline breaks the JXA at runtime. There's also no boundary check for control characters (NUL, etc.) which can't be safely escaped at all. Same shape of issue as #26 (lost apostrophes in
create_record) and #32 (executeJxashell-escape), but at the script-template layer.Fix
Mirror the pattern already used in
src/tools/search.tsandsrc/tools/importFile.ts:value,databaseName, every entry intags) withisJXASafeString. If any contains a control character (0x00-0x08,0x0B,0x0C,0x0E-0x1F,0x7F), return a structured error before any JXA is built.formatValueForJXA/escapeStringForJXA, then declare them asconst p*at the top of the script (pValue,pDatabaseName,pLookupType,pTags,pMatchAnyTag,pLimit). The script body references these constants and never re-interpolates raw input.The functional behavior from #34 — cross-database scope,
x-devonthink-item://UUID shortcut,matchAnyTaghandling, UUID dedup — is preserved unchanged.Test plan
npm test— 34/34 pass (7 new escaping tests intests/tools/lookupRecord.test.ts):valueis escaped (evil"injection.md→"evil\"injection.md")valueare escapeddatabaseNameis escapedtags[]entries is escapedvalueis rejected at the boundarydatabaseNameis rejected at the boundarytags[]is rejected at the boundarynpm run buildcleannpm run format:checkcleanNotes for the maintainer
\x00,\x01,\x07(TypeScript hex escapes) to keep the source as text — git would otherwise treat the file as binary.escapeStringForJXAto escape the few additional Unicode line-terminators (