fix(memorable): read origin URLs NUL-delimited, and keep a dangling ancestor symlink off the no-store fast path (security review, third pass)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -13,7 +13,7 @@
 
 ### Fixed
 - The recall hook's policy lookup is now a pure read that fails closed in either state root: a corrupt, empty or non-object store, a present key with an unknown tier, a path that is not a readable file (a dangling symlink included) and a path whose existence an unsearchable ancestor directory hides are all errors, never "no policy", and nothing is migrated, quarantined or rewritten. The opt-in counts only when it is exactly `"allow"` and every store that makes the remote read-only (including `~/.gstack` under a custom state root) opts it in.
-- The hook looks policy up by every configured `remote.origin.url`, as `get` and `/sync-gbrain` read it, instead of the first URL after `url.*.insteadOf` rewriting. A rewrite to another host, or a second origin URL, no longer dodges a deny: any denied or not-opted-in read-only URL skips recall. `hasRepoPolicyStore()` treats only confirmed absence as "no store", so a dangling symlink or an unreadable directory reaches the script's fail-closed checks instead of the no-policy fast path.
+- The hook looks policy up by every configured `remote.origin.url`, as `get` and `/sync-gbrain` read it, instead of the first URL after `url.*.insteadOf` rewriting. A rewrite to another host, or a second origin URL, no longer dodges a deny: any denied or not-opted-in read-only URL skips recall. `hasRepoPolicyStore()` treats only confirmed absence as "no store", so a dangling symlink at or above the store, a file in its path, or an unreadable directory reaches the script's fail-closed checks instead of the no-policy fast path. Origin URLs are read NUL-delimited, so a valid local path containing a newline is looked up whole.
 
 ### Docs
 - `docs/memorable-workflow-memory.md` has a short "Recall in a read-only repo" section and updated refusal and troubleshooting lines.
@@ -22,10 +22,10 @@
 - `test/memorable-user-prompt-hook.test.ts` drives the real hook: read-only stays blocked by default, the opt-in recalls on that remote and not on a second read-only remote, `deny` with a hand-written opt-in is skipped, and a corrupt store skips recall and is left in place.
 - `test/gbrain-repo-policy.test.ts` covers the CLI: per-remote keying (a different remote, a prefix-sharing remote and an unset remote stay off; the SSH spelling of the same remote matches), refusal on deny, read-write and unset, clearing on a tier change, malformed opt-ins reading off, corrupt and non-file stores in both roots failing with exit 2, and the second-store rule.
 - `test/gbrain-repo-policy-client.test.ts` covers `memorableRecallPolicy`: fast path, per-remote opt-in, deny, corrupt store, an unknown tier, a dangling-symlink store and a timeout.
-- Review hardening, each test red at the pre-review head: empty, whitespace, `null`, array, number and two-document stores and `false`/`null`/unknown tier values fail with exit 2 in either root; dangling symlinks and an unsearchable directory or ancestor fail closed; the lookup leaves a schema-less store byte-for-byte unchanged; and the hook refuses a deny remote whose origin is rewritten to another host or carries a second, unlisted URL in either order.
+- Review hardening, each test red at the pre-review head: empty, whitespace, `null`, array, number and two-document stores and `false`/`null`/unknown tier values fail with exit 2 in either root; dangling symlinks and an unsearchable directory or ancestor fail closed; the lookup leaves a schema-less store byte-for-byte unchanged; and the hook refuses a deny remote whose origin is rewritten to another host or carries a second, unlisted URL in either order, or whose local-path URL contains a newline; and a dangling symlink above the store or a file in its path skips the no-store fast path.
 
 ### Gate integrity
