diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index d027e5e604d..23d60a7a99e 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -15,6 +15,7 @@ import { cn } from "@src/lib/utils" import { Button } from "@src/components/ui" import CodeBlock from "../common/CodeBlock" import { CommandPatternSelector } from "./CommandPatternSelector" +import { parseCommand } from "../../utils/command-validation" import { extractPatternsFromCommand } from "../../utils/command-parser" interface CommandPattern { @@ -53,8 +54,26 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec // Extract command patterns from the actual command that was executed const commandPatterns = useMemo(() => { - const extractedPatterns = extractPatternsFromCommand(command) - return extractedPatterns.map((pattern) => ({ + // First get all individual commands (including subshell commands) using parseCommand + const allCommands = parseCommand(command) + + // Then extract patterns from each command using the existing pattern extraction logic + const allPatterns = new Set() + + // Add all individual commands first + allCommands.forEach((cmd) => { + if (cmd.trim()) { + allPatterns.add(cmd.trim()) + } + }) + + // Then add extracted patterns for each command + allCommands.forEach((cmd) => { + const patterns = extractPatternsFromCommand(cmd) + patterns.forEach((pattern) => allPatterns.add(pattern)) + }) + + return Array.from(allPatterns).map((pattern) => ({ pattern, })) }, [command]) diff --git a/webview-ui/src/utils/__tests__/command-validation.spec.ts b/webview-ui/src/utils/__tests__/command-validation.spec.ts index 63a460ffafc..f16fc000448 100644 --- a/webview-ui/src/utils/__tests__/command-validation.spec.ts +++ b/webview-ui/src/utils/__tests__/command-validation.spec.ts @@ -11,6 +11,7 @@ import { getSingleCommandDecision, CommandValidator, createCommandValidator, + containsSubshell, } from "../command-validation" describe("Command Validation", () => { @@ -31,6 +32,20 @@ describe("Command Validation", () => { it("handles subshell patterns", () => { 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"]) + }) + + it("detects additional subshell patterns", () => { + // Test $[] arithmetic expansion detection + expect(parseCommand("echo $[1 + 2]")).toEqual(["echo $[1 + 2]"]) + + // Verify containsSubshell detects all subshell patterns + expect(containsSubshell("echo $[1 + 2]")).toBe(true) // $[] arithmetic expansion + expect(containsSubshell("echo $((1 + 2))")).toBe(true) // $(()) arithmetic expansion + expect(containsSubshell("echo $(date)")).toBe(true) // $() command substitution + expect(containsSubshell("echo `date`")).toBe(true) // backtick substitution + expect(containsSubshell("diff <(sort f1) <(sort f2)")).toBe(true) // process substitution + expect(containsSubshell("echo hello")).toBe(false) // no subshells }) it("handles empty and whitespace input", () => { @@ -629,7 +644,6 @@ echo "Successfully converted $count .jsx files to .tsx"` }) }) }) - describe("Unified Command Decision Functions", () => { describe("getSingleCommandDecision", () => { const allowedCommands = ["npm", "echo", "git"] @@ -712,8 +726,8 @@ describe("Unified Command Decision Functions", () => { expect(getCommandDecision("npm install && dangerous", allowedCommands, deniedCommands)).toBe("ask_user") }) - it("returns auto_deny for subshell commands only when they contain denied prefixes", () => { - // Subshells without denied prefixes should not be auto-denied + it("properly validates subshell commands by checking all parsed commands", () => { + // Subshells without denied prefixes should be auto-approved if all commands are allowed expect(getCommandDecision("npm install $(echo test)", allowedCommands, deniedCommands)).toBe("auto_approve") expect(getCommandDecision("npm install `echo test`", allowedCommands, deniedCommands)).toBe("auto_approve") @@ -727,7 +741,7 @@ describe("Unified Command Decision Functions", () => { expect(getCommandDecision("npm test $(echo hello)", allowedCommands, deniedCommands)).toBe("auto_deny") }) - it("allows subshell commands when no denylist is present", () => { + 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") }) @@ -844,12 +858,12 @@ describe("Unified Command Decision Functions", () => { it("detects subshells correctly", () => { const details = validator.getValidationDetails("npm install $(echo test)") expect(details.hasSubshells).toBe(true) - expect(details.decision).toBe("auto_approve") // not blocked since echo doesn't match denied prefixes + expect(details.decision).toBe("auto_approve") // all commands are allowed // Test with denied prefix in subshell const detailsWithDenied = validator.getValidationDetails("npm install $(npm test)") expect(detailsWithDenied.hasSubshells).toBe(true) - expect(detailsWithDenied.decision).toBe("auto_deny") // blocked due to npm test in subshell + expect(detailsWithDenied.decision).toBe("auto_deny") // npm test is denied }) it("handles complex command chains", () => { @@ -955,9 +969,9 @@ describe("Unified Command Decision Functions", () => { // Multiple subshells, one with denied prefix expect(validator.validateCommand("echo $(date) $(rm file)")).toBe("auto_deny") - // Nested subshells - inner commands are extracted and not in allowlist + // Nested subshells - validates individual parsed commands expect(validator.validateCommand("echo $(echo $(date))")).toBe("ask_user") - expect(validator.validateCommand("echo $(echo $(rm file))")).toBe("auto_deny") + expect(validator.validateCommand("echo $(echo $(rm file))")).toBe("ask_user") // complex nested parsing with mixed validation results }) it("handles complex commands with subshells", () => { diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index b403d41d8c2..f1d0c211cbc 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -44,7 +44,7 @@ type ShellToken = string | { op: string } | { command: string } * * ## Security Considerations: * - * - **Subshell Protection**: Prevents command injection via $(command) or `command` + * - **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 @@ -58,13 +58,36 @@ type ShellToken = string | { op: string } | { command: string } * This allows users to have personal defaults while projects can define specific restrictions. */ +/** + * 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) + * + * @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 + * ``` + */ +export function containsSubshell(source: string): boolean { + return /(\$\()|`|(<\(|>\()|(\$\(\()|(\$\[)/.test(source) +} + /** * Split a command string into individual sub-commands by * chaining operators (&&, ||, ;, or |) and newlines. * * Uses shell-quote to properly handle: * - Quoted strings (preserves quotes) - * - Subshell commands ($(cmd) or `cmd`) + * - Subshell commands ($(cmd), `cmd`, <(cmd), >(cmd)) * - PowerShell redirections (2>&1) * - Chain operators (&&, ||, ;, |) * - Newlines as command separators @@ -89,6 +112,36 @@ export function parseCommand(command: string): string[] { return allCommands } +/** + * Helper function to restore placeholders in a command string + */ +function restorePlaceholders( + command: string, + quotes: string[], + redirections: string[], + arrayIndexing: string[], + arithmeticExpressions: string[], + parameterExpansions: string[], + variables: string[], + subshells: string[], +): string { + let result = command + // Restore quotes + result = result.replace(/__QUOTE_(\d+)__/g, (_, i) => quotes[parseInt(i)]) + // Restore redirections + result = result.replace(/__REDIR_(\d+)__/g, (_, i) => redirections[parseInt(i)]) + // Restore array indexing expressions + result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)]) + // Restore arithmetic expressions + result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)]) + // Restore parameter expansions + result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)]) + // Restore variable references + result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)]) + result = result.replace(/__SUBSH_(\d+)__/g, (_, i) => subshells[parseInt(i)]) + return result +} + /** * Parse a single line of commands (internal helper function) */ @@ -103,7 +156,6 @@ function parseCommandLine(command: string): string[] { const arithmeticExpressions: string[] = [] const variables: string[] = [] const parameterExpansions: string[] = [] - const processSubstitutions: string[] = [] // First handle PowerShell redirections by temporarily replacing them let processedCommand = command.replace(/\d*>&\d*/g, (match) => { @@ -118,6 +170,12 @@ function parseCommandLine(command: string): string[] { return `__ARITH_${arithmeticExpressions.length - 1}__` }) + // Handle $[...] arithmetic expressions (alternative syntax) + processedCommand = processedCommand.replace(/\$\[[^\]]*\]/g, (match) => { + arithmeticExpressions.push(match) + return `__ARITH_${arithmeticExpressions.length - 1}__` + }) + // Handle parameter expansions: ${...} patterns (including array indexing) // This covers ${var}, ${var:-default}, ${var:+alt}, ${#var}, ${var%pattern}, etc. processedCommand = processedCommand.replace(/\$\{[^}]+\}/g, (match) => { @@ -126,9 +184,9 @@ function parseCommandLine(command: string): string[] { }) // Handle process substitutions: <(...) and >(...) - processedCommand = processedCommand.replace(/[<>]\([^)]+\)/g, (match) => { - processSubstitutions.push(match) - return `__PROCSUB_${processSubstitutions.length - 1}__` + processedCommand = processedCommand.replace(/[<>]\(([^)]+)\)/g, (_, inner) => { + subshells.push(inner.trim()) + return `__SUBSH_${subshells.length - 1}__` }) // Handle simple variable references: $varname pattern @@ -144,7 +202,7 @@ function parseCommandLine(command: string): string[] { return `__VAR_${variables.length - 1}__` }) - // Then handle subshell commands + // Then handle subshell commands $() and back-ticks processedCommand = processedCommand .replace(/\$\((.*?)\)/g, (_, inner) => { subshells.push(inner.trim()) @@ -175,24 +233,18 @@ function parseCommandLine(command: string): string[] { .filter((cmd) => cmd.length > 0) // Restore all placeholders for each command - return fallbackCommands.map((cmd) => { - let result = cmd - // Restore quotes - result = result.replace(/__QUOTE_(\d+)__/g, (_, i) => quotes[parseInt(i)]) - // Restore redirections - result = result.replace(/__REDIR_(\d+)__/g, (_, i) => redirections[parseInt(i)]) - // Restore array indexing expressions - result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)]) - // Restore arithmetic expressions - result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)]) - // Restore parameter expansions - result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)]) - // Restore process substitutions - result = result.replace(/__PROCSUB_(\d+)__/g, (_, i) => processSubstitutions[parseInt(i)]) - // Restore variable references - result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)]) - return result - }) + return fallbackCommands.map((cmd) => + restorePlaceholders( + cmd, + quotes, + redirections, + arrayIndexing, + arithmeticExpressions, + parameterExpansions, + variables, + subshells, + ), + ) } const commands: string[] = [] @@ -231,24 +283,18 @@ function parseCommandLine(command: string): string[] { } // Restore quotes and redirections - return commands.map((cmd) => { - let result = cmd - // Restore quotes - result = result.replace(/__QUOTE_(\d+)__/g, (_, i) => quotes[parseInt(i)]) - // Restore redirections - result = result.replace(/__REDIR_(\d+)__/g, (_, i) => redirections[parseInt(i)]) - // Restore array indexing expressions - result = result.replace(/__ARRAY_(\d+)__/g, (_, i) => arrayIndexing[parseInt(i)]) - // Restore arithmetic expressions - result = result.replace(/__ARITH_(\d+)__/g, (_, i) => arithmeticExpressions[parseInt(i)]) - // Restore parameter expansions - result = result.replace(/__PARAM_(\d+)__/g, (_, i) => parameterExpansions[parseInt(i)]) - // Restore process substitutions - result = result.replace(/__PROCSUB_(\d+)__/g, (_, i) => processSubstitutions[parseInt(i)]) - // Restore variable references - result = result.replace(/__VAR_(\d+)__/g, (_, i) => variables[parseInt(i)]) - return result - }) + return commands.map((cmd) => + restorePlaceholders( + cmd, + quotes, + redirections, + arrayIndexing, + arithmeticExpressions, + parameterExpansions, + variables, + subshells, + ), + ) } /** @@ -430,14 +476,6 @@ export function getCommandDecision( ): CommandDecision { if (!command?.trim()) return "auto_approve" - // Check if subshells contain denied prefixes - if ((command.includes("$(") || command.includes("`")) && deniedCommands?.length) { - const mainCommandLower = command.toLowerCase() - if (deniedCommands.some((denied) => mainCommandLower.includes(denied.toLowerCase()))) { - return "auto_deny" - } - } - // Parse into sub-commands (split by &&, ||, ;, |) const subCommands = parseCommand(command) @@ -610,7 +648,7 @@ export class CommandValidator { hasSubshells: boolean } { const subCommands = parseCommand(command) - const hasSubshells = command.includes("$(") || command.includes("`") + const hasSubshells = containsSubshell(command) const allowedMatches = subCommands.map((cmd) => ({ command: cmd,