From 1b2deb65cd1029dfb59bb98eadcf6bb9df68dd37 Mon Sep 17 00:00:00 2001 From: telemarkdigital-publisher Date: Fri, 21 Aug 2026 14:04:27 +0200 Subject: [PATCH] refactor: centralize API error taxonomy --- README.md | 2 +- docs/api.md | 5 +- docs/architecture.md | 9 +- src/__tests__/errorHandler.test.ts | 306 +++++++++++++--------------- src/index.ts | 307 ++++++++++++++++++++++------- 5 files changed, 383 insertions(+), 246 deletions(-) diff --git a/README.md b/README.md index ec2821b..275bea5 100644 --- a/README.md +++ b/README.md @@ -179,7 +179,7 @@ cross-test bleed. This function is not exposed via any HTTP route. ## Error responses -Handlers use a shared `sendError` helper so 400/404/413/500-style responses keep the canonical `{ error, message, requestId }` shape. The request id is attached before JSON parsing, which keeps body-parser errors correlated with the `X-Request-Id` response header. +Handlers use a typed `ApiError` taxonomy and one final `apiErrorHandler` mapping so each stable error code has one HTTP status and safe client message. Responses include `{ code, error, message, requestId }`; `error` is retained as a compatibility alias for clients that already branch on it. The request id is attached before JSON parsing, which keeps body-parser errors correlated with the `X-Request-Id` response header. ## Contributing diff --git a/docs/api.md b/docs/api.md index 4798f6e..fb3ffaa 100644 --- a/docs/api.md +++ b/docs/api.md @@ -34,6 +34,7 @@ All error responses share a single canonical JSON shape: ```json { + "code": "invalid_request", "error": "invalid_request", "message": "human-readable explanation", "requestId": "0f8c…-uuid" @@ -41,7 +42,9 @@ All error responses share a single canonical JSON shape: ``` Some errors include extra fields (e.g. the `500` handler adds `method` -and `path`), but `error`, `message`, and `requestId` are always present. +and `path`), but `code`, `error`, `message`, and `requestId` are always +present. `error` is a compatibility alias for `code`; new clients should prefer +`code`. ### Error codes diff --git a/docs/architecture.md b/docs/architecture.md index c57e01f..63a1dd3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -83,10 +83,10 @@ Sets four hardening headers on every response: Registered `app.get/post/patch/delete` handlers. Each handler validates its inputs and calls `sendError` for client errors or `res.json` for success. Unhandled exceptions propagate to the error handler via `next(err)`. **10. 404 catch-all** -An `app.use` registered after all routes returns a structured 404 using `sendError` for any path/method combination that did not match a route. +An `app.use` registered after all routes forwards an `ApiError("not_found")` for any path/method combination that did not match a route. **11. Error handler** (4-argument `app.use`) -Catches any error passed to `next(err)` or thrown synchronously in a handler. Translates `entity.too.large` (body-parser overflow) into 413; all other errors become 500. The response always uses the canonical `{ error, message, requestId }` shape. +Catches any error passed to `next(err)` or thrown synchronously in a handler. The `API_ERROR_DEFINITIONS` taxonomy maps each stable `code` to one HTTP status and safe message, including parser errors such as `entity.too.large` and `entity.parse.failed`. Unexpected errors become `500 internal_error` without leaking internal details. The response always uses the canonical `{ code, error, message, requestId }` shape, where `error` is retained as a compatibility alias for `code`. --- @@ -139,10 +139,11 @@ In all cases the `X-Request-Id` header is already set (layer 2 runs first), so t ## Canonical Error Envelope -Every error response — whether from a route handler, the 404 catch-all, or the global error handler — uses the same shape produced by `sendError` in `src/index.ts`: +Every error response — whether from a route handler, the 404 catch-all, or the global error handler — uses the same taxonomy-backed shape produced by `sendError` / `apiErrorHandler` in `src/index.ts`: ```jsonc { + "code": "snake_case_error_code", // machine-readable "error": "snake_case_error_code", // machine-readable "message": "Human-readable detail.", "requestId": "uuid-or-caller-supplied-id", @@ -150,7 +151,7 @@ Every error response — whether from a route handler, the 404 catch-all, or the } ``` -Clients can branch on `error` for programmatic handling and log `requestId` for cross-service tracing. +Clients can branch on `code` for programmatic handling and log `requestId` for cross-service tracing. Existing clients may continue using the `error` alias. --- diff --git a/src/__tests__/errorHandler.test.ts b/src/__tests__/errorHandler.test.ts index 8cdb7ee..fc0804b 100644 --- a/src/__tests__/errorHandler.test.ts +++ b/src/__tests__/errorHandler.test.ts @@ -1,208 +1,166 @@ import request from "supertest"; -import express, { - type Request, - type Response, - type NextFunction, -} from "express"; -import app from "../index"; - -describe("Error handler — 413 payload_too_large", () => { - it("returns 413 when body exceeds 100 KiB", async () => { - // 101 KiB of data — just over the 100kb limit - const oversized = "x".repeat(101 * 1024); - const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send(JSON.stringify({ source: "USD", destination: oversized })); - expect(res.status).toBe(413); +import express, { type NextFunction, type Request } from "express"; +import app, { + API_ERROR_DEFINITIONS, + ApiError, + apiErrorHandler, +} from "../index"; + +const oversizedJson = JSON.stringify({ payload: "x".repeat(101 * 1024) }); + +describe("API error taxonomy unit coverage", () => { + it("maps each stable code to exactly one HTTP status", () => { + expect(API_ERROR_DEFINITIONS.invalid_request.status).toBe(400); + expect(API_ERROR_DEFINITIONS.not_found.status).toBe(404); + expect(API_ERROR_DEFINITIONS.conflict.status).toBe(409); + expect(API_ERROR_DEFINITIONS.internal_error.status).toBe(500); }); - it("413 body contains error: payload_too_large", async () => { - const oversized = "x".repeat(101 * 1024); - const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send(JSON.stringify({ source: "USD", destination: oversized })); - expect(res.body.error).toBe("payload_too_large"); - }); + it("formats domain errors through the centralized middleware", async () => { + const testApp = express(); + testApp.use((req: Request, _res, next: NextFunction) => { + (req as Request & { id?: string }).id = "unit-request-id"; + next(); + }); + testApp.get("/unit", (_req, _res, next) => { + next(new ApiError("conflict", "unit conflict", { field: "version" })); + }); + testApp.use(apiErrorHandler); - it("413 body contains a requestId", async () => { - const oversized = "x".repeat(101 * 1024); - const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send(JSON.stringify({ source: "USD", destination: oversized })); - expect(res.body.requestId).toBeDefined(); - expect(typeof res.body.requestId).toBe("string"); - expect(res.body.requestId.length).toBeGreaterThan(0); - }); + const res = await request(testApp).get("/unit"); - it("413 body does not leak a stack trace", async () => { - const oversized = "x".repeat(101 * 1024); - const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send(JSON.stringify({ source: "USD", destination: oversized })); - expect(JSON.stringify(res.body)).not.toMatch(/at\s+\w+\s+\(/); - expect(res.body.stack).toBeUndefined(); + expect(res.status).toBe(409); + expect(res.body).toMatchObject({ + code: "conflict", + error: "conflict", + message: "unit conflict", + field: "version", + requestId: "unit-request-id", + }); }); +}); - it("413 response echoes a valid X-Request-Id header", async () => { - const oversized = "x".repeat(101 * 1024); - const id = "test-413-request-id"; +describe("Centralized API error middleware integration", () => { + it("returns a consistent 400 validation shape", async () => { const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .set("X-Request-Id", id) - .send(JSON.stringify({ source: "USD", destination: oversized })); - expect(res.status).toBe(413); - expect(res.body.requestId).toBe(id); - }); + .get("/test/domain-validation") + .set("X-Request-Id", "validation-request-id"); - it("accepts a body exactly at the 100 KiB limit (no error)", async () => { - // Build a JSON payload whose total serialized size is at most 100 KiB. - // A field value of ~100 chars keeps us well within the limit. - const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send(JSON.stringify({ source: "USD", destination: "EUR" })); - // Should not be 413 — any other status is acceptable here - expect(res.status).not.toBe(413); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ + code: "invalid_request", + error: "invalid_request", + message: "amount must be a positive number", + field: "amount", + requestId: "validation-request-id", + }); }); -}); -describe("Error handler — 400 invalid_json (malformed body)", () => { - it("returns 400 for malformed JSON", async () => { + it("returns a consistent 404 not-found shape", async () => { const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send("{invalid json}"); - expect(res.status).toBe(400); - expect(res.body.error).toBe("invalid_json"); + .get("/api/v1/not-real") + .set("X-Request-Id", "not-found-request-id"); + + expect(res.status).toBe(404); + expect(res.body).toMatchObject({ + code: "not_found", + error: "not_found", + message: "No route for GET /api/v1/not-real", + requestId: "not-found-request-id", + }); }); - it("malformed JSON body does not leak raw parser text", async () => { + it("returns a consistent 409 conflict shape", async () => { const res = await request(app) - .post("/api/v1/pairs") - .set("Content-Type", "application/json") - .send("{bad"); - expect(res.body.message).toBe("request body is not valid JSON"); + .get("/test/domain-conflict") + .set("X-Request-Id", "conflict-request-id"); + + expect(res.status).toBe(409); + expect(res.body).toMatchObject({ + code: "conflict", + error: "conflict", + message: "resource version conflict", + requestId: "conflict-request-id", + }); }); - it("malformed JSON response includes a requestId", async () => { + it("returns 413 for oversized JSON without leaking parser internals", async () => { const res = await request(app) .post("/api/v1/pairs") .set("Content-Type", "application/json") - .send("{bad"); - expect(res.body.requestId).toBeDefined(); - }); -}); + .set("X-Request-Id", "too-large-request-id") + .send(oversizedJson); -describe("Error handler — 500 internal_error (generic branch)", () => { - let originalEnv: string | undefined; - - beforeAll(() => { - originalEnv = process.env.NODE_ENV; - // Set to non-production so the error message is echoed - process.env.NODE_ENV = "test"; - }); - - afterAll(() => { - process.env.NODE_ENV = originalEnv; + expect(res.status).toBe(413); + expect(res.body).toMatchObject({ + code: "payload_too_large", + error: "payload_too_large", + message: "request body exceeds the 100 KiB limit", + requestId: "too-large-request-id", + }); + expect(JSON.stringify(res.body)).not.toMatch(/at\s+\w+\s+\(/); + expect(res.body.stack).toBeUndefined(); }); - it("returns 500 on an unhandled error thrown by a route", async () => { - // The /api/v1/config PATCH endpoint with a body that passes parsing - // but triggers a runtime error via a specially crafted path is - // hard to replicate without injecting. Instead we test via a route - // that propagates to the error handler: malformed JSON causes express - // to call next(err), but to reach the generic 500 branch we need a - // non-SyntaxError and non-entity.too.large error. - // - // We can invoke the handler directly through supertest by mounting a - // temporary route — but since we only have access to `app`, we simulate - // this by sending a request that will exercise the known route paths. - // - // NOTE: Express does not expose a way to inject arbitrary errors through - // supertest without modifying the app. The generic 500 branch is covered - // via direct unit test of the error-handler shape below. + it("returns 400 for malformed JSON without echoing raw parser text", async () => { const res = await request(app) .post("/api/v1/pairs") .set("Content-Type", "application/json") - .send("{bad json that triggers SyntaxError}"); - // This exercises the error middleware path (parse error → 400) + .set("X-Request-Id", "bad-json-request-id") + .send("{bad"); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ + code: "invalid_json", + error: "invalid_json", + message: "request body is not valid JSON", + requestId: "bad-json-request-id", + }); + expect(JSON.stringify(res.body)).not.toContain("{bad"); }); - it("500 response shape includes method and path fields", async () => { - // Simulate the 500 branch by reaching the error handler with a - // programmatically constructed error object via a route that we - // know passes through the generic error handler. - // We test the shape assertion by calling the error handler directly. - const testApp = express(); - testApp.use(express.json({ limit: "100kb" })); - testApp.use((req: Request, res: Response, next: NextFunction) => { - const id = require("node:crypto").randomUUID(); - (req as Request & { id?: string }).id = id; - res.setHeader("X-Request-Id", id); - next(); - }); - testApp.get("/boom", (req: Request, res: Response, next: NextFunction) => { - next(new Error("deliberate test error")); + it("returns 500 for unexpected errors without leaking details", async () => { + const res = await request(app) + .get("/test/unexpected-error") + .set("X-Request-Id", "unexpected-request-id"); + + expect(res.status).toBe(500); + expect(res.body).toMatchObject({ + code: "internal_error", + error: "internal_error", + message: "An unexpected error occurred", + method: "GET", + path: "/test/unexpected-error", + requestId: "unexpected-request-id", }); - // Re-use the same error handler shape from index.ts - testApp.use( - (err: unknown, req: Request, res: Response, _next: NextFunction) => { - const isProduction = process.env.NODE_ENV === "production"; - const message = isProduction - ? "An unexpected error occurred" - : err instanceof Error - ? err.message - : "Unexpected server error"; - res.status(500).json({ - error: "internal_error", - message, - method: req.method, - path: req.path, - requestId: (req as Request & { id?: string }).id, - }); - }, - ); - - const testRes = await request(testApp).get("/boom"); - expect(testRes.status).toBe(500); - expect(testRes.body.error).toBe("internal_error"); - expect(testRes.body.method).toBe("GET"); - expect(testRes.body.path).toBe("/boom"); - expect(testRes.body.message).toBe("deliberate test error"); - expect(testRes.body.stack).toBeUndefined(); + expect(JSON.stringify(res.body)).not.toContain("secret connection string"); + expect(JSON.stringify(res.body)).not.toMatch(/at\s+\w+\s+\(/); + expect(res.body.stack).toBeUndefined(); }); - it("500 response body does not include a stack trace", async () => { - const testApp = express(); - testApp.use((req: Request, res: Response, next: NextFunction) => { - (req as Request & { id?: string }).id = - require("node:crypto").randomUUID(); - next(); - }); - testApp.get("/boom", (req: Request, res: Response, next: NextFunction) => { - next(new Error("oops")); - }); - testApp.use( - (err: unknown, req: Request, res: Response, _next: NextFunction) => { - res.status(500).json({ - error: "internal_error", - message: - err instanceof Error ? err.message : "Unexpected server error", - method: req.method, - path: req.path, - requestId: (req as Request & { id?: string }).id, - }); - }, - ); - - const testRes = await request(testApp).get("/boom"); - expect(testRes.body.stack).toBeUndefined(); - expect(JSON.stringify(testRes.body)).not.toMatch(/at\s+\w+\s+\(/); + it.each([ + ["validation", () => request(app).get("/test/domain-validation")], + ["not-found", () => request(app).get("/missing-route")], + ["conflict", () => request(app).get("/test/domain-conflict")], + ["unexpected", () => request(app).get("/test/unexpected-error")], + [ + "invalid-json", + () => + request(app) + .post("/api/v1/pairs") + .set("Content-Type", "application/json") + .send("{"), + ], + [ + "payload-too-large", + () => + request(app) + .post("/api/v1/pairs") + .set("Content-Type", "application/json") + .send(oversizedJson), + ], + ])("carries a requestId for %s errors", async (_name, makeRequest) => { + const res = await makeRequest().set("X-Request-Id", "shared-error-id"); + expect(res.body.requestId).toBe("shared-error-id"); }); }); diff --git a/src/index.ts b/src/index.ts index 8a97c65..16017bd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -222,6 +222,131 @@ export type ApiErrorCode = | "insufficient_liquidity" | "request_timeout"; +export type ApiErrorDefinition = { + readonly status: number; + readonly safeMessage: string; + readonly expose: boolean; +}; + +/** + * Single status/message taxonomy for API errors. Explicit route handlers and + * the final Express error middleware both resolve through this map so clients + * can rely on stable codes and statuses. + */ +export const API_ERROR_DEFINITIONS: Record = { + not_found: { + status: 404, + safeMessage: "resource not found", + expose: true, + }, + invalid_request: { + status: 400, + safeMessage: "request is invalid", + expose: true, + }, + invalid_json: { + status: 400, + safeMessage: "request body is not valid JSON", + expose: true, + }, + unauthorized: { + status: 401, + safeMessage: "authentication is required", + expose: true, + }, + forbidden: { + status: 403, + safeMessage: "permission denied", + expose: true, + }, + rate_limited: { + status: 429, + safeMessage: "too many requests", + expose: true, + }, + service_paused: { + status: 503, + safeMessage: "service is paused", + expose: true, + }, + internal_error: { + status: 500, + safeMessage: "An unexpected error occurred", + expose: false, + }, + not_acceptable: { + status: 406, + safeMessage: "requested response format is not acceptable", + expose: true, + }, + payload_too_large: { + status: 413, + safeMessage: "request body exceeds the 100 KiB limit", + expose: true, + }, + conflict: { + status: 409, + safeMessage: "resource conflict", + expose: true, + }, + method_not_allowed: { + status: 405, + safeMessage: "method not allowed", + expose: true, + }, + read_only_mode: { + status: 503, + safeMessage: "service is in read-only mode", + expose: true, + }, + pair_not_registered: { + status: 404, + safeMessage: "pair not registered", + expose: true, + }, + idempotency_conflict: { + status: 409, + safeMessage: "idempotency key conflicts with a different request body", + expose: true, + }, + unsupported_media_type: { + status: 415, + safeMessage: "unsupported media type", + expose: true, + }, + insufficient_liquidity: { + status: 422, + safeMessage: "insufficient liquidity", + expose: true, + }, + request_timeout: { + status: 503, + safeMessage: "Request timed out", + expose: true, + }, +}; + +export class ApiError extends Error { + readonly code: ApiErrorCode; + readonly status: number; + readonly expose: boolean; + readonly extra: ErrorResponseExtra; + + constructor( + code: ApiErrorCode, + message?: string, + extra: ErrorResponseExtra = {}, + ) { + const definition = API_ERROR_DEFINITIONS[code]; + super(message ?? definition.safeMessage); + this.name = "ApiError"; + this.code = code; + this.status = definition.status; + this.expose = definition.expose; + this.extra = extra; + } +} + /** * Validates an inbound X-Request-Id value. * @@ -242,8 +367,29 @@ export const isValidRequestId = (value: string): boolean => const getRequestId = (req: Request): string | undefined => (req as RequestWithId).id; +const writeApiErrorResponse = ( + res: Response, + req: Request, + apiError: ApiError, +) => { + const definition = API_ERROR_DEFINITIONS[apiError.code]; + const body: Record = { + code: apiError.code, + error: apiError.code, + message: apiError.expose ? apiError.message : definition.safeMessage, + ...apiError.extra, + }; + const requestId = getRequestId(req); + if (requestId !== undefined) { + body.requestId = requestId; + } + return res.status(apiError.status).json(body); +}; + /** - * Send the canonical API error body used by explicit handlers and middleware. + * Send the canonical API error body used by explicit handlers. The status + * argument is retained for call-site readability, while the emitted status + * comes from API_ERROR_DEFINITIONS so code-to-status mapping lives in one place. */ const sendError = ( res: Response, @@ -252,10 +398,75 @@ const sendError = ( error: ApiErrorCode, message: string, extra: ErrorResponseExtra = {}, -) => - res - .status(status) - .json({ error, message, ...extra, requestId: getRequestId(req) }); +) => { + const apiError = new ApiError(error, message, extra); + if (status !== apiError.status) { + logger.warn( + { + code: error, + requestedStatus: status, + mappedStatus: apiError.status, + requestId: getRequestId(req), + }, + "api error status resolved from taxonomy", + ); + } + return writeApiErrorResponse(res, req, apiError); +}; + +const hasParserType = (err: unknown, type: string): boolean => + Boolean( + err && + typeof err === "object" && + "type" in err && + (err as { type: unknown }).type === type, + ); + +const toApiError = (err: unknown): ApiError | undefined => { + if (err instanceof ApiError) { + return err; + } + if (hasParserType(err, "entity.too.large")) { + return new ApiError("payload_too_large"); + } + if (hasParserType(err, "entity.parse.failed") || err instanceof SyntaxError) { + return new ApiError("invalid_json"); + } + return undefined; +}; + +export const apiErrorHandler = ( + err: unknown, + req: Request, + res: Response, + next: NextFunction, +) => { + if (res.headersSent) { + next(err); + return; + } + + const apiError = + toApiError(err) ?? + new ApiError("internal_error", undefined, { + method: req.method, + path: req.path, + }); + + if (apiError.code === "internal_error") { + logger.error( + { + err, + requestId: getRequestId(req), + method: req.method, + path: req.path, + }, + "unhandled request error", + ); + } + + writeApiErrorResponse(res, req, apiError); +}; /** * Helper to retrieve the active request timeout in milliseconds. @@ -3056,73 +3267,37 @@ if (process.env.NODE_ENV === "test") { res.end(); }, delay); }); -} -// Unknown route: structured 404 echoing the request id. -app.use((req: Request, res: Response) => { - sendError( - res, - req, - 404, - "not_found", - `No route for ${req.method} ${req.path}`, + app.get( + "/test/domain-validation", + (_req: Request, _res: Response, next: NextFunction) => { + next( + new ApiError("invalid_request", "amount must be a positive number", { + field: "amount", + }), + ); + }, ); -}); -// Final 4-arg error handler. Any handler that throws or calls next(err) -// lands here; the response shape is the same canonical -// { error, message, requestId } as the explicit 400 / 404 bodies so -// clients can branch on `error` uniformly. -app.use((err: unknown, req: Request, res: Response, _next: NextFunction) => { - if ( - err && - typeof err === "object" && - "type" in err && - (err as { type: string }).type === "entity.too.large" - ) { - sendError( - res, - req, - 413, - "payload_too_large", - "request body exceeds the 100 KiB limit", - ); - return; - } - // Malformed JSON body. express.json() raises a SyntaxError tagged with - // `type: "entity.parse.failed"`; map it to a canonical 400 client error - // instead of letting it fall through to the generic 500. The message is - // fixed so the raw parser text (which can echo fragments of the input) is - // never leaked back to the caller. - if ( - err && - typeof err === "object" && - (("type" in err && - (err as { type: string }).type === "entity.parse.failed") || - err instanceof SyntaxError) - ) { - sendError(res, req, 400, "invalid_json", "request body is not valid JSON"); - return; - } - const isProduction = process.env.NODE_ENV === "production"; - logger.error( - { - err, - requestId: getRequestId(req), - method: req.method, - path: req.path, + app.get( + "/test/domain-conflict", + (_req: Request, _res: Response, next: NextFunction) => { + next(new ApiError("conflict", "resource version conflict")); }, - "unhandled request error", ); - const message = isProduction - ? "An unexpected error occurred" - : err instanceof Error - ? err.message - : "Unexpected server error"; - sendError(res, req, 500, "internal_error", message, { - method: req.method, - path: req.path, + + app.get("/test/unexpected-error", () => { + throw new Error("secret connection string"); }); +} + +// Unknown route: structured 404 echoing the request id. +app.use((req: Request, _res: Response, next: NextFunction) => { + next(new ApiError("not_found", `No route for ${req.method} ${req.path}`)); }); +// Final 4-arg error handler. Any thrown/next(err) domain, parser, or +// unexpected error lands in the same safe formatter. +app.use(apiErrorHandler); + export default app;