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."
  }
}

[zanemcca]

It would be nice if there was a way to add some custom logic in as well. The use case I am thinking of is sending an email after a 500 error code and then killing the process. Currently to achieve this I override errorHandler in remoting.

Will the new error handler be able to achieve this in some way @loay?

[bajtos]

It would be nice if there was a way to add some custom logic in as well. The use case I am thinking of is sending an email after a 500 error code and then killing the process. Currently to achieve this I override errorHandler in remoting.

IMO, that's out of scope of this module. You should disable strong-remoting's error handler completely and then install your own error handling middleware to handle this case.

[kamal0808]

@bajtos - Shouldn't error messages also be configurable so that when directly rendered in UI, they could be easily understood by user? How can we configure these error messages?

Like
"message": "TheProductinstance is not valid. Details:namecan't be blank (value: undefined).",
could become something user-friendly
"message": "Please enter a name for the product",

[bajtos]

Shouldn't error messages also be configurable so that when directly rendered in UI, they could be easily understood by user? How can we configure these error messages?

Yes, you should parse error.details.codes and build the nice error message yourself. If you are using English only and the default messages are ok for you, then you can use error.details.messages instead.

Having wrote that, this discussion is out of scope of this particular issue. Please open a new one if there is anything else to discuss and/or clarify.

[kamal0808]

Having wrote that, this discussion is out of scope of this particular issue. Please open a new one if there is anything else to discuss and/or clarify.

Sorry for that. Here's a new issue/question #2630

[bajtos]

Closing as done.