From 668d6391ae50e3c3332467505dc462706978dc77 Mon Sep 17 00:00:00 2001
From: garrytan <garrytan@gmail.com>
Date: Sat, 24 Oct 2026 20:56:26 +0000
Subject: [PATCH] fix(memorable): check every configured origin URL, and
 confirm a missing store through its ancestors (security review, second pass)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
---
 CHANGELOG.md                                  |  8 ++---
 bin/gstack-gbrain-repo-policy                 | 21 ++++++++------
 docs/memorable-workflow-memory.md             |  5 ++--
 .../hooks/memorable-user-prompt-hook.ts       | 29 ++++++++++---------
 test/gbrain-repo-policy.test.ts               | 13 +++++++++
 test/memorable-user-prompt-hook.test.ts       | 21 ++++++++++++++
 6 files changed, 69 insertions(+), 28 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d7c9171fc..54092afa8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,8 +12,8 @@
 - **`hosts/claude/hooks/memorable-user-prompt-hook.ts`**: a `read-only` remote is recalled only when it is opted in; `deny` is always skipped. The skip line in `hook-errors.log` names the opt-in command.
 
 ### 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 an unsearchable directory 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 the configured `remote.origin.url`, as `get` and `/sync-gbrain` do, instead of the URL after `url.*.insteadOf` rewriting, so a rewrite to another host no longer dodges a deny. `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 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.
 
 ### 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 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.
+- 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.
 
 ### 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. Twelve mutations of the new gate were run against the focused tests. Eleven 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, and a non-file path read as absent. The twelfth, 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. 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.
 - 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
diff --git a/bin/gstack-gbrain-repo-policy b/bin/gstack-gbrain-repo-policy
index 138ee07ae..f98c39e19 100755
--- a/bin/gstack-gbrain-repo-policy
+++ b/bin/gstack-gbrain-repo-policy
@@ -356,18 +356,21 @@ cmd_set() {
 # read from a single snapshot of the file with no write, migration or
 # quarantine (tier is none when the file or the key is absent; a legacy
 # "allow" reads as read-write in memory). Dies (exit 2) on anything else: a
-# path that is present but not a readable regular file, a directory that
-# cannot be searched, content that is not exactly one JSON object, or a
+# path that is present but not a readable regular file, a path whose
+# existence cannot be confirmed (an unsearchable ancestor), content that is not exactly one JSON object, or a
 # present key whose value is not a known tier.
 memorable_store_answer() {
-  local store="$1" key="$2" dir raw
-  dir=$(dirname "$store")
-  if [ -d "$dir" ] && ! [ -x "$dir" ]; then
-    die "cannot search $dir for the policy store — refusing the Memorable recall lookup."
-  fi
+  local store="$1" key="$2" up raw
   if ! [ -e "$store" ] && ! [ -L "$store" ]; then
-    printf 'none off\n'
-    return 0
+    # Absent only if the nearest existing ancestor is a searchable directory:
+    # an unsearchable one hides an existing store from every test above.
+    up=$(dirname "$store")
+    while ! [ -e "$up" ] && ! [ -L "$up" ] && [ "$up" != "/" ] && [ "$up" != "." ]; do up=$(dirname "$up"); done
+    if [ -d "$up" ] && [ -x "$up" ]; then
+      printf 'none off\n'
+      return 0
+    fi
+    die "cannot confirm whether $store exists ($up is not a searchable directory) — refusing the Memorable recall lookup."
   fi
   if ! [ -f "$store" ] || ! [ -r "$store" ]; then
     die "policy store $store exists but is not a readable file — refusing the Memorable recall lookup. Inspect or remove it."
diff --git a/docs/memorable-workflow-memory.md b/docs/memorable-workflow-memory.md
index 2bfb1bc18..9cf8ba715 100644
--- a/docs/memorable-workflow-memory.md
+++ b/docs/memorable-workflow-memory.md
@@ -185,8 +185,9 @@ later never silently re-opens recall. When `~/.gstack` also makes the remote
 read-only (see [state roots](state-root.md)), that store has to opt it in too.
 A corrupt, empty or unreadable policy store (or a dangling symlink in its
 place) skips recall for every repo until you fix it; the hook never resets it
-to "no policy" and never rewrites it. The lookup uses the configured
-`remote.origin.url`, not the URL after `url.*.insteadOf` rewriting. `gstack-gbrain-repo-policy list` shows
+to "no policy" and never rewrites it. The lookup checks every configured
+`remote.origin.url`, not the URL after `url.*.insteadOf` rewriting, and any
+denied or not-opted-in read-only one skips recall. `gstack-gbrain-repo-policy list` shows
 opted-in remotes with a `memorable=allow` column. Nothing about gbrain
 ingest changes: the repo stays `read-only` for `/sync-gbrain` and every
 other gbrain writer.
diff --git a/hosts/claude/hooks/memorable-user-prompt-hook.ts b/hosts/claude/hooks/memorable-user-prompt-hook.ts
index 37891e691..f040658ad 100644
--- a/hosts/claude/hooks/memorable-user-prompt-hook.ts
+++ b/hosts/claude/hooks/memorable-user-prompt-hook.ts
@@ -395,25 +395,28 @@ async function policyVeto(cwd: string, timeoutMs: number, remaining: () => numbe
   if (git.status === GIT_FATAL && NOT_A_REPO_RE.test(git.stderrTail.trim())) return 'ok';
   if (git.status !== 0) return 'error';
   if (!git.stdout.toString('utf8').trim()) return 'ok';
-  // The store is keyed by the configured URL (what `gstack-gbrain-repo-policy
+  // The store is keyed by the configured URLs (what `gstack-gbrain-repo-policy
   // get` and /sync-gbrain read), not the url.*.insteadOf rewrite that
-  // `remote get-url` applies: a rewrite to another host must not dodge a deny.
-  const raw = await runExternal('git', ['config', '--get', 'remote.origin.url'], {
+  // `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'], {
     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 url = raw.stdout.toString('utf8').trim();
-  if (!url) return 'error';
-  const res = memorableRecallPolicy(url, process.env, Math.max(1, Math.min(STAGE_CAP_MS, remaining())));
-  if (res.error) return 'error';
-  // `deny` and `read-only` are the tiers a user picks so a repo's content
-  // never lands in a shared store; a third-party memory service is one.
-  // Only an explicit per-remote opt-in (`gstack-gbrain-repo-policy memorable
-  // set <url> allow`) lifts the read-only veto; nothing lifts deny.
-  if (res.tier === 'deny') return 'skip';
-  if (res.tier === 'read-only') return res.memorableOptIn ? 'ok' : 'skip';
+  const urls = raw.stdout.toString('utf8').split('\n').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())));
+    if (res.error) return 'error';
+    // `deny` and `read-only` are the tiers a user picks so a repo's content
+    // never lands in a shared store; a third-party memory service is one.
+    // Only an explicit per-remote opt-in (`gstack-gbrain-repo-policy memorable
+    // set <url> allow`) lifts the read-only veto; nothing lifts deny.
+    if (res.tier === 'deny') return 'skip';
+    if (res.tier === 'read-only' && !res.memorableOptIn) return 'skip';
+  }
   return 'ok';
 }
 
