fix(access-policy): script-override exec auth, deny[] unconditional expand, fail-closed on broken file

src/agents/bash-tools.exec-runtime.ts

@@ -363,7 +363,12 @@ export async function runExecProcess(opts: {
     // Tool-layer exec path check — defense-in-depth for platforms where OS-level
     // enforcement (seatbelt/bwrap) is unavailable (Linux without bwrap, Windows).
     // Mirrors the checkAccessPolicy calls in read/write tools for consistency.
-    if (checkAccessPolicy(argv0, "exec", effectivePermissions) === "deny") {
+    //
+    // When a policy.scripts entry was found and sha256 passed, the hash check IS
+    // the exec-authorization signal — skip the base exec check so scripts don't
+    // also need a separate exec rule in the base policy to run.
+    const hasScriptOverride = argv0 in (opts.permissions.scripts ?? {});
+    if (!hasScriptOverride && checkAccessPolicy(argv0, "exec", effectivePermissions) === "deny") {
       throw new Error(`exec denied by access policy: ${argv0}`);
     }
     if (process.platform === "darwin") {

src/infra/access-policy-file.test.ts

@@ -3,6 +3,7 @@ import os from "node:os";
 import path from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import {
+  BROKEN_POLICY_FILE,
   _resetNotFoundWarnedForTest,
   loadAccessPolicyFile,
   mergeAccessPolicy,
@@ -112,69 +113,69 @@ describe("loadAccessPolicyFile", () => {
     expect(loadAccessPolicyFile()).toBeNull();
   });
 
-  it("returns null and logs error when file is invalid JSON", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when file is invalid JSON", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     const p = resolveAccessPolicyPath();
     fs.writeFileSync(p, "not json {{ broken");
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining("Cannot parse"));
-    expect(spy).toHaveBeenCalledWith(expect.stringContaining("DISABLED"));
+    expect(spy).toHaveBeenCalledWith(expect.stringContaining("Failing closed"));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when version is not 1", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when version is not 1", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 2, base: {} });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining("unsupported version"));
-    expect(spy).toHaveBeenCalledWith(expect.stringContaining("DISABLED"));
+    expect(spy).toHaveBeenCalledWith(expect.stringContaining("Failing closed"));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when base is not an object", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when base is not an object", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 1, base: ["r--"] });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining('"base" must be an object'));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when agents is not an object", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when agents is not an object", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 1, agents: "bad" });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining('"agents" must be an object'));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when a top-level key like 'rules' is misplaced", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when a top-level key like 'rules' is misplaced", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     // Common mistake: rules at top level instead of under base
     writeFile({ version: 1, rules: { "/**": "r--" } });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining('unexpected top-level key "rules"'));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when 'deny' is misplaced at top level", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when 'deny' is misplaced at top level", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 1, deny: ["~/.ssh/**"] });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining('unexpected top-level key "deny"'));
     spy.mockRestore();
   });
 
-  it("returns null and logs error when an agent block is not an object", () => {
+  it("returns BROKEN_POLICY_FILE and logs error when an agent block is not an object", () => {
     const spy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 1, agents: { subri: "rwx" } });
     const result = loadAccessPolicyFile();
-    expect(result).toBeNull();
+    expect(result).toBe(BROKEN_POLICY_FILE);
     expect(spy).toHaveBeenCalledWith(expect.stringContaining('agents["subri"] must be an object'));
     spy.mockRestore();
   });
@@ -220,13 +221,15 @@ describe("resolveAccessPolicyForAgent", () => {
     warnSpy.mockRestore();
   });
 
