From 1730ff7e3a238a2bd245b5eb00f6e62c004c58a4 Mon Sep 17 00:00:00 2001 From: svozza Date: Sat, 12 Sep 2026 11:18:07 +0000 Subject: [PATCH] fix(event-handler): stop recursive HTTP error handler dispatch --- docs/features/event-handler/http.md | 2 + packages/event-handler/src/http/Router.ts | 34 ++-- .../unit/http/Router/error-handling.test.ts | 180 +++++++++++++++++- .../tests/unit/http/Router/streaming.test.ts | 30 +++ 4 files changed, 232 insertions(+), 14 deletions(-) diff --git a/docs/features/event-handler/http.md b/docs/features/event-handler/http.md index 15de815aa7..dba081c138 100644 --- a/docs/features/event-handler/http.md +++ b/docs/features/event-handler/http.md @@ -368,6 +368,8 @@ If you need to send custom headers or a different response structure/code, you c !!! tip "You can throw HTTP errors in your route handlers, middleware, or custom error handlers!" +When a custom error handler throws an HTTP error, the router can pass it to another registered handler. Each handler runs at most once in that error-resolution chain. If dispatch would call a handler again, the router returns the HTTP error's built-in status and message instead. This also prevents cycles involving multiple handlers. + === "index.ts" ```ts hl_lines="3 11" diff --git a/packages/event-handler/src/http/Router.ts b/packages/event-handler/src/http/Router.ts index 201a1d6783..86d79f3aeb 100644 --- a/packages/event-handler/src/http/Router.ts +++ b/packages/event-handler/src/http/Router.ts @@ -556,22 +556,30 @@ class Router { } /** - * Handles errors by finding a registered error handler or falling - * back to a default handler. + * Handles errors through registered handlers and built-in responses. + * + * Calls each registered handler at most once per resolution chain. If a + * handler throws an HTTP error that would revisit a handler, returns that + * error's built-in response. * * @param error - The error to handle * @param options - Error resolve options including request context and scope - * @returns A Response object with appropriate status code and error details */ protected async handleError( error: Error, options: ErrorResolveOptions ): Promise { - const handler = this.errorHandlerRegistry.resolve(error); - if (handler !== null) { + const visitedHandlers = new Set(); + let currentError = error; + + while (true) { + const handler = this.errorHandlerRegistry.resolve(currentError); + if (handler === null || visitedHandlers.has(handler)) break; + visitedHandlers.add(handler); + try { const { scope, ...reqCtx } = options; - const body = await handler.apply(scope ?? this, [error, reqCtx]); + const body = await handler.apply(scope ?? this, [currentError, reqCtx]); if ( body instanceof Response || isExtendedAPIGatewayProxyResult(body) || @@ -579,20 +587,20 @@ class Router { ) { return body; } - return this.#errorBodyToWebResponse(body, error); + return this.#errorBodyToWebResponse(body, currentError); } catch (handlerError) { - if (handlerError instanceof HttpError) { - return await this.handleError(handlerError, options); + if (!(handlerError instanceof HttpError)) { + return this.#defaultErrorHandler(handlerError as Error); } - return this.#defaultErrorHandler(handlerError as Error); + currentError = handlerError; } } - if (error instanceof HttpError) { - return error.toWebResponse(); + if (currentError instanceof HttpError) { + return currentError.toWebResponse(); } - return this.#defaultErrorHandler(error); + return this.#defaultErrorHandler(currentError); } /** diff --git a/packages/event-handler/tests/unit/http/Router/error-handling.test.ts b/packages/event-handler/tests/unit/http/Router/error-handling.test.ts index a0911e7db1..db8115a412 100644 --- a/packages/event-handler/tests/unit/http/Router/error-handling.test.ts +++ b/packages/event-handler/tests/unit/http/Router/error-handling.test.ts @@ -8,7 +8,185 @@ import { NotFoundError, Router, } from '../../../../src/http/index.js'; -import { createTestEvent, createTestEventV2 } from '../helpers.js'; +import { + createTestALBEvent, + createTestEvent, + createTestEventV2, +} from '../helpers.js'; + +describe.each([ + { version: 'V1', createEvent: createTestEvent }, + { version: 'V2', createEvent: createTestEventV2 }, + { version: 'ALB', createEvent: createTestALBEvent }, +])('Class: Router - Error Handler Cycles ($version)', ({ createEvent }) => { + it.each([ + { name: 'specific', errorClass: BadRequestError }, + { name: 'catch-all', errorClass: Error }, + ])( + 'stops an async $name handler from re-entering', + async ({ errorClass }) => { + // Prepare + const app = new Router(); + let calls = 0; + const errorHandler = vi.fn(async (error: Error) => { + calls += 1; + // Keep a regression from hanging the test runner. + if (calls > 50) throw new Error('Test bailout'); + throw error; + }); + app.errorHandler(errorClass, errorHandler); + app.get('/test', () => { + throw new BadRequestError('Invalid request'); + }); + + // Act + const result = await app.resolve(createEvent('/test', 'GET'), context); + + // Assess + expect(errorHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(400); + expect(JSON.parse(result.body ?? '{}')).toEqual({ + statusCode: 400, + error: 'BadRequestError', + message: 'Invalid request', + }); + } + ); + + it('stops a synchronous handler from throwing fresh matching errors repeatedly', async () => { + // Prepare + const app = new Router(); + let calls = 0; + const errorHandler = vi.fn(() => { + calls += 1; + if (calls > 50) throw new Error('Test bailout'); + throw new BadRequestError('Replacement error'); + }); + app.errorHandler(BadRequestError, errorHandler); + app.get('/test', () => { + throw new BadRequestError('Original error'); + }); + + // Act + const result = await app.resolve(createEvent('/test', 'GET'), context); + + // Assess + expect(errorHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(400); + expect(JSON.parse(result.body ?? '{}').message).toBe('Replacement error'); + }); + + it('stops a cycle between two handlers', async () => { + // Prepare + const app = new Router(); + let calls = 0; + const firstHandler = vi.fn(async () => { + calls += 1; + if (calls > 50) throw new Error('Test bailout'); + throw new NotFoundError('Translated error'); + }); + const secondHandler = vi.fn(async () => { + throw new BadRequestError('Cycle returns to the first handler'); + }); + app.errorHandler(BadRequestError, firstHandler); + app.errorHandler(NotFoundError, secondHandler); + app.get('/test', () => { + throw new BadRequestError('Original error'); + }); + + // Act + const result = await app.resolve(createEvent('/test', 'GET'), context); + + // Assess + expect(firstHandler).toHaveBeenCalledTimes(1); + expect(secondHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(400); + expect(JSON.parse(result.body ?? '{}').message).toBe( + 'Cycle returns to the first handler' + ); + }); + + it('tracks a shared handler registered for multiple error types', async () => { + // Prepare + const app = new Router(); + let calls = 0; + const errorHandler = vi.fn(async () => { + calls += 1; + if (calls > 50) throw new Error('Test bailout'); + throw new NotFoundError('Translated error'); + }); + app.errorHandler([BadRequestError, NotFoundError], errorHandler); + app.get('/test', () => { + throw new BadRequestError('Original error'); + }); + + // Act + const result = await app.resolve(createEvent('/test', 'GET'), context); + + // Assess + expect(errorHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(404); + expect(JSON.parse(result.body ?? '{}').message).toBe('Translated error'); + }); + + it('allows delegation to a different error handler', async () => { + // Prepare + const app = new Router(); + const firstHandler = vi.fn(async () => { + throw new NotFoundError('Translated error'); + }); + const secondHandler = vi.fn( + async () => new Response('Custom not found', { status: 404 }) + ); + app.errorHandler(BadRequestError, firstHandler); + app.errorHandler(NotFoundError, secondHandler); + app.get('/test', () => { + throw new BadRequestError('Original error'); + }); + + // Act + const result = await app.resolve(createEvent('/test', 'GET'), context); + + // Assess + expect(firstHandler).toHaveBeenCalledTimes(1); + expect(secondHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(404); + expect(result.body).toBe('Custom not found'); + }); + + it('keeps error handler tracking local to concurrent requests', async () => { + // Prepare + const app = new Router(); + const entered = Promise.withResolvers(); + const release = Promise.withResolvers(); + let calls = 0; + const errorHandler = vi.fn(async (error: Error) => { + calls += 1; + if (calls > 50) throw new Error('Test bailout'); + entered.resolve(); + await release.promise; + throw error; + }); + app.errorHandler(BadRequestError, errorHandler); + app.get('/test/:id', ({ req }) => { + throw new BadRequestError(new URL(req.url).pathname); + }); + + // Act + const first = app.resolve(createEvent('/test/first', 'GET'), context); + await entered.promise; + const second = app.resolve(createEvent('/test/second', 'GET'), context); + release.resolve(); + const results = await Promise.all([first, second]); + + // Assess + expect(errorHandler).toHaveBeenCalledTimes(2); + expect(results.map((result) => result.statusCode)).toEqual([400, 400]); + expect( + results.map((result) => JSON.parse(result.body ?? '{}').message) + ).toEqual(['/test/first', '/test/second']); + }); +}); describe.each([ { version: 'V1', createEvent: createTestEvent }, diff --git a/packages/event-handler/tests/unit/http/Router/streaming.test.ts b/packages/event-handler/tests/unit/http/Router/streaming.test.ts index fbdda98ba2..dff306520a 100644 --- a/packages/event-handler/tests/unit/http/Router/streaming.test.ts +++ b/packages/event-handler/tests/unit/http/Router/streaming.test.ts @@ -2,6 +2,7 @@ import { Duplex, PassThrough, Readable } from 'node:stream'; import context from '@aws-lambda-powertools/testing-utils/context'; import { describe, expect, it, vi } from 'vitest'; import { + BadRequestError, Router, streamify, UnauthorizedError, @@ -16,6 +17,35 @@ describe.each([ { version: 'V1', createEvent: createTestEvent }, { version: 'V2', createEvent: createTestEventV2 }, ])('Class: Router - Streaming ($version)', ({ createEvent }) => { + it('streams the built-in HTTP response when an error handler rethrows', async () => { + // Prepare + const app = new Router(); + let calls = 0; + const errorHandler = vi.fn(async (error: Error) => { + calls += 1; + if (calls > 50) throw new Error('Test bailout'); + throw error; + }); + app.errorHandler(BadRequestError, errorHandler); + app.get('/test', () => { + throw new BadRequestError('Invalid request'); + }); + const handler = streamify(app); + const responseStream = new ResponseStream(); + + // Act + const result = await handler( + createEvent('/test', 'GET'), + responseStream, + context + ); + + // Assess + expect(errorHandler).toHaveBeenCalledTimes(1); + expect(result.statusCode).toBe(400); + expect(JSON.parse(result.body).message).toBe('Invalid request'); + }); + it('streams a simple JSON response', async () => { // Prepare const app = new Router();