Permalink
Browse files

fix(errors): include request data on unexpected errors

  • Loading branch information...
philbooth committed Feb 8, 2019
1 parent 4c5862c commit a0ac71c7cc0f4624ac5564cdbea95ead5237095f
Showing with 108 additions and 17 deletions.
  1. +0 −2 docs/api.md
  2. +29 −7 lib/error.js
  3. +1 −1 lib/server.js
  4. +77 −6 test/local/error.js
  5. +1 −1 test/local/server.js
@@ -312,8 +312,6 @@ for `code` and `errno` are:
A backend service request failed.
* `code: 500, errno: 998`:
An internal validation check failed.
* `code: 500, errno: 999`:
Unspecified error

The following errors
include additional response properties:
@@ -86,17 +86,17 @@ var ERRNO = {
UNEXPECTED_ERROR: 999
}

var DEFAULTS = {
const DEFAULTS = {
code: 500,
error: 'Internal Server Error',
errno: ERRNO.UNEXPECTED_ERROR,
message: 'Unspecified error',
info: 'https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format'
}

var TOO_LARGE = /^Payload (?:content length|size) greater than maximum allowed/
const TOO_LARGE = /^Payload (?:content length|size) greater than maximum allowed/

var BAD_SIGNATURE_ERRORS = [
const BAD_SIGNATURE_ERRORS = [
'Bad mac',
'Unknown algorithm',
'Missing required payload hash',
@@ -144,15 +144,15 @@ AppError.prototype.backtrace = function (traced) {
/**
Translates an error from Hapi format to our format
*/
AppError.translate = function (response) {
AppError.translate = function (request, response) {
var error
if (response instanceof AppError) {
return response
}
var payload = response.output.payload
var reason = response.reason
if (! payload) {
error = new AppError({})
error = AppError.unexpectedError(request)
} else if (payload.statusCode === 500 && /(socket hang up|ECONNREFUSED)/.test(reason)) {
// A connection to a remote service either was not made or timed out.
error = AppError.backendServiceFailure()
@@ -194,6 +194,9 @@ AppError.translate = function (response) {
info: payload.info,
stack: response.stack
})
if (payload.statusCode >= 500) {
decorateErrorWithRequest(error, request)
}
}
return error
}
@@ -898,9 +901,28 @@ AppError.internalValidationError = (op, data) => {
})
}

AppError.unexpectedError = () => {
return new AppError({})
AppError.unexpectedError = request => {
const error = new AppError({})
decorateErrorWithRequest(error, request)
return error
}

module.exports = AppError
module.exports.ERRNO = ERRNO

function decorateErrorWithRequest (error, request) {
if (request) {
error.output.payload.request = {
// request.app.devices and request.app.metricsContext are async, so can't be included here
acceptLanguage: request.app.acceptLanguage,
locale: request.app.locale,
geo: request.app.geo,
userAgent: request.app.ua,
method: request.method,
path: request.path,
query: request.query,
payload: request.payload,
headers: request.headers
}
}
}
@@ -250,7 +250,7 @@ async function create (log, error, config, routes, db, oauthdb, translator) {
let response = request.response
if (response.isBoom) {
logEndpointErrors(response, log)
response = error.translate(response)
response = error.translate(request, response)
if (config.env !== 'prod') {
response.backtrace(request.app.traced)
}
@@ -5,8 +5,9 @@
'use strict'

const { assert } = require('chai')
var messages = require('joi/lib/language')
const messages = require('joi/lib/language')
const AppError = require('../../lib/error')
const P = require('../../lib/promise')

describe('AppErrors', () => {

@@ -24,7 +25,7 @@ describe('AppErrors', () => {
assert.equal(typeof AppError, 'function')
assert.equal(AppError.length, 3)
assert.equal(typeof AppError.translate, 'function')
assert.equal(AppError.translate.length, 1)
assert.lengthOf(AppError.translate, 2)
assert.equal(typeof AppError.invalidRequestParameter, 'function')
assert.equal(AppError.invalidRequestParameter.length, 1)
assert.equal(typeof AppError.missingRequestParameter, 'function')
@@ -35,7 +36,7 @@ describe('AppErrors', () => {
it(
'should translate with missing required parameters',
() => {
var result = AppError.translate({
var result = AppError.translate(null, {
output: {
payload: {
message: 'foo' + messages.errors.any.required,
@@ -59,7 +60,7 @@ describe('AppErrors', () => {
it(
'should translate with invalid parameter',
() => {
var result = AppError.translate({
var result = AppError.translate(null, {
output: {
payload: {
validation: 'foo'
@@ -80,7 +81,7 @@ describe('AppErrors', () => {
it(
'should translate with missing payload',
() => {
var result = AppError.translate({
var result = AppError.translate(null, {
output: {}
})
assert.ok(result instanceof AppError, 'instanceof AppError')
@@ -112,10 +113,80 @@ describe('AppErrors', () => {
}
)

it('unexpectedError without request data', () => {
const err = AppError.unexpectedError()
assert.instanceOf(err, AppError)
assert.instanceOf(err, Error)
assert.equal(err.errno, 999)
assert.equal(err.message, 'Unspecified error')
assert.equal(err.output.statusCode, 500)
assert.equal(err.output.payload.error, 'Internal Server Error')
assert.isUndefined(err.output.payload.request)
})

it('unexpectedError with request data', () => {
const err = AppError.unexpectedError({
app: {
acceptLanguage: 'en, fr',
locale: 'en',
geo: {
city: 'Mountain View',
state: 'California'
},
ua: {
os: 'Android',
osVersion: '9'
},
devices: P.resolve([ { id: 1 } ]),
metricsContext: P.resolve({
service: 'sync'
})
},
method: 'GET',
path: '/v1/wibble',
query: {
foo: 'bar'
},
payload: {
baz: 'qux'
},
headers: {
wibble: 'blee'
}
})
assert.equal(err.errno, 999)
assert.equal(err.message, 'Unspecified error')
assert.equal(err.output.statusCode, 500)
assert.equal(err.output.payload.error, 'Internal Server Error')
assert.deepEqual(err.output.payload.request, {
acceptLanguage: 'en, fr',
locale: 'en',
geo: {
city: 'Mountain View',
state: 'California'
},
userAgent: {
os: 'Android',
osVersion: '9'
},
method: 'GET',
path: '/v1/wibble',
query: {
foo: 'bar'
},
payload: {
baz: 'qux'
},
headers: {
wibble: 'blee'
}
})
})

const reasons = ['socket hang up', 'ECONNREFUSED'];
reasons.forEach((reason) => {
it(`converts ${reason} errors to backend service error`, () => {
const result = AppError.translate({
const result = AppError.translate(null, {
output: {
payload: {
errno: 999,
@@ -389,7 +389,7 @@ describe('lib/server', () => {
})

it('called log.error correctly', () => {
assert.equal(log.error.callCount, 1)
assert.isAtLeast(log.error.callCount, 1)
const args = log.error.args[0]
assert.equal(args.length, 1)
assert.deepEqual(args[0], {

0 comments on commit a0ac71c

Please sign in to comment.