Skip to content

Fix script corruption in executeJxa shell-escape#32

Open
nzeribe wants to merge 1 commit into
dvcrn:mainfrom
nzeribe:fix/executejxa-shell-escape
Open

Fix script corruption in executeJxa shell-escape#32
nzeribe wants to merge 1 commit into
dvcrn:mainfrom
nzeribe:fix/executejxa-shell-escape

Conversation

@nzeribe
Copy link
Copy Markdown

@nzeribe nzeribe commented May 17, 2026

Summary

executeJxa builds the osascript command as a shell string and escapes single quotes with '''. That isn't a valid bash escape inside a single-quoted string — '' collapses to nothing, so every apostrophe in the JXA script is silently stripped before reaching osascript.

The bug is latent today because the project's JXA strings use double quotes throughout, but any tool emitting a single-quoted JXA string literal ('foo', a contraction in an error message thrown back via error.toString(), etc.) would produce either an osascript syntax error or, worse, a silently malformed script.

Fix

Switch from exec (shell-interpreted command string) to execFile with an argv array. osascript receives the script as a single argument — no shell, no escaping needed.

Test plan

  • `npm run format:check` passes
  • `npm test` passes (12/12)
  • `npm run build` succeeds
  • Manually verified an end-to-end record lookup (`getRecordWithUuid` + property access) round-trips valid JSON via the new `execFile` path

executeJxa built the osascript command as a shell string and escaped
single quotes with `'` -> `''`. That is not a valid bash escape inside
a single-quoted string; `''` collapses to nothing, so every apostrophe
in the JXA script was silently stripped before reaching osascript.

The bug is latent today because the project's JXA strings use double
quotes throughout, but any tool emitting a single-quoted JXA string
literal (an `'foo'`, an apostrophe in a thrown error message, etc.)
would produce a syntax error or, worse, a silently malformed script.

Switch to execFile with an argv array. osascript receives the script
as a single argument, no shell interpretation, no escaping needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the use of exec with execFile in src/applescript/execute.ts to improve security and handle script escaping more reliably when executing JXA scripts. Feedback was provided regarding the default maxBuffer size, suggesting an increase to 10MB to prevent potential ERR_CHILD_PROCESS_STDIO_MAXBUFFER errors when processing large amounts of metadata or document content from DEVONthink. Additionally, it was noted that for very large scripts, piping to osascript via stdin would be more robust than using the -e flag to avoid system command-line argument length limits.

return new Promise((resolve, reject) => {
const command = `osascript -l JavaScript -e '${script.replace(/'/g, "''")}'`;
exec(command, (error, stdout, stderr) => {
execFile("osascript", ["-l", "JavaScript", "-e", script], (error, stdout, stderr) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While execFile is a significant improvement over exec for security and escaping, it still uses a default maxBuffer of 1MB. For a tool interacting with a database like DEVONthink, which can return large amounts of metadata or document content (e.g., via get_record_content), this limit might be exceeded, causing the process to fail with ERR_CHILD_PROCESS_STDIO_MAXBUFFER. Consider increasing the buffer size.\n\nAdditionally, passing the script via the -e flag is subject to the system's command-line argument length limit (ARG_MAX). For very large scripts (e.g., when updating records with large text content), it would be more robust to pipe the script to osascript via stdin in the future.

Suggested change
execFile("osascript", ["-l", "JavaScript", "-e", script], (error, stdout, stderr) => {
\t\texecFile("osascript", ["-l", "JavaScript", "-e", script], { maxBuffer: 10 * 1024 * 1024 }, (error, stdout, stderr) => {

@dvcrn
Copy link
Copy Markdown
Owner

dvcrn commented May 18, 2026

Thanks for this, it LGTM! Let me try this change later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants