Handle substitution patterns in command validation (#7390)

Author: mrubens

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
 				})