diff --git a/src/core/auto-approval/__tests__/commands.spec.ts b/src/core/auto-approval/__tests__/commands.spec.ts index 6a238ec0eb3..90d2808ba9c 100644 --- a/src/core/auto-approval/__tests__/commands.spec.ts +++ b/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") + }) +}) diff --git a/src/core/auto-approval/commands.ts b/src/core/auto-approval/commands.ts index e4e9c62d157..d9e88c7ba26 100644 --- a/src/core/auto-approval/commands.ts +++ b/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 = /(? { 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,