fix: refactor error reporting in audit command

Primary work authored by [@wraithgar](https://github.com/wraithgar).
npm · Feb 13, 2023 · ed59aae · ed59aae
1 parent 5fc6473
commit ed59aae
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 92 deletions.
diff --git a/lib/commands/audit.js b/lib/commands/audit.js
@@ -25,7 +25,6 @@ class VerifySignatures {
     this.checkedPackages = new Set()
     this.auditedWithKeysCount = 0
     this.verifiedCount = 0
-    this.output = []
     this.exitCode = 0
   }
 
@@ -60,13 +59,13 @@ class VerifySignatures {
     const hasNoInvalidOrMissing = invalid.length === 0 && missing.length === 0
 
     if (!hasNoInvalidOrMissing) {
-      this.exitCode = 1
+      process.exitCode = 1
     }
 
     if (this.npm.config.get('json')) {
-      this.appendOutput(JSON.stringify({
-        invalid: this.makeJSON(invalid),
-        missing: this.makeJSON(missing),
+      this.npm.output(JSON.stringify({
+        invalid,
+        missing,
       }, null, 2))
       return
     }
@@ -76,54 +75,65 @@ class VerifySignatures {
     const auditedPlural = this.auditedWithKeysCount > 1 ? 's' : ''
     const timing = `audited ${this.auditedWithKeysCount} package${auditedPlural} in ` +
       `${Math.floor(Number(elapsed) / 1e9)}s`
-    this.appendOutput(`${timing}\n`)
+    this.npm.output(timing)
+    this.npm.output('')
 
     if (this.verifiedCount) {
       const verifiedBold = this.npm.chalk.bold('verified')
-      const msg = this.verifiedCount === 1 ?
-        `${this.verifiedCount} package has a ${verifiedBold} registry signature\n` :
-        `${this.verifiedCount} packages have ${verifiedBold} registry signatures\n`
-      this.appendOutput(msg)
+      if (this.verifiedCount === 1) {
+        this.npm.output(`${this.verifiedCount} package has a ${verifiedBold} registry signature`)
+      } else {
+        this.npm.output(`${this.verifiedCount} packages have ${verifiedBold} registry signatures`)
+      }
+      this.npm.output('')
     }
 
     if (missing.length) {
       const missingClr = this.npm.chalk.bold(this.npm.chalk.red('missing'))
-      const msg = missing.length === 1 ?
-        `package has a ${missingClr} registry signature` :
-        `packages have ${missingClr} registry signatures`
-      this.appendOutput(
-        `${missing.length} ${msg} but the registry is ` +
-        `providing signing keys:\n`
+      if (missing.length === 1) {
+        /* eslint-disable-next-line max-len */
+        this.npm.output(`1 package has a ${missingClr} registry signature but the registry is providing signing keys:`)
+      } else {
+        /* eslint-disable-next-line max-len */
+        this.npm.output(`${missing.length} packages have ${missingClr} registry signatures but the registry is providing signing keys:`)
+      }
+      this.npm.output('')
+      missing.map(m =>
+        this.npm.output(`${this.npm.chalk.red(`${m.name}@${m.version}`)} (${m.registry})`)
       )
-      this.appendOutput(this.humanOutput(missing))
     }
 
     if (invalid.length) {
+      if (missing.length) {
+        this.npm.output('')
+      }
       const invalidClr = this.npm.chalk.bold(this.npm.chalk.red('invalid'))
-      const msg = invalid.length === 1 ?
-        `${invalid.length} package has an ${invalidClr} registry signature:\n` :
-        `${invalid.length} packages have ${invalidClr} registry signatures:\n`
-      this.appendOutput(
-        `${missing.length ? '\n' : ''}${msg}`
+      // We can have either invalid signatures or invalid provenance
+      const invalidSignatures = this.invalid.filter(i => i.code === 'EINTEGRITYSIGNATURE')
+      if (invalidSignatures.length === 1) {
+        this.npm.output(`1 package has an ${invalidClr} registry signature:`)
+      // } else if (invalidSignatures.length > 1) {
+      } else {
+        // TODO move this back to an else if once provenance attestation audit is added
+        /* eslint-disable-next-line max-len */
+        this.npm.output(`${invalidSignatures.length} packages have ${invalidClr} registry signatures:`)
+      }
+      this.npm.output('')
+      invalidSignatures.map(i =>
+        this.npm.output(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`)
       )
-      this.appendOutput(this.humanOutput(invalid))
-      const tamperMsg = invalid.length === 1 ?
-        `\nSomeone might have tampered with this package since it was ` +
-        `published on the registry!\n` :
-        `\nSomeone might have tampered with these packages since they where ` +
-        `published on the registry!\n`
-      this.appendOutput(tamperMsg)
+      this.npm.output('')
+      if (invalid.length === 1) {
+        /* eslint-disable-next-line max-len */
+        this.npm.output(`Someone might have tampered with this package since it was published on the registry!`)
+      } else {
+        /* eslint-disable-next-line max-len */
+        this.npm.output(`Someone might have tampered with these packages since they were published on the registry!`)
+      }
+      this.npm.output('')
     }
   }
 
-  appendOutput (...args) {
-    this.output.push(...args.flat())
-  }
-
-  report () {
-    return { report: this.output.join('\n'), exitCode: this.exitCode }
-  }
-
   getEdgesOut (nodes, filterSet) {
     const edges = new Set()
     const registries = new Set()
@@ -249,11 +259,12 @@ class VerifySignatures {
       ...this.npm.flatOptions,
     })
     const signatures = _signatures || []
-    return {
+    const result = {
       integrity,
       signatures,
       resolved,
     }
+    return result
   }
 
   async getVerifiedInfo (edge) {
@@ -286,51 +297,33 @@ class VerifySignatures {
         this.verifiedCount += 1
       } else if (keys.length) {
         this.missing.push({
-          name,
-          version,
-          location,
-          resolved,
           integrity,
+          location,
+          name,
           registry,
+          resolved,
+          version,
         })
       }
     } catch (e) {
       if (e.code === 'EINTEGRITYSIGNATURE') {
-        const { signature, keyid, integrity, resolved } = e
         this.invalid.push({
+          code: e.code,
+          integrity: e.integrity,
+          keyid: e.keyid,
+          location,
           name,
+          registry,
+          resolved: e.resolved,
+          signature: e.signature,
           type,
           version,
-          resolved,
-          location,
-          integrity,
-          registry,
-          signature,
-          keyid,
         })
       } else {
         throw e
       }
     }
   }
-
-  humanOutput (list) {
-    return list.map(v =>
-      `${this.npm.chalk.red(`${v.name}@${v.version}`)} (${v.registry})`
-    ).join('\n')
-  }
-
-  makeJSON (deps) {
-    return deps.map(d => ({
-      name: d.name,
-      version: d.version,
-      location: d.location,
-      resolved: d.resolved,
-      integrity: d.integrity,
-      signature: d.signature,
-      keyid: d.keyid,
-    }))
-  }
 }
 
 class Audit extends ArboristWorkspaceCmd {
@@ -432,9 +425,6 @@ class Audit extends ArboristWorkspaceCmd {
 
     const verify = new VerifySignatures(tree, filterSet, this.npm, { ...opts })
     await verify.run()
-    const result = verify.report()
-    process.exitCode = process.exitCode || result.exitCode
-    this.npm.output(result.report)
   }
 }
 

diff --git a/tap-snapshots/test/lib/commands/audit.js.test.cjs b/tap-snapshots/test/lib/commands/audit.js.test.cjs
@@ -52,21 +52,25 @@ exports[`test/lib/commands/audit.js TAP audit signatures json output with invali
 {
   "invalid": [
     {
-      "name": "kms-demo",
-      "version": "1.0.0",
+      "code": "EINTEGRITYSIGNATURE",
+      "integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
+      "keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
       "location": "node_modules/kms-demo",
+      "name": "kms-demo",
+      "registry": "https://registry.npmjs.org/",
       "resolved": "https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz",
-      "integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
       "signature": "bogus",
-      "keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA"
+      "type": "dependencies",
+      "version": "1.0.0"
     }
   ],
   "missing": [
     {
-      "name": "async",
-      "version": "1.1.1",
       "location": "node_modules/async",
-      "resolved": "https://registry.npmjs.org/async/-/async-1.1.1.tgz"
+      "name": "async",
+      "registry": "https://registry.npmjs.org/",
+      "resolved": "https://registry.npmjs.org/async/-/async-1.1.1.tgz",
+      "version": "1.1.1"
     }
   ]
 }
@@ -76,13 +80,16 @@ exports[`test/lib/commands/audit.js TAP audit signatures json output with invali
 {
   "invalid": [
     {
-      "name": "kms-demo",
-      "version": "1.0.0",
+      "code": "EINTEGRITYSIGNATURE",
+      "integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
+      "keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
       "location": "node_modules/kms-demo",
+      "name": "kms-demo",
+      "registry": "https://registry.npmjs.org/",
       "resolved": "https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz",
-      "integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
       "signature": "bogus",
-      "keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA"
+      "type": "dependencies",
+      "version": "1.0.0"
     }
   ],
   "missing": []
@@ -204,7 +211,7 @@ audited 2 packages in xxx
 [email protected] (https://registry.npmjs.org/)
 [email protected] (https://registry.npmjs.org/)
 
-Someone might have tampered with these packages since they where published on the registry!
+Someone might have tampered with these packages since they were published on the registry!
 
 `
 

diff --git a/test/lib/commands/audit.js b/test/lib/commands/audit.js
@@ -758,7 +758,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -791,7 +791,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -914,7 +914,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -1095,7 +1095,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), JSON.stringify({ invalid: [], missing: [] }, null, 2))
     t.matchSnapshot(joinedOutput())
   })
@@ -1148,7 +1148,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -1257,7 +1257,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -1401,7 +1401,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 2 packages/)
     t.matchSnapshot(joinedOutput())
   })
@@ -1447,7 +1447,7 @@ t.test('audit signatures', async t => {
 
     await npm.exec('audit', ['signatures'])
 
-    t.equal(process.exitCode, 0, 'should exit successfully')
+    t.notOk(process.exitCode, 'should exit successfully')
     t.match(joinedOutput(), /audited 1 package/)
     t.matchSnapshot(joinedOutput())
   })
@@ -1625,7 +1625,7 @@ t.test('audit signatures', async t => {
 
       await npm.exec('audit', ['signatures'])
 
-      t.equal(process.exitCode, 0, 'should exit successfully')
+      t.notOk(process.exitCode, 'should exit successfully')
       t.match(joinedOutput(), /audited 3 packages/)
       t.matchSnapshot(joinedOutput())
     })
@@ -1678,7 +1678,7 @@ t.test('audit signatures', async t => {
 
       await npm.exec('audit', ['signatures'])
 
-      t.equal(process.exitCode, 0, 'should exit successfully')
+      t.notOk(process.exitCode, 'should exit successfully')
       t.match(joinedOutput(), /audited 2 packages/)
       t.matchSnapshot(joinedOutput())
     })