diff --git a/CHANGELOG.md b/CHANGELOG.md index 9155cd1e3d..6a9a24c9ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## Version 21 +### v21.11.6 + +- Fixed inconsistency between the actual catcher behavior and the error handling documentation: + - Removed conversion of non-`HttpError`s to `BadRequest` before passing them to `errorHandler`; + - A `ResultHandler` configured as `errorHandler` is responsible to handling all errors and responding accordingly. + - The default `errorHandler` is `defaultResultHandler`: + - Using `ensureHttpError()` it coverts non-`HttpError`s to `InternalServerError` and responds with status code `500`; + - The issue has occurred since [v19.0.0](#v1900). + ### v21.11.5 - Fixed: the output type of the `ez.raw()` schema (without an argument) was missing the `raw` property (since v19.0.0). diff --git a/README.md b/README.md index c1cd0b411c..b649d196f7 100644 --- a/README.md +++ b/README.md @@ -858,25 +858,25 @@ const resultHandler = new ResultHandler({ ## Error handling -`ResultHandler` is designed to be the entity responsible for centralized error handling. By default, that center is -the `defaultResultHandler`, however, since much can be customized, you should be aware that there are three possible -origins of errors that could happen in runtime and be handled the following way: - -- Ones related to `Endpoint` execution — handled by a `ResultHandler` assigned to the `EndpointsFactory` produced it: - - The following proprietary classes are available to you for customizing error handling in your `ResultHandler`: - - `InputValidationError` — when request payload does not match the `input` schema of the endpoint or middleware. - The default response status code is `400`, `cause` property contains the original `ZodError`; - - `OutputValidationError` — when returns of the endpoint's `handler` does not match its `output` schema (`500`); - - Errors thrown within endpoint's `handler`: - - `HttpError`, made by `createHttpError()` method of `http-errors` (required peer dependency). The default response - status code is taken from `error.statusCode`; - - Others, inheriting from `Error` class (`500`); -- Ones related to routing, parsing and upload issues — handled by `ResultHandler` assigned to `errorHandler` in config: - - Default is `defaultResultHandler` — it sets the response status code from the corresponding `HttpError`: - `400` for parsing, `404` for routing, `config.upload.limitError.statusCode` for upload issues, or `500` for others. - - `ResultHandler` must handle possible `error` and avoid throwing its own errors, otherwise: -- Ones related to `ResultHandler` execution — handled by `LastResortHandler`: - - Response status code is always `500` and the response itself is a plain text. +All runtime errors are handled by a `ResultHandler`. The default is `defaultResultHandler`. Using `ensureHttpError()` +it normalizes errors into consistent HTTP responses with sensible status codes. Errors can originate from three layers: + +- `Endpoint` execution (including attached `Middleware`): + - Handled by a `ResultHandler` used by `EndpointsFactory` (`defaultEndpointsFactory` uses `defaultResultHandler`); + - `InputValidationError`: request violates `input` schema, the default status code is `400`; + - `OutputValidationError`: handler violates `output` schema, the default status code is `500`; + - `HttpError`: can be thrown in handlers with help of `createHttpError()`, its `.statusCode` is used for response; + - For other errors the default status code is `500`; +- Routing, parsing and upload issues: + - Handled by `ResultHandler` configured as `errorHandler` (the defaults is `defaultResultHandler`); + - Parsing errors: passed through as-is (typically `HttpError` with `4XX` code used for response by default); + - Routing errors: `404` or `405`, based on `wrongMethodBehavior` configuration; + - Upload issues: thrown only if `upload.limitError` is configured (`HttpError::statusCode` can be used for response); + - For other errors the default status code is `500`; +- `ResultHandler` failures: + - Handled by `LastResortHandler` with status code `500` and a plain text response. + +You can customize it by passing a custom `ResultHandler` to `EndpointsFactory` and by configuring `errorHandler`. ## Production mode diff --git a/src/server-helpers.ts b/src/server-helpers.ts index 551f0b8fb4..027384fe57 100644 --- a/src/server-helpers.ts +++ b/src/server-helpers.ts @@ -5,7 +5,7 @@ import { AbstractResultHandler } from "./result-handler"; import { ActualLogger } from "./logger-helpers"; import { CommonConfig, ServerConfig } from "./config-type"; import { ErrorRequestHandler, RequestHandler, Request } from "express"; -import createHttpError, { isHttpError } from "http-errors"; +import createHttpError from "http-errors"; import { lastResortHandler } from "./last-resort"; import { ResultHandlerError } from "./errors"; import { ensureError } from "./common-helpers"; @@ -32,9 +32,7 @@ export const createParserFailureHandler = async (error, request, response, next) => { if (!error) return next(); return errorHandler.execute({ - error: isHttpError(error) - ? error - : createHttpError(400, ensureError(error).message), + error: ensureError(error), request, response, input: null, diff --git a/tests/system/__snapshots__/system.spec.ts.snap b/tests/system/__snapshots__/system.spec.ts.snap index 783e3e5115..f841ce22c5 100644 --- a/tests/system/__snapshots__/system.spec.ts.snap +++ b/tests/system/__snapshots__/system.spec.ts.snap @@ -66,19 +66,19 @@ exports[`App in production mode > Protocol > Should fail on invalid method 1`] = } `; -exports[`App in production mode > Protocol > Should fail on malformed body 1`] = ` +exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = ` { "error": { - "message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/, + "message": "key: Required", }, "status": "error", } `; -exports[`App in production mode > Protocol > Should fail when missing content type header 1`] = ` +exports[`App in production mode > Protocol > Should handle JSON parser failures 1`] = ` { "error": { - "message": "key: Required", + "message": StringMatching /\\(Unexpected end of JSON input\\|Unterminated string in JSON at position 25\\)/, }, "status": "error", } diff --git a/tests/system/system.spec.ts b/tests/system/system.spec.ts index 7089be3e89..a15237e2f1 100644 --- a/tests/system/system.spec.ts +++ b/tests/system/system.spec.ts @@ -1,5 +1,7 @@ import cors from "cors"; import depd from "depd"; +import express from "express"; +import { readFile } from "node:fs/promises"; import { z } from "zod"; import { EndpointsFactory, @@ -10,6 +12,7 @@ import { ResultHandler, BuiltinLogger, Middleware, + ez, } from "../../src"; import { givePort } from "../helpers"; import { setTimeout } from "node:timers/promises"; @@ -100,23 +103,44 @@ describe("App in production mode", async () => { output: z.object({}), handler: async () => setTimeout(5000, {}), }); + const rawEndpoint = new EndpointsFactory(defaultResultHandler).build({ + method: "post", + input: ez.raw(), + output: z.object({ crc: z.number() }), + handler: async ({ input: { raw } }) => ({ crc: raw.length }), + }); + const uploadEndpoint = new EndpointsFactory(defaultResultHandler).buildVoid({ + method: "post", + input: z.object({ avatar: ez.upload() }), + handler: vi.fn(), + }); const routing = { v1: { corsed: corsedEndpoint, faulty: faultyEndpoint, test: testEndpoint, long: longEndpoint, + raw: rawEndpoint, + upload: uploadEndpoint, }, }; vi.spyOn(process.stdout, "write").mockImplementation(vi.fn()); // mutes logo output const config = createConfig({ http: { listen: port }, compression: { threshold: 1 }, + rawParser: express.raw({ limit: 20 }), + upload: { + beforeUpload: ({ request }) => { + if ("trigger" in request.query) throw new Error("beforeUpload failure"); + }, + }, beforeRouting: ({ app, getLogger }) => { depd("express")("Sample deprecation message"); app.use((req, {}, next) => { const childLogger = getLogger(req); assert("isChild" in childLogger && childLogger.isChild); + if (req.path === "/trigger/beforeRouting") + return next(new Error("Failure of beforeRouting triggered")); next(); }); }, @@ -247,6 +271,35 @@ describe("App in production mode", async () => { "Content-Range,X-Content-Range", ); }); + + test("Should handle raw request", async () => { + const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, { + method: "POST", + headers: { "content-type": "application/octet-stream" }, + body: Buffer.from("testing"), + }); + expect(response.status).toBe(200); + const json = await response.json(); + expect(json).toEqual({ status: "success", data: { crc: 7 } }); + }); + + test("Should handle upload request", async () => { + const filename = "logo.svg"; + const logo = await readFile(filename, "utf-8"); + const data = new FormData(); + data.append( + "avatar", + new Blob([logo], { type: "image/svg+xml" }), + filename, + ); + const response = await fetch(`http://localhost:${port}/v1/upload`, { + method: "POST", + body: data, + }); + expect(response.status).toBe(200); + const json = await response.json(); + expect(json).toEqual({ data: {}, status: "success" }); + }); }); describe("Negative", () => { @@ -297,6 +350,38 @@ describe("App in production mode", async () => { expect(text).toBe("Internal Server Error"); expect(errorMethod.mock.lastCall).toMatchSnapshot(); }); + + test("Should treat beforeRouting error as internal", async () => { + const response = await fetch( + `http://127.0.0.1:${port}/trigger/beforeRouting`, + ); + expect(await response.json()).toEqual({ + status: "error", + error: { message: "Internal Server Error" }, + }); + expect(response.status).toBe(500); + }); + + test("Should treat beforeUpload error as internal", async () => { + const filename = "logo.svg"; + const logo = await readFile(filename, "utf-8"); + const data = new FormData(); + data.append( + "avatar", + new Blob([logo], { type: "image/svg+xml" }), + filename, + ); + const response = await fetch( + `http://localhost:${port}/v1/upload?trigger=beforeUpload`, + { method: "POST", body: data }, + ); + expect(response.status).toBe(500); + const json = await response.json(); + expect(json).toEqual({ + error: { message: "Internal Server Error" }, + status: "error", + }); + }); }); describe("Protocol", () => { @@ -316,7 +401,7 @@ describe("App in production mode", async () => { expect(json).toMatchSnapshot(); }); - test("Should fail on malformed body", async () => { + test("Should handle JSON parser failures", async () => { const response = await fetch(`http://127.0.0.1:${port}/v1/test`, { method: "POST", // valid method this time headers: { @@ -337,6 +422,20 @@ describe("App in production mode", async () => { }); }); + test("Should handle Raw parser failures", async () => { + const response = await fetch(`http://127.0.0.1:${port}/v1/raw`, { + method: "POST", + headers: { "content-type": "application/octet-stream" }, + body: Buffer.alloc(100), + }); + expect(response.status).toBe(413); + const json = await response.json(); + expect(json).toEqual({ + status: "error", + error: { message: "request entity too large" }, + }); + }); + test("Should fail when missing content type header", async () => { const response = await fetch(`http://127.0.0.1:${port}/v1/test`, { method: "POST", @@ -446,7 +545,7 @@ describe("App in production mode", async () => { await setTimeout(500); process.emit("FAKE" as "SIGTERM"); expect(infoMethod).toHaveBeenCalledWith("Graceful shutdown", { - sockets: 1, + sockets: expect.any(Number), timeout: 1000, }); await setTimeout(1500); diff --git a/tests/unit/server-helpers.spec.ts b/tests/unit/server-helpers.spec.ts index 6e835558e0..ef1078a3c8 100644 --- a/tests/unit/server-helpers.spec.ts +++ b/tests/unit/server-helpers.spec.ts @@ -35,9 +35,11 @@ describe("Server helpers", () => { test.each([ new SyntaxError("Unexpected end of JSON input"), + new Error("Anything"), createHttpError(400, "Unexpected end of JSON input"), + "just a text", ])( - "the handler should call error handler with correct error code %#", + "the handler should call error handler with given error %#", async (error) => { const errorHandler = new ResultHandler({ positive: vi.fn(), @@ -57,7 +59,7 @@ describe("Server helpers", () => { ); expect(spy).toHaveBeenCalledTimes(1); expect(spy.mock.calls[0][0].error).toEqual( - createHttpError(400, "Unexpected end of JSON input"), + error instanceof Error ? error : new Error(error), ); }, );