Improvements to subshell validation

Author: mrubens

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,19 @@ 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 $[] pattern
+			expect(containsSubshell("echo $[1 + 2]")).toBe(true)
+			expect(containsSubshell("echo $(date)")).toBe(true)
+			expect(containsSubshell("echo `date`")).toBe(true)
+			expect(containsSubshell("diff <(sort f1) <(sort f2)")).toBe(true)
+			expect(containsSubshell("echo hello")).toBe(false)
 		})
 
 		it("handles empty and whitespace input", () => {
@@ -629,7 +643,6 @@ echo "Successfully converted $count .jsx files to .tsx"`
 		})
 	})
 })
-
 describe("Unified Command Decision Functions", () => {
 	describe("getSingleCommandDecision", () => {
 		const allowedCommands = ["npm", "echo", "git"]
@@ -712,8 +725,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 +740,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 +857,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", () => {
@@ -957,7 +970,7 @@ describe("Unified Command Decision Functions", () => {
 
 				// Nested subshells - inner commands are extracted and not in allowlist
 				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 results in mixed decisions
 			})
 
 			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,26 @@ 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)
+ */
+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
@@ -103,7 +116,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 +130,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 +144,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 +162,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())
@@ -187,10 +205,9 @@ function parseCommandLine(command: string): string[] {
 			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)])
+			result = result.replace(/__SUBSH_(\d+)__/g, (_, i) => subshells[parseInt(i)])
 			return result
 		})
 	}
@@ -243,10 +260,9 @@ function parseCommandLine(command: string): string[] {
 		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)])
+		result = result.replace(/__SUBSH_(\d+)__/g, (_, i) => subshells[parseInt(i)])
 		return result
 	})
 }
@@ -430,14 +446,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 +618,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,