diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index e89cc7f23..4681508fb 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -6,7 +6,13 @@ */ import fs from 'fs/promises'; -import { findRepo, unregisterRepo, listRegisteredRepos } from '../storage/repo-manager.js'; +import { + findRepo, + unregisterRepo, + listRegisteredRepos, + assertSafeStoragePath, + UnsafeStoragePathError, +} from '../storage/repo-manager.js'; export const cleanCommand = async (options?: { force?: boolean; all?: boolean }) => { // --all flag: clean all indexed repos @@ -27,6 +33,24 @@ export const cleanCommand = async (options?: { force?: boolean; all?: boolean }) const entries = await listRegisteredRepos(); for (const entry of entries) { + // Safety guard (#1003 review — @magyargergo): same rationale as + // remove.ts. `~/.gitnexus/registry.json` is user-writable, so a + // corrupted or hand-edited entry could point storagePath at the + // repo root, an empty string, or anywhere else — and + // fs.rm(recursive: true) on any of those would be catastrophic. + // Skip poisoned entries without touching disk, but keep going + // through the rest of the registry (preserves the existing + // per-repo error-tolerance semantics of `clean --all`). + try { + assertSafeStoragePath(entry); + } catch (err) { + if (err instanceof UnsafeStoragePathError) { + console.error(`Refusing to clean ${entry.name}: ${err.message}`); + continue; + } + throw err; + } + try { await fs.rm(entry.storagePath, { recursive: true, force: true }); await unregisterRepo(entry.path); diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts index 1100b3e32..4d2ce0771 100644 --- a/gitnexus/src/cli/remove.ts +++ b/gitnexus/src/cli/remove.ts @@ -30,9 +30,11 @@ import fs from 'fs/promises'; import { readRegistry, resolveRegistryEntry, + assertSafeStoragePath, unregisterRepo, RegistryNotFoundError, RegistryAmbiguousTargetError, + UnsafeStoragePathError, } from '../storage/repo-manager.js'; export const removeCommand = async (target: string, options?: { force?: boolean }) => { @@ -72,6 +74,24 @@ export const removeCommand = async (target: string, options?: { force?: boolean return; } + // Safety guard (#1003 review — @magyargergo): refuse to proceed if + // the registry entry's `storagePath` isn't the canonical + // `/.gitnexus` subfolder. `~/.gitnexus/registry.json` is + // user-writable, so a corrupted or hand-edited entry could point + // storagePath at the repo root, an empty string (→ cwd), a parent + // dir, or anywhere else; `fs.rm(recursive: true, force: true)` on + // any of those would be a runtime disaster. Bail before touching + // disk, with an actionable hint for recovering a broken registry. + try { + assertSafeStoragePath(entry); + } catch (err) { + if (err instanceof UnsafeStoragePathError) { + console.error(`Error: ${err.message}`); + process.exit(1); + } + throw err; + } + // Deletion order: fs.rm first, then unregister. If fs.rm fails mid-way, // the registry entry stays so the user can retry. If fs.rm succeeds but // unregister throws (e.g. ENOSPC on registry write), the entry becomes diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index d7bf47228..8155e592f 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -550,6 +550,75 @@ export class RegistryAmbiguousTargetError extends Error { } } +/** + * Thrown by {@link assertSafeStoragePath} when a registry entry's + * `storagePath` does NOT point at the expected `/.gitnexus` + * subfolder. CLI destructive commands (`remove`, `clean --all`) should + * catch this and exit non-zero without deleting anything — the usual + * cause is a corrupted or hand-edited `~/.gitnexus/registry.json`, and + * proceeding would mean `fs.rm(recursive: true)` on whatever odd path + * the entry is pointing at. + */ +export class UnsafeStoragePathError extends Error { + readonly kind = 'UnsafeStoragePathError' as const; + constructor( + public readonly entry: RegistryEntry, + public readonly expectedStoragePath: string, + public readonly actualStoragePath: string, + ) { + super( + `Refusing to remove storage path for safety: expected ` + + `"${expectedStoragePath}" under the repo's .gitnexus subfolder, ` + + `but the registry entry has "${actualStoragePath}". ` + + `This usually means the registry entry is corrupted or was ` + + `hand-edited. Delete the entry manually from ~/.gitnexus/registry.json ` + + `and re-run analyze.`, + ); + this.name = 'UnsafeStoragePathError'; + } +} + +/** + * Guard rail for destructive CLI paths (`remove` #664, + * `clean --all` #258, future MCP `remove` tool): verify that a + * registry entry's `storagePath` is the canonical `/.gitnexus` + * subfolder of its `path`. If not, throw {@link UnsafeStoragePathError} + * so the caller exits without touching disk. + * + * Why this exists (#1003 review — @magyargergo): + * - `~/.gitnexus/registry.json` is a plain-text user-writable file. + * A corrupted, hand-edited, or downgrade/upgrade-racing entry + * could plausibly end up with `storagePath === ""` (resolves to + * cwd), `storagePath === path` (the repo root!), `storagePath` + * equal to a parent/sibling of the repo, or simply any arbitrary + * filesystem path. + * - `fs.rm(recursive: true, force: true)` on ANY of those would be + * a runtime disaster — at best delete the user's working tree, at + * worst nuke an unrelated directory tree they happen to own. + * - `clean` (default, cwd-scoped) is safe by construction — it + * re-derives storagePath from `findRepo(cwd)` and never trusts + * the registry field. But `clean --all` DOES iterate the registry + * and trust each entry's stored storagePath (same shape as + * `remove`), so this helper must be wired into that loop too. + * - `server/api.ts` recomputes storagePath from `getStoragePath(entry.path)` + * and so is likewise safe-by-construction. + * + * Pure string check — does NOT require the paths to exist on disk. + * Windows: case-insensitive; POSIX: case-sensitive. Matches the + * comparison shape used elsewhere in this module. + */ +export const assertSafeStoragePath = (entry: RegistryEntry): void => { + const expected = path.join(path.resolve(entry.path), '.gitnexus'); + const actual = path.resolve(entry.storagePath); + const matches = + process.platform === 'win32' + ? expected.toLowerCase() === actual.toLowerCase() + : expected === actual; + if (!matches) { + throw new UnsafeStoragePathError(entry, expected, actual); + } +}; + /** * Resolve a user-supplied target string (from `gitnexus remove ` * or equivalent MCP tool argument) to a single registry entry. diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index a23293e27..cc41999da 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -539,6 +539,195 @@ describe('CLI end-to-end', () => { fs.rmSync(parentB, { recursive: true, force: true }); } }, 240000); // 4-min outer budget (2 × ~60s analyze + 2 × fast remove) + + it('refuses to proceed when a registry entry points storagePath outside /.gitnexus (#1003)', () => { + // Regression guard for the safety gap flagged by @magyargergo on + // PR #1003: `~/.gitnexus/registry.json` is a user-writable JSON + // file, so a corrupted or hand-edited entry could point + // storagePath at the repo root (catastrophic: rm the working + // tree) or at any other arbitrary path. `remove --force` must + // refuse to call fs.rm when storagePath isn't the canonical + // `/.gitnexus`. We verify: + // 1. Exit code 1 with the actionable "registry entry corrupted" + // hint. + // 2. The .gitnexus/ storage dir is UNTOUCHED. + // 3. The repo itself (entry.path) is UNTOUCHED. + // 4. The registry entry is NOT removed (no partial mutation). + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-poison-')); + const repo = makeMiniRepoCopy('poisoned', 'gn-poison-'); + const parent = path.dirname(repo); + + try { + // Index the repo normally first so the registry has a valid + // entry we can then poison. + const r1 = runCliWithEnv( + ['analyze', '--name', 'poisoned-alias'], + repo, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r1.status === null) return; + expect(r1.status).toBe(0); + + const registryPath = path.join(gnHome, 'registry.json'); + const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(original).toHaveLength(1); + + // Poison the entry: set storagePath to the REPO ROOT itself. + // If the guard isn't in place, `remove --force` would call + // `fs.rm(repo, {recursive: true, force: true})` and wipe the + // entire working tree. + const poisoned = [{ ...original[0], storagePath: repo }]; + fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2)); + + // Sanity: storage dir and working tree both still exist. + expect(fs.existsSync(path.join(repo, '.gitnexus'))).toBe(true); + expect(fs.existsSync(repo)).toBe(true); + expect(fs.existsSync(path.join(repo, '.git'))).toBe(true); + + // Attempt the remove — must FAIL without deleting anything. + const r2 = runCliWithEnv( + ['remove', 'poisoned-alias', '--force'], + parent, + { GITNEXUS_HOME: gnHome }, + 15000, + ); + if (r2.status === null) return; + + expect( + r2.status, + [`remove should have exited 1`, `stdout: ${r2.stdout}`, `stderr: ${r2.stderr}`].join( + '\n', + ), + ).toBe(1); + const r2Output = `${r2.stdout}${r2.stderr}`; + // Must surface the actionable "registry corrupted" hint, not + // just a raw fs.rm error. + expect(r2Output).toMatch(/Refusing to remove/i); + expect(r2Output).toMatch(/registry\.json/i); + + // Repo + .gitnexus dir + .git dir must all still exist — the + // guard aborts BEFORE fs.rm. This is the whole point of the + // test: the working tree is not allowed to disappear. + expect(fs.existsSync(repo), 'repo working tree must survive').toBe(true); + expect(fs.existsSync(path.join(repo, '.gitnexus')), 'storage dir must survive').toBe(true); + expect(fs.existsSync(path.join(repo, '.git')), '.git must survive').toBe(true); + + // Registry unchanged — no partial mutation. + const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterRegistry).toHaveLength(1); + expect(afterRegistry[0].storagePath).toBe(repo); // still poisoned (we did that) + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parent, { recursive: true, force: true }); + } + }, 120000); // 2-min budget (1 × ~60s analyze + 1 × fast remove-refused) + }); + + // ─── clean --all: same safety guard applies (#1003 review) ─────── + // + // The `clean --all` path iterates over the registry and calls + // `fs.rm(entry.storagePath)` — identical trust-the-registry pattern + // as `remove` had before the guard. A poisoned entry must be SKIPPED + // (not aborted), so clean --all preserves its existing per-repo + // error-tolerance semantics: one bad entry does not halt cleanup of + // the rest. We verify: + // 1. The poisoned entry is NOT deleted (working tree + .gitnexus + // survive), and the CLI prints a "Refusing to clean" message. + // 2. The poisoned entry is left in the registry (nothing was + // mutated for it). + // 3. A co-existing well-formed entry IS still cleaned (both its + // .gitnexus dir AND its registry entry are gone). + describe('clean --all with a poisoned registry entry (#1003)', () => { + it('skips poisoned entries, cleans valid ones, never deletes the working tree', () => { + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-clean-poison-')); + const repoBad = makeMiniRepoCopy('bad-repo', 'gn-clean-bad-'); + const repoGood = makeMiniRepoCopy('good-repo', 'gn-clean-good-'); + const parentBad = path.dirname(repoBad); + const parentGood = path.dirname(repoGood); + + try { + // Analyze both so the registry has two well-formed entries. + for (const [repo, alias] of [ + [repoBad, 'bad-alias'], + [repoGood, 'good-alias'], + ] as const) { + const r = runCliWithEnv( + ['analyze', '--name', alias], + repo, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r.status === null) return; + expect(r.status, `analyze ${alias} exited ${r.status}: ${r.stdout}${r.stderr}`).toBe(0); + } + + const registryPath = path.join(gnHome, 'registry.json'); + const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(original).toHaveLength(2); + + // Poison the 'bad-alias' entry by pointing its storagePath at + // the repo root itself. If the guard isn't wired into the + // clean --all loop, `clean --all --force` would fs.rm the + // working tree. + const poisoned = original.map((e: { name: string; storagePath: string; path: string }) => + e.name === 'bad-alias' ? { ...e, storagePath: repoBad } : e, + ); + fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2)); + + // Sanity: both working trees and .gitnexus dirs still exist. + expect(fs.existsSync(repoBad)).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.gitnexus'))).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.git'))).toBe(true); + expect(fs.existsSync(path.join(repoGood, '.gitnexus'))).toBe(true); + + // clean --all --force from a neutral cwd (parentBad), so the + // command isn't "inside" either repo. + const r = runCliWithEnv( + ['clean', '--all', '--force'], + parentBad, + { GITNEXUS_HOME: gnHome }, + 30000, + ); + if (r.status === null) return; + + // clean --all's per-entry error handling always exits 0 at + // the end (it only logs per-repo failures). The important + // assertions are on side effects, not the exit code. + const output = `${r.stdout}${r.stderr}`; + expect(output).toMatch(/Refusing to clean/i); + expect(output).toMatch(/bad-alias/); + + // Poisoned repo: working tree + .gitnexus + .git all SURVIVE. + expect(fs.existsSync(repoBad), 'poisoned repo working tree must survive').toBe(true); + expect( + fs.existsSync(path.join(repoBad, '.gitnexus')), + 'poisoned repo .gitnexus must survive (guard refused to rm repo root)', + ).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.git')), '.git must survive').toBe(true); + + // Good repo: its .gitnexus IS gone (cleanup succeeded despite + // the poisoned sibling entry — per-entry error tolerance is + // preserved). + expect( + fs.existsSync(path.join(repoGood, '.gitnexus')), + 'good repo .gitnexus should be cleaned', + ).toBe(false); + // But the good repo's working tree stays (clean never touches + // anything outside .gitnexus). + expect(fs.existsSync(repoGood), 'good repo working tree must survive').toBe(true); + + // Registry post-state: poisoned entry still present (skipped, + // not mutated); good entry unregistered. + const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterRegistry).toHaveLength(1); + expect(afterRegistry[0].name).toBe('bad-alias'); + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parentBad, { recursive: true, force: true }); + fs.rmSync(parentGood, { recursive: true, force: true }); + } + }, 240000); // 4-min budget (2 × ~60s analyze + 1 × fast clean --all) }); describe('unhappy path', () => { diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 0d4e072d1..12c56d67a 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -17,9 +17,11 @@ import { listRegisteredRepos, resolveRegistryEntry, canonicalizePath, + assertSafeStoragePath, RegistryNameCollisionError, RegistryNotFoundError, RegistryAmbiguousTargetError, + UnsafeStoragePathError, type RegistryEntry, type RepoMeta, } from '../../src/storage/repo-manager.js'; @@ -690,3 +692,102 @@ describe('resolveRegistryEntry backward-compat with non-canonical stored paths ( expect(hit).toBe(entries[0]); }); }); + +// ─── assertSafeStoragePath (#1003 review — @magyargergo) ───────────── +// +// Guard rail against destroying more than the `.gitnexus/` subfolder. +// `~/.gitnexus/registry.json` is user-writable plain text, so a +// corrupted or hand-edited entry could put storagePath anywhere. +// These tests use synthetic `RegistryEntry` fixtures (no disk I/O) +// because the guard is a pure string check — it must not depend on +// the paths existing. + +describe('assertSafeStoragePath (#1003)', () => { + const prefix = process.platform === 'win32' ? 'D:\\' : '/tmp/'; + const repoPath = `${prefix}projects${path.sep}my-repo`; + const base: Omit = { + name: 'my-repo', + path: repoPath, + indexedAt: '2026-04-21T00:00:00.000Z', + lastCommit: 'deadbee', + }; + + it('accepts the canonical /.gitnexus storage path', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.join(repoPath, '.gitnexus'), + }; + expect(() => assertSafeStoragePath(entry)).not.toThrow(); + }); + + it('rejects when storagePath equals the repo path itself (would delete the code)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: repoPath, // catastrophic: rm the working tree + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is a parent of the repo path', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.dirname(repoPath), // also catastrophic + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is empty (path.resolve falls back to cwd)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: '', // path.resolve('') === process.cwd() — would rm cwd + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath points somewhere totally unrelated', () => { + const entry: RegistryEntry = { + ...base, + storagePath: `${prefix}some${path.sep}other${path.sep}place`, + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is a sibling .gitnexus (right basename, wrong parent)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.join(`${prefix}different${path.sep}repo`, '.gitnexus'), + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('UnsafeStoragePathError carries the original entry + expected + actual paths', () => { + const entry: RegistryEntry = { + ...base, + storagePath: `${prefix}evil${path.sep}path`, + }; + try { + assertSafeStoragePath(entry); + } catch (e) { + expect(e).toBeInstanceOf(UnsafeStoragePathError); + const err = e as UnsafeStoragePathError; + expect(err.kind).toBe('UnsafeStoragePathError'); + expect(err.entry).toBe(entry); + // Expected path is the canonical `/.gitnexus`. + expect(err.expectedStoragePath).toBe(path.join(path.resolve(repoPath), '.gitnexus')); + // Actual path is the corrupted value (resolved). + expect(err.actualStoragePath).toBe(path.resolve(entry.storagePath)); + // Message must suggest the recovery action. + expect(err.message).toContain('registry.json'); + } + }); + + it('Windows: storagePath match is case-insensitive to match register/unregister semantics', () => { + if (process.platform !== 'win32') return; + const entry: RegistryEntry = { + ...base, + storagePath: path.join(repoPath.toUpperCase(), '.GITNEXUS'), + }; + // Should accept because Windows paths are case-insensitive. + expect(() => assertSafeStoragePath(entry)).not.toThrow(); + }); +});