From 2ccb63659f9a757201658d5d019099b492d04a5b Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 13 Oct 2020 16:12:24 -0700 Subject: [PATCH] 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 --- lib/audit.js | 3 + lib/utils/audit-error.js | 39 ++++++++++++ lib/utils/reify-output.js | 25 +++++--- test/lib/audit.js | 92 +++++++++++++++++++++++++-- test/lib/utils/audit-error.js | 110 +++++++++++++++++++++++++++++++++ test/lib/utils/reify-output.js | 7 +++ 6 files changed, 260 insertions(+), 16 deletions(-) create mode 100644 lib/utils/audit-error.js create mode 100644 test/lib/utils/audit-error.js diff --git a/lib/audit.js b/lib/audit.js index ed2db91dd28cd..30c043fa03d4d 100644 --- a/lib/audit.js +++ b/lib/audit.js @@ -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({ @@ -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, diff --git a/lib/utils/audit-error.js b/lib/utils/audit-error.js new file mode 100644 index 0000000000000..91e880c46c846 --- /dev/null +++ b/lib/utils/audit-error.js @@ -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 diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index 09b31fa9413a6..10b276cd9f818 100644 --- a/lib/utils/reify-output.js +++ b/lib/utils/reify-output.js @@ -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 => { @@ -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 } @@ -64,15 +70,15 @@ 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) } } @@ -80,16 +86,15 @@ const reifyOutput = arb => { // 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 diff --git a/test/lib/audit.js b/test/lib/audit.js index cbbbcf56fc99d..4918cb2fc2711 100644 --- a/test/lib/audit.js +++ b/test/lib/audit.js @@ -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 @@ -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 => { @@ -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', @@ -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': () => {} @@ -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'] } } @@ -117,4 +197,4 @@ test('completion', t => { }) t.end() -}) \ No newline at end of file +}) diff --git a/test/lib/utils/audit-error.js b/test/lib/utils/audit-error.js new file mode 100644 index 0000000000000..f183a16e8d005 --- /dev/null +++ b/test/lib/utils/audit-error.js @@ -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() +}) diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js index 92a53707f7116..55f77f1d9d3a7 100644 --- a/test/lib/utils/reify-output.js +++ b/test/lib/utils/reify-output.js @@ -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: {