From 09d02c641f13ff630df90d39b19123fda3272a8d Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 23 Jul 2026 17:13:23 +0200 Subject: [PATCH] fix(coding-agent): closed bash approval rule bypasses Tested the shell-control guard against the raw command: whitespace normalization collapsed newlines/CR before the guard ran, so 'git status\nrm file.txt' rode a 'git *' allow rule while bash executed both lines. Honored tool-owned allow/prompt policies in yolo mode so per-command prompt rules were no longer silently discarded under the default approvalMode. Added precision regression tests through the real matcher (separators, subshells, redirects, env prefixes, path/quoting variants) that fail on the unfixed head. --- packages/coding-agent/src/tools/approval.ts | 9 ++++ packages/coding-agent/src/tools/bash.ts | 2 +- .../coding-agent/test/tools/approval.test.ts | 54 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/tools/approval.ts b/packages/coding-agent/src/tools/approval.ts index bc0870902..1cabaac1e 100644 --- a/packages/coding-agent/src/tools/approval.ts +++ b/packages/coding-agent/src/tools/approval.ts @@ -130,6 +130,15 @@ export function resolveApproval( } if (mode === "yolo") { + if (decision.policy) { + return { + policy: decision.policy, + tier: decision.tier, + override: false, + source: "tool", + ...(decision.reason ? { reason: decision.reason } : {}), + }; + } return { policy: userPolicy ?? "allow", tier: decision.tier, diff --git a/packages/coding-agent/src/tools/bash.ts b/packages/coding-agent/src/tools/bash.ts index 967df4742..d4d3151b1 100644 --- a/packages/coding-agent/src/tools/bash.ts +++ b/packages/coding-agent/src/tools/bash.ts @@ -179,7 +179,7 @@ function findBashApprovalPatternRule( rules: readonly BashApprovalPatternRule[], ): BashApprovalPatternRule | undefined { return rules.find(rule => { - if (rule.approval === "allow" && BASH_APPROVAL_SHELL_CONTROL_RE.test(normalizeBashApprovalPattern(command))) { + if (rule.approval === "allow" && BASH_APPROVAL_SHELL_CONTROL_RE.test(command)) { return false; } return commandMatchesBashApprovalPattern(command, rule.match); diff --git a/packages/coding-agent/test/tools/approval.test.ts b/packages/coding-agent/test/tools/approval.test.ts index ab53dc8bf..59c5d6b18 100644 --- a/packages/coding-agent/test/tools/approval.test.ts +++ b/packages/coding-agent/test/tools/approval.test.ts @@ -273,6 +273,60 @@ describe("tool-owned dynamic approval declarations", () => { reason: "Blocked by bash pattern: rm -rf *", }); }); + it("never auto-approves a command that only prefixes an allow pattern", () => { + const settingsOverrides = { + "bash.patterns": [{ match: "git *", approval: "allow" }], + }; + + // Shell control syntax after (or around) the allowed prefix must not ride the allow rule. + for (const command of [ + "git status; rm file.txt", + "git status && rm file.txt", + "git status | sh", + "git status\nrm file.txt", + "git status\r\nrm file.txt", + "git $(rm file.txt)", + "git `rm file.txt` status", + "git status > /etc/passwd", + "git status < seed", + // Different binary resolution than the pattern names. + "FOO=1 git status", + "/usr/bin/git status", + '"git" status', + "gitx status", + "git", + "", + ]) { + const decision = bashApproval(command, settingsOverrides); + expect(typeof decision === "object" ? decision.policy : undefined).not.toBe("allow"); + } + + for (const command of ["git status", "git status --short", "git status", "git\tstatus"]) { + expect(bashApproval(command, settingsOverrides)).toEqual({ tier: "write", policy: "allow" }); + } + }); + + it("honors bash pattern rules in yolo mode", () => { + const tool = createBashTool({ + "bash.patterns": [ + { match: "echo *", approval: "prompt" }, + { match: "git *", approval: "allow" }, + ], + }); + + expect(resolveApproval(tool, { command: "echo hello" }, "yolo", {})).toMatchObject({ + policy: "prompt", + source: "tool", + }); + expect(resolveApproval(tool, { command: "git status" }, "yolo", {})).toMatchObject({ + policy: "allow", + source: "tool", + }); + expect(resolveApproval(tool, { command: "true" }, "yolo", {})).toMatchObject({ + policy: "allow", + source: "mode", + }); + }); it("exports LSP and debug read-only action sets from their owning tools", () => { expect(LSP_READONLY_ACTIONS.has("diagnostics")).toBe(true);