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: #3311
Credit: @isaacs
Close: #3311
Reviewed-by: @wraithgar

lib/utils/reify-output.js

@@ -18,17 +18,20 @@ 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
   // which is a good thing, because there's no point printing all this other
   // 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 }) => {

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()
 })