From bda981feb82197a8e557cd86348510b3063fcdda Mon Sep 17 00:00:00 2001 From: Raymond Feng Date: Wed, 20 May 2020 14:33:11 -0700 Subject: [PATCH] feat: add options.rootProperty for json/xml Using `error` as the root property is a good default but we should be able to use a different one or don't use it at all. --- README.md | 51 +++++++++++++------------- index.d.ts | 1 + lib/handler.js | 5 ++- lib/send-html.js | 6 ++-- lib/send-json.js | 10 ++++-- lib/send-xml.js | 8 +++-- test/handler.test.js | 86 ++++++++++++++++++++++++++++++++++++++++---- 7 files changed, 126 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index 9792187..387fcb0 100644 --- a/README.md +++ b/README.md @@ -14,9 +14,9 @@ In debug mode, `strong-error-handler` returns full error stack traces and intern ## Supported versions -Current|Long Term Support|Maintenance -:-:|:-:|:-: -3.x|2.x|1.x +| Current | Long Term Support | Maintenance | +| :-----: | :---------------: | :---------: | +| 4.x | 3.x | 2.x | Learn more about our LTS plan in [docs](http://loopback.io/doc/en/contrib/Long-term-support.html). @@ -108,24 +108,25 @@ The content type of the response depends on the request's `Accepts` header. ## Options -| Option | Type | Default | Description | -| ---- | ---- | ---- | ---- | -| debug | Boolean    | `false` | If `true`, HTTP responses include all error properties, including sensitive data such as file paths, URLs and stack traces. See [Example output](#example) below. | -| log | Boolean | `true` | If `true`, all errors are printed via `console.error`, including an array of fields (custom error properties) that are safe to include in response messages (both 4xx and 5xx).
If `false`, sends only the error back in the response. | -| safeFields | [String] | `[]` | Specifies property names on errors that are allowed to be passed through in 4xx and 5xx responses. See [Safe error fields](#safe-error-fields) below. | -| defaultType | String | `"json"` | Specify the default response content type to use when the client does not provide any Accepts header. -| negotiateContentType | Boolean | true | Negotiate the response content type via Accepts request header. When disabled, strong-error-handler will always use the default content type when producing responses. Disabling content type negotiation is useful if you want to see JSON-formatted error responses in browsers, because browsers usually prefer HTML and XML over other content types. +| Option | Type | Default | Description | +| -------------------- | ------------------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| debug | Boolean    | `false` | If `true`, HTTP responses include all error properties, including sensitive data such as file paths, URLs and stack traces. See [Example output](#example) below. | +| log | Boolean | `true` | If `true`, all errors are printed via `console.error`, including an array of fields (custom error properties) that are safe to include in response messages (both 4xx and 5xx).
If `false`, sends only the error back in the response. | +| safeFields | [String] | `[]` | Specifies property names on errors that are allowed to be passed through in 4xx and 5xx responses. See [Safe error fields](#safe-error-fields) below. | +| defaultType | String | `"json"` | Specifies the default response content type to use when the client does not provide any Accepts header. | +| rootProperty | String or false | `"error"` | Specifies the root property name for json or xml. If the value is set to `false`, no wrapper will be added to the json object. The false value is ignored by XML as a root element is always required. | +| negotiateContentType | Boolean | true | Negotiate the response content type via Accepts request header. When disabled, strong-error-handler will always use the default content type when producing responses. Disabling content type negotiation is useful if you want to see JSON-formatted error responses in browsers, because browsers usually prefer HTML and XML over other content types. | ### Customizing log format -**Express** +**Express** -To use a different log format, add your own custom error-handling middleware then disable `errorHandler.log`. +To use a different log format, add your own custom error-handling middleware then disable `errorHandler.log`. For example, in an Express application: ```js app.use(myErrorLogger()); -app.use(errorHandler({ log: false })); +app.use(errorHandler({log: false})); ``` In general, add `strong-error-handler` as the last middleware function, just before calling `app.listen()`. @@ -234,7 +235,7 @@ To migrate a LoopBack 2.x application to use `strong-error-handler`: } -For more information, see +For more information, see [Migrating apps to LoopBack 3.0](http://loopback.io/doc/en/lb3/Migrating-to-3.0.html#update-use-of-rest-error-handler). ## Example @@ -252,17 +253,17 @@ The same error generated when `debug: true` : { statusCode: 500, name: 'Error', message: 'a test error message', - stack: 'Error: a test error message - at Context. (User/strong-error-handler/test/handler.test.js:220:21) - at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8) - at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7) - at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10) - at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12 - at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14) - at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7 - at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14) - at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5) - at tryOnImmediate (timers.js:543:15) + stack: 'Error: a test error message + at Context. (User/strong-error-handler/test/handler.test.js:220:21) + at callFnAsync (User/strong-error-handler/node_modules/mocha/lib/runnable.js:349:8) + at Test.Runnable.run (User/strong-error-handler/node_modules/mocha/lib/runnable.js:301:7) + at Runner.runTest (User/strong-error-handler/node_modules/mocha/lib/runner.js:422:10) + at User/strong-error-handler/node_modules/mocha/lib/runner.js:528:12 + at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:342:14) + at User/strong-error-handler/node_modules/mocha/lib/runner.js:352:7 + at next (User/strong-error-handler/node_modules/mocha/lib/runner.js:284:14) + at Immediate._onImmediate (User/strong-error-handler/node_modules/mocha/lib/runner.js:320:5) + at tryOnImmediate (timers.js:543:15) at processImmediate [as _immediateCallback] (timers.js:523:5)' }} ``` diff --git a/index.d.ts b/index.d.ts index 646c652..5cfe443 100644 --- a/index.d.ts +++ b/index.d.ts @@ -53,6 +53,7 @@ declare namespace errorHandlerFactory { safeFields?: string[]; defaultType?: string; negotiateContentType?: boolean; + rootProperty?: string | false; } /** diff --git a/lib/handler.js b/lib/handler.js index f0f746f..fd7533b 100644 --- a/lib/handler.js +++ b/lib/handler.js @@ -10,7 +10,6 @@ const SG = require('strong-globalize'); SG.SetRootDir(path.resolve(__dirname, '..')); const buildResponseData = require('./data-builder'); const debug = require('debug')('strong-error-handler'); -const format = require('util').format; const logToConsole = require('./logger'); const negotiateContentProducer = require('./content-negotiation'); @@ -50,7 +49,7 @@ function writeErrorToResponse(err, req, res, options) { options = options || {}; - if (res._header) { + if (res.headersSent) { debug('Response was already sent, closing the underlying connection'); return req.socket.destroy(); } @@ -66,7 +65,7 @@ function writeErrorToResponse(err, req, res, options) { res.statusCode = data.statusCode; const sendResponse = negotiateContentProducer(req, warn, options); - sendResponse(res, data); + sendResponse(res, data, options); function warn(msg) { res.header('X-Warning', msg); diff --git a/lib/send-html.js b/lib/send-html.js index b6c50a9..4f9dc97 100644 --- a/lib/send-html.js +++ b/lib/send-html.js @@ -17,10 +17,10 @@ const compiledTemplates = { module.exports = sendHtml; function sendHtml(res, data, options) { - const toRender = {options: {}, data: data}; + const toRender = {options, data}; // TODO: ability to call non-default template functions from options const body = compiledTemplates.default(toRender); - sendReponse(res, body); + sendResponse(res, body); } /** @@ -41,7 +41,7 @@ function loadDefaultTemplates() { return compileTemplate(defaultTemplate); } -function sendReponse(res, body) { +function sendResponse(res, body) { res.setHeader('Content-Type', 'text/html; charset=utf-8'); res.end(body); } diff --git a/lib/send-json.js b/lib/send-json.js index a9f9687..0776dc9 100644 --- a/lib/send-json.js +++ b/lib/send-json.js @@ -7,8 +7,14 @@ const safeStringify = require('fast-safe-stringify'); -module.exports = function sendJson(res, data) { - const content = safeStringify({error: data}); +module.exports = function sendJson(res, data, options) { + options = options || {}; + // Set `options.rootProperty` to not wrap the data into an `error` object + const err = options.rootProperty === false ? data : { + // Use `options.rootProperty`, if not set, default to `error` + [options.rootProperty || 'error']: data, + }; + const content = safeStringify(err); res.setHeader('Content-Type', 'application/json; charset=utf-8'); res.end(content, 'utf-8'); }; diff --git a/lib/send-xml.js b/lib/send-xml.js index cbfafdc..46e938e 100644 --- a/lib/send-xml.js +++ b/lib/send-xml.js @@ -7,8 +7,12 @@ const js2xmlparser = require('js2xmlparser'); -module.exports = function sendXml(res, data) { - const content = js2xmlparser.parse('error', data); +module.exports = function sendXml(res, data, options) { + options = options || {}; + // Xml always requires a root element. + // `options.rootProperty === false` is not honored + const root = options.rootProperty || 'error'; + const content = js2xmlparser.parse(root, data); res.setHeader('Content-Type', 'text/xml; charset=utf-8'); res.end(content, 'utf-8'); }; diff --git a/test/handler.test.js b/test/handler.test.js index 5427dae..2bdb92f 100644 --- a/test/handler.test.js +++ b/test/handler.test.js @@ -78,7 +78,7 @@ describe('strong-error-handler', function() { request.get('/').expect( 507, {error: {statusCode: 507, message: 'Insufficient Storage'}}, - done + done, ); }); }); @@ -146,7 +146,7 @@ describe('strong-error-handler', function() { it('handles array argument', function(done) { givenErrorHandlerForError( [new TypeError('ERR1'), new Error('ERR2')], - {log: true} + {log: true}, ); request.get('/api').end(function(err) { @@ -541,6 +541,32 @@ describe('strong-error-handler', function() { }); }); + it('honors rootProperty', function(done) { + givenErrorHandlerForError('Error Message', {rootProperty: 'data'}); + requestJson().expect(500).end(function(err, res) { + if (err) return done(err); + + expect(res.body.data).to.eql({ + statusCode: 500, + message: 'Internal Server Error', + }); + done(); + }); + }); + + it('honors rootProperty=false', function(done) { + givenErrorHandlerForError('Error Message', {rootProperty: false}); + requestJson().expect(500).end(function(err, res) { + if (err) return done(err); + + expect(res.body).to.eql({ + statusCode: 500, + message: 'Internal Server Error', + }); + done(); + }); + }); + function requestJson(url) { return request.get(url || '/') .set('Accept', 'text/plain') @@ -581,10 +607,10 @@ describe('strong-error-handler', function() { expect(res.statusCode).to.eql(404); const body = res.error.text; expect(body).to.match( - /Error<img onerror=alert\(1\) src=a><\/title>/ + /<title>Error<img onerror=alert\(1\) src=a><\/title>/, ); expect(body).to.match( - /with id <img onerror=alert\(1\) src=a> found for Model/ + /with id <img onerror=alert\(1\) src=a> found for Model/, ); done(); }); @@ -602,7 +628,7 @@ describe('strong-error-handler', function() { .expect(/<title>ErrorWithProps<\/title>/) .expect( /500(.*?)a test error message<img onerror=alert\(1\) src=a>/, - done + done, ); }); @@ -696,7 +722,7 @@ describe('strong-error-handler', function() { expect(body).to.not.match(/<extra>sensitive data<\/extra>/); expect(body).to.match(/<name>ValidationError<\/name>/); expect(body).to.match( - /<message>The model instance is not valid.<\/message>/ + /<message>The model instance is not valid.<\/message>/, ); done(); }); @@ -728,6 +754,54 @@ describe('strong-error-handler', function() { }); }); + it('honors options.rootProperty', function(done) { + const error = new ErrorWithProps({ + name: 'ValidationError', + message: 'The model instance is not valid.', + statusCode: 422, + details: 'some details', + extra: 'sensitive data', + }); + givenErrorHandlerForError(error, {rootProperty: 'myRoot'}); + requestXML() + .end(function(err, res) { + expect(res.statusCode).to.eql(422); + const body = res.error.text; + expect(body).to.match(/<myRoot>/); + expect(body).to.match(/<details>some details<\/details>/); + expect(body).to.not.match(/<extra>sensitive data<\/extra>/); + expect(body).to.match(/<name>ValidationError<\/name>/); + expect(body).to.match( + /<message>The model instance is not valid.<\/message>/, + ); + done(); + }); + }); + + it('ignores options.rootProperty = false', function(done) { + const error = new ErrorWithProps({ + name: 'ValidationError', + message: 'The model instance is not valid.', + statusCode: 422, + details: 'some details', + extra: 'sensitive data', + }); + givenErrorHandlerForError(error, {rootProperty: false}); + requestXML() + .end(function(err, res) { + expect(res.statusCode).to.eql(422); + const body = res.error.text; + expect(body).to.match(/<error>/); + expect(body).to.match(/<details>some details<\/details>/); + expect(body).to.not.match(/<extra>sensitive data<\/extra>/); + expect(body).to.match(/<name>ValidationError<\/name>/); + expect(body).to.match( + /<message>The model instance is not valid.<\/message>/, + ); + done(); + }); + }); + function requestXML(url) { return request.get(url || '/') .set('Accept', 'text/xml')