From 613ad5bfef697281ff5b4b1a6d30539552ec8cf4 Mon Sep 17 00:00:00 2001
From: masashiono0611 <masashi.ono.0611@gmail.com>
Date: Sun, 19 Jul 2026 02:08:38 +0900
Subject: [PATCH] fix(sync): restrict git auto-init self-heal to the
 anchor-resolved path only (#2964)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Second Codex review round (a10eeab) found the ownership check still too
loose:

- P1 (security): !opts.sourceId alone isn't proof gbrain owns repoPath.
  jobs.ts's `sync` job handler leaves sourceId undefined whenever
  job.data.repoPath doesn't match a registered source's local_path, so
  an admin-scope submit_job({name:'sync', data:{repoPath}}) MCP call
  could point the self-heal at an arbitrary directory and have it
  silently git-init + commit + ingest it. Gated both self-heal sites on
  !opts.repoPath too — only the path resolved from gbrain's own
  sync.repo_path anchor (never a caller-supplied one) is eligible.

- P2: the unborn-HEAD recovery site calls discoverGitRoot, which walks
  UP from repoPath and can resolve to an ANCESTOR repo for a
  --src-subpath/subdir-as-repoPath sync with an unborn HEAD. Committing
  there would `git add -A` sibling files well outside the sync scope.
  Added a check that gitContextRoot === realpathSync(repoPath) before
  self-healing; refuses (falls through to the original error) otherwise.

Tests rewritten to exercise the true self-heal-eligible path (anchor
config via engine.setConfig('sync.repo_path', dir), no repoPath/sourceId
passed) instead of an explicit repoPath, plus a new test asserting a
caller-supplied repoPath with no sourceId still throws.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS
---
 src/commands/sync.ts           | 45 +++++++++++++++-------
 test/sync-git-autoinit.test.ts | 69 ++++++++++++++++++----------------
 2 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/src/commands/sync.ts b/src/commands/sync.ts
index d73319a81..60a3a2dc6 100644
--- a/src/commands/sync.ts
+++ b/src/commands/sync.ts
@@ -1655,22 +1655,29 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
   // In the common case (repoPath == git root, no subpath) they are identical.
   serr(`[gbrain phase] sync.discover_git_root`);
   // #2964: a legacy `sync.repo_path`-anchored default brain (no `sources`
-  // row, no `opts.sourceId`) can reach here having never been `git init`-ed
-  // — e.g. a brain-pages dir that predates git-backed sync, or one rsync'd
-  // from another machine without its `.git`. gbrain owns this directory
-  // outright (it's not a user-external path the way an `opts.sourceId` +
-  // local `sources add --path` entry is), so self-heal by initializing it
-  // in place instead of failing the sync phase every single run. Mirrors
-  // the recloneIfMissing self-recovery above for owned remote clones.
-  // Scoped to !opts.sourceId and !opts.dryRun only — a registered local
-  // source without a remote_url stays a loud, actionable error (it's the
-  // user's own directory; silently git-initializing it without consent is
-  // an overreach), and a dry-run preview must never write to disk.
+  // row, no `opts.sourceId`, no caller-supplied `opts.repoPath`) can reach
+  // here having never been `git init`-ed — e.g. a brain-pages dir that
+  // predates git-backed sync, or one rsync'd from another machine without
+  // its `.git`. gbrain owns THAT directory outright (it's the one path
+  // resolved from gbrain's own persisted anchor, not a caller-named one),
+  // so self-heal by initializing it in place instead of failing the sync
+  // phase every single run. Mirrors the recloneIfMissing self-recovery
+  // above for owned remote clones.
+  //
+  // Ownership gate — `!opts.repoPath` is the load-bearing check, not just
+  // `!opts.sourceId`: `submit_job({name:'sync', data:{repoPath}})` reaches
+  // performSyncInner with sourceId undefined whenever the given path
+  // doesn't match a registered source's local_path (jobs.ts), so an
+  // admin-scope MCP caller could otherwise point this at an arbitrary
+  // directory and have it silently git-init + commit + ingest it. Only the
+  // anchor-resolved default path (`repoPath = opts.repoPath || anchor`,
+  // taking the anchor branch) is something gbrain chose for itself.
+  // `!opts.dryRun`: a dry-run preview must never write to disk.
   let gitContextRoot: string;
   try {
     gitContextRoot = realpathSync(discoverGitRoot(repoPath));
   } catch (err) {
-    if (opts.sourceId || opts.dryRun || !existsSync(repoPath)) throw err;
+    if (opts.sourceId || opts.repoPath || opts.dryRun || !existsSync(repoPath)) throw err;
     serr(`[gbrain] auto-recovery: git-initializing brain dir ${repoPath} (no git repo found).`);
     git(repoPath, ['init', '--quiet']);
     createSyncBaselineCommit(repoPath, engine.kind);
@@ -1806,8 +1813,18 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
     // this brain permanently wedged on "No commits in repo" every night
     // thereafter. Finish the same baseline-commit self-heal the
     // discoverGitRoot catch above would have done, gated the same way
-    // (gbrain-owned default brain only, never on a dry-run preview).
-    if (opts.sourceId || opts.dryRun) {
+    // (gbrain-owned default brain only, never on a dry-run preview) PLUS a
+    // scope check: `discoverGitRoot` walks UP from `repoPath`, so for a
+    // `--src-subpath`/subdir-as-repoPath sync it can resolve to an ANCESTOR
+    // repo, not `repoPath` itself. Committing there (`git add -A` at
+    // gitContextRoot) would capture sibling files well outside the sync
+    // scope — refuse instead of guessing.
+    if (
+      opts.sourceId ||
+      opts.repoPath ||
+      opts.dryRun ||
+      gitContextRoot !== realpathSync(repoPath)
+    ) {
       throw new Error(`No commits in repo ${repoPath}. Make at least one commit before syncing.`);
     }
     serr(`[gbrain] auto-recovery: repo has no commits yet, creating baseline commit ${gitContextRoot}.`);
diff --git a/test/sync-git-autoinit.test.ts b/test/sync-git-autoinit.test.ts
index 6c3635444..5c9550868 100644
--- a/test/sync-git-autoinit.test.ts
+++ b/test/sync-git-autoinit.test.ts
@@ -2,19 +2,22 @@
  * #2964 — sync phase self-heals a never-git-initialized default brain dir.
  *
  * A legacy `sync.repo_path`-anchored default brain (no `sources` row, no
- * `--source`) can reach `performSync` pointed at a directory that was never
- * `git init`-ed (predates git-backed sync, or was rsync'd from another
- * machine without its `.git`). Before this fix, `discoverGitRoot` threw
- * unconditionally and the dream cycle's sync phase failed every night with
- * no self-recovery, even though `doctor`'s sync checks reported "ok" (for
- * an unrelated reason — they only look at the `sources` table, which has
- * no rows for this kind of brain).
+ * `--source`, no caller-supplied `--repo`) can reach `performSync` pointed
+ * at a directory that was never `git init`-ed (predates git-backed sync, or
+ * was rsync'd from another machine without its `.git`). Before this fix,
+ * `discoverGitRoot` threw unconditionally and the dream cycle's sync phase
+ * failed every night with no self-recovery, even though `doctor`'s sync
+ * checks reported "ok" (for an unrelated reason — they only look at the
+ * `sources` table, which has no rows for this kind of brain).
  *
- * gbrain owns this directory outright, so the fix self-heals by
- * `git init`-ing it and capturing the current on-disk state as the sync
- * baseline — but ONLY for the default/no-`sourceId` path. A registered
- * local source (`sources add --path`, no `--url`) is the user's own
- * external directory and must keep failing loudly rather than being
+ * gbrain owns THAT directory outright, so the fix self-heals by `git
+ * init`-ing it and capturing the current on-disk state as the sync
+ * baseline — but ONLY when the path was resolved from gbrain's own
+ * persisted `sync.repo_path` anchor, never `sourceId` and never a
+ * caller-supplied `repoPath`. Either of those means someone else named the
+ * directory (a registered local source, or — per Codex review — an
+ * admin-scope `submit_job({name:'sync', data:{repoPath}})` caller with no
+ * matching source row), which must keep failing loudly rather than being
  * silently git-initialized without consent.
  */
 
@@ -48,18 +51,20 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     dir = mkdtempSync(join(tmpdir(), 'gbrain-2964-'));
     writeFileSync(join(dir, 'page1.md'), mdPage('Page 1'));
     writeFileSync(join(dir, 'page2.md'), mdPage('Page 2'));
+    // The self-heal-eligible path: gbrain's own persisted anchor, not a
+    // caller-supplied --repo / job.data.repoPath.
+    await engine.setConfig('sync.repo_path', dir);
   });
 
   afterEach(() => {
     if (dir) rmSync(dir, { recursive: true, force: true });
   });
 
