diff --git a/HISTORY.md b/HISTORY.md index 78dddc0..cd46356 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,3 +1,13 @@ +unreleased +========== + + * Add `message` option + * Add `stacktrace` option + * Add `text/plain` fallback response + * Ignore `err.status` with non-error code + * Remove `env` option; use `stacktrace` option instead + * Send complete HTML document + 0.4.1 / 2015-12-02 ================== diff --git a/README.md b/README.md index 71ef534..ca9a8a2 100644 --- a/README.md +++ b/README.md @@ -29,16 +29,29 @@ be written out to the `res`. When an error is written, the following information is added to the response: - * The `res.statusCode` is set from `err.status` (or `err.statusCode`). + * The `res.statusCode` is set from `err.status` (or `err.statusCode`) if the + value is 4xx or 5xx. * The body will be the HTML of the status code message if `env` is `'production'`, otherwise will be `err.stack`. The final handler will also unpipe anything from `req` when it is invoked. -#### options.env +#### options.message -By default, the environment is determined by `NODE_ENV` variable, but it can be -overridden by this option. +Determine if the error message should be displayed. By default this is false unless +`stacktrace` is enabled. When true, the default behavior will be used to display +the message, which is to use `err.message` if there is a valid `err.status` property. + +A function can also be provided to customize the message. The `err` value and outgoing +`status` code are passed and a message string is expected to be returned. If a falsy +value is returned, the response will act as if this option was the default. + +```js +var done = finalhandler(req, res, {message: function (err, status) { + // only display message when err.public is true + return err.public ? err.message : undefined +}}) +``` #### options.onerror @@ -46,6 +59,12 @@ Provide a function to be called with the `err` when it exists. Can be used for writing errors to a central location without excessive function generation. Called as `onerror(err, req, res)`. +#### options.stacktrace + +Specify if the stack trace of the error should be included in the response. By +default, this is false, but can be enabled when necessary (like in development). +It is not recommend to enable this on a production deployment + ## Examples ### always 404 @@ -99,6 +118,37 @@ var server = http.createServer(function (req, res) { server.listen(3000) ``` +### display err.message to users + +```js +var finalhandler = require('finalhandler') +var http = require('http') + +var server = http.createServer(function (req, res) { + var done = finalhandler(req, res, {message: true}) + var err = new Error('Please try again later') + err.status = 503 + done(err) +}) + +server.listen(3000) +``` + +### display error stack trace for development debugging + +```js +var finalhandler = require('finalhandler') +var http = require('http') + +var server = http.createServer(function (req, res) { + var done = finalhandler(req, res, {stacktrace: true}) + var err = new Error('oops') + done(err) +}) + +server.listen(3000) +``` + ### keep log of all errors ```js diff --git a/index.js b/index.js index 3033ac2..128126d 100644 --- a/index.js +++ b/index.js @@ -11,6 +11,7 @@ * @private */ +var accepts = require('accepts') var debug = require('debug')('finalhandler') var escapeHtml = require('escape-html') var http = require('http') @@ -48,13 +49,25 @@ module.exports = finalhandler function finalhandler (req, res, options) { var opts = options || {} - // get environment - var env = opts.env || process.env.NODE_ENV || 'development' + // get message option + var message = opts.message === true + ? getDefaultErrorMessage + : opts.message || false + + if (typeof message !== 'boolean' && typeof message !== 'function') { + throw new TypeError('option message must be boolean or function') + } // get error callback var onerror = opts.onerror + // get stack trace option + var stacktrace = opts.stacktrace || false + return function (err) { + var body + var constructBody + var msg var status = res.statusCode // ignore 404 on in-flight response @@ -65,31 +78,21 @@ function finalhandler (req, res, options) { // unhandled error if (err) { - // respect err.statusCode - if (err.statusCode) { - status = err.statusCode - } - - // respect err.status - if (err.status) { - status = err.status - } + // respect status code from error + status = getErrorStatusCode(err) || status // default status code to 500 if (!status || status < 400) { status = 500 } - // production gets a basic error message - var msg = env === 'production' - ? http.STATUS_CODES[status] - : err.stack || err.toString() - msg = escapeHtml(msg) - .replace(/\n/g, '
') - .replace(/\x20{2}/g, '  ') + '\n' + // build a stack trace or normal message + msg = stacktrace + ? getErrorStack(err, status, message) + : getErrorMessage(err, status, message) } else { status = 404 - msg = 'Cannot ' + escapeHtml(req.method) + ' ' + escapeHtml(req.originalUrl || req.url) + '\n' + msg = 'Cannot ' + req.method + ' ' + (req.originalUrl || req.url) } debug('default %s', status) @@ -104,17 +107,176 @@ function finalhandler (req, res, options) { return req.socket.destroy() } - send(req, res, status, msg) + // negotiate + var accept = accepts(req) + var type = accept.types('html', 'text') + + // construct body + switch (type) { + case 'html': + constructBody = constructHtmlBody + break + default: + // default to plain text + constructBody = constructTextBody + break + } + + // construct body + body = constructBody(status, msg) + + // send response + send(req, res, status, body) } } +/** + * Get HTML body string + * + * @param {number} status + * @param {string} message + * @return {Buffer} + * @private + */ + +function constructHtmlBody (status, message) { + var msg = escapeHtml(message) + .replace(/\n/g, '
') + .replace(/\x20{2}/g, '  ') + + var html = '\n' + + '\n' + + '\n' + + '\n' + + '' + escapeHtml(http.STATUS_CODES[status]) + '\n' + + '\n' + + '\n' + + msg + '\n' + + '\n' + + var body = new Buffer(html, 'utf8') + + body.type = 'text/html; charset=utf-8' + + return body +} + +/** + * Get plain text body string + * + * @param {number} status + * @param {string} message + * @return {Buffer} + * @private + */ + +function constructTextBody (status, message) { + var msg = message + '\n' + var body = new Buffer(msg, 'utf8') + + body.type = 'text/plain; charset=utf-8' + + return body +} + +/** + * Get message from error + * + * @param {object} err + * @param {number} status + * @param {function} message + * @return {string} + * @private + */ + +function getErrorMessage (err, status, message) { + var msg + + if (message) { + msg = message(err, status) + } + + return msg || http.STATUS_CODES[status] +} + +/** + * Get default message from error + * + * @param {object} err + * @return {string} + * @private + */ + +function getDefaultErrorMessage (err) { + return (err.status >= 400 && err.status < 600) || (err.statusCode >= 400 && err.statusCode < 600) + ? err.message + : undefined +} + +/** + * Get stack from error with custom message + * + * @param {object} err + * @param {number} status + * @param {function} message + * @return {string} + * @private + */ + +function getErrorStack (err, status, message) { + var stack = err.stack || '' + + if (message) { + var index = stack.indexOf('\n') + var msg = message(err, status) || err.message || String(err) + var name = err.name + + // slice implicit message from top of stack + if (index !== -1) { + stack = stack.substr(index) + } + + // prepend name and message to stack + stack = name + ? name + ': ' + msg + stack + : msg + stack + } else if (!stack) { + // stringify error when no message generator and no stack + stack = String(err) + } + + return stack +} + +/** + * Get status code from an Error object. + * + * @param {object} err + * @return {number} + * @private + */ + +function getErrorStatusCode (err) { + // check err.status + if (err.status >= 400 && err.status < 600) { + return err.status + } + + // check err.statusCode + if (err.statusCode >= 400 && err.statusCode < 600) { + return err.statusCode + } + + return undefined +} + /** * Send response. * * @param {IncomingMessage} req * @param {OutgoingMessage} res * @param {number} status - * @param {string} body + * @param {Buffer} body * @private */ @@ -126,8 +288,8 @@ function send (req, res, status, body) { res.setHeader('X-Content-Type-Options', 'nosniff') // standard headers - res.setHeader('Content-Type', 'text/html; charset=utf-8') - res.setHeader('Content-Length', Buffer.byteLength(body, 'utf8')) + res.setHeader('Content-Type', body.type) + res.setHeader('Content-Length', body.length) if (req.method === 'HEAD') { res.end() diff --git a/package.json b/package.json index 2421b64..4bc24ba 100644 --- a/package.json +++ b/package.json @@ -6,6 +6,7 @@ "license": "MIT", "repository": "pillarjs/finalhandler", "dependencies": { + "accepts": "~1.2.11", "debug": "~2.2.0", "escape-html": "~1.0.3", "on-finished": "~2.3.0", diff --git a/test/test.js b/test/test.js index 1f2dc42..7dc2c51 100644 --- a/test/test.js +++ b/test/test.js @@ -31,6 +31,24 @@ describe('finalhandler(req, res)', function () { .expect(400, done) }) + it('should ignore non-error err.statusCode code', function (done) { + var err = new Error() + err.statusCode = 201 + var server = createServer(err) + request(server) + .get('/') + .expect(500, done) + }) + + it('should ignore weird err.statusCode', function (done) { + var err = new Error() + err.statusCode = 'oh no' + var server = createServer(err) + request(server) + .get('/') + .expect(500, done) + }) + it('should use err.status', function (done) { var err = new Error() err.status = 400 @@ -40,6 +58,24 @@ describe('finalhandler(req, res)', function () { .expect(400, done) }) + it('should ignore non-error err.status code', function (done) { + var err = new Error() + err.status = 201 + var server = createServer(err) + request(server) + .get('/') + .expect(500, done) + }) + + it('should ignore weird err.status', function (done) { + var err = new Error() + err.status = 'oh no' + var server = createServer(err) + request(server) + .get('/') + .expect(500, done) + }) + it('should use err.status over err.statusCode', function (done) { var err = new Error() err.status = 400 @@ -61,11 +97,11 @@ describe('finalhandler(req, res)', function () { }) describe('404 response', function () { - it('include method and path', function (done) { + it('should include method and path', function (done) { var server = createServer() request(server) .get('/foo') - .expect(404, 'Cannot GET /foo\n', done) + .expect(404, /Cannot GET \/foo/, done) }) it('should handle HEAD', function (done) { @@ -93,21 +129,44 @@ describe('finalhandler(req, res)', function () { test.write(buf) test.expect(404, done) }) + + describe('when HTML acceptable', function () { + it('should respond with HTML', function (done) { + var server = createServer() + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect('Content-Type', 'text/html; charset=utf-8') + .expect(404, /    at/, done) + .expect(bodyShouldNotContain('boom!')) + .expect(500, /Internal Server Error/, done) }) it('should handle HEAD', function (done) { - var server = createServer() + var server = createServer(new Error('boom!')) request(server) .head('/foo') - .expect(404, '', done) + .expect(500, '', done) }) it('should include security header', function (done) { @@ -118,22 +177,6 @@ describe('finalhandler(req, res)', function () { .expect(500, done) }) - it('should handle non-error-objects', function (done) { - var server = createServer('lame string') - request(server) - .get('/foo') - .expect(500, 'lame string\n', done) - }) - - it('should send staus code name when production', function (done) { - var err = new Error('boom!') - err.status = 501 - var server = createServer(err, {env: 'production'}) - request(server) - .get('/foo') - .expect(501, 'Not Implemented\n', done) - }) - describe('when there is a request body', function () { it('should not hang/error when unread', function (done) { var buf = new Buffer(1024 * 16) @@ -181,6 +224,28 @@ describe('finalhandler(req, res)', function () { }) }) + describe('when HTML acceptable', function () { + it('should respond with HTML', function (done) { + var server = createServer(new Error('boom!')) + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect('Content-Type', 'text/html; charset=utf-8') + .expect(500, /= 400', function (done) { var server = http.createServer(function (req, res) { @@ -258,6 +323,96 @@ describe('finalhandler(req, res)', function () { }) }) + describe('message', function () { + it('should reject string', function () { + assert.throws(finalhandler.bind(null, {}, {}, {message: 'wat'}), /option message/) + }) + + it('should display error message for valid err.status', function (done) { + var err = new Error('boom!') + err.status = 500 + var server = createServer(err, {message: true}) + request(server) + .get('/foo') + .expect(500, /boom!/, done) + }) + + it('should display error message for valid err.statusCode', function (done) { + var err = new Error('boom!') + err.statusCode = 500 + var server = createServer(err, {message: true}) + request(server) + .get('/foo') + .expect(500, /boom!/, done) + }) + + it('should not display error message missing stack property', function (done) { + var err = new Error('boom!') + var server = createServer(err, {message: true}) + request(server) + .get('/foo') + .expect(bodyShouldNotContain('boom!')) + .expect(500, /Internal Server Error/, done) + }) + + it('should not display error message for bad status property', function (done) { + var err = new Error('boom!') + err.status = 'oh no' + var server = createServer(err, {message: true}) + request(server) + .get('/foo') + .expect(bodyShouldNotContain('boom!')) + .expect(500, /Internal Server Error/, done) + }) + + it('should escape message for HTML response', function (done) { + var err = new Error('!') + err.status = 500 + var server = createServer(err, {message: true}) + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect(500, /<boom>!/, done) + }) + + describe('when function', function () { + it('should use custom function for message', function (done) { + var err = new Error('boom!') + var server = createServer(err, {message: function (err, status) { + return 'custom ' + status + ' ' + err.message + }}) + + request(server) + .get('/foo') + .expect(500, /custom 500 boom!/, done) + }) + + it('should provide fallback for custom function', function (done) { + var err = new Error('boom!') + var server = createServer(err, {message: function () { + return undefined + }}) + + request(server) + .get('/foo') + .expect(bodyShouldNotContain('boom!')) + .expect(500, /Internal Server Error/, done) + }) + + it('should escape message for HTML response', function (done) { + var err = new Error('!') + var server = createServer(err, {message: function (err) { + return 'custom ' + err.message + }}) + + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect(500, /custom <boom>!/, done) + }) + }) + }) + describe('onerror', function () { it('should be invoked when error', function (done) { var err = new Error('boom!') @@ -273,8 +428,81 @@ describe('finalhandler(req, res)', function () { }) }) }) + + describe('stacktrace', function () { + it('should include error stack', function (done) { + var server = createServer(new Error('boom!'), {stacktrace: true}) + request(server) + .get('/foo') + .expect(500, /Error: boom!.*at.*:[0-9]+:[0-9]+/, done) + }) + + it('should escape error stack for HTML response', function (done) { + var server = createServer(new Error('boom!'), {stacktrace: true}) + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect(500, /Error: boom!
   at/, done) + }) + + it('should not escape error stack for plain text response', function (done) { + var server = createServer(new Error('boom!'), {stacktrace: true}) + request(server) + .get('/foo') + .set('Accept', 'application/x-bogus') + .expect('Content-Type', 'text/plain; charset=utf-8') + .expect(500, /Error: boom!\n\x20{4}at/, done) + }) + + it('should handle non-error-objects', function (done) { + var server = createServer('lame string', {stacktrace: true}) + request(server) + .get('/foo') + .set('Accept', 'text/html') + .expect(500, /lame string/, done) + }) + + describe('when message set', function () { + it('should use custom function for message', function (done) { + var err = new Error('boom!') + var server = createServer(err, {stacktrace: true, message: function (err, status) { + return 'custom ' + status + ' ' + err.message + }}) + + request(server) + .get('/foo') + .expect(500, /Error: custom 500 boom!.*at.*:[0-9]+:[0-9]+/, done) + }) + + it('should provide fallback for custom function', function (done) { + var err = new Error('boom!') + var server = createServer(err, {stacktrace: true, message: function () { + return undefined + }}) + + request(server) + .get('/foo') + .expect(500, /Error: boom!.*at.*:[0-9]+:[0-9]+/, done) + }) + + it('should handle non-error-objects', function (done) { + var err = 'lame string' + var server = createServer(err, {stacktrace: true, message: true}) + + request(server) + .get('/foo') + .expect(500, /lame string/, done) + }) + }) + }) }) +function bodyShouldNotContain (str) { + return function (res) { + assert.ok(res.text.indexOf(str) === -1, 'should not contain "' + str + '" in body') + } +} + function createServer (err, opts) { return http.createServer(function (req, res) { var done = finalhandler(req, res, opts)