feat!: render stripped back errors in production

This makes the error rendering middleware also take responsibility for
outputting an error page in production environments. In production no
error details are leaked and we only render the status code and message,
this replicates the behaviour currently in n-express.

We're doing this to reduce the footprint of n-express (#310) which
currently contains some error rendering logic. Once use of the
Reliability Kit error rendering middleware is more ubiquitous we should
be able to remove the error handling code from n-express in a new major
version.

While this is technically a breaking change, if an app is using
n-express then it's safe to upgrade. This is because this package
behaves in exactly the same way as the n-express error handler.

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)):
 

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);
 	};
 }
 

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",

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', () => {