From a4d86df3bab89865b341fed3a25959e4b4ee601e Mon Sep 17 00:00:00 2001 From: Kamil Mysliwiec Date: Thu, 22 Oct 2026 09:15:42 +0200 Subject: [PATCH] fix(express): leave 5xx http-errors errors unmapped so they are logged (#18232) Since #18214, every http-errors error that reaches the Express error handler is mapped to an HttpException, which the built-in exception filter does not log. Server faults such as EACCES from serving a static file, or a 5xx a middleware passes to next(), were answered without a log line. Only errors below 500 are mapped now, so a 5xx one is logged again, as on Fastify. Co-authored-by: rk --- .../hello-world/e2e/exceptions.spec.ts | 57 ++++++++++++++++++- .../adapters/express-adapter.ts | 4 +- .../test/adapters/express-adapter.spec.ts | 20 ++++--- 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/integration/hello-world/e2e/exceptions.spec.ts b/integration/hello-world/e2e/exceptions.spec.ts index 1aeaa07911..f0dea8ab16 100644 --- a/integration/hello-world/e2e/exceptions.spec.ts +++ b/integration/hello-world/e2e/exceptions.spec.ts @@ -3,6 +3,8 @@ import { Get, HttpStatus, INestApplication, + InternalServerErrorException, + Logger, Post, UnauthorizedException, } from '@nestjs/common'; @@ -13,6 +15,7 @@ import { } from '@nestjs/platform-fastify'; import { Test } from '@nestjs/testing'; import { RawServerDefault } from 'fastify'; +import createError from 'http-errors'; import request from 'supertest'; import { ErrorsController } from '../src/errors/errors.controller.js'; import { listenOnLoopback } from '../../_support/listen-on-loopback.js'; @@ -25,6 +28,14 @@ class ErrorBodyController { } } +@Controller() +class ErrorCauseController { + @Get('error-cause') + findAll() { + throw new InternalServerErrorException('Oops', { cause: createError(404) }); + } +} + @Controller() class SchemaController { @RouteSchema({ body: { type: 'object', required: ['name'] } }) @@ -39,10 +50,17 @@ describe('Error messages', () => { let app: INestApplication; beforeEach(async () => { const module = await Test.createTestingModule({ - controllers: [ErrorsController, ErrorBodyController], + controllers: [ + ErrorsController, + ErrorBodyController, + ErrorCauseController, + ], }).compile(); app = module.createNestApplication(); + app.use('/middleware-error', (req, res, next) => + next(createError(503, 'connect ECONNREFUSED 127.0.0.1:6379')), + ); server = app.getHttpServer(); await listenOnLoopback(app); }); @@ -99,8 +117,45 @@ describe('Error messages', () => { }); }); + it(`/GET (ServiceUnavailable logged when a middleware passes an "http-errors" error to next())`, async () => { + const logError = vi + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => {}); + + await request(server) + .get('/middleware-error') + .expect(HttpStatus.SERVICE_UNAVAILABLE) + .expect({ + statusCode: 503, + message: 'Service Unavailable', + }); + expect(logError).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + statusCode: 503, + message: 'connect ECONNREFUSED 127.0.0.1:6379', + }), + ); + }); + + it(`/GET (InternalServerError not logged when its cause is a 4xx "http-errors" error)`, async () => { + const logError = vi + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => {}); + + await request(server) + .get('/error-cause') + .expect(HttpStatus.INTERNAL_SERVER_ERROR) + .expect({ + statusCode: 500, + error: 'Internal Server Error', + message: 'Oops', + }); + expect(logError).not.toHaveBeenCalled(); + }); + afterEach(async () => { await app.close(); + vi.restoreAllMocks(); }); }); diff --git a/packages/platform-express/adapters/express-adapter.ts b/packages/platform-express/adapters/express-adapter.ts index ec5dc416f8..37c5705268 100644 --- a/packages/platform-express/adapters/express-adapter.ts +++ b/packages/platform-express/adapters/express-adapter.ts @@ -690,7 +690,9 @@ export class ExpressAdapter extends AbstractHttpAdapter< // and an expose flag. Without this case, errors like PayloadTooLargeError reach custom // exception filters as a raw http-errors instance rather than an HttpException, so a // @Catch(HttpException) filter never runs and a catch-all filter reports 500 (#18213). - case isHttpError(error): + // A 5xx one (e.g. EACCES from "send") is a server fault: it stays unmapped, + // so the built-in filter still logs it, as it does on Fastify. + case isHttpError(error) && error.statusCode < 500: return new HttpException( // An error that must not reach the client only gets a status text, // like the built-in exception filter answers it diff --git a/packages/platform-express/test/adapters/express-adapter.spec.ts b/packages/platform-express/test/adapters/express-adapter.spec.ts index 4bdc24e9b0..a72411b7fa 100644 --- a/packages/platform-express/test/adapters/express-adapter.spec.ts +++ b/packages/platform-express/test/adapters/express-adapter.spec.ts @@ -540,16 +540,18 @@ describe('ExpressAdapter', () => { expect(result.cause).toBe(error); }); - it.each([ - [500, 'Internal Server Error'], - [599, 'Internal server error'], - ])( - 'should not expose the message of an http-errors error with status %i', - (status, message) => { + it('should not expose the message of an http-errors error with expose: false', () => { + const error = createHttpError(404, 'internal detail', { expose: false }); + const result = expressAdapter.mapException(error) as HttpException; + expect(result.getStatus()).toBe(404); + expect(result.getResponse()).toBe('Not Found'); + }); + + it.each([500, 503])( + 'should not map an http-errors error with status %i', + status => { const error = createHttpError(status, 'internal detail'); - const result = expressAdapter.mapException(error) as HttpException; - expect(result.getStatus()).toBe(status); - expect(result.getResponse()).toBe(message); + expect(expressAdapter.mapException(error)).toBe(error); }, ); });