diff --git a/gitnexus/src/server/git-clone.ts b/gitnexus/src/server/git-clone.ts index 477f47a6e..742be319c 100644 --- a/gitnexus/src/server/git-clone.ts +++ b/gitnexus/src/server/git-clone.ts @@ -10,7 +10,7 @@ import path from 'path'; import fs from 'fs/promises'; import { isIP } from 'net'; import { logger } from '../core/logger.js'; -import { parseRepoNameFromUrl } from '../storage/git.js'; +import { parseRepoNameFromUrl, stripUrlCredentials } from '../storage/git.js'; import { getGlobalDir } from '../storage/repo-manager.js'; /** @@ -410,7 +410,10 @@ export async function assertRemoteMatchesRequestedUrl( } if (normalizeGitUrlForCompare(remoteUrl) !== normalizeGitUrlForCompare(requestedUrl)) { throw new Error( - `Existing clone at ${targetDir} has remote ${remoteUrl}, not the requested URL ${requestedUrl}`, + // Both URLs are echoed to the API caller and the server log, and either + // can carry `https://user:token@` userinfo — strip it here too (#2914). + `Existing clone at ${targetDir} has remote ${stripUrlCredentials(remoteUrl)}, ` + + `not the requested URL ${stripUrlCredentials(requestedUrl)}`, ); } } diff --git a/gitnexus/src/storage/git.ts b/gitnexus/src/storage/git.ts index 585322b11..4df906eac 100644 --- a/gitnexus/src/storage/git.ts +++ b/gitnexus/src/storage/git.ts @@ -201,6 +201,28 @@ export const getCurrentCommit = (repoPath: string): string => { } }; +/** + * Remove `user[:password]@` userinfo from an http(s) URL. + * + * `git config remote.origin.url` returns whatever was configured, and the + * HTTPS token form `https://x-access-token:@host/owner/repo` is how + * CI checkouts and credential helpers routinely authenticate. That string + * reached `registry.json`, the per-repo meta and the MCP `list_repos` + * payload verbatim, which turned repository discovery into credential + * disclosure (#2914). + * + * Only `http`/`https` are rewritten. `ssh://git@host/…` and the SCP-like + * `git@host:owner/repo` carry an SSH *user name*, not a secret, and are part + * of the remote's identity — dropping it would repoint the sibling-clone + * fingerprint (#2054) for every already-registered repo. + * + * The match is bounded by the authority (`[^/]*` cannot cross the first `/` + * after the scheme) and greedy to the last `@` in it, so a password + * containing `@` is removed whole rather than leaving its tail behind. + */ +export const stripUrlCredentials = (url: string): string => + url.replace(/^(https?:\/\/)[^/]*@/i, '$1'); + /** * Get a stable canonical identifier for the repo's `origin` remote, if any. * @@ -212,6 +234,10 @@ export const getCurrentCommit = (repoPath: string): string => { * survives those conventions. * * Normalisation strategy: + * - Strip http(s) userinfo credentials (see {@link stripUrlCredentials}). + * Done FIRST, before the host lower-casing below — that regex treats the + * whole `user:pass@host` span as the host and would mangle the secret's + * case on its way into the registry (#2914). * - Strip a trailing `.git` so `https://x/y` and `https://x/y.git` collapse. * - Strip a trailing `/` for the same reason. * - `git@github.com:foo/bar` and `https://github.com/foo/bar` are @@ -239,7 +265,9 @@ export const getRemoteUrl = (repoPath: string): string | undefined => { } if (!raw) return undefined; - let normalised = raw.replace(/\/$/, '').replace(/\.git$/, ''); + let normalised = stripUrlCredentials(raw) + .replace(/\/$/, '') + .replace(/\.git$/, ''); // Lower-case the host segment of `scheme://[user@]host[:port]/...` // and the host segment of `git@host:owner/repo` SCP form. diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 843d90a3d..cbfdf441f 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -18,7 +18,7 @@ import fs from 'fs/promises'; import { realpathSync } from 'fs'; import path from 'path'; import os from 'os'; -import { getInferredRepoName, resolveRepoIdentityRoot } from './git.js'; +import { getInferredRepoName, resolveRepoIdentityRoot, stripUrlCredentials } from './git.js'; import { stripWindowsLongPathPrefix } from '../lib/utils.js'; import { writeFileAtomic } from './fs-atomic.js'; import { logger } from '../core/logger.js'; @@ -1115,6 +1115,27 @@ const withRegistryLock = async (operation: () => Promise): Promise => { } }; +/** + * Drop credentials from every entry's `remoteUrl` (#2914). + * + * Applied on BOTH registry edges. Capture-time stripping in `getRemoteUrl` + * only covers values this version writes; a `registry.json` (or a per-repo + * meta that a re-register copies forward) written by an older version still + * holds the credential. Reading through here keeps it out of every consumer — + * `listRegisteredRepos`, MCP `list_repos`, `gitnexus list`, group sync — and + * writing through here means the next registry write drops it at rest instead + * of round-tripping it back to disk. + * + * Sanitised values compare equal to a freshly captured `getRemoteUrl`, so + * sibling-clone matching (#2054) is unaffected: both sides lose the same span. + */ +const sanitizeEntries = (entries: RegistryEntry[]): RegistryEntry[] => + entries.map((e) => { + if (!e.remoteUrl) return e; + const cleaned = stripUrlCredentials(e.remoteUrl); + return cleaned === e.remoteUrl ? e : { ...e, remoteUrl: cleaned }; + }); + /** * Read the global registry. Returns empty array if not found. */ @@ -1122,7 +1143,7 @@ export const readRegistry = async (): Promise => { try { const raw = await fs.readFile(getGlobalRegistryPath(), 'utf-8'); const data = JSON.parse(raw); - return Array.isArray(data) ? data : []; + return Array.isArray(data) ? sanitizeEntries(data) : []; } catch { return []; } @@ -1142,7 +1163,11 @@ export const readRegistry = async (): Promise => { */ const writeRegistry = async (entries: RegistryEntry[], attempts?: number): Promise => { await fs.mkdir(getGlobalDir(), { recursive: true }); - await writeFileAtomic(getGlobalRegistryPath(), JSON.stringify(entries, null, 2), attempts); + await writeFileAtomic( + getGlobalRegistryPath(), + JSON.stringify(sanitizeEntries(entries), null, 2), + attempts, + ); }; /** diff --git a/gitnexus/test/unit/git-utils.test.ts b/gitnexus/test/unit/git-utils.test.ts index b1fc8f7bd..feb49effd 100644 --- a/gitnexus/test/unit/git-utils.test.ts +++ b/gitnexus/test/unit/git-utils.test.ts @@ -189,17 +189,17 @@ describe('getGitRoot', () => { // ─── getRemoteUrl ───────────────────────────────────────────────────────── -describe('getRemoteUrl', () => { - const setupRepoWithRemote = (remoteUrl: string): string => { - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-remote-')); - // Use real fs paths and shellouts — the helper itself shells out to - // `git config`, so we need a real git repo for the assertion to be - // meaningful. - execSync('git init -q', { cwd: tmpDir }); - execSync(`git remote add origin ${remoteUrl}`, { cwd: tmpDir }); - return tmpDir; - }; +const setupRepoWithRemote = (remoteUrl: string): string => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-remote-')); + // Use real fs paths and shellouts — the helper itself shells out to + // `git config`, so we need a real git repo for the assertion to be + // meaningful. + execSync('git init -q', { cwd: tmpDir }); + execSync(`git remote add origin ${remoteUrl}`, { cwd: tmpDir }); + return tmpDir; +}; +describe('getRemoteUrl', () => { it('returns undefined for a non-git directory', async () => { const { getRemoteUrl } = await import('../../src/storage/git.js'); const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-test-')); @@ -255,6 +255,80 @@ describe('getRemoteUrl', () => { }); }); +// ─── credentials in remote URLs (#2914) ────────────────────────────────── +// +// `git config remote.origin.url` hands back whatever CI or a credential +// helper configured, including `https://x-access-token:@host/…`. +// That value is persisted (registry.json, the per-repo meta) and echoed by +// MCP `list_repos`, so it must lose its userinfo at capture time. Every +// credential below is an obviously fake constant. + +describe('remote URL credentials (#2914)', () => { + const FAKE_TOKEN = 'ExAmPle-FAKE-SECRET'; + + it('strips userinfo from an HTTPS remote before it can be persisted', async () => { + const { getRemoteUrl } = await import('../../src/storage/git.js'); + const tmpDir = setupRepoWithRemote( + `https://x-access-token:${FAKE_TOKEN}@github.com/example/project.git`, + ); + try { + expect(getRemoteUrl(tmpDir)).toBe('https://github.com/example/project'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('strips a username-only HTTPS remote (the PAT-as-username form)', async () => { + const { getRemoteUrl } = await import('../../src/storage/git.js'); + const tmpDir = setupRepoWithRemote(`https://${FAKE_TOKEN}@github.com/example/project.git`); + try { + expect(getRemoteUrl(tmpDir)).toBe('https://github.com/example/project'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + it('leaves plain HTTPS, SSH-URL and SCP-like remotes untouched', async () => { + const { stripUrlCredentials } = await import('../../src/storage/git.js'); + // The SSH forms carry a user NAME, not a secret, and are part of the + // remote's identity — rewriting them would repoint the #2054 fingerprint. + expect(stripUrlCredentials('https://github.com/example/project.git')).toBe( + 'https://github.com/example/project.git', + ); + expect(stripUrlCredentials('ssh://git@github.com/example/project.git')).toBe( + 'ssh://git@github.com/example/project.git', + ); + expect(stripUrlCredentials('git@github.com:example/project.git')).toBe( + 'git@github.com:example/project.git', + ); + // An `@` in the PATH is not userinfo — the authority ends at the first `/`. + expect(stripUrlCredentials('https://github.com/example/pro@ject')).toBe( + 'https://github.com/example/pro@ject', + ); + }); + + it('removes a password containing @ whole, leaving no tail behind', async () => { + const { stripUrlCredentials } = await import('../../src/storage/git.js'); + expect(stripUrlCredentials(`https://user:pa@ss-${FAKE_TOKEN}@host.example/o/r`)).toBe( + 'https://host.example/o/r', + ); + }); + + it('keeps the same fingerprint for credentialed and clean clones of one repo', async () => { + const { getRemoteUrl } = await import('../../src/storage/git.js'); + const withCreds = setupRepoWithRemote( + `https://x-access-token:${FAKE_TOKEN}@example.com/foo/bar.git`, + ); + const clean = setupRepoWithRemote('https://example.com/foo/bar'); + try { + expect(getRemoteUrl(withCreds)).toBe(getRemoteUrl(clean)); + } finally { + fs.rmSync(withCreds, { recursive: true, force: true }); + fs.rmSync(clean, { recursive: true, force: true }); + } + }); +}); + // ─── getCanonicalRepoRoot (#1259) ──────────────────────────────────────── // // Critical for the worktree-naming bug: when `gitnexus analyze` runs from a diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index a0a48d52e..1c6c20e5f 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -975,6 +975,112 @@ describe('registerRepo name override + collision guard (#829)', () => { // ─── registerRepo branch nesting (#2106) ───────────────────────────── +// ─── remoteUrl credentials (#2914) ─────────────────────────────────── +// +// The registry is the surface `list_repos` (MCP), `gitnexus list` and group +// sync read from, so a `remoteUrl` carrying `https://user:token@` turns repo +// discovery into credential disclosure. Capture-time stripping in +// `getRemoteUrl` only covers what THIS version writes — a registry.json (or a +// per-repo meta a re-register copies forward) written by an older version +// still holds one, so both registry edges sanitize. Fake credential only. + +describe('registry never emits or persists remoteUrl credentials (#2914)', () => { + const FAKE_TOKEN = 'ExAmPle-FAKE-SECRET'; + const CREDENTIALED = `https://x-access-token:${FAKE_TOKEN}@github.com/example/project`; + const CLEAN = 'https://github.com/example/project'; + + let tmpHome: Awaited>; + let tmpRepo: Awaited>; + let savedGitnexusHome: string | undefined; + let registryPath: string; + + const meta: RepoMeta = { + repoPath: '', + lastCommit: 'abc1234', + indexedAt: '2026-08-11T12:00:00.000Z', + stats: { files: 1, nodes: 1 }, + }; + + beforeEach(async () => { + tmpHome = await createTempDir('gitnexus-2914-home-'); + tmpRepo = await createTempDir('gitnexus-2914-repo-'); + savedGitnexusHome = process.env.GITNEXUS_HOME; + process.env.GITNEXUS_HOME = tmpHome.dbPath; + registryPath = path.join(tmpHome.dbPath, 'registry.json'); + }); + + afterEach(async () => { + if (savedGitnexusHome === undefined) delete process.env.GITNEXUS_HOME; + else process.env.GITNEXUS_HOME = savedGitnexusHome; + await tmpHome.cleanup(); + await tmpRepo.cleanup(); + }); + + /** A registry.json as an older version would have left it. */ + const seedLegacyRegistry = async (entryPath: string): Promise => { + const legacy: RegistryEntry[] = [ + { + name: 'legacy', + path: entryPath, + storagePath: path.join(entryPath, '.gitnexus'), + indexedAt: meta.indexedAt, + lastCommit: meta.lastCommit, + remoteUrl: CREDENTIALED, + }, + ]; + await fs.writeFile(registryPath, JSON.stringify(legacy, null, 2), 'utf-8'); + }; + + it('sanitizes a legacy on-disk entry before listRegisteredRepos returns it', async () => { + await seedLegacyRegistry(tmpRepo.dbPath); + + const entries = await listRegisteredRepos(); + + expect(entries).toHaveLength(1); + expect(entries[0].remoteUrl).toBe(CLEAN); + expect(JSON.stringify(entries)).not.toContain(FAKE_TOKEN); + }); + + it('never writes a credentialed remoteUrl to registry.json', async () => { + // meta.remoteUrl bypasses getRemoteUrl entirely — this is the legacy + // per-repo gitnexus.json being copied forward into a fresh registry. + await registerRepo(tmpRepo.dbPath, { ...meta, remoteUrl: CREDENTIALED }, { name: 'repro' }); + + const raw = await fs.readFile(registryPath, 'utf-8'); + expect(raw).not.toContain(FAKE_TOKEN); + expect((JSON.parse(raw) as RegistryEntry[])[0].remoteUrl).toBe(CLEAN); + }); + + it('scrubs an untouched legacy entry when some other repo is registered', async () => { + const other = await createTempDir('gitnexus-2914-other-'); + try { + await seedLegacyRegistry(other.dbPath); + await registerRepo(tmpRepo.dbPath, meta, { name: 'fresh' }); + + const raw = await fs.readFile(registryPath, 'utf-8'); + expect(raw).not.toContain(FAKE_TOKEN); + // The legacy entry survives — it is scrubbed, not dropped. + expect(JSON.parse(raw)).toHaveLength(2); + } finally { + await other.cleanup(); + } + }); + + it('still matches sibling clones after sanitization (#2054 fingerprint)', async () => { + await registerRepo(tmpRepo.dbPath, { ...meta, remoteUrl: CREDENTIALED }, { name: 'with-cred' }); + const other = await createTempDir('gitnexus-2914-sibling-'); + try { + await registerRepo(other.dbPath, { ...meta, remoteUrl: CLEAN }, { name: 'clean' }); + + const entries = await listRegisteredRepos(); + const remotes = entries.map((e) => e.remoteUrl); + expect(remotes).toEqual([CLEAN, CLEAN]); + } finally { + await other.cleanup(); + } + }); +}); + describe('registerRepo branch nesting (#2106)', () => { let tmpHome: Awaited>; let tmpRepo: Awaited>;