Skip to content

Commit 43dd4d8

Browse files
bdehamerwraithgar
bdehamer
authored and
wraithgar
committed
fix: refactor error reporting in audit command
Primary work authored by [@wraithgar](https://github.com/wraithgar).
1 parent 1f4c7ea commit 43dd4d8

File tree

3 files changed

+89
-92
lines changed

3 files changed

+89
-92
lines changed

‎lib/commands/audit.js

+60-70
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ class VerifySignatures {
2525
this.checkedPackages = new Set()
2626
this.auditedWithKeysCount = 0
2727
this.verifiedCount = 0
28-
this.output = []
2928
this.exitCode = 0
3029
}
3130

@@ -60,13 +59,13 @@ class VerifySignatures {
6059
const hasNoInvalidOrMissing = invalid.length === 0 && missing.length === 0
6160

6261
if (!hasNoInvalidOrMissing) {
63-
this.exitCode = 1
62+
process.exitCode = 1
6463
}
6564

6665
if (this.npm.config.get('json')) {
67-
this.appendOutput(JSON.stringify({
68-
invalid: this.makeJSON(invalid),
69-
missing: this.makeJSON(missing),
66+
this.npm.output(JSON.stringify({
67+
invalid,
68+
missing,
7069
}, null, 2))
7170
return
7271
}
@@ -76,54 +75,65 @@ class VerifySignatures {
7675
const auditedPlural = this.auditedWithKeysCount > 1 ? 's' : ''
7776
const timing = `audited ${this.auditedWithKeysCount} package${auditedPlural} in ` +
7877
`${Math.floor(Number(elapsed) / 1e9)}s`
79-
this.appendOutput(`${timing}\n`)
78+
this.npm.output(timing)
79+
this.npm.output('')
8080

8181
if (this.verifiedCount) {
8282
const verifiedBold = this.npm.chalk.bold('verified')
83-
const msg = this.verifiedCount === 1 ?
84-
`${this.verifiedCount} package has a ${verifiedBold} registry signature\n` :
85-
`${this.verifiedCount} packages have ${verifiedBold} registry signatures\n`
86-
this.appendOutput(msg)
83+
if (this.verifiedCount === 1) {
84+
this.npm.output(`${this.verifiedCount} package has a ${verifiedBold} registry signature`)
85+
} else {
86+
this.npm.output(`${this.verifiedCount} packages have ${verifiedBold} registry signatures`)
87+
}
88+
this.npm.output('')
8789
}
8890

8991
if (missing.length) {
9092
const missingClr = this.npm.chalk.bold(this.npm.chalk.red('missing'))
91-
const msg = missing.length === 1 ?
92-
`package has a ${missingClr} registry signature` :
93-
`packages have ${missingClr} registry signatures`
94-
this.appendOutput(
95-
`${missing.length} ${msg} but the registry is ` +
96-
`providing signing keys:\n`
93+
if (missing.length === 1) {
94+
/* eslint-disable-next-line max-len */
95+
this.npm.output(`1 package has a ${missingClr} registry signature but the registry is providing signing keys:`)
96+
} else {
97+
/* eslint-disable-next-line max-len */
98+
this.npm.output(`${missing.length} packages have ${missingClr} registry signatures but the registry is providing signing keys:`)
99+
}
100+
this.npm.output('')
101+
missing.map(m =>
102+
this.npm.output(`${this.npm.chalk.red(`${m.name}@${m.version}`)} (${m.registry})`)
97103
)
98-
this.appendOutput(this.humanOutput(missing))
99104
}
100105

101106
if (invalid.length) {
107+
if (missing.length) {
108+
this.npm.output('')
109+
}
102110
const invalidClr = this.npm.chalk.bold(this.npm.chalk.red('invalid'))
103-
const msg = invalid.length === 1 ?
104-
`${invalid.length} package has an ${invalidClr} registry signature:\n` :
105-
`${invalid.length} packages have ${invalidClr} registry signatures:\n`
106-
this.appendOutput(
107-
`${missing.length ? '\n' : ''}${msg}`
111+
// We can have either invalid signatures or invalid provenance
112+
const invalidSignatures = this.invalid.filter(i => i.code === 'EINTEGRITYSIGNATURE')
113+
if (invalidSignatures.length === 1) {
114+
this.npm.output(`1 package has an ${invalidClr} registry signature:`)
115+
// } else if (invalidSignatures.length > 1) {
116+
} else {
117+
//TODO move this back to an else if once provenance attestation audit is added
118+
/* eslint-disable-next-line max-len */
119+
this.npm.output(`${invalidSignatures.length} packages have ${invalidClr} registry signatures:`)
120+
}
121+
this.npm.output('')
122+
invalidSignatures.map(i =>
123+
this.npm.output(`${this.npm.chalk.red(`${i.name}@${i.version}`)} (${i.registry})`)
108124
)
109-
this.appendOutput(this.humanOutput(invalid))
110-
const tamperMsg = invalid.length === 1 ?
111-
`\nSomeone might have tampered with this package since it was ` +
112-
`published on the registry!\n` :
113-
`\nSomeone might have tampered with these packages since they where ` +
114-
`published on the registry!\n`
115-
this.appendOutput(tamperMsg)
125+
this.npm.output('')
126+
if (invalid.length === 1) {
127+
/* eslint-disable-next-line max-len */
128+
this.npm.output(`Someone might have tampered with this package since it was published on the registry!`)
129+
} else {
130+
/* eslint-disable-next-line max-len */
131+
this.npm.output(`Someone might have tampered with these packages since they were published on the registry!`)
132+
}
133+
this.npm.output('')
116134
}
117135
}
118136

119-
appendOutput (...args) {
120-
this.output.push(...args.flat())
121-
}
122-
123-
report () {
124-
return { report: this.output.join('\n'), exitCode: this.exitCode }
125-
}
126-
127137
getEdgesOut (nodes, filterSet) {
128138
const edges = new Set()
129139
const registries = new Set()
@@ -249,11 +259,12 @@ class VerifySignatures {
249259
...this.npm.flatOptions,
250260
})
251261
const signatures = _signatures || []
252-
return {
262+
const result = {
253263
integrity,
254264
signatures,
255265
resolved,
256266
}
267+
return result
257268
}
258269

259270
async getVerifiedInfo (edge) {
@@ -286,51 +297,33 @@ class VerifySignatures {
286297
this.verifiedCount += 1
287298
} else if (keys.length) {
288299
this.missing.push({
289-
name,
290-
version,
291-
location,
292-
resolved,
293300
integrity,
301+
location,
302+
name,
294303
registry,
304+
resolved,
305+
version,
295306
})
296307
}
297308
} catch (e) {
298309
if (e.code === 'EINTEGRITYSIGNATURE') {
299-
const { signature, keyid, integrity, resolved } = e
300310
this.invalid.push({
311+
code: e.code,
312+
integrity: e.integrity,
313+
keyid: e.keyid,
314+
location,
301315
name,
316+
registry,
317+
resolved: e.resolved,
318+
signature: e.signature,
302319
type,
303320
version,
304-
resolved,
305-
location,
306-
integrity,
307-
registry,
308-
signature,
309-
keyid,
310321
})
311322
} else {
312323
throw e
313324
}
314325
}
315326
}
316-
317-
humanOutput (list) {
318-
return list.map(v =>
319-
`${this.npm.chalk.red(`${v.name}@${v.version}`)} (${v.registry})`
320-
).join('\n')
321-
}
322-
323-
makeJSON (deps) {
324-
return deps.map(d => ({
325-
name: d.name,
326-
version: d.version,
327-
location: d.location,
328-
resolved: d.resolved,
329-
integrity: d.integrity,
330-
signature: d.signature,
331-
keyid: d.keyid,
332-
}))
333-
}
334327
}
335328

336329
class Audit extends ArboristWorkspaceCmd {
@@ -432,9 +425,6 @@ class Audit extends ArboristWorkspaceCmd {
432425

433426
const verify = new VerifySignatures(tree, filterSet, this.npm, { ...opts })
434427
await verify.run()
435-
const result = verify.report()
436-
process.exitCode = process.exitCode || result.exitCode
437-
this.npm.output(result.report)
438428
}
439429
}
440430

‎tap-snapshots/test/lib/commands/audit.js.test.cjs

+19-12
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,25 @@ exports[`test/lib/commands/audit.js TAP audit signatures json output with invali
5252
{
5353
"invalid": [
5454
{
55-
"name": "kms-demo",
56-
"version": "1.0.0",
55+
"code": "EINTEGRITYSIGNATURE",
56+
"integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
57+
"keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
5758
"location": "node_modules/kms-demo",
59+
"name": "kms-demo",
60+
"registry": "https://registry.npmjs.org/",
5861
"resolved": "https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz",
59-
"integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
6062
"signature": "bogus",
61-
"keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA"
63+
"type": "dependencies",
64+
"version": "1.0.0"
6265
}
6366
],
6467
"missing": [
6568
{
66-
"name": "async",
67-
"version": "1.1.1",
6869
"location": "node_modules/async",
69-
"resolved": "https://registry.npmjs.org/async/-/async-1.1.1.tgz"
70+
"name": "async",
71+
"registry": "https://registry.npmjs.org/",
72+
"resolved": "https://registry.npmjs.org/async/-/async-1.1.1.tgz",
73+
"version": "1.1.1"
7074
}
7175
]
7276
}
@@ -76,13 +80,16 @@ exports[`test/lib/commands/audit.js TAP audit signatures json output with invali
7680
{
7781
"invalid": [
7882
{
79-
"name": "kms-demo",
80-
"version": "1.0.0",
83+
"code": "EINTEGRITYSIGNATURE",
84+
"integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
85+
"keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA",
8186
"location": "node_modules/kms-demo",
87+
"name": "kms-demo",
88+
"registry": "https://registry.npmjs.org/",
8289
"resolved": "https://registry.npmjs.org/kms-demo/-/kms-demo-1.0.0.tgz",
83-
"integrity": "sha512-QqZ7VJ/8xPkS9s2IWB7Shj3qTJdcRyeXKbPQnsZjsPEwvutGv0EGeVchPcauoiDFJlGbZMFq5GDCurAGNSghJQ==",
8490
"signature": "bogus",
85-
"keyid": "SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA"
91+
"type": "dependencies",
92+
"version": "1.0.0"
8693
}
8794
],
8895
"missing": []
@@ -204,7 +211,7 @@ audited 2 packages in xxx
204211
[email protected] (https://registry.npmjs.org/)
205212
[email protected] (https://registry.npmjs.org/)
206213
207-
Someone might have tampered with these packages since they where published on the registry!
214+
Someone might have tampered with these packages since they were published on the registry!
208215
209216
`
210217

‎test/lib/commands/audit.js

+10-10
Original file line numberDiff line numberDiff line change
@@ -758,7 +758,7 @@ t.test('audit signatures', async t => {
758758

759759
await npm.exec('audit', ['signatures'])
760760

761-
t.equal(process.exitCode, 0, 'should exit successfully')
761+
t.notOk(process.exitCode, 'should exit successfully')
762762
t.match(joinedOutput(), /audited 1 package/)
763763
t.matchSnapshot(joinedOutput())
764764
})
@@ -791,7 +791,7 @@ t.test('audit signatures', async t => {
791791

792792
await npm.exec('audit', ['signatures'])
793793

794-
t.equal(process.exitCode, 0, 'should exit successfully')
794+
t.notOk(process.exitCode, 'should exit successfully')
795795
t.match(joinedOutput(), /audited 1 package/)
796796
t.matchSnapshot(joinedOutput())
797797
})
@@ -914,7 +914,7 @@ t.test('audit signatures', async t => {
914914

915915
await npm.exec('audit', ['signatures'])
916916

917-
t.equal(process.exitCode, 0, 'should exit successfully')
917+
t.notOk(process.exitCode, 'should exit successfully')
918918
t.match(joinedOutput(), /audited 1 package/)
919919
t.matchSnapshot(joinedOutput())
920920
})
@@ -1095,7 +1095,7 @@ t.test('audit signatures', async t => {
10951095

10961096
await npm.exec('audit', ['signatures'])
10971097

1098-
t.equal(process.exitCode, 0, 'should exit successfully')
1098+
t.notOk(process.exitCode, 'should exit successfully')
10991099
t.match(joinedOutput(), JSON.stringify({ invalid: [], missing: [] }, null, 2))
11001100
t.matchSnapshot(joinedOutput())
11011101
})
@@ -1148,7 +1148,7 @@ t.test('audit signatures', async t => {
11481148

11491149
await npm.exec('audit', ['signatures'])
11501150

1151-
t.equal(process.exitCode, 0, 'should exit successfully')
1151+
t.notOk(process.exitCode, 'should exit successfully')
11521152
t.match(joinedOutput(), /audited 1 package/)
11531153
t.matchSnapshot(joinedOutput())
11541154
})
@@ -1257,7 +1257,7 @@ t.test('audit signatures', async t => {
12571257

12581258
await npm.exec('audit', ['signatures'])
12591259

1260-
t.equal(process.exitCode, 0, 'should exit successfully')
1260+
t.notOk(process.exitCode, 'should exit successfully')
12611261
t.match(joinedOutput(), /audited 1 package/)
12621262
t.matchSnapshot(joinedOutput())
12631263
})
@@ -1401,7 +1401,7 @@ t.test('audit signatures', async t => {
14011401

14021402
await npm.exec('audit', ['signatures'])
14031403

1404-
t.equal(process.exitCode, 0, 'should exit successfully')
1404+
t.notOk(process.exitCode, 'should exit successfully')
14051405
t.match(joinedOutput(), /audited 2 packages/)
14061406
t.matchSnapshot(joinedOutput())
14071407
})
@@ -1447,7 +1447,7 @@ t.test('audit signatures', async t => {
14471447

14481448
await npm.exec('audit', ['signatures'])
14491449

1450-
t.equal(process.exitCode, 0, 'should exit successfully')
1450+
t.notOk(process.exitCode, 'should exit successfully')
14511451
t.match(joinedOutput(), /audited 1 package/)
14521452
t.matchSnapshot(joinedOutput())
14531453
})
@@ -1625,7 +1625,7 @@ t.test('audit signatures', async t => {
16251625

16261626
await npm.exec('audit', ['signatures'])
16271627

1628-
t.equal(process.exitCode, 0, 'should exit successfully')
1628+
t.notOk(process.exitCode, 'should exit successfully')
16291629
t.match(joinedOutput(), /audited 3 packages/)
16301630
t.matchSnapshot(joinedOutput())
16311631
})
@@ -1678,7 +1678,7 @@ t.test('audit signatures', async t => {
16781678

16791679
await npm.exec('audit', ['signatures'])
16801680

1681-
t.equal(process.exitCode, 0, 'should exit successfully')
1681+
t.notOk(process.exitCode, 'should exit successfully')
16821682
t.match(joinedOutput(), /audited 2 packages/)
16831683
t.matchSnapshot(joinedOutput())
16841684
})

0 commit comments

Comments
 (0)