From 23b55ab966448d96de130bdfa8ed74ad6d671dd7 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 24 Oct 2026 21:17:49 +0000 Subject: [PATCH] fix(memorable): use each NUL-delimited origin URL verbatim (security review, fourth pass) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 6 ++-- .../hooks/memorable-user-prompt-hook.ts | 2 +- test/memorable-user-prompt-hook.test.ts | 35 ++++++++++--------- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87d6593bd..b1413b2e7 100644 --- a/CHANGELOG.md +++ b/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 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. +- 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 and used verbatim, so a valid local path containing a newline or edge whitespace 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, 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. +- 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 or edge whitespace; 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. 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. +- 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. Seventeen mutations of the new gate were run against the focused tests. Sixteen 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 or trimmed, and the client fast path ignoring a dangling ancestor symlink. The seventeenth, 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/hosts/claude/hooks/memorable-user-prompt-hook.ts b/hosts/claude/hooks/memorable-user-prompt-hook.ts index 6522cf6aa..3f01d7d01 100644 --- a/hosts/claude/hooks/memorable-user-prompt-hook.ts +++ b/hosts/claude/hooks/memorable-user-prompt-hook.ts @@ -405,7 +405,7 @@ async function policyVeto(cwd: string, timeoutMs: number, remaining: () => numbe }); killInflight = null; if (raw.timedOut || raw.error || raw.status !== 0) return 'error'; - const urls = raw.stdout.toString('utf8').split('\0').map((u) => u.trim()).filter(Boolean); + const urls = raw.stdout.toString('utf8').split('\0').filter((u) => u !== ''); 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()))); diff --git a/test/memorable-user-prompt-hook.test.ts b/test/memorable-user-prompt-hook.test.ts index 27d1438d1..ef2506b99 100644 --- a/test/memorable-user-prompt-hook.test.ts +++ b/test/memorable-user-prompt-hook.test.ts @@ -335,23 +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', () => { + test('#3359 review: an origin URL with a newline or edge whitespace (valid local paths) is looked up verbatim, 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 }); + for (const name of ['remote\nname.git', 'remote.git ', ' remote.git']) { + const parent = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-nl-')); + try { + const remote = path.join(parent, name); + 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'); + fs.rmSync(path.join(home, 'calls.log'), { force: true }); + expect([name, runHook(JSON.stringify({ prompt: 'hello', cwd: repo }), {}, repo).stdout]).toEqual([name, '']); + expect(calls()).toBe(''); + } finally { + fs.rmSync(parent, { recursive: true, force: true }); + } } });