-- One deliberate widening of a privacy veto, approved by Garry: a read-only remote with an explicit opt-in is recalled. Nothing else is loosened, and the corrupt-store path is tightened. Fourteen mutations of the new gate were run against the focused tests. Thirteen turn at least one test red: the hook ignoring the opt-in, the hook dropping deny, the CLI reporting the opt-in on any tier, an unkeyed opt-in, a corrupt store read as no policy, the second store ignored, an opt-in accepted on deny, the hook using the rewritten URL, an existsSync fast path, any JSON document accepted, a non-file path read as absent, the hook checking only the last origin URL, and a missing path trusted without checking its ancestors. The fourteenth, the client reading unknown script output as no policy, stays green because the script can no longer produce such output. That check is a second layer and is left in place. No threshold, budget, timeout, trial count or case kind changed.
+- One deliberate widening of a privacy veto, approved by Garry: a read-only remote with an explicit opt-in is recalled. Nothing else is loosened, and the corrupt-store path is tightened. Sixteen mutations of the new gate were run against the focused tests. Fifteen turn at least one test red: the hook ignoring the opt-in, the hook dropping deny, the CLI reporting the opt-in on any tier, an unkeyed opt-in, a corrupt store read as no policy, the second store ignored, an opt-in accepted on deny, the hook using the rewritten URL, an existsSync fast path, any JSON document accepted, a non-file path read as absent, the hook checking only the last origin URL, a missing path trusted without checking its ancestors, origin URLs split on newlines, and the client fast path ignoring a dangling ancestor symlink. The sixteenth, the client reading unknown script output as no policy, stays green because the script can no longer produce such output. That check is a second layer and is left in place. No threshold, budget, timeout, trial count or case kind changed.
 - Known limit, unchanged by this PR: remotes that the store already treats as one key (`host:8443/x` and `host/8443/x`, or paths that differ only in case) share the opt-in exactly as they already share a tier.
 
 ## [1.91.186.0] - 2026-10-24

hosts/claude/hooks/memorable-user-prompt-hook.ts

