Skip to content

Commit 080a0f2

Browse files
wraithgarwraithgar
committed
fix!: remove old audit fallback request
BREAKING CHANGE: npm will no longer fall back to the old audit endpoint if the bulk advisory request fails. This legacy code has a long tail in npm. Getting rid of it was difficult because of how load-bearing some of those requests were in tests. This PR removes the old "mock server" that arborist tests spun up, and moved that logic into the existing mock registry that the cli uses. This will allow us to consolidate our logic in tests, and also outline more granularly which tests actually make registry requests. A few tests that were testing just the fallback behavior were also removed.
1 parent 75a3f12 commit 080a0f2

File tree

14 files changed

+1333
-1434
lines changed

14 files changed

+1333
-1434
lines changed

‎mock-registry/lib/index.js

+72-3
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,15 @@
1-
const pacote = require('pacote')
21
const Arborist = require('@npmcli/arborist')
3-
const npa = require('npm-package-arg')
42
const Nock = require('nock')
3+
const npa = require('npm-package-arg')
4+
const pacote = require('pacote')
5+
const path = require('node:path')
56
const stringify = require('json-stringify-safe')
67

8+
const { createReadStream } = require('node:fs')
9+
const fs = require('node:fs/promises')
10+
11+
const corgiDoc = 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*'
12+
713
const logReq = (req, ...keys) => {
814
const obj = JSON.parse(stringify(req))
915
const res = {}
@@ -15,6 +21,27 @@ const logReq = (req, ...keys) => {
1521
return stringify(res, null, 2)
1622
}
1723

24+
// helper to convert old audit results to new bulk results
25+
// TODO eventually convert the fixture files themselves
26+
const auditToBulk = audit => {
27+
const bulk = {}
28+
for (const advisory in audit.advisories) {
29+
const {
30+
id,
31+
url,
32+
title,
33+
severity = 'high',
34+
/* eslint-disable-next-line camelcase */
35+
vulnerable_versions = '*',
36+
module_name: name,
37+
} = audit.advisories[advisory]
38+
bulk[name] = bulk[name] || []
39+
/* eslint-disable-next-line camelcase */
40+
bulk[name].push({ id, url, title, severity, vulnerable_versions })
41+
}
42+
return bulk
43+
}
44+
1845
class MockRegistry {
1946
#tap
2047
#nock
@@ -66,14 +93,14 @@ class MockRegistry {
6693
// find mistakes quicker instead of waiting for the entire test to end
6794
t.afterEach((t) => {
6895
t.strictSame(server.pendingMocks(), [], 'no pending mocks after each')
69-
t.strictSame(server.activeMocks(), [], 'no active mocks after each')
7096
})
7197
}
7298

7399
t.teardown(() => {
74100
Nock.enableNetConnect()
75101
server.done()
76102
Nock.emitter.off('no match', noMatch)
103+
Nock.cleanAll()
77104
})
78105

79106
return server
@@ -453,6 +480,48 @@ class MockRegistry {
453480
}
454481
}
455482

483+
// bulk advisory audit endpoint
484+
audit ({ responseCode = 200, results = {}, convert = false, times = 1 } = {}) {
485+
this.nock = this.nock
486+
.post(this.fullPath('/-/npm/v1/security/advisories/bulk'))
487+
.times(times)
488+
.reply(
489+
responseCode,
490+
convert ? auditToBulk(results) : results
491+
)
492+
}
493+
494+
// Used in Arborist to mock the registry from fixture data on disk
495+
// Will eat up all GET requests to the entire registry, so it probably doesn't work with the other GET routes very well.
496+
mocks ({ dir }) {
497+
const exists = (p) => fs.stat(p).then((s) => s).catch(() => false)
498+
this.nock = this.nock.get(/.*/).reply(async function () {
499+
const { headers, path: url } = this.req
500+
const isCorgi = headers.accept.includes('application/vnd.npm.install-v1+json')
501+
const encodedUrl = url.replace(/@/g, '').replace(/%2f/gi, '/')
502+
const f = path.join(dir, 'registry-mocks', 'content', encodedUrl)
503+
let file = f
504+
let contentType = 'application/octet-stream'
505+
if (isCorgi && await exists(`${f}.min.json`)) {
506+
file = `${f}.min.json`
507+
contentType = corgiDoc
508+
} else if (await exists(`${f}.json`)) {
509+
file = `${f}.json`
510+
contentType = 'application/json'
511+
} else if (await exists(`${f}/index.json`)) {
512+
file = `${f}index.json`
513+
contentType = 'application/json'
514+
}
515+
const stats = await exists(file)
516+
if (stats) {
517+
const body = createReadStream(file)
518+
body.pause()
519+
return [200, body, { 'content-type': contentType, 'content-length': stats.size }]
520+
}
521+
return [404, { error: 'not found' }]
522+
}).persist()
523+
}
524+
456525
/**
457526
* this is a simpler convience method for creating mockable registry with
458527
* tarballs for specific versions

‎package-lock.json

+1
Original file line numberDiff line numberDiff line change
@@ -18456,6 +18456,7 @@
1845618456
},
1845718457
"devDependencies": {
1845818458
"@npmcli/eslint-config": "^5.0.1",
18459+
"@npmcli/mock-registry": "^1.0.0",
1845918460
"@npmcli/template-oss": "4.23.3",
1846018461
"benchmark": "^2.1.4",
1846118462
"minify-registry-metadata": "^4.0.0",

‎workspaces/arborist/lib/audit-report.js

+16-86
Original file line numberDiff line numberDiff line change
@@ -274,33 +274,6 @@ class AuditReport extends Map {
274274
throw new Error('do not call AuditReport.set() directly')
275275
}
276276

277-
// convert a quick-audit into a bulk advisory listing
278-
static auditToBulk (report) {
279-
if (!report.advisories) {
280-
// tack on the report json where the response body would go
281-
throw Object.assign(new Error('Invalid advisory report'), {
282-
body: JSON.stringify(report),
283-
})
284-
}
285-
286-
const bulk = {}
287-
const { advisories } = report
288-
for (const advisory of Object.values(advisories)) {
289-
const {
290-
id,
291-
url,
292-
title,
293-
severity = 'high',
294-
vulnerable_versions = '*',
295-
module_name: name,
296-
} = advisory
297-
bulk[name] = bulk[name] || []
298-
bulk[name].push({ id, url, title, severity, vulnerable_versions })
299-
}
300-
301-
return bulk
302-
}
303-
304277
async [_getReport] () {
305278
// if we're not auditing, just return false
306279
if (this.options.audit === false || this.options.offline === true || this.tree.inventory.size === 1) {
@@ -309,39 +282,24 @@ class AuditReport extends Map {
309282

310283
const timeEnd = time.start('auditReport:getReport')
311284
try {
312-
try {
313-
// first try the super fast bulk advisory listing
314-
const body = prepareBulkData(this.tree, this[_omit], this.filterSet)
315-
log.silly('audit', 'bulk request', body)
316-
317-
// no sense asking if we don't have anything to audit,
318-
// we know it'll be empty
319-
if (!Object.keys(body).length) {
320-
return null
321-
}
285+
const body = prepareBulkData(this.tree, this[_omit], this.filterSet)
286+
log.silly('audit', 'bulk request', body)
322287

323-
const res = await fetch('/-/npm/v1/security/advisories/bulk', {
324-
...this.options,
325-
registry: this.options.auditRegistry || this.options.registry,
326-
method: 'POST',
327-
gzip: true,
328-
body,
329-
})
330-
331-
return await res.json()
332-
} catch (er) {
333-
log.silly('audit', 'bulk request failed', String(er.body))
334-
// that failed, try the quick audit endpoint
335-
const body = prepareData(this.tree, this.options)
336-
const res = await fetch('/-/npm/v1/security/audits/quick', {
337-
...this.options,
338-
registry: this.options.auditRegistry || this.options.registry,
339-
method: 'POST',
340-
gzip: true,
341-
body,
342-
})
343-
return AuditReport.auditToBulk(await res.json())
288+
// no sense asking if we don't have anything to audit,
289+
// we know it'll be empty
290+
if (!Object.keys(body).length) {
291+
return null
344292
}
293+
294+
const res = await fetch('/-/npm/v1/security/advisories/bulk', {
295+
...this.options,
296+
registry: this.options.auditRegistry || this.options.registry,
297+
method: 'POST',
298+
gzip: true,
299+
body,
300+
})
301+
302+
return await res.json()
345303
} catch (er) {
346304
log.verbose('audit error', er)
347305
log.silly('audit error', String(er.body))
@@ -384,32 +342,4 @@ const prepareBulkData = (tree, omit, filterSet) => {
384342
return payload
385343
}
386344

387-
const prepareData = (tree, opts) => {
388-
const { npmVersion: npm_version } = opts
389-
const node_version = process.version
390-
const { platform, arch } = process
391-
const { NODE_ENV: node_env } = process.env
392-
const data = tree.meta.commit()
393-
// the legacy audit endpoint doesn't support any kind of pre-filtering
394-
// we just have to get the advisories and skip over them in the report
395-
return {
396-
name: data.name,
397-
version: data.version,
398-
requires: {
399-
...(tree.package.devDependencies || {}),
400-
...(tree.package.peerDependencies || {}),
401-
...(tree.package.optionalDependencies || {}),
402-
...(tree.package.dependencies || {}),
403-
},
404-
dependencies: data.dependencies,
405-
metadata: {
406-
node_version,
407-
npm_version,
408-
platform,
409-
arch,
410-
node_env,
411-
},
412-
}
413-
}
414-
415345
module.exports = AuditReport

‎workspaces/arborist/package.json

+2-1
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
},
4141
"devDependencies": {
4242
"@npmcli/eslint-config": "^5.0.1",
43+
"@npmcli/mock-registry": "^1.0.0",
4344
"@npmcli/template-oss": "4.23.3",
4445
"benchmark": "^2.1.4",
4546
"minify-registry-metadata": "^4.0.0",
@@ -81,7 +82,7 @@
8182
"test-env": [
8283
"LC_ALL=sk"
8384
],
84-
"timeout": "360",
85+
"timeout": "720",
8586
"nyc-arg": [
8687
"--exclude",
8788
"tap-snapshots/**"

‎workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs

+1
Original file line numberDiff line numberDiff line change
@@ -159887,6 +159887,7 @@ ArboristNode {
159887159887
"location": "node_modules/foo",
159888159888
"name": "foo",
159889159889
"path": "{CWD}/test/arborist/tap-testdir-build-ideal-tree-workspaces-should-allow-cyclic-peer-dependencies-between-workspaces-and-packages-from-a-repository/node_modules/foo",
159890+
"resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",
159890159891
"version": "1.0.0",
159891159892
},
159892159893
"workspace-a" => ArboristLink {

‎workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs

+8-5
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ exports[`test/arborist/reify.js TAP add a dep present in the tree, with v1 shrin
169169
{"dependencies":{"once":"^1.4.0","wrappy":"^1.0.2"}}
170170
`
171171

172-
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd > should output a successful tree in mkdirp folder 1`] = `
172+
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd not dry run > should output a successful tree in mkdirp folder 1`] = `
173173
ArboristNode {
174174
"children": Map {
175175
"abbrev" => ArboristNode {
@@ -183,7 +183,7 @@ ArboristNode {
183183
},
184184
"location": "node_modules/abbrev",
185185
"name": "abbrev",
186-
"path": "{CWD}/test/arborist/tap-testdir-reify-add-a-new-pkg-to-a-prefix-that-needs-to-be-mkdirpd/missing/path/to/root/node_modules/abbrev",
186+
"path": "{CWD}/test/arborist/tap-testdir-reify-add-a-new-pkg-to-a-prefix-that-needs-to-be-mkdirpd-not-dry-run/missing/path/to/root/node_modules/abbrev",
187187
"resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
188188
"version": "1.1.1",
189189
},
@@ -199,11 +199,11 @@ ArboristNode {
199199
"isProjectRoot": true,
200200
"location": "",
201201
"name": "root",
202-
"path": "{CWD}/test/arborist/tap-testdir-reify-add-a-new-pkg-to-a-prefix-that-needs-to-be-mkdirpd/missing/path/to/root",
202+
"path": "{CWD}/test/arborist/tap-testdir-reify-add-a-new-pkg-to-a-prefix-that-needs-to-be-mkdirpd-not-dry-run/missing/path/to/root",
203203
}
204204
`
205205

206-
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd > should place expected lockfile file into place 1`] = `
206+
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd not dry run > should place expected lockfile file into place 1`] = `
207207
{
208208
"name": "root",
209209
"lockfileVersion": 3,
@@ -225,7 +225,7 @@ exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be m
225225

226226
`
227227

228-
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd > should place expected package.json file into place 1`] = `
228+
exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be mkdirpd not dry run > should place expected package.json file into place 1`] = `
229229
{
230230
"dependencies": {
231231
"abbrev": "^1.1.1"
@@ -17900,6 +17900,7 @@ Object {
1790017900
"ruy": "bin/index.js",
1790117901
},
1790217902
"integrity": "sha512-VYppDTCM6INWUMKlWiKws4nVMuCNU5h+xjF6lj/0y90rLq017/m8aEpNy4zQSZFV2qz66U/hRZwwlSLJ5l5JMQ==",
17903+
"license": "ISC",
1790317904
"resolved": "https://registry.npmjs.org/ruy/-/ruy-1.0.0.tgz",
1790417905
"version": "1.0.0",
1790517906
},
@@ -33046,6 +33047,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
3304633047
"version": "7.3.2",
3304733048
"resolved": "https://registry.npmjs.org/semver/-/semver-7.3.2.tgz",
3304833049
"integrity": "sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==",
33050+
"license": "ISC",
3304933051
"bin": {
3305033052
"semver": "bin/semver.js"
3305133053
},
@@ -33073,6 +33075,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
3307333075
"version": "7.3.2",
3307433076
"resolved": "https://registry.npmjs.org/semver/-/semver-7.3.2.tgz",
3307533077
"integrity": "sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==",
33078+
"license": "ISC",
3307633079
"bin": {
3307733080
"semver": "bin/semver.js"
3307833081
},

0 commit comments

Comments
 (0)