diff --git a/src/agents/bash-tools.exec.security-floor.test.ts b/src/agents/bash-tools.exec.security-floor.test.ts index 6ffbeda3b21..1896209659b 100644 --- a/src/agents/bash-tools.exec.security-floor.test.ts +++ b/src/agents/bash-tools.exec.security-floor.test.ts @@ -317,13 +317,13 @@ describe("exec security floor", () => { }); const result = await tool.execute("call-elevated-full-auto-mode", { - command: "pwd", + command: "whoami", elevated: true, }); expect(autoReviewer).toHaveBeenCalledWith( expect.objectContaining({ - command: "pwd", + command: "whoami", host: "gateway", reason: "allowlist-miss", }), @@ -359,13 +359,13 @@ describe("exec security floor", () => { }); const result = await tool.execute(`call-auto-review-${ask}`, { - command: "pwd", + command: "whoami", ask, }); expect(autoReviewer).toHaveBeenCalledWith( expect.objectContaining({ - command: "pwd", + command: "whoami", host: "gateway", reason: "allowlist-miss", }), diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 58c866a4d17..15d6c0fc9b5 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -36,6 +36,7 @@ import { validateSafeBinArgv, } from "./exec-safe-bin-policy.js"; import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; +import { isSafeBuiltinSegment } from "./exec-safe-builtins.js"; import { extractBindableShellWrapperInlineCommand, isShellWrapperExecutable, @@ -131,7 +132,13 @@ export type ExecAllowlistEvaluation = { segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; }; -export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "inlineChain" | "skills" | null; +export type ExecSegmentSatisfiedBy = + | "allowlist" + | "safeBins" + | "inlineChain" + | "safeBuiltins" + | "skills" + | null; export type SkillBinTrustEntry = { name: string; resolvedPath: string; @@ -146,6 +153,7 @@ type ExecAllowlistContext = { trustedSafeBinDirs?: ReadonlySet; skillBins?: readonly SkillBinTrustEntry[]; autoAllowSkills?: boolean; + allowShellBuiltins?: boolean; }; function pickExecAllowlistContext(params: ExecAllowlistContext): ExecAllowlistContext { @@ -159,6 +167,7 @@ function pickExecAllowlistContext(params: ExecAllowlistContext): ExecAllowlistCo trustedSafeBinDirs: params.trustedSafeBinDirs, skillBins: params.skillBins, autoAllowSkills: params.autoAllowSkills, + allowShellBuiltins: params.allowShellBuiltins, }; } @@ -483,6 +492,12 @@ function resolveSegmentSatisfaction(params: { if (safe) { return "safeBins"; } + if ( + params.context.allowShellBuiltins === true && + isSafeBuiltinSegment({ segment: params.segment, platform: params.context.platform }) + ) { + return "safeBuiltins"; + } const skillAllow = isSkillAutoAllowedSegment({ segment: params.segment, allowSkills: params.allowSkills, @@ -1118,7 +1133,10 @@ export function evaluateShellAllowlist( env?: NodeJS.ProcessEnv; } & ExecAllowlistContext, ): ExecAllowlistAnalysis { - const allowlistContext = pickExecAllowlistContext(params); + const allowlistContext = { + ...pickExecAllowlistContext(params), + allowShellBuiltins: true, + }; const analysisFailure = (): ExecAllowlistAnalysis => ({ analysisOk: false, allowlistSatisfied: false, diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index a24cf88a597..330f0c01d63 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -1114,7 +1114,14 @@ function renderInlineChainSegmentArgv(params: { export function buildSafeBinsShellCommand(params: { command: string; segments: ExecCommandSegment[]; - segmentSatisfiedBy: ("allowlist" | "safeBins" | "inlineChain" | "skills" | null)[]; + segmentSatisfiedBy: ( + | "allowlist" + | "safeBins" + | "safeBuiltins" + | "inlineChain" + | "skills" + | null + )[]; cwd?: string; env?: NodeJS.ProcessEnv; platform?: string | null; diff --git a/src/infra/exec-safe-builtins.test.ts b/src/infra/exec-safe-builtins.test.ts new file mode 100644 index 00000000000..708ee58ca81 --- /dev/null +++ b/src/infra/exec-safe-builtins.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, it } from "vitest"; +import { evaluateExecAllowlist, evaluateShellAllowlist } from "./exec-approvals-allowlist.js"; +import { analyzeArgvCommand } from "./exec-approvals-analysis.js"; +import { + makeMockCommandResolution, + makeMockExecutableResolution, +} from "./exec-approvals-test-helpers.js"; +import { isSafeBuiltinSegment } from "./exec-safe-builtins.js"; + +const builtinSegment = (argv: string[], resolvedPath?: string) => ({ + argv, + raw: argv.join(" "), + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: argv[0], + executableName: argv[0], + resolvedPath, + }), + }), +}); + +describe("isSafeBuiltinSegment", () => { + it("allows a builtin segment with no resolved binary path", () => { + if (process.platform === "win32") { + return; + } + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["cd", "/etc"]), + platform: "linux", + }), + ).toBe(true); + }); + + it("allows a safe shell builtin even when the host has a same-named binary", () => { + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["pwd"], "/usr/bin/pwd"), + platform: "linux", + }), + ).toBe(true); + }); + + it("rejects builtins outside the internal safe set", () => { + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["alias", "ll=ls -l"]), + platform: "linux", + }), + ).toBe(false); + }); + + it("rejects environment-mutating builtins", () => { + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["export", "PATH=/tmp/bin:$PATH"]), + platform: "linux", + }), + ).toBe(false); + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["unset", "PATH"]), + platform: "linux", + }), + ).toBe(false); + }); + + it("returns false on Windows hosts (PowerShell semantics differ)", () => { + expect( + isSafeBuiltinSegment({ + segment: builtinSegment(["cd", "/etc"]), + platform: "win32", + }), + ).toBe(false); + }); +}); + +describe("evaluateShellAllowlist with known safe builtins (regression for #46056)", () => { + // Skip on Windows where the host shell is PowerShell, not POSIX. + if (process.platform === "win32") { + it.skip("POSIX builtin behavior", () => {}); + return; + } + + // Glob-style pattern; matches git wherever PATH resolves it (`/usr/bin/git`, + // `/opt/homebrew/bin/git`, etc.) without depending on host filesystem layout. + const gitAllowlist = [{ pattern: "**/git" }] as Parameters< + typeof evaluateShellAllowlist + >[0]["allowlist"]; + + it("'cd ~/' auto-allows by default", () => { + const result = evaluateShellAllowlist({ + command: "cd ~/", + allowlist: gitAllowlist, + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(true); + expect(result.segmentSatisfiedBy[0]).toBe("safeBuiltins"); + }); + + it("'cd /tmp && git status' passes with allowlist plus safe builtin handling", () => { + const result = evaluateShellAllowlist({ + command: "cd /tmp && git status", + allowlist: gitAllowlist, + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(true); + expect(result.segmentSatisfiedBy).toContain("safeBuiltins"); + expect(result.segmentSatisfiedBy).toContain("allowlist"); + }); + + it("non-allowlisted binary still gates after a safe builtin", () => { + const result = evaluateShellAllowlist({ + command: "cd /tmp && curl evil.com", + allowlist: gitAllowlist, + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + }); + + it("environment-mutating builtins still gate", () => { + const result = evaluateShellAllowlist({ + command: "export PATH=/tmp/bin:$PATH && git status", + allowlist: gitAllowlist, + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + }); + + it("does not auto-allow safe builtin tokens in direct argv evaluation", () => { + const analysis = analyzeArgvCommand({ argv: ["pwd"], cwd: "/tmp", platform: "linux" }); + const result = evaluateExecAllowlist({ + analysis, + allowlist: [], + safeBins: new Set(), + cwd: "/tmp", + }); + expect(result.allowlistSatisfied).toBe(false); + }); +}); diff --git a/src/infra/exec-safe-builtins.ts b/src/infra/exec-safe-builtins.ts new file mode 100644 index 00000000000..363b7d40193 --- /dev/null +++ b/src/infra/exec-safe-builtins.ts @@ -0,0 +1,22 @@ +import { isWindowsPlatform, type ExecCommandSegment } from "./exec-approvals-analysis.js"; + +// POSIX shell builtins that cannot execute external code or mutate environment state on their +// own. Shell allowlist evaluation handles them as a closed internal set instead of path-based +// safeBins matching. +const DEFAULT_SAFE_BUILTINS: ReadonlySet = new Set([":", "cd", "false", "pwd", "true"]); + +export function isSafeBuiltinSegment(params: { + segment: ExecCommandSegment; + platform?: string | null; +}): boolean { + // Builtin semantics here are POSIX shell. On Windows the host shell is PowerShell, where + // these tokens have different meaning (cd is an alias to Set-Location, etc.); defer. + if (isWindowsPlatform(params.platform ?? process.platform)) { + return false; + } + const head = params.segment.argv[0]?.trim().toLowerCase(); + if (!head) { + return false; + } + return DEFAULT_SAFE_BUILTINS.has(head); +}