diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 60a3a2dc6..5bd1a05ad 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -958,10 +958,41 @@ export function discoverGitRoot(inputPath: string): string { * headless nightly cron/launchd invocation, which has no reason to have * git signing/identity configured, and must not block on an unavailable * signing agent or pinentry prompt. + * + * db_only exclusion is recomputed directly and passed to `git add` as + * negative pathspecs, rather than relying solely on `manageGitignore` + * having written `.gitignore` successfully: that helper is deliberately + * best-effort (a broken gbrain.yml parse, or an unwritable .gitignore, + * only warns and returns — the right default for its OTHER callers, where + * .gitignore management is a side effect that must never kill the sync + * job). For a commit we are about to create ourselves, "fail open" there + * would mean silently committing db_only content into git history. Fail + * closed instead: db_only exclusion doesn't depend on the .gitignore + * write having succeeded. `loadStorageConfig` throwing (unreadable + * gbrain.yml, or a semantic overlap) propagates — better to leave this + * self-heal wedged with a clear error than commit unknown content. */ function createSyncBaselineCommit(repoPath: string, engineKind?: 'pglite' | 'postgres'): void { manageGitignore(repoPath, engineKind); - git(repoPath, ['add', '-A']); + const storageConfig = loadStorageConfig(repoPath); + // Only pathspec-exclude db_only dirs `.gitignore` did NOT already cover. + // A redundant negation pathspec for an already-ignored path makes git's + // `-A` bail with "paths ignored by .gitignore, use -f" + // (advice.addIgnoredFile) even though the negation is semantically + // correct and git would otherwise skip it silently. `check-ignore` + // throwing (not ignored, or the check itself failed) defaults to + // excluding explicitly — the fail-closed direction. + const excludePathspecs = (storageConfig?.db_only ?? []) + .filter((dir) => { + try { + git(repoPath, ['check-ignore', '-q', dir]); + return false; + } catch { + return true; + } + }) + .map((dir) => `:!${dir}`); + git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs]); git( repoPath, ['commit', '--quiet', '--allow-empty', '--no-gpg-sign', '-m', 'gbrain: initial commit (auto-init by sync)'], @@ -1035,6 +1066,38 @@ async function readSyncAnchor( return await engine.getConfig(`sync.${which}`); } +/** + * #2964: is `repoPath` gbrain's own legacy default anchor, as opposed to a + * path some caller merely happened to pass through unchanged? + * + * `!opts.sourceId` alone is NOT sufficient: `runPhaseSync` (dream cycle) + * and `submit_job({name:'sync', data:{repoPath}})` (MCP, jobs.ts) both + * reach `performSyncInner` with `sourceId` undefined AND `opts.repoPath` + * set explicitly — the former legitimately (it already resolved the + * anchor itself and is passing it through), the latter potentially with + * an attacker-controlled path. The two are indistinguishable from + * `opts.repoPath`'s mere presence/absence, so ownership has to be proven + * by VALUE: read the anchor fresh from config and require the resolved + * `repoPath` to equal it exactly. An arbitrary caller-supplied path only + * passes this check if it happens to already equal gbrain's own anchor — + * at which point self-healing it is exactly the legitimate case. + * + * `opts.srcSubpath` disqualifies unconditionally: a subpath-scoped sync + * only wants THAT subdirectory captured, but the self-heal baseline + * commit runs `git add -A` at the git root (there's no file list yet to + * scope it to — collection happens after this point) — see the P2 review + * finding on `createSyncBaselineCommit`'s callers. + */ +async function isAnchorOwnedSyncPath( + engine: BrainEngine, + opts: SyncOpts, + repoPath: string, +): Promise { + if (opts.sourceId || opts.srcSubpath) return false; + const anchor = await readSyncAnchor(engine, undefined, 'repo_path'); + return anchor !== null && anchor === repoPath; +} + async function writeSyncAnchor( engine: BrainEngine, sourceId: string | undefined, @@ -1654,30 +1717,23 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise { + // cycle.ts's runPhaseSync (the actual dream-cycle call site this bug + // was filed against) always passes `repoPath: brainDir` explicitly — + // it already resolved the anchor itself upstream and threads it + // through. Gating self-heal on `!opts.repoPath` would silently never + // fire here; ownership must be proven by matching the anchor's VALUE. + 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 }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + expect(existsSync(join(dir, '.git'))).toBe(true); + }); + 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, { noPull: true, noEmbed: true, full: true }); @@ -106,16 +125,34 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', () expect(existsSync(join(dir, '.git'))).toBe(false); }); - test('a caller-supplied repoPath with no sourceId still throws — not auto-inited (P1: MCP submit_job arbitrary-path guard)', async () => { + test('a caller-supplied repoPath that does NOT match the anchor still throws (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. + // doesn't match a registered source's local_path. Self-heal must not + // fire for a path that isn't gbrain's own anchor, even with no + // sourceId set — only exact anchor-value equality (the previous test) + // is eligible. + const other = mkdtempSync(join(tmpdir(), 'gbrain-2964-other-')); + writeFileSync(join(other, 'unrelated.md'), mdPage('Unrelated')); + try { + const { performSync } = await import('../src/commands/sync.ts'); + await expect( + performSync(engine, { repoPath: other, noPull: true, noEmbed: true, full: true }), + ).rejects.toThrow(/git repository/i); + expect(existsSync(join(other, '.git'))).toBe(false); + } finally { + rmSync(other, { recursive: true, force: true }); + } + }); + + test('--src-subpath on the anchor-resolved path still throws — not auto-inited (P2: subpath scope guard)', async () => { + // A self-heal baseline commit runs `git add -A` at the git root before + // any subpath-scoped file collection happens, so it would capture + // sibling directories a --src-subpath sync never intended to touch. const { performSync } = await import('../src/commands/sync.ts'); await expect( performSync(engine, { - repoPath: dir, + srcSubpath: 'wiki', noPull: true, noEmbed: true, full: true, @@ -127,10 +164,10 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', () 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 }), + performSync(engine, { repoPath: dir, 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. + // or commit on our behalf, even though this is otherwise self-heal-eligible. expect(existsSync(join(dir, '.git'))).toBe(false); }); @@ -142,10 +179,28 @@ 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, { noPull: true, noEmbed: true, full: true }); + const result = await performSync(engine, { repoPath: dir, noPull: true, noEmbed: true, full: true }); expect(result.status).toBe('first_sync'); expect(result.added).toBe(2); expect(execSync('git rev-parse HEAD', { cwd: dir }).toString().trim()).not.toBe(''); }); + + test('db_only paths are excluded from the baseline commit even without gbrain.yml write support (P2: fail-closed exclusion)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const { mkdirSync } = await import('fs'); + const { execSync } = await import('child_process'); + mkdirSync(join(dir, 'private-cache')); + writeFileSync(join(dir, 'private-cache', 'secret.bin'), 'binary-ish content'); + writeFileSync( + join(dir, 'gbrain.yml'), + 'storage:\n db_only:\n - private-cache\n', + ); + + await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + expect(existsSync(join(dir, '.git'))).toBe(true); + const tracked = execSync('git ls-files', { cwd: dir }).toString(); + expect(tracked).not.toContain('private-cache'); + }); });