fix: harden command auto-approval against inline JS false positives (#11382)

src/core/auto-approval/__tests__/commands.spec.ts

@@ -37,3 +37,65 @@ describe("getCommandDecision", () => {
 		expect(result).toBe("auto_approve")
 	})
 })
+
+describe("containsDangerousSubstitution — node -e one-liner false positive regression", () => {
+	const nodeOneLiner = `node -e "const fs=require('fs');const p=JSON.parse(fs.readFileSync('prd.json','utf8'));const allowed=new Set(['pending','in-progress','complete','blocked']);const bad=(p.items||[]).filter(i=>!allowed.has(i.status));console.log('meta.status',p.meta?.status);console.log('workstreams', (p.workstreams||[]).length);console.log('items', (p.items||[]).length);console.log('statusCounts', (p.items||[]).reduce((a,i)=>(a[i.status]=(a[i.status]||0)+1,a),{}));console.log('invalidStatuses', bad.length);if(bad.length){console.log(bad.map(i=>i.id+':'+i.status).join('\\\\n'));process.exit(2);} "`
+
+	it("should NOT flag the complex node -e one-liner as dangerous substitution", () => {
+		expect(containsDangerousSubstitution(nodeOneLiner)).toBe(false)
+	})
+})
+
+describe("containsDangerousSubstitution — arrow function patterns (should NOT be flagged)", () => {
+	it("should return false for node -e with simple arrow function", () => {
+		expect(containsDangerousSubstitution(`node -e "const a=(b)=>b"`)).toBe(false)
+	})
+
+	it("should return false for node -e with spaced arrow function", () => {
+		expect(containsDangerousSubstitution(`node -e "const fn = (x) => x * 2"`)).toBe(false)
+	})
+
+	it("should return false for node -e with arrow function in method chain", () => {
+		expect(containsDangerousSubstitution(`node -e "arr.filter(i=>!set.has(i))"`)).toBe(false)
+	})
+})
+
+describe("containsDangerousSubstitution — true positives still caught", () => {
+	it("should flag dangerous parameter expansion ${var@P}", () => {
+		expect(containsDangerousSubstitution('echo "${var@P}"')).toBe(true)
+	})
+
+	it("should flag here-string with command substitution <<<$(…)", () => {
+		expect(containsDangerousSubstitution("cat <<<$(whoami)")).toBe(true)
+	})
+
+	it("should flag indirect variable reference ${!var}", () => {
+		expect(containsDangerousSubstitution("echo ${!prefix}")).toBe(true)
+	})
+
+	it("should flag zsh process substitution =(…) at start of token", () => {
+		expect(containsDangerousSubstitution("echo =(cat /etc/passwd)")).toBe(true)
+	})
+
+	it("should flag zsh glob qualifier with code execution", () => {
+		expect(containsDangerousSubstitution("ls *(e:whoami:)")).toBe(true)
+	})
+})
+
+describe("getCommandDecision — integration with dangerous substitution checks", () => {
+	const allowedCommands = ["node", "echo"]
+
+	it("should auto-approve the complex node -e one-liner when node is allowed", () => {
+		const nodeOneLiner = `node -e "const fs=require('fs');const p=JSON.parse(fs.readFileSync('prd.json','utf8'));const allowed=new Set(['pending','in-progress','complete','blocked']);const bad=(p.items||[]).filter(i=>!allowed.has(i.status));console.log('meta.status',p.meta?.status);console.log('workstreams', (p.workstreams||[]).length);console.log('items', (p.items||[]).length);console.log('statusCounts', (p.items||[]).reduce((a,i)=>(a[i.status]=(a[i.status]||0)+1,a),{}));console.log('invalidStatuses', bad.length);if(bad.length){console.log(bad.map(i=>i.id+':'+i.status).join('\\\\n'));process.exit(2);} "`
+
+		expect(getCommandDecision(nodeOneLiner, allowedCommands)).toBe("auto_approve")
+	})
+
+	it("should ask user for echo $(whoami) because subshell whoami is not in the allowlist", () => {
+		expect(getCommandDecision("echo $(whoami)", allowedCommands)).toBe("ask_user")
+	})
+
+	it("should ask user for dangerous parameter expansion even when command is allowed", () => {
+		expect(getCommandDecision('echo "${var@P}"', allowedCommands)).toBe("ask_user")
+	})
+})

src/core/auto-approval/commands.ts

@@ -46,7 +46,7 @@ export function containsDangerousSubstitution(source: string): boolean {
 
 	// Check for zsh process substitution =(...) which executes commands
 	// =(...) creates a temporary file containing the output of the command, but executes it
-	const zshProcessSubstitution = /(?<![a-zA-Z0-9_])=\([^)]+\)/.test(source)
+	const zshProcessSubstitution = /(?:(?<=^)|(?<=[\s;|&(<]))=\([^)]+\)/.test(source)
 
 	// Check for zsh glob qualifiers with code execution (e:...:)
 	// Patterns like *(e:whoami:) or ?(e:rm -rf /:) execute commands during glob expansion

src/core/tools/ExecuteCommandTool.ts

@@ -43,7 +43,9 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 				return
 			}
 
-			const ignoredFileAttemptedToAccess = task.rooIgnoreController?.validateCommand(command)
+			const canonicalCommand = unescapeHtmlEntities(command)
+
+			const ignoredFileAttemptedToAccess = task.rooIgnoreController?.validateCommand(canonicalCommand)
 
 			if (ignoredFileAttemptedToAccess) {
 				await task.say("rooignore_error", ignoredFileAttemptedToAccess)
@@ -53,8 +55,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 
 			task.consecutiveMistakeCount = 0
 
-			const unescapedCommand = unescapeHtmlEntities(command)
-			const didApprove = await askApproval("command", unescapedCommand)
+			const didApprove = await askApproval("command", canonicalCommand)
 
 			if (!didApprove) {
 				return
@@ -78,7 +79,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 
 			// Check if command matches any prefix in the allowlist
 			const isCommandAllowlisted = commandTimeoutAllowlist.some((prefix) =>
-				unescapedCommand.startsWith(prefix.trim()),
+				canonicalCommand.startsWith(prefix.trim()),
 			)
 
 			// Convert seconds to milliseconds for internal use, but skip timeout if command is allowlisted
@@ -86,7 +87,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
 
 			const options: ExecuteCommandOptions = {
 				executionId,
-				command: unescapedCommand,
+				command: canonicalCommand,
 				customCwd,
 				terminalShellIntegrationDisabled,
 				commandExecutionTimeout,