From eb40d49d447c83863a1ac921d8fc67dd2aecbb0c Mon Sep 17 00:00:00 2001 From: subrih Date: Sat, 14 Mar 2026 08:43:36 -0700 Subject: [PATCH] fix(access-policy): merge scripts[policy], isDir guard in bwrap overrides, permAllows format check, script entry shape check --- src/infra/access-policy.test.ts | 70 ++++++++++++++++++++++++++++ src/infra/access-policy.ts | 30 +++++++++--- src/infra/exec-sandbox-bwrap.test.ts | 26 +++++++++++ src/infra/exec-sandbox-bwrap.ts | 13 +++++- 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index e72b6c4918b..7caa1778ab2 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -243,6 +243,18 @@ describe("checkAccessPolicy — malformed permission characters fail closed", () const config = { policy: { "/tmp/**": "rWx" as unknown as "rwx" } }; expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("deny"); }); + + it("treats 'aw-' (invalid first char) as deny for write even though index 1 is 'w'", () => { + // defense-in-depth: full format must be [r-][w-][x-]; 'a' at index 0 fails the regex + // so the entire string is rejected rather than accidentally granting write. + const config = { policy: { "/tmp/**": "aw-" as unknown as "r--" } }; + expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("deny"); + }); + + it("treats a 4-char string as deny (wrong length)", () => { + const config = { policy: { "/tmp/**": "rwx!" as unknown as "rwx" } }; + expect(checkAccessPolicy("/tmp/foo.txt", "exec", config)).toBe("deny"); + }); }); // --------------------------------------------------------------------------- @@ -1029,4 +1041,62 @@ describe("applyScriptPolicyOverride", () => { fs.rmSync(tmpDir, { recursive: true }); } }); + + it("merges scripts['policy'] into overrideRules when a script matches", () => { + // scripts["policy"] is the shared base for all named script entries. + // It must appear in overrideRules so the tool layer and OS sandbox enforce it. + const base: AccessPolicyConfig = { + policy: { "/**": "r--" }, + scripts: { + policy: { [`${HOME}/.secrets/token`]: "r--" }, + "/my/script.sh": { policy: { "/tmp/**": "rwx" } }, + }, + }; + const { overrideRules } = applyScriptPolicyOverride(base, "/my/script.sh"); + expect(overrideRules?.[`${HOME}/.secrets/token`]).toBe("r--"); + expect(overrideRules?.["/tmp/**"]).toBe("rwx"); + }); + + it("per-script policy wins over scripts['policy'] on conflict", () => { + const base: AccessPolicyConfig = { + policy: { "/**": "r--" }, + scripts: { + policy: { "/tmp/**": "r--" }, + "/my/script.sh": { policy: { "/tmp/**": "rwx" } }, + }, + }; + const { overrideRules } = applyScriptPolicyOverride(base, "/my/script.sh"); + expect(overrideRules?.["/tmp/**"]).toBe("rwx"); + }); + + it("includes scripts['policy'] even when per-script entry has no policy key", () => { + // A script entry with only sha256 and no policy still gets scripts["policy"] applied. + const base: AccessPolicyConfig = { + policy: { "/**": "r--" }, + scripts: { + policy: { [`${HOME}/.secrets/token`]: "r--" }, + "/my/script.sh": {}, + }, + }; + const { overrideRules } = applyScriptPolicyOverride(base, "/my/script.sh"); + expect(overrideRules?.[`${HOME}/.secrets/token`]).toBe("r--"); + }); + + it("returns base policy unchanged when script entry is a non-object truthy value", () => { + // A malformed entry like `true` or `"oops"` must not be treated as a valid override. + // Without the shape check, a truthy primitive would skip sha256 and mark hasScriptOverride=true. + const base: AccessPolicyConfig = { + policy: { "/**": "r--" }, + scripts: { + "/my/script.sh": true as unknown as import("../config/types.tools.js").ScriptPolicyEntry, + }, + }; + const { policy, overrideRules, hashMismatch } = applyScriptPolicyOverride( + base, + "/my/script.sh", + ); + expect(overrideRules).toBeUndefined(); + expect(hashMismatch).toBeUndefined(); + expect(policy).toBe(base); + }); }); diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 419fce3d89f..02a89ab4c31 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -220,6 +220,11 @@ const OP_GRANT_CHAR: Record = { exec: "x", }; +// Valid perm strings are exactly 3 chars: [r-][w-][x-]. +// Validated at parse time by validateAccessPolicyConfig, but also checked here +// as defense-in-depth so a malformed value never accidentally grants access. +const VALID_PERM_RE = /^[r-][w-][x-]$/; + /** * Returns true if the given permission string grants the requested operation. * An absent or malformed string is treated as "---" (deny all). @@ -227,7 +232,7 @@ const OP_GRANT_CHAR: Record = { * including typos fails closed rather than accidentally granting access. */ function permAllows(perm: PermStr | undefined, op: FsOp): boolean { - if (!perm) { + if (!perm || !VALID_PERM_RE.test(perm)) { return false; } return perm[OP_INDEX[op]] === OP_GRANT_CHAR[op]; @@ -567,12 +572,18 @@ export function applyScriptPolicyOverride( // resolveArgv0 always returns the realpathSync result, so both forms must be // normalized the same way or the lookup silently misses, skipping sha256 verification. const scripts = policy.scripts; - const override = scripts - ? (Object.entries(scripts).find( + const rawOverride = scripts + ? Object.entries(scripts).find( ([k]) => k !== "policy" && path.normalize(resolveScriptKey(k)) === path.normalize(resolvedArgv0), - )?.[1] as import("../config/types.tools.js").ScriptPolicyEntry | undefined) + )?.[1] : undefined; + // Reject non-object entries (e.g. true, "oops") — a truthy primitive would + // otherwise skip sha256 verification and act as an unchecked override grant. + const override = + rawOverride != null && typeof rawOverride === "object" && !Array.isArray(rawOverride) + ? (rawOverride as import("../config/types.tools.js").ScriptPolicyEntry) + : undefined; if (!override) { return { policy }; } @@ -610,9 +621,16 @@ export function applyScriptPolicyOverride( // a broadly denied subtree). const { scripts: _scripts, ...base } = policy; const merged: AccessPolicyConfig = { ...base }; + + // Merge scripts["policy"] (shared base for all matching scripts) with the + // per-script entry policy. Per-script wins on conflict (applied last). + const sharedPolicy = scripts?.["policy"]; + const mergedOverride: Record = { + ...sharedPolicy, + ...override.policy, + }; return { policy: merged, - overrideRules: - override.policy && Object.keys(override.policy).length > 0 ? override.policy : undefined, + overrideRules: Object.keys(mergedOverride).length > 0 ? mergedOverride : undefined, }; } diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index 6486ba0e123..8d4d02617ae 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -325,6 +325,32 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindArgs[secretIdx]).toBe("--ro-bind-try"); }); + it('script override "---" rule targeting a file does not emit --tmpfs (bwrap rejects file paths)', () => { + // The base-rules loop has an isDir guard before --tmpfs; the scriptOverrideRules loop must too. + // /etc/hosts is a real file; emitting --tmpfs /etc/hosts would make bwrap fail at runtime. + const config: AccessPolicyConfig = { policy: { "/**": "r--" } }; + const overrides = { "/etc/hosts": "---" as const }; + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const args = generateBwrapArgs(config, HOME, overrides); + const tmpfsMounts = args + .map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)) + .filter(Boolean); + expect(tmpfsMounts).not.toContain("/etc/hosts"); + expect(spy).toHaveBeenCalledWith(expect.stringContaining("/etc/hosts")); + } finally { + spy.mockRestore(); + } + }); + + it('script override "---" rule targeting a non-existent path emits --tmpfs (assumed directory)', () => { + const config: AccessPolicyConfig = { policy: { "/**": "r--" } }; + const overrides = { "/nonexistent-path-for-test/**": "---" as const }; + const args = generateBwrapArgs(config, HOME, overrides); + const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); + expect(tmpfsMounts).toContain("/nonexistent-path-for-test"); + }); + it('script override "-w-" under restrictive base emits --bind-try, not --tmpfs', () => { // Greptile: permAllowsWrite && (r || defaultR) condition was wrong — for -w- without /** // both flags are false so it fell to else → --tmpfs, silently blocking writes. diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 6d5e49887d3..d8462a1c6d0 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -255,7 +255,18 @@ export function generateBwrapArgs( } else if (perm[0] === "r") { args.push("--ro-bind-try", p, p); } else { - args.push("--tmpfs", p); + // Mirror the base-rules isDir guard — bwrap --tmpfs only accepts directories. + let isDir = true; + try { + isDir = fs.statSync(p).isDirectory(); + } catch { + // Non-existent — assume directory (forward-protection). + } + if (isDir) { + args.push("--tmpfs", p); + } else { + _warnBwrapFileDenyOnce(p); + } } } }