diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 1c71e4d3bd..77cffa3b49 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -1,3 +1,4 @@ +import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { DEFAULT_SAFE_BINS, analyzeShellCommand, @@ -9,7 +10,6 @@ import { type CommandResolution, type ExecCommandSegment, } from "./exec-approvals-analysis.js"; -import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { SAFE_BIN_GENERIC_PROFILE, SAFE_BIN_PROFILES, @@ -38,8 +38,6 @@ export function isSafeBinUsage(params: { argv: string[]; resolution: CommandResolution | null; safeBins: Set; - cwd?: string; - fileExists?: (filePath: string) => boolean; trustedSafeBinDirs?: ReadonlySet; }): boolean { // Windows host exec uses PowerShell, which has different parsing/expansion rules. @@ -116,7 +114,6 @@ function evaluateSegments( argv: segment.argv, resolution: segment.resolution, safeBins: params.safeBins, - cwd: params.cwd, trustedSafeBinDirs: params.trustedSafeBinDirs, }); const skillAllow = diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 9acedf9c44..dba99d0f7a 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -494,7 +494,6 @@ describe("exec approvals safe bins", () => { executableName, }, safeBins: normalizeSafeBins(testCase.safeBins ?? [executableName]), - cwd, }); expect(ok).toBe(testCase.expected); }); @@ -513,7 +512,6 @@ describe("exec approvals safe bins", () => { }, safeBins: normalizeSafeBins(["jq"]), trustedSafeBinDirs: new Set(["/custom/bin"]), - cwd: "/tmp", }); expect(ok).toBe(true); }); @@ -540,48 +538,22 @@ describe("exec approvals safe bins", () => { argv: ["sort", "-o", "existing.txt"], resolution, safeBins, - cwd, }); const missing = isSafeBinUsage({ argv: ["sort", "-o", "missing.txt"], resolution, safeBins, - cwd, }); const longFlag = isSafeBinUsage({ argv: ["sort", "--output=missing.txt"], resolution, safeBins, - cwd, }); expect(existing).toBe(false); expect(missing).toBe(false); expect(longFlag).toBe(false); }); - it("does not consult file existence callbacks for safe-bin decisions", () => { - if (process.platform === "win32") { - return; - } - let checkedExists = false; - const ok = isSafeBinUsage({ - argv: ["sort", "-o", "target.txt"], - resolution: { - rawExecutable: "sort", - resolvedPath: "/usr/bin/sort", - executableName: "sort", - }, - safeBins: normalizeSafeBins(["sort"]), - cwd: "/tmp", - fileExists: () => { - checkedExists = true; - return true; - }, - }); - expect(ok).toBe(false); - expect(checkedExists).toBe(false); - }); - it("threads trusted safe-bin dirs through allowlist evaluation", () => { if (process.platform === "win32") { return; @@ -847,7 +819,6 @@ describe("exec approvals node host allowlist check", () => { argv: ["unknown-tool", "--help"], resolution, safeBins: normalizeSafeBins(["jq", "curl"]), - cwd: "/tmp", }); expect(safe).toBe(false); }); @@ -868,7 +839,6 @@ describe("exec approvals node host allowlist check", () => { argv: ["jq", ".foo"], resolution, safeBins: normalizeSafeBins(["jq"]), - cwd: "/tmp", }); // Safe bins are disabled on Windows (PowerShell parsing/expansion differences). if (process.platform === "win32") { diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts index 5a6a7060a0..3fe3e46007 100644 --- a/src/infra/exec-safe-bin-policy.ts +++ b/src/infra/exec-safe-bin-policy.ts @@ -29,13 +29,14 @@ export type SafeBinProfile = { }; const NO_FLAGS = new Set(); +const asFlagSet = (flags: string[]): ReadonlySet => new Set(flags); export const SAFE_BIN_GENERIC_PROFILE: SafeBinProfile = {}; export const SAFE_BIN_PROFILES: Record = { jq: { maxPositional: 1, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--arg", "--argjson", "--argstr", @@ -47,7 +48,7 @@ export const SAFE_BIN_PROFILES: Record = { "-L", "-f", ]), - blockedFlags: new Set([ + blockedFlags: asFlagSet([ "--argfile", "--rawfile", "--slurpfile", @@ -59,7 +60,7 @@ export const SAFE_BIN_PROFILES: Record = { }, grep: { maxPositional: 1, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--regexp", "--file", "--max-count", @@ -82,7 +83,7 @@ export const SAFE_BIN_PROFILES: Record = { "-D", "-d", ]), - blockedFlags: new Set([ + blockedFlags: asFlagSet([ "--file", "--exclude-from", "--dereference-recursive", @@ -96,7 +97,7 @@ export const SAFE_BIN_PROFILES: Record = { }, cut: { maxPositional: 0, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--bytes", "--characters", "--fields", @@ -110,7 +111,7 @@ export const SAFE_BIN_PROFILES: Record = { }, sort: { maxPositional: 0, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--key", "--field-separator", "--buffer-size", @@ -127,11 +128,11 @@ export const SAFE_BIN_PROFILES: Record = { "-T", "-o", ]), - blockedFlags: new Set(["--files0-from", "--output", "-o"]), + blockedFlags: asFlagSet(["--files0-from", "--output", "-o"]), }, uniq: { maxPositional: 0, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--skip-fields", "--skip-chars", "--check-chars", @@ -143,11 +144,11 @@ export const SAFE_BIN_PROFILES: Record = { }, head: { maxPositional: 0, - valueFlags: new Set(["--lines", "--bytes", "-n", "-c"]), + valueFlags: asFlagSet(["--lines", "--bytes", "-n", "-c"]), }, tail: { maxPositional: 0, - valueFlags: new Set([ + valueFlags: asFlagSet([ "--lines", "--bytes", "--sleep-interval", @@ -163,8 +164,8 @@ export const SAFE_BIN_PROFILES: Record = { }, wc: { maxPositional: 0, - valueFlags: new Set(["--files0-from"]), - blockedFlags: new Set(["--files0-from"]), + valueFlags: asFlagSet(["--files0-from"]), + blockedFlags: asFlagSet(["--files0-from"]), }, }; @@ -175,14 +176,86 @@ function isSafeLiteralToken(value: string): boolean { return !hasGlobToken(value) && !isPathLikeToken(value); } +function isInvalidValueToken(value: string | undefined): boolean { + return !value || !isSafeLiteralToken(value); +} + +function consumeLongFlagToken( + args: string[], + index: number, + valueFlags: ReadonlySet, + blockedFlags: ReadonlySet, +): number { + const token = args[index]; + if (!token) { + return -1; + } + const eqIndex = token.indexOf("="); + const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; + if (blockedFlags.has(flag)) { + return -1; + } + if (eqIndex > 0) { + return isSafeLiteralToken(token.slice(eqIndex + 1)) ? index + 1 : -1; + } + if (!valueFlags.has(flag)) { + return index + 1; + } + return isInvalidValueToken(args[index + 1]) ? -1 : index + 2; +} + +function consumeShortFlagClusterToken( + args: string[], + index: number, + valueFlags: ReadonlySet, + blockedFlags: ReadonlySet, +): number { + const token = args[index]; + if (!token) { + return -1; + } + for (let j = 1; j < token.length; j += 1) { + const flag = `-${token[j]}`; + if (blockedFlags.has(flag)) { + return -1; + } + if (!valueFlags.has(flag)) { + continue; + } + const inlineValue = token.slice(j + 1); + if (inlineValue) { + return isSafeLiteralToken(inlineValue) ? index + 1 : -1; + } + return isInvalidValueToken(args[index + 1]) ? -1 : index + 2; + } + return hasGlobToken(token) ? -1 : index + 1; +} + +function consumePositionalToken(token: string, positional: string[]): boolean { + if (!isSafeLiteralToken(token)) { + return false; + } + positional.push(token); + return true; +} + +function validatePositionalCount(positional: string[], profile: SafeBinProfile): boolean { + const minPositional = profile.minPositional ?? 0; + if (positional.length < minPositional) { + return false; + } + return typeof profile.maxPositional !== "number" || positional.length <= profile.maxPositional; +} + export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { const valueFlags = profile.valueFlags ?? NO_FLAGS; const blockedFlags = profile.blockedFlags ?? NO_FLAGS; const positional: string[] = []; - - for (let i = 0; i < args.length; i += 1) { + let i = 0; + while (i < args.length) { const token = args[i]; if (!token) { + i += 1; continue; } if (token === "--") { @@ -191,82 +264,39 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo if (!rest || rest === "-") { continue; } - if (!isSafeLiteralToken(rest)) { + if (!consumePositionalToken(rest, positional)) { return false; } - positional.push(rest); } break; } if (token === "-") { + i += 1; continue; } if (!token.startsWith("-")) { - if (!isSafeLiteralToken(token)) { - return false; - } - positional.push(token); - continue; - } - - if (token.startsWith("--")) { - const eqIndex = token.indexOf("="); - const flag = eqIndex > 0 ? token.slice(0, eqIndex) : token; - if (blockedFlags.has(flag)) { - return false; - } - if (eqIndex > 0) { - if (!isSafeLiteralToken(token.slice(eqIndex + 1))) { - return false; - } - continue; - } - if (!valueFlags.has(flag)) { - continue; - } - const value = args[i + 1]; - if (!value || !isSafeLiteralToken(value)) { + if (!consumePositionalToken(token, positional)) { return false; } i += 1; continue; } - let consumedValue = false; - for (let j = 1; j < token.length; j += 1) { - const flag = `-${token[j]}`; - if (blockedFlags.has(flag)) { + if (token.startsWith("--")) { + const nextIndex = consumeLongFlagToken(args, i, valueFlags, blockedFlags); + if (nextIndex < 0) { return false; } - if (!valueFlags.has(flag)) { - continue; - } - const inlineValue = token.slice(j + 1); - if (inlineValue) { - if (!isSafeLiteralToken(inlineValue)) { - return false; - } - } else { - const value = args[i + 1]; - if (!value || !isSafeLiteralToken(value)) { - return false; - } - i += 1; - } - consumedValue = true; - break; + i = nextIndex; + continue; } - if (!consumedValue && hasGlobToken(token)) { + + const nextIndex = consumeShortFlagClusterToken(args, i, valueFlags, blockedFlags); + if (nextIndex < 0) { return false; } + i = nextIndex; } - const minPositional = profile.minPositional ?? 0; - if (positional.length < minPositional) { - return false; - } - if (typeof profile.maxPositional === "number" && positional.length > profile.maxPositional) { - return false; - } - return true; + return validatePositionalCount(positional, profile); }