From cd2d9c3b8db465947ab9473379f594baf4a5d5b6 Mon Sep 17 00:00:00 2001 From: subrih Date: Sat, 14 Mar 2026 05:44:16 -0700 Subject: [PATCH] =?UTF-8?q?refactor(access-policy):=20drop=20deny[]=20and?= =?UTF-8?q?=20default=20=E2=80=94=20rules=20only,=20---=20implies=20deny?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/agents/bash-tools.exec-runtime.ts | 21 +- .../pi-tools.read.edit-permission.test.ts | 3 - src/config/types.tools.ts | 9 +- src/infra/access-policy-file.test.ts | 122 +++----- src/infra/access-policy-file.ts | 33 +- src/infra/access-policy.test.ts | 285 +++++------------- src/infra/access-policy.ts | 119 +------- src/infra/exec-sandbox-bwrap.test.ts | 124 +++----- src/infra/exec-sandbox-bwrap.ts | 61 ++-- src/infra/exec-sandbox-seatbelt.test.ts | 125 +++----- src/infra/exec-sandbox-seatbelt.ts | 28 +- 11 files changed, 246 insertions(+), 684 deletions(-) diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 9252ea0056b..7e425ab4202 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -384,30 +384,15 @@ export async function runExecProcess(opts: { // enforcement (seatbelt/bwrap) is unavailable (Linux without bwrap, Windows). // Mirrors the checkAccessPolicy calls in read/write tools for consistency. // - // deny[] always wins — checked unconditionally even for scripts{} entries. - // The hash check authorises the policy overlay (effectivePermissions), not exec - // itself. A script in both deny[] and scripts{} is a config error; the deny wins. - // For scripts{} entries not in deny[], we skip the broader rules/default check - // so a sha256-matched script doesn't also need an explicit exec rule in the base policy. + // For scripts{} entries, skip the broader rules check — a sha256-matched script + // doesn't also need an explicit exec rule in the base policy. // Use resolveScriptKey so that both tilde keys ("~/bin/deploy.sh") and // symlink keys ("/usr/bin/python" → /usr/bin/python3.12) match argv0, which - // is always the realpathSync result from resolveArgv0. Without symlink - // resolution, a symlink-keyed script would silently bypass the override gate, - // running under the base policy with no integrity or deny-narrowing checks. + // is always the realpathSync result from resolveArgv0. const _scripts = opts.permissions.scripts ?? {}; const hasScriptOverride = Object.keys(_scripts).some( (k) => path.normalize(resolveScriptKey(k)) === path.normalize(argv0), ); - // Use default:"rwx" so only actual deny-pattern hits produce "deny". - // Without a default, permAllows(undefined, "exec") → false → every path - // not matched by a deny pattern would be incorrectly blocked. - const denyVerdict = checkAccessPolicy(argv0, "exec", { - deny: opts.permissions.deny, - default: "rwx", - }); - if (denyVerdict === "deny") { - throw new Error(`exec denied by access policy: ${argv0}`); - } if (!hasScriptOverride && checkAccessPolicy(argv0, "exec", effectivePermissions) === "deny") { throw new Error(`exec denied by access policy: ${argv0}`); } diff --git a/src/agents/pi-tools.read.edit-permission.test.ts b/src/agents/pi-tools.read.edit-permission.test.ts index b98b8411836..a5a0c632642 100644 --- a/src/agents/pi-tools.read.edit-permission.test.ts +++ b/src/agents/pi-tools.read.edit-permission.test.ts @@ -55,7 +55,6 @@ describe("createHostWorkspaceEditTool edit read-permission check", () => { // "-w-" policy: write allowed, read denied. // Edit must NOT be allowed to read the file even if write is permitted. const permissions: AccessPolicyConfig = { - default: "---", rules: { [`${tmpDir}/**`]: "-w-" }, }; createHostWorkspaceEditTool(tmpDir, { workspaceOnly: false, permissions }); @@ -73,7 +72,6 @@ describe("createHostWorkspaceEditTool edit read-permission check", () => { await fs.writeFile(filePath, "content", "utf8"); const permissions: AccessPolicyConfig = { - default: "---", rules: { [`${tmpDir}/**`]: "rw-" }, }; createHostWorkspaceEditTool(tmpDir, { workspaceOnly: false, permissions }); @@ -92,7 +90,6 @@ describe("createHostWorkspaceEditTool edit read-permission check", () => { // "r--" policy: read allowed, write denied. const permissions: AccessPolicyConfig = { - default: "---", rules: { [`${tmpDir}/**`]: "r--" }, }; createHostWorkspaceEditTool(tmpDir, { workspaceOnly: false, permissions }); diff --git a/src/config/types.tools.ts b/src/config/types.tools.ts index d7422fe3e41..41e6b933af6 100644 --- a/src/config/types.tools.ts +++ b/src/config/types.tools.ts @@ -10,8 +10,6 @@ export type PermStr = string; export type ScriptPolicyEntry = { /** Restrict/expand rules for this script. Merged over the base policy rules. */ rules?: Record; - /** Additional deny patterns added when this script runs (additive). */ - deny?: string[]; /** SHA-256 hex of the script file for integrity checking (best-effort, not atomic). */ sha256?: string; }; @@ -19,14 +17,13 @@ export type ScriptPolicyEntry = { /** * Filesystem RWX access-policy config loaded from `access-policy.json`. * Applied per-agent to read, write, and exec tool calls. + * + * Implicit fallback when no rule matches: `"---"` (deny-all). + * To set a permissive default, add a `"/**"` rule (e.g. `"/**": "r--"`). */ export type AccessPolicyConfig = { - /** Fallback permission when no rule matches. Defaults to `"---"` (deny-all) when absent. */ - default?: PermStr; /** Glob-pattern rules: path → permission string. Longest prefix wins. */ rules?: Record; - /** Patterns that are always denied regardless of rules (additive across merges). */ - deny?: string[]; /** Per-script argv0 policy overrides keyed by resolved binary path. */ scripts?: Record; }; diff --git a/src/infra/access-policy-file.test.ts b/src/infra/access-policy-file.test.ts index 6255ae48b96..807883b39c0 100644 --- a/src/infra/access-policy-file.test.ts +++ b/src/infra/access-policy-file.test.ts @@ -56,39 +56,15 @@ describe("mergeAccessPolicy", () => { }); it("returns base when override is undefined", () => { - const base = { default: "r--" }; + const base = { rules: { "/**": "r--" as const } }; expect(mergeAccessPolicy(base, undefined)).toEqual(base); }); it("returns override when base is undefined", () => { - const override = { default: "rwx" }; + const override = { rules: { "/**": "rwx" as const } }; expect(mergeAccessPolicy(undefined, override)).toEqual(override); }); - it("override default wins", () => { - const result = mergeAccessPolicy({ default: "r--" }, { default: "rwx" }); - expect(result?.default).toBe("rwx"); - }); - - it("base default survives when override has no default", () => { - const result = mergeAccessPolicy({ default: "r--" }, { rules: { "/**": "r-x" } }); - expect(result?.default).toBe("r--"); - }); - - it("deny arrays are concatenated — base denies cannot be removed", () => { - const result = mergeAccessPolicy( - { deny: ["~/.ssh/**", "~/.aws/**"] }, - { deny: ["~/.gnupg/**"] }, - ); - expect(result?.deny).toEqual(["~/.ssh/**", "~/.aws/**", "~/.gnupg/**"]); - }); - - it("override deny extends base deny", () => { - const result = mergeAccessPolicy({ deny: ["~/.ssh/**"] }, { deny: ["~/.env"] }); - expect(result?.deny).toContain("~/.ssh/**"); - expect(result?.deny).toContain("~/.env"); - }); - it("rules are shallow-merged, override key wins on collision", () => { const result = mergeAccessPolicy( { rules: { "/**": "r--", "~/**": "rw-" } }, @@ -99,9 +75,8 @@ describe("mergeAccessPolicy", () => { expect(result?.rules?.["~/dev/**"]).toBe("rwx"); // override adds }); - it("omits empty deny/rules from result", () => { - const result = mergeAccessPolicy({ default: "r--" }, { default: "rwx" }); - expect(result?.deny).toBeUndefined(); + it("omits empty rules from result", () => { + const result = mergeAccessPolicy({ scripts: { "/s.sh": { sha256: "abc" } } }, {}); expect(result?.rules).toBeUndefined(); }); @@ -113,16 +88,14 @@ describe("mergeAccessPolicy", () => { "/usr/local/bin/deploy.sh": { sha256: "abc123", rules: { "~/deploy/**": "rwx" as const }, - deny: ["~/.ssh/**"], }, }, }; const override = { scripts: { "/usr/local/bin/deploy.sh": { - // Agent block supplies same key — must NOT be able to drop sha256 or deny[]. + // Agent block supplies same key — must NOT be able to drop sha256. rules: { "~/deploy/**": "r--" as const }, // narrower override — fine - deny: ["~/extra-deny/**"], }, }, }; @@ -130,9 +103,6 @@ describe("mergeAccessPolicy", () => { const merged = result?.scripts?.["/usr/local/bin/deploy.sh"]; // sha256 from base must survive. expect(merged?.sha256).toBe("abc123"); - // deny[] must be additive — base deny cannot be removed. - expect(merged?.deny).toContain("~/.ssh/**"); - expect(merged?.deny).toContain("~/extra-deny/**"); // rules: override key wins on collision. expect(merged?.rules?.["~/deploy/**"]).toBe("r--"); }); @@ -148,17 +118,6 @@ describe("mergeAccessPolicy", () => { // New script from override is added. expect(result?.scripts?.["/bin/new.sh"]?.rules?.["/tmp/**"]).toBe("rwx"); }); - - it("scripts deep-merge: base deny[] cannot be removed by override supplying empty deny[]", () => { - const base = { - scripts: { "/bin/s.sh": { deny: ["~/.secrets/**"] } }, - }; - const override = { - scripts: { "/bin/s.sh": { deny: [] } }, // empty override deny — base must survive - }; - const result = mergeAccessPolicy(base, override); - expect(result?.scripts?.["/bin/s.sh"]?.deny).toContain("~/.secrets/**"); - }); }); // --------------------------------------------------------------------------- @@ -219,12 +178,12 @@ describe("loadAccessPolicyFile", () => { spy.mockRestore(); }); - it("returns BROKEN_POLICY_FILE and logs error when 'deny' is misplaced at top level", () => { + it("returns BROKEN_POLICY_FILE and logs error when 'scripts' is misplaced at top level", () => { const spy = vi.spyOn(console, "error").mockImplementation(() => {}); - writeFile({ version: 1, deny: ["~/.ssh/**"] }); + writeFile({ version: 1, scripts: { "/bin/s.sh": { sha256: "abc" } } }); const result = loadAccessPolicyFile(); expect(result).toBe(BROKEN_POLICY_FILE); - expect(spy).toHaveBeenCalledWith(expect.stringContaining('unexpected top-level key "deny"')); + expect(spy).toHaveBeenCalledWith(expect.stringContaining('unexpected top-level key "scripts"')); spy.mockRestore(); }); @@ -240,7 +199,7 @@ describe("loadAccessPolicyFile", () => { it("returns parsed file when valid", () => { const content: AccessPolicyFile = { version: 1, - base: { default: "r--", deny: ["~/.ssh/**"] }, + base: { rules: { "/**": "r--", "~/.ssh/**": "---" } }, agents: { subri: { rules: { "~/dev/**": "rwx" } } }, }; writeFile(content); @@ -251,7 +210,7 @@ describe("loadAccessPolicyFile", () => { throw new Error("unexpected"); } expect(result.version).toBe(1); - expect(result.base?.default).toBe("r--"); + expect(result.base?.rules?.["/**"]).toBe("r--"); expect(result.agents?.subri?.rules?.["~/dev/**"]).toBe("rwx"); }); }); @@ -262,7 +221,7 @@ describe("loadAccessPolicyFile", () => { describe("loadAccessPolicyFile — mtime cache", () => { it("returns cached result on second call without re-reading the file", () => { - writeFile({ version: 1, base: { default: "r--" } }); + writeFile({ version: 1, base: { rules: { "/**": "r--" } } }); const spy = vi.spyOn(fs, "readFileSync"); loadAccessPolicyFile(); // populate cache loadAccessPolicyFile(); // should hit cache @@ -272,11 +231,11 @@ describe("loadAccessPolicyFile — mtime cache", () => { }); it("re-reads when mtime changes (file updated)", () => { - writeFile({ version: 1, base: { default: "r--" } }); + writeFile({ version: 1, base: { rules: { "/**": "r--" } } }); loadAccessPolicyFile(); // populate cache // Rewrite the file — on most filesystems this bumps mtime. Force a detectable // mtime change by setting it explicitly via utimesSync. - writeFile({ version: 1, base: { default: "rwx" } }); + writeFile({ version: 1, base: { rules: { "/**": "rwx" } } }); const future = Date.now() / 1000 + 1; fs.utimesSync(FP_FILE, future, future); const result = loadAccessPolicyFile(); @@ -284,7 +243,7 @@ describe("loadAccessPolicyFile — mtime cache", () => { if (result === null || result === BROKEN_POLICY_FILE) { throw new Error("unexpected"); } - expect(result.base?.default).toBe("rwx"); + expect(result.base?.rules?.["/**"]).toBe("rwx"); }); it("clears cache when file is deleted", () => { @@ -328,7 +287,7 @@ describe("resolveAccessPolicyForAgent", () => { it("does not warn when config file exists and is valid", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); - writeFile({ version: 1, base: { default: "r--" } }); + writeFile({ version: 1, base: { rules: { "/**": "r--" } } }); resolveAccessPolicyForAgent("subri"); expect(warnSpy).not.toHaveBeenCalled(); warnSpy.mockRestore(); @@ -341,8 +300,8 @@ describe("resolveAccessPolicyForAgent", () => { const result = resolveAccessPolicyForAgent("subri"); expect(warnSpy).not.toHaveBeenCalled(); expect(errSpy).toHaveBeenCalledWith(expect.stringContaining("Failing closed")); - // Broken file must fail-closed: deny-all policy, not undefined - expect(result).toEqual({ default: "---" }); + // Broken file must fail-closed: deny-all policy (empty rules = implicit "---"), not undefined + expect(result).toEqual({}); warnSpy.mockRestore(); errSpy.mockRestore(); }); @@ -351,50 +310,48 @@ describe("resolveAccessPolicyForAgent", () => { const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); writeFile({ version: 1, rules: { "/**": "r--" } }); // misplaced key — broken const result = resolveAccessPolicyForAgent("subri"); - expect(result).toEqual({ default: "---" }); + expect(result).toEqual({}); // Attempt to mutate the returned object — must not affect the next call. // If DENY_ALL_POLICY is not frozen this would silently corrupt it. try { - (result as Record)["default"] = "rwx"; + (result as Record)["rules"] = { "/**": "rwx" }; } catch { // Object.freeze throws in strict mode — that's fine too. } _resetNotFoundWarnedForTest(); const result2 = resolveAccessPolicyForAgent("subri"); - expect(result2).toEqual({ default: "---" }); + expect(result2).toEqual({}); errSpy.mockRestore(); }); it("returns base when no agent block exists", () => { writeFile({ version: 1, - base: { default: "r--", deny: ["~/.ssh/**"] }, + base: { rules: { "/**": "r--", [`~/.ssh/**`]: "---" } }, }); const result = resolveAccessPolicyForAgent("subri"); - expect(result?.default).toBe("r--"); - expect(result?.deny).toContain("~/.ssh/**"); + expect(result?.rules?.["/**"]).toBe("r--"); + expect(result?.rules?.["~/.ssh/**"]).toBe("---"); }); it("merges base + named agent", () => { writeFile({ version: 1, - base: { default: "---", deny: ["~/.ssh/**"], rules: { "/**": "r--" } }, - agents: { subri: { rules: { "~/dev/**": "rwx" }, default: "r--" } }, + base: { rules: { "/**": "r--", [`~/.ssh/**`]: "---" } }, + agents: { subri: { rules: { "~/dev/**": "rwx" } } }, }); const result = resolveAccessPolicyForAgent("subri"); - // default: agent wins - expect(result?.default).toBe("r--"); - // deny: additive - expect(result?.deny).toContain("~/.ssh/**"); - // rules: merged + // rules: merged, agent rule wins on collision expect(result?.rules?.["/**"]).toBe("r--"); expect(result?.rules?.["~/dev/**"]).toBe("rwx"); + // base "---" rule preserved + expect(result?.rules?.["~/.ssh/**"]).toBe("---"); }); it("wildcard agent applies before named agent", () => { writeFile({ version: 1, - base: { default: "---" }, + base: {}, agents: { "*": { rules: { "/usr/bin/**": "r-x" } }, subri: { rules: { "~/dev/**": "rwx" } }, @@ -403,27 +360,26 @@ describe("resolveAccessPolicyForAgent", () => { const result = resolveAccessPolicyForAgent("subri"); expect(result?.rules?.["/usr/bin/**"]).toBe("r-x"); // from wildcard expect(result?.rules?.["~/dev/**"]).toBe("rwx"); // from named agent - expect(result?.default).toBe("---"); // from base }); it("wildcard applies even when no named agent block", () => { writeFile({ version: 1, - base: { default: "---" }, - agents: { "*": { deny: ["~/.ssh/**"] } }, + base: {}, + agents: { "*": { rules: { [`~/.ssh/**`]: "---" } } }, }); const result = resolveAccessPolicyForAgent("other-agent"); - expect(result?.deny).toContain("~/.ssh/**"); + expect(result?.rules?.["~/.ssh/**"]).toBe("---"); }); it("wildcard key itself is not treated as a named agent", () => { writeFile({ version: 1, - agents: { "*": { deny: ["~/.ssh/**"] } }, + agents: { "*": { rules: { [`~/.ssh/**`]: "---" } } }, }); // Requesting agentId "*" should not double-apply wildcard as named const result = resolveAccessPolicyForAgent("*"); - expect(result?.deny).toEqual(["~/.ssh/**"]); + expect(result?.rules?.["~/.ssh/**"]).toBe("---"); }); it("returns undefined when file is empty (no base, no agents)", () => { @@ -470,14 +426,14 @@ describe("resolveAccessPolicyForAgent", () => { errSpy.mockRestore(); }); - it("named agent deny extends global deny — global deny cannot be removed", () => { + it("narrowing rules from base and agent are all preserved in merged result", () => { writeFile({ version: 1, - base: { deny: ["~/.ssh/**"] }, - agents: { paranoid: { deny: ["~/.aws/**"] } }, + base: { rules: { [`~/.ssh/**`]: "---" } }, + agents: { paranoid: { rules: { [`~/.aws/**`]: "---" } } }, }); const result = resolveAccessPolicyForAgent("paranoid"); - expect(result?.deny).toContain("~/.ssh/**"); - expect(result?.deny).toContain("~/.aws/**"); + expect(result?.rules?.["~/.ssh/**"]).toBe("---"); + expect(result?.rules?.["~/.aws/**"]).toBe("---"); }); }); diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index 87c613ffad3..915b9880d86 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -15,9 +15,8 @@ export type AccessPolicyFile = { * base → agents["*"] → agents[agentId] * * Within each layer: - * - deny: additive (concat) — a base deny entry can never be removed by an override * - rules: shallow-merge, override key wins on collision - * - default: override wins if set + * - scripts: deep-merge per key; base sha256 is preserved */ agents?: Record; }; @@ -31,9 +30,8 @@ export function resolveAccessPolicyPath(): string { /** * Merge two AccessPolicyConfig layers. - * - deny: additive (cannot remove a base deny) * - rules: shallow merge, override key wins - * - default: override wins if set + * - scripts: deep-merge per key; base sha256 is preserved (cannot be removed by override) */ export function mergeAccessPolicy( base: AccessPolicyConfig | undefined, @@ -48,12 +46,11 @@ export function mergeAccessPolicy( if (!override) { return base; } - const deny = [...(base.deny ?? []), ...(override.deny ?? [])]; const rules = { ...base.rules, ...override.rules }; - // scripts: deep-merge per key — base sha256 and deny[] are preserved regardless of + // scripts: deep-merge per key — base sha256 is preserved regardless of // what the agent override supplies. A plain spread ({ ...base.scripts, ...override.scripts }) - // would silently drop the admin-configured hash integrity check and per-script deny list - // when an agent block supplies the same script key, defeating the security intent. + // would silently drop the admin-configured hash integrity check when an agent block + // supplies the same script key, defeating the security intent. const mergedScripts: NonNullable = { ...base.scripts }; for (const [key, overrideEntry] of Object.entries(override.scripts ?? {})) { const baseEntry = base.scripts?.[key]; @@ -61,7 +58,6 @@ export function mergeAccessPolicy( mergedScripts[key] = overrideEntry; continue; } - const entryDeny = [...(baseEntry.deny ?? []), ...(overrideEntry.deny ?? [])]; mergedScripts[key] = { // sha256: base always wins — cannot be removed or replaced by an agent override. ...(baseEntry.sha256 !== undefined ? { sha256: baseEntry.sha256 } : {}), @@ -69,26 +65,16 @@ export function mergeAccessPolicy( ...(Object.keys({ ...baseEntry.rules, ...overrideEntry.rules }).length > 0 ? { rules: { ...baseEntry.rules, ...overrideEntry.rules } } : {}), - // deny: additive — base per-script deny cannot be removed. - ...(entryDeny.length > 0 ? { deny: entryDeny } : {}), }; } const scripts = Object.keys(mergedScripts).length > 0 ? mergedScripts : undefined; const result: AccessPolicyConfig = {}; - if (deny.length > 0) { - result.deny = deny; - } if (Object.keys(rules).length > 0) { result.rules = rules; } if (scripts) { result.scripts = scripts; } - if (override.default !== undefined) { - result.default = override.default; - } else if (base.default !== undefined) { - result.default = base.default; - } return result; } @@ -119,8 +105,8 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s } // Catch common mistake: AccessPolicyConfig fields accidentally at top level - // (e.g. user puts "rules" or "deny" directly instead of under "base"). - for (const key of ["rules", "deny", "default", "scripts"] as const) { + // (e.g. user puts "rules" or "scripts" directly instead of under "base"). + for (const key of ["rules", "scripts"] as const) { if (p[key] !== undefined) { errors.push( `${filePath}: unexpected top-level key "${key}" — did you mean to put it under "base"?`, @@ -249,8 +235,9 @@ export function _resetNotFoundWarnedForTest(): void { * Logs errors on invalid perm strings but does not throw — bad strings fall back to * deny-all for that entry (handled downstream by checkAccessPolicy's permAllows logic). */ -/** Deny-all policy returned when the policy file is present but broken (fail-closed). */ -const DENY_ALL_POLICY: AccessPolicyConfig = Object.freeze({ default: "---" }); +/** Deny-all policy returned when the policy file is present but broken (fail-closed). + * Empty rules + implicit "---" fallback = deny everything. */ +const DENY_ALL_POLICY: AccessPolicyConfig = Object.freeze({}); export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfig | undefined { const file = loadAccessPolicyFile(); diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index f92f0c63b3d..20d0a55643f 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -34,9 +34,7 @@ describe("validateAccessPolicyConfig", () => { it("returns no errors for a valid config", () => { expect( validateAccessPolicyConfig({ - rules: { "/**": "r--", [`${HOME}/**`]: "rwx" }, - deny: [`${HOME}/.ssh/**`], - default: "---", + rules: { "/**": "r--", [`${HOME}/**`]: "rwx", [`${HOME}/.ssh/**`]: "---" }, }), ).toEqual([]); }); @@ -45,24 +43,6 @@ describe("validateAccessPolicyConfig", () => { expect(validateAccessPolicyConfig({})).toEqual([]); }); - it("rejects invalid default perm string — too short", () => { - const errs = validateAccessPolicyConfig({ default: "rw" }); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/default/); - }); - - it("rejects invalid default perm string — too long", () => { - const errs = validateAccessPolicyConfig({ default: "rwxr" }); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/default/); - }); - - it("rejects invalid default perm string — wrong chars", () => { - const errs = validateAccessPolicyConfig({ default: "rq-" }); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/default/); - }); - it("rejects invalid rule perm value", () => { const errs = validateAccessPolicyConfig({ rules: { "/**": "rx" } }); expect(errs).toHaveLength(1); @@ -75,90 +55,17 @@ describe("validateAccessPolicyConfig", () => { expect(errs[0]).toMatch(/rules/); }); - it("reports multiple errors when both default and a rule are invalid", () => { - const errs = validateAccessPolicyConfig({ - default: "bad", - rules: { "/**": "xyz" }, - }); - expect(errs.length).toBeGreaterThanOrEqual(2); + it("reports an error when a rule perm value is invalid", () => { + const errs = validateAccessPolicyConfig({ rules: { "/**": "xyz" } }); + expect(errs.length).toBeGreaterThanOrEqual(1); }); - it("rejects empty deny entry", () => { - const errs = validateAccessPolicyConfig({ deny: [""] }); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/deny/); - }); - - it("auto-expands a bare directory path in deny[] to /**", () => { - const dir = os.tmpdir(); - const config: AccessPolicyConfig = { deny: [dir] }; - const errs = validateAccessPolicyConfig(config); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/auto-expanded/); - expect(config.deny?.[0]).toBe(`${dir}/**`); - }); - - it("auto-expands bare deny[] entry even when the directory does not yet exist", () => { - // Non-existent paths are treated as future directories and expanded to /**. - const nonExistent = path.join(os.tmpdir(), "openclaw-test-nonexistent-" + Date.now()); - const config: AccessPolicyConfig = { deny: [nonExistent] }; - const errs = validateAccessPolicyConfig(config); - expect(config.deny?.[0]).toBe(`${nonExistent}/**`); - expect(errs[0]).toMatch(/auto-expanded/); - }); - - it("auto-expands non-existent versioned directory names (v1.0, app-2.3) in deny[]", () => { - // Versioned names like "v1.0" or "pkg-1.0.0" look like files via naive dot-detection - // but are almost always directories. The tightened heuristic requires the extension - // to contain at least one letter — digits-only extensions (like ".0") are treated as - // directory-like and expanded to /**. - const base = os.tmpdir(); - const versionedDir = path.join(base, `openclaw-test-v1.0-${Date.now()}`); - const config: AccessPolicyConfig = { deny: [versionedDir] }; - validateAccessPolicyConfig(config); - expect(config.deny?.[0]).toBe(`${versionedDir}/**`); // must be expanded - }); - - it("does NOT auto-expand a non-existent deny[] path that looks like a file (has extension)", () => { - // "~/future-secrets.key" doesn't exist yet but the extension heuristic should - // prevent expansion to "~/future-secrets.key/**" — the user intends to protect - // the file itself, not a subtree of non-existent children. - const fileLikePath = path.join(os.tmpdir(), `openclaw-test-${Date.now()}.key`); - const config: AccessPolicyConfig = { deny: [fileLikePath] }; - validateAccessPolicyConfig(config); - expect(config.deny?.[0]).toBe(fileLikePath); // must NOT be expanded to /** - }); - - it("auto-expands non-existent bare dotnames (.ssh, .aws, .env) to /** — treats as future directory", () => { - // Bare dotnames without a secondary extension (.ssh, .aws, .env, .gnupg) cannot be - // reliably identified as file vs. directory before they exist. The safe choice is to - // expand to /** so the subtree is protected if a directory is created there. - // When the path later exists as a file, statSync confirms it and the bare pattern is kept. - const base = os.tmpdir(); - for (const name of [".ssh", ".aws", ".env", ".netrc", ".gnupg", ".config"]) { - _resetAutoExpandedWarnedForTest(); - const p = path.join(base, name); - const config: AccessPolicyConfig = { deny: [p] }; - validateAccessPolicyConfig(config); - expect(config.deny?.[0]).toBe(`${p}/**`); // expanded to protect subtree - } - }); - - it("does NOT auto-expand a bare deny[] entry that is an existing file", () => { - // A specific file like "~/.ssh/id_rsa" must stay as an exact-match pattern. - // Expanding it to "~/.ssh/id_rsa/**" would only match non-existent children, - // leaving the file itself unprotected at the tool layer and in bwrap. - // process.execPath is the running node/bun binary — always a real file. + it("file-specific '---' rule blocks access via checkAccessPolicy", () => { + // A "---" rule on a specific file path must block reads at the tool layer. const file = process.execPath; - const config: AccessPolicyConfig = { deny: [file] }; - validateAccessPolicyConfig(config); - expect(config.deny?.[0]).toBe(file); // kept as bare path, not expanded - }); - - it("file-specific deny[] entry blocks access to the file via checkAccessPolicy", () => { - // Regression: bare file path in deny[] must block reads at the tool layer. - const file = process.execPath; - const config: AccessPolicyConfig = { default: "rwx", deny: [file] }; + const config: AccessPolicyConfig = { + rules: { "/**": "rwx", [file]: "---" }, + }; validateAccessPolicyConfig(config); // applies normalization in-place expect(checkAccessPolicy(file, "read", config)).toBe("deny"); }); @@ -177,22 +84,10 @@ describe("validateAccessPolicyConfig", () => { expect(errs.some((e) => e.includes("rwX") && e.includes("scripts"))).toBe(true); }); - it("validates scripts[].deny entries and emits diagnostics for empty patterns", () => { - const config: AccessPolicyConfig = { - scripts: { - "/usr/local/bin/deploy.sh": { - deny: ["", "~/.secrets/**"], // first entry is invalid empty string - }, - }, - }; - const errs = validateAccessPolicyConfig(config); - expect(errs.some((e) => e.includes("scripts") && e.includes("deny"))).toBe(true); - }); - - it("accepts valid 'rwx' and '---' perm strings", () => { - expect(validateAccessPolicyConfig({ default: "rwx" })).toEqual([]); - expect(validateAccessPolicyConfig({ default: "---" })).toEqual([]); - expect(validateAccessPolicyConfig({ default: "r-x" })).toEqual([]); + it("accepts valid rule perm strings", () => { + expect(validateAccessPolicyConfig({ rules: { "/**": "rwx" } })).toEqual([]); + expect(validateAccessPolicyConfig({ rules: { "/**": "---" } })).toEqual([]); + expect(validateAccessPolicyConfig({ rules: { "/**": "r-x" } })).toEqual([]); }); it("auto-expands a bare path that points to a real directory", () => { @@ -283,22 +178,11 @@ describe("validateAccessPolicyConfig", () => { expect(second.filter((e) => e.includes("mid-path wildcard"))).toHaveLength(0); }); - it("emits a one-time diagnostic for mid-path wildcard deny[] entries", () => { - _resetMidPathWildcardWarnedForTest(); - const errs = validateAccessPolicyConfig({ - deny: ["/tmp/*/sensitive/**"], - }); - expect(errs).toHaveLength(1); - expect(errs[0]).toMatch(/mid-path wildcard/); - expect(errs[0]).toMatch(/OS-level.*enforcement/); - }); - it("does NOT emit mid-path wildcard diagnostic for final-segment wildcards", () => { _resetMidPathWildcardWarnedForTest(); // "/home/user/**" — wildcard is in the final segment, no path separator follows. const errs = validateAccessPolicyConfig({ - rules: { "/home/user/**": "r--", "~/**": "rwx" }, - deny: ["/tmp/**"], + rules: { "/home/user/**": "r--", "~/**": "rwx", "/tmp/**": "---" }, }); expect(errs.filter((e) => e.includes("mid-path wildcard"))).toHaveLength(0); }); @@ -311,17 +195,17 @@ describe("validateAccessPolicyConfig", () => { describe("checkAccessPolicy — malformed permission characters fail closed", () => { it("treats a typo like 'r1-' as deny for write (only exact 'w' grants write)", () => { // "r1-": index 1 is "1", not "w" — must deny write, not allow it. - const config = { rules: { "/tmp/**": "r1-" as unknown as "r--" }, default: "---" }; + const config = { rules: { "/tmp/**": "r1-" as unknown as "r--" } }; expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("deny"); }); it("treats 'R--' (uppercase) as deny for read (only lowercase 'r' grants read)", () => { - const config = { rules: { "/tmp/**": "R--" as unknown as "r--" }, default: "---" }; + const config = { rules: { "/tmp/**": "R--" as unknown as "r--" } }; expect(checkAccessPolicy("/tmp/foo.txt", "read", config)).toBe("deny"); }); it("treats 'rWx' (uppercase W) as deny for write", () => { - const config = { rules: { "/tmp/**": "rWx" as unknown as "rwx" }, default: "---" }; + const config = { rules: { "/tmp/**": "rWx" as unknown as "rwx" } }; expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("deny"); }); }); @@ -332,29 +216,28 @@ describe("checkAccessPolicy — malformed permission characters fail closed", () describe("checkAccessPolicy — trailing slash shorthand", () => { it('"/tmp/" is equivalent to "/tmp/**"', () => { - const config: AccessPolicyConfig = { rules: { "/tmp/": "rwx" }, default: "---" }; + const config: AccessPolicyConfig = { rules: { "/tmp/": "rwx" } }; expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("allow"); expect(checkAccessPolicy("/tmp/a/b/c", "write", config)).toBe("allow"); }); it('"~/" is equivalent to "~/**"', () => { - const config: AccessPolicyConfig = { rules: { "~/": "rw-" }, default: "---" }; + const config: AccessPolicyConfig = { rules: { "~/": "rw-" } }; expect(checkAccessPolicy(`${HOME}/foo.txt`, "read", config)).toBe("allow"); expect(checkAccessPolicy(`${HOME}/foo.txt`, "write", config)).toBe("allow"); expect(checkAccessPolicy(`${HOME}/foo.txt`, "exec", config)).toBe("deny"); }); - it("trailing slash in deny list blocks subtree", () => { + it('"---" rule with trailing slash blocks subtree', () => { const config: AccessPolicyConfig = { - rules: { "/**": "rwx" }, - deny: [`${HOME}/.ssh/`], + rules: { "/**": "rwx", [`${HOME}/.ssh/`]: "---" }, }; expect(checkAccessPolicy(`${HOME}/.ssh/id_rsa`, "read", config)).toBe("deny"); }); it("trailing slash and /** produce identical results", () => { - const withSlash: AccessPolicyConfig = { rules: { "/tmp/": "rwx" }, default: "---" }; - const withGlob: AccessPolicyConfig = { rules: { "/tmp/**": "rwx" }, default: "---" }; + const withSlash: AccessPolicyConfig = { rules: { "/tmp/": "rwx" } }; + const withGlob: AccessPolicyConfig = { rules: { "/tmp/**": "rwx" } }; const paths = ["/tmp/a", "/tmp/a/b", "/tmp/a/b/c.txt"]; for (const p of paths) { expect(checkAccessPolicy(p, "write", withSlash)).toBe( @@ -368,7 +251,6 @@ describe("checkAccessPolicy — trailing slash shorthand", () => { // path ~/.openclaw/heartbeat (no trailing component), not just its contents. const config: AccessPolicyConfig = { rules: { "/**": "r--", [`${HOME}/.openclaw/heartbeat/`]: "rw-" }, - default: "---", }; expect(checkAccessPolicy(`${HOME}/.openclaw/heartbeat`, "write", config)).toBe("allow"); expect(checkAccessPolicy(`${HOME}/.openclaw/heartbeat/test.txt`, "write", config)).toBe( @@ -376,10 +258,9 @@ describe("checkAccessPolicy — trailing slash shorthand", () => { ); }); - it("trailing slash in deny list blocks the directory itself", () => { + it('"---" trailing-slash rule blocks the directory itself and its contents', () => { const config: AccessPolicyConfig = { - rules: { "/**": "rwx" }, - deny: [`${HOME}/.ssh/`], + rules: { "/**": "rwx", [`${HOME}/.ssh/`]: "---" }, }; // Both the directory and its contents should be denied. expect(checkAccessPolicy(`${HOME}/.ssh`, "read", config)).toBe("deny"); @@ -400,7 +281,6 @@ describe.skipIf(process.platform !== "darwin")( "/var/**": "r--", "/etc/**": "r--", }, - default: "---", }; it("/private/tmp path is treated as /tmp — write allowed", () => { @@ -419,19 +299,17 @@ describe.skipIf(process.platform !== "darwin")( expect(checkAccessPolicy("/tmp/foo.txt", "write", config)).toBe("allow"); }); - it("deny list entry /tmp/** also blocks /private/tmp/**", () => { + it('"---" rule for /tmp/** also blocks /private/tmp/**', () => { const denyConfig: AccessPolicyConfig = { - deny: ["/tmp/**"], - rules: { "/**": "rwx" }, + rules: { "/**": "rwx", "/tmp/**": "---" }, }; expect(checkAccessPolicy("/private/tmp/evil.sh", "exec", denyConfig)).toBe("deny"); }); - it("/private/tmp/** pattern in deny list blocks /tmp/** target", () => { - // Pattern written with /private/tmp must still match the normalized /tmp target. + it("/private/tmp/** deny rule blocks /tmp/** target", () => { + // Rule written with /private/tmp must still match the normalized /tmp target. const denyConfig: AccessPolicyConfig = { - deny: ["/private/tmp/**"], - rules: { "/**": "rwx" }, + rules: { "/**": "rwx", "/private/tmp/**": "---" }, }; expect(checkAccessPolicy("/tmp/evil.sh", "read", denyConfig)).toBe("deny"); }); @@ -439,7 +317,6 @@ describe.skipIf(process.platform !== "darwin")( it("/private/tmp/** rule matches /tmp/** target", () => { // Rule written with /private/* prefix must match a /tmp/* target path. const cfg: AccessPolicyConfig = { - default: "---", rules: { "/private/tmp/**": "rwx" }, }; expect(checkAccessPolicy("/tmp/foo.txt", "write", cfg)).toBe("allow"); @@ -500,15 +377,13 @@ describe("findBestRule", () => { }); // --------------------------------------------------------------------------- -// checkAccessPolicy — deny list +// checkAccessPolicy — "---" rules act as deny // --------------------------------------------------------------------------- -describe("checkAccessPolicy — deny list", () => { - it("deny always blocks, even when a rule would allow", () => { +describe('checkAccessPolicy — "---" rules act as deny', () => { + it('"---" rule blocks all ops, even when a broader rule would allow', () => { const config: AccessPolicyConfig = { - rules: { "/**": "rwx" }, - deny: [`${HOME}/.ssh/**`], - default: "rwx", + rules: { "/**": "rwx", [`${HOME}/.ssh/**`]: "---" }, }; expect(checkAccessPolicy(`${HOME}/.ssh/id_rsa`, "read", config)).toBe("deny"); expect(checkAccessPolicy(`${HOME}/.ssh/id_rsa`, "write", config)).toBe("deny"); @@ -516,20 +391,18 @@ describe("checkAccessPolicy — deny list", () => { }); it.skipIf(process.platform === "win32")( - "deny does not affect paths outside the deny glob", + '"---" rule does not affect paths outside its glob', () => { const config: AccessPolicyConfig = { - rules: { "/**": "rwx" }, - deny: [`${HOME}/.ssh/**`], + rules: { "/**": "rwx", [`${HOME}/.ssh/**`]: "---" }, }; expect(checkAccessPolicy(`${HOME}/workspace/foo.py`, "read", config)).toBe("allow"); }, ); - it("multiple deny entries — first match blocks", () => { + it("multiple narrowing rules block distinct subtrees", () => { const config: AccessPolicyConfig = { - rules: { "/**": "rwx" }, - deny: [`${HOME}/.ssh/**`, `${HOME}/.gnupg/**`], + rules: { "/**": "rwx", [`${HOME}/.ssh/**`]: "---", [`${HOME}/.gnupg/**`]: "---" }, }; expect(checkAccessPolicy(`${HOME}/.gnupg/secring.gpg`, "read", config)).toBe("deny"); }); @@ -599,30 +472,30 @@ describe("checkAccessPolicy — rules", () => { }); // --------------------------------------------------------------------------- -// checkAccessPolicy — default +// checkAccessPolicy — implicit fallback to "---" // --------------------------------------------------------------------------- -describe("checkAccessPolicy — default", () => { - it("uses default when no rule matches", () => { +describe("checkAccessPolicy — implicit fallback to ---", () => { + it("denies all ops when no rule matches (implicit --- fallback)", () => { const config: AccessPolicyConfig = { rules: { [`${HOME}/**`]: "rwx" }, - default: "r--", + }; + expect(checkAccessPolicy("/etc/passwd", "read", config)).toBe("deny"); + expect(checkAccessPolicy("/etc/passwd", "write", config)).toBe("deny"); + expect(checkAccessPolicy("/etc/passwd", "exec", config)).toBe("deny"); + }); + + it('"/**" rule acts as catch-all for unmatched paths', () => { + const config: AccessPolicyConfig = { + rules: { [`${HOME}/**`]: "rwx", "/**": "r--" }, }; expect(checkAccessPolicy("/etc/passwd", "read", config)).toBe("allow"); expect(checkAccessPolicy("/etc/passwd", "write", config)).toBe("deny"); }); - it("absent default is treated as --- (deny all)", () => { - const config: AccessPolicyConfig = { - rules: { [`${HOME}/**`]: "rwx" }, - }; - expect(checkAccessPolicy("/etc/passwd", "read", config)).toBe("deny"); - }); - - it("default --- denies all ops on unmatched paths", () => { + it("empty rules deny everything via implicit fallback", () => { const config: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "rw-" }, - default: "---", }; expect(checkAccessPolicy("/tmp/foo", "read", config)).toBe("deny"); expect(checkAccessPolicy("/tmp/foo", "write", config)).toBe("deny"); @@ -635,27 +508,26 @@ describe("checkAccessPolicy — default", () => { // --------------------------------------------------------------------------- describe("checkAccessPolicy — precedence integration", () => { - it("deny beats rules beats default — all three in play", () => { + it("narrowing rule beats broader allow — all in play", () => { const config: AccessPolicyConfig = { rules: { "/**": "r--", [`${HOME}/**`]: "rwx", + [`${HOME}/.ssh/**`]: "---", }, - deny: [`${HOME}/.ssh/**`], - default: "---", }; - // Rule allows home paths + // Broader home rule allows writes expect(checkAccessPolicy(`${HOME}/workspace/foo`, "write", config)).toBe("allow"); - // Deny beats the home rule + // Narrowing "---" beats the home rwx rule expect(checkAccessPolicy(`${HOME}/.ssh/id_rsa`, "read", config)).toBe("deny"); - // Outer rule applies outside home + // Outer "/**" rule applies outside home expect(checkAccessPolicy("/etc/hosts", "read", config)).toBe("allow"); expect(checkAccessPolicy("/etc/hosts", "write", config)).toBe("deny"); - // Nothing matches /proc → default --- - expect(checkAccessPolicy("/proc/self/mem", "read", config)).toBe("allow"); // matches /** + // "/proc/self/mem" matches "/**" (r--) + expect(checkAccessPolicy("/proc/self/mem", "read", config)).toBe("allow"); }); - it("empty config denies everything (no rules, no default)", () => { + it("empty config denies everything (implicit --- fallback)", () => { const config: AccessPolicyConfig = {}; expect(checkAccessPolicy("/anything", "read", config)).toBe("deny"); expect(checkAccessPolicy("/anything", "write", config)).toBe("deny"); @@ -672,13 +544,11 @@ describe("checkAccessPolicy — precedence integration", () => { // --------------------------------------------------------------------------- describe("checkAccessPolicy — symlink resolved-path scenarios", () => { - it("denies read on resolved symlink target that falls under deny list", () => { - // ~/workspace/link → ~/.ssh/id_rsa (symlink in allowed dir to denied file) - // Caller passes the resolved path; deny wins. + it('denies read on resolved symlink target covered by "---" rule', () => { + // ~/workspace/link → ~/.ssh/id_rsa (symlink in allowed dir to denied-subpath) + // Caller passes the resolved path; the "---" rule wins. const config: AccessPolicyConfig = { - rules: { [`${HOME}/workspace/**`]: "rw-" }, - deny: [`${HOME}/.ssh/**`], - default: "---", + rules: { [`${HOME}/workspace/**`]: "rw-", [`${HOME}/.ssh/**`]: "---" }, }; expect(checkAccessPolicy(`${HOME}/.ssh/id_rsa`, "read", config, HOME)).toBe("deny"); }); @@ -691,7 +561,6 @@ describe("checkAccessPolicy — symlink resolved-path scenarios", () => { [`${HOME}/workspace/**`]: "rw-", [`${HOME}/workspace/secret/**`]: "r--", }, - default: "---", }; expect(checkAccessPolicy(`${HOME}/workspace/secret/file.txt`, "write", config, HOME)).toBe( "deny", @@ -702,13 +571,10 @@ describe("checkAccessPolicy — symlink resolved-path scenarios", () => { ); }); - it("symlink source path in allowed dir would be allowed; resolved denied target is denied", () => { + it("symlink source path in allowed dir is allowed; resolved denied target is denied", () => { // This illustrates that the policy must be checked on the resolved path. - // The symlink path itself looks allowed; the real target does not. const config: AccessPolicyConfig = { - rules: { [`${HOME}/workspace/**`]: "rw-" }, - deny: [`${HOME}/.aws/**`], - default: "---", + rules: { [`${HOME}/workspace/**`]: "rw-", [`${HOME}/.aws/**`]: "---" }, }; // Source path (the symlink) — allowed expect(checkAccessPolicy(`${HOME}/workspace/creds`, "read", config, HOME)).toBe("allow"); @@ -980,7 +846,7 @@ describe("resolveScriptKey", () => { describe("applyScriptPolicyOverride", () => { it("returns base policy unchanged when no scripts block", () => { - const base: AccessPolicyConfig = { rules: { "/**": "r--" }, default: "---" }; + const base: AccessPolicyConfig = { rules: { "/**": "r--" } }; const { policy, hashMismatch } = applyScriptPolicyOverride(base, "/any/path"); expect(hashMismatch).toBeUndefined(); expect(policy).toBe(base); @@ -1021,10 +887,9 @@ describe("applyScriptPolicyOverride", () => { } }); - it("returns override rules separately so seatbelt emits them after deny", () => { + it("returns override rules separately so seatbelt emits them after base rules", () => { const base: AccessPolicyConfig = { rules: { "/**": "r--" }, - default: "---", scripts: { "/my/script.sh": { rules: { [`${HOME}/.openclaw/credentials/`]: "r--" } } }, }; const { policy, overrideRules, hashMismatch } = applyScriptPolicyOverride( @@ -1035,23 +900,11 @@ describe("applyScriptPolicyOverride", () => { // Base rules unchanged in policy expect(policy.rules?.["/**"]).toBe("r--"); expect(policy.rules?.[`${HOME}/.openclaw/credentials/`]).toBeUndefined(); - // Override rules returned separately — caller emits them after deny in seatbelt profile + // Override rules returned separately — caller emits them last in seatbelt profile expect(overrideRules?.[`${HOME}/.openclaw/credentials/`]).toBe("r--"); expect(policy.scripts).toBeUndefined(); }); - it("appends deny additively", () => { - const base: AccessPolicyConfig = { - deny: [`${HOME}/.ssh/**`], - scripts: { - "/my/script.sh": { deny: ["/tmp/**"] }, - }, - }; - const { policy } = applyScriptPolicyOverride(base, "/my/script.sh"); - expect(policy.deny).toContain(`${HOME}/.ssh/**`); - expect(policy.deny).toContain("/tmp/**"); - }); - it("override rules returned separately — base policy rule unchanged", () => { const base: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "r--" }, @@ -1099,7 +952,7 @@ describe("applyScriptPolicyOverride", () => { // A direct object lookup misses tilde keys; ~ must be expanded before comparing. const absPath = path.join(os.homedir(), "bin", "deploy.sh"); const base: AccessPolicyConfig = { - default: "rwx", + rules: { "/**": "rwx" }, scripts: { "~/bin/deploy.sh": { rules: { "/secret/**": "---" } } }, }; const { overrideRules, hashMismatch } = applyScriptPolicyOverride(base, absPath); diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 9081f96cac3..e6fc9adf25e 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -43,17 +43,11 @@ function hasMidPathWildcard(pattern: string): boolean { /** * Validates and normalizes an AccessPolicyConfig for well-formedness. * Returns an array of human-readable diagnostic strings; empty = valid. - * May mutate config.rules and config.deny in place (e.g. auto-expanding bare directory paths). + * May mutate config.rules in place (e.g. auto-expanding bare directory paths). */ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] { const errors: string[] = []; - if (config.default !== undefined && !PERM_STR_RE.test(config.default)) { - errors.push( - `access-policy.default "${config.default}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`, - ); - } - if (config.rules) { for (const [pattern, perm] of Object.entries(config.rules)) { if (!pattern) { @@ -114,83 +108,6 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] } } } - if (entry.deny) { - for (let i = 0; i < entry.deny.length; i++) { - if (!entry.deny[i]) { - errors.push( - `access-policy.scripts["${scriptPath}"].deny[${i}] must be a non-empty glob pattern`, - ); - } - } - } - } - } - - if (config.deny) { - for (let i = 0; i < config.deny.length; i++) { - const pattern = config.deny[i]; - if (!pattern) { - errors.push(`access-policy.deny[${i}] must be a non-empty glob pattern`); - continue; - } - if (hasMidPathWildcard(pattern) && !_midPathWildcardWarned.has(`deny:${pattern}`)) { - _midPathWildcardWarned.add(`deny:${pattern}`); - errors.push( - `access-policy.deny entry "${pattern}" contains a mid-path wildcard — OS-level (bwrap/Seatbelt) enforcement cannot apply; tool-layer enforcement is still active.`, - ); - } - // Bare-path auto-expand for directories: "~/.ssh" → "~/.ssh/**" so the - // entire directory tree is denied, not just the directory inode itself. - // For paths that exist and are confirmed files (statSync), keep the bare - // pattern — expanding to "/**" would only match non-existent children, - // leaving the file itself unprotected at both the tool layer and bwrap. - // Non-existent paths are treated as future directories and always expanded - // so the subtree is protected before the directory is created. - if (!pattern.endsWith("/") && !/[*?[]/.test(pattern)) { - const expandedForStat = pattern.startsWith("~") - ? pattern.replace(/^~(?=$|[/\\])/, os.homedir()) - : pattern; - let isExistingFile = false; - // For non-existent paths: treat as a future file (skip /**-expansion) when - // the last segment looks like a filename — has a dot but is not a dotfile-only - // name (e.g. ".ssh") and has a non-empty extension (e.g. "secrets.key"). - // This preserves the intent of "deny: ['~/future-secrets.key']" where the - // user wants to protect that specific file once it is created. - // Plain names without an extension (e.g. "myfolder") are still treated as - // future directories and expanded to /**. - let looksLikeFile = false; - try { - isExistingFile = !fs.statSync(expandedForStat).isDirectory(); - } catch { - const lastName = - expandedForStat - .replace(/[/\\]$/, "") - .split(/[/\\]/) - .pop() ?? ""; - // Looks like a file when the last segment has a non-leading dot followed by a - // letter-containing extension — covers "secrets.key", "config.json". - // Note: pure dotnames like ".npmrc", ".env", ".ssh" do NOT match this regex - // (they have no non-leading dot) and are therefore expanded to /** below. - // Digit-only suffixes (v1.0, app-2.3) are treated as versioned directory names. - // Bare dotnames without a secondary extension (.ssh, .aws, .env, .gnupg) are - // NOT treated as file-like: they are expanded to /** so the subtree is protected - // when the path does not yet exist. For .env-style plain files the expansion is - // conservative but safe — once the file exists, statSync confirms it and the bare - // path is kept. The leading-dot heuristic was removed because it also matched - // common directory names (.ssh, .aws, .config) and silently skipped expansion. - looksLikeFile = /[^.]\.[a-zA-Z][^/\\]*$/.test(lastName); - } - if (!isExistingFile && !looksLikeFile) { - const fixed = `${pattern}/**`; - config.deny[i] = fixed; - if (!_autoExpandedWarned.has(`deny:${pattern}`)) { - _autoExpandedWarned.add(`deny:${pattern}`); - errors.push( - `access-policy.deny["${pattern}"] auto-expanded to "${fixed}" so it covers all directory contents.`, - ); - } - } - } } } @@ -313,9 +230,9 @@ export function findBestRule( * Checks whether a given operation on targetPath is permitted by the config. * * Precedence: - * 1. deny[] — any matching glob always blocks, no override. - * 2. rules — longest matching glob wins; check the relevant bit. - * 3. default — catch-all (treated as "---" when absent). + * 1. rules — longest matching glob wins; check the relevant bit. + * 2. implicit fallback — `"---"` (deny-all) when no rule matches. + * Use `"/**": "r--"` (or any other perm) as an explicit catch-all rule. */ export function checkAccessPolicy( targetPath: string, @@ -341,16 +258,7 @@ export function checkAccessPolicy( ); } - // 1. deny list always wins. - for (const pattern of config.deny ?? []) { - // Normalize so /private/tmp/** patterns match /tmp/** targets on macOS. - const expanded = normalizePlatformPath(expandPattern(pattern, homeDir)); - if (matchesPattern(expanded)) { - return "deny"; - } - } - - // 2. rules — longest match wins (check both path and path + "/" variants). + // rules — longest match wins (check both path and path + "/" variants). let bestPerm: PermStr | null = null; let bestLen = -1; for (const [pattern, perm] of Object.entries(config.rules ?? {})) { @@ -365,8 +273,8 @@ export function checkAccessPolicy( return permAllows(bestPerm, op) ? "allow" : "deny"; } - // 3. default catch-all (absent = "---" = deny all). - return permAllows(config.default, op) ? "allow" : "deny"; + // Implicit fallback: "---" (deny-all) when no rule matches. + return "deny"; } /** @@ -637,16 +545,11 @@ export function applyScriptPolicyOverride( // Build the merged policy WITHOUT the override rules merged in. // Override rules are returned separately so the caller can emit them AFTER - // the deny list in the seatbelt profile (last-match-wins — grants must come - // after deny entries to override broad deny patterns like ~/.secrets/**). + // the base rules in the seatbelt profile (last-match-wins — grants must come + // after broader rules to override them, e.g. a script-specific grant inside + // a broadly denied subtree). const { scripts: _scripts, ...base } = policy; - const merged: AccessPolicyConfig = { - ...base, - deny: [...(base.deny ?? []), ...(override.deny ?? [])], - }; - if (merged.deny?.length === 0) { - delete merged.deny; - } + const merged: AccessPolicyConfig = { ...base }; return { policy: merged, overrideRules: diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index d44cf2447d0..bc47b2e659f 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -14,14 +14,14 @@ const HOME = os.homedir(); // Windows/macOS CI does not fail on fs.statSync calls against Unix-only paths // like /etc/hosts that don't exist there. describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { - it("starts with --ro-bind / / when default allows reads", () => { - const config: AccessPolicyConfig = { default: "r--" }; + it("starts with --ro-bind / / when /** rule allows reads", () => { + const config: AccessPolicyConfig = { rules: { "/**": "r--" } }; const args = generateBwrapArgs(config, HOME); expect(args.slice(0, 3)).toEqual(["--ro-bind", "/", "/"]); }); - it("does not use --ro-bind / / when default is ---", () => { - const config: AccessPolicyConfig = { default: "---" }; + it("does not use --ro-bind / / when no /** rule (restrictive base)", () => { + const config: AccessPolicyConfig = {}; const args = generateBwrapArgs(config, HOME); // Should not contain root bind const rootBindIdx = args.findIndex( @@ -31,15 +31,14 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { }); it("ends with --", () => { - const config: AccessPolicyConfig = { default: "r--" }; + const config: AccessPolicyConfig = { rules: { "/**": "r--" } }; const args = generateBwrapArgs(config, HOME); expect(args[args.length - 1]).toBe("--"); }); - it("adds --tmpfs for each deny entry", () => { + it('adds --tmpfs for "---" rules in permissive mode', () => { const config: AccessPolicyConfig = { - deny: [`${HOME}/.ssh/**`, `${HOME}/.gnupg/**`], - default: "r--", + rules: { "/**": "r--", [`${HOME}/.ssh/**`]: "---", [`${HOME}/.gnupg/**`]: "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -47,10 +46,9 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(tmpfsMounts).toContain(`${HOME}/.gnupg`); }); - it("expands ~ in deny patterns using homeDir", () => { + it('expands ~ in "---" rules using homeDir', () => { const config: AccessPolicyConfig = { - deny: ["~/.ssh/**"], - default: "r--", + rules: { "/**": "r--", "~/.ssh/**": "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -59,8 +57,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it("adds --bind for paths with w bit in rules", () => { const config: AccessPolicyConfig = { - rules: { [`${HOME}/workspace/**`]: "rw-" }, - default: "r--", + rules: { "/**": "r--", [`${HOME}/workspace/**`]: "rw-" }, }; const args = generateBwrapArgs(config, HOME); const bindPairs: string[] = []; @@ -74,8 +71,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it("does not add --bind for read-only rules on permissive base", () => { const config: AccessPolicyConfig = { - rules: { "/usr/bin/**": "r--" }, - default: "r--", + rules: { "/**": "r--", "/usr/bin/**": "r--" }, }; const args = generateBwrapArgs(config, HOME); // /usr/bin should NOT appear as a --bind-try (it's already ro-bound via /) @@ -88,11 +84,9 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindPairs).not.toContain("/usr/bin"); }); - it("deny entry tmpfs appears in args regardless of rule for that path", () => { + it('"---" rule for sensitive path appears in args regardless of broader rule', () => { const config: AccessPolicyConfig = { - rules: { [`${HOME}/**`]: "rwx" }, - deny: [`${HOME}/.ssh/**`], - default: "r--", + rules: { "/**": "r--", [`${HOME}/**`]: "rwx", [`${HOME}/.ssh/**`]: "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -106,25 +100,22 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it("adds --proc /proc in permissive mode so /proc is accessible inside the sandbox", () => { // --ro-bind / / does not propagate kernel filesystems (procfs) into the new // mount namespace; without --proc /proc, shells and Python fail in the sandbox. - const args = generateBwrapArgs({ default: "r--" }, HOME); + const args = generateBwrapArgs({ rules: { "/**": "r--" } }, HOME); const procIdx = args.indexOf("--proc"); expect(procIdx).toBeGreaterThan(-1); expect(args[procIdx + 1]).toBe("/proc"); }); - it("adds --tmpfs /tmp in permissive mode", () => { - const config: AccessPolicyConfig = { default: "r--" }; + it("adds --tmpfs /tmp in permissive mode (/** allows reads)", () => { + const config: AccessPolicyConfig = { rules: { "/**": "r--" } }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).toContain("/tmp"); }); - it("does not add --tmpfs /tmp in restrictive mode even with no explicit /tmp rule", () => { - // Regression guard: the defaultAllowsRead guard on the /tmp block must prevent - // a writable tmpfs being mounted under default:"---" when no /tmp rule exists. - // explicitTmpPerm === null is true (no rule), but defaultAllowsRead is false, - // so the entire /tmp block must be skipped. - const config: AccessPolicyConfig = { default: "---" }; + it("does not add --tmpfs /tmp in restrictive mode (no /** rule)", () => { + // Without a "/**" rule, the base is restrictive — no /tmp by default. + const config: AccessPolicyConfig = {}; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).not.toContain("/tmp"); @@ -133,7 +124,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it("skips --tmpfs /tmp in permissive mode when policy explicitly restricts /tmp writes", () => { // A rule "/tmp/**": "r--" means the user wants /tmp read-only; the base --ro-bind / / // already makes it readable. Adding --tmpfs /tmp would silently grant write access. - const config: AccessPolicyConfig = { default: "r--", rules: { "/tmp/**": "r--" } }; + const config: AccessPolicyConfig = { rules: { "/**": "r--", "/tmp/**": "r--" } }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).not.toContain("/tmp"); @@ -144,7 +135,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { // but the rules loop always follows with --bind-try /tmp /tmp which wins (last mount wins // in bwrap). The --tmpfs was dead code. Confirm: explicit rw- rule → no --tmpfs /tmp, // but --bind-try /tmp IS present. - const config: AccessPolicyConfig = { default: "r--", rules: { "/tmp/**": "rw-" } }; + const config: AccessPolicyConfig = { rules: { "/**": "r--", "/tmp/**": "rw-" } }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); const bindMounts = args @@ -154,19 +145,19 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindMounts).toContain("/tmp"); }); - it("does not add --tmpfs /tmp in restrictive mode (default: ---)", () => { - const config: AccessPolicyConfig = { default: "---" }; + it("does not add --tmpfs /tmp in restrictive mode (no /** rule) — regression guard", () => { + // When there is no "/**" rule at all, no /tmp mount should appear. + const config: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "rwx" } }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).not.toContain("/tmp"); }); it('"---" rule in permissive mode gets --tmpfs overlay to block reads', () => { - // With default:"r--", --ro-bind / / makes everything readable. A narrowing + // With "/**":"r--", --ro-bind / / makes everything readable. A narrowing // rule like "/secret/**": "---" must overlay --tmpfs so the path is hidden. const config: AccessPolicyConfig = { - default: "r--", - rules: { [`${HOME}/secret/**`]: "---" }, + rules: { "/**": "r--", [`${HOME}/secret/**`]: "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -179,15 +170,10 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { }); it("narrowing rule on an existing file does not emit --tmpfs (bwrap only accepts dirs)", () => { - // Reproducer: { "default": "r--", "rules": { "~/secrets.key": "---" } } - // where ~/secrets.key is an existing file. The old code emitted --tmpfs on the - // file path, causing bwrap to abort with "Not a directory". Fix: mirror the - // isDir guard already present in the deny[] branch. // process.execPath is always an existing file — use it as the test target. const filePath = process.execPath; const config: AccessPolicyConfig = { - default: "r--", - rules: { [filePath]: "---" }, + rules: { "/**": "r--", [filePath]: "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -198,8 +184,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it('"--x" rule in permissive mode gets --tmpfs overlay to block reads', () => { // Execute-only rules have no read bit — same treatment as "---" in permissive mode. const config: AccessPolicyConfig = { - default: "r--", - rules: { [`${HOME}/scripts/**`]: "--x" }, + rules: { "/**": "r--", [`${HOME}/scripts/**`]: "--x" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -212,7 +197,6 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { // for a "---" rule would silently grant read access to paths that should // be fully blocked. const config: AccessPolicyConfig = { - default: "---", rules: { [`${HOME}/workspace/**`]: "rwx", // allowed: should produce --bind-try [`${HOME}/workspace/private/**`]: "---", // denied: must NOT produce any mount @@ -232,7 +216,6 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it('"--x" rules do not create --ro-bind-try mounts in restrictive mode', () => { // Same as "---" case: execute-only rules also must not emit read mounts. const config: AccessPolicyConfig = { - default: "---", rules: { [`${HOME}/scripts/**`]: "--x" }, }; const args = generateBwrapArgs(config, HOME); @@ -243,12 +226,11 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { }); it('"-w-" rule in restrictive mode emits --bind-try so writes do not silently fail', () => { - // A write-only rule ("-w-") under default:"---" now emits --bind-try so the path + // A write-only rule ("-w-") without "/**" now emits --bind-try so the path // exists in the bwrap namespace and writes succeed. bwrap cannot enforce // write-without-read at the mount level; reads are also permitted at the OS layer, // but the tool layer still denies read tool calls per the "-w-" rule. const config: AccessPolicyConfig = { - default: "---", rules: { [`${HOME}/logs/**`]: "-w-" }, }; const args = generateBwrapArgs(config, HOME); @@ -259,12 +241,11 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { }); it('"-w-" rule in permissive mode emits --bind-try (write upgrade, reads already allowed)', () => { - // Under default:"r--", --ro-bind / / already grants reads everywhere. + // Under "/**":"r--", --ro-bind / / already grants reads everywhere. // A "-w-" rule upgrades to rw for that path — reads are not newly leaked // since the base already allowed them. const config: AccessPolicyConfig = { - default: "r--", - rules: { [`${HOME}/output/**`]: "-w-" }, + rules: { "/**": "r--", [`${HOME}/output/**`]: "-w-" }, }; const args = generateBwrapArgs(config, HOME); const bindMounts = args @@ -278,9 +259,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { // The pattern must be silently ignored rather than binding /home. const fakeHome = "/home/testuser"; const config: AccessPolicyConfig = { - default: "r--", - deny: ["/home/*/.ssh/**"], - rules: { "/home/*/.config/**": "---" }, + rules: { "/**": "r--", "/home/*/.config/**": "---" }, }; const args = generateBwrapArgs(config, fakeHome); const allMountTargets = args @@ -296,8 +275,7 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { // "/var/log/secret*" must mount "/var/log", NOT the literal prefix "/var/log/secret" // which is not a directory and leaves entries like "/var/log/secret.old" unprotected. const config: AccessPolicyConfig = { - default: "r--", - deny: ["/var/log/secret*"], + rules: { "/**": "r--", "/var/log/secret*": "---" }, }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); @@ -315,7 +293,6 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { [`${HOME}/dev/secret/**`]: "r--", [`${HOME}/dev/**`]: "rw-", }, - default: "---", }; const args = generateBwrapArgs(config, HOME); const bindArgs = args.filter((a) => a === "--bind-try" || a === "--ro-bind-try"); @@ -333,12 +310,11 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindArgs[secretIdx]).toBe("--ro-bind-try"); }); - it('script override "-w-" under restrictive default emits --bind-try, not --tmpfs', () => { - // Greptile: permAllowsWrite && (r || defaultR) condition was wrong — for -w- under --- + 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. // Fix: any write-granting override always emits --bind-try. const config: AccessPolicyConfig = { - default: "---", rules: { [`${HOME}/workspace/**`]: "rwx" }, }; const overrides = { [`${HOME}/logs/**`]: "-w-" as const }; @@ -351,20 +327,16 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(tmpfsMounts).not.toContain(`${HOME}/logs`); }); - it("skips --tmpfs for deny[] entry that resolves to an existing file (not a directory)", () => { - // /etc/hosts is a file on both macOS and Linux; bwrap --tmpfs rejects file paths. - // The deny entry is expanded to "/etc/hosts/**" by validateAccessPolicyConfig, and - // patternToPath strips the "/**" back to "/etc/hosts". generateBwrapArgs must not - // emit "--tmpfs /etc/hosts" — it should be silently skipped. - const config: AccessPolicyConfig = { default: "r--", deny: ["/etc/hosts/**"] }; + it("narrowing rule that resolves to an existing file does not emit --tmpfs (bwrap only accepts dirs)", () => { + // /etc/hosts is a file on Linux; bwrap --tmpfs rejects file paths. + // generateBwrapArgs must not emit "--tmpfs /etc/hosts" — it should be silently skipped. + const config: AccessPolicyConfig = { rules: { "/**": "r--", "/etc/hosts/**": "---" } }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).not.toContain("/etc/hosts"); }); - it("emits a console.error warning when a file-specific deny[] entry is skipped", () => { - // Use /etc/passwd (always a file) rather than /etc/hosts which is already in - // _bwrapFileDenyWarnedPaths from the generateBwrapArgs test above. + it("emits a console.error warning when a file-specific narrowing rule path is skipped", () => { const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); try { _warnBwrapFileDenyOnce("/etc/passwd"); @@ -375,9 +347,11 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { } }); - it("still emits --tmpfs for deny[] entry that resolves to a directory", () => { + it('still emits --tmpfs for "---" rule that resolves to a directory', () => { // Non-existent paths are treated as directories (forward-protection). - const config: AccessPolicyConfig = { default: "r--", deny: [`${HOME}/.nonexistent-dir/**`] }; + const config: AccessPolicyConfig = { + rules: { "/**": "r--", [`${HOME}/.nonexistent-dir/**`]: "---" }, + }; const args = generateBwrapArgs(config, HOME); const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); expect(tmpfsMounts).toContain(`${HOME}/.nonexistent-dir`); @@ -386,8 +360,8 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { it("trailing-slash rule is treated as /** and resolves to correct path", () => { // "/tmp/" is shorthand for "/tmp/**" — must produce the same mount target // and sort-order length as an explicit "/tmp/**" rule. - const withSlash = generateBwrapArgs({ default: "---", rules: { "/tmp/": "rw-" } }, HOME); - const withGlob = generateBwrapArgs({ default: "---", rules: { "/tmp/**": "rw-" } }, HOME); + const withSlash = generateBwrapArgs({ rules: { "/tmp/": "rw-" } }, HOME); + const withGlob = generateBwrapArgs({ rules: { "/tmp/**": "rw-" } }, HOME); const bindOf = (args: string[]) => args.map((a, i) => (args[i - 1] === "--bind-try" ? a : null)).filter(Boolean); expect(bindOf(withSlash)).toContain("/tmp"); @@ -397,17 +371,17 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { describe("wrapCommandWithBwrap", () => { it("starts with bwrap", () => { - const result = wrapCommandWithBwrap("ls /tmp", { default: "r--" }, HOME); + const result = wrapCommandWithBwrap("ls /tmp", { rules: { "/**": "r--" } }, HOME); expect(result).toMatch(/^bwrap /); }); it("contains -- separator before the command", () => { - const result = wrapCommandWithBwrap("ls /tmp", { default: "r--" }, HOME); + const result = wrapCommandWithBwrap("ls /tmp", { rules: { "/**": "r--" } }, HOME); expect(result).toContain("-- /bin/sh -c"); }); it("wraps command in /bin/sh -c", () => { - const result = wrapCommandWithBwrap("cat /etc/hosts", { default: "r--" }, HOME); + const result = wrapCommandWithBwrap("cat /etc/hosts", { rules: { "/**": "r--" } }, HOME); expect(result).toContain("/bin/sh -c"); expect(result).toContain("cat /etc/hosts"); }); diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 36ba42db230..09a276b1223 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -27,11 +27,10 @@ const SYSTEM_RO_BIND_PATHS = ["/usr", "/bin", "/lib", "/lib64", "/sbin", "/etc", let bwrapAvailableCache: boolean | undefined; -// Warn once per process when a file-specific deny[] entry cannot be enforced at +// Warn once per process when a file-specific "---" rule cannot be enforced at // the OS layer (bwrap --tmpfs only accepts directories). Tool-layer enforcement // still applies for read/write/edit tool calls, but exec commands that access // the file directly inside the sandbox are not blocked at the syscall level. -// See docs/tools/access-policy.md — "File-specific deny[] entries on Linux". const _bwrapFileDenyWarnedPaths = new Set(); /** Reset the one-time file-deny warning set. Only for use in tests. */ export function _resetBwrapFileDenyWarnedPathsForTest(): void { @@ -43,10 +42,10 @@ export function _warnBwrapFileDenyOnce(filePath: string): void { } _bwrapFileDenyWarnedPaths.add(filePath); console.error( - `[access-policy] bwrap: deny[] entry "${filePath}" resolves to a file — ` + + `[access-policy] bwrap: "---" rule for "${filePath}" resolves to a file — ` + `OS-level (bwrap) enforcement is not applied. ` + `Tool-layer enforcement still blocks read/write/edit tool calls. ` + - `To protect this file at the OS layer on Linux, deny its parent directory instead.`, + `To protect this file at the OS layer on Linux, use a "---" rule on its parent directory instead.`, ); } @@ -119,25 +118,27 @@ function permAllowsWrite(perm: PermStr): boolean { * Generate bwrap argument array for the given permissions config. * * Strategy: - * 1. Start with --ro-bind / / (read-only view of entire host FS) - * 2. For each rule with w bit: upgrade to --bind (read-write) - * 3. For each deny[] entry: overlay with --tmpfs (empty, blocks reads too) - * 4. Add /tmp and /dev as writable tmpfs mounts (required for most processes) - * 5. When default is "---": use a more restrictive base (only bind explicit allow paths) + * 1. Check the "/**" rule to determine permissive vs restrictive base. + * 2. Permissive base (r in "/**"): --ro-bind / / (read-only view of entire host FS). + * 3. Restrictive base (no r in "/**"): only bind system paths needed to run processes. + * 4. For each rule with w bit: upgrade to --bind (read-write). + * 5. For each "---" rule in permissive mode: overlay with --tmpfs to hide the path. + * 6. Add /tmp and /dev as writable tmpfs mounts (required for most processes). */ export function generateBwrapArgs( config: AccessPolicyConfig, homeDir: string = os.homedir(), /** - * Script-specific override rules to emit AFTER the deny list so they win over - * broad deny patterns — mirrors the Seatbelt scriptOverrideRules behaviour. + * Script-specific override rules to emit AFTER the base rules so they win over + * broader patterns — mirrors the Seatbelt scriptOverrideRules behaviour. * In bwrap, later mounts win, so script grants must come last. */ scriptOverrideRules?: Record, ): string[] { const args: string[] = []; - const defaultPerm = config.default ?? "---"; - const defaultAllowsRead = defaultPerm[0] === "r"; + // Determine base stance from the "/**" catch-all rule (replaces the removed `default` field). + const catchAllPerm = findBestRule("/**", config.rules ?? {}, homeDir) ?? "---"; + const defaultAllowsRead = catchAllPerm[0] === "r"; if (defaultAllowsRead) { // Permissive base: everything is read-only by default. @@ -199,12 +200,12 @@ export function generateBwrapArgs( // a restrictive base will also permit reads at the OS layer. The tool layer still // denies read tool calls per the rule, so the practical exposure is exec-only paths. args.push("--bind-try", p, p); - } else if (defaultPerm[0] !== "r" && perm[0] === "r") { + } else if (catchAllPerm[0] !== "r" && perm[0] === "r") { // Restrictive base: only bind paths that the rule explicitly allows reads on. // Do NOT emit --ro-bind-try for "---" or "--x" rules — the base already denies // by not mounting; emitting a mount here would grant read access. args.push("--ro-bind-try", p, p); - } else if (defaultPerm[0] === "r" && perm[0] !== "r") { + } else if (catchAllPerm[0] === "r" && perm[0] !== "r") { // Permissive base + narrowing rule (no read bit): overlay with tmpfs so the // path is hidden even though --ro-bind / / made it readable by default. // This mirrors what deny[] does — without this, "---" rules under a permissive @@ -227,35 +228,7 @@ export function generateBwrapArgs( // Permissive base + read-only rule: already covered by --ro-bind / /; no extra mount. } - // deny[] entries: overlay with empty tmpfs — path exists but is empty. - // tmpfs overlay hides the real contents regardless of how the path was expressed. - // Guard: bwrap --tmpfs only accepts a directory as the mount point. deny[] entries - // like "~/.ssh/id_rsa" are unconditionally expanded to "~/.ssh/id_rsa/**" by - // validateAccessPolicyConfig and resolve back to the file path here. Passing a - // file to --tmpfs causes bwrap to error out ("Not a directory"). Non-existent - // paths are assumed to be directories (the common case for protecting future dirs). - for (const pattern of config.deny ?? []) { - const p = patternToPath(pattern, homeDir); - if (!p || p === "/") { - continue; - } - let isDir = true; - try { - isDir = fs.statSync(p).isDirectory(); - } catch { - // Non-existent path — assume directory (forward-protection for dirs not yet created). - } - if (isDir) { - args.push("--tmpfs", p); - } else { - // File-specific entry: tool-layer checkAccessPolicy still denies read/write/edit - // tool calls, but exec commands inside the sandbox can still access the file - // directly. Warn operators so they know to deny the parent directory instead. - _warnBwrapFileDenyOnce(p); - } - } - - // Script-specific override mounts — emitted after deny[] so they can reopen + // Script-specific override mounts — emitted after base rules so they can reopen // a base-denied path for a trusted script (same precedence as Seatbelt). if (scriptOverrideRules) { const overrideEntries = Object.entries(scriptOverrideRules).toSorted(([a], [b]) => { diff --git a/src/infra/exec-sandbox-seatbelt.test.ts b/src/infra/exec-sandbox-seatbelt.test.ts index b66d64db877..4b7a2296529 100644 --- a/src/infra/exec-sandbox-seatbelt.test.ts +++ b/src/infra/exec-sandbox-seatbelt.test.ts @@ -16,54 +16,48 @@ describe("generateSeatbeltProfile", () => { expect(profile).toMatch(/^\(version 1\)/); }); - it("uses (deny default) when default is ---", () => { - const profile = generateSeatbeltProfile({ default: "---" }, HOME); + it("uses (deny default) when no /** catch-all rule", () => { + const profile = generateSeatbeltProfile({}, HOME); expect(profile).toContain("(deny default)"); expect(profile).not.toContain("(allow default)"); }); - it("uses (allow default) when default has any permission", () => { - const profile = generateSeatbeltProfile({ default: "r--" }, HOME); + it("uses (allow default) when /** rule has any permission", () => { + const profile = generateSeatbeltProfile({ rules: { "/**": "r--" } }, HOME); expect(profile).toContain("(allow default)"); expect(profile).not.toContain("(deny default)"); }); - it("includes system baseline reads when default is ---", () => { - const profile = generateSeatbeltProfile({ default: "---" }, HOME); + it("includes system baseline reads when no catch-all rule", () => { + const profile = generateSeatbeltProfile({}, HOME); expect(profile).toContain("(allow file-read*"); expect(profile).toContain("/usr/lib"); expect(profile).toContain("/System/Library"); }); - skipOnWindows("deny list entries appear as deny file-read*, file-write*, process-exec*", () => { + skipOnWindows("--- rule emits deny file-read*, file-write*, process-exec* for that path", () => { const config: AccessPolicyConfig = { - deny: [`${HOME}/.ssh/**`], - default: "rwx", + rules: { "/**": "rwx", [`${HOME}/.ssh/**`]: "---" }, }; const profile = generateSeatbeltProfile(config, HOME); expect(profile).toContain(`(deny file-read*`); expect(profile).toContain(`(deny file-write*`); expect(profile).toContain(`(deny process-exec*`); - // Should contain the path expect(profile).toContain(HOME + "/.ssh"); }); - skipOnWindows("expands ~ in deny patterns using provided homeDir", () => { + skipOnWindows("expands ~ in --- rules using provided homeDir", () => { const config: AccessPolicyConfig = { - deny: ["~/.ssh/**"], - default: "rwx", + rules: { "/**": "rwx", "~/.ssh/**": "---" }, }; const profile = generateSeatbeltProfile(config, HOME); expect(profile).toContain(HOME + "/.ssh"); - // Should NOT contain literal ~ - const denySection = profile.split("; Deny list")[1] ?? ""; - expect(denySection).not.toContain("~/.ssh"); + expect(profile).not.toContain("~/.ssh"); }); skipOnWindows("expands ~ in rules using provided homeDir", () => { const config: AccessPolicyConfig = { rules: { "~/**": "rw-" }, - default: "---", }; const profile = generateSeatbeltProfile(config, HOME); expect(profile).toContain(HOME); @@ -72,7 +66,6 @@ describe("generateSeatbeltProfile", () => { it("rw- rule emits allow read+write, deny exec for that path", () => { const config: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "rw-" }, - default: "---", }; const profile = generateSeatbeltProfile(config, HOME); expect(profile).toContain(`(allow file-read*`); @@ -83,7 +76,6 @@ describe("generateSeatbeltProfile", () => { it("r-x rule emits allow read+exec, deny write for that path", () => { const config: AccessPolicyConfig = { rules: { "/usr/bin/**": "r-x" }, - default: "---", }; const profile = generateSeatbeltProfile(config, HOME); const rulesSection = profile.split("; User-defined path rules")[1] ?? ""; @@ -92,17 +84,19 @@ describe("generateSeatbeltProfile", () => { expect(rulesSection).toContain("(deny file-write*"); }); - it("deny list section appears after rules section", () => { + it("narrowing --- rule appears after broader allow rule in profile", () => { + // SBPL last-match-wins: the --- rule for .ssh must appear after the broader rwx rule. const config: AccessPolicyConfig = { - rules: { [`${HOME}/**`]: "rwx" }, - deny: [`${HOME}/.ssh/**`], - default: "r--", + rules: { [`${HOME}/**`]: "rwx", [`${HOME}/.ssh/**`]: "---" }, }; const profile = generateSeatbeltProfile(config, HOME); const rulesIdx = profile.indexOf("; User-defined path rules"); - const denyIdx = profile.indexOf("; Deny list"); expect(rulesIdx).toBeGreaterThan(-1); - expect(denyIdx).toBeGreaterThan(rulesIdx); + // The broader allow must appear before the narrowing deny. + const allowIdx = profile.indexOf("(allow file-read*"); + const denyIdx = profile.lastIndexOf("(deny file-read*"); + expect(allowIdx).toBeGreaterThan(-1); + expect(denyIdx).toBeGreaterThan(allowIdx); }); it("handles empty config without throwing", () => { @@ -110,18 +104,17 @@ describe("generateSeatbeltProfile", () => { }); it("permissive base with no exec bit includes system baseline exec paths", () => { - // default:"r--" emits (deny process-exec* (subpath "/")) but must also allow + // "/**": "r--" emits (deny process-exec* (subpath "/")) but must also allow // system binaries — otherwise ls, grep, cat all fail inside the sandbox. - const profile = generateSeatbeltProfile({ default: "r--" }, HOME); + const profile = generateSeatbeltProfile({ rules: { "/**": "r--" } }, HOME); expect(profile).toContain("(allow process-exec*"); expect(profile).toContain("/bin"); expect(profile).toContain("/usr/bin"); }); it("permissive base with exec bit does NOT add redundant exec baseline", () => { - // default:"rwx" already allows everything including exec — no extra baseline needed. - const profile = generateSeatbeltProfile({ default: "rwx" }, HOME); - // (allow default) covers exec; no separate baseline exec section needed + // "/**": "rwx" already allows everything including exec — no extra baseline needed. + const profile = generateSeatbeltProfile({ rules: { "/**": "rwx" } }, HOME); expect(profile).toContain("(allow default)"); expect(profile).not.toContain("System baseline exec"); }); @@ -131,7 +124,6 @@ describe("generateSeatbeltProfile", () => { // Without deny ops in the override block, write would still be allowed. const config: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "rw-" }, - default: "---", }; const overrideRules: Record = { [`${HOME}/workspace/locked/**`]: "r--" }; const profile = generateSeatbeltProfile(config, HOME, overrideRules); @@ -141,70 +133,37 @@ describe("generateSeatbeltProfile", () => { expect(overrideSection).toContain(`${HOME}/workspace/locked`); }); - it("omits /private/tmp baseline when default is --- and no rule grants /tmp", () => { - // In restrictive mode without an explicit /tmp rule, /tmp should NOT be in - // the baseline — emitting it unconditionally would contradict default: "---". - const config: AccessPolicyConfig = { default: "---" }; - const profile = generateSeatbeltProfile(config, HOME); + it("omits /private/tmp baseline when no rule grants /tmp", () => { + const profile = generateSeatbeltProfile({}, HOME); expect(profile).not.toContain(`(subpath "/private/tmp")`); }); it("includes /private/tmp baseline when a rule grants read access to /tmp", () => { - const config: AccessPolicyConfig = { - default: "---", - rules: { "/tmp/**": "rw-" }, - }; - const profile = generateSeatbeltProfile(config, HOME); + const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "rw-" } }, HOME); expect(profile).toContain(`(subpath "/private/tmp")`); }); it("read-only /tmp rule does not grant file-write* on /private/tmp", () => { - // A policy of "/tmp/**": "r--" must grant reads but NOT writes to /tmp. - // The old code used (r || w) as the gate for both ops, so r-- inadvertently - // granted file-write* alongside read ops. - const config: AccessPolicyConfig = { - default: "---", - rules: { "/tmp/**": "r--" }, - }; - const profile = generateSeatbeltProfile(config, HOME); - // Read ops must be allowed for /tmp. + const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "r--" } }, HOME); expect(profile).toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); - // Write must NOT be present for /tmp. expect(profile).not.toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); }); it("write-only /tmp rule grants file-write* but not read ops on /private/tmp", () => { - const config: AccessPolicyConfig = { - default: "---", - rules: { "/tmp/**": "-w-" }, - }; - const profile = generateSeatbeltProfile(config, HOME); + const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "-w-" } }, HOME); expect(profile).toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); expect(profile).not.toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); }); - it("exec-only /tmp rule grants process-exec* on /private/tmp in restrictive mode", () => { - // Regression: when default:"---" and a rule grants exec on /tmp (e.g. "--x"), - // the seatbelt profile must emit (allow process-exec* (subpath "/private/tmp")). - // Without this fix, no exec allowance was emitted and binaries in /tmp could not - // be executed even though the policy explicitly permitted it. - const config: AccessPolicyConfig = { - default: "---", - rules: { "/tmp/**": "--x" }, - }; - const profile = generateSeatbeltProfile(config, HOME); + it("exec-only /tmp rule grants process-exec* on /private/tmp", () => { + const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "--x" } }, HOME); expect(profile).toMatch(/allow process-exec\*[^)]*\(subpath "\/private\/tmp"\)/); - // Read and write must NOT be granted. expect(profile).not.toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); expect(profile).not.toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); }); - it("r-x /tmp rule grants both read and exec on /private/tmp in restrictive mode", () => { - const config: AccessPolicyConfig = { - default: "---", - rules: { "/tmp/**": "r-x" }, - }; - const profile = generateSeatbeltProfile(config, HOME); + it("r-x /tmp rule grants both read and exec on /private/tmp", () => { + const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "r-x" } }, HOME); expect(profile).toMatch(/allow file-read[^)]*\(subpath "\/private\/tmp"\)/); expect(profile).toMatch(/allow process-exec\*[^)]*\(subpath "\/private\/tmp"\)/); expect(profile).not.toMatch(/allow file-write\*[^)]*\(subpath "\/private\/tmp"\)/); @@ -221,14 +180,15 @@ describe("generateSeatbeltProfile", () => { // --------------------------------------------------------------------------- skipOnWindows( - "deny list for sensitive path appears after workspace allow — symlink to deny target is blocked", + "--- rule for sensitive path appears after workspace allow — symlink to deny target is blocked", () => { // If ~/workspace/link → ~/.ssh/id_rsa, seatbelt evaluates ~/.ssh/id_rsa. - // The deny entry for ~/.ssh must appear after the workspace allow so it wins. + // The --- rule for ~/.ssh must appear after the workspace allow so it wins. const config: AccessPolicyConfig = { - rules: { [`${HOME}/workspace/**`]: "rw-" }, - deny: [`${HOME}/.ssh/**`], - default: "---", + rules: { + [`${HOME}/workspace/**`]: "rw-", + [`${HOME}/.ssh/**`]: "---", + }, }; const profile = generateSeatbeltProfile(config, HOME); const workspaceAllowIdx = profile.indexOf("(allow file-read*"); @@ -243,16 +203,11 @@ describe("generateSeatbeltProfile", () => { skipOnWindows( "restrictive rule on subdir appears after broader rw rule — covers symlink to restricted subtree", () => { - // ~/workspace/** is rw-, ~/workspace/secret/** is r--. - // A symlink ~/workspace/link → ~/workspace/secret/file: seatbelt sees the - // real path ~/workspace/secret/... which must hit the narrower r-- rule. - // The deny write for secret must appear after the allow write for workspace. const config: AccessPolicyConfig = { rules: { [`${HOME}/workspace/**`]: "rw-", [`${HOME}/workspace/secret/**`]: "r--", }, - default: "---", }; const profile = generateSeatbeltProfile(config, HOME); const workspaceWriteIdx = profile.indexOf("(allow file-write*"); @@ -265,11 +220,9 @@ describe("generateSeatbeltProfile", () => { it("glob patterns are stripped to their longest concrete prefix", () => { const config: AccessPolicyConfig = { - deny: ["/Users/kaveri/.ssh/**"], - default: "rwx", + rules: { "/Users/kaveri/.ssh/**": "---", "/**": "rwx" }, }; const profile = generateSeatbeltProfile(config, "/Users/kaveri"); - // ** should not appear in profile — stripped to subpath expect(profile).not.toContain("**"); expect(profile).toContain("/Users/kaveri/.ssh"); }); diff --git a/src/infra/exec-sandbox-seatbelt.ts b/src/infra/exec-sandbox-seatbelt.ts index 01ea9db7c87..9c1aa267092 100644 --- a/src/infra/exec-sandbox-seatbelt.ts +++ b/src/infra/exec-sandbox-seatbelt.ts @@ -170,10 +170,11 @@ export function generateSeatbeltProfile( lines.push("(version 1)"); lines.push(""); - // Determine base stance from default permission. - const defaultPerm = config.default ?? "---"; + // Determine base stance from the "/**" catch-all rule (replaces the removed `default` field). + const catchAllPerm = findBestRule("/**", config.rules ?? {}, homeDir) ?? "---"; + const defaultPerm = catchAllPerm; // alias for readability below const defaultAllowsAnything = - defaultPerm[0] === "r" || defaultPerm[1] === "w" || defaultPerm[2] === "x"; + catchAllPerm[0] === "r" || catchAllPerm[1] === "w" || catchAllPerm[2] === "x"; if (defaultAllowsAnything) { // Permissive base: allow everything, then restrict. @@ -209,7 +210,7 @@ export function generateSeatbeltProfile( // unconditionally granting /tmp access when default: "---". // Use "/tmp/." so glob rules like "/tmp/**" match correctly — findBestRule // on "/tmp" alone would miss "/**"-suffixed patterns that only match descendants. - const tmpPerm = findBestRule("/tmp/.", config.rules ?? {}, homeDir) ?? config.default ?? "---"; + const tmpPerm = findBestRule("/tmp/.", config.rules ?? {}, homeDir) ?? "---"; // 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") { @@ -262,24 +263,7 @@ export function generateSeatbeltProfile( } } - // deny[] entries — always win over base rules. - if ((config.deny ?? []).length > 0) { - lines.push(""); - lines.push("; Deny list — wins over base rules"); - for (const pattern of config.deny ?? []) { - for (const expanded of expandSbplAliases(pattern)) { - const matcher = patternToSbplMatcher(expanded, homeDir); - if (!matcher) { - continue; - } - lines.push(`(deny ${SEATBELT_READ_OPS} ${matcher})`); - lines.push(`(deny ${SEATBELT_WRITE_OPS} ${matcher})`); - lines.push(`(deny ${SEATBELT_EXEC_OPS} ${matcher})`); - } - } - } - - // Script-override rules emitted last — they win over deny entries above. + // Script-override rules emitted last — they win over base rules above. // Required when a script grant covers a path inside a denied subtree. // In SBPL, last matching rule wins. if (scriptOverrideRules && Object.keys(scriptOverrideRules).length > 0) {