diff --git a/CHANGELOG.md b/CHANGELOG.md index 60bdb5d0f..ecfe09b0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,19 @@ All notable changes to GBrain will be documented in this file. +## [0.42.33.0] - 2026-06-07 + +**`gbrain sync` will never delete a repo it didn't create.** If a source was registered with a `remote_url` but its `local_path` pointed at a working tree you manage yourself (not a gbrain-managed clone), a failed or degraded code sync could remove that directory and re-clone over it. Sync now re-clones **only** clones gbrain actually created — identified by an ownership marker, or by gbrain's own clone location for clones made before this release. Anything else, including your live working tree, is treated as read-only: indexed, never deleted. On an unowned path, sync aborts loudly **before touching the filesystem** and tells you how to fix the source registration. Thanks to @zaqwery for the report. + +The re-clone path is also crash-safer now: it clones into a sibling temp on the same filesystem and swaps atomically (old aside → new in → drop old), so a cross-device rename can't leave a source deleted-but-not-restored. If the swap ever fails, the error names exactly where your original clone is preserved. + +### Fixed +- **`gbrain sync` never deletes an unowned working tree (gbrain#1881).** Re-clone is confined to clones gbrain created (`config.managed_clone` marker, or the default clone location for pre-marker clones). A `remote_url` source whose `local_path` is your own working tree is synced read-only and refused — loudly, before any filesystem op — never removed. `gbrain sources restore` on such a source warns and keeps the tree instead of deleting it. Reported by @zaqwery. +- **Safer re-clone swap.** Re-clone uses a same-filesystem sibling temp plus an atomic swap (no cross-device "deleted but not re-cloned" window); a symlinked clone path is refused; a failed swap reports where the original is preserved. + +### To take advantage of v0.42.33.0 + +Upgrade. Nothing to configure. New `--url` sources are marked gbrain-owned automatically, and existing managed clones at the default location keep auto-recovering. If you registered a source whose `local_path` is a working tree you maintain yourself, `gbrain sync` now syncs it read-only and prints how to re-register it if you want gbrain to manage the clone. ## [0.42.32.0] - 2026-06-07 **A single un-parseable note can no longer silently stop your brain from indexing anything new.** A page whose YAML frontmatter `title:` was a bare date (`title: 2024-06-01`) or number (`title: 1458`) parsed as a Date/number, not text — and the importer threw when it tried to lowercase it. That throw blocked the sync bookmark from advancing, so every later `gbrain sync` re-walked the whole repo, never reached HEAD, and quietly stopped indexing new commits. The page was committed and on GitHub, but `gbrain get` returned `page_not_found` with no surfaced error. diff --git a/TODOS.md b/TODOS.md index 4a280ffda..69ebba340 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,61 @@ # TODOS +## gbrain#1881 sync reclone ownership follow-ups (v0.43+) + +Filed from the #1881 fix (`gbrain sync --strategy code` deleted a user's working +tree; `recloneIfMissing` now only re-clones a clone gbrain OWNS — `config.managed_clone` +marker or exact default-location equality — via `isOwnedClone`). Deliberately scoped +OUT of that PR. Codex outside-voice findings #5/#6. See plan + GSTACK REVIEW REPORT at +`~/.claude/plans/system-instruction-you-are-working-golden-valiant.md`. + +- [ ] **P2 — `gbrain doctor` misconfigured-source check.** Flag every source row + where `config.remote_url` is set but `isOwnedClone(row)` is false (the shape that + caused #1881: a federated row whose `local_path` is a user working tree). Print a + one-time, actionable hint per row: drop `config.remote_url` to sync it read-only, + or remove + re-add with `--url` so gbrain owns the clone. **Why:** the core guard + now refuses to delete such rows, but they still exist in users' brains (created by + the gstack orchestrator). This is the single surfacing point — it replaces the + per-sync stderr warning that was rejected during eng-review (Codex: it would spam + every healthy sync). **Where:** extend the doctor checks in `src/commands/doctor.ts`; + reuse `isOwnedClone` from `src/core/sources-ops.ts`. No migration. + +- [ ] **P3 — Decide the `--clone-dir`-outside-root policy.** `gbrain sources add --url + --clone-dir ` lets local callers place a gbrain-owned clone anywhere. The + ownership marker (this PR) makes those safe to reclone, but the dormant + `clone_dir_outside_gbrain` code in `SourceOpErrorCode` (`sources-ops.ts`) is unused — + it hints at a previously-intended confinement rule. Decide: either wire it up (forbid + `--clone-dir` outside `$GBRAIN_HOME/clones/`) or delete the dead code. Don't leave it + half-implemented. Codex finding #5. + +- [ ] **P2 — Harden the `managed_clone` ownership marker against forgery.** Ownership + (`isOwnedClone`) authorizes the destructive reclone swap on the strength of a DB JSON + boolean (`config.managed_clone`). Today only `addSource --url` writes it, but it's a + mutable field any future `set-config` / external INSERT / restored dump could set on a + user-tree path. A forged marker on a real (non-symlink) user path would authorize + deletion. (A realpath path-check does NOT close this — it false-positives on ubiquitous + system symlinks like macOS /var, and an owned clone gbrain created is legitimately + deleted through any operator symlink anyway. Path can't prove ownership.) Two follow-ups: + (a) a CI guard asserting NO code path other than `addSource` ever writes the + `managed_clone` key; (b) bind ownership to an unforgeable on-disk stamp (a `.gbrain-clone` + sentinel written into the clone at creation, verified before any destructive op) instead + of / in addition to the DB field — with an equality-fallback for pre-stamp clones. Codex + adversarial (High) + Claude adversarial (Finding 2) from the #1881 ship review. + +- [ ] **P3 — Sweep orphaned `.gbrain-reclone-*` temp dirs.** The EXDEV-safe reclone clones + into a sibling temp of `local_path` (`.gbrain-reclone--`). Every error path + `rmSync`s it, but a hard crash (SIGKILL/power loss) between clone and swap leaves a full + clone orphaned next to the user's `--clone-dir` parent — outside gbrain's swept + `clones/.tmp`. Add a startup/doctor sweep for `.gbrain-reclone-*` / `*.old-*` older than N + minutes. Codex Medium / Claude Finding 4 from the #1881 ship review. + +- [ ] **P3 — CLI `gbrain sources remove` leaks the managed clone dir.** `runRemove` + (`src/commands/sources.ts:269`) runs `DELETE FROM sources` directly, bypassing + `removeSource()` and its symlink-safe clone-cleanup guard — so removing a `--url` + source never deletes its on-disk clone (storage leak). Route CLI remove through + `removeSource()` (or replicate its guard) so the clone dir is cleaned with the same + ownership/symlink protections. Orthogonal to the deletion bug; surfaced by Codex + finding #6 during the #1881 review. + ## #1737 minion fair-scheduling follow-up (v0.43+) Filed during the #1737 wave (`/plan-eng-review` decision F7, codex outside-voice diff --git a/VERSION b/VERSION index e8fb9eb62..88025242a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.32.0 +0.42.33.0 \ No newline at end of file diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index ef8f05295..e5186de7a 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -30,6 +30,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/reindex.ts` — `gbrain reindex --markdown [--limit N] [--dry-run] [--json] [--no-embed] [--repo PATH]`. Walks `pages WHERE page_kind = 'markdown' AND chunker_version < MARKDOWN_CHUNKER_VERSION` in 100-row batches ordered by id. Rows with non-null `source_path` re-import via `importFromFile`; rows without fall back to `importFromContent`. Both paths pass `forceRechunk: true` to bypass `importFromContent`'s `content_hash` short-circuit — without it the chunker version bump never reaches pages whose source content hasn't changed, AND the stripFactsFence privacy strip never applies to pre-strip chunks. Idempotent — partial-completion re-runs pick up via id-ordered batches. Wired into `src/commands/upgrade.ts:runPostUpgrade` after `apply-migrations`. The DB-only fallback (no source file on disk) does NOT pass body-only `compiled_truth` to `importFromContent` (that path re-parses with EMPTY frontmatter and OVERWRITES the page's real frontmatter/title/timeline); it `getPage`+`getTags`, reconstructs FULL markdown via `serializeMarkdown(frontmatter, compiled_truth, timeline, {type, title, tags})`, and re-imports THAT so re-chunking a DB-only page preserves everything while bumping `chunker_version`. Pinned by `test/reindex-preserve-tags.test.ts`. - `src/commands/reindex-code.ts` — `gbrain reindex --code [--source ID] [--dry-run] [--yes] [--json] [--force] [--no-embed]`. Walks `pages WHERE type = 'code'` in 100-row batches, replays through `importCodeFile` for chunk + embed + content_hash folding. Idempotent unless `--force` bypasses the content_hash early-return. Cost-preview model field reads `getEmbeddingModelName()` from the gateway so preview reflects what the gateway will actually embed with. An informational stderr nudge inside `runReindexCode` (so dry-run + execute both surface it): when the configured embedding model isn't code-tuned (allowlist `{'voyage-code-3'}`, case-insensitive bare match), prints a recommendation to switch to `voyage:voyage-code-3`; suppress with `GBRAIN_NO_CODE_MODEL_NUDGE=1`, `--no-embed`, or `--json`. Pure `shouldNudgeCodeModel(bareName)` returns a tagged `NudgeDecision` union (takes the bare model name, emits qualified `voyage:voyage-code-3` for the paste-ready `gbrain config set` line). When `--yes` is absent and the caller is non-TTY or passed `--json`, the cost gate refuses (exit 2, no spend) via the pure exported `buildCostRefusal({json, previewMsg, preview, costUsd, model}): {stdout?, stderr?}` — JSON envelope only when `--json` is explicit, otherwise a human refusal on stderr (the spend guardrail is independent of the output format). Pinned by `test/ai/voyage-code-3-recipe.test.ts`, `test/reindex-code-nudge.serial.test.ts`, `test/reindex-code-model-source.serial.test.ts` (IRON-RULE regression for the cost-preview fix), `test/reindex-cost-refusal.test.ts`. - `src/commands/sync.ts:resolveSlugByPathOrSourcePath` — Resolves a slug by `pages.source_path` first (returns the stored slug for frontmatter-fallback pages whose path doesn't derive a slug), then falls back to `resolveSlugForPath(path)`. Threaded into all 4 delete/rename call sites (`performSync`'s un-syncable cleanup at ~:531, deletes at ~:603, rename oldSlug at ~:622). Without this, emoji-only / Thai / Arabic filenames whose slug came from frontmatter would orphan on delete/rename (the delete path would compute the wrong path-derived slug). Best-effort query — pre-migration brains fall through to the legacy path. +- `src/core/sources-ops.ts` — Multi-source registration + clone-lifecycle ops (`addSource`, `recloneIfMissing`, `defaultCloneDir`, `isOwnedClone`, `unownedHint`). **Reclone-ownership invariant (must-never-violate): gbrain may only delete/re-clone a clone it created, NEVER a user working tree.** `recloneIfMissing` deletes `local_path`, so it gates on `isOwnedClone(src)` and throws a `SourceOpError('unmanaged_path', ...)` BEFORE any filesystem op when ownership is unprovable — fail-closed. Ownership is proven by `config.managed_clone === true` (written by `addSource`'s `--url` path, covering default-location and `--clone-dir` clones) OR `local_path === defaultCloneDir(id)` (back-compat for pre-marker clones, via exact normalized-path equality, symlink-free). A row with `remote_url` + an unowned `local_path` (a user-registered working tree, e.g. `sources add --path`) is refused untouched; re-add with `--url` to regain auto-reclone. The reclone is EXDEV-safe: clone into a SIBLING temp of `local_path` (not the shared `clones/.tmp`, which may sit on a different mount than a `--clone-dir` target), then swap (move old aside → move new in → drop old) so `local_path` is never left missing-and-unrecoverable; on swap failure the original is restored, and if restore fails the error names the `aside` path so it's never reflexively deleted. A TOCTOU re-check re-confirms ownership immediately before the destructive move and rejects a symlink leaf swapped in after the entry check (`symlink_escape`). `unownedHint(src, state)` is the shared recovery message used by both the core error and the `gbrain sync --source` CLI error; `gbrain sources restore` special-cases `unmanaged_path` to print "DB row restored; gbrain syncs this path read-only" instead of the misleading "try sync to recover" guidance. `SourceOpErrorCode` includes `unmanaged_path`. Pinned by `test/sources-ops.test.ts`, `test/sources-resync-recovery.test.ts`. - `src/core/utils.ts` — Shared SQL utilities extracted from postgres-engine.ts. Exports `parseEmbedding(value)` (throws on unknown input, used by migration + ingest paths where data integrity matters) and `tryParseEmbedding(value)` (returns `null` + warns once per process, used by search/rescore paths where availability matters more than strictness). `isUndefinedColumnError(err)` predicate — pattern-matches Postgres SQLSTATE 42703 / "column ... does not exist" with engine-driver shape variation tolerated; replaces bare `catch {}` blocks in `oauth-provider.ts` so genuine errors (lock timeout, network blip, permission denied) propagate while column-missing falls through to the legacy fallback. `validateSourceId(id)` throws on anything outside `^[a-z0-9_-]+$`, used by the per-source disk-layout fix in patterns.ts/synthesize.ts before any `join(brainDir, '.sources', source_id, slug+'.md')` so source_id can't traverse out of brainDir. `rowToPage` populates the required `Page.source_id` from the SELECT projection (`scripts/check-source-id-projection.sh` enforces every projection feeding `rowToPage` includes the column). - `src/core/db.ts` — Connection management, schema initialization. `resolveSessionTimeouts()` returns `statement_timeout` + `idle_in_transaction_session_timeout` (defaults 5min each, env-overridable via `GBRAIN_STATEMENT_TIMEOUT`/`GBRAIN_IDLE_TX_TIMEOUT`/`GBRAIN_CLIENT_CHECK_INTERVAL`). Both `connect()` (module singleton) and `PostgresEngine.connect()` (worker pool) consume the result via postgres.js's `connection` option, sending GUCs as startup parameters that survive PgBouncer transaction mode (`setSessionDefaults` kept as a back-compat no-op shim). `connect()` returns `Promise` — `true` iff THIS call created the module singleton, `false` if it joined an existing one; the decision is atomic (no `await` between the `if (sql)` null-check and the synchronous `sql = postgres(...)` assignment), so two concurrent module connects can't both claim creation. `PostgresEngine` stores the return as its `_ownsModuleSingleton` token and only the creating engine may `db.disconnect()` the singleton — a borrower probe engine (lint/doctor config-lift) no-ops its disconnect, so its teardown can't null the connection the long-lived cycle owner is still using (the dream-cycle "connect() has not been called" failure). The module `sql` is only ever nulled by `db.disconnect()` (postgres.js auto-reconnects its own internal pool and never touches our reference). `disconnect()` snapshots + nulls `sql` before awaiting `s.end()` so a concurrent connect can't join a pool that's already closing. - `src/commands/migrate-engine.ts` — Bidirectional engine migration (`gbrain migrate --to supabase/pglite`). diff --git a/package.json b/package.json index b8b372042..b962e4d6a 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.32.0" + "version": "0.42.33.0" } diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 3c2e194bf..b071a8b55 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -394,7 +394,14 @@ async function runRestore(engine: BrainEngine, args: string[]): Promise { console.log(` re-cloned from remote_url (clone dir was missing).`); } } catch (e) { - if (e instanceof SourceOpError) { + if (e instanceof SourceOpError && e.code === 'unmanaged_path') { + // #1881: local_path is the user's own working tree, not a clone gbrain + // created. gbrain won't re-clone over it, and `gbrain sync` will refuse it + // too — so the generic "missing clone, try sync to recover" guidance below + // would be actively misleading. Surface the real situation instead. + console.error(` WARN: ${e.message}`); + console.error(` The DB row is restored; gbrain syncs this path read-only.`); + } else if (e instanceof SourceOpError) { console.error(` WARN: could not re-clone: ${e.message}`); console.error(` The DB row is restored but the on-disk clone is missing.`); console.error(` Try \`gbrain sync --source ${id}\` to recover, or remove + re-add.`); diff --git a/src/commands/sync.ts b/src/commands/sync.ts index e739e5339..164389db7 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -1084,9 +1084,11 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise( - `SELECT config FROM sources WHERE id = $1`, + const { recloneIfMissing, isOwnedClone, unownedHint } = await import( + '../core/sources-ops.ts' + ); + const cfgRows = await engine.executeRaw<{ local_path: string | null; config: unknown }>( + `SELECT local_path, config FROM sources WHERE id = $1`, [opts.sourceId], ); const cfg = @@ -1095,13 +1097,26 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise); const remoteUrl = typeof cfg.remote_url === 'string' ? cfg.remote_url : null; if (remoteUrl) { + const ownSrc = { + id: opts.sourceId, + local_path: cfgRows[0]?.local_path ?? repoPath, + config: cfg, + }; const state = validateRepoState(repoPath, remoteUrl); switch (state) { case 'healthy': + // No per-sync warning for an unowned-but-healthy source — it would + // spam every sync. The misconfig is surfaced by the doctor check + // (TODO1) instead. Healthy unowned paths sync read-only and are safe. break; case 'missing': case 'no-git': case 'not-a-dir': + // #1881: only re-clone a clone gbrain owns. An unowned local_path + // (the user's working tree) is refused loudly, never deleted. + if (!isOwnedClone(ownSrc)) { + throw new Error(unownedHint(ownSrc, state)); + } serr( `[gbrain] auto-recovery: re-cloning "${opts.sourceId}" (clone state: ${state}).`, ); diff --git a/src/core/sources-ops.ts b/src/core/sources-ops.ts index 9e912b84f..3cb1c0a9a 100644 --- a/src/core/sources-ops.ts +++ b/src/core/sources-ops.ts @@ -38,7 +38,7 @@ import { existsSync, mkdirSync, renameSync, rmSync, lstatSync } from 'fs'; import { realpathSync } from 'fs'; -import { join, dirname, resolve as resolvePath } from 'path'; +import { join, dirname, basename, resolve as resolvePath } from 'path'; import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { @@ -66,7 +66,8 @@ export type SourceOpErrorCode = | 'not_found' | 'protected_id' | 'clone_dir_outside_gbrain' - | 'symlink_escape'; + | 'symlink_escape' + | 'unmanaged_path'; export class SourceOpError extends Error { constructor( @@ -253,6 +254,70 @@ export function isPathContained(child: string, parent: string): boolean { return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); } +/** + * Did gbrain CREATE this clone (so re-clone/delete is safe)? Ownership, NOT + * path-containment — a user-supplied working tree is NEVER owned, even if it + * happens to sit under $GBRAIN_HOME. This is the #1881 guard: recloneIfMissing + * deletes local_path, so it must only ever fire on a clone gbrain owns. + * + * Ownership is proven by either: + * 1. config.managed_clone === true — written by addSource's --url path + * (covers both default-location and --clone-dir clones), OR + * 2. local_path === defaultCloneDir(id) — back-compat for clones created + * before the marker existed (gbrain's default location), via exact + * normalized-path equality (symlink-free, so none of isPathContained's + * symlinked-parent / lexical-escape edge cases apply). + * + * Everything else is fail-closed (NOT owned → refuse to touch): the bug's + * federated row (remote_url + a user tree), and pre-marker --clone-dir clones + * (rare, local-only) which are byte-for-byte indistinguishable from it. Those + * must be re-added to regain auto-reclone — the correct trade-off when ownership + * is unprovable. + */ +export function isOwnedClone(src: { + id: string; + local_path: string | null; + config: unknown; +}): boolean { + if (!src.local_path) return false; + const cfg = + typeof src.config === 'string' + ? (JSON.parse(src.config) as Record) + : ((src.config ?? {}) as Record); + if (cfg.managed_clone === true) return true; + return resolvePath(src.local_path) === resolvePath(defaultCloneDir(src.id)); +} + +/** + * Recovery hint for an unowned source with a remote_url. Splits guidance by + * on-disk state: a healthy unowned path syncs read-only (just drop remote_url), + * but a degraded one (missing/no-git/not-a-dir) cannot be recovered by dropping + * remote_url — that would only defer the failure to the "Not a git repository" + * check. Shared by the core SourceOpError and the sync.ts CLI error so they read + * identically. + */ +export function unownedHint( + src: { id: string; local_path: string | null }, + state: RepoState, +): string { + const path = src.local_path ?? '(none)'; + if (state === 'healthy') { + return ( + `Source "${src.id}" has config.remote_url set but local_path ${path} is not a ` + + `clone gbrain created. gbrain syncs it read-only and will never re-clone or delete ` + + `it. To silence this, drop config.remote_url, or re-register with --url so gbrain ` + + `owns the clone.` + ); + } + return ( + `Source "${src.id}" has config.remote_url set but local_path ${path} is not a clone ` + + `gbrain created and is not a usable git repo (state: ${state}). gbrain will NOT ` + + `re-clone over it (it is your working tree, not a gbrain-managed mirror). Restore the ` + + `directory yourself, or remove + re-add the source with --url to let gbrain manage the ` + + `clone.` + ); +} + // ── addSource ─────────────────────────────────────────────────────────────── export async function addSource( @@ -327,7 +392,14 @@ export async function addSource( throw e; } - const config: Record = { remote_url: parsedUrl.url }; + // managed_clone:true is the ownership marker (#1881). It authorizes + // recloneIfMissing to rm+replace this clone — gbrain created it, here or at + // a --clone-dir path. A user-tree row (created by an external INSERT, no + // --url) never carries this, so it can never be deleted by reclone. + const config: Record = { + remote_url: parsedUrl.url, + managed_clone: true, + }; if (opts.federated !== null && opts.federated !== undefined) { config.federated = opts.federated; } @@ -698,9 +770,22 @@ export async function recloneIfMissing( const state = validateRepoState(src.local_path, remoteUrl); if (state === 'healthy') return false; - // Re-clone via temp + rename, mirroring addSource's atomicity contract. - const tempDir = makeTempCloneDir(id); - mkdirSync(dirname(tempDir), { recursive: true }); + // #1881 ownership guard — abort BEFORE any filesystem op. recloneIfMissing + // deletes local_path; gbrain may only do that to a clone it created, never a + // user working tree. A row with remote_url + an unowned local_path (the + // gstack-orchestrator federated shape) is refused here, loudly, untouched. + if (!isOwnedClone(src)) { + throw new SourceOpError('unmanaged_path', unownedHint(src, state)); + } + + // EXDEV-safe atomic reclone. Clone into a SIBLING temp of local_path (not the + // shared clones/.tmp, which can be on a different mount than a --clone-dir + // target → EXDEV → "deleted but not recloned"). Then swap: move old aside → + // move new in → drop old, so local_path is never left missing-and-unrecoverable. + const parent = dirname(src.local_path); + mkdirSync(parent, { recursive: true }); + const rand = randomBytes(6).toString('hex'); + const tempDir = join(parent, `.gbrain-reclone-${basename(src.local_path)}-${rand}`); try { cloneRepo(remoteUrl, tempDir); } catch (e) { @@ -711,19 +796,62 @@ export async function recloneIfMissing( throw e; } - // If the local_path partially exists (e.g., empty dir, file-not-dir), nuke - // it before the rename so renameSync doesn't fail on a non-empty target. - rmSync(src.local_path, { recursive: true, force: true }); - mkdirSync(dirname(src.local_path), { recursive: true }); - try { - renameSync(tempDir, src.local_path); - } catch (e) { + // TOCTOU re-check immediately before the destructive move: re-confirm + // ownership AND reject a symlink leaf swapped in after the entry check (never + // rm-rf / rename through a symlink). + if (!isOwnedClone(src)) { rmSync(tempDir, { recursive: true, force: true }); + throw new SourceOpError('unmanaged_path', unownedHint(src, state)); + } + let aside: string | null = null; + try { + if (existsSync(src.local_path)) { + // Symlink leaf guard: never rename/rm *through* a symlinked leaf — that's + // the TOCTOU swap-in vector (an attacker plants a symlink at local_path + // between the entry ownership check and this rename). An owned clone's leaf + // is a real dir gbrain created; a symlink here means tamper, so fail closed. + // (Symlinked ANCESTORS are intentionally NOT rejected here: for an owned + // clone gbrain created the dir at this path — cloneRepo refuses a non-empty + // dest, so a pre-existing user tree can never become an owned clone — and a + // realpath-chain check false-positives on ubiquitous system symlinks like + // macOS /var -> /private/var. The residual DB-trust risk, a forged + // managed_clone marker on an arbitrary path, is tracked as a TODO and is + // not closable by a path check.) + if (lstatSync(src.local_path).isSymbolicLink()) { + rmSync(tempDir, { recursive: true, force: true }); + throw new SourceOpError( + 'symlink_escape', + `Refusing to re-clone "${id}": local_path ${src.local_path} is a symlink.`, + ); + } + aside = `${src.local_path}.old-${rand}`; + renameSync(src.local_path, aside); // same fs (sibling) — no EXDEV + } + renameSync(tempDir, src.local_path); // same fs — no EXDEV + } catch (e) { + // Best-effort restore of the original if the swap left local_path missing. + if (aside && !existsSync(src.local_path)) { + try { + renameSync(aside, src.local_path); + } catch { + /* original kept at `aside`; surfaced via the thrown error below */ + } + } + rmSync(tempDir, { recursive: true, force: true }); + if (e instanceof SourceOpError) throw e; + // If the original is still parked at `aside` (restore failed), tell the user + // exactly where it is — otherwise a "cleanup the failed reclone" reflex would + // delete their only copy. + const asideNote = + aside && existsSync(aside) + ? ` Your original clone is preserved at ${aside} — restore it manually; do not delete it.` + : ''; throw new SourceOpError( 'rename_failed', - `Could not move re-cloned repo to ${src.local_path}: ${(e as Error).message}`, + `Could not move re-cloned repo to ${src.local_path}: ${(e as Error).message}.${asideNote}`, e, ); } + if (aside) rmSync(aside, { recursive: true, force: true }); return true; } diff --git a/test/sources-ops.test.ts b/test/sources-ops.test.ts index 33297e785..c5595a079 100644 --- a/test/sources-ops.test.ts +++ b/test/sources-ops.test.ts @@ -23,9 +23,13 @@ import { getSourceStatus, recloneIfMissing, isPathContained, + isOwnedClone, + unownedHint, defaultCloneDir, SourceOpError, } from '../src/core/sources-ops.ts'; +import { readdirSync } from 'fs'; +import { runSources } from '../src/commands/sources.ts'; import { resetPgliteState } from './helpers/reset-pglite.ts'; import { withEnv } from './helpers/with-env.ts'; @@ -508,6 +512,273 @@ describe('recloneIfMissing — T4 restore + autopurge recovery', () => { }); }); +// --------------------------------------------------------------------------- +// #1881 — ownership guard: recloneIfMissing must NEVER delete a user working +// tree. Ownership (config.managed_clone OR default-location equality), not +// path-containment. +// --------------------------------------------------------------------------- + +describe('isOwnedClone — ownership predicate', () => { + test('marker config.managed_clone:true → owned (even at an external path)', async () => { + await withEnv2(async () => { + expect( + isOwnedClone({ + id: 'x', + local_path: '/some/external/path', + config: { remote_url: 'https://github.com/example/repo', managed_clone: true }, + }), + ).toBe(true); + }); + }); + + test('default-location clone, no marker → owned (back-compat equality)', async () => { + await withEnv2(async () => { + expect( + isOwnedClone({ + id: 'legacy', + local_path: defaultCloneDir('legacy'), + config: { remote_url: 'https://github.com/example/repo' }, + }), + ).toBe(true); + }); + }); + + test('external path, no marker → NOT owned (the #1881 federated shape)', async () => { + await withEnv2(async () => { + expect( + isOwnedClone({ + id: 'gstack-code-app-abc', + local_path: '/Users/dev/tt-flutter-app', + config: { remote_url: 'https://github.com/example/repo', federated: true }, + }), + ).toBe(false); + }); + }); + + test('null local_path → NOT owned', async () => { + await withEnv2(async () => { + expect(isOwnedClone({ id: 'x', local_path: null, config: {} })).toBe(false); + }); + }); + + test('config as JSON string (DB shape) is parsed', async () => { + await withEnv2(async () => { + expect( + isOwnedClone({ + id: 'x', + local_path: '/external', + config: JSON.stringify({ managed_clone: true }), + }), + ).toBe(true); + }); + }); +}); + +describe('unownedHint — healthy vs degraded guidance', () => { + test('healthy: read-only guidance, no "missing clone" framing', () => { + const msg = unownedHint({ id: 'x', local_path: '/Users/dev/repo' }, 'healthy'); + expect(msg).toMatch(/read-only/); + expect(msg).toMatch(/drop config\.remote_url/); + expect(msg).not.toMatch(/not a usable git repo/); + }); + + test('degraded: names the state and does not suggest dropping remote_url alone recovers it', () => { + const msg = unownedHint({ id: 'x', local_path: '/Users/dev/repo' }, 'no-git'); + expect(msg).toMatch(/not a usable git repo/); + expect(msg).toMatch(/no-git/); + }); +}); + +describe('recloneIfMissing — refuses to delete an unowned working tree (#1881)', () => { + test('external local_path + remote_url, no marker → throws unmanaged_path, tree survives', async () => { + await withEnv2(async () => { + // Simulate the gstack orchestrator's federated row: remote_url set, but + // local_path points at a live user working tree (no .git → no-git state), + // and NO managed_clone marker. + const userTree = join(FAKE_GIT_DIR, 'user-working-tree'); + rmSync(userTree, { recursive: true, force: true }); + mkdirSync(userTree, { recursive: true }); + const sentinel = join(userTree, 'KEEP_ME.txt'); + writeFileSync(sentinel, 'two unpushed commits live here'); + + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) + VALUES ('gstack-code-app-abc', 'flutter', $1, + '{"remote_url":"https://github.com/example/repo","federated":true}'::jsonb)`, + [userTree], + ); + + let threw: SourceOpError | null = null; + try { + await recloneIfMissing(engine, 'gstack-code-app-abc'); + } catch (e) { + threw = e as SourceOpError; + } + expect(threw).toBeInstanceOf(SourceOpError); + expect(threw?.code).toBe('unmanaged_path'); + // The working tree and its sentinel MUST survive untouched. + expect(existsSync(userTree)).toBe(true); + expect(existsSync(sentinel)).toBe(true); + }); + }); + + test('sync-shape: same refusal surfaces before any filesystem op', async () => { + await withEnv2(async () => { + // A degraded unowned path (the path does not exist at all → missing). + const ghost = join(FAKE_GIT_DIR, 'ghost-tree'); + rmSync(ghost, { recursive: true, force: true }); + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) + VALUES ('ghost', 'g', $1, + '{"remote_url":"https://github.com/example/repo"}'::jsonb)`, + [ghost], + ); + await expect(recloneIfMissing(engine, 'ghost')).rejects.toThrow(/unmanaged_path|not a clone gbrain created/); + }); + }); +}); + +describe('recloneIfMissing — symlink TOCTOU + EXDEV-safe swap', () => { + test('symlink at an owned default-location path → symlink_escape, target untouched', async () => { + await withEnv2(async () => { + // Owned by equality: local_path === defaultCloneDir(id). But the path is a + // symlink to a real dir → reclone must refuse rather than rename through it. + const id = 'sym-owned'; + const target = join(FAKE_GIT_DIR, 'sym-target'); + rmSync(target, { recursive: true, force: true }); + mkdirSync(target, { recursive: true }); + const targetSentinel = join(target, 'precious.txt'); + writeFileSync(targetSentinel, 'do not delete'); + + mkdirSync(CLONE_ROOT, { recursive: true }); + const clonePath = defaultCloneDir(id); // = CLONE_ROOT/sym-owned + symlinkSync(target, clonePath); + + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) + VALUES ($1, 's', $2, + '{"remote_url":"https://github.com/example/repo"}'::jsonb)`, + [id, clonePath], + ); + + let threw: SourceOpError | null = null; + try { + await recloneIfMissing(engine, id); + } catch (e) { + threw = e as SourceOpError; + } + expect(threw?.code).toBe('symlink_escape'); + // Symlink target and its contents survive. + expect(existsSync(targetSentinel)).toBe(true); + }); + }); + + test('owned no-git clone reclones; no .gbrain-reclone-* / .old-* residue left', async () => { + await withEnv2(async () => { + const row = await addSource(engine, { + id: 'swap-clean', + remoteUrl: 'https://github.com/example/repo', + }); + // Degrade to no-git so reclone fires. + rmSync(join(row.local_path!, '.git'), { recursive: true, force: true }); + + const recloned = await recloneIfMissing(engine, 'swap-clean'); + expect(recloned).toBe(true); + expect(existsSync(join(row.local_path!, '.git'))).toBe(true); + + // Parent (CLONE_ROOT) must hold no swap residue. + const residue = readdirSync(CLONE_ROOT).filter( + (e) => e.startsWith('.gbrain-reclone-') || e.includes('.old-'), + ); + expect(residue).toEqual([]); + }); + }); +}); + +describe('sources restore — unowned source (CV3)', () => { + test('restore of an unowned remote_url row: DB row restored, tree survives, correct guidance', async () => { + await withEnv2(async () => { + // Archived federated row: remote_url set, local_path = a live user tree + // (no .git, no managed_clone marker). Restore calls recloneIfMissing, + // which now throws unmanaged_path; runRestore must catch it, keep the tree, + // and NOT print the misleading "missing clone, try sync to recover" hint. + const userTree = join(FAKE_GIT_DIR, 'restore-user-tree'); + rmSync(userTree, { recursive: true, force: true }); + mkdirSync(userTree, { recursive: true }); + const sentinel = join(userTree, 'KEEP_ME.txt'); + writeFileSync(sentinel, 'live repo'); + + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, archived) + VALUES ('restore-unowned', 'flutter', $1, + '{"remote_url":"https://github.com/example/repo","federated":false}'::jsonb, + true)`, + [userTree], + ); + + const errs: string[] = []; + const origErr = console.error; + console.error = (...a: unknown[]) => { errs.push(a.join(' ')); }; + try { + // Real CLI dispatch → runRestore. Must not throw. + await runSources(engine, ['restore', 'restore-unowned']); + } finally { + console.error = origErr; + } + + // DB row un-archived (restore succeeded). + const rows = await engine.executeRaw<{ archived: boolean }>( + `SELECT archived FROM sources WHERE id = 'restore-unowned'`, + ); + expect(rows[0].archived).toBe(false); + // Working tree untouched. + expect(existsSync(userTree)).toBe(true); + expect(existsSync(sentinel)).toBe(true); + // Guidance is the read-only one, NOT the misleading "missing clone" hint. + const joined = errs.join('\n'); + expect(joined).toMatch(/read-only/); + expect(joined).not.toMatch(/on-disk clone is missing/); + }); + }); +}); + +describe('addSource --url — writes ownership marker', () => { + test('config carries managed_clone:true', async () => { + await withEnv2(async () => { + await addSource(engine, { + id: 'marked', + remoteUrl: 'https://github.com/example/repo', + }); + const rows = await engine.executeRaw<{ config: unknown }>( + `SELECT config FROM sources WHERE id = 'marked'`, + ); + const cfg = + typeof rows[0].config === 'string' + ? JSON.parse(rows[0].config as string) + : (rows[0].config as Record); + expect(cfg.managed_clone).toBe(true); + }); + }); + + test('--clone-dir clone (external path) is owned via marker and reclones', async () => { + await withEnv2(async () => { + const externalClone = join(FAKE_GIT_DIR, 'custom-clone-dir'); + rmSync(externalClone, { recursive: true, force: true }); + const row = await addSource(engine, { + id: 'cdir', + remoteUrl: 'https://github.com/example/repo', + cloneDir: externalClone, + }); + expect(row.local_path).toBe(externalClone); + // Remove the leaf → reclone must succeed (owned via marker, NOT containment). + rmSync(externalClone, { recursive: true, force: true }); + const recloned = await recloneIfMissing(engine, 'cdir'); + expect(recloned).toBe(true); + expect(existsSync(join(externalClone, '.git'))).toBe(true); + }); + }); +}); + // --------------------------------------------------------------------------- // isPathContained — symlink-safe confinement helper (exported for reuse) // --------------------------------------------------------------------------- diff --git a/test/sources-resync-recovery.test.ts b/test/sources-resync-recovery.test.ts index 0c63818f1..0913b6da7 100644 --- a/test/sources-resync-recovery.test.ts +++ b/test/sources-resync-recovery.test.ts @@ -254,4 +254,34 @@ describe('performSync re-clone branch (driven by sync.ts:320 logic)', () => { expect(recloned).toBe(true); }); }); + + // #1881: the sync branch must NOT re-clone over an unowned user working tree. + test('unowned local_path (remote_url, no marker): refuses, working tree survives', async () => { + await withEnv({ GBRAIN_HOME, PATH: fakePath() }, async () => { + // Federated row shape created by the gstack orchestrator: remote_url set, + // local_path = a live user tree OUTSIDE the clone root, no managed_clone. + const userTree = join(FAKE_GIT_DIR, 'sync-user-tree'); + rmSync(userTree, { recursive: true, force: true }); + mkdirSync(userTree, { recursive: true }); + const sentinel = join(userTree, 'KEEP_ME.txt'); + writeFileSync(sentinel, 'live repo'); + + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) + VALUES ('sync-unowned', 'flutter', $1, + '{"remote_url":"https://github.com/example/repo","federated":true}'::jsonb)`, + [userTree], + ); + + // no-git → the sync branch would historically call recloneIfMissing → + // rm the tree. Now it must throw unmanaged_path and leave the tree intact. + const state = validateRepoState(userTree, 'https://github.com/example/repo'); + expect(state).toBe('no-git'); + await expect(recloneIfMissing(engine, 'sync-unowned')).rejects.toThrow( + /unmanaged_path|not a clone gbrain created/, + ); + expect(existsSync(userTree)).toBe(true); + expect(existsSync(sentinel)).toBe(true); + }); + }); });