diff --git a/test/gbrain-repo-policy.test.ts b/test/gbrain-repo-policy.test.ts
index f3d1d22be..60436c450 100644
--- a/test/gbrain-repo-policy.test.ts
+++ b/test/gbrain-repo-policy.test.ts
@@ -564,6 +564,19 @@ describe('memorable recall opt-in (#3359)', () => {
     } finally {
       fs.chmodSync(legacy, 0o700);
     }
+    const nested = path.join(tmpHome, 'ancestor', 'nested');
+    fs.mkdirSync(nested, { recursive: true });
+    fs.writeFileSync(path.join(nested, 'gbrain-repo-policy.json'), JSON.stringify({ 'github.com/acme/client-a': 'deny' }));
+    fs.chmodSync(path.join(tmpHome, 'ancestor'), 0o600);
+    try {
+      const r = memo(A, { GSTACK_TEST_LEGACY_ROOT: nested });
+      expect([r.status, r.stdout]).toEqual([2, '']);
+      expect(r.stderr).toContain('not a searchable directory');
+    } finally {
+      fs.chmodSync(path.join(tmpHome, 'ancestor'), 0o700);
+    }
+    const absentUnder = path.join(tmpHome, 'no', 'such', 'root');
+    expect(memo(A, { GSTACK_TEST_LEGACY_ROOT: absentUnder }).stdout).toBe('unset\toff');
   });
 
   test('review hardening: the lookup never writes: a schema-less store is not migrated or rewritten', () => {
diff --git a/test/memorable-user-prompt-hook.test.ts b/test/memorable-user-prompt-hook.test.ts
index 59d928d7e..c9789a496 100644
--- a/test/memorable-user-prompt-hook.test.ts
+++ b/test/memorable-user-prompt-hook.test.ts
@@ -314,6 +314,27 @@ describe('gate on: the mediated hand-off', () => {
     }
   });
 
+  test('#3359 review: a second configured origin URL cannot dodge the first URL\'s deny, in either order', () => {
+    gateOn();
+    const denied = 'https://fixture.invalid/org/denied.git';
+    const other = 'https://fixture.invalid/org/unlisted.git';
+    expect(spawnSync('bash', [POLICY, 'set', denied, 'deny'], { env, encoding: 'utf8', timeout: 20_000 }).status).toBe(0);
+    for (const order of [[denied, other], [other, denied]]) {
+      const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-multi-'));
+      try {
+        spawnSync('git', ['init', '-q'], { cwd: repo, timeout: 10_000 });
+        spawnSync('git', ['remote', 'add', 'origin', order[0]], { cwd: repo, timeout: 10_000 });
+        spawnSync('git', ['config', '--local', '--add', 'remote.origin.url', order[1]], { cwd: repo, timeout: 10_000 });
+        for (const u of order) spawnSync('git', ['config', '--local', `url.${u}.insteadOf`, u], { cwd: repo, timeout: 10_000 });
+        fs.rmSync(path.join(home, 'calls.log'), { force: true });
+        expect(runHook(JSON.stringify({ prompt: 'hello', cwd: repo }), {}, repo).stdout).toBe('');
+        expect(calls()).toBe('');
+      } finally {
+        fs.rmSync(repo, { 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.