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() +})