diff --git a/src/core/auto-approval/__tests__/commands.spec.ts b/src/core/auto-approval/__tests__/commands.spec.ts index 90d2808ba9c..b4665865027 100644 --- a/src/core/auto-approval/__tests__/commands.spec.ts +++ b/src/core/auto-approval/__tests__/commands.spec.ts @@ -1,4 +1,15 @@ -import { containsDangerousSubstitution, getCommandDecision } from "../commands" +import { + containsShellFileRedirection, + containsBackgroundOperator, + containsDangerousSubstitution, + getCommandDecision, + getSingleCommandDecision, + findLongestPrefixMatch, +} from "../commands" + +// --------------------------------------------------------------------------- +// Hannes's tests: zsh false-positive regressions (#11365, #11382) +// --------------------------------------------------------------------------- describe("containsDangerousSubstitution", () => { describe("zsh array assignments (should NOT be flagged)", () => { @@ -99,3 +110,278 @@ describe("getCommandDecision — integration with dangerous substitution checks" expect(getCommandDecision('echo "${var@P}"', allowedCommands)).toBe("ask_user") }) }) + +// --------------------------------------------------------------------------- +// Shell redirection & background operator detection (#11367) +// --------------------------------------------------------------------------- + +describe("containsShellFileRedirection", () => { + // Should detect file redirection + it("detects output redirection >", () => { + expect(containsShellFileRedirection("git show > out.txt")).toBe(true) + }) + + it("detects append redirection >>", () => { + expect(containsShellFileRedirection("git show >> out.txt")).toBe(true) + }) + + it("detects input redirection <", () => { + expect(containsShellFileRedirection("cat < in.txt")).toBe(true) + }) + + it("detects here-document <<", () => { + expect(containsShellFileRedirection("cat << EOF")).toBe(true) + }) + + it("detects redirection to sensitive path", () => { + expect(containsShellFileRedirection("git show > ~/.ssh/id_rsa")).toBe(true) + }) + + it("detects &> (bash stdout+stderr redirection)", () => { + expect(containsShellFileRedirection("cmd &> out.txt")).toBe(true) + }) + + it("detects >&file (shell redirection to file)", () => { + expect(containsShellFileRedirection("cmd >&out.txt")).toBe(true) + }) + + it("detects 2> (stderr to file)", () => { + expect(containsShellFileRedirection("cmd 2> err.log")).toBe(true) + }) + + // Should NOT detect safe fd-to-fd redirections + it("does not flag 2>&1 (fd-to-fd)", () => { + expect(containsShellFileRedirection("git show 2>&1")).toBe(false) + }) + + it("does not flag >&2 (fd-to-fd)", () => { + expect(containsShellFileRedirection("echo error >&2")).toBe(false) + }) + + it("does not flag 1>&2 (fd-to-fd)", () => { + expect(containsShellFileRedirection("cmd 1>&2")).toBe(false) + }) + + it("does not flag <&3 (input fd-to-fd)", () => { + expect(containsShellFileRedirection("cmd <&3")).toBe(false) + }) + + it("does not flag 0<&4 (input fd-to-fd with explicit fd)", () => { + expect(containsShellFileRedirection("cmd 0<&4")).toBe(false) + }) + + // Token boundary: >&2file is file redirection, not fd-to-fd + it("flags >&2file (word starts with digit but is not pure fd)", () => { + expect(containsShellFileRedirection("echo hi >&2file")).toBe(true) + }) + + it("flags <&3in (word starts with digit but is not pure fd)", () => { + expect(containsShellFileRedirection("cmd <&3in")).toBe(true) + }) + + // fd-to-fd followed by operators should still strip correctly + it("does not flag 2>&1 followed by && (chain operator)", () => { + expect(containsShellFileRedirection("cmd 2>&1&& echo ok")).toBe(false) + }) + + it("detects file redirection after fd-to-fd (2>&1>out.txt)", () => { + expect(containsShellFileRedirection("cmd 2>&1>out.txt")).toBe(true) + }) + + it("does not flag 0<&4 followed by pipe", () => { + expect(containsShellFileRedirection("cmd 0<&4| cat")).toBe(false) + }) + + // Should not flag commands without redirection + it("does not flag plain command", () => { + expect(containsShellFileRedirection("git status")).toBe(false) + }) + + it("does not flag command with flags", () => { + expect(containsShellFileRedirection("git log --oneline -n 10")).toBe(false) + }) + + // Mixed: fd redirect + file redirect + it("detects file redirection even with fd redirect present", () => { + expect(containsShellFileRedirection("cmd 2>&1 > out.txt")).toBe(true) + }) + + // Quote-aware: operators inside quotes are literal, not redirection + it("does not flag > inside double quotes (arrow function)", () => { + expect(containsShellFileRedirection(`node -e "const f=(a)=>a"`)).toBe(false) + }) + + it("does not flag > inside single quotes", () => { + expect(containsShellFileRedirection("echo 'hello > world'")).toBe(false) + }) + + it("detects > outside quotes even when quoted content has >", () => { + expect(containsShellFileRedirection(`node -e "x" > out.txt`)).toBe(true) + }) + + it("does not flag < inside double quotes", () => { + expect(containsShellFileRedirection(`node -e "if (a < b) {}"`)).toBe(false) + }) +}) + +describe("containsBackgroundOperator", () => { + it("detects trailing &", () => { + expect(containsBackgroundOperator("sleep 10 &")).toBe(true) + }) + + it("detects mid-command &", () => { + expect(containsBackgroundOperator("cmd & other")).toBe(true) + }) + + // Should NOT flag && (chain operator) + it("does not flag && (chain)", () => { + expect(containsBackgroundOperator("git add . && git commit")).toBe(false) + }) + + // Should NOT flag &> (redirection) + it("does not flag &> (redirection)", () => { + expect(containsBackgroundOperator("cmd &> out.txt")).toBe(false) + }) + + // Should NOT flag fd redirection containing & + it("does not flag 2>&1 (fd redirection)", () => { + expect(containsBackgroundOperator("git show 2>&1")).toBe(false) + }) + + it("does not flag >&2 (fd redirection)", () => { + expect(containsBackgroundOperator("echo error >&2")).toBe(false) + }) + + it("does not flag <&3 (input fd duplication)", () => { + expect(containsBackgroundOperator("cmd <&3")).toBe(false) + }) + + it("does not flag plain command", () => { + expect(containsBackgroundOperator("git status")).toBe(false) + }) + + // Quote-aware: & inside quotes is literal + it("does not flag & inside double quotes", () => { + expect(containsBackgroundOperator(`node -e "a & b"`)).toBe(false) + }) +}) + +describe("getCommandDecision with shell operators", () => { + const allowlist = ["git", "git show", "cat", "echo", "npm"] + + it("auto-approves allowlisted command without shell operators", () => { + expect(getCommandDecision("git show", allowlist)).toBe("auto_approve") + }) + + it("auto-approves allowlisted command with flags", () => { + expect(getCommandDecision("git show --stat", allowlist)).toBe("auto_approve") + }) + + it("forces ask_user for output redirection on allowlisted prefix", () => { + expect(getCommandDecision("git show > ~/.ssh/id_rsa", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for append redirection on allowlisted prefix", () => { + expect(getCommandDecision("git show >> out.txt", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for input redirection on allowlisted prefix", () => { + expect(getCommandDecision("cat < /etc/passwd", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for &> redirection on allowlisted prefix", () => { + expect(getCommandDecision("git show &> out.txt", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for background operator on allowlisted prefix", () => { + expect(getCommandDecision("git show &", allowlist)).toBe("ask_user") + }) + + it("preserves auto-approve for fd-to-fd redirect (2>&1)", () => { + expect(getCommandDecision("git show 2>&1", allowlist)).toBe("auto_approve") + }) + + it("preserves auto-approve for input fd-to-fd redirect (<&3)", () => { + expect(getCommandDecision("cat <&3", allowlist)).toBe("auto_approve") + }) + + it("forces ask_user for >&2file (word redirection, not fd-to-fd)", () => { + expect(getCommandDecision("echo hi >&2file", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for <&3in (word redirection, not fd-to-fd)", () => { + expect(getCommandDecision("cat <&3in", allowlist)).toBe("ask_user") + }) + + it("forces ask_user for compound command where one segment has redirection", () => { + expect(getCommandDecision("git show && echo ok > out.txt", allowlist)).toBe("ask_user") + }) + + it("still asks user for unknown commands (no regression)", () => { + expect(getCommandDecision("rm -rf /", allowlist)).toBe("ask_user") + }) + + it("still denies denylisted commands (no regression)", () => { + expect(getCommandDecision("git push", allowlist, ["git push"])).toBe("auto_deny") + }) + + it("forces ask_user for >&file (pre-existing strip regex must not remove it)", () => { + expect(getCommandDecision("echo hi >&out.txt", allowlist)).toBe("ask_user") + }) +}) + +// --------------------------------------------------------------------------- +// Shared / regression coverage +// --------------------------------------------------------------------------- + +describe("containsDangerousSubstitution (existing, regression)", () => { + it("detects dangerous parameter expansion", () => { + expect(containsDangerousSubstitution('echo "${var@P}"')).toBe(true) + }) + + it("does not flag normal commands", () => { + expect(containsDangerousSubstitution("git status")).toBe(false) + }) +}) + +describe("findLongestPrefixMatch", () => { + it("finds exact prefix match", () => { + expect(findLongestPrefixMatch("git status", ["git"])).toBe("git") + }) + + it("finds longest match among multiple", () => { + expect(findLongestPrefixMatch("git push origin", ["git", "git push"])).toBe("git push") + }) + + it("returns null for no match", () => { + expect(findLongestPrefixMatch("npm install", ["git"])).toBe(null) + }) + + it("handles wildcard", () => { + expect(findLongestPrefixMatch("anything", ["*"])).toBe("*") + }) +}) + +describe("getSingleCommandDecision", () => { + it("auto-approves when only allowlist matches", () => { + expect(getSingleCommandDecision("git status", ["git"], ["npm"])).toBe("auto_approve") + }) + + it("auto-denies when only denylist matches", () => { + expect(getSingleCommandDecision("rm -rf", [], ["rm"])).toBe("auto_deny") + }) + + it("asks user when no lists match", () => { + expect(getSingleCommandDecision("unknown", ["git"], ["npm"])).toBe("ask_user") + }) + + it("longer denylist wins conflict", () => { + expect(getSingleCommandDecision("git push origin", ["git"], ["git push"])).toBe("auto_deny") + }) + + it("longer allowlist wins conflict", () => { + expect(getSingleCommandDecision("git push --dry-run", ["git push --dry-run"], ["git push"])).toBe( + "auto_approve", + ) + }) +}) diff --git a/src/core/auto-approval/commands.ts b/src/core/auto-approval/commands.ts index d9e88c7ba26..9f0dd5bcd80 100644 --- a/src/core/auto-approval/commands.ts +++ b/src/core/auto-approval/commands.ts @@ -64,6 +64,102 @@ export function containsDangerousSubstitution(source: string): boolean { ) } +/** + * Strip single-quoted and double-quoted segments from a shell command string, + * returning only the unquoted portions. This allows operator detection to + * ignore characters that the shell treats as literal data inside quotes. + * + * Respects backslash escapes outside single quotes (and inside double quotes). + * Single-quoted segments have no escape sequences per POSIX sh. + * + * @param cmd - The command string to process + * @returns The command with all quoted content removed + */ +function stripQuotedSegments(cmd: string): string { + let result = "" + let inSingle = false + let inDouble = false + let escape = false + + for (let i = 0; i < cmd.length; i++) { + const ch = cmd[i] + + if (escape) { + escape = false + continue + } + + if (ch === "\\" && !inSingle) { + escape = true + continue + } + + if (ch === "'" && !inDouble) { + inSingle = !inSingle + continue + } + + if (ch === '"' && !inSingle) { + inDouble = !inDouble + continue + } + + if (!inSingle && !inDouble) { + result += ch + } + } + + return result +} + +/** + * Detect shell file redirection operators that could alter command behavior + * in dangerous ways when executed with `shell: true`. + * + * When a user auto-approves a command prefix like "git show", the model can + * append redirection operators (e.g., "git show > ~/.ssh/id_rsa") that the + * prefix matcher would still approve. Since execution uses shell interpretation, + * the redirection is silently honored. + * + * This is quote-aware: operators inside single or double quotes are literal + * data to the shell and are not flagged (e.g., `node -e "const f=(a)=>a"`). + * + * Safe fd-to-fd redirections (2>&1, >&2) are stripped before detection. + * + * @param command - The command string to analyze + * @returns true if file redirection operators are detected + */ +export function containsShellFileRedirection(command: string): boolean { + const unquoted = stripQuotedSegments(command) + // Strip fd-to-fd redirections which don't write to files. + // Output: 2>&1, >&2, 1>&2. Input: <&3, 0<&4. + // Token boundary prevents stripping >&2 from >&2file (which is file redirection). + const stripped = unquoted + .replace(/\d*>&\d+(?=$|\s|[;&|()<>])/g, "") + .replace(/\d*<&\d+(?=$|\s|[;&|()<>])/g, "") + // Any remaining < or > is file redirection + return /[<>]/.test(stripped) +} + +/** + * Detect standalone background operator (&) in a command string. + * + * Backgrounding is a control-flow modifier under `shell: true` that can + * detach processes or alter execution semantics. Auto-approval should not + * silently grant background execution. + * + * This is quote-aware: & inside quotes is literal data, not a background operator. + * + * Matches standalone & but not && (chain operator), &> (redirection), >& (fd redirection), or <& (input fd duplication). + * + * @param command - The command string to analyze + * @returns true if a background operator is detected + */ +export function containsBackgroundOperator(command: string): boolean { + const unquoted = stripQuotedSegments(command) + return /(^|[^&><])&([^&>]|$)/.test(unquoted) +} + /** * Find the longest matching prefix from a list of prefixes for a given command. * @@ -267,8 +363,12 @@ export function getCommandDecision( // Check each sub-command and collect decisions const decisions: CommandDecision[] = subCommands.map((cmd) => { - // Remove simple PowerShell-like redirections (e.g. 2>&1) before checking - const cmdWithoutRedirection = cmd.replace(/\d*>&\d*/, "").trim() + // Remove fd-to-fd redirections (e.g. 2>&1, <&3) before prefix matching. + // Token boundary prevents stripping >&2 from >&2file (which is file redirection). + const cmdWithoutRedirection = cmd + .replace(/\d*>&\d+(?=$|\s|[;&|()<>])/g, "") + .replace(/\d*<&\d+(?=$|\s|[;&|()<>])/g, "") + .trim() return getSingleCommandDecision(cmdWithoutRedirection, allowedCommands, deniedCommands) }) @@ -283,6 +383,15 @@ export function getCommandDecision( return "ask_user" } + // Require explicit user approval for shell redirection or background operators. + // These can alter command semantics in ways the prefix matcher cannot evaluate + // (e.g., "git show > ~/.ssh/id_rsa" would prefix-match "git show" but redirect + // output to a sensitive file). Since commands execute with shell: true, these + // operators are interpreted by the shell. + if (containsShellFileRedirection(command) || containsBackgroundOperator(command)) { + return "ask_user" + } + // If all sub-commands are approved, approve the whole command if (decisions.every((decision) => decision === "auto_approve")) { return "auto_approve"