From 299a8e721f378b9a297048d730d7672e908c5e2f Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 00:40:11 +0530 Subject: [PATCH 1/8] fix(osv-sync): preserve last good advisory DB on failed refresh --- src/advisory/osv-sync.ts | 198 ++++++++++++++++++++++----------------- tests/osv-sync.test.ts | 23 +++++ 2 files changed, 137 insertions(+), 84 deletions(-) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index 766beb0b8..d3290a866 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -73,6 +73,41 @@ export function getDefaultAdvisoryDbPath(outputPath?: string): string { return path.join(os.homedir(), ".cache", "cve-lite", "advisories.db"); } +/** + * Replaces the advisory database after a successful sync. POSIX rename replaces + * the destination atomically. Windows refuses to rename over an existing file, + * so move the old database aside only after the new one is complete and restore + * it if the second rename fails. + */ +function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { + try { + fs.renameSync(tempPath, dbPath); + return; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "EEXIST" && code !== "EPERM") throw error; + } + + const backupPath = `${dbPath}.backup-${process.pid}-${Date.now()}`; + let movedExisting = false; + try { + fs.renameSync(dbPath, backupPath); + movedExisting = true; + fs.renameSync(tempPath, dbPath); + fs.rmSync(backupPath, { force: true }); + } catch (error) { + if (movedExisting && !fs.existsSync(dbPath) && fs.existsSync(backupPath)) { + try { + fs.renameSync(backupPath, dbPath); + } catch { + // Preserve the original replacement error. The backup remains available + // for recovery if the filesystem also rejected the restore. + } + } + throw error; + } +} + export async function syncOsvAdvisories( options: SyncOsvAdvisoriesOptions = {}, ): Promise { @@ -89,95 +124,99 @@ export async function syncOsvAdvisories( }); fs.mkdirSync(path.dirname(dbPath), { recursive: true }); - if (fs.existsSync(dbPath)) { - fs.rmSync(dbPath, { force: true }); - } + const tempDir = fs.mkdtempSync(path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-`)); + const tempDbPath = path.join(tempDir, path.basename(dbPath)); - const response = await fetchImpl(sourceUrl); - if (!response.ok) { - throw new Error(`OSV dump download failed: ${response.status} ${response.statusText}`); - } + try { + const response = await fetchImpl(sourceUrl); + if (!response.ok) { + throw new Error(`OSV dump download failed: ${response.status} ${response.statusText}`); + } + + const totalBytesHeader = response.headers.get("content-length"); + const totalBytes = totalBytesHeader ? Number(totalBytesHeader) : null; + const zippedBytes = await readResponseBytes(response, (bytesReceived) => { + onProgress?.({ + phase: "download", + bytesReceived, + totalBytes, + message: + totalBytes && totalBytes > 0 + ? `Downloading advisory dump: ${formatBytes(bytesReceived)} / ${formatBytes(totalBytes)}` + : `Downloading advisory dump: ${formatBytes(bytesReceived)}`, + }); + }); - const totalBytesHeader = response.headers.get("content-length"); - const totalBytes = totalBytesHeader ? Number(totalBytesHeader) : null; - const zippedBytes = await readResponseBytes(response, (bytesReceived) => { onProgress?.({ - phase: "download", - bytesReceived, + phase: "download-complete", + bytesReceived: zippedBytes.byteLength, totalBytes, message: totalBytes && totalBytes > 0 - ? `Downloading advisory dump: ${formatBytes(bytesReceived)} / ${formatBytes(totalBytes)}` - : `Downloading advisory dump: ${formatBytes(bytesReceived)}`, + ? `Download complete: ${formatBytes(zippedBytes.byteLength)} / ${formatBytes(totalBytes)}` + : `Download complete: ${formatBytes(zippedBytes.byteLength)}`, }); - }); + await yieldToEventLoop(); - onProgress?.({ - phase: "download-complete", - bytesReceived: zippedBytes.byteLength, - totalBytes, - message: - totalBytes && totalBytes > 0 - ? `Download complete: ${formatBytes(zippedBytes.byteLength)} / ${formatBytes(totalBytes)}` - : `Download complete: ${formatBytes(zippedBytes.byteLength)}`, - }); - await yieldToEventLoop(); - - const archiveEntries = unzipSync(zippedBytes); - const advisoryEntries = Object.entries(archiveEntries).filter(([entryName]) => entryName.endsWith(".json")); - onProgress?.({ - phase: "extract", - totalEntries: Object.keys(archiveEntries).length, - advisoryEntries: advisoryEntries.length, - message: `Archive loaded. Processing ${advisoryEntries.length} advisory ${pluralize(advisoryEntries.length, "record")}`, - }); - await yieldToEventLoop(); - const db = new LocalAdvisoryDatabase(dbPath); - - try { - const parsedVulns: OsvVuln[] = []; + const archiveEntries = unzipSync(zippedBytes); + const advisoryEntries = Object.entries(archiveEntries).filter(([entryName]) => entryName.endsWith(".json")); + onProgress?.({ + phase: "extract", + totalEntries: Object.keys(archiveEntries).length, + advisoryEntries: advisoryEntries.length, + message: `Archive loaded. Processing ${advisoryEntries.length} advisory ${pluralize(advisoryEntries.length, "record")}`, + }); + await yieldToEventLoop(); + const db = new LocalAdvisoryDatabase(tempDbPath); let advisoryCount = 0; - const progressInterval = 250; - for (const [, bytes] of advisoryEntries) { - const text = Buffer.from(bytes).toString("utf8"); - const vuln = JSON.parse(text) as OsvVuln; - if (!vuln.id) { - continue; + try { + const parsedVulns: OsvVuln[] = []; + const progressInterval = 250; + + for (const [, bytes] of advisoryEntries) { + const text = Buffer.from(bytes).toString("utf8"); + const vuln = JSON.parse(text) as OsvVuln; + if (!vuln.id) { + continue; + } + + // OSV's /v1/querybatch endpoint excludes withdrawn advisories at query + // time; mirror that here so offline scans don't surface false positives + // from advisories that have been retracted. + if (vuln.withdrawn) { + continue; + } + + parsedVulns.push(vuln); + advisoryCount += 1; + + if ( + advisoryCount === 1 || + advisoryCount % progressInterval === 0 || + advisoryCount === advisoryEntries.length + ) { + onProgress?.({ + phase: "ingest", + processedAdvisories: advisoryCount, + totalAdvisories: advisoryEntries.length, + message: `Processing advisory records: ${advisoryCount} / ${advisoryEntries.length}`, + }); + await yieldToEventLoop(); + } } - // OSV's /v1/querybatch endpoint excludes withdrawn advisories at query - // time; mirror that here so offline scans don't surface false positives - // from advisories that have been retracted. - if (vuln.withdrawn) { - continue; - } + db.bulkUpsertVulnerabilities(parsedVulns); - parsedVulns.push(vuln); - advisoryCount += 1; - - if ( - advisoryCount === 1 || - advisoryCount % progressInterval === 0 || - advisoryCount === advisoryEntries.length - ) { - onProgress?.({ - phase: "ingest", - processedAdvisories: advisoryCount, - totalAdvisories: advisoryEntries.length, - message: `Processing advisory records: ${advisoryCount} / ${advisoryEntries.length}`, - }); - await yieldToEventLoop(); - } + db.setMetadata({ + lastSyncAt: new Date().toISOString(), + sourceUrl, + }); + } finally { + db.close(); } - db.bulkUpsertVulnerabilities(parsedVulns); - - db.setMetadata({ - lastSyncAt: new Date().toISOString(), - sourceUrl, - }); - + replaceAdvisoryDatabase(tempDbPath, dbPath); onProgress?.({ phase: "complete", advisoryCount, @@ -186,17 +225,8 @@ export async function syncOsvAdvisories( }); return { advisoryCount, dbPath, sourceUrl }; - } catch (error) { - if (fs.existsSync(dbPath)) { - fs.rmSync(dbPath, { force: true }); - } - throw error; } finally { - try { - db.close(); - } catch { - // ignore close errors during sync cleanup - } + fs.rmSync(tempDir, { recursive: true, force: true }); } } diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index fb3c1de4b..3f22ceba8 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -213,6 +213,29 @@ describe("syncOsvAdvisories", () => { ).rejects.toThrow("OSV dump download failed: 502 Bad Gateway"); }); + it("preserves the existing advisory DB when a refresh download fails", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const previousDb = "previous-known-good-db"; + fs.writeFileSync(dbPath, previousDb, "utf8"); + + try { + await expect( + syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => { + throw new Error("simulated network failure"); + }, + }), + ).rejects.toThrow("simulated network failure"); + + expect(fs.readFileSync(dbPath, "utf8")).toBe(previousDb); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } finally { + removeDir(tempDir); + } + }); + it("uses the default user-local advisory DB path when no output path is provided", () => { const advisoryPath = getDefaultAdvisoryDbPath(); From af5a935a1c9a3213f93da1a71a40a6a30ac52c96 Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 01:02:44 +0530 Subject: [PATCH 2/8] refactor(osv-sync): isolate ingestion and harden rollback --- src/advisory/osv-sync.ts | 119 +++++++++++++++++++++++---------------- tests/osv-sync.test.ts | 37 ++++++++++++ 2 files changed, 106 insertions(+), 50 deletions(-) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index d3290a866..09729c80b 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -94,18 +94,77 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { fs.renameSync(dbPath, backupPath); movedExisting = true; fs.renameSync(tempPath, dbPath); - fs.rmSync(backupPath, { force: true }); } catch (error) { - if (movedExisting && !fs.existsSync(dbPath) && fs.existsSync(backupPath)) { - try { - fs.renameSync(backupPath, dbPath); - } catch { - // Preserve the original replacement error. The backup remains available - // for recovery if the filesystem also rejected the restore. - } + if (!movedExisting) throw error; + + try { + fs.renameSync(backupPath, dbPath); + } catch (restoreError) { + const originalMessage = error instanceof Error ? error.message : String(error); + const restoreMessage = restoreError instanceof Error ? restoreError.message : String(restoreError); + throw new Error( + `Failed to install refreshed advisory database: ${originalMessage}. ` + + `The previous database could not be restored and remains at ${backupPath}: ${restoreMessage}`, + { cause: restoreError }, + ); } throw error; } + + try { + fs.rmSync(backupPath, { force: true }); + } catch { + // The new database is installed. A leftover backup is harmless. + } +} + +type AdvisoryArchiveEntry = readonly [name: string, bytes: Uint8Array]; + +async function ingestAdvisories( + db: LocalAdvisoryDatabase, + advisoryEntries: readonly AdvisoryArchiveEntry[], + sourceUrl: string, + onProgress?: (event: SyncProgressEvent) => void, +): Promise { + const parsedVulns: OsvVuln[] = []; + let advisoryCount = 0; + const progressInterval = 250; + + for (const [, bytes] of advisoryEntries) { + const text = Buffer.from(bytes).toString("utf8"); + const vuln = JSON.parse(text) as OsvVuln; + if (!vuln.id) continue; + + // OSV's /v1/querybatch endpoint excludes withdrawn advisories at query + // time; mirror that here so offline scans don't surface false positives + // from advisories that have been retracted. + if (vuln.withdrawn) continue; + + parsedVulns.push(vuln); + advisoryCount += 1; + + if ( + advisoryCount === 1 || + advisoryCount % progressInterval === 0 || + advisoryCount === advisoryEntries.length + ) { + onProgress?.({ + phase: "ingest", + processedAdvisories: advisoryCount, + totalAdvisories: advisoryEntries.length, + message: `Processing advisory records: ${advisoryCount} / ${advisoryEntries.length}`, + }); + await yieldToEventLoop(); + } + } + + db.bulkUpsertVulnerabilities(parsedVulns); + db.setMetadata({ + lastSyncAt: new Date().toISOString(), + sourceUrl, + }); + + return advisoryCount; } export async function syncOsvAdvisories( @@ -168,50 +227,10 @@ export async function syncOsvAdvisories( }); await yieldToEventLoop(); const db = new LocalAdvisoryDatabase(tempDbPath); - let advisoryCount = 0; + let advisoryCount: number; try { - const parsedVulns: OsvVuln[] = []; - const progressInterval = 250; - - for (const [, bytes] of advisoryEntries) { - const text = Buffer.from(bytes).toString("utf8"); - const vuln = JSON.parse(text) as OsvVuln; - if (!vuln.id) { - continue; - } - - // OSV's /v1/querybatch endpoint excludes withdrawn advisories at query - // time; mirror that here so offline scans don't surface false positives - // from advisories that have been retracted. - if (vuln.withdrawn) { - continue; - } - - parsedVulns.push(vuln); - advisoryCount += 1; - - if ( - advisoryCount === 1 || - advisoryCount % progressInterval === 0 || - advisoryCount === advisoryEntries.length - ) { - onProgress?.({ - phase: "ingest", - processedAdvisories: advisoryCount, - totalAdvisories: advisoryEntries.length, - message: `Processing advisory records: ${advisoryCount} / ${advisoryEntries.length}`, - }); - await yieldToEventLoop(); - } - } - - db.bulkUpsertVulnerabilities(parsedVulns); - - db.setMetadata({ - lastSyncAt: new Date().toISOString(), - sourceUrl, - }); + advisoryCount = await ingestAdvisories(db, advisoryEntries, sourceUrl, onProgress); } finally { db.close(); } diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index 3f22ceba8..c092f19e4 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -24,6 +24,10 @@ function createZipPayload(files: Record): Uint8Array { ); } +function createRawZipPayload(files: Record): Uint8Array { + return zipSync(files); +} + describe("syncOsvAdvisories", () => { it("downloads the OSV npm dump and populates the local advisory DB", async () => { const tempDir = createTempDir(); @@ -236,6 +240,39 @@ describe("syncOsvAdvisories", () => { } }); + it("preserves the existing advisory DB when ingestion fails", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const previousDb = "previous-known-good-db"; + fs.writeFileSync(dbPath, previousDb, "utf8"); + const zipPayload = createRawZipPayload({ + "invalid.json": Buffer.from("{not valid json", "utf8"), + }); + + try { + await expect( + syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => + ({ + ok: true, + headers: new Headers(), + arrayBuffer: async () => + zipPayload.buffer.slice( + zipPayload.byteOffset, + zipPayload.byteOffset + zipPayload.byteLength, + ), + }) as Response, + }), + ).rejects.toThrow("Expected property name"); + + expect(fs.readFileSync(dbPath, "utf8")).toBe(previousDb); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } finally { + removeDir(tempDir); + } + }); + it("uses the default user-local advisory DB path when no output path is provided", () => { const advisoryPath = getDefaultAdvisoryDbPath(); From 514779b65f677fd2f43dc818ebbfbdd182105ea3 Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 01:09:13 +0530 Subject: [PATCH 3/8] fix(osv-sync): reject empty advisory refreshes --- src/advisory/osv-sync.ts | 7 +++++++ tests/osv-sync.test.ts | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index 09729c80b..a9098eb11 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -235,6 +235,13 @@ export async function syncOsvAdvisories( db.close(); } + if (advisoryCount === 0) { + throw new Error( + `OSV dump from ${sourceUrl} contained no usable advisory records. ` + + `The existing advisory database was not changed. Check the source URL, then run "cve-lite advisories sync" again.`, + ); + } + replaceAdvisoryDatabase(tempDbPath, dbPath); onProgress?.({ phase: "complete", diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index c092f19e4..f8e396b79 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -273,6 +273,40 @@ describe("syncOsvAdvisories", () => { } }); + it("rejects a refresh with no usable advisories and preserves the existing DB", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const previousDb = "previous-known-good-db"; + fs.writeFileSync(dbPath, previousDb, "utf8"); + const zipPayload = createZipPayload({ "README.txt": "no advisory records" }); + + try { + await expect( + syncOsvAdvisories({ + outputPath: dbPath, + sourceUrl: "https://mirror.example/empty.zip", + fetchImpl: async () => + ({ + ok: true, + headers: new Headers(), + arrayBuffer: async () => + zipPayload.buffer.slice( + zipPayload.byteOffset, + zipPayload.byteOffset + zipPayload.byteLength, + ), + }) as Response, + }), + ).rejects.toThrow( + "OSV dump from https://mirror.example/empty.zip contained no usable advisory records. The existing advisory database was not changed.", + ); + + expect(fs.readFileSync(dbPath, "utf8")).toBe(previousDb); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } finally { + removeDir(tempDir); + } + }); + it("uses the default user-local advisory DB path when no output path is provided", () => { const advisoryPath = getDefaultAdvisoryDbPath(); From 03ce7571f5cfce4100385641429958db24b0322b Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 01:19:21 +0530 Subject: [PATCH 4/8] fix(osv-sync): preserve refresh errors during cleanup --- src/advisory/osv-sync.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index a9098eb11..3b9ba1cba 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -185,6 +185,8 @@ export async function syncOsvAdvisories( fs.mkdirSync(path.dirname(dbPath), { recursive: true }); const tempDir = fs.mkdtempSync(path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-`)); const tempDbPath = path.join(tempDir, path.basename(dbPath)); + let databaseInstalled = false; + let refreshError: unknown; try { const response = await fetchImpl(sourceUrl); @@ -243,6 +245,7 @@ export async function syncOsvAdvisories( } replaceAdvisoryDatabase(tempDbPath, dbPath); + databaseInstalled = true; onProgress?.({ phase: "complete", advisoryCount, @@ -251,8 +254,24 @@ export async function syncOsvAdvisories( }); return { advisoryCount, dbPath, sourceUrl }; + } catch (error) { + refreshError = error; + throw error; } finally { - fs.rmSync(tempDir, { recursive: true, force: true }); + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch (cleanupError) { + const cleanupMessage = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); + if (databaseInstalled) { + console.warn(`Could not remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}`); + } else { + const refreshMessage = refreshError instanceof Error ? refreshError.message : String(refreshError); + throw new Error( + `${refreshMessage}. Also failed to remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}`, + { cause: cleanupError }, + ); + } + } } } From fce291e009e92a8b25b49708c60b09ac5c272f95 Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 01:29:59 +0530 Subject: [PATCH 5/8] fix(osv-sync): preserve refresh causes during cleanup --- src/advisory/osv-sync.ts | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index 3b9ba1cba..dbe25ce42 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -187,6 +187,8 @@ export async function syncOsvAdvisories( const tempDbPath = path.join(tempDir, path.basename(dbPath)); let databaseInstalled = false; let refreshError: unknown; + let cleanupError: unknown; + let syncResult: SyncOsvAdvisoriesResult | undefined; try { const response = await fetchImpl(sourceUrl); @@ -253,26 +255,36 @@ export async function syncOsvAdvisories( message: `Sync complete. Stored ${advisoryCount} advisory ${pluralize(advisoryCount, "record")} in ${dbPath}`, }); - return { advisoryCount, dbPath, sourceUrl }; + syncResult = { advisoryCount, dbPath, sourceUrl }; } catch (error) { refreshError = error; - throw error; } finally { try { fs.rmSync(tempDir, { recursive: true, force: true }); - } catch (cleanupError) { - const cleanupMessage = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); - if (databaseInstalled) { - console.warn(`Could not remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}`); - } else { - const refreshMessage = refreshError instanceof Error ? refreshError.message : String(refreshError); - throw new Error( - `${refreshMessage}. Also failed to remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}`, - { cause: cleanupError }, - ); - } + } catch (error) { + cleanupError = error; } } + + if (cleanupError) { + const cleanupMessage = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); + const cleanupRecovery = `Resolve the filesystem error and remove ${tempDir} manually.`; + if (databaseInstalled) { + console.warn(`Could not remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}. ${cleanupRecovery}`); + } else if (refreshError) { + const refreshMessage = refreshError instanceof Error ? refreshError.message : String(refreshError); + throw new Error( + `${refreshMessage}. Also failed to remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}. ${cleanupRecovery}`, + { cause: refreshError }, + ); + } + } + + if (refreshError) { + throw refreshError; + } + + return syncResult!; } async function readResponseBytes( From 8f5599eed18b19682f16a86a1d0914380ce9eeac Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 18:45:21 +0530 Subject: [PATCH 6/8] fix: address advisory sync review feedback --- src/advisory/osv-sync.ts | 220 +++++++++++++++++------------- src/cli/advisories-sync.ts | 10 +- tests/cli-integration.test.ts | 4 +- tests/cli/advisories-sync.test.ts | 103 ++++++++++++++ tests/osv-sync.test.ts | 125 ++++++++++++++++- 5 files changed, 366 insertions(+), 96 deletions(-) create mode 100644 tests/cli/advisories-sync.test.ts diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index dbe25ce42..5c2d3b164 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -20,8 +20,20 @@ export type SyncOsvAdvisoriesResult = { advisoryCount: number; dbPath: string; sourceUrl: string; + warnings?: string[]; }; +export class AdvisorySyncError extends Error { + constructor( + message: string, + readonly databaseState: "unchanged" | "replaced" | "recovery-required", + cause: unknown, + ) { + super(message, { cause }); + this.name = "AdvisorySyncError"; + } +} + export type SyncProgressEvent = | { phase: "init"; @@ -74,10 +86,10 @@ export function getDefaultAdvisoryDbPath(outputPath?: string): string { } /** - * Replaces the advisory database after a successful sync. POSIX rename replaces - * the destination atomically. Windows refuses to rename over an existing file, - * so move the old database aside only after the new one is complete and restore - * it if the second rename fails. + * Replaces the advisory database after a successful sync. Node's rename replaces + * existing files on Windows too, but an open or locked target can cause EPERM. + * If replacement is refused, move the old database aside only after the new one + * is complete and restore it if the second rename fails. */ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { try { @@ -102,10 +114,11 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { } catch (restoreError) { const originalMessage = error instanceof Error ? error.message : String(error); const restoreMessage = restoreError instanceof Error ? restoreError.message : String(restoreError); - throw new Error( + throw new AdvisorySyncError( `Failed to install refreshed advisory database: ${originalMessage}. ` + `The previous database could not be restored and remains at ${backupPath}: ${restoreMessage}`, - { cause: restoreError }, + "recovery-required", + restoreError, ); } throw error; @@ -167,85 +180,125 @@ async function ingestAdvisories( return advisoryCount; } -export async function syncOsvAdvisories( - options: SyncOsvAdvisoriesOptions = {}, -): Promise { - const fetchImpl = options.fetchImpl ?? fetch; - const onProgress = options.onProgress; - const sourceUrl = options.sourceUrl ?? DEFAULT_OSV_NPM_DUMP_URL; - const dbPath = getDefaultAdvisoryDbPath(options.outputPath); - +async function downloadAdvisoryArchive( + sourceUrl: string, + dbPath: string, + fetchImpl: FetchLike, + onProgress?: SyncOsvAdvisoriesOptions["onProgress"], +): Promise { onProgress?.({ phase: "init", sourceUrl, dbPath, message: `Sync initiated. Downloading advisory dump from ${sourceUrl}`, }); + const response = await fetchImpl(sourceUrl); + if (!response.ok) { + throw new Error(`OSV dump download failed: ${response.status} ${response.statusText}`); + } - fs.mkdirSync(path.dirname(dbPath), { recursive: true }); - const tempDir = fs.mkdtempSync(path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-`)); - const tempDbPath = path.join(tempDir, path.basename(dbPath)); - let databaseInstalled = false; - let refreshError: unknown; - let cleanupError: unknown; - let syncResult: SyncOsvAdvisoriesResult | undefined; - - try { - const response = await fetchImpl(sourceUrl); - if (!response.ok) { - throw new Error(`OSV dump download failed: ${response.status} ${response.statusText}`); - } - - const totalBytesHeader = response.headers.get("content-length"); - const totalBytes = totalBytesHeader ? Number(totalBytesHeader) : null; - const zippedBytes = await readResponseBytes(response, (bytesReceived) => { - onProgress?.({ - phase: "download", - bytesReceived, - totalBytes, - message: - totalBytes && totalBytes > 0 - ? `Downloading advisory dump: ${formatBytes(bytesReceived)} / ${formatBytes(totalBytes)}` - : `Downloading advisory dump: ${formatBytes(bytesReceived)}`, - }); - }); - + const totalBytesHeader = response.headers.get("content-length"); + const totalBytes = totalBytesHeader ? Number(totalBytesHeader) : null; + const zippedBytes = await readResponseBytes(response, (bytesReceived) => { onProgress?.({ - phase: "download-complete", - bytesReceived: zippedBytes.byteLength, + phase: "download", + bytesReceived, totalBytes, message: totalBytes && totalBytes > 0 - ? `Download complete: ${formatBytes(zippedBytes.byteLength)} / ${formatBytes(totalBytes)}` - : `Download complete: ${formatBytes(zippedBytes.byteLength)}`, + ? `Downloading advisory dump: ${formatBytes(bytesReceived)} / ${formatBytes(totalBytes)}` + : `Downloading advisory dump: ${formatBytes(bytesReceived)}`, }); - await yieldToEventLoop(); + }); + onProgress?.({ + phase: "download-complete", + bytesReceived: zippedBytes.byteLength, + totalBytes, + message: + totalBytes && totalBytes > 0 + ? `Download complete: ${formatBytes(zippedBytes.byteLength)} / ${formatBytes(totalBytes)}` + : `Download complete: ${formatBytes(zippedBytes.byteLength)}`, + }); + await yieldToEventLoop(); + return zippedBytes; +} - const archiveEntries = unzipSync(zippedBytes); - const advisoryEntries = Object.entries(archiveEntries).filter(([entryName]) => entryName.endsWith(".json")); - onProgress?.({ - phase: "extract", - totalEntries: Object.keys(archiveEntries).length, - advisoryEntries: advisoryEntries.length, - message: `Archive loaded. Processing ${advisoryEntries.length} advisory ${pluralize(advisoryEntries.length, "record")}`, - }); - await yieldToEventLoop(); - const db = new LocalAdvisoryDatabase(tempDbPath); - let advisoryCount: number; +async function buildAdvisoryDatabase( + zippedBytes: Uint8Array, + tempDbPath: string, + sourceUrl: string, + onProgress?: SyncOsvAdvisoriesOptions["onProgress"], +): Promise { + const archiveEntries = unzipSync(zippedBytes); + const advisoryEntries = Object.entries(archiveEntries).filter(([entryName]) => entryName.endsWith(".json")); + onProgress?.({ + phase: "extract", + totalEntries: Object.keys(archiveEntries).length, + advisoryEntries: advisoryEntries.length, + message: `Archive loaded. Processing ${advisoryEntries.length} advisory ${pluralize(advisoryEntries.length, "record")}`, + }); + await yieldToEventLoop(); + const db = new LocalAdvisoryDatabase(tempDbPath); + let advisoryCount: number; + try { + advisoryCount = await ingestAdvisories(db, advisoryEntries, sourceUrl, onProgress); + } finally { + db.close(); + } + if (advisoryCount === 0) { + throw new Error( + `OSV dump from ${sourceUrl} contained no usable advisory records. ` + + `Check the source URL, then run "cve-lite advisories sync" again.`, + ); + } + return advisoryCount; +} - try { - advisoryCount = await ingestAdvisories(db, advisoryEntries, sourceUrl, onProgress); - } finally { - db.close(); - } +/** Removes staged files, returning a warning for the caller to report. */ +function cleanupTempDirectory(tempDir: string): string | undefined { + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return `Could not remove temporary advisory sync directory ${tempDir}: ${message}. ` + + `Resolve the filesystem error and remove ${tempDir} manually.`; + } +} - if (advisoryCount === 0) { - throw new Error( - `OSV dump from ${sourceUrl} contained no usable advisory records. ` - + `The existing advisory database was not changed. Check the source URL, then run "cve-lite advisories sync" again.`, - ); - } +function buildSyncError( + refreshError: unknown, + databaseInstalled: boolean, + cleanupWarning?: string, +): AdvisorySyncError { + const message = refreshError instanceof Error ? refreshError.message : String(refreshError); + const databaseState = refreshError instanceof AdvisorySyncError + ? refreshError.databaseState + : databaseInstalled ? "replaced" : "unchanged"; + return new AdvisorySyncError( + cleanupWarning ? `${message}. ${cleanupWarning}` : message, + databaseState, + refreshError, + ); +} + +export async function syncOsvAdvisories( + options: SyncOsvAdvisoriesOptions = {}, +): Promise { + const onProgress = options.onProgress; + const sourceUrl = options.sourceUrl ?? DEFAULT_OSV_NPM_DUMP_URL; + const dbPath = getDefaultAdvisoryDbPath(options.outputPath); + let tempDir: string | undefined; + let databaseInstalled = false; + let refreshError: unknown; + let cleanupWarning: string | undefined; + let syncResult: SyncOsvAdvisoriesResult | undefined; + try { + fs.mkdirSync(path.dirname(dbPath), { recursive: true }); + tempDir = fs.mkdtempSync(path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-`)); + const tempDbPath = path.join(tempDir, path.basename(dbPath)); + const zippedBytes = await downloadAdvisoryArchive(sourceUrl, dbPath, options.fetchImpl ?? fetch, onProgress); + const advisoryCount = await buildAdvisoryDatabase(zippedBytes, tempDbPath, sourceUrl, onProgress); replaceAdvisoryDatabase(tempDbPath, dbPath); databaseInstalled = true; onProgress?.({ @@ -254,37 +307,18 @@ export async function syncOsvAdvisories( dbPath, message: `Sync complete. Stored ${advisoryCount} advisory ${pluralize(advisoryCount, "record")} in ${dbPath}`, }); - syncResult = { advisoryCount, dbPath, sourceUrl }; } catch (error) { refreshError = error; } finally { - try { - fs.rmSync(tempDir, { recursive: true, force: true }); - } catch (error) { - cleanupError = error; - } - } - - if (cleanupError) { - const cleanupMessage = cleanupError instanceof Error ? cleanupError.message : String(cleanupError); - const cleanupRecovery = `Resolve the filesystem error and remove ${tempDir} manually.`; - if (databaseInstalled) { - console.warn(`Could not remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}. ${cleanupRecovery}`); - } else if (refreshError) { - const refreshMessage = refreshError instanceof Error ? refreshError.message : String(refreshError); - throw new Error( - `${refreshMessage}. Also failed to remove temporary advisory sync directory ${tempDir}: ${cleanupMessage}. ${cleanupRecovery}`, - { cause: refreshError }, - ); - } + if (tempDir !== undefined) cleanupWarning = cleanupTempDirectory(tempDir); } - if (refreshError) { - throw refreshError; + if (syncResult === undefined) { + throw buildSyncError(refreshError, databaseInstalled, cleanupWarning); } - - return syncResult!; + if (cleanupWarning) syncResult.warnings = [cleanupWarning]; + return syncResult; } async function readResponseBytes( diff --git a/src/cli/advisories-sync.ts b/src/cli/advisories-sync.ts index 4d40d0311..17f0c8840 100644 --- a/src/cli/advisories-sync.ts +++ b/src/cli/advisories-sync.ts @@ -1,5 +1,6 @@ import process from "node:process"; -import { syncOsvAdvisories } from "../advisory/osv-sync.js"; +import { AdvisorySyncError, syncOsvAdvisories } from "../advisory/osv-sync.js"; +import { logWarn } from "../output/formatters.js"; import { createSpinner } from "../output/spinner.js"; import { chalk } from "../utils/chalk.js"; import { pluralize } from "../utils/string.js"; @@ -26,7 +27,14 @@ export async function runAdvisoriesSync({ options, fetchImpl }: AdvisoriesSyncPa } spinner.update(event.message); }, + }).catch((error: unknown) => { + spinner.stop(); + if (error instanceof AdvisorySyncError && error.databaseState === "unchanged") { + throw new Error(`${error.message} The existing advisory database was not changed.`, { cause: error }); + } + throw error; }); + for (const warning of result.warnings ?? []) logWarn(warning, options); if (usePlainProgressLogs) { console.log(`Advisory sync complete (${result.advisoryCount} records)`); } else { diff --git a/tests/cli-integration.test.ts b/tests/cli-integration.test.ts index aa0fff62e..499198154 100644 --- a/tests/cli-integration.test.ts +++ b/tests/cli-integration.test.ts @@ -4,6 +4,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { PassThrough } from "node:stream"; +import type { writeHtmlReport } from "../src/output/html-reporter.js"; import type { Finding, PackageRef, ScanInput } from "../src/types.js"; import { EXIT_ERROR } from "../src/types.js"; import { stripAnsi } from "../src/utils/chalk.js"; @@ -34,7 +35,7 @@ const buildSuggestedFixCommandPlanMock = jest.fn(); const spawnMock = jest.fn(); const buildReportDataMock = jest.fn(); const runOverrideAuditMock = jest.fn(() => Promise.resolve({ findings: [], skipped: [] })); -const writeHtmlReportMock = jest.fn(); +const writeHtmlReportMock = jest.fn(); const installSkillMock = jest.fn(); const writeSarifReportMock = jest.fn(() => "cve-lite-scan-test.sarif"); const deriveLockfileUriMock = jest.fn(() => "package-lock.json"); @@ -82,6 +83,7 @@ jest.unstable_mockModule("../src/scanner.js", () => ({ jest.unstable_mockModule("../src/advisory/osv-sync.js", () => ({ syncOsvAdvisories: syncOsvAdvisoriesMock, + AdvisorySyncError: class extends Error {}, })); jest.unstable_mockModule("../src/output/formatters.js", () => ({ diff --git a/tests/cli/advisories-sync.test.ts b/tests/cli/advisories-sync.test.ts new file mode 100644 index 000000000..94bd21be5 --- /dev/null +++ b/tests/cli/advisories-sync.test.ts @@ -0,0 +1,103 @@ +import { jest } from "@jest/globals"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { zipSync } from "fflate"; +import { runAdvisoriesSync } from "../../src/cli/advisories-sync.js"; +import { stripAnsi } from "../../src/utils/chalk.js"; +import { isSslCertificateError } from "../../src/utils/network.js"; +import type { ParsedOptions } from "../../src/types.js"; + +describe("advisory sync CLI reporting", () => { + let tempDir: string; + let dbPath: string; + let options: ParsedOptions; + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "cve-lite-sync-cli-")); + dbPath = path.join(tempDir, "advisories.db"); + options = { output: dbPath, failOn: "high", batchSize: "100", incompletePolicy: "warn" }; + fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); + jest.spyOn(console, "log").mockImplementation(() => {}); + jest.spyOn(console, "warn").mockImplementation(() => {}); + jest.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + it.each(["network", "http", "extraction", "ingestion", "empty"])( + "reports preservation after a %s failure", + async failure => { + const payload = failure === "ingestion" + ? zipSync({ "invalid.json": Buffer.from("not JSON") }) + : zipSync({ "README.txt": Buffer.from("no advisories") }); + await expect(runAdvisoriesSync({ + options, + fetchImpl: async () => { + if (failure === "network") throw new Error("connection refused"); + if (failure === "http") return new Response("unavailable", { status: 500 }); + if (failure === "extraction") return new Response("not a zip"); + return new Response(Buffer.from(payload)); + }, + })).rejects.toThrow(/The existing advisory database was not changed\.$/); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + }, + ); + + it("preserves certificate diagnostics through the CLI error wrapper", async () => { + const tlsError = Object.assign(new Error("certificate verification failed"), { + code: "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + }); + let caught: unknown; + try { + await runAdvisoriesSync({ options, fetchImpl: async () => { throw tlsError; } }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect(isSslCertificateError(caught)).toBe(true); + }); + + it.each([false, true])("keeps cleanup warnings in CLI output with json=%s", async json => { + const rmSync = fs.rmSync; + jest.spyOn(fs, "rmSync").mockImplementation((target, rmOptions) => { + if (String(target).includes(".advisories.db.tmp-")) throw new Error("cleanup denied"); + rmSync(target, rmOptions); + }); + const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify({ id: "OSV-1" })) }); + await runAdvisoriesSync({ + options: { ...options, json }, + fetchImpl: async () => new Response(Buffer.from(payload)), + }); + const messages = jest.mocked(console.log).mock.calls.map(args => stripAnsi(args.join(" "))); + expect(messages.some(message => message.includes("cleanup denied"))).toBe(!json); + expect(messages.some(message => message.includes("Advisory sync complete (1 records)"))).toBe(true); + expect(console.warn).not.toHaveBeenCalled(); + expect(console.error).not.toHaveBeenCalled(); + }); + + it("does not claim preservation when installation and restoration both fail", async () => { + const renameSync = fs.renameSync; + let renameCount = 0; + jest.spyOn(fs, "renameSync").mockImplementation((from, to) => { + renameCount += 1; + if (renameCount === 1) throw Object.assign(new Error("target locked"), { code: "EPERM" }); + if (renameCount >= 3) throw new Error("install or restore denied"); + renameSync(from, to); + }); + const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify({ id: "OSV-1" })) }); + let caught: unknown; + try { + await runAdvisoriesSync({ options, fetchImpl: async () => new Response(Buffer.from(payload)) }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toContain("could not be restored and remains at"); + expect((caught as Error).message).not.toContain("database was not changed"); + }); +}); diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index f8e396b79..73697abab 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -1,3 +1,4 @@ +import { jest } from "@jest/globals"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -297,7 +298,7 @@ describe("syncOsvAdvisories", () => { }) as Response, }), ).rejects.toThrow( - "OSV dump from https://mirror.example/empty.zip contained no usable advisory records. The existing advisory database was not changed.", + "OSV dump from https://mirror.example/empty.zip contained no usable advisory records. Check the source URL,", ); expect(fs.readFileSync(dbPath, "utf8")).toBe(previousDb); @@ -307,6 +308,128 @@ describe("syncOsvAdvisories", () => { } }); + it.each([undefined, null, 0, ""])("rejects a falsy download failure (%s) instead of returning success", async failure => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); + try { + await expect(syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => { throw failure; }, + })).rejects.toMatchObject({ databaseState: "unchanged", cause: failure }); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } finally { + removeDir(tempDir); + } + }); + + it("returns cleanup warnings after installation without printing or failing the sync", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const rmSync = fs.rmSync; + const cleanupSpy = jest.spyOn(fs, "rmSync").mockImplementation((target, options) => { + if (String(target).includes(".advisories.db.tmp-")) throw new Error("cleanup denied"); + rmSync(target, options); + }); + const warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + try { + const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + const result = await syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(zipPayload)), + }); + expect(result.advisoryCount).toBe(1); + expect(result.warnings).toEqual([expect.stringMatching(/cleanup denied.*remove .* manually/)]); + expect(warnSpy).not.toHaveBeenCalled(); + const db = new LocalAdvisoryDatabase(dbPath, { readonly: true }); + try { + expect(db.getVulnerability("OSV-1")).toMatchObject({ id: "OSV-1" }); + } finally { + db.close(); + } + } finally { + cleanupSpy.mockRestore(); + warnSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it("retains the refresh cause and recovery guidance when temporary cleanup also fails", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); + const refreshError = new Error("certificate verification failed"); + const cleanupSpy = jest.spyOn(fs, "rmSync").mockImplementation(() => { throw new Error("cleanup denied"); }); + try { + await expect(syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => { throw refreshError; }, + })).rejects.toMatchObject({ + databaseState: "unchanged", + cause: refreshError, + message: expect.stringMatching(/certificate verification failed.*cleanup denied.*remove .* manually/), + }); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + } finally { + cleanupSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it.each([false, true])("reports the database state when installation fails and restoration fails=%s", async restoreFails => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); + const renameSync = fs.renameSync; + let renameCount = 0; + const renameSpy = jest.spyOn(fs, "renameSync").mockImplementation((from, to) => { + renameCount += 1; + if (renameCount === 1) throw Object.assign(new Error("target locked"), { code: "EPERM" }); + if (renameCount === 3) throw new Error("install denied"); + if (renameCount === 4 && restoreFails) throw new Error("restore denied"); + renameSync(from, to); + }); + try { + const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + await expect(syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(zipPayload)), + })).rejects.toMatchObject({ + databaseState: restoreFails ? "recovery-required" : "unchanged", + message: restoreFails + ? expect.stringMatching(/install denied.*remains at .*backup-.*restore denied/) + : "install denied", + }); + if (restoreFails) { + const backup = fs.readdirSync(tempDir).find(name => name.includes(".backup-")); + expect(backup).toBeDefined(); + expect(fs.readFileSync(path.join(tempDir, backup!), "utf8")).toBe("previous-known-good-db"); + } else { + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } + } finally { + renameSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it("reports replacement if the completion callback throws after installing the database", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + try { + const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + await expect(syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(zipPayload)), + onProgress: event => { if (event.phase === "complete") throw new Error("callback failed"); }, + })).rejects.toMatchObject({ databaseState: "replaced", message: "callback failed" }); + } finally { + removeDir(tempDir); + } + }); + it("uses the default user-local advisory DB path when no output path is provided", () => { const advisoryPath = getDefaultAdvisoryDbPath(); From c47f7a3c8d4081be2d2ad0b81c96d35b648083d8 Mon Sep 17 00:00:00 2001 From: vian Date: Tue, 3 Nov 2026 20:16:17 +0530 Subject: [PATCH 7/8] fix: address advisory refresh edge-case review findings --- src/advisory/osv-sync.ts | 21 ++++++---- src/cli/advisories-sync.ts | 11 +++-- tests/cli-integration.test.ts | 1 + tests/cli/advisories-sync.test.ts | 43 +++++++++++++++++++- tests/osv-sync.test.ts | 67 ++++++++++++++++++++++++++++--- 5 files changed, 125 insertions(+), 18 deletions(-) diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index 5c2d3b164..a653897b4 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -91,7 +91,7 @@ export function getDefaultAdvisoryDbPath(outputPath?: string): string { * If replacement is refused, move the old database aside only after the new one * is complete and restore it if the second rename fails. */ -function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { +function replaceAdvisoryDatabase(tempPath: string, dbPath: string): string | undefined { try { fs.renameSync(tempPath, dbPath); return; @@ -116,7 +116,9 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { const restoreMessage = restoreError instanceof Error ? restoreError.message : String(restoreError); throw new AdvisorySyncError( `Failed to install refreshed advisory database: ${originalMessage}. ` - + `The previous database could not be restored and remains at ${backupPath}: ${restoreMessage}`, + + `The previous database could not be restored and remains at ${backupPath}: ${restoreMessage}. ` + + `Close any program using ${dbPath}, resolve the filesystem error, ` + + `then rename ${backupPath} to ${dbPath} to restore the previous database.`, "recovery-required", restoreError, ); @@ -126,8 +128,10 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): void { try { fs.rmSync(backupPath, { force: true }); - } catch { - // The new database is installed. A leftover backup is harmless. + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return `Could not remove advisory database backup ${backupPath}: ${message}. ` + + `The refreshed database was installed. Resolve the filesystem error and remove ${backupPath} manually.`; } } @@ -147,6 +151,7 @@ async function ingestAdvisories( const text = Buffer.from(bytes).toString("utf8"); const vuln = JSON.parse(text) as OsvVuln; if (!vuln.id) continue; + if (!Array.isArray(vuln.affected) || vuln.affected.length === 0) continue; // OSV's /v1/querybatch endpoint excludes withdrawn advisories at query // time; mirror that here so offline scans don't surface false positives @@ -291,6 +296,7 @@ export async function syncOsvAdvisories( let databaseInstalled = false; let refreshError: unknown; let cleanupWarning: string | undefined; + let replacementWarning: string | undefined; let syncResult: SyncOsvAdvisoriesResult | undefined; try { @@ -299,7 +305,7 @@ export async function syncOsvAdvisories( const tempDbPath = path.join(tempDir, path.basename(dbPath)); const zippedBytes = await downloadAdvisoryArchive(sourceUrl, dbPath, options.fetchImpl ?? fetch, onProgress); const advisoryCount = await buildAdvisoryDatabase(zippedBytes, tempDbPath, sourceUrl, onProgress); - replaceAdvisoryDatabase(tempDbPath, dbPath); + replacementWarning = replaceAdvisoryDatabase(tempDbPath, dbPath); databaseInstalled = true; onProgress?.({ phase: "complete", @@ -314,10 +320,11 @@ export async function syncOsvAdvisories( if (tempDir !== undefined) cleanupWarning = cleanupTempDirectory(tempDir); } + const warnings = [replacementWarning, cleanupWarning].filter((warning): warning is string => warning !== undefined); if (syncResult === undefined) { - throw buildSyncError(refreshError, databaseInstalled, cleanupWarning); + throw buildSyncError(refreshError, databaseInstalled, warnings.join(" ") || undefined); } - if (cleanupWarning) syncResult.warnings = [cleanupWarning]; + if (warnings.length > 0) syncResult.warnings = warnings; return syncResult; } diff --git a/src/cli/advisories-sync.ts b/src/cli/advisories-sync.ts index 17f0c8840..c9290858b 100644 --- a/src/cli/advisories-sync.ts +++ b/src/cli/advisories-sync.ts @@ -1,5 +1,6 @@ import process from "node:process"; -import { AdvisorySyncError, syncOsvAdvisories } from "../advisory/osv-sync.js"; +import fs from "node:fs"; +import { AdvisorySyncError, getDefaultAdvisoryDbPath, syncOsvAdvisories } from "../advisory/osv-sync.js"; import { logWarn } from "../output/formatters.js"; import { createSpinner } from "../output/spinner.js"; import { chalk } from "../utils/chalk.js"; @@ -12,6 +13,7 @@ export interface AdvisoriesSyncParams { } export async function runAdvisoriesSync({ options, fetchImpl }: AdvisoriesSyncParams): Promise { + const databaseExisted = fs.existsSync(getDefaultAdvisoryDbPath(options.output)); const spinner = createSpinner("Preparing advisory sync...", options); const usePlainProgressLogs = !process.stdout.isTTY || !!options.json; const result = await syncOsvAdvisories({ @@ -30,11 +32,14 @@ export async function runAdvisoriesSync({ options, fetchImpl }: AdvisoriesSyncPa }).catch((error: unknown) => { spinner.stop(); if (error instanceof AdvisorySyncError && error.databaseState === "unchanged") { - throw new Error(`${error.message} The existing advisory database was not changed.`, { cause: error }); + const stateMessage = databaseExisted + ? "The existing advisory database was not changed." + : "No advisory database was created."; + throw new Error(`${error.message} ${stateMessage}`, { cause: error }); } throw error; }); - for (const warning of result.warnings ?? []) logWarn(warning, options); + for (const warning of result.warnings ?? []) logWarn(warning, { json: options.json }); if (usePlainProgressLogs) { console.log(`Advisory sync complete (${result.advisoryCount} records)`); } else { diff --git a/tests/cli-integration.test.ts b/tests/cli-integration.test.ts index 499198154..21b3fe989 100644 --- a/tests/cli-integration.test.ts +++ b/tests/cli-integration.test.ts @@ -83,6 +83,7 @@ jest.unstable_mockModule("../src/scanner.js", () => ({ jest.unstable_mockModule("../src/advisory/osv-sync.js", () => ({ syncOsvAdvisories: syncOsvAdvisoriesMock, + getDefaultAdvisoryDbPath: (outputPath?: string) => outputPath ?? "/tmp/default-advisories.db", AdvisorySyncError: class extends Error {}, })); diff --git a/tests/cli/advisories-sync.test.ts b/tests/cli/advisories-sync.test.ts index 94bd21be5..fd1280a62 100644 --- a/tests/cli/advisories-sync.test.ts +++ b/tests/cli/advisories-sync.test.ts @@ -8,6 +8,14 @@ import { stripAnsi } from "../../src/utils/chalk.js"; import { isSslCertificateError } from "../../src/utils/network.js"; import type { ParsedOptions } from "../../src/types.js"; +const USABLE_ADVISORY = { + id: "OSV-1", + affected: [{ + package: { ecosystem: "npm", name: "lodash" }, + ranges: [{ events: [{ introduced: "0" }, { fixed: "4.17.21" }] }], + }], +}; + describe("advisory sync CLI reporting", () => { let tempDir: string; let dbPath: string; @@ -62,13 +70,22 @@ describe("advisory sync CLI reporting", () => { expect(isSslCertificateError(caught)).toBe(true); }); + it("does not imply an offline database exists after the first sync fails", async () => { + fs.rmSync(dbPath); + await expect(runAdvisoriesSync({ + options, + fetchImpl: async () => { throw new Error("connection refused"); }, + })).rejects.toThrow("connection refused No advisory database was created."); + expect(fs.existsSync(dbPath)).toBe(false); + }); + it.each([false, true])("keeps cleanup warnings in CLI output with json=%s", async json => { const rmSync = fs.rmSync; jest.spyOn(fs, "rmSync").mockImplementation((target, rmOptions) => { if (String(target).includes(".advisories.db.tmp-")) throw new Error("cleanup denied"); rmSync(target, rmOptions); }); - const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify({ id: "OSV-1" })) }); + const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify(USABLE_ADVISORY)) }); await runAdvisoriesSync({ options: { ...options, json }, fetchImpl: async () => new Response(Buffer.from(payload)), @@ -80,6 +97,28 @@ describe("advisory sync CLI reporting", () => { expect(console.error).not.toHaveBeenCalled(); }); + it("reports cleanup warnings when --output - names a literal database file", async () => { + const cwd = process.cwd(); + const rmSync = fs.rmSync; + jest.spyOn(fs, "rmSync").mockImplementation((target, rmOptions) => { + if (String(target).includes(".-.tmp-")) throw new Error("cleanup denied"); + rmSync(target, rmOptions); + }); + try { + process.chdir(tempDir); + const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify(USABLE_ADVISORY)) }); + await runAdvisoriesSync({ + options: { ...options, output: "-" }, + fetchImpl: async () => new Response(Buffer.from(payload)), + }); + expect(fs.existsSync(path.join(tempDir, "-"))).toBe(true); + const messages = jest.mocked(console.log).mock.calls.map(args => stripAnsi(args.join(" "))); + expect(messages.some(message => message.includes("cleanup denied"))).toBe(true); + } finally { + process.chdir(cwd); + } + }); + it("does not claim preservation when installation and restoration both fail", async () => { const renameSync = fs.renameSync; let renameCount = 0; @@ -89,7 +128,7 @@ describe("advisory sync CLI reporting", () => { if (renameCount >= 3) throw new Error("install or restore denied"); renameSync(from, to); }); - const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify({ id: "OSV-1" })) }); + const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify(USABLE_ADVISORY)) }); let caught: unknown; try { await runAdvisoriesSync({ options, fetchImpl: async () => new Response(Buffer.from(payload)) }); diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index 73697abab..1bc2fd551 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -6,6 +6,14 @@ import { zipSync } from "fflate"; import { LocalAdvisoryDatabase } from "../src/advisory/local-db.js"; import { getDefaultAdvisoryDbPath, syncOsvAdvisories } from "../src/advisory/osv-sync.js"; +const USABLE_ADVISORY = { + id: "OSV-1", + affected: [{ + package: { ecosystem: "npm", name: "lodash" }, + ranges: [{ events: [{ introduced: "0" }, { fixed: "4.17.21" }] }], + }], +}; + function createTempDir(): string { return fs.mkdtempSync(path.join(os.tmpdir(), "cve-lite-osv-sync-")); } @@ -274,12 +282,17 @@ describe("syncOsvAdvisories", () => { } }); - it("rejects a refresh with no usable advisories and preserves the existing DB", async () => { + it.each([ + { scenario: "README-only", files: { "README.txt": "no advisory records" } }, + { scenario: "withdrawn-only", files: { "withdrawn.json": { ...USABLE_ADVISORY, withdrawn: "2026-01-15T00:00:00Z" } } }, + { scenario: "id-only", files: { "id-only.json": { id: "OSV-1" } } }, + { scenario: "empty-affected", files: { "empty.json": { id: "OSV-1", affected: [] } } }, + ])("rejects a $scenario refresh with no usable advisories and preserves the existing DB", async ({ files }) => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); const previousDb = "previous-known-good-db"; fs.writeFileSync(dbPath, previousDb, "utf8"); - const zipPayload = createZipPayload({ "README.txt": "no advisory records" }); + const zipPayload = createZipPayload(files); try { await expect( @@ -334,7 +347,7 @@ describe("syncOsvAdvisories", () => { }); const warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); try { - const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); const result = await syncOsvAdvisories({ outputPath: dbPath, fetchImpl: async () => new Response(Buffer.from(zipPayload)), @@ -377,6 +390,48 @@ describe("syncOsvAdvisories", () => { } }); + it.each([false, true])("reports failed backup removal with temporary cleanup failure=%s", async tempCleanupFails => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); + const renameSync = fs.renameSync; + const rmSync = fs.rmSync; + let renameCount = 0; + const renameSpy = jest.spyOn(fs, "renameSync").mockImplementation((from, to) => { + if (++renameCount === 1) throw Object.assign(new Error("target locked"), { code: "EPERM" }); + renameSync(from, to); + }); + const cleanupSpy = jest.spyOn(fs, "rmSync").mockImplementation((target, options) => { + if (String(target).includes(".backup-")) throw new Error("backup cleanup denied"); + if (tempCleanupFails && String(target).includes(".tmp-")) throw new Error("temporary cleanup denied"); + rmSync(target, options); + }); + try { + const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); + const result = await syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(zipPayload)), + }); + const backup = fs.readdirSync(tempDir).find(name => name.includes(".backup-")); + expect(backup).toBeDefined(); + expect(result.warnings).toContainEqual(expect.stringContaining(`backup ${path.join(tempDir, backup!)}: backup cleanup denied`)); + expect(result.warnings).toContainEqual(expect.stringContaining(`remove ${path.join(tempDir, backup!)} manually`)); + expect(result.warnings).toHaveLength(tempCleanupFails ? 2 : 1); + if (tempCleanupFails) expect(result.warnings).toContainEqual(expect.stringContaining("temporary cleanup denied")); + expect(fs.readFileSync(path.join(tempDir, backup!), "utf8")).toBe("previous-known-good-db"); + const db = new LocalAdvisoryDatabase(dbPath, { readonly: true }); + try { + expect(db.findMatchingVulnerabilityIds({ ecosystem: "npm", name: "lodash", version: "4.17.20" })).toEqual(["OSV-1"]); + } finally { + db.close(); + } + } finally { + renameSpy.mockRestore(); + cleanupSpy.mockRestore(); + removeDir(tempDir); + } + }); + it.each([false, true])("reports the database state when installation fails and restoration fails=%s", async restoreFails => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); @@ -391,14 +446,14 @@ describe("syncOsvAdvisories", () => { renameSync(from, to); }); try { - const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); await expect(syncOsvAdvisories({ outputPath: dbPath, fetchImpl: async () => new Response(Buffer.from(zipPayload)), })).rejects.toMatchObject({ databaseState: restoreFails ? "recovery-required" : "unchanged", message: restoreFails - ? expect.stringMatching(/install denied.*remains at .*backup-.*restore denied/) + ? expect.stringMatching(/install denied.*remains at .*backup-.*restore denied.*Close any program using .*rename .* to .*restore the previous database/) : "install denied", }); if (restoreFails) { @@ -419,7 +474,7 @@ describe("syncOsvAdvisories", () => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); try { - const zipPayload = createZipPayload({ "OSV-1.json": { id: "OSV-1" } }); + const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); await expect(syncOsvAdvisories({ outputPath: dbPath, fetchImpl: async () => new Response(Buffer.from(zipPayload)), From 42cf13cfbedacf60d717186c1ac80779d0898c0c Mon Sep 17 00:00:00 2001 From: vian Date: Wed, 4 Nov 2026 18:57:31 +0530 Subject: [PATCH 8/8] fix: preserve concurrent syncs and clean abandoned staging --- src/advisory/osv-sync.ts | 74 +++++++--- src/advisory/staging-directory.ts | 109 ++++++++++++++ src/cli/advisories-sync.ts | 4 +- src/utils/string.ts | 6 + tests/cli/advisories-sync.test.ts | 37 ++++- tests/osv-sync.test.ts | 237 +++++++++++++++++++++++++++++- 6 files changed, 436 insertions(+), 31 deletions(-) create mode 100644 src/advisory/staging-directory.ts diff --git a/src/advisory/osv-sync.ts b/src/advisory/osv-sync.ts index a653897b4..0ab61d987 100644 --- a/src/advisory/osv-sync.ts +++ b/src/advisory/osv-sync.ts @@ -1,10 +1,12 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { randomUUID } from "node:crypto"; import { unzipSync } from "fflate"; import type { FetchLike, OsvVuln } from "../types.js"; import { LocalAdvisoryDatabase } from "./local-db.js"; -import { pluralize } from "../utils/string.js"; +import { ensureSentence, pluralize } from "../utils/string.js"; +import { cleanupAbandonedStagingDirectories, recordStagingOwner, stagingDirectoryPrefix } from "./staging-directory.js"; const DEFAULT_OSV_NPM_DUMP_URL = "https://storage.googleapis.com/osv-vulnerabilities/npm/all.zip"; export const ADVISORY_DB_STALE_AFTER_MS = 7 * 24 * 60 * 60 * 1000; @@ -100,7 +102,7 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): string | und if (code !== "EEXIST" && code !== "EPERM") throw error; } - const backupPath = `${dbPath}.backup-${process.pid}-${Date.now()}`; + const backupPath = `${dbPath}.backup-${process.pid}-${randomUUID()}`; let movedExisting = false; try { fs.renameSync(dbPath, backupPath); @@ -109,30 +111,52 @@ function replaceAdvisoryDatabase(tempPath: string, dbPath: string): string | und } catch (error) { if (!movedExisting) throw error; - try { - fs.renameSync(backupPath, dbPath); - } catch (restoreError) { - const originalMessage = error instanceof Error ? error.message : String(error); - const restoreMessage = restoreError instanceof Error ? restoreError.message : String(restoreError); - throw new AdvisorySyncError( - `Failed to install refreshed advisory database: ${originalMessage}. ` - + `The previous database could not be restored and remains at ${backupPath}: ${restoreMessage}. ` - + `Close any program using ${dbPath}, resolve the filesystem error, ` - + `then rename ${backupPath} to ${dbPath} to restore the previous database.`, - "recovery-required", - restoreError, - ); - } - throw error; + restoreAdvisoryDatabase(backupPath, dbPath, error); } + return cleanupBackup(backupPath, "The refreshed database was installed."); +} + +function cleanupBackup(backupPath: string, databaseMessage: string): string | undefined { try { fs.rmSync(backupPath, { force: true }); } catch (error) { const message = error instanceof Error ? error.message : String(error); - return `Could not remove advisory database backup ${backupPath}: ${message}. ` - + `The refreshed database was installed. Resolve the filesystem error and remove ${backupPath} manually.`; + return `Could not remove advisory database backup ${backupPath}: ${ensureSentence(message)} ` + + `${databaseMessage} Resolve the filesystem error and remove ${backupPath} manually.`; + } +} + +function restoreAdvisoryDatabase(backupPath: string, dbPath: string, installError: unknown): never { + const originalMessage = installError instanceof Error ? installError.message : String(installError); + try { + // Hardlink creation is atomic and refuses an existing target. An exists-check + // followed by rename can overwrite a database installed by another sync. + fs.linkSync(backupPath, dbPath); + } catch (restoreError) { + const restoreMessage = restoreError instanceof Error ? restoreError.message : String(restoreError); + throw new AdvisorySyncError( + `Failed to install refreshed advisory database: ${ensureSentence(originalMessage)} ` + + `The previous database could not be restored and remains at ${backupPath}: ${ensureSentence(restoreMessage)} ` + + `No existing database at ${dbPath} was overwritten. Close any program using these files and inspect both databases. ` + + `After resolving the filesystem error, rename ${backupPath} to ${dbPath} only if the target is absent; do not overwrite a newer database.`, + "recovery-required", + restoreError, + ); + } + const warning = cleanupBackup(backupPath, "The previous database was restored."); + if (warning) throw new AdvisorySyncError(`${ensureSentence(originalMessage)} ${warning}`, "unchanged", installError); + throw installError; +} + +function rejectDirectoryOutput(dbPath: string): void { + try { + if (!fs.lstatSync(dbPath).isDirectory()) return; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return; + throw error; } + throw new Error(`Advisory database output ${dbPath} is a directory. Choose a database file path, for example ${path.join(dbPath, "advisories.db")}.`); } type AdvisoryArchiveEntry = readonly [name: string, bytes: Uint8Array]; @@ -265,7 +289,7 @@ function cleanupTempDirectory(tempDir: string): string | undefined { fs.rmSync(tempDir, { recursive: true, force: true }); } catch (error) { const message = error instanceof Error ? error.message : String(error); - return `Could not remove temporary advisory sync directory ${tempDir}: ${message}. ` + return `Could not remove temporary advisory sync directory ${tempDir}: ${ensureSentence(message)} ` + `Resolve the filesystem error and remove ${tempDir} manually.`; } } @@ -280,7 +304,7 @@ function buildSyncError( ? refreshError.databaseState : databaseInstalled ? "replaced" : "unchanged"; return new AdvisorySyncError( - cleanupWarning ? `${message}. ${cleanupWarning}` : message, + cleanupWarning ? `${ensureSentence(message)} ${cleanupWarning}` : message, databaseState, refreshError, ); @@ -292,16 +316,20 @@ export async function syncOsvAdvisories( const onProgress = options.onProgress; const sourceUrl = options.sourceUrl ?? DEFAULT_OSV_NPM_DUMP_URL; const dbPath = getDefaultAdvisoryDbPath(options.outputPath); + rejectDirectoryOutput(dbPath); let tempDir: string | undefined; let databaseInstalled = false; let refreshError: unknown; let cleanupWarning: string | undefined; let replacementWarning: string | undefined; let syncResult: SyncOsvAdvisoriesResult | undefined; + const startupWarnings: string[] = []; try { fs.mkdirSync(path.dirname(dbPath), { recursive: true }); - tempDir = fs.mkdtempSync(path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-`)); + startupWarnings.push(...cleanupAbandonedStagingDirectories(dbPath)); + tempDir = fs.mkdtempSync(stagingDirectoryPrefix(dbPath)); + recordStagingOwner(tempDir, dbPath); const tempDbPath = path.join(tempDir, path.basename(dbPath)); const zippedBytes = await downloadAdvisoryArchive(sourceUrl, dbPath, options.fetchImpl ?? fetch, onProgress); const advisoryCount = await buildAdvisoryDatabase(zippedBytes, tempDbPath, sourceUrl, onProgress); @@ -320,7 +348,7 @@ export async function syncOsvAdvisories( if (tempDir !== undefined) cleanupWarning = cleanupTempDirectory(tempDir); } - const warnings = [replacementWarning, cleanupWarning].filter((warning): warning is string => warning !== undefined); + const warnings = [...startupWarnings, replacementWarning, cleanupWarning].filter((warning): warning is string => warning !== undefined); if (syncResult === undefined) { throw buildSyncError(refreshError, databaseInstalled, warnings.join(" ") || undefined); } diff --git a/src/advisory/staging-directory.ts b/src/advisory/staging-directory.ts new file mode 100644 index 000000000..3fbdce7bb --- /dev/null +++ b/src/advisory/staging-directory.ts @@ -0,0 +1,109 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { randomUUID } from "node:crypto"; +import { ensureSentence } from "../utils/string.js"; + +const OWNER_FILE = "owner.json"; + +type StagingOwner = { + version: 1; + pid: number; + hostname: string; + dbPath: string; + directoryName: string; +}; + +export function stagingDirectoryPrefix(dbPath: string): string { + return path.join(path.dirname(dbPath), `.${path.basename(dbPath)}.tmp-${process.pid}-`); +} + +/** Called after the caller records the directory, so finally also covers write failures. */ +export function recordStagingOwner(tempDir: string, dbPath: string): void { + const owner: StagingOwner = { + version: 1, + pid: process.pid, + hostname: os.hostname(), + dbPath, + directoryName: path.basename(tempDir), + }; + fs.writeFileSync(path.join(tempDir, OWNER_FILE), JSON.stringify(owner), { flag: "wx", mode: 0o600 }); +} + +function readOwner(directory: string, dbPath: string): StagingOwner | null { + const entry = fs.lstatSync(directory); + if (!entry.isDirectory() || entry.isSymbolicLink()) return null; + const metadata = fs.lstatSync(path.join(directory, OWNER_FILE)); + if (!metadata.isFile() || metadata.isSymbolicLink()) return null; + const parsed: unknown = JSON.parse(fs.readFileSync(path.join(directory, OWNER_FILE), "utf8")); + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return null; + const owner = parsed as Partial; + if (owner.version !== 1 || !Number.isSafeInteger(owner.pid) || owner.pid! <= 0 || owner.pid! > 2147483647) return null; + if (owner.hostname !== os.hostname() || owner.dbPath !== dbPath || typeof owner.directoryName !== "string") return null; + const prefix = `.${path.basename(dbPath)}.tmp-${owner.pid}-`; + if (!owner.directoryName.startsWith(prefix) || path.basename(owner.directoryName) !== owner.directoryName) return null; + const name = path.basename(directory); + if (name !== owner.directoryName && !name.startsWith(`${owner.directoryName}.cleanup-`)) return null; + // On POSIX, do not sweep another user's staging directory, even with matching metadata. + if (process.getuid && (entry.uid !== process.getuid() || metadata.uid !== process.getuid())) return null; + return owner as StagingOwner; +} + +function ownerHasExited(pid: number): boolean { + try { + process.kill(pid, 0); + return false; + } catch (error) { + // EPERM and any unfamiliar error mean the process may still be active. + return (error as NodeJS.ErrnoException).code === "ESRCH"; + } +} + +function removeAbandonedDirectory(directory: string, dbPath: string): void { + const before = fs.lstatSync(directory); + const owner = readOwner(directory, dbPath); + if (owner === null || !ownerHasExited(owner.pid)) return; + if (path.basename(directory) !== owner.directoryName) { + const claim = path.basename(directory).match(/\.cleanup-([1-9]\d*)-[\da-f-]{36}$/i); + if (claim === null || !ownerHasExited(Number(claim[1]))) return; + } + const current = fs.lstatSync(directory); + if (current.dev !== before.dev || current.ino !== before.ino) return; + const claimedPath = path.join(path.dirname(directory), `${owner.directoryName}.cleanup-${process.pid}-${randomUUID()}`); + // A claim avoids two scavengers recursively removing the same directory. + fs.renameSync(directory, claimedPath); + const claimed = fs.lstatSync(claimedPath); + const claimedOwner = readOwner(claimedPath, dbPath); + if (claimed.dev !== before.dev || claimed.ino !== before.ino || claimedOwner === null || !ownerHasExited(claimedOwner.pid)) return; + try { + fs.rmSync(claimedPath, { recursive: true, force: true }); + } catch (error) { + throw new Error(`Could not remove abandoned advisory sync directory ${claimedPath}: ${ensureSentence(error instanceof Error ? error.message : String(error))} Resolve the filesystem error and remove it manually once no sync is using it.`, { cause: error }); + } +} + +/** Never infer ownership from a glob or age: legacy/unverifiable directories are left alone. */ +export function cleanupAbandonedStagingDirectories(dbPath: string): string[] { + const warnings: string[] = []; + const parent = path.dirname(dbPath); + const prefix = `.${path.basename(dbPath)}.tmp-`; + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(parent, { withFileTypes: true }); + } catch (error) { + return [`Could not inspect abandoned advisory sync directories in ${parent}: ${ensureSentence(error instanceof Error ? error.message : String(error))} Check directory permissions and remove abandoned staging files manually once no sync is using them.`]; + } + for (const entry of entries) { + if (!entry.isDirectory() || entry.isSymbolicLink() || !entry.name.startsWith(prefix)) continue; + const directory = path.join(parent, entry.name); + try { + removeAbandonedDirectory(directory, dbPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT" || error instanceof SyntaxError) continue; + warnings.push(error instanceof Error && error.message.startsWith("Could not remove abandoned advisory sync directory") + ? error.message + : `Could not inspect abandoned advisory sync directory ${directory}: ${ensureSentence(error instanceof Error ? error.message : String(error))} Check this directory and remove it manually only when no sync is using it.`); + } + } + return warnings; +} diff --git a/src/cli/advisories-sync.ts b/src/cli/advisories-sync.ts index c9290858b..528f42947 100644 --- a/src/cli/advisories-sync.ts +++ b/src/cli/advisories-sync.ts @@ -4,7 +4,7 @@ import { AdvisorySyncError, getDefaultAdvisoryDbPath, syncOsvAdvisories } from " import { logWarn } from "../output/formatters.js"; import { createSpinner } from "../output/spinner.js"; import { chalk } from "../utils/chalk.js"; -import { pluralize } from "../utils/string.js"; +import { ensureSentence, pluralize } from "../utils/string.js"; import type { FetchLike, ParsedOptions } from "../types.js"; export interface AdvisoriesSyncParams { @@ -35,7 +35,7 @@ export async function runAdvisoriesSync({ options, fetchImpl }: AdvisoriesSyncPa const stateMessage = databaseExisted ? "The existing advisory database was not changed." : "No advisory database was created."; - throw new Error(`${error.message} ${stateMessage}`, { cause: error }); + throw new Error(`${ensureSentence(error.message)} ${stateMessage}`, { cause: error }); } throw error; }); diff --git a/src/utils/string.ts b/src/utils/string.ts index a4a6b63b9..82f5e3473 100644 --- a/src/utils/string.ts +++ b/src/utils/string.ts @@ -2,6 +2,12 @@ export function pluralize(count: number, singular: string, plural = `${singular} return count === 1 ? singular : plural; } +/** End diagnostic text as a sentence without duplicating existing punctuation. */ +export function ensureSentence(message: string): string { + const text = message.trimEnd(); + return text && !/[.!?]$/.test(text) ? `${text}.` : text; +} + export function isOutputStdout(output?: string): boolean { return output?.trim() === "-"; } diff --git a/tests/cli/advisories-sync.test.ts b/tests/cli/advisories-sync.test.ts index fd1280a62..9e88cb20b 100644 --- a/tests/cli/advisories-sync.test.ts +++ b/tests/cli/advisories-sync.test.ts @@ -70,15 +70,49 @@ describe("advisory sync CLI reporting", () => { expect(isSslCertificateError(caught)).toBe(true); }); + it("reports a directory output as invalid input without calling it an existing database", async () => { + const fetchImpl = jest.fn(async () => new Response("unexpected")); + let caught: unknown; + try { + await runAdvisoriesSync({ options: { ...options, output: tempDir }, fetchImpl }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect((caught as Error).message).toContain(`${tempDir} is a directory.`); + expect((caught as Error).message).not.toContain("The existing advisory database"); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + }); + it("does not imply an offline database exists after the first sync fails", async () => { fs.rmSync(dbPath); await expect(runAdvisoriesSync({ options, fetchImpl: async () => { throw new Error("connection refused"); }, - })).rejects.toThrow("connection refused No advisory database was created."); + })).rejects.toThrow("connection refused. No advisory database was created."); expect(fs.existsSync(dbPath)).toBe(false); }); + it.each([ + { detail: "fetch failed", sentence: "fetch failed." }, + { detail: "fetch failed.", sentence: "fetch failed." }, + { detail: "fetch failed. ", sentence: "fetch failed." }, + ])("separates the failure and database-state sentences for '$detail'", async ({ detail, sentence }) => { + await expect(runAdvisoriesSync({ + options, + fetchImpl: async () => { throw new Error(detail); }, + })).rejects.toThrow(`${sentence} The existing advisory database was not changed.`); + }); + + it("separates an HTTP failure from the first-sync state message", async () => { + fs.rmSync(dbPath); + await expect(runAdvisoriesSync({ + options, + fetchImpl: async () => new Response("unavailable", { status: 500, statusText: "Internal Server Error" }), + })).rejects.toThrow("OSV dump download failed: 500 Internal Server Error. No advisory database was created."); + }); + it.each([false, true])("keeps cleanup warnings in CLI output with json=%s", async json => { const rmSync = fs.rmSync; jest.spyOn(fs, "rmSync").mockImplementation((target, rmOptions) => { @@ -128,6 +162,7 @@ describe("advisory sync CLI reporting", () => { if (renameCount >= 3) throw new Error("install or restore denied"); renameSync(from, to); }); + jest.spyOn(fs, "linkSync").mockImplementation(() => { throw new Error("restore denied"); }); const payload = zipSync({ "OSV-1.json": Buffer.from(JSON.stringify(USABLE_ADVISORY)) }); let caught: unknown; try { diff --git a/tests/osv-sync.test.ts b/tests/osv-sync.test.ts index 1bc2fd551..3640ad77c 100644 --- a/tests/osv-sync.test.ts +++ b/tests/osv-sync.test.ts @@ -37,7 +37,182 @@ function createRawZipPayload(files: Record): Uint8Array { return zipSync(files); } +function createOwnedStagingDirectory(dbPath: string, pid: number, suffix: string, overrides: Record = {}): string { + const directoryName = `.${path.basename(dbPath)}.tmp-${pid}-${suffix}`; + const directory = path.join(path.dirname(dbPath), directoryName); + fs.mkdirSync(directory); + fs.writeFileSync(path.join(directory, "owner.json"), JSON.stringify({ + version: 1, pid, hostname: os.hostname(), dbPath, directoryName, ...overrides, + })); + fs.writeFileSync(path.join(directory, "partial.db"), "incomplete"); + return directory; +} + describe("syncOsvAdvisories", () => { + it("rejects a real directory output before creating staging files or fetching", async () => { + const tempDir = createTempDir(); + const directory = path.join(tempDir, "do-not-replace"); + fs.mkdirSync(directory); + fs.writeFileSync(path.join(directory, "keep.txt"), "important"); + const fetchImpl = jest.fn(async () => new Response("unexpected")); + const mkdirSpy = jest.spyOn(fs, "mkdirSync"); + const mkdtempSpy = jest.spyOn(fs, "mkdtempSync"); + const renameSpy = jest.spyOn(fs, "renameSync"); + try { + await expect(syncOsvAdvisories({ outputPath: directory, fetchImpl })).rejects.toThrow( + `Advisory database output ${directory} is a directory. Choose a database file path, for example ${path.join(directory, "advisories.db")}.`, + ); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(mkdirSpy).not.toHaveBeenCalled(); + expect(mkdtempSpy).not.toHaveBeenCalled(); + expect(renameSpy).not.toHaveBeenCalled(); + expect(fs.readFileSync(path.join(directory, "keep.txt"), "utf8")).toBe("important"); + expect(fs.readdirSync(tempDir)).toEqual(["do-not-replace"]); + } finally { + jest.restoreAllMocks(); + removeDir(tempDir); + } + }); + + it("keeps replacing an output symlink without changing the file it points to", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const targetPath = path.join(tempDir, "original.db"); + fs.writeFileSync(targetPath, "original target"); + fs.symlinkSync(targetPath, dbPath); + try { + await syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(createZipPayload({ "OSV-1.json": USABLE_ADVISORY }))), + }); + expect(fs.lstatSync(dbPath).isSymbolicLink()).toBe(false); + expect(fs.readFileSync(targetPath, "utf8")).toBe("original target"); + const db = new LocalAdvisoryDatabase(dbPath, { readonly: true }); + try { expect(db.getVulnerability("OSV-1")).toMatchObject({ id: "OSV-1" }); } finally { db.close(); } + } finally { + removeDir(tempDir); + } + }); + + it("cleans staging files if ownership metadata cannot be written", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db"); + const writeFileSync = fs.writeFileSync; + const writeSpy = jest.spyOn(fs, "writeFileSync").mockImplementation((file, data, options) => { + if (path.basename(String(file)) === "owner.json") throw new Error("owner write denied"); + writeFileSync(file, data, options); + }); + const fetchImpl = jest.fn(async () => new Response("unexpected")); + try { + await expect(syncOsvAdvisories({ outputPath: dbPath, fetchImpl })).rejects.toMatchObject({ + databaseState: "unchanged", message: "owner write denied", + }); + expect(fetchImpl).not.toHaveBeenCalled(); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + expect(fs.readdirSync(tempDir)).toEqual(["advisories.db"]); + } finally { + writeSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it("sweeps only verifiably abandoned staging directories and preserves active, foreign, unknown and symlink paths", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const stale = createOwnedStagingDirectory(dbPath, 123456, "stale"); + const active = createOwnedStagingDirectory(dbPath, process.pid, "active"); + const forbidden = createOwnedStagingDirectory(dbPath, 123457, "permission-denied"); + const foreign = createOwnedStagingDirectory(dbPath, 123456, "foreign", { hostname: "another-host" }); + const invalid = createOwnedStagingDirectory(dbPath, 123456, "invalid-metadata"); + fs.writeFileSync(path.join(invalid, "owner.json"), "null"); + const unknown = path.join(tempDir, ".advisories.db.tmp-legacy"); + fs.mkdirSync(unknown); + const symlink = path.join(tempDir, ".advisories.db.tmp-symlink"); + fs.symlinkSync(stale, symlink, "dir"); + const kill = process.kill; + const killSpy = jest.spyOn(process, "kill").mockImplementation((pid, signal) => { + if (pid === 123456) throw Object.assign(new Error("process exited"), { code: "ESRCH" }); + if (pid === 123457) throw Object.assign(new Error("not permitted"), { code: "EPERM" }); + return kill(pid, signal); + }); + try { + const result = await syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => { + expect(fs.existsSync(stale)).toBe(false); + for (const directory of [active, forbidden, foreign, invalid, unknown]) expect(fs.existsSync(directory)).toBe(true); + expect(fs.lstatSync(symlink).isSymbolicLink()).toBe(true); + return new Response(Buffer.from(createZipPayload({ "OSV-1.json": USABLE_ADVISORY }))); + }, + }); + expect(result.warnings).toBeUndefined(); + } finally { + killSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it("reclaims a crashed sweep's claim but does not disturb a live sweep's claim", async () => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + const originalDead = createOwnedStagingDirectory(dbPath, 123456, "dead-claim"); + const originalLive = createOwnedStagingDirectory(dbPath, 123456, "live-claim"); + const uuid = "00000000-0000-4000-8000-000000000000"; + const deadClaim = `${originalDead}.cleanup-123457-${uuid}`; + const liveClaim = `${originalLive}.cleanup-${process.pid}-${uuid}`; + fs.renameSync(originalDead, deadClaim); + fs.renameSync(originalLive, liveClaim); + const kill = process.kill; + const killSpy = jest.spyOn(process, "kill").mockImplementation((pid, signal) => { + if (pid === 123456 || pid === 123457) throw Object.assign(new Error("process exited"), { code: "ESRCH" }); + return kill(pid, signal); + }); + try { + await syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(createZipPayload({ "OSV-1.json": USABLE_ADVISORY }))), + }); + expect(fs.existsSync(deadClaim)).toBe(false); + expect(fs.existsSync(liveClaim)).toBe(true); + } finally { + killSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it.each([false, true])("reports abandoned-directory cleanup errors when download fails=%s", async downloadFails => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db"); + createOwnedStagingDirectory(dbPath, 123456, "stale"); + const killSpy = jest.spyOn(process, "kill").mockImplementation(() => { throw Object.assign(new Error("process exited"), { code: "ESRCH" }); }); + const rmSync = fs.rmSync; + const cleanupSpy = jest.spyOn(fs, "rmSync").mockImplementation((target, options) => { + if (String(target).includes(".cleanup-")) throw new Error("stale cleanup denied"); + rmSync(target, options); + }); + try { + const sync = syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => { + if (downloadFails) throw new Error("network unavailable."); + return new Response(Buffer.from(createZipPayload({ "OSV-1.json": USABLE_ADVISORY }))); + }, + }); + if (downloadFails) { + await expect(sync).rejects.toMatchObject({ message: expect.stringMatching(/network unavailable\. Could not remove abandoned.*stale cleanup denied/), databaseState: "unchanged" }); + expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); + } else { + await expect(sync).resolves.toMatchObject({ advisoryCount: 1, warnings: [expect.stringMatching(/Could not remove abandoned.*stale cleanup denied/)] }); + } + } finally { + cleanupSpy.mockRestore(); + killSpy.mockRestore(); + removeDir(tempDir); + } + }); + it("downloads the OSV npm dump and populates the local advisory DB", async () => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); @@ -368,11 +543,11 @@ describe("syncOsvAdvisories", () => { } }); - it("retains the refresh cause and recovery guidance when temporary cleanup also fails", async () => { + it.each(["certificate verification failed", "certificate verification failed."])("retains the refresh cause and recovery guidance when temporary cleanup also fails: %s", async message => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); fs.writeFileSync(dbPath, "previous-known-good-db", "utf8"); - const refreshError = new Error("certificate verification failed"); + const refreshError = new Error(message); const cleanupSpy = jest.spyOn(fs, "rmSync").mockImplementation(() => { throw new Error("cleanup denied"); }); try { await expect(syncOsvAdvisories({ @@ -381,7 +556,7 @@ describe("syncOsvAdvisories", () => { })).rejects.toMatchObject({ databaseState: "unchanged", cause: refreshError, - message: expect.stringMatching(/certificate verification failed.*cleanup denied.*remove .* manually/), + message: expect.stringMatching(/certificate verification failed\. Could not remove temporary.*cleanup denied.*remove .* manually/), }); expect(fs.readFileSync(dbPath, "utf8")).toBe("previous-known-good-db"); } finally { @@ -442,9 +617,13 @@ describe("syncOsvAdvisories", () => { renameCount += 1; if (renameCount === 1) throw Object.assign(new Error("target locked"), { code: "EPERM" }); if (renameCount === 3) throw new Error("install denied"); - if (renameCount === 4 && restoreFails) throw new Error("restore denied"); renameSync(from, to); }); + const linkSync = fs.linkSync; + const linkSpy = jest.spyOn(fs, "linkSync").mockImplementation((from, to) => { + if (restoreFails) throw Object.assign(new Error("restore denied"), { code: "ENOTSUP" }); + linkSync(from, to); + }); try { const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); await expect(syncOsvAdvisories({ @@ -453,7 +632,7 @@ describe("syncOsvAdvisories", () => { })).rejects.toMatchObject({ databaseState: restoreFails ? "recovery-required" : "unchanged", message: restoreFails - ? expect.stringMatching(/install denied.*remains at .*backup-.*restore denied.*Close any program using .*rename .* to .*restore the previous database/) + ? expect.stringMatching(/install denied.*remains at .*backup-.*restore denied.*Close any program using .*rename .* to .*only if the target is absent; do not overwrite a newer database/) : "install denied", }); if (restoreFails) { @@ -466,6 +645,46 @@ describe("syncOsvAdvisories", () => { } } finally { renameSpy.mockRestore(); + linkSpy.mockRestore(); + removeDir(tempDir); + } + }); + + it.each(["install", "restore"])("does not overwrite a newer database installed during %s failure recovery", async racePhase => { + const tempDir = createTempDir(); + const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db"); + const renameSync = fs.renameSync; + const linkSync = fs.linkSync; + let renameCount = 0; + const renameSpy = jest.spyOn(fs, "renameSync").mockImplementation((from, to) => { + renameCount += 1; + if (renameCount === 1) throw Object.assign(new Error("target locked"), { code: "EPERM" }); + if (renameCount === 3) { + if (racePhase === "install") fs.writeFileSync(dbPath, "concurrently-installed-db"); + throw new Error("install denied"); + } + renameSync(from, to); + }); + const linkSpy = jest.spyOn(fs, "linkSync").mockImplementation((from, to) => { + if (racePhase === "restore") fs.writeFileSync(dbPath, "concurrently-installed-db"); + linkSync(from, to); + }); + try { + await expect(syncOsvAdvisories({ + outputPath: dbPath, + fetchImpl: async () => new Response(Buffer.from(createZipPayload({ "OSV-1.json": USABLE_ADVISORY }))), + })).rejects.toMatchObject({ + databaseState: "recovery-required", + message: expect.stringContaining("only if the target is absent; do not overwrite a newer database"), + }); + expect(fs.readFileSync(dbPath, "utf8")).toBe("concurrently-installed-db"); + const backups = fs.readdirSync(tempDir).filter(name => name.includes(".backup-")); + expect(backups).toHaveLength(1); + expect(fs.readFileSync(path.join(tempDir, backups[0]), "utf8")).toBe("previous-known-good-db"); + } finally { + renameSpy.mockRestore(); + linkSpy.mockRestore(); removeDir(tempDir); } }); @@ -473,6 +692,7 @@ describe("syncOsvAdvisories", () => { it("reports replacement if the completion callback throws after installing the database", async () => { const tempDir = createTempDir(); const dbPath = path.join(tempDir, "advisories.db"); + fs.writeFileSync(dbPath, "previous-known-good-db"); try { const zipPayload = createZipPayload({ "OSV-1.json": USABLE_ADVISORY }); await expect(syncOsvAdvisories({ @@ -480,6 +700,13 @@ describe("syncOsvAdvisories", () => { fetchImpl: async () => new Response(Buffer.from(zipPayload)), onProgress: event => { if (event.phase === "complete") throw new Error("callback failed"); }, })).rejects.toMatchObject({ databaseState: "replaced", message: "callback failed" }); + const db = new LocalAdvisoryDatabase(dbPath, { readonly: true }); + try { + expect(db.getVulnerability("OSV-1")).toMatchObject({ id: "OSV-1" }); + expect(db.findMatchingVulnerabilityIds({ ecosystem: "npm", name: "lodash", version: "4.17.20" })).toEqual(["OSV-1"]); + } finally { + db.close(); + } } finally { removeDir(tempDir); }