From f361ee8fe92070d6a1ca3baadb6e90c2c71e36a0 Mon Sep 17 00:00:00 2001 From: pq198363-ops <246611021+pq198363-ops@users.noreply.github.com> Date: Sat, 4 Jul 2026 07:41:29 +0800 Subject: [PATCH] security: harden terminal error responses --- README.md | 14 ++++++ src/error-handling.test.ts | 99 ++++++++++++++++++++++++++++++++++++++ src/routes/errors.ts | 81 ++++++++++++++++++++++++++----- 3 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 src/error-handling.test.ts diff --git a/README.md b/README.md index 337ee4f..b61ec1c 100644 --- a/README.md +++ b/README.md @@ -231,6 +231,20 @@ BASE_URL=http://localhost:3001 } ``` +## Error responses + +Write endpoints return stable JSON envelopes for body-level failures. Malformed +JSON is reported as `400 invalid_request` with the message +`Malformed JSON in request body`; the raw parser message and request body are not +echoed back to clients. Bodies over the 100 KiB JSON limit remain +`413 payload_too_large`. + +Unhandled server exceptions are logged with the request id, method, path, error +message, and stack trace. Client-facing `500 internal_error` responses keep the +request id for correlation but always use the generic message +`Unexpected server error` so internal paths or secrets from exception messages +are not leaked. + ## CI/CD On push/PR to `main`, GitHub Actions runs: diff --git a/src/error-handling.test.ts b/src/error-handling.test.ts new file mode 100644 index 0000000..619342e --- /dev/null +++ b/src/error-handling.test.ts @@ -0,0 +1,99 @@ +import { afterEach, beforeEach, describe, it } from "node:test"; +import assert from "node:assert"; +import express from "express"; +import request from "supertest"; +import { createApp } from "./index.js"; +import { installPreRouteMiddleware } from "./middleware/index.js"; +import { installErrorHandlers } from "./routes/errors.js"; +import { usageStore } from "./store/state.js"; + +const originalConsoleError = console.error; + +beforeEach(() => { + usageStore.clear(); +}); + +afterEach(() => { + console.error = originalConsoleError; +}); + +function createThrowingApp() { + const app = express(); + installPreRouteMiddleware(app); + app.get("/boom", () => { + throw new Error("sensitive path /var/private/token.txt"); + }); + installErrorHandlers(app); + return app; +} + +void describe("terminal error handling", () => { + void it("returns a structured 400 for malformed JSON without leaking parser text", async () => { + const res = await request(createApp()) + .post("/api/v1/usage") + .set("Content-Type", "application/json") + .set("X-Request-Id", "bad-json-request") + .send('{"agent":'); + + assert.strictEqual(res.status, 400); + assert.deepStrictEqual(res.body, { + error: "invalid_request", + message: "Malformed JSON in request body", + requestId: "bad-json-request", + }); + assert.strictEqual(JSON.stringify(res.body).includes("Unexpected end"), false); + assert.strictEqual(JSON.stringify(res.body).includes('{"agent":'), false); + }); + + void it("keeps valid JSON writes unaffected", async () => { + const res = await request(createApp()) + .post("/api/v1/usage") + .set("X-Request-Id", "valid-json-request") + .send({ agent: "agent-json", serviceId: "svc-json", requests: 1 }); + + assert.strictEqual(res.status, 201); + assert.deepStrictEqual(res.body, { + agent: "agent-json", + serviceId: "svc-json", + total: 1, + }); + }); + + void it("keeps oversized JSON mapped to payload_too_large", async () => { + const res = await request(createApp()) + .post("/api/v1/usage") + .set("Content-Type", "application/json") + .set("X-Request-Id", "oversized-request") + .send({ agent: "a".repeat(120 * 1024), serviceId: "svc-json", requests: 1 }); + + assert.strictEqual(res.status, 413); + assert.deepStrictEqual(res.body, { + error: "payload_too_large", + message: "request body exceeds the 100 KiB limit", + requestId: "oversized-request", + }); + }); + + void it("redacts client-facing 500 messages while logging internal details", async () => { + const logged: string[] = []; + console.error = (...args: unknown[]) => { + logged.push(args.map(String).join(" ")); + }; + + const res = await request(createThrowingApp()) + .get("/boom") + .set("X-Request-Id", "boom-request"); + + assert.strictEqual(res.status, 500); + assert.deepStrictEqual(res.body, { + error: "internal_error", + message: "Unexpected server error", + method: "GET", + path: "/boom", + requestId: "boom-request", + }); + assert.strictEqual(JSON.stringify(res.body).includes("/var/private"), false); + assert.ok(logged.some((line) => line.includes("boom-request"))); + assert.ok(logged.some((line) => line.includes("/var/private/token.txt"))); + }); +}); diff --git a/src/routes/errors.ts b/src/routes/errors.ts index fd76332..a8a54c7 100644 --- a/src/routes/errors.ts +++ b/src/routes/errors.ts @@ -6,39 +6,98 @@ import { } from "express"; import { getRequestId } from "../types.js"; +type ExpressError = Error & { + body?: unknown; + status?: number; + statusCode?: number; + type?: string; +}; + +function requestIdForError(req: Request): string | undefined { + const middlewareId = getRequestId(req); + if (middlewareId) return middlewareId; + + const incoming = req.header("x-request-id"); + return incoming && incoming.length <= 200 ? incoming : undefined; +} + +function isPayloadTooLargeError(err: unknown): boolean { + return ( + err !== null && + typeof err === "object" && + "type" in err && + (err as ExpressError).type === "entity.too.large" + ); +} + +function isMalformedJsonError(err: unknown): boolean { + if (!(err instanceof SyntaxError)) return false; + const expressError = err as ExpressError; + return ( + expressError.type === "entity.parse.failed" || + expressError.status === 400 || + expressError.statusCode === 400 + ); +} + +function logInternalError( + err: unknown, + req: Request, + requestId: string | undefined +): void { + const error = err instanceof Error ? err : new Error(String(err)); + console.error( + JSON.stringify({ + requestId, + method: req.method, + path: req.path, + message: error.message, + stack: error.stack, + }) + ); +} + /** * Installs the terminal 404 and error handlers after all route modules. */ export function installErrorHandlers(app: Application): void { app.use((req: Request, res: Response) => { + const requestId = requestIdForError(req); res.status(404).json({ error: "not_found", message: `No route for ${req.method} ${req.path}`, - requestId: getRequestId(req), + requestId, }); }); 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" - ) { + const requestId = requestIdForError(req); + + if (isMalformedJsonError(err)) { + res.status(400).json({ + error: "invalid_request", + message: "Malformed JSON in request body", + requestId, + }); + return; + } + + if (isPayloadTooLargeError(err)) { res.status(413).json({ error: "payload_too_large", message: "request body exceeds the 100 KiB limit", - requestId: getRequestId(req), + requestId, }); return; } - const message = err instanceof Error ? err.message : "Unexpected server error"; + + logInternalError(err, req, requestId); res.status(500).json({ error: "internal_error", - message, + message: "Unexpected server error", method: req.method, path: req.path, - requestId: getRequestId(req), + requestId, }); }); }