diff --git a/packages/middleware-render-error-info/README.md b/packages/middleware-render-error-info/README.md index 77797c40..097fcbae 100644 --- a/packages/middleware-render-error-info/README.md +++ b/packages/middleware-render-error-info/README.md @@ -1,7 +1,7 @@ ## @dotcom-reliability-kit/middleware-render-error-info -Express middleware to render error information in a browser in a way that makes local debugging easier. This module is part of [FT.com Reliability Kit](https://github.com/Financial-Times/dotcom-reliability-kit#readme). +Express middleware to render error information in a browser in a way that makes local debugging easier and production error rendering more consistent. This module is part of [FT.com Reliability Kit](https://github.com/Financial-Times/dotcom-reliability-kit#readme). * [Usage](#usage) * [`renderErrorInfoPage`](#rendererrorinfopage) @@ -27,7 +27,9 @@ const renderErrorInfoPage = require('@dotcom-reliability-kit/middleware-render-e ### `renderErrorInfoPage` -The `renderErrorInfoPage` function can be used to generate Express middleware which renders an error debugging page. The error page will only ever display in a non-production environment, that is when the `NODE_ENV` environment variable is either **empty** or set to **`"development"`**. +The `renderErrorInfoPage` function can be used to generate Express middleware which renders an error debugging page in local development and a sensible stripped-back error page in production. + +When the `NODE_ENV` environment variable is either **empty** or set to **`"development"`** then a full debug page will be rendered. Otherwise only the error status code and message will be output, e.g. `500 Server Error`. This ensures that we don't leak important error information in production. > **Warning** > This middleware **must** be added to your Express app _after_ all your application routes – you won't get rendered errors for any routes which are mounted after this middleware. @@ -38,8 +40,8 @@ const app = express(); app.use(renderErrorInfoPage()); ``` -> **Note** -> If you're using [@dotcom-reliability-kit/middleware-log-errors](https://github.com/Financial-Times/dotcom-reliability-kit/tree/main/packages/middleware-log-errors#readme) in your app, it's best to mount the error page middleware _after_ the logging middleware. Otherwise the error will never be logged in local development, which may cause some confusion. +> **Warning** +> If you're using [@dotcom-reliability-kit/middleware-log-errors](https://github.com/Financial-Times/dotcom-reliability-kit/tree/main/packages/middleware-log-errors#readme) in your app, it's best to mount the error page middleware _after_ the logging middleware. Otherwise the error will never be logged. Once you've mounted the middleware, if you're working locally you should now see a detailed error page when you encounter an error in your app (assuming you're [relying on the Express error handler to serve errors](https://github.com/Financial-Times/dotcom-reliability-kit/blob/main/docs/getting-started/handling-errors.md#bubbling-up-in-express)): diff --git a/packages/middleware-render-error-info/lib/index.js b/packages/middleware-render-error-info/lib/index.js index d73b6fdc..77b5ba09 100644 --- a/packages/middleware-render-error-info/lib/index.js +++ b/packages/middleware-render-error-info/lib/index.js @@ -2,6 +2,7 @@ const appInfo = require('@dotcom-reliability-kit/app-info'); const { logRecoverableError } = require('@dotcom-reliability-kit/log-error'); const renderErrorPage = require('./render-error-page'); const serializeError = require('@dotcom-reliability-kit/serialize-error'); +const { STATUS_CODES } = require('node:http'); /** * @typedef {object} ErrorRenderingOptions @@ -19,9 +20,6 @@ const serializeError = require('@dotcom-reliability-kit/serialize-error'); * Returns error info rendering middleware. */ function createErrorRenderingMiddleware(options = {}) { - // Only render the error info page if we're not in production. - const performRendering = appInfo.environment === 'development'; - return function errorRenderingMiddleware(error, request, response, next) { // If headers have been sent already then we need to hand off to // the final Express error handler as documented here: @@ -32,46 +30,54 @@ function createErrorRenderingMiddleware(options = {}) { return next(error); } - // If we're not supposed to perform error rendering, then we hand off to the - // default error handler. - if (!performRendering) { - return next(error); - } + // Serialize the error and extract the status code + const serializedError = serializeError(error); + let statusCode = serializedError.statusCode || 500; - // It's unlikely that this will fail but we want to be sure - // that any rendering errors are caught properly - try { - // Serialize the error and extract the status code - const serializedError = serializeError(error); - const statusCode = serializedError.statusCode || 500; + // If the error has a status code of less than `400` we + // should default to `500` to ensure bad error handling + // doesn't send false positive status codes. We also check + // that the status code is a valid number. + if ( + Number.isNaN(statusCode) || // Possible if `error.status` is something unexpected, like an object + statusCode < 400 || + statusCode > 599 + ) { + statusCode = 500; + } - // If the error has a status code of less than `400` we - // should default to `500` to ensure bad error handling - // doesn't send false positive status codes. We also check - // that the status code is a valid number. - const isValidErrorStatus = - !Number.isNaN(statusCode) && // Possible if `error.status` is something unexpected, like an object - statusCode >= 400 && - statusCode <= 599; + // Set the response status to match the error status code and + // always send HTML + response.status(statusCode); + response.set('content-type', 'text/html'); - // Render an HTML error page - response.status(isValidErrorStatus ? statusCode : 500); - response.set('content-type', 'text/html'); - return response.send( - renderErrorPage({ - request, - response, - serializedError - }) - ); - } catch (/** @type {any} */ renderingError) { - logRecoverableError({ - error: renderingError, - logger: options.logger, - request - }); - next(error); + // Render a full error page in non-production environments + if (appInfo.environment === 'development') { + // It's unlikely that this will fail but we want to be sure + // that any rendering errors are caught properly + try { + // Render an HTML error page + return response.send( + renderErrorPage({ + request, + response, + serializedError + }) + ); + } catch (/** @type {any} */ renderingError) { + logRecoverableError({ + error: renderingError, + logger: options.logger, + request + }); + } } + + // Either rendering has failed or we're in production. We render a + // heavily stripped back error + const statusMessage = STATUS_CODES[statusCode] || STATUS_CODES[500]; + const output = `${statusCode} ${statusMessage}\n`; + response.send(output); }; } diff --git a/packages/middleware-render-error-info/package.json b/packages/middleware-render-error-info/package.json index 0a3bb082..82d1fadc 100644 --- a/packages/middleware-render-error-info/package.json +++ b/packages/middleware-render-error-info/package.json @@ -1,7 +1,7 @@ { "name": "@dotcom-reliability-kit/middleware-render-error-info", "version": "3.1.0", - "description": "Express middleware to render error information in a way that makes local debugging easier.", + "description": "Express middleware to render error information in a way that makes local debugging easier and production error rendering more consistent.", "repository": { "type": "git", "url": "https://github.com/Financial-Times/dotcom-reliability-kit.git", diff --git a/packages/middleware-render-error-info/test/unit/lib/index.spec.js b/packages/middleware-render-error-info/test/unit/lib/index.spec.js index fc7db79e..40f6fddd 100644 --- a/packages/middleware-render-error-info/test/unit/lib/index.spec.js +++ b/packages/middleware-render-error-info/test/unit/lib/index.spec.js @@ -40,6 +40,13 @@ const { logRecoverableError } = require('@dotcom-reliability-kit/log-error'); jest.mock('@dotcom-reliability-kit/app-info', () => ({})); const appInfo = require('@dotcom-reliability-kit/app-info'); +jest.mock('node:http', () => ({ + STATUS_CODES: { + 456: 'Mock Error', + 500: 'Server Error' + } +})); + describe('@dotcom-reliability-kit/middleware-render-error-info', () => { let middleware; @@ -243,14 +250,42 @@ describe('@dotcom-reliability-kit/middleware-render-error-info', () => { middleware(error, request, response, next); }); - it('does not render and send an error info page', () => { - expect(response.status).toBeCalledTimes(0); - expect(response.set).toBeCalledTimes(0); - expect(response.send).toBeCalledTimes(0); + it('responds with the serialized error status code', () => { + expect(response.status).toBeCalledTimes(1); + expect(response.status).toBeCalledWith(500); }); - it('calls `next` with the original error', () => { - expect(next).toBeCalledWith(error); + it('responds with a Content-Type header of "text/html"', () => { + expect(response.set).toBeCalledTimes(1); + expect(response.set).toBeCalledWith('content-type', 'text/html'); + }); + + it('responds with a simple status code and message in the body', () => { + expect(response.send).toBeCalledTimes(1); + const html = response.send.mock.calls[0][0]; + expect(html).toStrictEqual('500 Server Error\n'); + }); + + it('does not call `next` with the original error', () => { + expect(next).toBeCalledTimes(0); + }); + + describe('when the serialized error has a nonexistent `statusCode` property', () => { + beforeEach(() => { + serializeError.mockReturnValueOnce({ + statusCode: 477, + data: {} + }); + response.send = jest.fn(); + + middleware(error, request, response, next); + }); + + it('responds with a simple status code and the default server error message in the body', () => { + expect(response.send).toBeCalledTimes(1); + const html = response.send.mock.calls[0][0]; + expect(html).toStrictEqual('477 Server Error\n'); + }); }); }); @@ -259,9 +294,22 @@ describe('@dotcom-reliability-kit/middleware-render-error-info', () => { beforeEach(() => { renderingError = new Error('rendering failed'); - response.send.mockImplementation(() => { - throw renderingError; + + // We fail getting the request method as this will + // ensure that the rendering fails without having + // to mock the entire rendering method + delete request.method; + Object.defineProperty(request, 'method', { + get: () => { + throw renderingError; + } }); + + response = { + send: jest.fn(), + set: jest.fn(), + status: jest.fn() + }; next = jest.fn(); middleware(error, request, response, next); }); @@ -275,8 +323,24 @@ describe('@dotcom-reliability-kit/middleware-render-error-info', () => { }); }); - it('calls `next` with the original error', () => { - expect(next).toBeCalledWith(error); + it('responds with the serialized error status code', () => { + expect(response.status).toBeCalledTimes(1); + expect(response.status).toBeCalledWith(500); + }); + + it('responds with a Content-Type header of "text/html"', () => { + expect(response.set).toBeCalledTimes(1); + expect(response.set).toBeCalledWith('content-type', 'text/html'); + }); + + it('responds with a simple status code and message in the body', () => { + expect(response.send).toBeCalledTimes(1); + const html = response.send.mock.calls[0][0]; + expect(html).toStrictEqual('500 Server Error\n'); + }); + + it('does not call `next` with the original error', () => { + expect(next).toBeCalledTimes(0); }); describe('when the logger option is set', () => {