From 0c03afec6dc112dd1c10e321cff5ed704c077654 Mon Sep 17 00:00:00 2001 From: Tanishk Gangwar Date: Sun, 12 Jul 2026 16:45:12 +0530 Subject: [PATCH] fix(arborist): allow audit fix to downgrade to a patched older version when the only safe version in the declared range is older than what's installed, pickManifest resolved it correctly but CanPlaceDep discarded it, since it only ever replaced with semver-gte versions. thread the AuditReport through CanPlaceDep and allow the downgrade when the current node is vulnerable and the replacement is not.canReplace() still guards against breaking dependents. fixes #9557 --- workspaces/arborist/lib/can-place-dep.js | 10 ++- workspaces/arborist/lib/place-dep.js | 1 + workspaces/arborist/test/arborist/audit.js | 57 +++++++++++++++++ workspaces/arborist/test/can-place-dep.js | 72 ++++++++++++++++++++++ 4 files changed, 139 insertions(+), 1 deletion(-) diff --git a/workspaces/arborist/lib/can-place-dep.js b/workspaces/arborist/lib/can-place-dep.js index 1a3ccff669227..5752bd096dcbb 100644 --- a/workspaces/arborist/lib/can-place-dep.js +++ b/workspaces/arborist/lib/can-place-dep.js @@ -62,6 +62,7 @@ class CanPlaceDep { parent = null, peerPath = [], explicitRequest = false, + auditReport = null, } = options debug(() => { @@ -92,6 +93,7 @@ class CanPlaceDep { this.target = target this.edge = edge this.explicitRequest = explicitRequest + this.auditReport = auditReport // preventing cycles when we check peer sets this.peerPath = peerPath @@ -170,7 +172,12 @@ class CanPlaceDep { const { version: curVer } = current const { version: newVer } = dep - const tryReplace = curVer && newVer && semver.gte(newVer, curVer) + const securityDowngrade = !!( + this.auditReport && + this.auditReport.isVulnerable(current) && + !this.auditReport.isVulnerable(dep) + ) + const tryReplace = curVer && newVer && (semver.gte(newVer, curVer) || securityDowngrade) if (tryReplace && dep.canReplace(current)) { // It's extremely rare that a replaceable node would be a conflict, if // the current one wasn't a conflict, but it is theoretically possible @@ -383,6 +390,7 @@ class CanPlaceDep { peerPath, // always place peers in preferDedupe mode preferDedupe: true, + auditReport: this.auditReport, }) /* istanbul ignore next */ debug(() => { diff --git a/workspaces/arborist/lib/place-dep.js b/workspaces/arborist/lib/place-dep.js index 83bdd803409e0..34a62144e9be4 100644 --- a/workspaces/arborist/lib/place-dep.js +++ b/workspaces/arborist/lib/place-dep.js @@ -96,6 +96,7 @@ class PlaceDep { target, preferDedupe: this.preferDedupe, explicitRequest: this.explicitRequest, + auditReport: this.auditReport, }) this.checks.set(target, cpd) diff --git a/workspaces/arborist/test/arborist/audit.js b/workspaces/arborist/test/arborist/audit.js index aeb9ec42dd32b..eac829eb35039 100644 --- a/workspaces/arborist/test/arborist/audit.js +++ b/workspaces/arborist/test/arborist/audit.js @@ -54,6 +54,63 @@ t.test('audit fix reifies out the bad deps', async t => { t.matchSnapshot(tree, 'reified out the bad mkdirp and minimist') }) +t.test('audit fix downgrades to a patched older version when the vulnerable range excludes it', async t => { + // Regression test for npm/cli#9557: `npm audit fix` reported a fix was + // available but did nothing. This happens when the only non-vulnerable + // version that still satisfies the declared semver range is *older* than + // what's installed (e.g. a vuln was introduced partway through a range, + // and no newer patched version has been published yet). + const registry = new MockRegistry({ tap: t, registry: 'https://registry.npmjs.org' }) + + const manifest = registry.manifest({ name: 'vuln-pkg', versions: ['1.0.0', '1.0.1', '1.0.2'] }) + // the packument may be fetched more than once (once for the audit report, + // once while building the ideal tree for the fix), allow any number of + // fetches instead of guessing an exact count. + registry.nock = registry.nock.get(registry.fullPath('/vuln-pkg')).reply(200, manifest).persist() + + const tarballSrc = t.testdir({ + 'package.json': JSON.stringify({ name: 'vuln-pkg', version: '1.0.1' }), + }) + await registry.tarball({ manifest: manifest.versions['1.0.1'], tarball: tarballSrc }) + + registry.audit({ + results: { + 'vuln-pkg': [{ + id: 1, + url: 'https://example.com/advisories/1', + title: 'test vulnerability', + severity: 'moderate', + vulnerable_versions: '1.0.2', + }], + }, + }) + + // author the project state directly on disk instead of installing via a + // live reify- `reify({ add: [...] })` would overwrite the declared + // range with the exact version added, defeating the repro. + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'project', + version: '1.0.0', + dependencies: { 'vuln-pkg': '^1.0.0' }, + }), + 'package-lock.json': JSON.stringify({ + requires: true, + lockfileVersion: 1, + dependencies: { + 'vuln-pkg': { + version: '1.0.2', + resolved: 'https://registry.npmjs.org/vuln-pkg/-/vuln-pkg-1.0.2.tgz', + integrity: 'sha512-8VwMnyGCONIs6cWue2IdpHxHnAjzxnw2Zr7MkVxB2vjmQ2ivqGFb4LEG3SMnv0Gb2F/G/2yA8zUaiL1gywDCCg==', + }, + }, + }), + }) + + const fixed = await newArb(path).audit({ fix: true }) + t.equal(fixed.children.get('vuln-pkg').version, '1.0.1', 'downgraded to the patched older version') +}) + t.test('audit does not do globals', async t => { await t.rejects(newArb('.', { global: true }).audit(), { message: '`npm audit` does not support testing globals', diff --git a/workspaces/arborist/test/can-place-dep.js b/workspaces/arborist/test/can-place-dep.js index 60aedb543795c..b21b05e47f342 100644 --- a/workspaces/arborist/test/can-place-dep.js +++ b/workspaces/arborist/test/can-place-dep.js @@ -1409,3 +1409,75 @@ t.test('constructor debug throws', t => { t.end() }) + +t.test('audit fix replaces a vulnerable current node with an older patched one', t => { + const path = '/some/path' + + const makeTree = () => new Node({ + path, + pkg: { name: 'project', version: '1.0.0', dependencies: { a: '^1.0.0' } }, + children: [{ pkg: { name: 'a', version: '1.0.3' } }], + }) + + const fakeAuditReport = (vulnerableVersions) => ({ + isVulnerable (node) { + return node.name === 'a' && vulnerableVersions.includes(node.version) + }, + }) + + t.test('replaces when current is vulnerable and dep is not', t => { + const tree = makeTree() + const target = tree + const edge = tree.edgesOut.get('a') + const dep = new Node({ pkg: { name: 'a', version: '1.0.0' } }) + const vr = new Node({ sourceReference: tree, path, pkg: { ...tree.package } }) + dep.parent = vr + + const cpd = new CanPlaceDep({ + target, + edge, + dep, + auditReport: fakeAuditReport(['1.0.3']), + }) + t.equal(cpd.canPlace, REPLACE, 'downgrades to the patched older version') + t.end() + }) + + t.test('keeps current when neither current nor dep is flagged vulnerable', t => { + const tree = makeTree() + const target = tree + const edge = tree.edgesOut.get('a') + const dep = new Node({ pkg: { name: 'a', version: '1.0.0' } }) + const vr = new Node({ sourceReference: tree, path, pkg: { ...tree.package } }) + dep.parent = vr + + const cpd = new CanPlaceDep({ + target, + edge, + dep, + auditReport: fakeAuditReport([]), + }) + t.equal(cpd.canPlace, KEEP, 'no audit reason to prefer the older version') + t.end() + }) + + t.test('does not downgrade to another vulnerable version', t => { + const tree = makeTree() + const target = tree + const edge = tree.edgesOut.get('a') + const dep = new Node({ pkg: { name: 'a', version: '1.0.0' } }) + const vr = new Node({ sourceReference: tree, path, pkg: { ...tree.package } }) + dep.parent = vr + + const cpd = new CanPlaceDep({ + target, + edge, + dep, + auditReport: fakeAuditReport(['1.0.3', '1.0.0']), + }) + t.equal(cpd.canPlace, KEEP, 'no fix available, so current node is kept') + t.end() + }) + + t.end() +})