diff --git a/gitnexus/src/core/group/bridge-db.ts b/gitnexus/src/core/group/bridge-db.ts index 76eb5c0f7..ef45d6819 100644 --- a/gitnexus/src/core/group/bridge-db.ts +++ b/gitnexus/src/core/group/bridge-db.ts @@ -1,6 +1,6 @@ import fsp from 'node:fs/promises'; import path from 'node:path'; -import { createHash } from 'node:crypto'; +import { createHash, randomBytes } from 'node:crypto'; import lbug from '@ladybugdb/core'; import type { LbugValue } from '@ladybugdb/core'; import type { BridgeHandle, BridgeMeta, StoredContract, CrossLink, RepoSnapshot } from './types.js'; @@ -24,7 +24,7 @@ import { dedupeContracts, dedupeCrossLinks } from './normalization.js'; * - `.shadow` — non-blocking concurrent checkpoint sidecar (added in * LadybugDB 0.15.4); same pairing constraint as `.wal`. * - * `bridge-db` writes to a `bridge.lbug.tmp` file and then atomically renames + * `bridge-db` writes to a `bridge.lbug.tmp.` file and then atomically renames * it into place. The rename only moves the main file; sidecars must be * cleaned up explicitly or the next writer trips the database-id check. */ @@ -41,6 +41,26 @@ async function removeLbugFile(basePath: string): Promise { } } +/** + * Remove all stale `bridge.lbug.tmp.*` files (and their sidecars) from a + * group directory. With randomBytes-based temp names, a crashed writeBridge + * leaves behind a uniquely-named tmp file that no future run will target by + * name — so we glob for the prefix and clean up everything matching. + */ +async function cleanStaleBridgeTmpFiles(groupDir: string): Promise { + try { + const entries = await fsp.readdir(groupDir); + const staleBases = entries.filter( + (e) => e.startsWith('bridge.lbug.tmp.') && !LBUG_SIDECAR_SUFFIXES.some((s) => e.endsWith(s)), + ); + for (const name of staleBases) { + await removeLbugFile(path.join(groupDir, name)); + } + } catch { + /* best-effort: directory may not exist yet */ + } +} + export function contractNodeId( repo: string, contractId: string, @@ -276,7 +296,7 @@ export async function retryRename(src: string, dst: string, attempts = 3): Promi export async function writeBridgeMeta(groupDir: string, meta: BridgeMeta): Promise { const target = path.join(groupDir, 'meta.json'); - const tmp = `${target}.tmp.${Date.now()}`; + const tmp = `${target}.tmp.${randomBytes(8).toString('hex')}`; await fsp.writeFile(tmp, JSON.stringify(meta, null, 2), 'utf-8'); // Use retryRename for consistency with writeBridge's atomic swap — on // Windows a concurrent reader can cause EBUSY/EPERM even on a tiny @@ -346,7 +366,7 @@ export async function writeBridge( const crossLinks = dedupeCrossLinks(input.crossLinks); const finalPath = path.join(groupDir, 'bridge.lbug'); - const tmpPath = path.join(groupDir, 'bridge.lbug.tmp'); + const tmpPath = path.join(groupDir, `bridge.lbug.tmp.${randomBytes(8).toString('hex')}`); const bakPath = path.join(groupDir, 'bridge.lbug.bak'); const report: WriteBridgeReport = { @@ -366,11 +386,12 @@ export async function writeBridge( } }; - // Clean up any leftover tmp main file AND its `.wal` / `.shadow` sidecars. - // LadybugDB 0.16.0 rejects opening a database whose sidecars belong to a - // different database instance (database-id check), so any stale sidecar - // from a crashed previous run will fail the next writeBridge. - await removeLbugFile(tmpPath); + // Clean up stale tmp files left behind by previously crashed writeBridge + // runs. With randomBytes-based names each run picks a unique path, so + // the old fixed-name `removeLbugFile(tmpPath)` was a no-op — stale + // artifacts accumulated. The glob-based helper finds *all* leftover + // `bridge.lbug.tmp.*` entries and removes them (including sidecars). + await cleanStaleBridgeTmpFiles(groupDir); // 1. Create temp DB, insert all data. // diff --git a/gitnexus/src/core/group/storage.ts b/gitnexus/src/core/group/storage.ts index 5380b9867..99bf27fbd 100644 --- a/gitnexus/src/core/group/storage.ts +++ b/gitnexus/src/core/group/storage.ts @@ -2,6 +2,7 @@ import * as fs from 'node:fs'; import * as fsp from 'node:fs/promises'; import * as path from 'node:path'; import * as os from 'node:os'; +import { randomBytes } from 'node:crypto'; import type { ContractRegistry } from './types.js'; const CONTRACTS_FILE = 'contracts.json'; @@ -34,7 +35,7 @@ export async function writeContractRegistry( registry: ContractRegistry, ): Promise { const targetPath = path.join(groupDir, CONTRACTS_FILE); - const tmpPath = `${targetPath}.tmp.${Date.now()}`; + const tmpPath = `${targetPath}.tmp.${randomBytes(8).toString('hex')}`; await fsp.writeFile(tmpPath, JSON.stringify(registry, null, 2), 'utf-8'); await fsp.rename(tmpPath, targetPath); diff --git a/gitnexus/test/unit/group/insecure-tempfile.test.ts b/gitnexus/test/unit/group/insecure-tempfile.test.ts new file mode 100644 index 000000000..17798a199 --- /dev/null +++ b/gitnexus/test/unit/group/insecure-tempfile.test.ts @@ -0,0 +1,192 @@ +/** + * Security tests for insecure tempfile remediation (#1318 U6). + * + * CodeQL js/insecure-temporary-file flags predictable temp filenames + * (e.g. Date.now() suffix) because an attacker with write access to + * the same directory can win a symlink race. The fix replaces all + * predictable suffixes with crypto.randomBytes(8). + * + * Two layers: + * 1. Structural — source-grep confirms randomBytes, not Date.now(). + * 2. Behavioural — writeContractRegistry produces no leftover tmp files + * and the final file is correctly written. + */ +import { beforeAll, describe, expect, it, beforeEach, afterEach } from 'vitest'; +import fs from 'node:fs'; +import fsp from 'node:fs/promises'; +import path from 'node:path'; +import os from 'node:os'; +import { writeContractRegistry, readContractRegistry } from '../../../src/core/group/storage.js'; +import { writeBridgeMeta, readBridgeMeta } from '../../../src/core/group/bridge-db.js'; +import type { ContractRegistry, BridgeMeta } from '../../../src/core/group/types.js'; + +// --------------------------------------------------------------------------- +// Structural: source files use randomBytes, not Date.now(), for temp paths +// --------------------------------------------------------------------------- + +describe('insecure tempfile — structural guards (#1318 U6)', () => { + let bridgeSource: string; + let storageSource: string; + + beforeAll(async () => { + bridgeSource = await fsp.readFile( + path.join(__dirname, '..', '..', '..', 'src', 'core', 'group', 'bridge-db.ts'), + 'utf-8', + ); + storageSource = await fsp.readFile( + path.join(__dirname, '..', '..', '..', 'src', 'core', 'group', 'storage.ts'), + 'utf-8', + ); + }); + + it('bridge-db.ts imports randomBytes from node:crypto', () => { + expect(bridgeSource).toMatch(/import\s*\{[^}]*randomBytes[^}]*\}\s*from\s*'node:crypto'/); + }); + + it('bridge-db.ts uses randomBytes for bridge.lbug temp path', () => { + expect(bridgeSource).toMatch(/bridge\.lbug\.tmp\.\$\{randomBytes/); + }); + + it('bridge-db.ts uses randomBytes for meta.json temp path', () => { + expect(bridgeSource).toMatch(/\.tmp\.\$\{randomBytes\(8\)\.toString\('hex'\)\}/); + }); + + it('bridge-db.ts does not use Date.now() in any temp path', () => { + // Match Date.now() specifically in tmp-path contexts — not in unrelated code. + const tmpDateNow = bridgeSource.match(/\.tmp\.\$\{Date\.now\(\)\}/g) ?? []; + expect(tmpDateNow.length).toBe(0); + }); + + it('bridge-db.ts uses readdir-based cleanup for stale bridge tmp files', () => { + expect(bridgeSource).toMatch(/cleanStaleBridgeTmpFiles/); + expect(bridgeSource).toMatch(/readdir\(groupDir\)/); + expect(bridgeSource).toMatch(/startsWith\('bridge\.lbug\.tmp\.'\)/); + }); + + it('bridge-db.ts calls cleanStaleBridgeTmpFiles before openBridgeDb in writeBridge', () => { + // Ensure cleanup happens before the DB is opened with the new random path. + const cleanIdx = bridgeSource.indexOf('cleanStaleBridgeTmpFiles(groupDir)'); + const openIdx = bridgeSource.indexOf('openBridgeDb(tmpPath)'); + expect(cleanIdx).toBeGreaterThan(-1); + expect(openIdx).toBeGreaterThan(-1); + expect(cleanIdx).toBeLessThan(openIdx); + }); + + it('storage.ts imports randomBytes from node:crypto', () => { + expect(storageSource).toMatch(/import\s*\{[^}]*randomBytes[^}]*\}\s*from\s*'node:crypto'/); + }); + + it('storage.ts uses randomBytes for contracts.json temp path', () => { + expect(storageSource).toMatch(/\.tmp\.\$\{randomBytes\(8\)\.toString\('hex'\)\}/); + }); + + it('storage.ts does not use Date.now() in any temp path', () => { + const tmpDateNow = storageSource.match(/\.tmp\.\$\{Date\.now\(\)\}/g) ?? []; + expect(tmpDateNow.length).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// Behavioural: writeContractRegistry atomic write leaves no tmp files +// --------------------------------------------------------------------------- + +describe('insecure tempfile — behavioural (#1318 U6)', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-u6-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + const sampleRegistry: ContractRegistry = { + version: 1, + generatedAt: '2026-05-06T00:00:00Z', + repoSnapshots: {}, + missingRepos: [], + contracts: [], + crossLinks: [], + }; + + it('writeContractRegistry leaves no .tmp files after completion', async () => { + await writeContractRegistry(tmpDir, sampleRegistry); + + const files = await fsp.readdir(tmpDir); + const tmpFiles = files.filter((f) => f.includes('.tmp.')); + expect(tmpFiles).toEqual([]); + }); + + it('writeContractRegistry writes correct data to final path', async () => { + await writeContractRegistry(tmpDir, sampleRegistry); + + const loaded = await readContractRegistry(tmpDir); + expect(loaded).not.toBeNull(); + expect(loaded!.version).toBe(1); + expect(loaded!.generatedAt).toBe('2026-05-06T00:00:00Z'); + }); + + it('concurrent writes do not collide (randomBytes prevents same-ms race)', async () => { + // Fire two writes simultaneously — with Date.now() these could collide + // if they land in the same millisecond. With randomBytes they can't. + await Promise.all([ + writeContractRegistry(tmpDir, { ...sampleRegistry, generatedAt: 'A' }), + writeContractRegistry(tmpDir, { ...sampleRegistry, generatedAt: 'B' }), + ]); + + const loaded = await readContractRegistry(tmpDir); + expect(loaded).not.toBeNull(); + // One of the two writes wins the rename — we just verify no crash. + expect(['A', 'B']).toContain(loaded!.generatedAt); + }); +}); + +// --------------------------------------------------------------------------- +// Behavioural: writeBridgeMeta atomic write leaves no tmp files +// --------------------------------------------------------------------------- + +describe('insecure tempfile — writeBridgeMeta behavioural (#1318 U6)', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-u6-meta-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + const sampleMeta: BridgeMeta = { + version: 1, + generatedAt: '2026-05-06T00:00:00Z', + missingRepos: ['repo-x'], + }; + + it('writeBridgeMeta leaves no .tmp files after completion', async () => { + await writeBridgeMeta(tmpDir, sampleMeta); + + const files = await fsp.readdir(tmpDir); + const tmpFiles = files.filter((f) => f.includes('.tmp.')); + expect(tmpFiles).toEqual([]); + }); + + it('writeBridgeMeta writes correct data to meta.json', async () => { + await writeBridgeMeta(tmpDir, sampleMeta); + + const loaded = await readBridgeMeta(tmpDir); + expect(loaded.version).toBe(1); + expect(loaded.generatedAt).toBe('2026-05-06T00:00:00Z'); + expect(loaded.missingRepos).toEqual(['repo-x']); + }); + + it('concurrent writeBridgeMeta calls do not collide', async () => { + await Promise.all([ + writeBridgeMeta(tmpDir, { ...sampleMeta, generatedAt: 'A' }), + writeBridgeMeta(tmpDir, { ...sampleMeta, generatedAt: 'B' }), + ]); + + const loaded = await readBridgeMeta(tmpDir); + expect(['A', 'B']).toContain(loaded.generatedAt); + }); +});