fix(access-policy): bwrap tmpfs file guard, DENY_ALL freeze, access read-only check, seatbelt ? wildcard strip

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);

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<string, unknown>)["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,

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();

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.

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

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")');
+  });
 });

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).