-  test('default (no sourceId) sync on a non-git dir auto-inits git and imports files', async () => {
+  test('anchor-resolved sync (no repoPath, no sourceId) on a non-git dir auto-inits git and imports files', async () => {
     const { performSync } = await import('../src/commands/sync.ts');
     expect(existsSync(join(dir, '.git'))).toBe(false);
 
     const result = await performSync(engine, {
-      repoPath: dir,
       noPull: true,
       noEmbed: true,
       full: true,
@@ -74,23 +79,14 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
 
   test('a second sync after auto-init sees no changes (baseline commit captured current on-disk state)', async () => {
     const { performSync } = await import('../src/commands/sync.ts');
-    const first = await performSync(engine, {
-      repoPath: dir,
-      noPull: true,
-      noEmbed: true,
-      full: true,
-    });
+    const first = await performSync(engine, { noPull: true, noEmbed: true, full: true });
     expect(first.added).toBe(2);
 
     // No new files, no explicit `full` — a real incremental sync against the
     // auto-init baseline. Before this fix there was no baseline to diff
     // against (sync errored outright); a naive fix that skipped the initial
     // commit would make this call re-report both files as "added" again.
-    const second = await performSync(engine, {
-      repoPath: dir,
-      noPull: true,
-      noEmbed: true,
-    });
+    const second = await performSync(engine, { noPull: true, noEmbed: true });
     expect(second.status).not.toBe('first_sync');
     expect(second.added).toBe(0);
     expect(second.modified).toBe(0);
@@ -110,17 +106,29 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     expect(existsSync(join(dir, '.git'))).toBe(false);
   });
 
-  test('--dry-run on a non-git dir throws without writing anything to disk', async () => {
+  test('a caller-supplied repoPath with no sourceId still throws — not auto-inited (P1: MCP submit_job arbitrary-path guard)', async () => {
+    // Mirrors jobs.ts: submit_job({name:'sync', data:{repoPath}}) reaches
+    // performSyncInner with sourceId left undefined whenever repoPath
+    // doesn't match a registered source's local_path. Auto-init must not
+    // fire here even though sourceId is unset — only the anchor-resolved
+    // default path is gbrain's own.
     const { performSync } = await import('../src/commands/sync.ts');
     await expect(
       performSync(engine, {
         repoPath: dir,
-        dryRun: true,
         noPull: true,
         noEmbed: true,
         full: true,
       }),
     ).rejects.toThrow(/git repository/i);
+    expect(existsSync(join(dir, '.git'))).toBe(false);
+  });
+
+  test('--dry-run on the anchor-resolved path throws without writing anything to disk', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    await expect(
+      performSync(engine, { dryRun: true, noPull: true, noEmbed: true, full: true }),
+    ).rejects.toThrow(/git repository/i);
     // The whole point of --dry-run is "preview only" — it must never git-init
     // or commit on our behalf, even though we could self-heal.
     expect(existsSync(join(dir, '.git'))).toBe(false);
@@ -134,12 +142,7 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     // discoverGitRoot succeeds, but `git rev-parse HEAD` still fails.
     execSync('git init -q', { cwd: dir });
 
-    const result = await performSync(engine, {
-      repoPath: dir,
-      noPull: true,
-      noEmbed: true,
-      full: true,
-    });
+    const result = await performSync(engine, { noPull: true, noEmbed: true, full: true });
 
     expect(result.status).toBe('first_sync');
     expect(result.added).toBe(2);