Error handling in production [EPIC PLACEHOLDER]

[bajtos]

Per the discussion in expressjs/errorhandler#13, we should not use errorhandler module in production:

The PR #1502 is only a false sense of security; yes, the stack property will no longer be included, but it will still expose anything util.inspect will provide for non-Error objects.
(...)
There still remains a big problem with using this in production: the err.message will be exposed still, which will contain a full path if that err came from fs.readFile, or a hostname if that came from a net operation in the newer io.js builds, etc.
(...)
In reality, only the finalhandler module provides suitable out-of-the-box production error support (and you get that by... not adding an error handler to your express app, which is what the example in this module's README does).

For example, we can use api-error-handler for JSON API requests/responses in strong-remoting.

I am creating this GH issue as a place where we can discuss possible approaches and agree on a solution to implement.

/to @ritch @raymondfeng
/cc @fabien @clarkorz

---

Subtasks

• Create a new module strong-error-handler with the usual scaffolding (jscs and jshint, mocha + chai for unit tests, etc.). License: dual MIT + SL
• Implement the middleware per the proposal below, include extensive test suite
• Rework strong-remoting to use this new error handler. Consider how to support existing config options. (Optionally, we may defer this change for strong-remoting 3.0.)
• Modify the template in loopback-workspace to setup this new error handling middleware in scaffolded apps
• Write a documentation draft
• Write a migration guide for existing apps

[bajtos]

General requirements:

• Support at least HTML (templated + static files) and JSON response formats, plain-text and XML are nice to have.
• A config option to enable/disable insecure development mode (see errorhandler module) where sensitive info like stack traces is included in response payload.
• A config option to enable/disable logging of errors stderr. This is just for convenience - by default, LoopBack app can be configured to log to stderr. Users wishing to use a specialised module like express-bunyan-logger or express-winston can turn this off. Error logging can be turned off when running the unit-tests too.

Security considerations

• If a user passes down a non-error object, calling util.inspect (Object.keys) on it will reveal a lot of internal details, which are similar in nature to a stack trace.
• Node.js core errors like "file not found error" include file paths in the message. While it's useful for troubleshooting errors, exposing paths or host+port information in production may create a security vulnerability.
• The client does not have much choice when it comes to handling 5xx errors, therefore we can strip all information (including error message) from the response body. This rule works well for Node.js core errors like "file not found" too - they are treated as 500 Internal Error and thus any sensitive details in the error message will not be disclosed to the client.
• 4xx errors are different. For example, validation errors include details that are necessary for the client to render the errors in the UI and which must be preserved in the response. ATM, LoopBack prescribes to store this information in "err.details" field.

Proposal

Implement a new middleware module called loopback-error-handler accepting the following config:

{
  // enable the development mode?
  // In dev, all error properties (including) stack traces are sent in the response
  debug: true,

  // log errors to stderr?
  log: true,

  // Array of fields (custom Error properties) that are safe to include
  // in response messages (both 4xx and 5xx).
  // "details" property is always included in 4xx responses (not in 5xx)
  safeFields: ['errorCode'],

  // the default type to use when there is no (recognised) Accept header
  // use 'html' for websites, 'json' for API servers
  defaultType: 'json' // or 'html' or 'text' or 'xml'

  // map of templates to use for rendering responses
  views: {
    // the default view
    default: path.resolve(__dirname, 'views/error-default.jade'),
    404: path.resolve(__dirname, 'views/error-not-found.jade'),
    // etc.
  },

  // map of static files to send as a response
  files: {
    // default: path.resolve(__dirname, 'public/error.html'),
    401: path.resolve(__dirname, 'views/error-unauthorized.html'),
    // etc.
  },

  // a custom JSON serializer function for producing JSON response bodies
  // @param sanitizedData: response data containing only safe properties
  // @param originalError: the original Error object
  jsonSerializer: function(sanitizedData, originalError) {
    if (originalError.name === 'ValidationError') {
      var details = sanitizedData.details || {};
      sanitizedData.issueCount =  details.codes && Object.keys(details.codes).length;
    }
    return sanitizedData;
  }
}

JSON response fields

1. debug:true - all error fields are sent in the HTTP response
2. debug:false & status code is 4xx - the error object contains the following fields:
   • name
   • message - as provided by Error.message
   • statusCode
   • details
   • + any fields from options.safeFields
3. debug:false & status code is 5xx or <400 or falsey - the error object
   contains the following fields
   • statusCode
   • message - the string from HTTP spec
   • + any fields from options.safeFields

Implementation Notes

• When the error handler is called while the response is in progress (headers were sent), then we should not modify the response but close the client socket instead.

  if (res._header) {
    return req.socket.destroy()
  }

• The response should include "X-Content-Type-Options: nosniff" header.

• Status codes <400 should be converted to 500.

References

• https://github.com/ericelliott/express-error-handler
• https://github.com/expressjs/errorhandler
• https://github.com/expressjs/api-error-handler
• expressjs/errorhandler#13

An example of the current error response format:

{
  "error": {
    "name": "ValidationError",
    "status": 422,
    "message": "The `Product` instance is not valid. Details: `name` can't be blank (value: undefined).",
    "statusCode": 422,
    "details": {
      "context": "Product",
      "codes": {
        "name": ["presence"]
      },
      "messages": {
        "name": ["can't be blank"]
      }
    },
    "stack": "ValidationError: The `Product` instance is not valid. etc."
  }
}