Improvements to subshell validation (RooCodeInc#6379)

Author: mrubens

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<CommandPattern[]>(() => {
-		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<string>()
+
+		// 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])

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", () => {

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,