diff --git a/src/infra/access-policy-file.test.ts b/src/infra/access-policy-file.test.ts index 7ade68f01f3..b1db78f86d9 100644 --- a/src/infra/access-policy-file.test.ts +++ b/src/infra/access-policy-file.test.ts @@ -65,6 +65,22 @@ describe("mergeAccessPolicy", () => { expect(mergeAccessPolicy(undefined, override)).toEqual(override); }); + it("returns a copy when base is undefined — mutations do not corrupt the original", () => { + // mergeAccessPolicy(undefined, x) was returning x by reference. validateAccessPolicyConfig + // calls autoExpandBareDir which mutates .policy in-place, permanently corrupting the cached + // agents["*"] object for all subsequent calls in the same process. + const override = { policy: { "/tmp": "rwx" as const } }; + const result = mergeAccessPolicy(undefined, override); + // Simulate autoExpandBareDir mutating the result. + if (result?.policy) { + result.policy["/tmp/**"] = "rwx"; + delete result.policy["/tmp"]; + } + // The original override must be unchanged. + expect(override.policy).toEqual({ "/tmp": "rwx" }); + expect(override.policy["/tmp/**" as keyof typeof override.policy]).toBeUndefined(); + }); + it("rules are shallow-merged, override key wins on collision", () => { const result = mergeAccessPolicy( { policy: { "/**": "r--", "~/**": "rw-" } }, @@ -201,6 +217,22 @@ describe("loadAccessPolicyFile", () => { spy.mockRestore(); }); + it('returns BROKEN_POLICY_FILE when scripts["policy"] is a primitive (not an object)', () => { + // scripts["policy"] must be a Record; a primitive like `true` + // silently passes structural validation and is treated as an empty shared policy. + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + writeFile({ + version: 1, + agents: { "*": { scripts: { policy: true as unknown as Record } } }, + }); + const result = loadAccessPolicyFile(); + expect(result).toBe(BROKEN_POLICY_FILE); + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('scripts["policy"] must be an object'), + ); + spy.mockRestore(); + }); + it("returns parsed file when valid", () => { const content: AccessPolicyFile = { version: 1, diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index 9630703dbad..4878f2a5beb 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -42,7 +42,15 @@ export function mergeAccessPolicy( return undefined; } if (!base) { - return override; + // Return a shallow copy so that validateAccessPolicyConfig → autoExpandBareDir + // does not mutate the cached agents["*"] object in _fileCache. Without this, + // the first call permanently corrupts policy entries for all subsequent calls + // in the same process. + return { + ...override, + policy: override.policy ? { ...override.policy } : undefined, + scripts: override.scripts ? { ...override.scripts } : undefined, + }; } if (!override) { return base; @@ -140,8 +148,17 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s for (const [scriptKey, scriptEntry] of Object.entries( scripts as Record, )) { - if ( - scriptKey !== "policy" && + if (scriptKey === "policy") { + // scripts["policy"] must be an object (Record), not a primitive. + if ( + scriptEntry != null && + (typeof scriptEntry !== "object" || Array.isArray(scriptEntry)) + ) { + errors.push( + `${filePath}: agents["${agentId}"].scripts["policy"] must be an object (Record), got ${JSON.stringify(scriptEntry)}`, + ); + } + } else if ( scriptEntry != null && typeof scriptEntry === "object" && !Array.isArray(scriptEntry) diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index 7c610950ce6..ea8d88fce04 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -125,6 +125,23 @@ describe("validateAccessPolicyConfig", () => { ).toBe(true); }); + it('emits "---"-specific mid-path wildcard diagnostic for scripts["policy"] deny rules', () => { + // "---" with a mid-path wildcard cannot be enforced at the OS layer — + // the diagnostic must say "OS-level enforcement cannot apply", not the generic prefix-match message. + const config: AccessPolicyConfig = { + scripts: { + policy: { "/home/*/secrets/**": "---" }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect( + errs.some( + (e) => + e.includes("OS-level") && e.includes("cannot apply") && e.includes('scripts["policy"]'), + ), + ).toBe(true); + }); + it("emits mid-path wildcard diagnostic for per-script policy entries", () => { const config: AccessPolicyConfig = { scripts: { @@ -137,6 +154,21 @@ describe("validateAccessPolicyConfig", () => { ); }); + it('emits "---"-specific mid-path wildcard diagnostic for per-script deny rules', () => { + // Same as scripts["policy"] — per-script "---" mid-path must get the stronger warning. + const config: AccessPolicyConfig = { + scripts: { + "/deploy.sh": { policy: { "/home/*/secrets/**": "---" } }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect( + errs.some( + (e) => e.includes("OS-level") && e.includes("cannot apply") && e.includes("/deploy.sh"), + ), + ).toBe(true); + }); + it("validates scripts[].policy perm strings and emits diagnostics for bad ones", () => { // A typo like "rwX" in a script's policy must produce a diagnostic, not silently // fail closed (which would deny exec with no operator-visible error). @@ -1124,6 +1156,28 @@ describe("applyScriptPolicyOverride", () => { } }); + it("uppercase sha256 in config matches (case-normalized at comparison)", () => { + // Validation regex uses /i so uppercase passes; crypto.digest("hex") returns lowercase. + // Without .toLowerCase() at comparison, uppercase sha256 always fails at runtime — silent + // misconfiguration that denies exec with no useful error. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "ap-test-")); + const scriptPath = path.join(tmpDir, "script.sh"); + const content = "#!/bin/sh\necho hi\n"; + fs.writeFileSync(scriptPath, content); + const hashLower = crypto.createHash("sha256").update(Buffer.from(content)).digest("hex"); + const hashUpper = hashLower.toUpperCase(); + const realScriptPath = fs.realpathSync(scriptPath); + try { + const base: AccessPolicyConfig = { + scripts: { [scriptPath]: { sha256: hashUpper, policy: { "/tmp/**": "rwx" } } }, + }; + const { hashMismatch } = applyScriptPolicyOverride(base, realScriptPath); + expect(hashMismatch).toBeUndefined(); + } finally { + 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. diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 7f5831d05fa..9ebc7cec471 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -137,9 +137,15 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] !_midPathWildcardWarned.has(`scripts:policy:${pattern}`) ) { _midPathWildcardWarned.add(`scripts:policy:${pattern}`); - errors.push( - `access-policy.scripts["policy"]["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, - ); + if (perm === "---") { + errors.push( + `access-policy.scripts["policy"]["${pattern}"] contains a mid-path wildcard with "---" — OS-level (bwrap/Seatbelt) enforcement cannot apply; tool-layer enforcement is still active.`, + ); + } else { + errors.push( + `access-policy.scripts["policy"]["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, + ); + } } autoExpandBareDir(sharedPolicy, pattern, perm, errors); } @@ -179,9 +185,15 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] !_midPathWildcardWarned.has(`scripts:${scriptPath}:${pattern}`) ) { _midPathWildcardWarned.add(`scripts:${scriptPath}:${pattern}`); - errors.push( - `access-policy.scripts["${scriptPath}"].policy["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, - ); + if (perm === "---") { + errors.push( + `access-policy.scripts["${scriptPath}"].policy["${pattern}"] contains a mid-path wildcard with "---" — OS-level (bwrap/Seatbelt) enforcement cannot apply; tool-layer enforcement is still active.`, + ); + } else { + errors.push( + `access-policy.scripts["${scriptPath}"].policy["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`, + ); + } } autoExpandBareDir(scriptEntry.policy, pattern, perm, errors); } @@ -648,7 +660,10 @@ export function applyScriptPolicyOverride( } catch { return { policy, hashMismatch: true }; } - if (actualHash !== override.sha256) { + // Normalize to lowercase: crypto.digest("hex") always returns lowercase, but + // the validation regex accepts uppercase (/i). Without normalization an uppercase + // sha256 in config passes validation and then silently fails here at runtime. + if (actualHash !== override.sha256.toLowerCase()) { return { policy, hashMismatch: true }; } } diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index 639ac353b7a..0e347a5180d 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -193,6 +193,24 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(tmpfsMounts).toContain(`${HOME}/scripts`); }); + it('"---" rule on SYSTEM_RO_BIND_PATHS path emits --tmpfs in restrictive mode', () => { + // SYSTEM_RO_BIND_PATHS (/etc, /usr, /bin, /lib, /lib64, /sbin, /opt) are unconditionally + // --ro-bind-try mounted in restrictive mode. Without a --tmpfs overlay, a "---" rule on + // e.g. "/etc/**" has no OS-level effect — syscalls inside the sandbox can still read + // /etc/passwd, /etc/shadow, etc. The fix: treat deny rules the same in both modes. + const config: AccessPolicyConfig = { + policy: { "/etc/**": "---" }, + }; + const args = generateBwrapArgs(config, HOME); + const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); + expect(tmpfsMounts).toContain("/etc"); + // Must NOT emit a read mount for a deny rule. + const roBound = args + .map((a, i) => (a === "--ro-bind-try" ? args[i + 1] : null)) + .filter(Boolean); + expect(roBound).not.toContain("/etc"); + }); + it('"---" rules do not create --ro-bind-try mounts in restrictive mode', () => { // A rule with "---" permission should NOT produce any bwrap mount — the // restrictive base already denies by not mounting. Emitting --ro-bind-try @@ -410,6 +428,41 @@ describe.skipIf(process.platform !== "linux")("generateBwrapArgs", () => { expect(bindOf(withSlash)).toContain("/tmp"); expect(bindOf(withSlash)).toEqual(bindOf(withGlob)); }); + + it("malformed perm string in base rules emits no mount (fail closed, not --ro-bind-try)", () => { + // A malformed perm like "rwxoops" must not produce a --ro-bind-try mount. + // Previously the else-if branch accessed perm[0] without VALID_PERM_RE guard, + // which could emit --ro-bind-try for a rule meant to be restrictive. + const config: AccessPolicyConfig = { + policy: { + [`${HOME}/workspace/**`]: + "rwxoops" as unknown as import("../config/types.tools.js").PermStr, + }, + }; + const args = generateBwrapArgs(config, HOME); + const roBound = args + .map((a, i) => (a === "--ro-bind-try" ? args[i + 1] : null)) + .filter(Boolean); + const rwBound = args.map((a, i) => (a === "--bind-try" ? args[i + 1] : null)).filter(Boolean); + // Malformed perm must not produce any mount for this path. + expect(roBound).not.toContain(`${HOME}/workspace`); + expect(rwBound).not.toContain(`${HOME}/workspace`); + }); + + it("malformed perm string in script override emits no --ro-bind-try (fail closed)", () => { + // Same VALID_PERM_RE guard required in the scriptOverrideRules loop. + const config: AccessPolicyConfig = { policy: { "/**": "r--" } }; + const overrides = { + [`${HOME}/data/**`]: "rwxoops" as unknown as import("../config/types.tools.js").PermStr, + }; + const args = generateBwrapArgs(config, HOME, overrides); + const roBound = args + .map((a, i) => (a === "--ro-bind-try" ? args[i + 1] : null)) + .filter(Boolean); + const rwBound = args.map((a, i) => (a === "--bind-try" ? args[i + 1] : null)).filter(Boolean); + expect(roBound).not.toContain(`${HOME}/data`); + expect(rwBound).not.toContain(`${HOME}/data`); + }); }); describe("wrapCommandWithBwrap", () => { diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index fb21437413b..226ec4a009c 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -215,16 +215,22 @@ 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 (catchAllPerm[0] !== "r" && perm[0] === "r") { + } else if (VALID_PERM_RE.test(perm) && 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. + // VALID_PERM_RE guard: malformed perm falls through to no-op (fail closed). args.push("--ro-bind-try", p, p); - } 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 - // default are silently ignored at the bwrap layer. + } else if (VALID_PERM_RE.test(perm) && perm[0] !== "r") { + // Deny/exec-only rule: overlay with --tmpfs to hide the path. + // Two cases handled identically: + // Permissive base (catchAllPerm[0] === "r"): --ro-bind / / made path readable; + // --tmpfs hides it. + // Restrictive base (catchAllPerm[0] !== "r"): SYSTEM_RO_BIND_PATHS unconditionally + // mounts /etc, /usr, /bin, /lib, /lib64, /sbin, /opt; a "---" rule on those paths + // had no effect without this branch because the three prior branches all require + // perm[0] === "r". For non-system paths in restrictive mode, --tmpfs is a no-op + // (nothing mounted there to overlay), so emitting it is harmless. // Guard: bwrap --tmpfs only accepts a directory as the mount point. If the // resolved path is a file, skip the mount and warn — same behaviour as deny[]. // Non-existent paths are assumed to be directories (forward-protection). @@ -241,6 +247,7 @@ export function generateBwrapArgs( } } // Permissive base + read-only rule: already covered by --ro-bind / /; no extra mount. + // Restrictive base + read-only rule: emitted as --ro-bind-try above. } // Script-specific override mounts — emitted after base rules so they can reopen @@ -261,7 +268,8 @@ export function generateBwrapArgs( // and writes succeed. bwrap mounts are ordered; this override comes after // deny[] tmpfs entries, so --bind-try wins regardless of the base policy. args.push("--bind-try", p, p); - } else if (perm[0] === "r") { + } else if (VALID_PERM_RE.test(perm) && perm[0] === "r") { + // VALID_PERM_RE guard: malformed perm falls through to the deny branch below. args.push("--ro-bind-try", p, p); } else { // Mirror the base-rules isDir guard — bwrap --tmpfs only accepts directories. diff --git a/src/infra/exec-sandbox-seatbelt.test.ts b/src/infra/exec-sandbox-seatbelt.test.ts index a3202abdc93..8b00b4dd955 100644 --- a/src/infra/exec-sandbox-seatbelt.test.ts +++ b/src/infra/exec-sandbox-seatbelt.test.ts @@ -226,6 +226,20 @@ describe("generateSeatbeltProfile", () => { expect(profile).not.toContain("**"); expect(profile).toContain("/Users/kaveri/.ssh"); }); + + it("bracket glob patterns are treated as wildcards, not SBPL literals", () => { + // Previously /[*?]/ missed [ — a pattern like "/usr/bin/[abc]" was emitted as + // sbplLiteral("/usr/bin/[abc]") which only matches a file literally named "[abc]". + // Fix: /[*?[]/ detects bracket globs and strips to the concrete prefix. + const config: AccessPolicyConfig = { + policy: { "/**": "rwx", "/usr/bin/[abc]": "---" as const }, + }; + const profile = generateSeatbeltProfile(config, HOME); + // Must NOT emit the literal bracket pattern. + expect(profile).not.toContain("[abc]"); + // Must use the concrete prefix /usr/bin as an approximate subpath target. + expect(profile).toContain("/usr/bin"); + }); }); describe("wrapCommandWithSeatbelt", () => { diff --git a/src/infra/exec-sandbox-seatbelt.ts b/src/infra/exec-sandbox-seatbelt.ts index bbe7f871ccc..d77a0bc3176 100644 --- a/src/infra/exec-sandbox-seatbelt.ts +++ b/src/infra/exec-sandbox-seatbelt.ts @@ -108,12 +108,15 @@ function patternToSbplMatcher(pattern: string, homeDir: string, perm?: PermStr): // Both * and ? are wildcard characters in glob syntax; strip from whichever // appears first so patterns like "/tmp/file?.txt" don't embed a literal ? // in the SBPL literal matcher. - const withoutWild = expanded.replace(/[/\\]?[*?].*$/, ""); + // Strip from the first glob metacharacter (*, ?, or [) to get the longest concrete prefix. + const withoutWild = expanded.replace(/[/\\]?[*?[].*$/, ""); const base = withoutWild || "/"; // If the original pattern had wildcards, use subpath (recursive match). - // Otherwise use literal (exact match). - if (/[*?]/.test(expanded)) { + // Includes bracket globs ([abc]) — previously only * and ? were detected, + // causing [abc] to be emitted as an SBPL literal that only matches a file + // literally named "[abc]", not the intended character-class targets. + if (/[*?[]/.test(expanded)) { const wildcardIdx = expanded.search(/[*?[]/); const afterWildcard = expanded.slice(wildcardIdx + 1); if (/[/\\]/.test(afterWildcard)) {