From 554e8a5cd7034052a59a9ada31e4b8f73712211a Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 26 May 2021 12:30:57 -0700 Subject: [PATCH] fix: set audit exit code properly When running 'npm audit', we properly exited correctly with the appropriate exitCode based on the audit level config and the report results. However, when going through the reifyFinish() function (as we do for 'npm audit fix'), we were not setting that properly if the auditLevel was not set. Furthermore, if the auditLevel WAS set, we were setting the exit code to non-zero for *other* reify commands (install, update, etc.), where the audit information should be strictly advisory. When --json and --loglevel=silent were set, the exitCode was never being set properly. This fixes all these problems. PR-URL: https://github.com/npm/cli/pull/3311 Credit: @isaacs Close: #3311 Reviewed-by: @wraithgar --- lib/utils/reify-output.js | 34 +++++-- test/lib/utils/reify-output.js | 165 ++++++++++++++++++++++++++++----- 2 files changed, 171 insertions(+), 28 deletions(-) diff --git a/lib/utils/reify-output.js b/lib/utils/reify-output.js index ddad32121e8b4..bf3fa7fb2e13d 100644 --- a/lib/utils/reify-output.js +++ b/lib/utils/reify-output.js @@ -18,10 +18,6 @@ const auditError = require('./audit-error.js') // TODO: output JSON if flatOptions.json is true const reifyOutput = (npm, arb) => { - // don't print any info in --silent mode - if (log.levels[log.level] > log.levels.error) - return - const { diff, actualTree } = arb // note: fails and crashes if we're running audit fix and there was an error @@ -29,6 +25,13 @@ const reifyOutput = (npm, arb) => { // stuff in that case! const auditReport = auditError(npm, arb.auditReport) ? null : arb.auditReport + // don't print any info in --silent mode, but we still need to + // set the exitCode properly from the audit report, if we have one. + if (log.levels[log.level] > log.levels.error) { + getAuditReport(npm, auditReport) + return + } + const summary = { added: 0, removed: 0, @@ -68,6 +71,8 @@ const reifyOutput = (npm, arb) => { if (npm.flatOptions.json) { if (auditReport) { + // call this to set the exit code properly + getAuditReport(npm, auditReport) summary.audit = npm.command === 'audit' ? auditReport : auditReport.toJSON().metadata } @@ -83,11 +88,25 @@ const reifyOutput = (npm, 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. +// If the loglevel is set higher than 'error', then we just run the report +// to get the exitCode set appropriately. const printAuditReport = (npm, report) => { + const res = getAuditReport(npm, report) + if (!res || !res.report) + return + npm.output(`\n${res.report}`) +} + +const getAuditReport = (npm, report) => { if (!report) return - const reporter = npm.command !== 'audit' ? 'install' : 'detail' + // when in silent mode, we print nothing. the JSON output is + // going to just JSON.stringify() the report object. + const reporter = log.levels[log.level] > log.levels.error ? 'quiet' + : npm.flatOptions.json ? 'quiet' + : npm.command !== 'audit' ? 'install' + : 'detail' const defaultAuditLevel = npm.command !== 'audit' ? 'none' : 'low' const auditLevel = npm.flatOptions.auditLevel || defaultAuditLevel @@ -96,8 +115,9 @@ const printAuditReport = (npm, report) => { ...npm.flatOptions, auditLevel, }) - process.exitCode = process.exitCode || res.exitCode - npm.output('\n' + res.report) + if (npm.command === 'audit') + process.exitCode = process.exitCode || res.exitCode + return res } const packagesChangedMessage = (npm, { added, removed, changed, audited }) => { diff --git a/test/lib/utils/reify-output.js b/test/lib/utils/reify-output.js index 2142566b90dea..3ffbdf86a2989 100644 --- a/test/lib/utils/reify-output.js +++ b/test/lib/utils/reify-output.js @@ -187,31 +187,154 @@ t.test('print appropriate message for many packages', (t) => { }) }) -t.test('no output when silent', t => { - npm.output = out => { - t.fail('should not get output when silent', { actual: out }) - } - t.teardown(() => log.level = 'warn') - log.level = 'silent' - reifyOutput(npm, { - actualTree: { inventory: { size: 999 }, children: [] }, - auditReport: { - toJSON: () => { - throw new Error('this should not get called') - }, - vulnerabilities: {}, - metadata: { - vulnerabilities: { - total: 99, - }, +t.test('showing and not showing audit report', async t => { + const auditReport = { + toJSON: () => auditReport, + auditReportVersion: 2, + vulnerabilities: { + minimist: { + name: 'minimist', + severity: 'low', + via: [ + { + id: 1179, + url: 'https://npmjs.com/advisories/1179', + title: 'Prototype Pollution', + severity: 'low', + vulnerable_versions: '<0.2.1 || >=1.0.0 <1.2.3', + }, + ], + effects: [], + range: '<0.2.1 || >=1.0.0 <1.2.3', + nodes: [ + 'node_modules/minimist', + ], + fixAvailable: true, }, }, - diff: { - children: [ - { action: 'ADD', ideal: { location: 'loc' } }, - ], + metadata: { + vulnerabilities: { + info: 0, + low: 1, + moderate: 0, + high: 0, + critical: 0, + total: 1, + }, + dependencies: { + prod: 1, + dev: 0, + optional: 0, + peer: 0, + peerOptional: 0, + total: 1, + }, }, + } + + t.test('no output when silent', t => { + npm.output = out => { + t.fail('should not get output when silent', { actual: out }) + } + t.teardown(() => log.level = 'warn') + log.level = 'silent' + reifyOutput(npm, { + actualTree: { inventory: { size: 999 }, children: [] }, + auditReport, + diff: { + children: [ + { action: 'ADD', ideal: { location: 'loc' } }, + ], + }, + }) + t.end() }) + + t.test('output when not silent', t => { + const OUT = [] + npm.output = out => { + OUT.push(out) + } + reifyOutput(npm, { + actualTree: { inventory: new Map(), children: [] }, + auditReport, + diff: { + children: [ + { action: 'ADD', ideal: { location: 'loc' } }, + ], + }, + }) + t.match(OUT.join('\n'), /Run `npm audit` for details\.$/, 'got audit report') + t.end() + }) + + for (const json of [true, false]) { + t.test(`json=${json}`, t => { + t.teardown(() => { + delete npm.flatOptions.json + }) + npm.flatOptions.json = json + t.test('set exit code when cmd is audit', t => { + npm.output = () => {} + const { exitCode } = process + const { command } = npm + npm.flatOptions.auditLevel = 'low' + t.teardown(() => { + delete npm.flatOptions.auditLevel + npm.command = command + // only set exitCode back if we're passing tests + if (t.passing()) + process.exitCode = exitCode + }) + + process.exitCode = 0 + npm.command = 'audit' + reifyOutput(npm, { + actualTree: { inventory: new Map(), children: [] }, + auditReport, + diff: { + children: [ + { action: 'ADD', ideal: { location: 'loc' } }, + ], + }, + }) + + t.equal(process.exitCode, 1, 'set exit code') + t.end() + }) + + t.test('do not set exit code when cmd is install', t => { + npm.output = () => {} + const { exitCode } = process + const { command } = npm + npm.flatOptions.auditLevel = 'low' + t.teardown(() => { + delete npm.flatOptions.auditLevel + npm.command = command + // only set exitCode back if we're passing tests + if (t.passing()) + process.exitCode = exitCode + }) + + process.exitCode = 0 + npm.command = 'install' + reifyOutput(npm, { + actualTree: { inventory: new Map(), children: [] }, + auditReport, + diff: { + children: [ + { action: 'ADD', ideal: { location: 'loc' } }, + ], + }, + }) + + t.equal(process.exitCode, 0, 'did not set exit code') + t.end() + }) + t.end() + }) + } + t.end() })