From db6ddaf0b3b9253cef9ef77191cfd84add63b631 Mon Sep 17 00:00:00 2001 From: subrih Date: Fri, 13 Mar 2026 19:11:51 -0700 Subject: [PATCH] fix(access-policy): bwrap tmpfs file guard, DENY_ALL freeze, access read-only check, seatbelt ? wildcard strip --- src/agents/pi-tools.read.ts | 15 +++++++++++++-- src/infra/access-policy-file.test.ts | 18 ++++++++++++++++++ src/infra/access-policy-file.ts | 2 +- src/infra/exec-sandbox-bwrap.test.ts | 19 +++++++++++++++++++ src/infra/exec-sandbox-bwrap.ts | 18 +++++++++++++++++- src/infra/exec-sandbox-seatbelt.test.ts | 9 +++++++++ src/infra/exec-sandbox-seatbelt.ts | 5 ++++- 7 files changed, 81 insertions(+), 5 deletions(-) diff --git a/src/agents/pi-tools.read.ts b/src/agents/pi-tools.read.ts index ac2103ac655..8c695744cee 100644 --- a/src/agents/pi-tools.read.ts +++ b/src/agents/pi-tools.read.ts @@ -864,6 +864,17 @@ function createHostEditOperations( return resolved; } + // access() checks existence only — requires read permission but not write. + // Using assertEditPermitted here would block existence checks on r-- paths before + // any write is attempted, producing a misleading "write access denied" error. + function assertReadPermitted(absolutePath: string): string { + const resolved = safeRealpath(absolutePath); + if (permissions && checkAccessPolicy(resolved, "read", permissions) === "deny") { + throw new Error(`Permission denied: read access to ${resolved} is not allowed.`); + } + return resolved; + } + if (!workspaceOnly) { // When workspaceOnly is false, allow edits anywhere on the host return { @@ -876,7 +887,7 @@ function createHostEditOperations( await writeHostFile(resolved, content); }, access: async (absolutePath: string) => { - const resolved = assertEditPermitted(absolutePath); + const resolved = assertReadPermitted(absolutePath); await fs.access(resolved); }, } as const; @@ -904,7 +915,7 @@ function createHostEditOperations( }); }, access: async (absolutePath: string) => { - const resolved = assertEditPermitted(absolutePath); + const resolved = assertReadPermitted(absolutePath); let relative: string; try { relative = toRelativeWorkspacePath(resolvedRoot, resolved); diff --git a/src/infra/access-policy-file.test.ts b/src/infra/access-policy-file.test.ts index 58cc8ba8e10..4d8f64862f2 100644 --- a/src/infra/access-policy-file.test.ts +++ b/src/infra/access-policy-file.test.ts @@ -234,6 +234,24 @@ describe("resolveAccessPolicyForAgent", () => { errSpy.mockRestore(); }); + it("deny-all policy returned on broken file is frozen — mutation does not corrupt future calls", () => { + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + writeFile({ version: 1, rules: { "/**": "r--" } }); // misplaced key — broken + const result = resolveAccessPolicyForAgent("subri"); + expect(result).toEqual({ default: "---" }); + // 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"; + } catch { + // Object.freeze throws in strict mode — that's fine too. + } + _resetNotFoundWarnedForTest(); + const result2 = resolveAccessPolicyForAgent("subri"); + expect(result2).toEqual({ default: "---" }); + errSpy.mockRestore(); + }); + it("returns base when no agent block exists", () => { writeFile({ version: 1, diff --git a/src/infra/access-policy-file.ts b/src/infra/access-policy-file.ts index d41e519b259..1ce1fb5f2b4 100644 --- a/src/infra/access-policy-file.ts +++ b/src/infra/access-policy-file.ts @@ -190,7 +190,7 @@ export function _resetNotFoundWarnedForTest(): void { * 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 = { default: "---" }; +const DENY_ALL_POLICY: AccessPolicyConfig = Object.freeze({ default: "---" }); export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfig | undefined { const file = loadAccessPolicyFile(); diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index 2d6f8d15853..d02c45dbdc7 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -282,6 +282,25 @@ describe("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/**"] }; + 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("still emits --tmpfs for deny[] entry that resolves to a directory", () => { + // Non-existent paths are treated as directories (forward-protection). + const config: AccessPolicyConfig = { default: "r--", deny: [`${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`); + }); + 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. diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 9ba6b84ea26..f7b052c8c8e 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -1,4 +1,5 @@ import { execFile } from "node:child_process"; +import fs from "node:fs"; import os from "node:os"; import { promisify } from "node:util"; import type { AccessPolicyConfig, PermStr } from "../config/types.tools.js"; @@ -171,12 +172,27 @@ export function generateBwrapArgs( // 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; } - args.push("--tmpfs", p); + 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); + } + // File-specific entry: tool-layer checkAccessPolicy already denies reads/writes; + // bwrap cannot mount tmpfs over a file so skip the OS-layer mount silently. } // Script-specific override mounts — emitted after deny[] so they can reopen diff --git a/src/infra/exec-sandbox-seatbelt.test.ts b/src/infra/exec-sandbox-seatbelt.test.ts index c0a906b96a5..eaa97e95dec 100644 --- a/src/infra/exec-sandbox-seatbelt.test.ts +++ b/src/infra/exec-sandbox-seatbelt.test.ts @@ -300,4 +300,13 @@ describe("generateSeatbeltProfile — mid-path wildcard guard", () => { const profile = generateSeatbeltProfile({ rules: { "/tmp/**": "rwx" } }, HOME); expect(profile).toContain('(subpath "/tmp")'); }); + + skipOnWindows("? wildcard is stripped correctly — no literal ? in SBPL matcher", () => { + // Pattern "/tmp/file?.txt" has a ? wildcard; the strip regex must remove it so + // the SBPL matcher does not contain a raw "?" character. Stripping "?.txt" from + // "/tmp/file?.txt" yields "/tmp/file" — a more precise subpath than "/tmp". + const profile = generateSeatbeltProfile({ rules: { "/tmp/file?.txt": "r--" } }, HOME); + expect(profile).not.toMatch(/\?/); // no literal ? in the emitted profile + expect(profile).toContain('(subpath "/tmp/file")'); + }); }); diff --git a/src/infra/exec-sandbox-seatbelt.ts b/src/infra/exec-sandbox-seatbelt.ts index ab6412e096c..bb8ce918067 100644 --- a/src/infra/exec-sandbox-seatbelt.ts +++ b/src/infra/exec-sandbox-seatbelt.ts @@ -100,7 +100,10 @@ function patternToSbplMatcher(pattern: string, homeDir: string): string | null { : withExpanded; // Strip trailing wildcard segments to get the longest concrete prefix. - const withoutWild = expanded.replace(/[/\\]?\*.*$/, ""); + // 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(/[/\\]?[*?].*$/, ""); const base = withoutWild || "/"; // If the original pattern had wildcards, use subpath (recursive match).