Skip to content

Commit 2ccb636

Browse files
isaacsisaacs
committed
Handle errors from audit endpoint appropriately
If we're running the 'audit' command, then a failed endpoint means that the command failed. Error out in that case. Otherwise, if it's a quick audit as part of another command, just return a value to indicate that we should not print audit info. This avoids showing '0 vulnerabilities found', which, while amusingly technically correct, is misleading and not very helpful. Fix: #1951 Credit: @isaacs Close: #1956 Reviewed-by: @darcyclarke
1 parent 03fca6a commit 2ccb636

6 files changed

Lines changed: 260 additions & 16 deletions

File tree

‎lib/audit.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ const auditReport = require('npm-audit-report')
33
const npm = require('./npm.js')
44
const output = require('./utils/output.js')
55
const reifyOutput = require('./utils/reify-output.js')
6+
const auditError = require('./utils/audit-error.js')
67

78
const audit = async args => {
89
const arb = new Arborist({
@@ -15,6 +16,8 @@ const audit = async args => {
1516
if (fix) {
1617
reifyOutput(arb)
1718
} else {
19+
// will throw if there's an error, because this is an audit command
20+
auditError(arb.auditReport)
1821
const reporter = npm.flatOptions.json ? 'json' : 'detail'
1922
const result = auditReport(arb.auditReport, {
2023
...npm.flatOptions,

‎lib/utils/audit-error.js‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// print an error or just nothing if the audit report has an error
2+
// this is called by the audit command, and by the reify-output util
3+
// prints a JSON version of the error if it's --json
4+
// returns 'true' if there was an error, false otherwise
5+
6+
const output = require('./output.js')
7+
const npm = require('../npm.js')
8+
const auditError = (report) => {
9+
if (!report || !report.error) {
10+
return false
11+
}
12+
13+
if (npm.command !== 'audit') {
14+
return true
15+
}
16+
17+
const { error } = report
18+
19+
// ok, we care about it, then
20+
npm.log.warn('audit', error.message)
21+
const { body: errBody } = error
22+
const body = Buffer.isBuffer(errBody) ? errBody.toString() : errBody
23+
if (npm.flatOptions.json) {
24+
output(JSON.stringify({
25+
message: error.message,
26+
method: error.method,
27+
uri: error.uri,
28+
headers: error.headers,
29+
statusCode: error.statusCode,
30+
body
31+
}, null, 2))
32+
} else {
33+
output(body)
34+
}
35+
36+
throw 'audit endpoint returned an error'
37+
}
38+
39+
module.exports = auditError

‎lib/utils/reify-output.js‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const { depth } = require('treeverse')
1616
const ms = require('ms')
1717
const auditReport = require('npm-audit-report')
1818
const { readTree: getFundingInfo } = require('libnpmfund')
19+
const auditError = require('./audit-error.js')
1920

2021
// TODO: output JSON if flatOptions.json is true
2122
const reifyOutput = arb => {
@@ -24,13 +25,18 @@ const reifyOutput = arb => {
2425
return
2526
}
2627

27-
const { diff, auditReport, actualTree } = arb
28+
const { diff, actualTree } = arb
29+
30+
// note: fails and crashes if we're running audit fix and there was an error
31+
// which is a good thing, because there's no point printing all this other
32+
// stuff in that case!
33+
const auditReport = auditError(arb.auditReport) ? null : arb.auditReport
2834

2935
const summary = {
3036
added: 0,
3137
removed: 0,
3238
changed: 0,
33-
audited: auditReport ? actualTree.inventory.size : 0,
39+
audited: auditReport && !auditReport.error ? actualTree.inventory.size : 0,
3440
funding: 0
3541
}
3642

@@ -64,32 +70,31 @@ const reifyOutput = arb => {
6470
}
6571

6672
if (npm.flatOptions.json) {
67-
if (arb.auditReport) {
68-
summary.audit = npm.command === 'audit' ? arb.auditReport
69-
: arb.auditReport.toJSON().metadata
73+
if (auditReport) {
74+
summary.audit = npm.command === 'audit' ? auditReport
75+
: auditReport.toJSON().metadata
7076
}
7177
output(JSON.stringify(summary, 0, 2))
7278
} else {
7379
packagesChangedMessage(summary)
7480
packagesFundingMessage(summary)
75-
printAuditReport(arb)
81+
printAuditReport(auditReport)
7682
}
7783
}
7884

7985
// if we're running `npm audit fix`, then we print the full audit report
8086
// at the end if there's still stuff, because it's silly for `npm audit`
8187
// to tell you to run `npm audit` for details. otherwise, use the summary
8288
// report. if we get here, we know it's not quiet or json.
83-
const printAuditReport = arb => {
84-
if (!arb.auditReport) {
89+
const printAuditReport = report => {
90+
if (!report) {
8591
return
8692
}
87-
8893
const reporter = npm.command !== 'audit' ? 'install' : 'detail'
8994
const defaultAuditLevel = npm.command !== 'audit' ? 'none' : 'low'
9095
const auditLevel = npm.flatOptions.auditLevel || defaultAuditLevel
9196

92-
const res = auditReport(arb.auditReport, {
97+
const res = auditReport(report, {
9398
reporter,
9499
...npm.flatOptions,
95100
auditLevel

‎test/lib/audit.js‎

Lines changed: 86 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
const { test } = require('tap')
1+
const t = require('tap')
22
const requireInject = require('require-inject')
33
const audit = require('../../lib/audit.js')
44

5-
test('should audit using Arborist', t => {
5+
t.test('should audit using Arborist', t => {
66
let ARB_ARGS = null
77
let AUDIT_CALLED = false
88
let REIFY_OUTPUT_CALLED = false
@@ -29,6 +29,7 @@ test('should audit using Arborist', t => {
2929
ARB_OBJ = this
3030
this.audit = () => {
3131
AUDIT_CALLED = true
32+
this.auditReport = {}
3233
}
3334
},
3435
'../../lib/utils/reify-output.js': arb => {
@@ -62,7 +63,7 @@ test('should audit using Arborist', t => {
6263
t.end()
6364
})
6465

65-
test('should audit - json', t => {
66+
t.test('should audit - json', t => {
6667
const audit = requireInject('../../lib/audit.js', {
6768
'../../lib/npm.js': {
6869
prefix: 'foo',
@@ -75,7 +76,9 @@ test('should audit - json', t => {
7576
exitCode: 0
7677
}),
7778
'@npmcli/arborist': function () {
78-
this.audit = () => {}
79+
this.audit = () => {
80+
this.auditReport = {}
81+
}
7982
},
8083
'../../lib/utils/reify-output.js': () => {},
8184
'../../lib/utils/output.js': () => {}
@@ -87,7 +90,84 @@ test('should audit - json', t => {
8790
})
8891
})
8992

90-
test('completion', t => {
93+
t.test('report endpoint error', t => {
94+
for (const json of [true, false]) {
95+
t.test(`json=${json}`, t => {
96+
const OUTPUT = []
97+
const LOGS = []
98+
const mocks = {
99+
'../../lib/npm.js': {
100+
prefix: 'foo',
101+
command: 'audit',
102+
flatOptions: {
103+
json
104+
},
105+
log: {
106+
warn: (...warning) => LOGS.push(warning)
107+
}
108+
},
109+
'npm-audit-report': () => {
110+
throw new Error('should not call audit report when there are errors')
111+
},
112+
'@npmcli/arborist': function () {
113+
this.audit = () => {
114+
this.auditReport = {
115+
error: {
116+
message: 'hello, this didnt work',
117+
method: 'POST',
118+
uri: 'https://example.com/',
119+
headers: {
120+
head: ['ers']
121+
},
122+
statusCode: 420,
123+
body: json ? { nope: 'lol' }
124+
: Buffer.from('i had a vuln but i eated it lol')
125+
}
126+
}
127+
}
128+
},
129+
'../../lib/utils/reify-output.js': () => {},
130+
'../../lib/utils/output.js': (...msg) => {
131+
OUTPUT.push(msg)
132+
}
133+
}
134+
// have to pass mocks to both to get the npm and output set right
135+
const auditError = requireInject('../../lib/utils/audit-error.js', mocks)
136+
const audit = requireInject('../../lib/audit.js', {
137+
...mocks,
138+
'../../lib/utils/audit-error.js': auditError
139+
})
140+
141+
audit([], (err) => {
142+
t.equal(err, 'audit endpoint returned an error')
143+
t.strictSame(OUTPUT, [
144+
[
145+
json ? '{\n' +
146+
' "message": "hello, this didnt work",\n' +
147+
' "method": "POST",\n' +
148+
' "uri": "https://example.com/",\n' +
149+
' "headers": {\n' +
150+
' "head": [\n' +
151+
' "ers"\n' +
152+
' ]\n' +
153+
' },\n' +
154+
' "statusCode": 420,\n' +
155+
' "body": {\n' +
156+
' "nope": "lol"\n' +
157+
' }\n' +
158+
'}'
159+
: 'i had a vuln but i eated it lol'
160+
]
161+
])
162+
t.strictSame(LOGS, [['audit', 'hello, this didnt work']])
163+
t.end()
164+
})
165+
})
166+
}
167+
t.end()
168+
})
169+
170+
t.test('completion', t => {
91171
t.test('fix', t => {
92172
audit.completion({
93173
conf: { argv: { remain: ['npm', 'audit'] } }
@@ -117,4 +197,4 @@ test('completion', t => {
117197
})
118198

119199
t.end()
120-
})
200+
})

‎test/lib/utils/audit-error.js‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
const t = require('tap')
2+
const requireInject = require('require-inject')
3+
4+
const LOGS = []
5+
const npm = {
6+
command: null,
7+
flatOptions: {},
8+
log: {
9+
warn: (...msg) => LOGS.push(msg)
10+
}
11+
}
12+
const OUTPUT = []
13+
const output = (...msg) => OUTPUT.push(msg)
14+
const auditError = requireInject('../../../lib/utils/audit-error.js', {
15+
'../../../lib/npm.js': npm,
16+
'../../../lib/utils/output.js': output
17+
})
18+
19+
t.afterEach(cb => {
20+
npm.flatOptions = {}
21+
OUTPUT.length = 0
22+
LOGS.length = 0
23+
cb()
24+
})
25+
26+
t.test('no error, not audit command', t => {
27+
npm.command = 'install'
28+
t.equal(auditError({}), false, 'no error')
29+
t.strictSame(OUTPUT, [], 'no output')
30+
t.strictSame(LOGS, [], 'no warnings')
31+
t.end()
32+
})
33+
34+
t.test('error, not audit command', t => {
35+
npm.command = 'install'
36+
t.equal(auditError({
37+
error: {
38+
message: 'message',
39+
body: Buffer.from('body'),
40+
method: 'POST',
41+
uri: 'https://example.com/not/a/registry',
42+
headers: {
43+
head: ['ers']
44+
},
45+
statusCode: '420'
46+
}
47+
}), true, 'had error')
48+
t.strictSame(OUTPUT, [], 'no output')
49+
t.strictSame(LOGS, [], 'no warnings')
50+
t.end()
51+
})
52+
53+
t.test('error, audit command, not json', t => {
54+
npm.command = 'audit'
55+
npm.flatOptions.json = false
56+
t.throws(() => auditError({
57+
error: {
58+
message: 'message',
59+
body: Buffer.from('body'),
60+
method: 'POST',
61+
uri: 'https://example.com/not/a/registry',
62+
headers: {
63+
head: ['ers']
64+
},
65+
statusCode: '420'
66+
}
67+
}))
68+
69+
t.strictSame(OUTPUT, [ [ 'body' ] ], 'some output')
70+
t.strictSame(LOGS, [ [ 'audit', 'message' ] ], 'some warnings')
71+
t.end()
72+
})
73+
74+
t.test('error, audit command, json', t => {
75+
npm.command = 'audit'
76+
npm.flatOptions.json = true
77+
t.throws(() => auditError({
78+
error: {
79+
message: 'message',
80+
body: { response: 'body' },
81+
method: 'POST',
82+
uri: 'https://example.com/not/a/registry',
83+
headers: {
84+
head: ['ers']
85+
},
86+
statusCode: '420'
87+
}
88+
}))
89+
90+
t.strictSame(OUTPUT, [
91+
[
92+
'{\n' +
93+
' "message": "message",\n' +
94+
' "method": "POST",\n' +
95+
' "uri": "https://example.com/not/a/registry",\n' +
96+
' "headers": {\n' +
97+
' "head": [\n' +
98+
' "ers"\n' +
99+
' ]\n' +
100+
' },\n' +
101+
' "statusCode": "420",\n' +
102+
' "body": {\n' +
103+
' "response": "body"\n' +
104+
' }\n' +
105+
'}'
106+
]
107+
], 'some output')
108+
t.strictSame(LOGS, [ [ 'audit', 'message' ] ], 'some warnings')
109+
t.end()
110+
})

‎test/lib/utils/reify-output.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,13 @@ t.test('single package', (t) => {
7777
)
7878

7979
reifyOutput({
80+
// a report with an error is the same as no report at all, if
81+
// the command is not 'audit'
82+
auditReport: {
83+
error: {
84+
message: 'no audit for youuuuu'
85+
}
86+
},
8087
actualTree: {
8188
name: 'foo',
8289
package: {

0 commit comments

Comments
 (0)