Skip to content

Commit

Permalink
Handle errors from audit endpoint appropriately
Browse files Browse the repository at this point in the history
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
  • Loading branch information
isaacs committed Oct 15, 2020
1 parent 03fca6a commit 2ccb636
Show file tree
Hide file tree
Showing 6 changed files with 260 additions and 16 deletions.
3 changes: 3 additions & 0 deletions lib/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const auditReport = require('npm-audit-report')
const npm = require('./npm.js')
const output = require('./utils/output.js')
const reifyOutput = require('./utils/reify-output.js')
const auditError = require('./utils/audit-error.js')

const audit = async args => {
const arb = new Arborist({
Expand All @@ -15,6 +16,8 @@ const audit = async args => {
if (fix) {
reifyOutput(arb)
} else {
// will throw if there's an error, because this is an audit command
auditError(arb.auditReport)
const reporter = npm.flatOptions.json ? 'json' : 'detail'
const result = auditReport(arb.auditReport, {
...npm.flatOptions,
Expand Down
39 changes: 39 additions & 0 deletions lib/utils/audit-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// print an error or just nothing if the audit report has an error
// this is called by the audit command, and by the reify-output util
// prints a JSON version of the error if it's --json
// returns 'true' if there was an error, false otherwise

const output = require('./output.js')
const npm = require('../npm.js')
const auditError = (report) => {
if (!report || !report.error) {
return false
}

if (npm.command !== 'audit') {
return true
}

const { error } = report

// ok, we care about it, then
npm.log.warn('audit', error.message)
const { body: errBody } = error
const body = Buffer.isBuffer(errBody) ? errBody.toString() : errBody
if (npm.flatOptions.json) {
output(JSON.stringify({
message: error.message,
method: error.method,
uri: error.uri,
headers: error.headers,
statusCode: error.statusCode,
body
}, null, 2))
} else {
output(body)
}

throw 'audit endpoint returned an error'
}

module.exports = auditError
25 changes: 15 additions & 10 deletions lib/utils/reify-output.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const { depth } = require('treeverse')
const ms = require('ms')
const auditReport = require('npm-audit-report')
const { readTree: getFundingInfo } = require('libnpmfund')
const auditError = require('./audit-error.js')

// TODO: output JSON if flatOptions.json is true
const reifyOutput = arb => {
Expand All @@ -24,13 +25,18 @@ const reifyOutput = arb => {
return
}

const { diff, auditReport, actualTree } = arb
const { diff, actualTree } = arb

// note: fails and crashes if we're running audit fix and there was an error
// which is a good thing, because there's no point printing all this other
// stuff in that case!
const auditReport = auditError(arb.auditReport) ? null : arb.auditReport

const summary = {
added: 0,
removed: 0,
changed: 0,
audited: auditReport ? actualTree.inventory.size : 0,
audited: auditReport && !auditReport.error ? actualTree.inventory.size : 0,
funding: 0
}

Expand Down Expand Up @@ -64,32 +70,31 @@ const reifyOutput = arb => {
}

if (npm.flatOptions.json) {
if (arb.auditReport) {
summary.audit = npm.command === 'audit' ? arb.auditReport
: arb.auditReport.toJSON().metadata
if (auditReport) {
summary.audit = npm.command === 'audit' ? auditReport
: auditReport.toJSON().metadata
}
output(JSON.stringify(summary, 0, 2))
} else {
packagesChangedMessage(summary)
packagesFundingMessage(summary)
printAuditReport(arb)
printAuditReport(auditReport)
}
}

// if we're running `npm audit fix`, then we print the full audit report
// at the end if there's still stuff, because it's silly for `npm audit`
// to tell you to run `npm audit` for details. otherwise, use the summary
// report. if we get here, we know it's not quiet or json.
const printAuditReport = arb => {
if (!arb.auditReport) {
const printAuditReport = report => {
if (!report) {
return
}

const reporter = npm.command !== 'audit' ? 'install' : 'detail'
const defaultAuditLevel = npm.command !== 'audit' ? 'none' : 'low'
const auditLevel = npm.flatOptions.auditLevel || defaultAuditLevel

const res = auditReport(arb.auditReport, {
const res = auditReport(report, {
reporter,
...npm.flatOptions,
auditLevel
Expand Down
92 changes: 86 additions & 6 deletions test/lib/audit.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const { test } = require('tap')
const t = require('tap')
const requireInject = require('require-inject')
const audit = require('../../lib/audit.js')

test('should audit using Arborist', t => {
t.test('should audit using Arborist', t => {
let ARB_ARGS = null
let AUDIT_CALLED = false
let REIFY_OUTPUT_CALLED = false
Expand All @@ -29,6 +29,7 @@ test('should audit using Arborist', t => {
ARB_OBJ = this
this.audit = () => {
AUDIT_CALLED = true
this.auditReport = {}
}
},
'../../lib/utils/reify-output.js': arb => {
Expand Down Expand Up @@ -62,7 +63,7 @@ test('should audit using Arborist', t => {
t.end()
})

test('should audit - json', t => {
t.test('should audit - json', t => {
const audit = requireInject('../../lib/audit.js', {
'../../lib/npm.js': {
prefix: 'foo',
Expand All @@ -75,7 +76,9 @@ test('should audit - json', t => {
exitCode: 0
}),
'@npmcli/arborist': function () {
this.audit = () => {}
this.audit = () => {
this.auditReport = {}
}
},
'../../lib/utils/reify-output.js': () => {},
'../../lib/utils/output.js': () => {}
Expand All @@ -87,7 +90,84 @@ test('should audit - json', t => {
})
})

test('completion', t => {
t.test('report endpoint error', t => {
for (const json of [true, false]) {
t.test(`json=${json}`, t => {
const OUTPUT = []
const LOGS = []
const mocks = {
'../../lib/npm.js': {
prefix: 'foo',
command: 'audit',
flatOptions: {
json
},
log: {
warn: (...warning) => LOGS.push(warning)
}
},
'npm-audit-report': () => {
throw new Error('should not call audit report when there are errors')
},
'@npmcli/arborist': function () {
this.audit = () => {
this.auditReport = {
error: {
message: 'hello, this didnt work',
method: 'POST',
uri: 'https://example.com/',
headers: {
head: ['ers']
},
statusCode: 420,
body: json ? { nope: 'lol' }
: Buffer.from('i had a vuln but i eated it lol')
}
}
}
},
'../../lib/utils/reify-output.js': () => {},
'../../lib/utils/output.js': (...msg) => {
OUTPUT.push(msg)
}
}
// have to pass mocks to both to get the npm and output set right
const auditError = requireInject('../../lib/utils/audit-error.js', mocks)
const audit = requireInject('../../lib/audit.js', {
...mocks,
'../../lib/utils/audit-error.js': auditError
})

audit([], (err) => {
t.equal(err, 'audit endpoint returned an error')
t.strictSame(OUTPUT, [
[
json ? '{\n' +
' "message": "hello, this didnt work",\n' +
' "method": "POST",\n' +
' "uri": "https://example.com/",\n' +
' "headers": {\n' +
' "head": [\n' +
' "ers"\n' +
' ]\n' +
' },\n' +
' "statusCode": 420,\n' +
' "body": {\n' +
' "nope": "lol"\n' +
' }\n' +
'}'
: 'i had a vuln but i eated it lol'
]
])
t.strictSame(LOGS, [['audit', 'hello, this didnt work']])
t.end()
})
})
}
t.end()
})

t.test('completion', t => {
t.test('fix', t => {
audit.completion({
conf: { argv: { remain: ['npm', 'audit'] } }
Expand Down Expand Up @@ -117,4 +197,4 @@ test('completion', t => {
})

t.end()
})
})
110 changes: 110 additions & 0 deletions test/lib/utils/audit-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
const t = require('tap')
const requireInject = require('require-inject')

const LOGS = []
const npm = {
command: null,
flatOptions: {},
log: {
warn: (...msg) => LOGS.push(msg)
}
}
const OUTPUT = []
const output = (...msg) => OUTPUT.push(msg)
const auditError = requireInject('../../../lib/utils/audit-error.js', {
'../../../lib/npm.js': npm,
'../../../lib/utils/output.js': output
})

t.afterEach(cb => {
npm.flatOptions = {}
OUTPUT.length = 0
LOGS.length = 0
cb()
})

t.test('no error, not audit command', t => {
npm.command = 'install'
t.equal(auditError({}), false, 'no error')
t.strictSame(OUTPUT, [], 'no output')
t.strictSame(LOGS, [], 'no warnings')
t.end()
})

t.test('error, not audit command', t => {
npm.command = 'install'
t.equal(auditError({
error: {
message: 'message',
body: Buffer.from('body'),
method: 'POST',
uri: 'https://example.com/not/a/registry',
headers: {
head: ['ers']
},
statusCode: '420'
}
}), true, 'had error')
t.strictSame(OUTPUT, [], 'no output')
t.strictSame(LOGS, [], 'no warnings')
t.end()
})

t.test('error, audit command, not json', t => {
npm.command = 'audit'
npm.flatOptions.json = false
t.throws(() => auditError({
error: {
message: 'message',
body: Buffer.from('body'),
method: 'POST',
uri: 'https://example.com/not/a/registry',
headers: {
head: ['ers']
},
statusCode: '420'
}
}))

t.strictSame(OUTPUT, [ [ 'body' ] ], 'some output')
t.strictSame(LOGS, [ [ 'audit', 'message' ] ], 'some warnings')
t.end()
})

t.test('error, audit command, json', t => {
npm.command = 'audit'
npm.flatOptions.json = true
t.throws(() => auditError({
error: {
message: 'message',
body: { response: 'body' },
method: 'POST',
uri: 'https://example.com/not/a/registry',
headers: {
head: ['ers']
},
statusCode: '420'
}
}))

t.strictSame(OUTPUT, [
[
'{\n' +
' "message": "message",\n' +
' "method": "POST",\n' +
' "uri": "https://example.com/not/a/registry",\n' +
' "headers": {\n' +
' "head": [\n' +
' "ers"\n' +
' ]\n' +
' },\n' +
' "statusCode": "420",\n' +
' "body": {\n' +
' "response": "body"\n' +
' }\n' +
'}'
]
], 'some output')
t.strictSame(LOGS, [ [ 'audit', 'message' ] ], 'some warnings')
t.end()
})
7 changes: 7 additions & 0 deletions test/lib/utils/reify-output.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ t.test('single package', (t) => {
)

reifyOutput({
// a report with an error is the same as no report at all, if
// the command is not 'audit'
auditReport: {
error: {
message: 'no audit for youuuuu'
}
},
actualTree: {
name: 'foo',
package: {
Expand Down

0 comments on commit 2ccb636

Please sign in to comment.