From 2b5a24d0bc24df40c261c18dcf2d91e712e9f53c Mon Sep 17 00:00:00 2001 From: Arjun R <149392539+arjun-vegeta@users.noreply.github.com> Date: Wed, 24 Jun 2026 22:45:07 +0530 Subject: [PATCH] fix(arborist): fix audit-report determinism due to dropped via links (#9603) **Description:** Fixes #8989 **Bug:** `npm audit` was occasionally generating non-deterministic output with missing vulnerabilities when metavulns from identical-range dependencies caused a collision. **Root Cause:** In `AuditReport#init`, the deduplication logic (`if (!seen.has(k))`) was aggressively dropping identically-computed metavuln advisory ranges across different dependency paths. Because `vuln.addVia()` was invoked *inside* this block, a duplicated range collision would simply skip the execution and permanently drop the `via` link (and its corresponding `effects` link). Consequently, depending on the random `Promise.all` resolution order of the metavuln calculator across network calls, different `via` branches were being non-deterministically truncated. **Fix:** This PR extracts the `addVia` operation from the `seen` deduplication block and implements a comprehensive post-loop reconciliation pass. We ensure that: 1. `via` and `effects` links are deterministically established *after* all dependencies and metavulns are cleanly instantiated. 2. The reconciliation ignores already deleted advisories, guaranteeing that the cleanup lifecycle (`deleteAdvisory`) cannot be accidentally resurrected. 3. Because we aren't repeatedly calling the `fixAvailable` setter during the primary loop iterations, performance overhead remains minimized. **Testing:** - Added a new strict programmatic determinism test in `audit-report.js` covering the identical range metavuln fallback (`*`) condition. - Confirmed the new test correctly reproduces the regression (fails deterministically on unfixed code by dropping the `via` path). - All 27 existing suite tests pass flawlessly. (cherry picked from commit 690bf170d38a70fb47097f144affda1b2b09f121) --- workspaces/arborist/lib/audit-report.js | 16 ++++-- workspaces/arborist/test/audit-report.js | 69 ++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/audit-report.js b/workspaces/arborist/lib/audit-report.js index de2e092a96ac1..1c3e15dee1924 100644 --- a/workspaces/arborist/lib/audit-report.js +++ b/workspaces/arborist/lib/audit-report.js @@ -152,11 +152,6 @@ class AuditReport extends Map { continue } - // we will have loaded the source already if this is a metavuln - if (advisory.type === 'metavuln') { - vuln.addVia(this.get(advisory.dependency)) - } - // already marked this one, no need to do it again if (vuln.nodes.has(node)) { continue @@ -219,6 +214,17 @@ class AuditReport extends Map { } } + // post-loop reconciliation: establish all via links + // done here to ensure all vulns exist, avoid redundant setter triggers, + // and correctly handle multiple paths and omission cleanups. + for (const vuln of this.values()) { + for (const advisory of vuln.advisories) { + if (advisory.type === 'metavuln') { + vuln.addVia(this.get(advisory.dependency)) + } + } + } + timeEnd() } diff --git a/workspaces/arborist/test/audit-report.js b/workspaces/arborist/test/audit-report.js index 0322b48dbd534..ad22416a3e356 100644 --- a/workspaces/arborist/test/audit-report.js +++ b/workspaces/arborist/test/audit-report.js @@ -567,3 +567,72 @@ t.test('audit with filterSet limiting to only mkdirp and minimist', async t => { t.equal(report.get('nyc'), undefined, 'no nyc vuln reported') t.equal(report.get('mkdirp').simpleRange, '0.4.1 - 0.5.1', 'mkdirp vuln reported') }) + +t.test('determinism: multiple metavulns with identical range but different dependencies', async t => { + const registry = createRegistry(t) + + // Create a tree where A depends on both B and C. Both B and C are vulnerable. + const path = t.testdir() + const tree = new Node({ + path, + pkg: { + name: 'root', + dependencies: { + A: '1.0.0', + }, + }, + children: [ + { + pkg: { name: 'A', version: '1.0.0', dependencies: { B: '1.0.0', C: '1.0.0' } }, + }, + { + pkg: { name: 'B', version: '1.0.0' }, + }, + { + pkg: { name: 'C', version: '1.0.0' }, + }, + ], + }) + + registry.audit({ + times: 5, + results: { + B: [{ id: 1, url: 'https://B', title: 'B vuln', severity: 'high', vulnerable_versions: '*' }], + C: [{ id: 2, url: 'https://C', title: 'C vuln', severity: 'high', vulnerable_versions: '*' }], + }, + }) + + // We intentionally do not mock the packuments for A, B, and C. + // By using the fixtures directory, unmatched GET requests will receive a 404. + // This triggers the metavuln calculator fallback which defaults the effective range to `*`. + // As a result, both B and C trigger metavulns on A with identical ranges (`*`), + // producing the identical collision key `A@*` required to reproduce the determinism bug. + registry.mocks({ dir: join(__dirname, 'fixtures') }) + + // We loop 5 times just to show it is deterministic. + const results = [] + let lastReport + for (let i = 0; i < 5; i++) { + lastReport = await AuditReport.load(tree, { registry: 'https://registry.npmjs.org' }) + results.push(JSON.stringify(lastReport.toJSON())) + } + + const uniqueResults = new Set(results) + t.equal(uniqueResults.size, 1, 'output is identical across runs') + + // The key assertion is that BOTH B and C are correctly included in A's via list, + // which formally proves the determinism bug dropping a via path is fixed. + const A = lastReport.get('A') + const B = lastReport.get('B') + const C = lastReport.get('C') + + t.ok(A, 'A is vulnerable') + const viaNames = [...A.via].map(v => v.name) + const BEffects = [...B.effects].map(v => v.name) + const CEffects = [...C.effects].map(v => v.name) + + t.ok(viaNames.includes('B'), 'A via list includes B') + t.ok(viaNames.includes('C'), 'A via list includes C') + t.ok(BEffects.includes('A'), 'B effects includes A') + t.ok(CEffects.includes('A'), 'C effects includes A') +})