From 3daddacf044623cbeba2023f671e74fb4330841f Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Thu, 31 Jul 2025 10:48:28 -0400 Subject: [PATCH] Handle more variations of chaining and subshell --- .../__tests__/command-validation.spec.ts | 55 +++++++++++ webview-ui/src/utils/command-validation.ts | 94 +++++++++++++------ 2 files changed, 120 insertions(+), 29 deletions(-) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index f16fc00044..29370c471f 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -21,6 +21,14 @@ describe("Command Validation", () => { expect(parseCommand("npm test || npm run build")).toEqual(["npm test", "npm run build"]) expect(parseCommand("npm test; npm run build")).toEqual(["npm test", "npm run build"]) expect(parseCommand("npm test | npm run build")).toEqual(["npm test", "npm run build"]) + expect(parseCommand("npm test & npm run build")).toEqual(["npm test", "npm run build"]) + }) + + it("handles & operator for background execution", () => { + expect(parseCommand("ls & whoami")).toEqual(["ls", "whoami"]) + expect(parseCommand("ls & whoami & pwd")).toEqual(["ls", "whoami", "pwd"]) + expect(parseCommand("ls && whoami & pwd || echo done")).toEqual(["ls", "whoami", "pwd", "echo done"]) + expect(parseCommand("ls&whoami")).toEqual(["ls", "whoami"]) }) it("preserves quoted content", () => { @@ -48,6 +56,53 @@ describe("Command Validation", () => { expect(containsSubshell("echo hello")).toBe(false) // no subshells }) + it("detects subshell grouping patterns", () => { + // Basic subshell grouping with shell operators + expect(containsSubshell("(ls; rm file)")).toBe(true) + expect(containsSubshell("(cd /tmp && rm -rf *)")).toBe(true) + expect(containsSubshell("(command1 || command2)")).toBe(true) + expect(containsSubshell("(ls | grep test)")).toBe(true) + expect(containsSubshell("(sleep 10 & echo done)")).toBe(true) + + // Nested subshells + expect(containsSubshell("(cd /tmp && (rm -rf * || echo failed))")).toBe(true) + + // Multiple operators in subshell + expect(containsSubshell("(cmd1; cmd2 && cmd3 | cmd4)")).toBe(true) + + // Subshell with spaces + expect(containsSubshell("( ls ; rm file )")).toBe(true) + }) + + it("does NOT detect legitimate parentheses usage", () => { + // Function calls should not be flagged as subshells + expect(containsSubshell("myfunction(arg1, arg2)")).toBe(false) + expect(containsSubshell("func( arg1, arg2 )")).toBe(false) + + // Simple parentheses without operators + expect(containsSubshell("(simple text)")).toBe(false) + + // Parentheses in strings + expect(containsSubshell('echo "this (has) parentheses"')).toBe(false) + + // Empty parentheses + expect(containsSubshell("()")).toBe(false) + }) + + it("handles mixed subshell patterns", () => { + // Mixed subshell types + expect(containsSubshell("(echo $(date); rm file)")).toBe(true) + + // Subshell with command substitution + expect(containsSubshell("(ls `pwd`; echo done)")).toBe(true) + + // No subshells + expect(containsSubshell("echo hello world")).toBe(false) + + // Empty string + expect(containsSubshell("")).toBe(false) + }) + it("handles empty and whitespace input", () => { expect(parseCommand("")).toEqual([]) expect(parseCommand(" ")).toEqual([]) diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index f1d0c211cb..700aed554b 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -36,18 +36,18 @@ type ShellToken = string | { op: string } | { command: string } * * ## Command Processing Pipeline: * - * 1. **Subshell Detection**: Commands containing $() or `` are blocked if denylist exists - * 2. **Command Parsing**: Split chained commands (&&, ||, ;, |) into individual commands - * 3. **Pattern Matching**: For each command, find longest matching prefixes in both lists - * 4. **Decision Logic**: Apply longest prefix match rule to determine approval/denial - * 5. **Aggregation**: Combine decisions (any denial blocks the entire command chain) + * 1. **Subshell Detection**: Commands containing dangerous patterns like $(), ``, or (cmd1; cmd2) are flagged as security risks + * 2. **Command Parsing**: Split chained commands (&&, ||, ;, |, &) into individual commands for separate validation + * 3. **Pattern Matching**: For each individual command, find the longest matching prefix in both allowlist and denylist + * 4. **Decision Logic**: Apply longest prefix match rule - more specific (longer) matches take precedence + * 5. **Aggregation**: Combine individual decisions - if any command is denied, the entire chain is denied * * ## Security Considerations: * - * - **Subshell Protection**: Prevents command injection via $(command), `command`, or process substitution - * - **Chain Analysis**: Each command in a chain (cmd1 && cmd2) is validated separately - * - **Case Insensitive**: All matching is case-insensitive for consistency - * - **Whitespace Handling**: Commands are trimmed and normalized before matching + * - **Subshell Protection**: Detects and blocks command injection attempts via command substitution, process substitution, and subshell grouping + * - **Chain Analysis**: Each command in a chain (cmd1 && cmd2) is validated separately to prevent bypassing via chaining + * - **Case Insensitive**: All pattern matching is case-insensitive for consistent behavior across different input styles + * - **Whitespace Handling**: Commands are trimmed and normalized before matching to prevent whitespace-based bypasses * * ## Configuration Merging: * @@ -59,37 +59,73 @@ type ShellToken = string | { op: string } | { command: string } */ /** - * Detect subshell usage and command substitution patterns: - * - $() - command substitution - * - `` - backticks (legacy command substitution) - * - <() - process substitution (input) - * - >() - process substitution (output) - * - $(()) - arithmetic expansion - * - $[] - arithmetic expansion (alternative syntax) + * Detect subshell usage and command substitution patterns that could be security risks. + * + * Subshells allow executing commands in isolated environments and can be used to bypass + * command validation by hiding dangerous commands inside substitution patterns. + * + * Detected patterns: + * - $() - command substitution: executes command and substitutes output + * - `` - backticks (legacy command substitution): same as $() but older syntax + * - <() - process substitution (input): creates temporary file descriptor for command output + * - >() - process substitution (output): creates temporary file descriptor for command input + * - $(()) - arithmetic expansion: evaluates mathematical expressions (can contain commands) + * - $[] - arithmetic expansion (alternative syntax): same as $(()) but older syntax + * - (cmd1; cmd2) - subshell grouping: executes multiple commands in isolated subshell + * + * @param source - The command string to analyze for subshell patterns + * @returns true if any subshell patterns are detected, false otherwise * * @example * ```typescript - * containsSubshell("echo $(date)") // true - command substitution - * containsSubshell("echo `date`") // true - backtick substitution - * containsSubshell("diff <(sort f1)") // true - process substitution - * containsSubshell("echo $((1+2))") // true - arithmetic expansion - * containsSubshell("echo $[1+2]") // true - arithmetic expansion (alt) - * containsSubshell("echo hello") // false - no subshells + * // Command substitution - executes 'date' and substitutes its output + * containsSubshell("echo $(date)") // true + * + * // Backtick substitution - legacy syntax for command substitution + * containsSubshell("echo `date`") // true + * + * // Process substitution - creates file descriptor for command output + * containsSubshell("diff <(sort f1)") // true + * + * // Arithmetic expansion - can contain command execution + * containsSubshell("echo $((1+2))") // true + * containsSubshell("echo $[1+2]") // true + * + * // Subshell grouping - executes commands in isolated environment + * containsSubshell("(ls; rm file)") // true + * containsSubshell("(cd /tmp && rm -rf *)") // true + * + * // Safe patterns that should NOT be flagged + * containsSubshell("func(arg1, arg2)") // false - function call, not subshell + * containsSubshell("echo hello") // false - no subshell patterns + * containsSubshell("(simple text)") // false - no shell operators in parentheses * ``` */ export function containsSubshell(source: string): boolean { - return /(\$\()|`|(<\(|>\()|(\$\(\()|(\$\[)/.test(source) + // Check for command substitution, process substitution, and arithmetic expansion patterns + // These patterns allow executing commands and substituting their output, which can bypass validation + const commandSubstitutionPatterns = /(\$\()|`|(<\(|>\()|(\$\(\()|(\$\[)/.test(source) + + // Check for subshell grouping: parentheses containing shell command operators + // Pattern explanation: \( = literal opening paren, [^)]* = any chars except closing paren, + // [;&|]+ = one or more shell operators (semicolon, ampersand, pipe), [^)]* = any chars except closing paren, \) = literal closing paren + // This detects dangerous patterns like: (cmd1; cmd2), (cmd1 && cmd2), (cmd1 || cmd2), (cmd1 | cmd2), (cmd1 & cmd2) + // But avoids false positives like function calls: func(arg1, arg2) - no shell operators inside + const subshellGroupingPattern = /\([^)]*[;&|]+[^)]*\)/.test(source) + + // Return true if any subshell pattern is detected + return commandSubstitutionPatterns || subshellGroupingPattern } /** * Split a command string into individual sub-commands by - * chaining operators (&&, ||, ;, or |) and newlines. + * chaining operators (&&, ||, ;, |, or &) and newlines. * * Uses shell-quote to properly handle: * - Quoted strings (preserves quotes) * - Subshell commands ($(cmd), `cmd`, <(cmd), >(cmd)) * - PowerShell redirections (2>&1) - * - Chain operators (&&, ||, ;, |) + * - Chain operators (&&, ||, ;, |, &) * - Newlines as command separators */ export function parseCommand(command: string): string[] { @@ -228,7 +264,7 @@ function parseCommandLine(command: string): string[] { // Simple fallback: split by common operators const fallbackCommands = processedCommand - .split(/(?:&&|\|\||;|\|)/) + .split(/(?:&&|\|\||;|\||&)/) .map((cmd) => cmd.trim()) .filter((cmd) => cmd.length > 0) @@ -253,13 +289,13 @@ function parseCommandLine(command: string): string[] { for (const token of tokens) { if (typeof token === "object" && "op" in token) { // Chain operator - split command - if (["&&", "||", ";", "|"].includes(token.op)) { + if (["&&", "||", ";", "|", "&"].includes(token.op)) { if (currentCommand.length > 0) { commands.push(currentCommand.join(" ")) currentCommand = [] } } else { - // Other operators (>, &) are part of the command + // Other operators (>) are part of the command currentCommand.push(token.op) } } else if (typeof token === "string") { @@ -436,7 +472,7 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user" * * **Decision Logic:** * 1. **Subshell Protection**: If subshells ($() or ``) are present and denylist exists → auto-deny - * 2. **Command Parsing**: Split command chains (&&, ||, ;, |) into individual commands + * 2. **Command Parsing**: Split command chains (&&, ||, ;, |, &) into individual commands * 3. **Individual Validation**: For each sub-command, apply longest prefix match rule * 4. **Aggregation**: Combine decisions using "any denial blocks all" principle *