fix: handle more error rendering edge-cases

This handles a couple of edge-cases we were missing that we _do_ handle
in n-express. These are required before we can consider replacing the
n-express error handling with Reliability Kit. We've done the following:

  * Ensure that we don't try to render the error page if the response
    headers have already been sent. This is recommended in the Express
    documentation

  * Ensure that the HTTP status code we send is between 400 and 599.
    This prevents poor error handling in apps from accidentally sending
    false positives by responding with 200 statuses for errors.

packages/middleware-render-error-info/lib/index.js

@@ -5,7 +5,7 @@ const serializeError = require('@dotcom-reliability-kit/serialize-error');
 
 /**
  * @typedef {object} ErrorRenderingOptions
- * @property {import('@dotcom-reliability-kit/log-error').Logger & Object<string, any>} [logger]
+ * @property {import('@dotcom-reliability-kit/log-error').Logger & {[key: string]: any}} [logger]
  *     The logger to use to output errors. Defaults to Reliability Kit logger.
  */
 
@@ -23,29 +23,55 @@ function createErrorRenderingMiddleware(options = {}) {
 	const performRendering = appInfo.environment === 'development';
 
 	return function errorRenderingMiddleware(error, request, response, next) {
-		if (performRendering) {
-			// It's unlikely that this will fail but we want to be sure
-			// that any rendering errors are caught properly
-			try {
-				const serializedError = serializeError(error);
-				response.status(serializedError.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
-				});
-			}
+		// If headers have been sent already then we need to hand off to
+		// the final Express error handler as documented here:
+		// https://expressjs.com/en/guide/error-handling.html#the-default-error-handler
+		//
+		// This ensures that the app doesn't crash with `ERR_STREAM_WRITE_AFTER_END`
+		if (response.headersSent) {
+			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);
+		}
+
+		// 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.
+			const isValidErrorStatus =
+				!Number.isNaN(statusCode) && // Possible if `error.status` is something unexpected, like an object
+				statusCode >= 400 &&
+				statusCode <= 599;
+
+			// 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);
 		}
-		next(error);
 	};
 }
 

packages/middleware-render-error-info/test/unit/lib/index.spec.js

@@ -172,6 +172,64 @@ describe('@dotcom-reliability-kit/middleware-render-error-info', () => {
 			});
 		});
 
+		describe('when the serialized error has a `statusCode` property lower than 400', () => {
+			beforeEach(() => {
+				serializeError.mockReturnValue({
+					statusCode: 399,
+					data: {}
+				});
+				response.status = jest.fn();
+
+				middleware(error, request, response, next);
+			});
+
+			it('responds with a 500 status code', () => {
+				expect(response.status).toBeCalledTimes(1);
+				expect(response.status).toBeCalledWith(500);
+			});
+		});
+
+		describe('when the serialized error has a `statusCode` property greater than 500', () => {
+			beforeEach(() => {
+				serializeError.mockReturnValue({
+					statusCode: 600,
+					data: {}
+				});
+				response.status = jest.fn();
+
+				middleware(error, request, response, next);
+			});
+
+			it('responds with a 500 status code', () => {
+				expect(response.status).toBeCalledTimes(1);
+				expect(response.status).toBeCalledWith(500);
+			});
+		});
+
+		describe('when the response headers have already been sent', () => {
+			beforeEach(() => {
+				middleware = createErrorRenderingMiddleware();
+				response = {
+					headersSent: true,
+					send: jest.fn(),
+					set: jest.fn(),
+					status: jest.fn()
+				};
+				next = jest.fn();
+				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('calls `next` with the original error', () => {
+				expect(next).toBeCalledWith(error);
+			});
+		});
+
 		describe('when the environment is set to "production"', () => {
 			beforeEach(() => {
 				appInfo.environment = 'production';