From 89aad7b922835e40b4df54a9e6195a5f8ee2e5b6 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 19:24:23 +0100 Subject: [PATCH] refactor: tighten safe-bin policy model and docs parity --- docs/tools/exec-approvals.md | 10 ++ src/infra/exec-approvals.test.ts | 158 ++++++++++++++++--------- src/infra/exec-safe-bin-policy.test.ts | 52 +++++++- src/infra/exec-safe-bin-policy.ts | 113 ++++++++++-------- 4 files changed, 227 insertions(+), 106 deletions(-) diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index e002fc937f9..f977952c83c 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -131,6 +131,16 @@ File-oriented options are denied for default safe bins (for example `sort -o`, ` `grep -f/--file`). Safe bins also enforce explicit per-binary flag policy for options that break stdin-only behavior (for example `sort -o/--output/--compress-program` and grep recursive flags). +Denied flags by safe-bin profile: + + + +- `grep`: `--dereference-recursive`, `--directories`, `--exclude-from`, `--file`, `--recursive`, `-R`, `-d`, `-f`, `-r` +- `jq`: `--argfile`, `--from-file`, `--library-path`, `--rawfile`, `--slurpfile`, `-L`, `-f` +- `sort`: `--compress-program`, `--files0-from`, `--output`, `-o` +- `wc`: `--files0-from` + + Safe bins also force argv tokens to be treated as **literal text** at execution time (no globbing and no `$VARS` expansion) for stdin-only segments, so patterns like `*` or `$HOME/...` cannot be used to smuggle file reads. diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 4befd13202a..0d4b2e3b1ee 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -519,6 +519,103 @@ describe("exec approvals safe bins", () => { setup?: (cwd: string) => void; }; + function buildDeniedFlagVariantCases(params: { + executableName: string; + resolvedPath: string; + safeBins?: string[]; + flag: string; + takesValue: boolean; + label: string; + }): SafeBinCase[] { + const value = "blocked"; + const argvVariants: string[][] = []; + if (!params.takesValue) { + argvVariants.push([params.executableName, params.flag]); + } else if (params.flag.startsWith("--")) { + argvVariants.push([params.executableName, `${params.flag}=${value}`]); + argvVariants.push([params.executableName, params.flag, value]); + } else if (params.flag.startsWith("-")) { + argvVariants.push([params.executableName, `${params.flag}${value}`]); + argvVariants.push([params.executableName, params.flag, value]); + } else { + argvVariants.push([params.executableName, params.flag, value]); + } + return argvVariants.map((argv) => ({ + name: `${params.label} (${argv.slice(1).join(" ")})`, + argv, + resolvedPath: params.resolvedPath, + expected: false, + safeBins: params.safeBins ?? [params.executableName], + executableName: params.executableName, + })); + } + + const deniedFlagCases: SafeBinCase[] = [ + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "-o", + takesValue: true, + label: "blocks sort output flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--output", + takesValue: true, + label: "blocks sort output flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--compress-program", + takesValue: true, + label: "blocks sort external program flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "grep", + resolvedPath: "/usr/bin/grep", + flag: "-R", + takesValue: false, + label: "blocks grep recursive flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "grep", + resolvedPath: "/usr/bin/grep", + flag: "--recursive", + takesValue: false, + label: "blocks grep recursive flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "grep", + resolvedPath: "/usr/bin/grep", + flag: "--file", + takesValue: true, + label: "blocks grep file-pattern flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "jq", + resolvedPath: "/usr/bin/jq", + flag: "-f", + takesValue: true, + label: "blocks jq file-program flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "jq", + resolvedPath: "/usr/bin/jq", + flag: "--from-file", + takesValue: true, + label: "blocks jq file-program flag", + }), + ...buildDeniedFlagVariantCases({ + executableName: "wc", + resolvedPath: "/usr/bin/wc", + flag: "--files0-from", + takesValue: true, + label: "blocks wc file-list flag", + }), + ]; + const cases: SafeBinCase[] = [ { name: "allows safe bins with non-path args", @@ -540,54 +637,7 @@ describe("exec approvals safe bins", () => { expected: false, cwd: "/tmp", }, - { - name: "blocks sort output path via -o ", - argv: ["sort", "-o", "malicious.sh"], - resolvedPath: "/usr/bin/sort", - expected: false, - safeBins: ["sort"], - executableName: "sort", - }, - { - name: "blocks sort output path via attached short option (-ofile)", - argv: ["sort", "-omalicious.sh"], - resolvedPath: "/usr/bin/sort", - expected: false, - safeBins: ["sort"], - executableName: "sort", - }, - { - name: "blocks sort output path via --output=file", - argv: ["sort", "--output=malicious.sh"], - resolvedPath: "/usr/bin/sort", - expected: false, - safeBins: ["sort"], - executableName: "sort", - }, - { - name: "blocks sort external program flag via --compress-program=", - argv: ["sort", "--compress-program=sh"], - resolvedPath: "/usr/bin/sort", - expected: false, - safeBins: ["sort"], - executableName: "sort", - }, - { - name: "blocks sort external program flag via --compress-program ", - argv: ["sort", "--compress-program", "sh"], - resolvedPath: "/usr/bin/sort", - expected: false, - safeBins: ["sort"], - executableName: "sort", - }, - { - name: "blocks grep recursive flags that read cwd", - argv: ["grep", "-R", "needle"], - resolvedPath: "/usr/bin/grep", - expected: false, - safeBins: ["grep"], - executableName: "grep", - }, + ...deniedFlagCases, { name: "blocks grep file positional when pattern uses -e", argv: ["grep", "-e", "needle", ".env"], @@ -690,13 +740,13 @@ describe("exec approvals safe bins", () => { for (const [name, fixture] of Object.entries(SAFE_BIN_PROFILE_FIXTURES)) { const profile = SAFE_BIN_PROFILES[name]; expect(profile).toBeDefined(); - const fixtureBlockedFlags = fixture.blockedFlags ?? []; - const compiledBlockedFlags = profile?.blockedFlags ?? new Set(); - for (const blockedFlag of fixtureBlockedFlags) { - expect(compiledBlockedFlags.has(blockedFlag)).toBe(true); + const fixtureDeniedFlags = fixture.deniedFlags ?? []; + const compiledDeniedFlags = profile?.deniedFlags ?? new Set(); + for (const deniedFlag of fixtureDeniedFlags) { + expect(compiledDeniedFlags.has(deniedFlag)).toBe(true); } - expect(Array.from(compiledBlockedFlags).toSorted()).toEqual( - [...fixtureBlockedFlags].toSorted(), + expect(Array.from(compiledDeniedFlags).toSorted()).toEqual( + [...fixtureDeniedFlags].toSorted(), ); } }); diff --git a/src/infra/exec-safe-bin-policy.test.ts b/src/infra/exec-safe-bin-policy.test.ts index 89bcd74df51..a300c20b7bd 100644 --- a/src/infra/exec-safe-bin-policy.test.ts +++ b/src/infra/exec-safe-bin-policy.test.ts @@ -1,5 +1,26 @@ +import fs from "node:fs"; +import path from "node:path"; import { describe, expect, it } from "vitest"; -import { SAFE_BIN_PROFILES, validateSafeBinArgv } from "./exec-safe-bin-policy.js"; +import { + SAFE_BIN_PROFILE_FIXTURES, + SAFE_BIN_PROFILES, + renderSafeBinDeniedFlagsDocBullets, + validateSafeBinArgv, +} from "./exec-safe-bin-policy.js"; + +const SAFE_BIN_DOC_DENIED_FLAGS_START = ""; +const SAFE_BIN_DOC_DENIED_FLAGS_END = ""; + +function buildDeniedFlagArgvVariants(flag: string): string[][] { + const value = "blocked"; + if (flag.startsWith("--")) { + return [[`${flag}=${value}`], [flag, value], [flag]]; + } + if (flag.startsWith("-")) { + return [[`${flag}${value}`], [flag, value], [flag]]; + } + return [[flag]]; +} describe("exec safe bin policy grep", () => { const grepProfile = SAFE_BIN_PROFILES.grep; @@ -34,3 +55,32 @@ describe("exec safe bin policy sort", () => { expect(validateSafeBinArgv(["--compress-program", "sh"], sortProfile)).toBe(false); }); }); + +describe("exec safe bin policy denied-flag matrix", () => { + for (const [binName, fixture] of Object.entries(SAFE_BIN_PROFILE_FIXTURES)) { + const profile = SAFE_BIN_PROFILES[binName]; + const deniedFlags = fixture.deniedFlags ?? []; + for (const deniedFlag of deniedFlags) { + const variants = buildDeniedFlagArgvVariants(deniedFlag); + for (const variant of variants) { + it(`${binName} denies ${deniedFlag} (${variant.join(" ")})`, () => { + expect(validateSafeBinArgv(variant, profile)).toBe(false); + }); + } + } + } +}); + +describe("exec safe bin policy docs parity", () => { + it("keeps denied-flag docs in sync with policy fixtures", () => { + const docsPath = path.resolve(process.cwd(), "docs/tools/exec-approvals.md"); + const docs = fs.readFileSync(docsPath, "utf8").replaceAll("\r\n", "\n"); + const start = docs.indexOf(SAFE_BIN_DOC_DENIED_FLAGS_START); + const end = docs.indexOf(SAFE_BIN_DOC_DENIED_FLAGS_END); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + const actual = docs.slice(start + SAFE_BIN_DOC_DENIED_FLAGS_START.length, end).trim(); + const expected = renderSafeBinDeniedFlagsDocBullets(); + expect(actual).toBe(expected); + }); +}); diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts index 5dfc8b109d1..fc40f9b9be8 100644 --- a/src/infra/exec-safe-bin-policy.ts +++ b/src/infra/exec-safe-bin-policy.ts @@ -26,15 +26,15 @@ function hasGlobToken(value: string): boolean { export type SafeBinProfile = { minPositional?: number; maxPositional?: number; - valueFlags?: ReadonlySet; - blockedFlags?: ReadonlySet; + allowedValueFlags?: ReadonlySet; + deniedFlags?: ReadonlySet; }; export type SafeBinProfileFixture = { minPositional?: number; maxPositional?: number; - valueFlags?: readonly string[]; - blockedFlags?: readonly string[]; + allowedValueFlags?: readonly string[]; + deniedFlags?: readonly string[]; }; const NO_FLAGS: ReadonlySet = new Set(); @@ -50,8 +50,8 @@ function compileSafeBinProfile(fixture: SafeBinProfileFixture): SafeBinProfile { return { minPositional: fixture.minPositional, maxPositional: fixture.maxPositional, - valueFlags: toFlagSet(fixture.valueFlags), - blockedFlags: toFlagSet(fixture.blockedFlags), + allowedValueFlags: toFlagSet(fixture.allowedValueFlags), + deniedFlags: toFlagSet(fixture.deniedFlags), }; } @@ -68,19 +68,8 @@ export const SAFE_BIN_GENERIC_PROFILE_FIXTURE: SafeBinProfileFixture = {}; export const SAFE_BIN_PROFILE_FIXTURES: Record = { jq: { maxPositional: 1, - valueFlags: [ - "--arg", - "--argjson", - "--argstr", - "--argfile", - "--rawfile", - "--slurpfile", - "--from-file", - "--library-path", - "-L", - "-f", - ], - blockedFlags: [ + allowedValueFlags: ["--arg", "--argjson", "--argstr"], + deniedFlags: [ "--argfile", "--rawfile", "--slurpfile", @@ -95,30 +84,25 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = // Allowing one positional is ambiguous because -e consumes the pattern and // frees the positional slot for a filename. maxPositional: 0, - valueFlags: [ + allowedValueFlags: [ "--regexp", - "--file", "--max-count", "--after-context", "--before-context", "--context", "--devices", - "--directories", "--binary-files", "--exclude", - "--exclude-from", "--include", "--label", "-e", - "-f", "-m", "-A", "-B", "-C", "-D", - "-d", ], - blockedFlags: [ + deniedFlags: [ "--file", "--exclude-from", "--dereference-recursive", @@ -132,7 +116,7 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = }, cut: { maxPositional: 0, - valueFlags: [ + allowedValueFlags: [ "--bytes", "--characters", "--fields", @@ -146,7 +130,7 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = }, sort: { maxPositional: 0, - valueFlags: [ + allowedValueFlags: [ "--key", "--field-separator", "--buffer-size", @@ -154,28 +138,33 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = "--parallel", "--batch-size", "--random-source", - "--files0-from", - "--output", "-k", "-t", "-S", "-T", - "-o", ], // --compress-program can invoke an external executable and breaks stdin-only guarantees. - blockedFlags: ["--compress-program", "--files0-from", "--output", "-o"], + deniedFlags: ["--compress-program", "--files0-from", "--output", "-o"], }, uniq: { maxPositional: 0, - valueFlags: ["--skip-fields", "--skip-chars", "--check-chars", "--group", "-f", "-s", "-w"], + allowedValueFlags: [ + "--skip-fields", + "--skip-chars", + "--check-chars", + "--group", + "-f", + "-s", + "-w", + ], }, head: { maxPositional: 0, - valueFlags: ["--lines", "--bytes", "-n", "-c"], + allowedValueFlags: ["--lines", "--bytes", "-n", "-c"], }, tail: { maxPositional: 0, - valueFlags: [ + allowedValueFlags: [ "--lines", "--bytes", "--sleep-interval", @@ -191,8 +180,7 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = }, wc: { maxPositional: 0, - valueFlags: ["--files0-from"], - blockedFlags: ["--files0-from"], + deniedFlags: ["--files0-from"], }, }; @@ -201,6 +189,29 @@ export const SAFE_BIN_GENERIC_PROFILE = compileSafeBinProfile(SAFE_BIN_GENERIC_P export const SAFE_BIN_PROFILES: Record = compileSafeBinProfiles(SAFE_BIN_PROFILE_FIXTURES); +export function resolveSafeBinDeniedFlags( + fixtures: Readonly> = SAFE_BIN_PROFILE_FIXTURES, +): Record { + const out: Record = {}; + for (const [name, fixture] of Object.entries(fixtures)) { + const denied = Array.from(new Set(fixture.deniedFlags ?? [])).toSorted(); + if (denied.length > 0) { + out[name] = denied; + } + } + return out; +} + +export function renderSafeBinDeniedFlagsDocBullets( + fixtures: Readonly> = SAFE_BIN_PROFILE_FIXTURES, +): string { + const deniedByBin = resolveSafeBinDeniedFlags(fixtures); + const bins = Object.keys(deniedByBin).toSorted(); + return bins + .map((bin) => `- \`${bin}\`: ${deniedByBin[bin].map((flag) => `\`${flag}\``).join(", ")}`) + .join("\n"); +} + function isSafeLiteralToken(value: string): boolean { if (!value || value === "-") { return true; @@ -217,16 +228,16 @@ function consumeLongOptionToken( index: number, flag: string, inlineValue: string | undefined, - valueFlags: ReadonlySet, - blockedFlags: ReadonlySet, + allowedValueFlags: ReadonlySet, + deniedFlags: ReadonlySet, ): number { - if (blockedFlags.has(flag)) { + if (deniedFlags.has(flag)) { return -1; } if (inlineValue !== undefined) { return isSafeLiteralToken(inlineValue) ? index + 1 : -1; } - if (!valueFlags.has(flag)) { + if (!allowedValueFlags.has(flag)) { return index + 1; } return isInvalidValueToken(args[index + 1]) ? -1 : index + 2; @@ -238,15 +249,15 @@ function consumeShortOptionClusterToken( raw: string, cluster: string, flags: string[], - valueFlags: ReadonlySet, - blockedFlags: ReadonlySet, + allowedValueFlags: ReadonlySet, + deniedFlags: ReadonlySet, ): number { for (let j = 0; j < flags.length; j += 1) { const flag = flags[j]; - if (blockedFlags.has(flag)) { + if (deniedFlags.has(flag)) { return -1; } - if (!valueFlags.has(flag)) { + if (!allowedValueFlags.has(flag)) { continue; } const inlineValue = cluster.slice(j + 1); @@ -275,8 +286,8 @@ function validatePositionalCount(positional: string[], profile: SafeBinProfile): } export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): boolean { - const valueFlags = profile.valueFlags ?? NO_FLAGS; - const blockedFlags = profile.blockedFlags ?? NO_FLAGS; + const allowedValueFlags = profile.allowedValueFlags ?? NO_FLAGS; + const deniedFlags = profile.deniedFlags ?? NO_FLAGS; const positional: string[] = []; let i = 0; while (i < args.length) { @@ -315,8 +326,8 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo i, token.flag, token.inlineValue, - valueFlags, - blockedFlags, + allowedValueFlags, + deniedFlags, ); if (nextIndex < 0) { return false; @@ -331,8 +342,8 @@ export function validateSafeBinArgv(args: string[], profile: SafeBinProfile): bo token.raw, token.cluster, token.flags, - valueFlags, - blockedFlags, + allowedValueFlags, + deniedFlags, ); if (nextIndex < 0) { return false;