diff --git a/docs/tools/exec.md b/docs/tools/exec.md index dd222e0bda..d8aee42e79 100644 --- a/docs/tools/exec.md +++ b/docs/tools/exec.md @@ -51,7 +51,7 @@ Notes: - `tools.exec.ask` (default: `on-miss`) - `tools.exec.node` (default: unset) - `tools.exec.pathPrepend`: list of directories to prepend to `PATH` for exec runs (gateway + sandbox only). -- `tools.exec.safeBins`: stdin-only safe binaries that can run without explicit allowlist entries (resolved path must come from trusted binary directories). +- `tools.exec.safeBins`: stdin-only safe binaries that can run without explicit allowlist entries. For behavior details, see [Safe bins](/tools/exec-approvals#safe-bins-stdin-only). Example: diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 76fa424c9d..f90864a31e 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -12,48 +12,7 @@ import { type CommandResolution, type ExecCommandSegment, } from "./exec-approvals-analysis.js"; - -const DEFAULT_SAFE_BIN_TRUSTED_DIRS = [ - "/bin", - "/usr/bin", - "/usr/local/bin", - "/opt/homebrew/bin", - "/opt/local/bin", - "/snap/bin", - "/run/current-system/sw/bin", -]; - -function normalizeTrustedDir(value: string): string | null { - const trimmed = value.trim(); - if (!trimmed) { - return null; - } - return path.resolve(trimmed); -} - -function collectTrustedSafeBinDirs(): Set { - const trusted = new Set(); - for (const entry of DEFAULT_SAFE_BIN_TRUSTED_DIRS) { - const normalized = normalizeTrustedDir(entry); - if (normalized) { - trusted.add(normalized); - } - } - const pathEntries = (process.env.PATH ?? process.env.Path ?? "") - .split(path.delimiter) - .map((entry) => normalizeTrustedDir(entry)) - .filter((entry): entry is string => Boolean(entry)); - for (const entry of pathEntries) { - trusted.add(entry); - } - return trusted; -} - -const TRUSTED_SAFE_BIN_DIRS = collectTrustedSafeBinDirs(); - -function isTrustedSafeBinPath(resolvedPath: string): boolean { - return TRUSTED_SAFE_BIN_DIRS.has(path.dirname(path.resolve(resolvedPath))); -} +import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; function isPathLikeToken(value: string): boolean { const trimmed = value.trim(); @@ -109,6 +68,7 @@ export function isSafeBinUsage(params: { safeBins: Set; cwd?: string; fileExists?: (filePath: string) => boolean; + trustedSafeBinDirs?: ReadonlySet; }): boolean { // Windows host exec uses PowerShell, which has different parsing/expansion rules. // Keep safeBins conservative there (require explicit allowlist entries). @@ -132,7 +92,12 @@ export function isSafeBinUsage(params: { if (!resolution?.resolvedPath) { return false; } - if (!isTrustedSafeBinPath(resolution.resolvedPath)) { + if ( + !isTrustedSafeBinPath({ + resolvedPath: resolution.resolvedPath, + trustedDirs: params.trustedSafeBinDirs, + }) + ) { return false; } const cwd = params.cwd ?? process.cwd(); diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 93d10979dd..bbb73a4a04 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -385,44 +385,60 @@ describe("exec approvals shell allowlist (chained commands)", () => { }); describe("exec approvals safe bins", () => { - it("allows safe bins with non-path args", () => { - if (process.platform === "win32") { - return; - } - const dir = makeTempDir(); - const ok = isSafeBinUsage({ - argv: ["jq", ".foo"], - resolution: { - rawExecutable: "jq", - resolvedPath: "/usr/bin/jq", - executableName: "jq", - }, - safeBins: normalizeSafeBins(["jq"]), - cwd: dir, - }); - expect(ok).toBe(true); - }); + type SafeBinCase = { + name: string; + argv: string[]; + resolvedPath: string; + expected: boolean; + cwd?: string; + setup?: (cwd: string) => void; + }; - it("blocks safe bins with file args", () => { - if (process.platform === "win32") { - return; - } - const dir = makeTempDir(); - fs.writeFileSync(path.join(dir, "secret.json"), "{}"); - const ok = isSafeBinUsage({ + const cases: SafeBinCase[] = [ + { + name: "allows safe bins with non-path args", + argv: ["jq", ".foo"], + resolvedPath: "/usr/bin/jq", + expected: true, + }, + { + name: "blocks safe bins with file args", argv: ["jq", ".foo", "secret.json"], - resolution: { - rawExecutable: "jq", - resolvedPath: "/usr/bin/jq", - executableName: "jq", - }, - safeBins: normalizeSafeBins(["jq"]), - cwd: dir, - }); - expect(ok).toBe(false); - }); + resolvedPath: "/usr/bin/jq", + expected: false, + setup: (cwd) => fs.writeFileSync(path.join(cwd, "secret.json"), "{}"), + }, + { + name: "blocks safe bins resolved from untrusted directories", + argv: ["jq", ".foo"], + resolvedPath: "/tmp/evil-bin/jq", + expected: false, + cwd: "/tmp", + }, + ]; - it("blocks safe bins resolved from untrusted directories", () => { + for (const testCase of cases) { + it(testCase.name, () => { + if (process.platform === "win32") { + return; + } + const cwd = testCase.cwd ?? makeTempDir(); + testCase.setup?.(cwd); + const ok = isSafeBinUsage({ + argv: testCase.argv, + resolution: { + rawExecutable: "jq", + resolvedPath: testCase.resolvedPath, + executableName: "jq", + }, + safeBins: normalizeSafeBins(["jq"]), + cwd, + }); + expect(ok).toBe(testCase.expected); + }); + } + + it("supports injected trusted safe-bin dirs for tests/callers", () => { if (process.platform === "win32") { return; } @@ -430,13 +446,14 @@ describe("exec approvals safe bins", () => { argv: ["jq", ".foo"], resolution: { rawExecutable: "jq", - resolvedPath: "/tmp/evil-bin/jq", + resolvedPath: "/custom/bin/jq", executableName: "jq", }, safeBins: normalizeSafeBins(["jq"]), + trustedSafeBinDirs: new Set(["/custom/bin"]), cwd: "/tmp", }); - expect(ok).toBe(false); + expect(ok).toBe(true); }); }); diff --git a/src/infra/exec-safe-bin-trust.test.ts b/src/infra/exec-safe-bin-trust.test.ts new file mode 100644 index 0000000000..70d4a7795d --- /dev/null +++ b/src/infra/exec-safe-bin-trust.test.ts @@ -0,0 +1,57 @@ +import path from "node:path"; +import { describe, expect, it } from "vitest"; +import { + buildTrustedSafeBinDirs, + getTrustedSafeBinDirs, + isTrustedSafeBinPath, +} from "./exec-safe-bin-trust.js"; + +describe("exec safe bin trust", () => { + it("builds trusted dirs from defaults and injected PATH", () => { + const dirs = buildTrustedSafeBinDirs({ + pathEnv: "/custom/bin:/alt/bin:/custom/bin", + delimiter: ":", + baseDirs: ["/usr/bin"], + }); + + expect(dirs.has(path.resolve("/usr/bin"))).toBe(true); + expect(dirs.has(path.resolve("/custom/bin"))).toBe(true); + expect(dirs.has(path.resolve("/alt/bin"))).toBe(true); + expect(dirs.size).toBe(3); + }); + + it("memoizes trusted dirs per PATH snapshot", () => { + const a = getTrustedSafeBinDirs({ + pathEnv: "/first/bin", + delimiter: ":", + refresh: true, + }); + const b = getTrustedSafeBinDirs({ + pathEnv: "/first/bin", + delimiter: ":", + }); + const c = getTrustedSafeBinDirs({ + pathEnv: "/second/bin", + delimiter: ":", + }); + + expect(a).toBe(b); + expect(c).not.toBe(b); + }); + + it("validates resolved paths using injected trusted dirs", () => { + const trusted = new Set([path.resolve("/usr/bin")]); + expect( + isTrustedSafeBinPath({ + resolvedPath: "/usr/bin/jq", + trustedDirs: trusted, + }), + ).toBe(true); + expect( + isTrustedSafeBinPath({ + resolvedPath: "/tmp/evil/jq", + trustedDirs: trusted, + }), + ).toBe(false); + }); +}); diff --git a/src/infra/exec-safe-bin-trust.ts b/src/infra/exec-safe-bin-trust.ts new file mode 100644 index 0000000000..b5f27c5b0d --- /dev/null +++ b/src/infra/exec-safe-bin-trust.ts @@ -0,0 +1,101 @@ +import path from "node:path"; + +const DEFAULT_SAFE_BIN_TRUSTED_DIRS = [ + "/bin", + "/usr/bin", + "/usr/local/bin", + "/opt/homebrew/bin", + "/opt/local/bin", + "/snap/bin", + "/run/current-system/sw/bin", +]; + +type TrustedSafeBinDirsParams = { + pathEnv?: string | null; + delimiter?: string; + baseDirs?: readonly string[]; +}; + +type TrustedSafeBinPathParams = { + resolvedPath: string; + trustedDirs?: ReadonlySet; + pathEnv?: string | null; + delimiter?: string; +}; + +type TrustedSafeBinCache = { + key: string; + dirs: Set; +}; + +let trustedSafeBinCache: TrustedSafeBinCache | null = null; + +function normalizeTrustedDir(value: string): string | null { + const trimmed = value.trim(); + if (!trimmed) { + return null; + } + return path.resolve(trimmed); +} + +function buildTrustedSafeBinCacheKey(pathEnv: string, delimiter: string): string { + return `${delimiter}\u0000${pathEnv}`; +} + +export function buildTrustedSafeBinDirs(params: TrustedSafeBinDirsParams = {}): Set { + const delimiter = params.delimiter ?? path.delimiter; + const pathEnv = params.pathEnv ?? ""; + const baseDirs = params.baseDirs ?? DEFAULT_SAFE_BIN_TRUSTED_DIRS; + const trusted = new Set(); + + for (const entry of baseDirs) { + const normalized = normalizeTrustedDir(entry); + if (normalized) { + trusted.add(normalized); + } + } + + const pathEntries = pathEnv + .split(delimiter) + .map((entry) => normalizeTrustedDir(entry)) + .filter((entry): entry is string => Boolean(entry)); + for (const entry of pathEntries) { + trusted.add(entry); + } + + return trusted; +} + +export function getTrustedSafeBinDirs( + params: { + pathEnv?: string | null; + delimiter?: string; + refresh?: boolean; + } = {}, +): Set { + const delimiter = params.delimiter ?? path.delimiter; + const pathEnv = params.pathEnv ?? process.env.PATH ?? process.env.Path ?? ""; + const key = buildTrustedSafeBinCacheKey(pathEnv, delimiter); + + if (!params.refresh && trustedSafeBinCache?.key === key) { + return trustedSafeBinCache.dirs; + } + + const dirs = buildTrustedSafeBinDirs({ + pathEnv, + delimiter, + }); + trustedSafeBinCache = { key, dirs }; + return dirs; +} + +export function isTrustedSafeBinPath(params: TrustedSafeBinPathParams): boolean { + const trustedDirs = + params.trustedDirs ?? + getTrustedSafeBinDirs({ + pathEnv: params.pathEnv, + delimiter: params.delimiter, + }); + const resolvedDir = path.dirname(path.resolve(params.resolvedPath)); + return trustedDirs.has(resolvedDir); +}