fix: refactor error reporting in audit command

Primary work authored by [@wraithgar](https://github.com/wraithgar).

Author: bdehamer

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,66 @@ 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:`)
+ // Ignoring coverage because this error state can't happen until we have
+ // other kinds of invalid responses, i.e. provenance attestation
+ /* istanbul ignore next */
+ } else if (invalidSignatures.length > 1) {
+ /* 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 +260,12 @@ class VerifySignatures {
  ...this.npm.flatOptions,
  })
  const signatures = _signatures || []
- return {
+ const result = {
  integrity,
  signatures,
  resolved,
  }
+ return result
  }
 
  async getVerifiedInfo (edge) {
@@ -286,51 +298,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 +426,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)
  }
 }
 

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
 async@1.1.1 (https://registry.npmjs.org/)
 kms-demo@1.0.0 (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!
 
 `
 

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