Handle errors from audit endpoint appropriately

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,

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

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,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

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

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

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: {