diff --git a/src/agents/bash-tools.exec-runtime.test.ts b/src/agents/bash-tools.exec-runtime.test.ts index 35a38b5483d..53291425e70 100644 --- a/src/agents/bash-tools.exec-runtime.test.ts +++ b/src/agents/bash-tools.exec-runtime.test.ts @@ -10,7 +10,11 @@ vi.mock("../infra/system-events.js", () => ({ import { requestHeartbeatNow } from "../infra/heartbeat-wake.js"; import { enqueueSystemEvent } from "../infra/system-events.js"; -import { emitExecSystemEvent } from "./bash-tools.exec-runtime.js"; +import { + _resetBwrapUnavailableWarnedForTest, + _resetWindowsUnconfiguredWarnedForTest, + emitExecSystemEvent, +} from "./bash-tools.exec-runtime.js"; const requestHeartbeatNowMock = vi.mocked(requestHeartbeatNow); const enqueueSystemEventMock = vi.mocked(enqueueSystemEvent); @@ -62,3 +66,21 @@ describe("emitExecSystemEvent", () => { expect(requestHeartbeatNowMock).not.toHaveBeenCalled(); }); }); + +// --------------------------------------------------------------------------- +// One-time warning reset helpers (exported for tests) +// --------------------------------------------------------------------------- + +describe("_resetBwrapUnavailableWarnedForTest / _resetWindowsUnconfiguredWarnedForTest", () => { + it("exports _resetBwrapUnavailableWarnedForTest as a function", () => { + // Verify the export exists and is callable — the reset enables repeated + // warning tests without cross-test state leakage. + expect(typeof _resetBwrapUnavailableWarnedForTest).toBe("function"); + expect(() => _resetBwrapUnavailableWarnedForTest()).not.toThrow(); + }); + + it("exports _resetWindowsUnconfiguredWarnedForTest as a function", () => { + expect(typeof _resetWindowsUnconfiguredWarnedForTest).toBe("function"); + expect(() => _resetWindowsUnconfiguredWarnedForTest()).not.toThrow(); + }); +}); diff --git a/src/agents/bash-tools.exec-runtime.ts b/src/agents/bash-tools.exec-runtime.ts index 67cd18ced2e..3d63b08a358 100644 --- a/src/agents/bash-tools.exec-runtime.ts +++ b/src/agents/bash-tools.exec-runtime.ts @@ -311,6 +311,11 @@ function _warnBwrapUnavailableOnce(): void { ); } +/** Reset the one-time bwrap-unavailable warning flag. Only for use in tests. */ +export function _resetBwrapUnavailableWarnedForTest(): void { + _bwrapUnavailableWarned = false; +} + let _windowsUnconfiguredWarned = false; function _warnWindowsUnconfiguredOnce(): void { if (_windowsUnconfiguredWarned) { @@ -322,6 +327,11 @@ function _warnWindowsUnconfiguredOnce(): void { ); } +/** Reset the one-time Windows-unconfigured warning flag. Only for use in tests. */ +export function _resetWindowsUnconfiguredWarnedForTest(): void { + _windowsUnconfiguredWarned = false; +} + export async function runExecProcess(opts: { command: string; // Execute this instead of `command` (which is kept for display/session/logging). diff --git a/src/infra/access-policy-file.test.ts b/src/infra/access-policy-file.test.ts index 62b014b5a94..afd54177624 100644 --- a/src/infra/access-policy-file.test.ts +++ b/src/infra/access-policy-file.test.ts @@ -102,6 +102,61 @@ describe("mergeAccessPolicy", () => { expect(result?.deny).toBeUndefined(); expect(result?.rules).toBeUndefined(); }); + + it("scripts deep-merge: base sha256 is preserved when override supplies same script key", () => { + // Security regression: a shallow spread ({ ...base.scripts, ...override.scripts }) would + // silently drop the admin-configured sha256 hash check, defeating integrity enforcement. + const base = { + scripts: { + "/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[]. + rules: { "~/deploy/**": "r--" as const }, // narrower override — fine + deny: ["~/extra-deny/**"], + }, + }, + }; + const result = mergeAccessPolicy(base, override); + 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--"); + }); + + it("scripts deep-merge: override-only script key is added verbatim", () => { + const base = { scripts: { "/bin/existing.sh": { sha256: "deadbeef" } } }; + const override = { + scripts: { "/bin/new.sh": { rules: { "/tmp/**": "rwx" as const } } }, + }; + const result = mergeAccessPolicy(base, override); + // Base script untouched. + expect(result?.scripts?.["/bin/existing.sh"]?.sha256).toBe("deadbeef"); + // 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/**"); + }); }); // --------------------------------------------------------------------------- diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index 1ce1fb5f2b4..60955ffaf5d 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -50,8 +50,30 @@ export function mergeAccessPolicy( } const deny = [...(base.deny ?? []), ...(override.deny ?? [])]; const rules = { ...base.rules, ...override.rules }; - // scripts: shallow merge — override key wins (same semantics as rules) - const scripts = { ...base.scripts, ...override.scripts }; + // scripts: deep-merge per key — base sha256 and deny[] are 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. + const mergedScripts: NonNullable = { ...base.scripts }; + for (const [key, overrideEntry] of Object.entries(override.scripts ?? {})) { + const baseEntry = base.scripts?.[key]; + if (!baseEntry) { + 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 } : {}), + // rules: shallow-merge, override key wins on collision. + ...(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; @@ -59,7 +81,7 @@ export function mergeAccessPolicy( if (Object.keys(rules).length > 0) { result.rules = rules; } - if (Object.keys(scripts).length > 0) { + if (scripts) { result.scripts = scripts; } if (override.default !== undefined) { @@ -228,7 +250,7 @@ export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfi // Only print the footer when there are real permission-string errors — // auto-expand diagnostics ("rule auto-expanded to ...") are informational // and the footer would mislead operators into thinking the policy is broken. - if (errors.some((e) => !e.includes("auto-expanded"))) { + if (errors.some((e) => !e.includes("auto-expanded") && !e.includes("mid-path wildcard"))) { console.error(`[access-policy] Bad permission strings are treated as "---" (deny all).`); } } diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index c34536b6beb..f9f16559190 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -6,6 +6,7 @@ import { beforeEach, describe, expect, it } from "vitest"; import type { AccessPolicyConfig } from "../config/types.tools.js"; import { _resetAutoExpandedWarnedForTest, + _resetMidPathWildcardWarnedForTest, applyScriptPolicyOverride, checkAccessPolicy, findBestRule, @@ -26,6 +27,7 @@ const HOME = os.homedir(); describe("validateAccessPolicyConfig", () => { beforeEach(() => { _resetAutoExpandedWarnedForTest(); + _resetMidPathWildcardWarnedForTest(); }); it("returns no errors for a valid config", () => { @@ -255,6 +257,48 @@ describe("validateAccessPolicyConfig", () => { }), ).toEqual([]); }); + + it("emits a one-time diagnostic for mid-path wildcard rules (OS-level enforcement skipped)", () => { + _resetMidPathWildcardWarnedForTest(); + // "/home/*/secrets/**" has a wildcard in a non-final segment — bwrap and + // Seatbelt cannot derive a concrete mount path so they skip it silently. + // validateAccessPolicyConfig must surface this so operators know. + const errs = validateAccessPolicyConfig({ + rules: { "/home/*/secrets/**": "---" }, + }); + expect(errs).toHaveLength(1); + expect(errs[0]).toMatch(/mid-path wildcard/); + expect(errs[0]).toMatch(/OS-level.*enforcement/); + }); + + it("deduplicates mid-path wildcard rule diagnostics across calls", () => { + _resetMidPathWildcardWarnedForTest(); + const config = { rules: { "/home/*/secrets/**": "---" } }; + const first = validateAccessPolicyConfig(config); + const second = validateAccessPolicyConfig(config); + expect(first.filter((e) => e.includes("mid-path wildcard"))).toHaveLength(1); + 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/**"], + }); + expect(errs.filter((e) => e.includes("mid-path wildcard"))).toHaveLength(0); + }); }); // --------------------------------------------------------------------------- diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 757af4d6f9e..860d6d0505f 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -18,6 +18,28 @@ export function _resetAutoExpandedWarnedForTest(): void { _autoExpandedWarned.clear(); } +// Track mid-path wildcard patterns already warned about — one diagnostic per pattern. +const _midPathWildcardWarned = new Set(); + +/** Reset the mid-path wildcard warning set. Only for use in tests. */ +export function _resetMidPathWildcardWarnedForTest(): void { + _midPathWildcardWarned.clear(); +} + +/** + * Returns true when a glob pattern has a wildcard character (*, ?, or bracket) + * in a non-final path segment (e.g. "/home/*\/secrets/**"). + * bwrap and Seatbelt both skip such patterns at the OS layer because the + * concrete mount/deny path cannot be derived — only the tool layer enforces them. + */ +function hasMidPathWildcard(pattern: string): boolean { + const wildcardIdx = pattern.search(/[*?[]/); + if (wildcardIdx === -1) { + return false; + } + return /[/\\]/.test(pattern.slice(wildcardIdx)); +} + /** * Validates and normalizes an AccessPolicyConfig for well-formedness. * Returns an array of human-readable diagnostic strings; empty = valid. @@ -42,6 +64,12 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] `access-policy.rules["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`, ); } + if (hasMidPathWildcard(pattern) && !_midPathWildcardWarned.has(`rules:${pattern}`)) { + _midPathWildcardWarned.add(`rules:${pattern}`); + errors.push( + `access-policy.rules["${pattern}"] contains a mid-path wildcard — OS-level (bwrap/Seatbelt) enforcement cannot apply; tool-layer enforcement is still active.`, + ); + } // If a bare path (no glob metacharacters, no trailing /) points to a real // directory it would match only the directory entry itself, not its // contents. Auto-expand to "/**" and notify — the fix is unambiguous. @@ -105,6 +133,12 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] 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