From 65946937a02aac47fa5f5423cbd756fa19b8ae53 Mon Sep 17 00:00:00 2001 From: subrih Date: Sat, 14 Mar 2026 09:33:05 -0700 Subject: [PATCH] fix: validation completeness and consumer invariant consistency across all enforcement layers - permAllowsWrite (bwrap), permToOps/deniedOps (seatbelt): guard all positional perm accesses with VALID_PERM_RE - catchAllPerm/tmpPerm (seatbelt): validate rawPerm before positional access; fail closed to '---' - hasScriptOverride (exec-runtime): check entry shape (non-null object, not array) before setting bypass flag - scripts["policy"] merged into overrideRules in applyScriptPolicyOverride (was silently dropped) - mergeAccessPolicy: reject non-object script entries before propagating - validateAccessPolicyFileStructure: recurse into per-script entries to catch removed deny/default fields - validateAccessPolicyConfig: reject non-object entries, validate sha256 format, emit mid-path wildcard diagnostics for scripts["policy"] AND per-script policy blocks (previously only config.policy) - env-prefix regex: handle escaped quotes in double-quoted values ((?:[^"\\]|\\.)*) - _resetBwrapAvailableCacheForTest: export added for test isolation - Tests added for all of the above --- src/agents/bash-tools.exec-runtime.ts | 14 ++++- src/infra/access-policy-file.ts | 35 +++++++++++- src/infra/access-policy.test.ts | 82 +++++++++++++++++++++++++++ src/infra/access-policy.ts | 47 +++++++++++++-- src/infra/exec-sandbox-bwrap.test.ts | 14 +++++ src/infra/exec-sandbox-bwrap.ts | 13 ++++- src/infra/exec-sandbox-seatbelt.ts | 19 ++++++- 7 files changed, 211 insertions(+), 13 deletions(-) diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 91e53968001..243af461794 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -390,9 +390,17 @@ export async function runExecProcess(opts: { // symlink keys ("/usr/bin/python" → /usr/bin/python3.12) match argv0, which // is always the realpathSync result from resolveArgv0. const _scripts = opts.permissions.scripts ?? {}; - // Skip the reserved "policy" key — it holds shared rules, not a per-script entry. - const hasScriptOverride = Object.keys(_scripts).some( - (k) => k !== "policy" && path.normalize(resolveScriptKey(k)) === path.normalize(argv0), + // Skip the reserved "policy" key and malformed (non-object) entries — only a + // validated ScriptPolicyEntry object counts as a legitimate script override. + // A truthy primitive (true, "oops") would otherwise bypass the base exec gate + // even though applyScriptPolicyOverride already rejected the entry. + const hasScriptOverride = Object.entries(_scripts).some( + ([k, v]) => + k !== "policy" && + v != null && + typeof v === "object" && + !Array.isArray(v) && + path.normalize(resolveScriptKey(k)) === path.normalize(argv0), ); if (!hasScriptOverride && checkAccessPolicy(argv0, "exec", effectivePermissions) === "deny") { throw new Error(`exec denied by access policy: ${argv0}`); diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index ee19e703ba0..9630703dbad 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -66,6 +66,15 @@ export function mergeAccessPolicy( const baseEntry = base.scripts?.[key] as | import("../config/types.tools.js").ScriptPolicyEntry | undefined; + // Reject non-object entries — a primitive (true, "rwx") is not a valid ScriptPolicyEntry. + // validateAccessPolicyConfig also catches this, but the merge layer must not propagate it. + if ( + overrideEntry == null || + typeof overrideEntry !== "object" || + Array.isArray(overrideEntry) + ) { + continue; + } const overrideScriptEntry = overrideEntry as import("../config/types.tools.js").ScriptPolicyEntry; if (!baseEntry) { @@ -123,7 +132,27 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s if (typeof block !== "object" || block === null || Array.isArray(block)) { errors.push(`${filePath}: agents["${agentId}"] must be an object`); } else { - checkRemovedKeys(block as Record, `agents["${agentId}"]`); + const agentBlock = block as Record; + checkRemovedKeys(agentBlock, `agents["${agentId}"]`); + // Recurse into per-script entries so old "deny"/"default" fields are caught there too. + const scripts = agentBlock["scripts"]; + if (scripts != null && typeof scripts === "object" && !Array.isArray(scripts)) { + for (const [scriptKey, scriptEntry] of Object.entries( + scripts as Record, + )) { + if ( + scriptKey !== "policy" && + scriptEntry != null && + typeof scriptEntry === "object" && + !Array.isArray(scriptEntry) + ) { + checkRemovedKeys( + scriptEntry as Record, + `agents["${agentId}"].scripts["${scriptKey}"]`, + ); + } + } + } } } } @@ -298,7 +327,9 @@ export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfi // auto-expand diagnostics ("rule auto-expanded to ...") are informational // and the footer would mislead operators into thinking the policy is broken. if (errors.some((e) => !e.includes("auto-expanded") && !e.includes("mid-path wildcard"))) { - console.error(`[access-policy] Bad permission strings are treated as "---" (deny all).`); + console.error( + `[access-policy] Bad permission strings are treated as "---" (deny all) at the tool layer and OS sandbox layer.`, + ); } } } diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index 7caa1778ab2..7c610950ce6 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -70,6 +70,73 @@ describe("validateAccessPolicyConfig", () => { expect(checkAccessPolicy(file, "read", config)).toBe("deny"); }); + it("rejects non-object script entries (e.g. a bare string or boolean)", () => { + // A primitive entry like "/deploy.sh": "rwx" or "/deploy.sh": true would bypass + // the exec gate — validateAccessPolicyConfig must reject it at load time. + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": "rwx" as unknown as import("../config/types.tools.js").ScriptPolicyEntry, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("/deploy.sh") && e.includes("must be an object"))).toBe( + true, + ); + }); + + it("rejects a sha256 value with wrong length", () => { + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": { sha256: "abc123" }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("sha256") && e.includes("64-character"))).toBe(true); + }); + + it("rejects a sha256 value with non-hex characters", () => { + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": { sha256: "z".repeat(64) }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("sha256") && e.includes("64-character"))).toBe(true); + }); + + it("accepts a valid 64-char hex sha256", () => { + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": { sha256: "a".repeat(64) }, + }, + }; + expect(validateAccessPolicyConfig(config)).toEqual([]); + }); + + it("emits mid-path wildcard diagnostic for scripts['policy'] entries", () => { + const config: AccessPolicyConfig = { + scripts: { + policy: { "/home/*/workspace/**": "r--" }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect( + errs.some((e) => e.includes("mid-path wildcard") && e.includes('scripts["policy"]')), + ).toBe(true); + }); + + it("emits mid-path wildcard diagnostic for per-script policy entries", () => { + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": { policy: { "/home/*/workspace/**": "r--" } }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("mid-path wildcard") && e.includes("/deploy.sh"))).toBe( + true, + ); + }); + it("validates scripts[].policy perm strings and emits diagnostics for bad ones", () => { // A typo like "rwX" in a script's policy must produce a diagnostic, not silently // fail closed (which would deny exec with no operator-visible error). @@ -717,6 +784,21 @@ describe("resolveArgv0", () => { expect(result).toMatch(/sh$/); }); + it("handles env assignment with escaped quote inside double-quoted value", () => { + // MYVAR="a\"b" /usr/bin/python script.py — the \" inside the value must not + // truncate the match, which would leave `b"` as the next token and misidentify + // it as argv0 instead of /usr/bin/python. + const result = resolveArgv0('MYVAR="a\\"b" /bin/sh -c echo'); + expect(result).not.toBeNull(); + expect(result).toMatch(/sh$/); + }); + + it("handles multiple env assignments with escaped quotes in values", () => { + const result = resolveArgv0('A="x\\"y" B="p\\"q" /bin/sh -c echo'); + expect(result).not.toBeNull(); + expect(result).toMatch(/sh$/); + }); + // The following tests use /bin/sh and Unix env behaviour — skip on Windows where // /bin/sh doesn't exist and env resolves to env.EXE with different semantics. const itUnix = it.skipIf(process.platform === "win32"); diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 02a89ab4c31..7f5831d05fa 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -132,6 +132,15 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] `access-policy.scripts["policy"]["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`, ); } + if ( + hasMidPathWildcard(pattern) && + !_midPathWildcardWarned.has(`scripts:policy:${pattern}`) + ) { + _midPathWildcardWarned.add(`scripts:policy:${pattern}`); + errors.push( + `access-policy.scripts["policy"]["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, + ); + } autoExpandBareDir(sharedPolicy, pattern, perm, errors); } } @@ -139,14 +148,41 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] if (scriptPath === "policy") { continue; // handled above } - const scriptEntry = entry as import("../config/types.tools.js").ScriptPolicyEntry | undefined; - if (scriptEntry?.policy) { + // Reject non-object entries (e.g. true, "rwx") — a truthy primitive would + // bypass the exec gate in hasScriptOverride and applyScriptPolicyOverride. + if (entry == null || typeof entry !== "object" || Array.isArray(entry)) { + errors.push( + `access-policy.scripts["${scriptPath}"] must be an object (e.g. { sha256: "...", policy: {...} }), got ${JSON.stringify(entry)}`, + ); + continue; + } + const scriptEntry = entry as import("../config/types.tools.js").ScriptPolicyEntry; + // Validate sha256 format when present — a typo causes silent exec denial at runtime. + if (scriptEntry.sha256 !== undefined) { + if (!/^[0-9a-f]{64}$/i.test(scriptEntry.sha256)) { + errors.push( + `access-policy.scripts["${scriptPath}"].sha256 "${scriptEntry.sha256}" is invalid: must be a 64-character lowercase hex string`, + ); + } + } + if (scriptEntry.policy) { for (const [pattern, perm] of Object.entries(scriptEntry.policy)) { if (!PERM_STR_RE.test(perm)) { errors.push( `access-policy.scripts["${scriptPath}"].policy["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`, ); } + // Emit mid-path wildcard diagnostic for per-script policy blocks, + // matching the same warning emitted for config.policy entries. + if ( + hasMidPathWildcard(pattern) && + !_midPathWildcardWarned.has(`scripts:${scriptPath}:${pattern}`) + ) { + _midPathWildcardWarned.add(`scripts:${scriptPath}:${pattern}`); + errors.push( + `access-policy.scripts["${scriptPath}"].policy["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, + ); + } autoExpandBareDir(scriptEntry.policy, pattern, perm, errors); } } @@ -396,11 +432,14 @@ export function resolveArgv0(command: string, cwd?: string, _depth = 0): string // FOO='a b') prevents misparse when a quoted env value contains spaces — a naive // whitespace-split would break FOO='a b' /script.sh into ["FOO='a", "b'", "/script.sh"] // and incorrectly treat "b'" as the argv0, bypassing script policy lookups. - const envPrefixRe = /^[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*)\s*/; + // Double-quoted values: allow backslash-escaped characters (e.g. "a\"b") so the + // regex doesn't truncate at the escaped quote and misidentify the next token as argv0. + // Single-quoted values: no escaping in POSIX sh single quotes, so [^']* is correct. + const envPrefixRe = /^[A-Za-z_][A-Za-z0-9_]*=(?:"(?:[^"\\]|\\.)*"|'[^']*'|\S*)\s*/; let rest = trimmed; while (envPrefixRe.test(rest)) { // Capture a literal PATH= override; skip if the value contains $ (unexpandable). - const pathM = rest.match(/^PATH=(?:"([^"]*)"|'([^']*)'|(\S+))\s*/); + const pathM = rest.match(/^PATH=(?:"((?:[^"\\]|\\.)*)"|'([^']*)'|(\S+))\s*/); if (pathM) { const val = pathM[1] ?? pathM[2] ?? pathM[3] ?? ""; if (!val.includes("$")) { diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index 8d4d02617ae..639ac353b7a 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -2,9 +2,11 @@ import os from "node:os"; import { describe, expect, it, vi } from "vitest"; import type { AccessPolicyConfig } from "../config/types.tools.js"; import { + _resetBwrapAvailableCacheForTest, _resetBwrapFileDenyWarnedPathsForTest, _warnBwrapFileDenyOnce, generateBwrapArgs, + isBwrapAvailable, wrapCommandWithBwrap, } from "./exec-sandbox-bwrap.js"; @@ -428,6 +430,18 @@ describe("wrapCommandWithBwrap", () => { }); }); +describe("_resetBwrapAvailableCacheForTest", () => { + it("clears the availability cache so isBwrapAvailable re-probes", async () => { + // Prime the cache with one result, then reset and verify the next call re-checks. + await isBwrapAvailable(); // populates cache + _resetBwrapAvailableCacheForTest(); + // After reset, isBwrapAvailable must re-probe (result may differ in test env — just + // verify it returns a boolean without throwing, proving the cache was cleared). + const result = await isBwrapAvailable(); + expect(typeof result).toBe("boolean"); + }); +}); + describe("_resetBwrapFileDenyWarnedPathsForTest", () => { it("clears the warned-paths set so the same path can warn again", () => { const spy = vi.spyOn(console, "error").mockImplementation(() => {}); diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index d8462a1c6d0..fb21437413b 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -36,6 +36,10 @@ const _bwrapFileDenyWarnedPaths = new Set(); export function _resetBwrapFileDenyWarnedPathsForTest(): void { _bwrapFileDenyWarnedPaths.clear(); } +/** Reset the bwrap availability cache. Only for use in tests. */ +export function _resetBwrapAvailableCacheForTest(): void { + bwrapAvailableCache = undefined; +} export function _warnBwrapFileDenyOnce(filePath: string): void { if (_bwrapFileDenyWarnedPaths.has(filePath)) { return; @@ -116,8 +120,11 @@ function patternToPath(pattern: string, homeDir: string, perm?: PermStr): string return parentDir || "/"; } +// Keep in sync with VALID_PERM_RE in access-policy.ts and exec-sandbox-seatbelt.ts. +const VALID_PERM_RE = /^[r-][w-][x-]$/; + function permAllowsWrite(perm: PermStr): boolean { - return perm[1] === "w"; + return VALID_PERM_RE.test(perm) && perm[1] === "w"; } /** @@ -143,7 +150,9 @@ export function generateBwrapArgs( ): string[] { const args: string[] = []; // Determine base stance from the "/**" catch-all rule (replaces the removed `default` field). - const catchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---"; + const rawCatchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---"; + // Validate format before positional access — malformed perm falls back to "---" (fail closed). + const catchAllPerm = VALID_PERM_RE.test(rawCatchAllPerm) ? rawCatchAllPerm : "---"; const defaultAllowsRead = catchAllPerm[0] === "r"; if (defaultAllowsRead) { diff --git a/src/infra/exec-sandbox-seatbelt.ts b/src/infra/exec-sandbox-seatbelt.ts index c48ce7f53b7..bbe7f871ccc 100644 --- a/src/infra/exec-sandbox-seatbelt.ts +++ b/src/infra/exec-sandbox-seatbelt.ts @@ -135,7 +135,13 @@ function patternToSbplMatcher(pattern: string, homeDir: string, perm?: PermStr): return { matcher: sbplLiteral(base), approximate: false }; } +// Keep in sync with VALID_PERM_RE in access-policy.ts and exec-sandbox-bwrap.ts. +const VALID_PERM_RE = /^[r-][w-][x-]$/; + function permToOps(perm: PermStr): string[] { + if (!VALID_PERM_RE.test(perm)) { + return []; + } const ops: string[] = []; if (perm[0] === "r") { ops.push(SEATBELT_READ_OPS); @@ -150,6 +156,10 @@ function permToOps(perm: PermStr): string[] { } function deniedOps(perm: PermStr): string[] { + // Malformed perm — deny everything (fail closed). + if (!VALID_PERM_RE.test(perm)) { + return [SEATBELT_READ_OPS, SEATBELT_WRITE_OPS, SEATBELT_EXEC_OPS]; + } const ops: string[] = []; if (perm[0] !== "r") { ops.push(SEATBELT_READ_OPS); @@ -184,7 +194,9 @@ export function generateSeatbeltProfile( lines.push(""); // Determine base stance from the "/**" catch-all rule (replaces the removed `default` field). - const catchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---"; + const rawCatchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---"; + // Validate format before positional access — malformed perm falls back to "---" (fail closed). + const catchAllPerm = VALID_PERM_RE.test(rawCatchAllPerm) ? rawCatchAllPerm : "---"; const defaultPerm = catchAllPerm; // alias for readability below const defaultAllowsAnything = catchAllPerm[0] === "r" || catchAllPerm[1] === "w" || catchAllPerm[2] === "x"; @@ -223,7 +235,10 @@ export function generateSeatbeltProfile( // unconditionally granting /tmp access when default: "---". // findBestRule probes both the path and path+"/" internally, so "/tmp" correctly // matches glob rules like "/tmp/**" without needing the "/tmp/." workaround. - const tmpPerm = findBestRule("/tmp", config.policy ?? {}, homeDir) ?? "---"; + const rawTmpPerm = findBestRule("/tmp", config.policy ?? {}, homeDir) ?? "---"; + // Validate before positional access — malformed perm falls back to "---" (fail closed), + // consistent with permToOps/deniedOps and the tool-layer permAllows guard. + const tmpPerm = VALID_PERM_RE.test(rawTmpPerm) ? rawTmpPerm : "---"; // Emit read and write allowances independently so a read-only policy like // "/tmp/**": "r--" does not accidentally grant write access to /tmp. if (tmpPerm[0] === "r") {