Handle substitution patterns in command validation (#7390)

webview-ui/src/utils/__tests__/command-validation.spec.ts

@@ -11,7 +11,7 @@ import {
 	getSingleCommandDecision,
 	CommandValidator,
 	createCommandValidator,
-	containsSubshell,
+	containsDangerousSubstitution,
 } from "../command-validation"
 
 describe("Command Validation", () => {
@@ -43,66 +43,6 @@ describe("Command Validation", () => {
 			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("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([])
@@ -261,6 +201,120 @@ ls -la || echo "Failed"`
 			expect(isAutoApprovedSingleCommand("npm test", [])).toBe(false)
 		})
 	})
+
+	describe("containsDangerousSubstitution", () => {
+		it("detects parameter expansion with @P operator (prompt string expansion)", () => {
+			// This is the specific vulnerability from the report - @P can execute commands
+			expect(containsDangerousSubstitution('echo "${var1=aa\\140whoami\\140c}${var1@P}"')).toBe(true)
+			expect(containsDangerousSubstitution("echo ${var@P}")).toBe(true)
+			expect(containsDangerousSubstitution("result=${input@P}")).toBe(true)
+			expect(containsDangerousSubstitution("${somevar@P}")).toBe(true)
+		})
+
+		it("detects other dangerous parameter expansion operators", () => {
+			// @Q - Quote removal
+			expect(containsDangerousSubstitution("echo ${var@Q}")).toBe(true)
+			// @E - Escape sequence expansion
+			expect(containsDangerousSubstitution("echo ${var@E}")).toBe(true)
+			// @A - Assignment statement
+			expect(containsDangerousSubstitution("echo ${var@A}")).toBe(true)
+			// @a - Attribute flags
+			expect(containsDangerousSubstitution("echo ${var@a}")).toBe(true)
+		})
+
+		it("detects parameter assignments with octal escape sequences", () => {
+			// Octal \140 is backtick, which can execute commands
+			expect(containsDangerousSubstitution('echo "${var=\\140whoami\\140}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:=\\140ls\\140}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var+\\140pwd\\140}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:-\\140date\\140}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:+\\140echo test\\140}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:?\\140rm file\\140}"')).toBe(true)
+			// Test various octal patterns
+			expect(containsDangerousSubstitution('echo "${var=\\001\\140\\141}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var=\\777}"')).toBe(true)
+		})
+
+		it("detects parameter assignments with hex escape sequences", () => {
+			// Hex \x60 is backtick
+			expect(containsDangerousSubstitution('echo "${var=\\x60whoami\\x60}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:=\\x60ls\\x60}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var+\\x60pwd\\x60}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:-\\x60date\\x60}"')).toBe(true)
+			// Test various hex patterns
+			expect(containsDangerousSubstitution('echo "${var=\\x00\\x60\\x61}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var=\\xFF}"')).toBe(true)
+		})
+
+		it("detects parameter assignments with unicode escape sequences", () => {
+			// Unicode \u0060 is backtick
+			expect(containsDangerousSubstitution('echo "${var=\\u0060whoami\\u0060}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:=\\u0060ls\\u0060}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var+\\u0060pwd\\u0060}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var:-\\u0060date\\u0060}"')).toBe(true)
+			// Test various unicode patterns
+			expect(containsDangerousSubstitution('echo "${var=\\u0000\\u0060\\u0061}"')).toBe(true)
+			expect(containsDangerousSubstitution('echo "${var=\\uFFFF}"')).toBe(true)
+		})
+
+		it("detects indirect variable references", () => {
+			// ${!var} performs indirect expansion which can be dangerous
+			expect(containsDangerousSubstitution("echo ${!var}")).toBe(true)
+			expect(containsDangerousSubstitution("result=${!indirect}")).toBe(true)
+			expect(containsDangerousSubstitution("${!prefix*}")).toBe(true)
+			expect(containsDangerousSubstitution("${!prefix@}")).toBe(true)
+		})
+
+		it("detects here-strings with command substitution", () => {
+			expect(containsDangerousSubstitution("cat <<<$(whoami)")).toBe(true)
+			expect(containsDangerousSubstitution("read <<<`date`")).toBe(true)
+			expect(containsDangerousSubstitution("grep pattern <<< $(ls)")).toBe(true)
+			expect(containsDangerousSubstitution("sort <<< `pwd`")).toBe(true)
+		})
+
+		it("does NOT flag safe parameter expansions", () => {
+			// Regular parameter expansions without dangerous operators
+			expect(containsDangerousSubstitution("echo ${var}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var:-default}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var:+alternative}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${#var}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var%pattern}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var#pattern}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var/old/new}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var^^}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var,,}")).toBe(false)
+			expect(containsDangerousSubstitution("echo ${var:0:5}")).toBe(false)
+
+			// Parameter assignments without escape sequences
+			expect(containsDangerousSubstitution('echo "${var=normal text}"')).toBe(false)
+			expect(containsDangerousSubstitution('echo "${var:-default value}"')).toBe(false)
+			expect(containsDangerousSubstitution('echo "${var:+alternative}"')).toBe(false)
+
+			// Here-strings without command substitution
+			expect(containsDangerousSubstitution("cat <<<plain_text")).toBe(false)
+			expect(containsDangerousSubstitution('read <<<"static string"')).toBe(false)
+			expect(containsDangerousSubstitution("grep <<<$var")).toBe(false) // Plain variable, not command substitution
+		})
+
+		it("handles complex combinations of dangerous patterns", () => {
+			// Multiple dangerous patterns in one command
+			expect(containsDangerousSubstitution('echo "${var1=\\140ls\\140}${var1@P}" && ${!indirect}')).toBe(true)
+			// Nested patterns
+			expect(containsDangerousSubstitution('echo "${outer=${inner@P}}"')).toBe(true)
+			// Mixed with safe patterns
+			expect(containsDangerousSubstitution("echo ${safe:-default} ${dangerous@P}")).toBe(true)
+		})
+
+		it("detects the exact exploit from the security report", () => {
+			// The exact pattern reported in the vulnerability
+			const exploit = 'echo "${var1=aa\\140whoami\\140c}${var1@P}"'
+			expect(containsDangerousSubstitution(exploit)).toBe(true)
+
+			// Variations of the exploit
+			expect(containsDangerousSubstitution('echo "${x=\\140id\\140}${x@P}"')).toBe(true)
+			expect(containsDangerousSubstitution('result="${cmd=\\x60pwd\\x60}${cmd@P}"')).toBe(true)
+		})
+	})
 })
 
 /**
@@ -771,6 +825,97 @@ describe("Unified Command Decision Functions", () => {
 			expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands)).toBe("auto_approve")
 		})
 
+		describe("dangerous substitution handling", () => {
+			it("prevents auto-approve for commands with dangerous parameter expansion", () => {
+				// Commands that would normally be auto-approved are blocked by dangerous patterns
+				expect(getCommandDecision("echo ${var@P}", allowedCommands, deniedCommands)).toBe("ask_user")
+				expect(getCommandDecision("echo hello", allowedCommands, deniedCommands)).toBe("auto_approve") // Safe version
+
+				// Even with allowed prefix, dangerous patterns prevent auto-approval
+				expect(getCommandDecision("npm install ${var@P}", allowedCommands, deniedCommands)).toBe("ask_user")
+				expect(
+					getCommandDecision('echo "${var1=\\140whoami\\140c}${var1@P}"', allowedCommands, deniedCommands),
+				).toBe("ask_user")
+			})
+
+			it("does NOT override auto_deny decisions with dangerous patterns", () => {
+				// If a command would be denied, dangerous patterns don't change that
+				expect(getCommandDecision("npm test ${var@P}", allowedCommands, deniedCommands)).toBe("auto_deny")
+				expect(getCommandDecision('npm test "${var=\\140ls\\140}"', allowedCommands, deniedCommands)).toBe(
+					"auto_deny",
+				)
+
+				// Regular denied commands without dangerous patterns
+				expect(getCommandDecision("npm test --coverage", allowedCommands, deniedCommands)).toBe("auto_deny")
+			})
+
+			it("prevents auto-approval for various dangerous substitution types", () => {
+				// Octal escape sequences
+				expect(getCommandDecision('echo "${var=\\140ls\\140}"', allowedCommands, deniedCommands)).toBe(
+					"ask_user",
+				)
+				expect(getCommandDecision('npm run "${var:=\\140pwd\\140}"', allowedCommands, deniedCommands)).toBe(
+					"ask_user",
+				)
+
+				// Hex escape sequences
+				expect(getCommandDecision('echo "${var=\\x60whoami\\x60}"', allowedCommands, deniedCommands)).toBe(
+					"ask_user",
+				)
+
+				// Indirect variable references
+				expect(getCommandDecision("echo ${!var}", allowedCommands, deniedCommands)).toBe("ask_user")
+
+				// Here-strings with command substitution
+				expect(getCommandDecision("cat <<<$(whoami)", allowedCommands, deniedCommands)).toBe("ask_user")
+				expect(getCommandDecision("read <<<`date`", allowedCommands, deniedCommands)).toBe("ask_user")
+			})
+
+			it("allows safe parameter expansions to follow normal rules", () => {
+				// Safe parameter expansions should follow normal allowlist/denylist rules
+				expect(getCommandDecision("echo ${var}", allowedCommands, deniedCommands)).toBe("auto_approve")
+				expect(getCommandDecision("echo ${var:-default}", allowedCommands, deniedCommands)).toBe("auto_approve")
+				expect(getCommandDecision("npm install ${package_name}", allowedCommands, deniedCommands)).toBe(
+					"auto_approve",
+				)
+
+				// Here-strings without command substitution are safe
+				expect(getCommandDecision("echo test <<<$var", allowedCommands, deniedCommands)).toBe("auto_approve")
+			})
+
+			it("handles command chains correctly with dangerous patterns", () => {
+				// If any part of a chain has dangerous substitution, prevent auto-approval
+				expect(getCommandDecision("npm install && echo ${var@P}", allowedCommands, deniedCommands)).toBe(
+					"ask_user",
+				)
+				expect(
+					getCommandDecision('echo safe && echo "${var=\\140ls\\140}"', allowedCommands, deniedCommands),
+				).toBe("ask_user")
+
+				// But if chain would be denied, keep the deny decision
+				expect(getCommandDecision("npm test ${var@P} && echo safe", allowedCommands, deniedCommands)).toBe(
+					"auto_deny",
+				)
+				expect(getCommandDecision("npm install && npm test ${var@P}", allowedCommands, deniedCommands)).toBe(
+					"auto_deny",
+				)
+
+				// Safe chains should still be auto-approved
+				expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands)).toBe(
+					"auto_approve",
+				)
+			})
+
+			it("handles the exact exploit from the security report", () => {
+				const exploit = 'echo "${var1=aa\\140whoami\\140c}${var1@P}"'
+				// Even though 'echo' is in the allowlist, the dangerous pattern prevents auto-approval
+				expect(getCommandDecision(exploit, allowedCommands, deniedCommands)).toBe("ask_user")
+
+				// But if it were a denied command, it would still be denied
+				expect(getCommandDecision(`npm test ${exploit}`, allowedCommands, deniedCommands)).toBe("auto_deny")
+			})
+		})
+
 		it("returns auto_deny for commands with any sub-command auto-denied", () => {
 			expect(getCommandDecision("npm test", allowedCommands, deniedCommands)).toBe("auto_deny")
 			expect(getCommandDecision("npm install && npm test", allowedCommands, deniedCommands)).toBe("auto_deny")
@@ -899,7 +1044,6 @@ describe("Unified Command Decision Functions", () => {
 
 					expect(details.decision).toBe("auto_approve")
 					expect(details.subCommands).toEqual(["npm install", "echo done"])
-					expect(details.hasSubshells).toBe(false)
 					expect(details.allowedMatches).toHaveLength(2)
 					expect(details.deniedMatches).toHaveLength(2)
 
@@ -912,12 +1056,10 @@ 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") // 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") // npm test is denied
 				})
 

webview-ui/src/utils/command-validation.ts

@@ -36,7 +36,7 @@ type ShellToken = string | { op: string } | { command: string }
  *
  * ## Command Processing Pipeline:
  *
- * 1. **Subshell Detection**: Commands containing dangerous patterns like $(), ``, or (cmd1; cmd2) are flagged as security risks
+ * 1. **Dangerous Substitution Detection**: Commands containing dangerous patterns like ${var@P} are never auto-approved
  * 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
@@ -44,7 +44,7 @@ type ShellToken = string | { op: string } | { command: string }
  *
  * ## Security Considerations:
  *
- * - **Subshell Protection**: Detects and blocks command injection attempts via command substitution, process substitution, and subshell grouping
+ * - **Dangerous Substitution Protection**: Detects dangerous parameter expansions and escape sequences that could execute commands
  * - **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
@@ -59,62 +59,51 @@ type ShellToken = string | { op: string } | { command: string }
  */
 
 /**
- * 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.
+ * Detect dangerous parameter substitutions that could lead to command execution.
+ * These patterns are never auto-approved and always require explicit user approval.
  *
  * 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
- * // 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
- * ```
+ * - ${var@P} - Prompt string expansion (interprets escape sequences and executes embedded commands)
+ * - ${var@Q} - Quote removal
+ * - ${var@E} - Escape sequence expansion
+ * - ${var@A} - Assignment statement
+ * - ${var@a} - Attribute flags
+ * - ${var=value} with escape sequences - Can embed commands via \140 (backtick), \x60, or \u0060
+ * - ${!var} - Indirect variable references
+ * - <<<$(...) or <<<`...` - Here-strings with command substitution
+ *
+ * @param source - The command string to analyze
+ * @returns true if dangerous substitution patterns are detected, false otherwise
  */
-export function containsSubshell(source: string): boolean {
-	// 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
+export function containsDangerousSubstitution(source: string): boolean {
+	// Check for dangerous parameter expansion operators that can execute commands
+	// ${var@P} - Prompt string expansion (interprets escape sequences and executes embedded commands)
+	// ${var@Q} - Quote removal
+	// ${var@E} - Escape sequence expansion
+	// ${var@A} - Assignment statement
+	// ${var@a} - Attribute flags
+	const dangerousParameterExpansion = /\$\{[^}]*@[PQEAa][^}]*\}/.test(source)
+
+	// Check for parameter expansions with assignments that could contain escape sequences
+	// ${var=value} or ${var:=value} can embed commands via escape sequences like \140 (backtick)
+	// Also check for ${var+value}, ${var:-value}, ${var:+value}, ${var:?value}
+	const parameterAssignmentWithEscapes =
+		/\$\{[^}]*[=+\-?][^}]*\\[0-7]{3}[^}]*\}/.test(source) || // octal escapes
+		/\$\{[^}]*[=+\-?][^}]*\\x[0-9a-fA-F]{2}[^}]*\}/.test(source) || // hex escapes
+		/\$\{[^}]*[=+\-?][^}]*\\u[0-9a-fA-F]{4}[^}]*\}/.test(source) // unicode escapes
+
+	// Check for indirect variable references that could execute commands
+	// ${!var} performs indirect expansion which can be dangerous with crafted variable names
+	const indirectExpansion = /\$\{![^}]+\}/.test(source)
+
+	// Check for here-strings with command substitution
+	// <<<$(...) or <<<`...` can execute commands
+	const hereStringWithSubstitution = /<<<\s*(\$\(|`)/.test(source)
+
+	// Return true if any dangerous pattern is detected
+	return (
+		dangerousParameterExpansion || parameterAssignmentWithEscapes || indirectExpansion || hereStringWithSubstitution
+	)
 }
 
 /**
@@ -471,15 +460,15 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user"
  * to resolve conflicts between allowlist and denylist patterns.
  *
  * **Decision Logic:**
- * 1. **Subshell Protection**: If subshells ($() or ``) are present and denylist exists → auto-deny
+ * 1. **Dangerous Substitution Protection**: Commands with dangerous parameter expansions are never auto-approved
  * 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
  *
  * **Return Values:**
- * - `"auto_approve"`: All sub-commands are explicitly allowed
+ * - `"auto_approve"`: All sub-commands are explicitly allowed and no dangerous patterns detected
  * - `"auto_deny"`: At least one sub-command is explicitly denied
- * - `"ask_user"`: Mixed or no matches found, requires user decision
+ * - `"ask_user"`: Mixed or no matches found, requires user decision, or contains dangerous patterns
  *
  * **Examples:**
  * ```typescript
@@ -487,6 +476,10 @@ export type CommandDecision = "auto_approve" | "auto_deny" | "ask_user"
  * getCommandDecision("git status", ["git"], [])
  * // Returns "auto_approve"
  *
+ * // Dangerous pattern - never auto-approved
+ * getCommandDecision('echo "${var@P}"', ["echo"], [])
+ * // Returns "ask_user"
+ *
  * // Longest prefix match - denial wins
  * getCommandDecision("git push origin", ["git"], ["git push"])
  * // Returns "auto_deny"
@@ -528,6 +521,11 @@ export function getCommandDecision(
 		return "auto_deny"
 	}
 
+	// Require explicit user approval for dangerous patterns
+	if (containsDangerousSubstitution(command)) {
+		return "ask_user"
+	}
+
 	// If all sub-commands are approved, approve the whole command
 	if (decisions.every((decision) => decision === "auto_approve")) {
 		return "auto_approve"
@@ -681,10 +679,10 @@ export class CommandValidator {
 		subCommands: string[]
 		allowedMatches: Array<{ command: string; match: string | null }>
 		deniedMatches: Array<{ command: string; match: string | null }>
-		hasSubshells: boolean
+		hasDangerousSubstitution: boolean
 	} {
 		const subCommands = parseCommand(command)
-		const hasSubshells = containsSubshell(command)
+		const hasDangerousSubstitution = containsDangerousSubstitution(command)
 
 		const allowedMatches = subCommands.map((cmd) => ({
 			command: cmd,
@@ -701,7 +699,7 @@ export class CommandValidator {
 			subCommands,
 			allowedMatches,
 			deniedMatches,
-			hasSubshells,
+			hasDangerousSubstitution,
 		}
 	}