From 9e0d925f9563bf12d8b3a95e1a238b6eaac1819f Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 13:09:26 -0700 Subject: [PATCH 1/9] Check for a few more esoteric subshell patterns --- .../__tests__/command-validation.spec.ts | 36 +++++++++++++++++++ webview-ui/src/utils/command-validation.ts | 9 ++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index e87bae07e5..58edc86723 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -313,6 +313,30 @@ ls -la || echo "Failed"` expect(containsDangerousSubstitution("rm *(e:sudo apt install malware:)")).toBe(true) }) + it('detects bash $"..." string interpolation with command substitution', () => { + // The exact example from the issue + expect(containsDangerousSubstitution('echo $"test$(whoami)"')).toBe(true) + + // Various forms of command substitution inside $"..." + expect(containsDangerousSubstitution('echo $"Hello $(date)"')).toBe(true) + expect(containsDangerousSubstitution('echo $"User: `whoami`"')).toBe(true) + expect(containsDangerousSubstitution('echo $"test`pwd`"')).toBe(true) + + // Multiple command substitutions + expect(containsDangerousSubstitution('echo $"$(date) - $(whoami)"')).toBe(true) + expect(containsDangerousSubstitution('echo $"User: `whoami` in `pwd`"')).toBe(true) + + // Command substitution at different positions + expect(containsDangerousSubstitution('echo $"$(whoami) is the user"')).toBe(true) + expect(containsDangerousSubstitution('echo $"The user is $(whoami)"')).toBe(true) + expect(containsDangerousSubstitution('echo $"Current $(date) time"')).toBe(true) + + // Complex command substitutions + expect(containsDangerousSubstitution('echo $"Files: $(ls -la)"')).toBe(true) + expect(containsDangerousSubstitution('echo $"Process: $(ps aux | grep node)"')).toBe(true) + expect(containsDangerousSubstitution('echo $"Result: `rm -rf /`"')).toBe(true) + }) + it("does NOT flag safe parameter expansions", () => { // Regular parameter expansions without dangerous operators expect(containsDangerousSubstitution("echo ${var}")).toBe(false) @@ -351,6 +375,15 @@ ls -la || echo "Failed"` expect(containsDangerousSubstitution("rm *.txt")).toBe(false) expect(containsDangerousSubstitution("cat ?(foo|bar)")).toBe(false) expect(containsDangerousSubstitution("echo *(^/)")).toBe(false) // Safe glob qualifier (not e:) + + // Safe $"..." strings without command substitution + expect(containsDangerousSubstitution('echo $"Hello World"')).toBe(false) + expect(containsDangerousSubstitution('echo $"This is a test"')).toBe(false) + expect(containsDangerousSubstitution('echo $"User: $USER"')).toBe(false) // Variable expansion is safe + expect(containsDangerousSubstitution('echo $"Path: ${PATH}"')).toBe(false) // Variable expansion is safe + expect(containsDangerousSubstitution('echo $"Count: $count"')).toBe(false) + expect(containsDangerousSubstitution('echo $"Translated string with no substitution"')).toBe(false) + expect(containsDangerousSubstitution('echo $""')).toBe(false) // Empty translated string }) it("handles complex combinations of dangerous patterns", () => { @@ -379,6 +412,9 @@ ls -la || echo "Failed"` // The zsh glob qualifier exploit expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true) + + // The bash $"..." string interpolation exploit + expect(containsDangerousSubstitution('echo $"test$(whoami)"')).toBe(true) }) }) }) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 572ca32bad..3a35b02849 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -73,6 +73,7 @@ type ShellToken = string | { op: string } | { command: string } * - <<<$(...) or <<<`...` - Here-strings with command substitution * - =(...) - Zsh process substitution that executes commands * - *(e:...:) or similar - Zsh glob qualifiers with code execution + * - $"..." with command substitution - Bash translated strings with embedded command execution * * @param source - The command string to analyze * @returns true if dangerous substitution patterns are detected, false otherwise @@ -111,6 +112,11 @@ export function containsDangerousSubstitution(source: string): boolean { // This regex matches patterns like *(e:...:), ?(e:...:), +(e:...:), @(e:...:), !(e:...:) const zshGlobQualifier = /[*?+@!]\(e:[^:]+:\)/.test(source) + // Check for $"..." string interpolation with command substitution + // $"..." is a bash feature for translated strings that allows command substitution inside + // e.g., echo $"test$(whoami)" or echo $"test`pwd`" + const bashTranslatedStringWithSubstitution = /\$"[^"]*(\$\(|`)[^"]*"/.test(source) + // Return true if any dangerous pattern is detected return ( dangerousParameterExpansion || @@ -118,7 +124,8 @@ export function containsDangerousSubstitution(source: string): boolean { indirectExpansion || hereStringWithSubstitution || zshProcessSubstitution || - zshGlobQualifier + zshGlobQualifier || + bashTranslatedStringWithSubstitution ) } From 952d8af3e8f67d56c8170d25d69eecbe4f41d996 Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 15:32:40 -0700 Subject: [PATCH 2/9] Better handle nested backticks --- .../utils/__tests__/command-validation.spec.ts | 11 +++++++++++ webview-ui/src/utils/command-validation.ts | 15 ++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 58edc86723..2427fc64b2 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -43,6 +43,12 @@ describe("Command Validation", () => { expect(parseCommand("diff <(sort f1) <(sort f2)")).toEqual(["diff", "sort f1", "sort f2"]) }) + it("handles nested backticks with escaped inner backticks", () => { + const cmd = "echo `echo \\`whoami\\``" + // Should surface both the outer echo and the inner whoami as separate sub-commands + expect(parseCommand(cmd)).toEqual(["echo", "echo", "whoami"]) + }) + it("handles empty and whitespace input", () => { expect(parseCommand("")).toEqual([]) expect(parseCommand(" ")).toEqual([]) @@ -1075,6 +1081,11 @@ describe("Unified Command Decision Functions", () => { // Main command with denied prefix should also be auto-denied expect(getCommandDecision("npm test $(echo hello)", allowedCommands, deniedCommands)).toBe("auto_deny") + + // Nested backticks with escaped inner backticks should not be auto-approved when inner command isn't allowed + expect(getCommandDecision("echo `echo \\`whoami\\``", ["echo"], [])).toBe("ask_user") + // And should be auto-denied when inner command is on the denylist + expect(getCommandDecision("echo `echo \\`whoami\\``", ["echo"], ["whoami"])).toBe("auto_deny") }) it("properly validates subshell commands when no denylist is present", () => { diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 3a35b02849..a4c4f8c107 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -256,8 +256,10 @@ function parseCommandLine(command: string): string[] { subshells.push(inner.trim()) return `__SUBSH_${subshells.length - 1}__` }) - .replace(/`(.*?)`/g, (_, inner) => { - subshells.push(inner.trim()) + // Handle backticks with support for escaped backticks (e.g., \`) + .replace(/`((?:\\`|[^`])*)`/g, (_, inner) => { + const unescaped = inner.replace(/\\`/g, "`").trim() + subshells.push(unescaped) return `__SUBSH_${subshells.length - 1}__` }) @@ -318,7 +320,14 @@ function parseCommandLine(command: string): string[] { commands.push(currentCommand.join(" ")) currentCommand = [] } - commands.push(subshells[parseInt(subshellMatch[1])]) + // Expand subshell into its constituent commands to catch nested substitutions + const subshellContent = subshells[parseInt(subshellMatch[1])] + const expanded = parseCommand(subshellContent) + if (expanded.length > 0) { + commands.push(...expanded) + } else if (subshellContent.trim()) { + commands.push(subshellContent.trim()) + } } else { currentCommand.push(token) } From 63b171b1a2f75563c4f872fde03470aad9002445 Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 15:43:28 -0700 Subject: [PATCH 3/9] More patterns --- .../src/utils/__tests__/command-validation.spec.ts | 4 ++++ webview-ui/src/utils/command-validation.ts | 11 +++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 2427fc64b2..6180e6d28e 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -301,6 +301,8 @@ ls -la || echo "Failed"` it("detects zsh glob qualifiers with code execution (e:...:)", () => { // Basic glob qualifier with command execution expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true) + // Zsh extended glob with qualifier list and brace-arg: e{'whoami'} as reported + expect(containsDangerousSubstitution("ls *(.e{'whoami'})")).toBe(true) // Various glob patterns with code execution expect(containsDangerousSubstitution("cat ?(e:rm -rf /:)")).toBe(true) @@ -1043,6 +1045,8 @@ describe("Unified Command Decision Functions", () => { const globExploit = "ls *(e:whoami:)" // Even though 'ls' might be allowed, the dangerous pattern prevents auto-approval expect(getCommandDecision(globExploit, ["ls", "echo"], [])).toBe("ask_user") + // Zsh extended glob with qualifier list and brace-arg form + expect(getCommandDecision("ls *(.e{'whoami'})", ["ls", "echo"], [])).toBe("ask_user") // Various forms should all be blocked expect(getCommandDecision("cat ?(e:rm -rf /:)", ["cat"], [])).toBe("ask_user") diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index a4c4f8c107..b92aba0d36 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -107,10 +107,13 @@ export function containsDangerousSubstitution(source: string): boolean { // =(...) creates a temporary file containing the output of the command, but executes it const zshProcessSubstitution = /=\([^)]+\)/.test(source) - // Check for zsh glob qualifiers with code execution (e:...:) - // Patterns like *(e:whoami:) or ?(e:rm -rf /:) execute commands during glob expansion - // This regex matches patterns like *(e:...:), ?(e:...:), +(e:...:), @(e:...:), !(e:...:) - const zshGlobQualifier = /[*?+@!]\(e:[^:]+:\)/.test(source) + // Check for zsh glob qualifiers with code execution via the "e" qualifier + // This must detect multiple zsh forms that execute code during glob expansion: + // - Classic: *(e:whoami:) or ?(e:rm -rf /:) etc. + // - With other qualifiers: *(.e:whoami:) (dot means "plain files" plus e:...) + // - Brace/quoted argument forms: *(e{'whoami'}), *(.e{'whoami'}), *(e{"whoami"}), *(.e{"whoami"}) + // Any appearance of an "e" qualifier with an argument inside a glob qualifier list is dangerous. + const zshGlobQualifier = /\([^)]*e\s*(?::[^:]*:|\{[^}]*\}|'[^']*'|"[^"]*")[^)]*\)/.test(source) // Check for $"..." string interpolation with command substitution // $"..." is a bash feature for translated strings that allows command substitution inside From 3d737c66c5003824874986337c0d42933b15c15f Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 15:51:02 -0700 Subject: [PATCH 4/9] More restrictions --- webview-ui/src/utils/__tests__/command-validation.spec.ts | 7 +++++++ webview-ui/src/utils/command-validation.ts | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 6180e6d28e..acea548e14 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -319,6 +319,11 @@ ls -la || echo "Failed"` // Glob qualifiers with complex commands expect(containsDangerousSubstitution("ls *(e:open -a Calculator:)")).toBe(true) expect(containsDangerousSubstitution("rm *(e:sudo apt install malware:)")).toBe(true) + + // Plus-shorthand qualifier that executes a command + expect(containsDangerousSubstitution("ls *(+whoami)")).toBe(true) + expect(containsDangerousSubstitution("ls *(.+{'whoami'})")).toBe(true) + expect(containsDangerousSubstitution('ls *(+"whoami")')).toBe(true) }) it('detects bash $"..." string interpolation with command substitution', () => { @@ -1045,6 +1050,8 @@ describe("Unified Command Decision Functions", () => { const globExploit = "ls *(e:whoami:)" // Even though 'ls' might be allowed, the dangerous pattern prevents auto-approval expect(getCommandDecision(globExploit, ["ls", "echo"], [])).toBe("ask_user") + // Plus-shorthand form should also prevent auto-approval + expect(getCommandDecision("ls *(+whoami)", ["ls", "echo"], [])).toBe("ask_user") // Zsh extended glob with qualifier list and brace-arg form expect(getCommandDecision("ls *(.e{'whoami'})", ["ls", "echo"], [])).toBe("ask_user") diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index b92aba0d36..9b4f93dad9 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -115,6 +115,12 @@ export function containsDangerousSubstitution(source: string): boolean { // Any appearance of an "e" qualifier with an argument inside a glob qualifier list is dangerous. const zshGlobQualifier = /\([^)]*e\s*(?::[^:]*:|\{[^}]*\}|'[^']*'|"[^"]*")[^)]*\)/.test(source) + // Check for zsh glob qualifier shorthand that executes code using +command during glob expansion + // Examples: *(+whoami), *(.+{'whoami'}), *(+"whoami") + // Treat + followed by a non-digit token inside a qualifier list as executable (exclude numeric-only like (+1)) + const zshGlobQualifierPlusShorthand = + /[?*@+!]\([^)]*\+\s*(?:\{[^}]*\}|'[^']*'|"[^"]*"|[a-zA-Z_][^)\s]*)[^)]*\)/.test(source) + // Check for $"..." string interpolation with command substitution // $"..." is a bash feature for translated strings that allows command substitution inside // e.g., echo $"test$(whoami)" or echo $"test`pwd`" @@ -128,6 +134,7 @@ export function containsDangerousSubstitution(source: string): boolean { hereStringWithSubstitution || zshProcessSubstitution || zshGlobQualifier || + zshGlobQualifierPlusShorthand || bashTranslatedStringWithSubstitution ) } From 04e16f3f1e340793fcdd4da1b8d345379a4b865d Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 16:04:04 -0700 Subject: [PATCH 5/9] Even more (fish shell this time) --- .../utils/__tests__/command-validation.spec.ts | 11 +++++++++++ webview-ui/src/utils/command-validation.ts | 15 +++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index acea548e14..2f7b3ba4f7 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -41,6 +41,9 @@ describe("Command Validation", () => { expect(parseCommand("npm test $(echo test)")).toEqual(["npm test", "echo test"]) expect(parseCommand("npm test `echo test`")).toEqual(["npm test", "echo test"]) expect(parseCommand("diff <(sort f1) <(sort f2)")).toEqual(["diff", "sort f1", "sort f2"]) + // fish-style command substitution using parentheses + expect(parseCommand("echo (whoami)")).toEqual(["echo", "whoami"]) + expect(parseCommand("echo (echo test)")).toEqual(["echo", "echo test"]) }) it("handles nested backticks with escaped inner backticks", () => { @@ -1097,6 +1100,14 @@ describe("Unified Command Decision Functions", () => { expect(getCommandDecision("echo `echo \\`whoami\\``", ["echo"], [])).toBe("ask_user") // And should be auto-denied when inner command is on the denylist expect(getCommandDecision("echo `echo \\`whoami\\``", ["echo"], ["whoami"])).toBe("auto_deny") + + // Fish-style substitutions behave like subshells + expect(getCommandDecision("npm install (echo test)", allowedCommands, deniedCommands)).toBe("auto_approve") + expect(getCommandDecision("npm install (npm test)", allowedCommands, deniedCommands)).toBe("auto_deny") + + // Ensure hidden subshell commands are validated even if outer command is allowed + expect(getCommandDecision("echo (whoami)", ["echo", "ls"], [])).toBe("ask_user") + expect(getCommandDecision("echo (whoami)", ["echo", "ls"], ["whoami"])).toBe("auto_deny") }) it("properly validates subshell commands when no denylist is present", () => { diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 9b4f93dad9..f309722991 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -273,6 +273,21 @@ function parseCommandLine(command: string): string[] { return `__SUBSH_${subshells.length - 1}__` }) + // Handle fish-style command substitutions and POSIX subshell grouping: ( ... ) + // At this point we've already replaced $(), <() and >() earlier, so any remaining (...) are either + // fish command substitutions or subshell groupings. We treat them as subshells to validate inner commands. + processedCommand = processedCommand.replace(/\(([^()]*)\)/g, (full, inner: string, offset: number, str: string) => { + // If this was actually a pattern preceded by $, < or > it would have been handled earlier. + // Guard anyway: if the preceding character indicates a different construct, keep original. + const prevChar = offset > 0 ? str[offset - 1] : "" + if (prevChar === "$" || prevChar === "<" || prevChar === ">") { + return full + } + const content = (inner || "").trim() + if (!content) return full + subshells.push(content) + return `__SUBSH_${subshells.length - 1}__` + }) // Then handle quoted strings processedCommand = processedCommand.replace(/"[^"]*"/g, (match) => { quotes.push(match) From 5152e12a16bf57c1858e63d5b6a5f6d63dcb958b Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 16:23:07 -0700 Subject: [PATCH 6/9] more esoteric still --- .../__tests__/command-validation.spec.ts | 14 +++ webview-ui/src/utils/command-validation.ts | 108 +++++++++++++++++- 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 2f7b3ba4f7..d432c143fc 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -578,6 +578,11 @@ echo "Successfully converted $count .jsx files to .tsx"` expect(result).toEqual(["total=$((price * quantity + tax))"]) }) + it("extracts subshells inside arithmetic expressions", () => { + const cmd = "arr=(); echo $((arr[$(whoami)]))" + expect(parseCommand(cmd)).toEqual(["arr=()", "echo $((arr[$(whoami)]))", "whoami"]) + }) + it("should handle complex parameter expansions without errors", () => { const commands = [ "echo ${var:-default}", @@ -1110,6 +1115,15 @@ describe("Unified Command Decision Functions", () => { expect(getCommandDecision("echo (whoami)", ["echo", "ls"], ["whoami"])).toBe("auto_deny") }) + it("flags subshell commands inside arithmetic expressions", () => { + // When whoami appears inside $(( ... )) via $(whoami), it must be validated + expect(getCommandDecision("arr=(); echo $((arr[$(whoami)]))", ["echo", "arr="], [])).toBe("ask_user") + // If whoami is explicitly denied, the whole chain must be auto-denied + expect(getCommandDecision("arr=(); echo $((arr[$(whoami)]))", ["echo", "arr="], ["whoami"])).toBe( + "auto_deny", + ) + }) + it("properly validates subshell commands when no denylist is present", () => { expect(getCommandDecision("npm install $(echo test)", allowedCommands)).toBe("auto_approve") expect(getCommandDecision("npm install `echo test`", allowedCommands)).toBe("auto_approve") diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index f309722991..6f9aa2e79c 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -214,6 +214,8 @@ function parseCommandLine(command: string): string[] { const arithmeticExpressions: string[] = [] const variables: string[] = [] const parameterExpansions: string[] = [] + // Commands extracted from within arithmetic expressions (e.g., $(whoami) inside $((...))) + const embeddedSubshellCommands: string[] = [] // First handle PowerShell redirections by temporarily replacing them let processedCommand = command.replace(/\d*>&\d*/g, (match) => { @@ -221,15 +223,104 @@ function parseCommandLine(command: string): string[] { return `__REDIR_${redirections.length - 1}__` }) - // Handle arithmetic expressions: $((...)) pattern - // Match the entire arithmetic expression including nested parentheses - processedCommand = processedCommand.replace(/\$\(\([^)]*(?:\)[^)]*)*\)\)/g, (match) => { - arithmeticExpressions.push(match) - return `__ARITH_${arithmeticExpressions.length - 1}__` + // Protect bash array empty initializer in assignments: name=() + // Without this, shell-quote may split it into "name=", "(" and ")" + processedCommand = processedCommand.replace(/=\s*\(\s*\)/g, (_match) => { + arrayIndexing.push("()") + return `=__ARRAY_${arrayIndexing.length - 1}__` }) + // Handle arithmetic expressions: $((...)) pattern with balanced parsing. + // We must protect the whole arithmetic region from shell-quote tokenization + // while still discovering any $(...) or backticks inside it. + { + let out = "" + for (let i = 0; i < processedCommand.length; i++) { + // Detect start of $(( ... )) + if (processedCommand[i] === "$" && processedCommand[i + 1] === "(" && processedCommand[i + 2] === "(") { + const start = i + // Track balanced parentheses depth. We saw "((" + let depth = 2 + i += 3 + while (i < processedCommand.length && depth > 0) { + const ch = processedCommand[i] + if (ch === "(") depth++ + else if (ch === ")") depth-- + i++ + } + // i currently points to the char AFTER the one that closed depth to 0 + const match = processedCommand.slice(start, i) + // Extract subshells $(...) inside arithmetic, but skip arithmetic "$((" by requiring next char != "(" + match.replace(/\$\((?!\()(.*?)\)/g, (_m, inner) => { + const trimmed = String(inner).trim() + if (trimmed) { + subshells.push(trimmed) + const expanded = parseCommand(trimmed) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(trimmed) + } + } + return _m + }) + // Extract backtick subshells inside arithmetic + match.replace(/`((?:\\`|[^`])*)`/g, (_m, inner) => { + const unescaped = String(inner).replace(/\\`/g, "`").trim() + if (unescaped) { + subshells.push(unescaped) + const expanded = parseCommand(unescaped) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(unescaped) + } + } + return _m + }) + + arithmeticExpressions.push(match) + out += `__ARITH_${arithmeticExpressions.length - 1}__` + // Compensate for loop's i++ after slice end + i -= 1 + } else { + out += processedCommand[i] + } + } + processedCommand = out + } + // Handle $[...] arithmetic expressions (alternative syntax) processedCommand = processedCommand.replace(/\$\[[^\]]*\]/g, (match) => { + // Extract subshells inside $[ ... ] arithmetic expressions as well + // Skip arithmetic "$((" by ensuring char after "$(" is not "(" + match.replace(/\$\((?!\()(.*?)\)/g, (_m, inner) => { + const trimmed = String(inner).trim() + if (trimmed) { + subshells.push(trimmed) + const expanded = parseCommand(trimmed) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(trimmed) + } + } + return _m + }) + match.replace(/`((?:\\`|[^`])*)`/g, (_m, inner) => { + const unescaped = String(inner).replace(/\\`/g, "`").trim() + if (unescaped) { + subshells.push(unescaped) + const expanded = parseCommand(unescaped) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(unescaped) + } + } + return _m + }) + arithmeticExpressions.push(match) return `__ARITH_${arithmeticExpressions.length - 1}__` }) @@ -262,7 +353,8 @@ function parseCommandLine(command: string): string[] { // Then handle subshell commands $() and back-ticks processedCommand = processedCommand - .replace(/\$\((.*?)\)/g, (_, inner) => { + // Handle command substitution, but avoid arithmetic "$((" by requiring next char != "(" + .replace(/\$\((?!\()(.*?)\)/g, (_, inner) => { subshells.push(inner.trim()) return `__SUBSH_${subshells.length - 1}__` }) @@ -364,6 +456,10 @@ function parseCommandLine(command: string): string[] { commands.push(currentCommand.join(" ")) } + // Include any subshell commands discovered inside arithmetic expressions + if (embeddedSubshellCommands.length > 0) { + commands.push(...embeddedSubshellCommands) + } // Restore quotes and redirections return commands.map((cmd) => restorePlaceholders( From 80a24ddacf52660d8a92783d2395d40af968ac5b Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 16:43:10 -0700 Subject: [PATCH 7/9] fix null exception --- webview-ui/src/utils/command-validation.ts | 30 +++++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 6f9aa2e79c..866549820e 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -372,11 +372,17 @@ function parseCommandLine(command: string): string[] { // If this was actually a pattern preceded by $, < or > it would have been handled earlier. // Guard anyway: if the preceding character indicates a different construct, keep original. const prevChar = offset > 0 ? str[offset - 1] : "" - if (prevChar === "$" || prevChar === "<" || prevChar === ">") { + // Also skip array initializers like name=(...) which follow '=' + if (prevChar === "$" || prevChar === "<" || prevChar === ">" || prevChar === "=") { return full } const content = (inner || "").trim() if (!content) return full + // Avoid creating nested placeholders like (__SUBSH_0__) -> __SUBSH_1__ + // Keep original when content is already a subshell placeholder + if (/^__SUBSH_\d+__$/.test(content)) { + return full + } subshells.push(content) return `__SUBSH_${subshells.length - 1}__` }) @@ -433,17 +439,27 @@ function parseCommandLine(command: string): string[] { // Check if it's a subshell placeholder const subshellMatch = token.match(/__SUBSH_(\d+)__/) if (subshellMatch) { + // Split current accumulated command before expanding subshell content if (currentCommand.length > 0) { commands.push(currentCommand.join(" ")) currentCommand = [] } // Expand subshell into its constituent commands to catch nested substitutions - const subshellContent = subshells[parseInt(subshellMatch[1])] - const expanded = parseCommand(subshellContent) - if (expanded.length > 0) { - commands.push(...expanded) - } else if (subshellContent.trim()) { - commands.push(subshellContent.trim()) + const idx = parseInt(subshellMatch[1], 10) + const subshellContent = subshells[idx] + if (typeof subshellContent === "string") { + const expanded = parseCommand(subshellContent) + if (expanded.length > 0) { + commands.push(...expanded) + } else { + const trimmed = subshellContent.trim() + if (trimmed) { + commands.push(trimmed) + } + } + } else { + // No mapping found for this placeholder in current context; keep token as part of command + currentCommand.push(token) } } else { currentCommand.push(token) From 37faf01202770803537d52d430792828eccfce85 Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 19:16:00 -0700 Subject: [PATCH 8/9] gpt5 fixes from review --- .../__tests__/command-validation.spec.ts | 44 +++ webview-ui/src/utils/command-validation.ts | 255 +++++++++++++----- 2 files changed, 226 insertions(+), 73 deletions(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index d432c143fc..3471ee2150 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -52,6 +52,19 @@ describe("Command Validation", () => { expect(parseCommand(cmd)).toEqual(["echo", "echo", "whoami"]) }) + it("does not treat parentheses inside quotes as subshells", () => { + expect(parseCommand('echo "(whoami)"')).toEqual(['echo "(whoami)"']) + expect(parseCommand("echo '(date)'")).toEqual(["echo '(date)'"]) + expect(parseCommand('printf "(x)"')).toEqual(['printf "(x)"']) + }) + + it("parses POSIX grouping subshell: (cd dir && ls)", () => { + expect(parseCommand("(cd dir && ls)")).toEqual(["cd dir", "ls"]) + }) + + it("parses multiple groupings separated by a pipe: (echo a) | (echo b)", () => { + expect(parseCommand("(echo a) | (echo b)")).toEqual(["echo a", "echo b"]) + }) it("handles empty and whitespace input", () => { expect(parseCommand("")).toEqual([]) expect(parseCommand(" ")).toEqual([]) @@ -329,6 +342,11 @@ ls -la || echo "Failed"` expect(containsDangerousSubstitution('ls *(+"whoami")')).toBe(true) }) + it("does not flag non-glob e:...: parenthetical as dangerous", () => { + expect(containsDangerousSubstitution("(e:whoami:)")).toBe(false) + expect(containsDangerousSubstitution('echo "(e:whoami:)"')).toBe(false) + }) + it('detects bash $"..." string interpolation with command substitution', () => { // The exact example from the issue expect(containsDangerousSubstitution('echo $"test$(whoami)"')).toBe(true) @@ -353,6 +371,10 @@ ls -la || echo "Failed"` expect(containsDangerousSubstitution('echo $"Result: `rm -rf /`"')).toBe(true) }) + it('detects $"..." with escaped quotes and command substitution', () => { + expect(containsDangerousSubstitution('echo $"val=\\"$(whoami)\\""')).toBe(true) + }) + it("does NOT flag safe parameter expansions", () => { // Regular parameter expansions without dangerous operators expect(containsDangerousSubstitution("echo ${var}")).toBe(false) @@ -617,6 +639,11 @@ echo "Successfully converted $count .jsx files to .tsx"` }) }) + it("should handle process substitutions with nested/quoted parentheses", () => { + expect(() => parseCommand('cmd >(grep "(")')).not.toThrow() + expect(() => parseCommand('cmd >(grep "\\(foo\\)")')).not.toThrow() + }) + it("should handle special bash variables without errors", () => { const commands = [ "echo $?", @@ -1514,3 +1541,20 @@ describe("Unified Command Decision Functions", () => { }) }) }) + +// Additional regression tests from jr/more-subshells review +describe("Regression: escaped quotes and process substitution", () => { + it("parses double-quoted strings with escaped quotes without splitting", () => { + expect(parseCommand('echo "val=\\"x\\""')).toEqual(['echo "val=\\"x\\""']) + }) + + it("does not flag bash array assignments as zsh process substitution", () => { + expect(containsDangerousSubstitution("arr=(a b)")).toBe(false) + expect(containsDangerousSubstitution("arr=()")).toBe(false) + }) + + it("handles escaped quotes inside backticks", () => { + const cmd = 'echo `printf \\"%s\\"`' + expect(parseCommand(cmd)).toEqual(["echo", 'printf "%s"']) + }) +}) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 866549820e..244c85c7cd 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -105,26 +105,29 @@ export function containsDangerousSubstitution(source: string): boolean { // Check for zsh process substitution =(...) which executes commands // =(...) creates a temporary file containing the output of the command, but executes it - const zshProcessSubstitution = /=\([^)]+\)/.test(source) + // Tightened regex avoids matching bash array assignments like arr=(a b) or arr=() by ensuring + // '=' is not immediately preceded by an identifier, ']' or '}'. + const zshProcessSubstitution = /(?(cmd)) + * - POSIX grouping subshells ((...)) and fish-style (cmd) substitutions as separate sub-commands * - PowerShell redirections (2>&1) * - Chain operators (&&, ||, ;, |, &) * - Newlines as command separators @@ -224,10 +228,10 @@ function parseCommandLine(command: string): string[] { }) // Protect bash array empty initializer in assignments: name=() - // Without this, shell-quote may split it into "name=", "(" and ")" - processedCommand = processedCommand.replace(/=\s*\(\s*\)/g, (_match) => { + // Require an identifier immediately before '=' to avoid matching unrelated patterns like "x = ()" + processedCommand = processedCommand.replace(/([A-Za-z_][A-Za-z0-9_]*)\s*=\s*\(\s*\)/g, (_match, name: string) => { arrayIndexing.push("()") - return `=__ARRAY_${arrayIndexing.length - 1}__` + return `${name}=__ARRAY_${arrayIndexing.length - 1}__` }) // Handle arithmetic expressions: $((...)) pattern with balanced parsing. @@ -239,13 +243,21 @@ function parseCommandLine(command: string): string[] { // Detect start of $(( ... )) if (processedCommand[i] === "$" && processedCommand[i + 1] === "(" && processedCommand[i + 2] === "(") { const start = i - // Track balanced parentheses depth. We saw "((" + // Track balanced parentheses depth with basic quote awareness. We saw "((" let depth = 2 + let inSingle = false + let inDouble = false i += 3 while (i < processedCommand.length && depth > 0) { const ch = processedCommand[i] - if (ch === "(") depth++ - else if (ch === ")") depth-- + if (ch === "'" && !inDouble) { + inSingle = !inSingle + } else if (ch === '"' && !inSingle) { + inDouble = !inDouble + } else if (!inSingle && !inDouble) { + if (ch === "(") depth++ + else if (ch === ")") depth-- + } i++ } // i currently points to the char AFTER the one that closed depth to 0 @@ -290,40 +302,64 @@ function parseCommandLine(command: string): string[] { processedCommand = out } - // Handle $[...] arithmetic expressions (alternative syntax) - processedCommand = processedCommand.replace(/\$\[[^\]]*\]/g, (match) => { - // Extract subshells inside $[ ... ] arithmetic expressions as well - // Skip arithmetic "$((" by ensuring char after "$(" is not "(" - match.replace(/\$\((?!\()(.*?)\)/g, (_m, inner) => { - const trimmed = String(inner).trim() - if (trimmed) { - subshells.push(trimmed) - const expanded = parseCommand(trimmed) - if (expanded.length > 0) { - embeddedSubshellCommands.push(...expanded) - } else { - embeddedSubshellCommands.push(trimmed) - } - } - return _m - }) - match.replace(/`((?:\\`|[^`])*)`/g, (_m, inner) => { - const unescaped = String(inner).replace(/\\`/g, "`").trim() - if (unescaped) { - subshells.push(unescaped) - const expanded = parseCommand(unescaped) - if (expanded.length > 0) { - embeddedSubshellCommands.push(...expanded) - } else { - embeddedSubshellCommands.push(unescaped) + // Handle $[...] arithmetic expressions (alternative syntax) with balanced scanning + { + let out = "" + for (let i = 0; i < processedCommand.length; i++) { + if (processedCommand[i] === "$" && processedCommand[i + 1] === "[") { + const start = i + i += 2 + let depth = 1 + let inSingle = false + let inDouble = false + while (i < processedCommand.length && depth > 0) { + const ch = processedCommand[i] + if (ch === "'" && !inDouble) inSingle = !inSingle + else if (ch === '"' && !inSingle) inDouble = !inDouble + else if (!inSingle && !inDouble) { + if (ch === "[") depth++ + else if (ch === "]") depth-- + } + i++ } - } - return _m - }) + const match = processedCommand.slice(start, i) + // Extract subshells inside $[ ... ] arithmetic expressions as well + match.replace(/\$\((?!\()(.*?)\)/g, (_m, inner) => { + const trimmed = String(inner).trim() + if (trimmed) { + subshells.push(trimmed) + const expanded = parseCommand(trimmed) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(trimmed) + } + } + return _m + }) + match.replace(/`((?:\\`|[^`])*)`/g, (_m, inner) => { + const unescaped = String(inner).replace(/\\`/g, "`").trim() + if (unescaped) { + subshells.push(unescaped) + const expanded = parseCommand(unescaped) + if (expanded.length > 0) { + embeddedSubshellCommands.push(...expanded) + } else { + embeddedSubshellCommands.push(unescaped) + } + } + return _m + }) - arithmeticExpressions.push(match) - return `__ARITH_${arithmeticExpressions.length - 1}__` - }) + arithmeticExpressions.push(match) + out += `__ARITH_${arithmeticExpressions.length - 1}__` + i -= 1 + } else { + out += processedCommand[i] + } + } + processedCommand = out + } // Handle parameter expansions: ${...} patterns (including array indexing) // This covers ${var}, ${var:-default}, ${var:+alt}, ${#var}, ${var%pattern}, etc. @@ -332,11 +368,40 @@ function parseCommandLine(command: string): string[] { return `__PARAM_${parameterExpansions.length - 1}__` }) - // Handle process substitutions: <(...) and >(...) - processedCommand = processedCommand.replace(/[<>]\(([^)]+)\)/g, (_, inner) => { - subshells.push(inner.trim()) - return `__SUBSH_${subshells.length - 1}__` - }) + // Handle process substitutions: <(...) and >(...) with balanced scanning + { + let out = "" + for (let i = 0; i < processedCommand.length; i++) { + if ((processedCommand[i] === "<" || processedCommand[i] === ">") && processedCommand[i + 1] === "(") { + const start = i + i += 2 + let depth = 1 + let inSingle = false + let inDouble = false + while (i < processedCommand.length && depth > 0) { + const ch = processedCommand[i] + if (ch === "'" && !inDouble) inSingle = !inSingle + else if (ch === '"' && !inSingle) inDouble = !inDouble + else if (!inSingle && !inDouble) { + if (ch === "(") depth++ + else if (ch === ")") depth-- + } + i++ + } + const inner = processedCommand.slice(start + 2, i - 1).trim() + if (inner) { + subshells.push(inner) + out += `__SUBSH_${subshells.length - 1}__` + } else { + out += processedCommand.slice(start, i) + } + i -= 1 + } else { + out += processedCommand[i] + } + } + processedCommand = out + } // Handle simple variable references: $varname pattern // This prevents shell-quote from splitting $count into separate tokens @@ -365,39 +430,79 @@ function parseCommandLine(command: string): string[] { return `__SUBSH_${subshells.length - 1}__` }) - // Handle fish-style command substitutions and POSIX subshell grouping: ( ... ) - // At this point we've already replaced $(), <() and >() earlier, so any remaining (...) are either - // fish command substitutions or subshell groupings. We treat them as subshells to validate inner commands. - processedCommand = processedCommand.replace(/\(([^()]*)\)/g, (full, inner: string, offset: number, str: string) => { - // If this was actually a pattern preceded by $, < or > it would have been handled earlier. - // Guard anyway: if the preceding character indicates a different construct, keep original. - const prevChar = offset > 0 ? str[offset - 1] : "" - // Also skip array initializers like name=(...) which follow '=' - if (prevChar === "$" || prevChar === "<" || prevChar === ">" || prevChar === "=") { - return full - } - const content = (inner || "").trim() - if (!content) return full - // Avoid creating nested placeholders like (__SUBSH_0__) -> __SUBSH_1__ - // Keep original when content is already a subshell placeholder - if (/^__SUBSH_\d+__$/.test(content)) { - return full - } - subshells.push(content) - return `__SUBSH_${subshells.length - 1}__` + // Mask quoted strings BEFORE handling fish-style parentheses to avoid false subshells inside quotes + processedCommand = processedCommand.replace(/"((?:\\.|[^"\\])*)"/g, (match) => { + quotes.push(match) + return `__QUOTE_${quotes.length - 1}__` }) - // Then handle quoted strings - processedCommand = processedCommand.replace(/"[^"]*"/g, (match) => { + // Also mask single-quoted strings before handling parentheses + processedCommand = processedCommand.replace(/'[^']*'/g, (match) => { quotes.push(match) return `__QUOTE_${quotes.length - 1}__` }) + // Handle fish-style command substitutions and POSIX subshell grouping: ( ... ) + // Use balanced scanning to support nested parentheses while respecting quotes. + // We already handled $(), <() and >() earlier, so remaining (...) are either fish substitutions or groupings. + { + let out = "" + for (let i = 0; i < processedCommand.length; i++) { + const ch = processedCommand[i] + if (ch === "(") { + const prevChar = i > 0 ? processedCommand[i - 1] : "" + // Skip constructs that were or will be handled elsewhere + if (prevChar === "$" || prevChar === "<" || prevChar === ">" || prevChar === "=") { + out += ch + continue + } + let j = i + 1 + let depth = 1 + let inSingle = false + let inDouble = false + while (j < processedCommand.length && depth > 0) { + const cj = processedCommand[j] + if (cj === "'" && !inDouble) inSingle = !inSingle + else if (cj === '"' && !inSingle) inDouble = !inDouble + else if (!inSingle && !inDouble) { + if (cj === "(") depth++ + else if (cj === ")") depth-- + } + j++ + } + if (depth === 0) { + const inner = processedCommand.slice(i + 1, j - 1).trim() + if (inner) { + // Avoid generating placeholder around an existing subshell placeholder + if (/^__SUBSH_\d+__$/.test(inner)) { + out += processedCommand.slice(i, j) + } else { + subshells.push(inner) + out += `__SUBSH_${subshells.length - 1}__` + } + } else { + // Empty grouping, keep as-is + out += processedCommand.slice(i, j) + } + i = j - 1 + } else { + // Unbalanced; keep the '(' and continue + out += ch + } + } else { + out += ch + } + } + processedCommand = out + } + let tokens: ShellToken[] try { tokens = parse(processedCommand) as ShellToken[] } catch (error: any) { // If shell-quote fails to parse, fall back to simple splitting - console.warn("shell-quote parse error:", error.message, "for command:", processedCommand) + if (typeof process !== "undefined" && process.env?.NODE_ENV !== "production") { + console.warn("shell-quote parse error:", error.message, "for command:", processedCommand) + } // Simple fallback: split by common operators const fallbackCommands = processedCommand @@ -662,6 +767,10 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" * // Returns "ask_user" * ``` * + * Ordering note: parseCommand appends subshell commands discovered inside arithmetic expressions + * to the end of the subCommands list. Since any denial results in an auto_deny decision, this + * ordering does not affect outcomes, but is documented here for clarity. + * * @param command - The full command string to validate * @param allowedCommands - List of allowed command prefixes * @param deniedCommands - Optional list of denied command prefixes From c9c43335895fd02da7e003fc23aa2240b3eb0606 Mon Sep 17 00:00:00 2001 From: John Richmond <5629+jr@users.noreply.github.com> Date: Mon, 29 Sep 2025 20:55:49 -0700 Subject: [PATCH 9/9] Potential fix for code scanning alert no. 189: Inefficient regular expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- webview-ui/src/utils/command-validation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 244c85c7cd..6596e4a853 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -127,7 +127,7 @@ export function containsDangerousSubstitution(source: string): boolean { // Check for $"..." string interpolation with command substitution // $"..." is a bash feature for translated strings that allows command substitution inside // Handle escaped quotes within $"...": use (?:\\.|[^"])* to avoid premature termination on \" - const bashTranslatedStringWithSubstitution = /\$"(?:\\.|[^"])*(?:\$\(|`)(?:\\.|[^"])*"/.test(source) + const bashTranslatedStringWithSubstitution = /\$"(?:\\.|[^\\"])*(?:\$\(|`)(?:\\.|[^\\"])*"/.test(source) // Return true if any dangerous pattern is detected return (