Skip to content

Commit

Permalink
fix: refactor error reporting in audit command
Browse files Browse the repository at this point in the history 
Primary work authored by [@wraithgar](https://github.com/wraithgar).
  • Loading branch information
@bdehamer @wraithgar
bdehamer authored and wraithgar committed Feb 13, 2023
1 parent 1f4c7ea commit 43dd4d8
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 92 deletions.
130 changes: 60 additions & 70 deletions lib/commands/audit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ class VerifySignatures {
this.checkedPackages = new Set()
this.auditedWithKeysCount = 0
this.verifiedCount = 0
this.output = []
this.exitCode = 0
}

Expand Down Expand Up @@ -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
}
Expand All @@ -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()
Expand Down Expand Up @@ -249,11 +259,12 @@ class VerifySignatures {
...this.npm.flatOptions,
})
const signatures = _signatures || []
return {
const result = {
integrity,
signatures,
resolved,
}
return result
}

async getVerifiedInfo (edge) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}

Expand Down
31 changes: 19 additions & 12 deletions tap-snapshots/test/lib/commands/audit.js.test.cjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Expand All @@ -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": []
Expand Down Expand Up @@ -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!
`

Expand Down
20 changes: 10 additions & 10 deletions test/lib/commands/audit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down Expand Up @@ -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())
})
Expand Down

0 comments on commit 43dd4d8

Please sign in to comment.