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.
npm · Nov 18, 2024 · 88b8e32 · 88b8e32
1 parent 780afc5
commit 88b8e32
Show file tree

Hide file tree

Showing 14 changed files with 1,277 additions and 1,391 deletions.
diff --git a/mock-registry/lib/index.js b/mock-registry/lib/index.js
@@ -1,9 +1,14 @@
-const pacote = require('pacote')
 const Arborist = require('@npmcli/arborist')
-const npa = require('npm-package-arg')
 const Nock = require('nock')
+const npa = require('npm-package-arg')
+const pacote = require('pacote')
+const path = require('node:path')
 const stringify = require('json-stringify-safe')
 
+const fs = require('node:fs/promises')
+
+const corgiDoc = 'application/vnd.npm.install-v1+json; q=1.0, application/json; q=0.8, */*'
+
 const logReq = (req, ...keys) => {
   const obj = JSON.parse(stringify(req))
   const res = {}
@@ -15,6 +20,27 @@ const logReq = (req, ...keys) => {
   return stringify(res, null, 2)
 }
 
+// helper to convert old audit results to new bulk results
+// TODO eventually convert the fixture files themselves
+const auditToBulk = audit => {
+  const bulk = {}
+  for (const advisory in audit.advisories) {
+    const {
+      id,
+      url,
+      title,
+      severity = 'high',
+      /* eslint-disable-next-line camelcase */
+      vulnerable_versions = '*',
+      module_name: name,
+    } = audit.advisories[advisory]
+    bulk[name] = bulk[name] || []
+    /* eslint-disable-next-line camelcase */
+    bulk[name].push({ id, url, title, severity, vulnerable_versions })
+  }
+  return bulk
+}
+
 class MockRegistry {
   #tap
   #nock
@@ -66,14 +92,14 @@ class MockRegistry {
       // find mistakes quicker instead of waiting for the entire test to end
       t.afterEach((t) => {
         t.strictSame(server.pendingMocks(), [], 'no pending mocks after each')
-        t.strictSame(server.activeMocks(), [], 'no active mocks after each')
       })
     }
 
     t.teardown(() => {
       Nock.enableNetConnect()
       server.done()
       Nock.emitter.off('no match', noMatch)
+      Nock.cleanAll()
     })
 
     return server
@@ -453,6 +479,48 @@ class MockRegistry {
     }
   }
 
+  // bulk advisory audit endpoint
+  audit ({ responseCode = 200, results = {}, convert = false, times = 1 } = {}) {
+    this.nock = this.nock
+      .post(this.fullPath('/-/npm/v1/security/advisories/bulk'))
+      .times(times)
+      .reply(
+        responseCode,
+        convert ? auditToBulk(results) : results
+      )
+  }
+
+  // Used in Arborist to mock the registry from fixture data on disk
+  // Will eat up all GET requests to the entire registry, so it probably doesn't work with the other GET routes very well.
+  mocks ({ dir }) {
+    const exists = (p) => fs.stat(p).then((s) => s).catch(() => false)
+    this.nock = this.nock.get(/.*/).reply(async function () {
+      const { headers, path: url } = this.req
+      const isCorgi = headers.accept.includes('application/vnd.npm.install-v1+json')
+      const encodedUrl = url.replace(/@/g, '').replace(/%2f/gi, '/')
+      const f = path.join(dir, 'registry-mocks', 'content', encodedUrl)
+      let file = f
+      let contentType = 'application/octet-stream'
+      if (isCorgi && await exists(`${f}.min.json`)) {
+        file = `${f}.min.json`
+        contentType = corgiDoc
+      } else if (await exists(`${f}.json`)) {
+        file = `${f}.json`
+        contentType = 'application/json'
+      } else if (await exists(`${f}/index.json`)) {
+        file = `${f}index.json`
+        contentType = 'application/json'
+      }
+      const stats = await exists(file)
+      if (stats) {
+        const body = require('node:fs').createReadStream(file)
+        body.pause()
+        return [200, body, { 'content-type': contentType, 'content-length': stats.size }]
+      }
+      return [404, { error: 'not found' }]
+    }).persist()
+  }
+
   /**
    * this is a simpler convience method for creating mockable registry with
    * tarballs for specific versions

diff --git a/package-lock.json b/package-lock.json
@@ -16922,6 +16922,7 @@
       },
       "devDependencies": {
         "@npmcli/eslint-config": "^5.0.1",
+        "@npmcli/mock-registry": "^1.0.0",
         "@npmcli/template-oss": "4.23.3",
         "benchmark": "^2.1.4",
         "minify-registry-metadata": "^4.0.0",

diff --git a/workspaces/arborist/lib/audit-report.js b/workspaces/arborist/lib/audit-report.js
@@ -274,33 +274,6 @@ class AuditReport extends Map {
     throw new Error('do not call AuditReport.set() directly')
   }
 
-  // convert a quick-audit into a bulk advisory listing
-  static auditToBulk (report) {
-    if (!report.advisories) {
-      // tack on the report json where the response body would go
-      throw Object.assign(new Error('Invalid advisory report'), {
-        body: JSON.stringify(report),
-      })
-    }
-
-    const bulk = {}
-    const { advisories } = report
-    for (const advisory of Object.values(advisories)) {
-      const {
-        id,
-        url,
-        title,
-        severity = 'high',
-        vulnerable_versions = '*',
-        module_name: name,
-      } = advisory
-      bulk[name] = bulk[name] || []
-      bulk[name].push({ id, url, title, severity, vulnerable_versions })
-    }
-
-    return bulk
-  }
-
   async [_getReport] () {
     // if we're not auditing, just return false
     if (this.options.audit === false || this.options.offline === true || this.tree.inventory.size === 1) {
@@ -309,39 +282,24 @@ class AuditReport extends Map {
 
     const timeEnd = time.start('auditReport:getReport')
     try {
-      try {
-        // first try the super fast bulk advisory listing
-        const body = prepareBulkData(this.tree, this[_omit], this.filterSet)
-        log.silly('audit', 'bulk request', body)
-
-        // no sense asking if we don't have anything to audit,
-        // we know it'll be empty
-        if (!Object.keys(body).length) {
-          return null
-        }
+      const body = prepareBulkData(this.tree, this[_omit], this.filterSet)
+      log.silly('audit', 'bulk request', body)
 
-        const res = await fetch('/-/npm/v1/security/advisories/bulk', {
-          ...this.options,
-          registry: this.options.auditRegistry || this.options.registry,
-          method: 'POST',
-          gzip: true,
-          body,
-        })
-
-        return await res.json()
-      } catch (er) {
-        log.silly('audit', 'bulk request failed', String(er.body))
-        // that failed, try the quick audit endpoint
-        const body = prepareData(this.tree, this.options)
-        const res = await fetch('/-/npm/v1/security/audits/quick', {
-          ...this.options,
-          registry: this.options.auditRegistry || this.options.registry,
-          method: 'POST',
-          gzip: true,
-          body,
-        })
-        return AuditReport.auditToBulk(await res.json())
+      // no sense asking if we don't have anything to audit,
+      // we know it'll be empty
+      if (!Object.keys(body).length) {
+        return null
       }
+
+      const res = await fetch('/-/npm/v1/security/advisories/bulk', {
+        ...this.options,
+        registry: this.options.auditRegistry || this.options.registry,
+        method: 'POST',
+        gzip: true,
+        body,
+      })
+
+      return await res.json()
     } catch (er) {
       log.verbose('audit error', er)
       log.silly('audit error', String(er.body))
@@ -384,32 +342,4 @@ const prepareBulkData = (tree, omit, filterSet) => {
   return payload
 }
 
-const prepareData = (tree, opts) => {
-  const { npmVersion: npm_version } = opts
-  const node_version = process.version
-  const { platform, arch } = process
-  const { NODE_ENV: node_env } = process.env
-  const data = tree.meta.commit()
-  // the legacy audit endpoint doesn't support any kind of pre-filtering
-  // we just have to get the advisories and skip over them in the report
-  return {
-    name: data.name,
-    version: data.version,
-    requires: {
-      ...(tree.package.devDependencies || {}),
-      ...(tree.package.peerDependencies || {}),
-      ...(tree.package.optionalDependencies || {}),
-      ...(tree.package.dependencies || {}),
-    },
-    dependencies: data.dependencies,
-    metadata: {
-      node_version,
-      npm_version,
-      platform,
-      arch,
-      node_env,
-    },
-  }
-}
-
 module.exports = AuditReport
diff --git a/workspaces/arborist/package.json b/workspaces/arborist/package.json
@@ -41,6 +41,7 @@
   },
   "devDependencies": {
     "@npmcli/eslint-config": "^5.0.1",
+    "@npmcli/mock-registry": "^1.0.0",
     "@npmcli/template-oss": "4.23.3",
     "benchmark": "^2.1.4",
     "minify-registry-metadata": "^4.0.0",
@@ -82,7 +83,7 @@
     "test-env": [
       "LC_ALL=sk"
     ],
-    "timeout": "360",
+    "timeout": "720",
     "nyc-arg": [
       "--exclude",
       "tap-snapshots/**"

diff --git a/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs
@@ -159887,6 +159887,7 @@ ArboristNode {
       "location": "node_modules/foo",
       "name": "foo",
       "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",
+      "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",
       "version": "1.0.0",
     },
     "workspace-a" => ArboristLink {

diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs
@@ -169,7 +169,7 @@ exports[`test/arborist/reify.js TAP add a dep present in the tree, with v1 shrin
 {"dependencies":{"once":"^1.4.0","wrappy":"^1.0.2"}}
 `
 
-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`] = `
+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`] = `
 ArboristNode {
   "children": Map {
     "abbrev" => ArboristNode {
@@ -183,7 +183,7 @@ ArboristNode {
       },
       "location": "node_modules/abbrev",
       "name": "abbrev",
-      "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",
+      "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",
       "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
       "version": "1.1.1",
     },
@@ -199,11 +199,11 @@ ArboristNode {
   "isProjectRoot": true,
   "location": "",
   "name": "root",
-  "path": "{CWD}/test/arborist/tap-testdir-reify-add-a-new-pkg-to-a-prefix-that-needs-to-be-mkdirpd/missing/path/to/root",
+  "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",
 }
 `
 
-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`] = `
+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`] = `
 {
   "name": "root",
   "lockfileVersion": 3,
@@ -225,7 +225,7 @@ exports[`test/arborist/reify.js TAP add a new pkg to a prefix that needs to be m
 
 `
 
-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`] = `
+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`] = `
 {
   "dependencies": {
     "abbrev": "^1.1.1"
@@ -17900,6 +17900,7 @@ Object {
         "ruy": "bin/index.js",
       },
       "integrity": "sha512-VYppDTCM6INWUMKlWiKws4nVMuCNU5h+xjF6lj/0y90rLq017/m8aEpNy4zQSZFV2qz66U/hRZwwlSLJ5l5JMQ==",
+      "license": "ISC",
       "resolved": "https://registry.npmjs.org/ruy/-/ruy-1.0.0.tgz",
       "version": "1.0.0",
     },
@@ -33046,6 +33047,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
       "version": "7.3.2",
       "resolved": "https://registry.npmjs.org/semver/-/semver-7.3.2.tgz",
       "integrity": "sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==",
+      "license": "ISC",
       "bin": {
         "semver": "bin/semver.js"
       },
@@ -33073,6 +33075,7 @@ exports[`test/arborist/reify.js TAP save proper lockfile with bins when upgradin
       "version": "7.3.2",
       "resolved": "https://registry.npmjs.org/semver/-/semver-7.3.2.tgz",
       "integrity": "sha512-OrOb32TeeambH6UrhtShmF7CRDqhL6/5XpPNp2DuRH6+9QLw/orhp72j87v8Qa1ScDkvrrBNpZcDejAirJmfXQ==",
+      "license": "ISC",
       "bin": {
         "semver": "bin/semver.js"
       },