From c81cc0b41ef87321796d30e29ed7997da8a642eb Mon Sep 17 00:00:00 2001 From: Luke Karrys Date: Mon, 11 Apr 2022 17:53:33 -0700 Subject: [PATCH] fix(arborist): dont skip adding advisories to audit based on name/range When generating an audit report, a cache of seen advisories is kept to avoid doing any repeat fanout work on its nodes. Previously this cache was also preventing audits from being added to the report. This has been fixed so the cache is only used to prevent extra work, but all valid advisories are added to the output. Fixes #4681 --- workspaces/arborist/lib/audit-report.js | 75 +++++----- .../test/audit-report.js.test.cjs | 129 ++++++++++++++++++ 2 files changed, 165 insertions(+), 39 deletions(-) diff --git a/workspaces/arborist/lib/audit-report.js b/workspaces/arborist/lib/audit-report.js index 4dc6dd177c1e4..9bef84686f4b4 100644 --- a/workspaces/arborist/lib/audit-report.js +++ b/workspaces/arborist/lib/audit-report.js @@ -134,16 +134,7 @@ class AuditReport extends Map { const seen = new Set() for (const advisory of advisories) { const { name, range } = advisory - - // don't flag the exact same name/range more than once - // adding multiple advisories with the same range is fine, but no - // need to search for nodes we already would have added. const k = `${name}@${range}` - if (seen.has(k)) { - continue - } - - seen.add(k) const vuln = this.get(name) || new Vuln({ name, advisory }) if (this.has(name)) { @@ -151,44 +142,50 @@ class AuditReport extends Map { } super.set(name, vuln) - const p = [] - for (const node of this.tree.inventory.query('packageName', name)) { - if (!shouldAudit(node, this[_omit], this.filterSet)) { - continue - } + // don't flag the exact same name/range more than once + // adding multiple advisories with the same range is fine, but no + // need to search for nodes we already would have added. + if (!seen.has(k)) { + const p = [] + for (const node of this.tree.inventory.query('packageName', name)) { + if (!shouldAudit(node, this[_omit], this.filterSet)) { + continue + } - // if not vulnerable by this advisory, keep searching - if (!advisory.testVersion(node.version)) { - continue - } + // if not vulnerable by this advisory, keep searching + if (!advisory.testVersion(node.version)) { + continue + } - // we will have loaded the source already if this is a metavuln - if (advisory.type === 'metavuln') { - vuln.addVia(this.get(advisory.dependency)) - } + // 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 - } + // already marked this one, no need to do it again + if (vuln.nodes.has(node)) { + continue + } - // haven't marked this one yet. get its dependents. - vuln.nodes.add(node) - for (const { from: dep, spec } of node.edgesIn) { - if (dep.isTop && !vuln.topNodes.has(dep)) { - this[_checkTopNode](dep, vuln, spec) - } else { + // haven't marked this one yet. get its dependents. + vuln.nodes.add(node) + for (const { from: dep, spec } of node.edgesIn) { + if (dep.isTop && !vuln.topNodes.has(dep)) { + this[_checkTopNode](dep, vuln, spec) + } else { // calculate a metavuln, if necessary - const calc = this.calculator.calculate(dep.packageName, advisory) - p.push(calc.then(meta => { - if (meta.testVersion(dep.version, spec)) { - advisories.add(meta) - } - })) + const calc = this.calculator.calculate(dep.packageName, advisory) + p.push(calc.then(meta => { + if (meta.testVersion(dep.version, spec)) { + advisories.add(meta) + } + })) + } } } + await Promise.all(p) + seen.add(k) } - await Promise.all(p) // make sure we actually got something. if not, remove it // this can happen if you are loading from a lockfile created by diff --git a/workspaces/arborist/tap-snapshots/test/audit-report.js.test.cjs b/workspaces/arborist/tap-snapshots/test/audit-report.js.test.cjs index 91d7ecffceaeb..cc1354e64c818 100644 --- a/workspaces/arborist/tap-snapshots/test/audit-report.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/audit-report.js.test.cjs @@ -123,6 +123,15 @@ exports[`test/audit-report.js TAP all severity levels > json version 1`] = ` "severity": "high", "range": "<3.0.8 || >=4.0.0 <4.5.3" }, + { + "source": 1325, + "name": "handlebars", + "dependency": "handlebars", + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "severity": "high", + "range": "<3.0.8 || >=4.0.0 <4.5.3" + }, { "source": 755, "name": "handlebars", @@ -448,6 +457,15 @@ exports[`test/audit-report.js TAP all severity levels > json version 1`] = ` "url": "https://npmjs.com/advisories/1478", "severity": "high", "range": ">=4.1.0" + }, + { + "source": 1479, + "name": "subtext", + "dependency": "subtext", + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1479", + "severity": "high", + "range": ">=0.0.0" } ], "effects": [], @@ -558,6 +576,15 @@ exports[`test/audit-report.js TAP audit outdated nyc and mkdirp > json version 1 "severity": "high", "range": "<3.0.8 || >=4.0.0 <4.5.3" }, + { + "source": 1325, + "name": "handlebars", + "dependency": "handlebars", + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "severity": "high", + "range": "<3.0.8 || >=4.0.0 <4.5.3" + }, { "source": 755, "name": "handlebars", @@ -918,6 +945,15 @@ exports[`test/audit-report.js TAP audit outdated nyc and mkdirp with before: opt "severity": "high", "range": "<3.0.8 || >=4.0.0 <4.5.3" }, + { + "source": 1325, + "name": "handlebars", + "dependency": "handlebars", + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "severity": "high", + "range": "<3.0.8 || >=4.0.0 <4.5.3" + }, { "source": 755, "name": "handlebars", @@ -1278,6 +1314,15 @@ exports[`test/audit-report.js TAP audit outdated nyc and mkdirp with newer endpo "severity": "high", "range": "<3.0.8 || >=4.0.0 <4.5.3" }, + { + "source": 1325, + "name": "handlebars", + "dependency": "handlebars", + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "severity": "high", + "range": "<3.0.8 || >=4.0.0 <4.5.3" + }, { "source": 755, "name": "handlebars", @@ -2144,6 +2189,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined, @@ -2623,6 +2682,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined, @@ -2791,6 +2864,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined, @@ -3270,6 +3357,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined, @@ -3458,6 +3559,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined, @@ -3927,6 +4042,20 @@ Object { "versions": undefined, "vulnerableVersions": undefined, }, + Object { + "cvss": undefined, + "cwe": undefined, + "dependency": "handlebars", + "id": undefined, + "name": "handlebars", + "range": "<3.0.8 || >=4.0.0 <4.5.3", + "severity": "high", + "source": 1325, + "title": "Prototype Pollution", + "url": "https://npmjs.com/advisories/1325", + "versions": undefined, + "vulnerableVersions": undefined, + }, Object { "cvss": undefined, "cwe": undefined,