-  it("does not warn when config file exists but is broken (error already logged)", () => {
+  it("returns deny-all and logs error when config file is broken (fail-closed)", () => {
     const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
     const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
     writeFile({ version: 1, rules: { "/**": "r--" } }); // misplaced key — triggers error
-    resolveAccessPolicyForAgent("subri");
+    const result = resolveAccessPolicyForAgent("subri");
     expect(warnSpy).not.toHaveBeenCalled();
-    expect(errSpy).toHaveBeenCalledWith(expect.stringContaining("DISABLED"));
+    expect(errSpy).toHaveBeenCalledWith(expect.stringContaining("Failing closed"));
+    // Broken file must fail-closed: deny-all policy, not undefined
+    expect(result).toEqual({ default: "---" });
     warnSpy.mockRestore();
     errSpy.mockRestore();
   });

src/infra/access-policy-file.ts

@@ -110,10 +110,20 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s
 }
 
 /**
- * Read and parse the sidecar file. Returns null if the file does not exist.
- * Logs a clear error (and returns null) if the file is present but broken.
+ * Sentinel returned by loadAccessPolicyFile when the file exists but is broken.
+ * Callers must treat this as a deny-all policy (default:"---") rather than
+ * disabling enforcement — a corrupted file should fail-closed, not fail-open.
  */
-export function loadAccessPolicyFile(): AccessPolicyFile | null {
+export const BROKEN_POLICY_FILE = Symbol("broken-policy-file");
+
+/**
+ * Read and parse the sidecar file.
+ * - Returns null if the file does not exist (opt-in not configured).
+ * - Returns BROKEN_POLICY_FILE if the file exists but is malformed/unreadable
+ *   (callers must treat this as default:"---" — fail-closed).
+ * - Returns the parsed file on success.
+ */
+export function loadAccessPolicyFile(): AccessPolicyFile | null | typeof BROKEN_POLICY_FILE {
   const filePath = resolveAccessPolicyPath();
   if (!fs.existsSync(filePath)) {
     return null;
@@ -127,14 +137,14 @@ export function loadAccessPolicyFile(): AccessPolicyFile | null {
     console.error(
       `[access-policy] Cannot parse ${filePath}: ${err instanceof Error ? err.message : String(err)}`,
     );
-    console.error(`[access-policy] Permissions enforcement is DISABLED until the file is fixed.`);
-    return null;
+    console.error(`[access-policy] Failing closed (default: "---") until the file is fixed.`);
+    return BROKEN_POLICY_FILE;
   }
 
   if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
     console.error(`[access-policy] ${filePath}: must be a JSON object at the top level.`);
-    console.error(`[access-policy] Permissions enforcement is DISABLED until the file is fixed.`);
-    return null;
+    console.error(`[access-policy] Failing closed (default: "---") until the file is fixed.`);
+    return BROKEN_POLICY_FILE;
   }
 
   const p = parsed as Record<string, unknown>;
@@ -142,8 +152,8 @@ export function loadAccessPolicyFile(): AccessPolicyFile | null {
     console.error(
       `[access-policy] ${filePath}: unsupported version ${JSON.stringify(p["version"])} (expected 1).`,
     );
-    console.error(`[access-policy] Permissions enforcement is DISABLED until the file is fixed.`);
-    return null;
+    console.error(`[access-policy] Failing closed (default: "---") until the file is fixed.`);
+    return BROKEN_POLICY_FILE;
   }
 
   // Structural validation — catches wrong nesting, misplaced keys, etc.
@@ -152,8 +162,8 @@ export function loadAccessPolicyFile(): AccessPolicyFile | null {
     for (const err of structErrors) {
       console.error(`[access-policy] ${err}`);
     }
-    console.error(`[access-policy] Permissions enforcement is DISABLED until the file is fixed.`);
-    return null;
+    console.error(`[access-policy] Failing closed (default: "---") until the file is fixed.`);
+    return BROKEN_POLICY_FILE;
   }
 
   return parsed as AccessPolicyFile;
@@ -179,8 +189,15 @@ 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 = { default: "---" };
+
 export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfig | undefined {
   const file = loadAccessPolicyFile();
+  if (file === BROKEN_POLICY_FILE) {
+    // File exists but is malformed — fail-closed: deny everything until fixed.
+    return DENY_ALL_POLICY;
+  }
   if (!file) {
     // access-policy.json is entirely opt-in — silently return undefined when the
     // file is absent so users who have not configured the feature see no noise.

src/infra/access-policy.test.ts

@@ -95,6 +95,16 @@ describe("validateAccessPolicyConfig", () => {
     expect(config.deny?.[0]).toBe(`${dir}/**`);
   });
 
+  it("auto-expands bare deny[] entry even when the directory does not yet exist", () => {
+    // Greptile: deny[] must expand unconditionally — stat-gated expansion leaves
+    // non-existent paths unexpanded, silently allowing files created there later.
+    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("accepts valid 'rwx' and '---' perm strings", () => {
     expect(validateAccessPolicyConfig({ default: "rwx" })).toEqual([]);
     expect(validateAccessPolicyConfig({ default: "---" })).toEqual([]);

src/infra/access-policy.ts

@@ -82,25 +82,21 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[]
         errors.push(`access-policy.deny[${i}] must be a non-empty glob pattern`);
         continue;
       }
-      // Same bare-directory auto-expand as rules: "~/.ssh" → "~/.ssh/**" so the
-      // entire directory is denied, not just the directory entry itself.
+      // Unconditional bare-path auto-expand: "~/.ssh" → "~/.ssh/**" so the
+      // entire directory tree is denied, not just the directory inode itself.
+      // Unlike rules (where an allow for a non-existent path is harmless), deny[]
+      // entries are proactive security controls — a user writing deny:["~/.creds"]
+      // intends to block that subtree even before the directory is created. Relying
+      // on statSync would silently leave the bare pattern unexpanded if the directory
+      // doesn't exist yet, creating a gap when it is later created.
       if (!pattern.endsWith("/") && !/[*?[]/.test(pattern)) {
-        const expanded = pattern.startsWith("~")
-          ? pattern.replace(/^~(?=$|\/)/, os.homedir())
-          : pattern;
-        try {
-          if (fs.statSync(expanded).isDirectory()) {
-            const fixed = `${pattern}/**`;
-            config.deny[i] = fixed;
-            if (!_autoExpandedWarned.has(`deny:${pattern}`)) {
-              _autoExpandedWarned.add(`deny:${pattern}`);
-              errors.push(
-                `access-policy.deny["${pattern}"] is a directory — entry auto-expanded to "${fixed}" so it covers all contents.`,
-              );
-            }
-          }
-        } catch {
-          // Path inaccessible or missing — no action needed.
+        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.`,
+          );
         }
       }
     }