@@ -399,13 +399,13 @@ async function policyVeto(cwd: string, timeoutMs: number, remaining: () => numbe
   // get` and /sync-gbrain read), not the url.*.insteadOf rewrite that
   // `remote get-url` applies, and every configured origin URL is checked:
   // neither a rewrite nor an added URL may dodge a deny.
-  const raw = await runExternal('git', ['config', '--get-all', 'remote.origin.url'], {
+  const raw = await runExternal('git', ['config', '--null', '--get-all', 'remote.origin.url'], {
     cwd, timeoutMs: Math.max(1, Math.min(timeoutMs, remaining())), maxBuffer: GIT_MAX_BUFFER, env: gitEnv(process.env),
     onSpawn: (kill) => { killInflight = kill; },
   });
   killInflight = null;
   if (raw.timedOut || raw.error || raw.status !== 0) return 'error';
-  const urls = raw.stdout.toString('utf8').split('\n').map((u) => u.trim()).filter(Boolean);
+  const urls = raw.stdout.toString('utf8').split('\0').map((u) => u.trim()).filter(Boolean);
   if (urls.length === 0) return 'error';
   for (const url of urls) {
     const res = memorableRecallPolicy(url, process.env, Math.max(1, Math.min(STAGE_CAP_MS, remaining())));

lib/gbrain-repo-policy-client.ts

@@ -17,8 +17,8 @@
  */
 
 import { spawnSync } from "child_process";
-import { lstatSync } from "fs";
-import { join } from "path";
+import { lstatSync, statSync } from "fs";
+import { dirname, join } from "path";
 import { mergedStateRoots, resolveStateRoot } from "./state-root";
 
 export type RepoPolicyTierValue = "deny" | "read-only" | "read-write" | "none";
@@ -46,19 +46,35 @@ export function repoPolicyStorePath(env: NodeJS.ProcessEnv = process.env): strin
 /**
  * No store on disk = no policy was ever set (the fast path — no subprocess).
  * The ~/.gstack store counts too: its deny tiers merge into every state root.
- * Only confirmed absence (ENOENT/ENOTDIR) is "no store": a dangling symlink,
- * or a path that cannot be checked (an unsearchable directory), counts as a
- * store, so the caller spawns the script and its own fail-closed rules apply.
+ * Only confirmed absence is "no store": ENOENT whose nearest existing
+ * ancestor resolves (so no dangling symlink cut the walk short). A dangling
+ * symlink at the store or above it, a non-directory component, or a path that
+ * cannot be checked (an unsearchable directory) counts as a store, so the
+ * caller spawns the script and its own fail-closed rules apply.
  */
 export function hasRepoPolicyStore(env: NodeJS.ProcessEnv = process.env): boolean {
   return mergedStateRoots(env).some((root) => {
+    let path = join(root, "gbrain-repo-policy.json");
     try {
-      lstatSync(join(root, "gbrain-repo-policy.json"));
+      lstatSync(path);
       return true;
     } catch (err) {
-      const code = (err as NodeJS.ErrnoException).code;
-      return code !== "ENOENT" && code !== "ENOTDIR";
+      if ((err as NodeJS.ErrnoException).code !== "ENOENT") return true;
     }
+    for (let up = dirname(path); up !== path; path = up, up = dirname(up)) {
+      try {
+        lstatSync(up);
+      } catch {
+        continue;
+      }
+      try {
+        statSync(up);
+        return false;
+      } catch {
+        return true;
+      }
+    }
+    return false;
   });
 }
 

test/gbrain-repo-policy-client.test.ts

@@ -253,6 +253,18 @@ describe("memorableRecallPolicy (#3359: per-remote Memorable recall opt-in)", ()
     expect(hasRepoPolicyStore(env())).toBe(true);
     expect(memorableRecallPolicy(A, env())).toEqual({ tier: "none", memorableOptIn: false, error: "unreadable" });
   });
+  test("a dangling symlink above the store, or a file in its path, is not the no-store fast path", () => {
+    fs.symlinkSync(path.join(tmpHome, "missing-dir"), path.join(tmpHome, "link"));
+    const viaLink = { ...env(), GSTACK_TEST_LEGACY_ROOT: path.join(tmpHome, "link", "child") };
+    expect(hasRepoPolicyStore(viaLink)).toBe(true);
+    expect(memorableRecallPolicy(A, viaLink)).toEqual({ tier: "none", memorableOptIn: false, error: "unreadable" });
+    fs.writeFileSync(path.join(tmpHome, "plain-file"), "x");
+    expect(hasRepoPolicyStore({ ...env(), GSTACK_TEST_LEGACY_ROOT: path.join(tmpHome, "plain-file", "child") })).toBe(true);
+    fs.mkdirSync(path.join(tmpHome, "real-dir"));
+    fs.symlinkSync(path.join(tmpHome, "real-dir"), path.join(tmpHome, "good-link"));
+    expect(hasRepoPolicyStore({ ...env(), GSTACK_TEST_LEGACY_ROOT: path.join(tmpHome, "good-link", "child") })).toBe(false);
+    expect(hasRepoPolicyStore({ ...env(), GSTACK_TEST_LEGACY_ROOT: path.join(tmpHome, "no", "such", "root") })).toBe(false);
+  });
   test("a timed-out spawn classifies as unreadable", () => {
     expect(run(["set", A, "read-only"]).status).toBe(0);
     expect(memorableRecallPolicy(A, env(), 1)).toEqual({ tier: "none", memorableOptIn: false, error: "unreadable" });

test/memorable-user-prompt-hook.test.ts

@@ -335,6 +335,26 @@ describe('gate on: the mediated hand-off', () => {
     }
   });
 
+  test('#3359 review: an origin URL containing a newline (a valid local path) is looked up whole, so its deny holds', () => {
+    gateOn();
+    const parent = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-nl-'));
+    try {
+      const remote = path.join(parent, 'remote\nname.git');
+      fs.mkdirSync(remote);
+      expect(spawnSync('git', ['init', '-q', '--bare', remote], { timeout: 10_000 }).status).toBe(0);
+      const repo = path.join(parent, 'work');
+      fs.mkdirSync(repo);
+      spawnSync('git', ['init', '-q'], { cwd: repo, timeout: 10_000 });
+      spawnSync('git', ['remote', 'add', 'origin', remote], { cwd: repo, timeout: 10_000 });
+      expect(spawnSync('bash', [POLICY, 'set', remote, 'deny'], { env, encoding: 'utf8', timeout: 20_000 }).status).toBe(0);
+      expect(spawnSync('bash', [POLICY, 'memorable', 'get', remote], { env, encoding: 'utf8', timeout: 20_000 }).stdout).toBe('deny\toff\n');
+      expect(runHook(JSON.stringify({ prompt: 'hello', cwd: repo }), {}, repo).stdout).toBe('');
+      expect(calls()).toBe('');
+    } finally {
+      fs.rmSync(parent, { recursive: true, force: true });
+    }
+  });
+
   test('gate flipped off between two runs: the second run spawns nothing (the mid-flight re-check itself cannot be interleaved from outside)', () => {
     // The pre-spawn re-check reads the same store; a deterministic mid-flight flip would need a
     // seam inside main(). This pins the observable contract only: once off, no spawn.