diff --git a/src/api/error-handler.ts b/src/api/error-handler.ts new file mode 100644 index 000000000..c60befed5 --- /dev/null +++ b/src/api/error-handler.ts @@ -0,0 +1,104 @@ +// The app's global error boundary (`app.onError`). +// +// ── WHAT THIS DOES AND DOES NOT CHANGE ─────────────────────────────────────────────────────────────── +// Hono ALWAYS installs a default error handler internally (hono-base.js), so an uncaught route throw was +// never an unhandled crash: it already became a 500, and `compose.js` sets `context.error` BEFORE dispatching +// to whichever handler is installed — which is what worker-posthog.ts's capture middleware reads. Installing +// this one preserves that (the assignment is unconditional and upstream of the handler), so error capture is +// unaffected. What the default gets WRONG for this app is the shape of what it returns and logs: +// +// 1. `c.text("Internal Server Error", 500)` — a text/plain body from a JSON API. Every other route in this +// app answers `{ error: ... }`, so a client parsing JSON hits a parse failure on precisely the response +// it most needs to understand. Now a 500 is `{ error: "internal_error" }` like every other error here. +// +// 2. `console.error(err)` — a raw Error object. This codebase's log forwarder keys on STRUCTURED JSON +// (`{level:"error", event, ...}`); a bare Error is not classified, and carries no method/path, so a +// production 500 could not be traced back to the route that raised it. Now it is one structured line +// with the request's method and path. +// +// ── WHAT IS DELIBERATELY PRESERVED ─────────────────────────────────────────────────────────────────── +// An `HTTPException` carries its OWN response and status. Hono's default returns it verbatim, and so does +// this: swallowing it into a 500 would turn every intentional 401/403/404/413/422 raised by middleware into +// an opaque server error, which is the single most damaging thing a global handler can get wrong. +// +// ── WHAT NEVER REACHES THE CLIENT ──────────────────────────────────────────────────────────────────── +// The body carries a fixed code and nothing else. No message, no stack, no cause — several routes here are +// unauthenticated, and an error string can carry a query fragment, a binding name, or an upstream URL. The +// detail goes to the log, which is operator-only, and is bounded there. +import type { Context, MiddlewareHandler } from "hono"; +import { errorMessage, errorStack } from "../utils/json"; + +/** The single body every unexpected failure returns. A stable code rather than prose: a client can branch on + * it, and it cannot accidentally grow to include something internal. */ +export const INTERNAL_ERROR_BODY = { error: "internal_error" } as const; + +/** + * The global `onError` handler. Exported so it can be tested through a real Hono dispatch rather than by + * faking a Context — the failure modes worth pinning (status preservation, no leakage) are only meaningful + * end to end. + */ +export function handleAppError(error: Error, c: Context): Response { + // Duck-typed on `getResponse` exactly as Hono's own default handler does, rather than + // `instanceof HTTPException`: a workspace with several packages depending on hono can end up with two + // copies in one module graph, and an `instanceof` check across them would collapse an intentional 4xx + // into a 500. Asserted with a hand-rolled foreign exception in the tests. + if (typeof error === "object" && "getResponse" in error && typeof (error as { getResponse: unknown }).getResponse === "function") { + const response = (error as unknown as { getResponse: () => Response }).getResponse(); + // `c.newResponse(res.body, res)` rather than returning `res` directly, mirroring Hono's default: it + // re-applies the context's own headers (CORS, content-type negotiation) that middleware already set. + return c.newResponse(response.body, response); + } + console.error( + JSON.stringify({ + level: "error", + event: "unhandled_route_error", + method: c.req.method, + // `c.req.path` is the matched path WITHOUT the query string, so a token or id in a query parameter + // cannot ride into the log. + path: c.req.path, + message: errorMessage(error), + stack: errorStack(error), + }), + ); + return c.json(INTERNAL_ERROR_BODY, 500); +} + +/** + * The other half of the boundary: a thrown NON-Error. + * + * Hono's `compose()` routes a caught throw to `onError` only when it is `instanceof Error` -- otherwise it + * RE-THROWS (`else { throw err; }`). So `throw "a string"`, a rejected promise carrying a plain object, or a + * library that throws a non-Error escapes `app.onError` entirely: no JSON response, no structured log, and + * no `context.error`, so the PostHog capture middleware never sees it either. Verified empirically, not + * assumed -- a test throws a bare string and, without this, the request rejects rather than responding. + * + * A try/catch around `next()` is the right shape for exactly this case and ONLY this case: an `Error` thrown + * below has already been converted to a response several dispatch levels down (which is why + * worker-posthog.ts documents that a try/catch here would never see one), so this adds no second handling + * path for the normal case -- it catches precisely what `onError` structurally cannot. + * + * Register OUTERMOST so it also covers a non-Error thrown by another middleware. + */ +export function nonErrorBoundary(): MiddlewareHandler { + return async (c, next) => { + try { + await next(); + } catch (thrown) { + // An Error reaching here would mean Hono's own handling changed; re-throw rather than silently + // duplicating onError's job, so that change surfaces instead of being masked. + if (thrown instanceof Error) throw thrown; + console.error( + JSON.stringify({ + level: "error", + event: "unhandled_non_error_throw", + method: c.req.method, + path: c.req.path, + // `String(thrown)` rather than the value: a thrown object could serialize to something enormous + // or circular, and this line has to be loggable no matter what was thrown. + thrown: String(thrown).slice(0, 500), + }), + ); + return c.json(INTERNAL_ERROR_BODY, 500); + } + }; +} diff --git a/src/api/routes.ts b/src/api/routes.ts index 6e6b6bca9..96ded3b59 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -1,5 +1,6 @@ import { Hono, type Context } from "hono"; import { requiresApiToken } from "../auth/route-auth"; +import { handleAppError, nonErrorBoundary } from "./error-handler"; import { createWorkerPostHogErrorMiddleware } from "./worker-posthog"; import { z } from "zod"; import { parsePositiveInt } from "../utils/json"; @@ -1162,6 +1163,13 @@ function internalOpsAgentConfig(env: Env): OpsAgentConfig { export function createApp() { const app = new Hono(); + // The global error boundary. Hono installs its own default regardless, so this REPLACES a handler that + // returned a text/plain body from a JSON API and logged a raw Error the forwarder cannot classify -- + // see error-handler.ts for what it preserves (HTTPException status) and what it refuses to leak. + app.onError(handleAppError); + // Registered OUTERMOST: Hono routes only `instanceof Error` to onError and RE-THROWS anything else, so a + // thrown non-Error would otherwise escape the boundary entirely -- no response, no log, no c.error. + app.use("*", nonErrorBoundary()); // Registered FIRST/outermost so it wraps every other middleware and route below, including a thrown // exception from the CORS/rate-limit middleware right after this. REPLACES the old Sentry middleware // entirely (2026-07-25 epic #8286 correction: full replacement, not a parallel-run). No-ops completely diff --git a/test/unit/api-error-handler.test.ts b/test/unit/api-error-handler.test.ts new file mode 100644 index 000000000..379396fe9 --- /dev/null +++ b/test/unit/api-error-handler.test.ts @@ -0,0 +1,218 @@ +import { describe, expect, it, vi } from "vitest"; +import { Hono } from "hono"; +import { HTTPException } from "hono/http-exception"; +import { handleAppError, nonErrorBoundary, INTERNAL_ERROR_BODY } from "../../src/api/error-handler"; + +// The global error boundary. Every assertion below runs through a REAL Hono dispatch rather than a faked +// Context: the properties worth pinning (status preservation, no leakage, `c.error` still being set for the +// PostHog capture) are only meaningful end to end, and a hand-built context would let all three pass while +// the wiring was wrong. + +/** A minimal app wired exactly the way createApp wires it, with routes that raise each failure shape. */ +function appWithErrors() { + const app = new Hono(); + app.onError(handleAppError); + app.use("*", nonErrorBoundary()); + app.get("/boom", () => { + throw new Error("upstream token abc123 rejected at https://internal.example/db"); + }); + app.get("/http-exception/:status", (c) => { + throw new HTTPException(Number(c.req.param("status")) as 401, { message: "nope" }); + }); + app.get("/non-error", () => { + // A thrown non-Error still has to produce a response rather than escaping the boundary. + throw "a bare string"; + }); + app.get("/ok", (c) => c.json({ ok: true })); + return app; +} + +describe("global onError handler", () => { + it("REGRESSION: an unexpected throw returns JSON, not Hono's default text/plain body", async () => { + const warn = vi.spyOn(console, "error").mockImplementation(() => undefined); + const res = await appWithErrors().request("/boom"); + expect(res.status).toBe(500); + // Every other route in this app answers `{ error: ... }`; a text/plain 500 breaks a client's JSON parse + // on precisely the response it most needs to read. + expect(res.headers.get("content-type")).toContain("application/json"); + expect(await res.json()).toEqual(INTERNAL_ERROR_BODY); + warn.mockRestore(); + }); + + it("REGRESSION: the response body leaks NOTHING — no message, stack, URL or token from the error", async () => { + const warn = vi.spyOn(console, "error").mockImplementation(() => undefined); + const body = await (await appWithErrors().request("/boom")).text(); + for (const secret of ["abc123", "internal.example", "upstream token", "rejected", "at /", ".ts:"]) { + expect(body).not.toContain(secret); + } + // The whole body is the fixed code and nothing else. + expect(JSON.parse(body)).toEqual({ error: "internal_error" }); + warn.mockRestore(); + }); + + it("logs ONE structured line the forwarder can classify, with method and path for correlation", async () => { + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + await appWithErrors().request("/boom", { method: "GET" }); + expect(errors).toHaveLength(1); + const logged = JSON.parse(errors[0] as string) as Record; + // Structured, not a raw Error object — the forwarder keys on `level`/`event`. + expect(logged).toMatchObject({ level: "error", event: "unhandled_route_error", method: "GET", path: "/boom" }); + // The detail the BODY withholds lives here, where only an operator sees it. + expect(String(logged["message"])).toContain("abc123"); + expect(typeof logged["stack"]).toBe("string"); + warn.mockRestore(); + }); + + it("REGRESSION: a query string never reaches the log — `c.req.path` is the matched path only", async () => { + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + await appWithErrors().request("/boom?token=supersecret&id=42"); + expect(errors[0]).not.toContain("supersecret"); + expect(JSON.parse(errors[0] as string).path).toBe("/boom"); + warn.mockRestore(); + }); + + it("REGRESSION: an HTTPException keeps its OWN status — an intentional 4xx never collapses into a 500", async () => { + // The single most damaging thing a global handler can get wrong: turning every deliberate 401/403/404 + // raised by middleware into an opaque server error. + const app = appWithErrors(); + for (const status of [400, 401, 403, 404, 413, 422, 429]) { + const res = await app.request(`/http-exception/${status}`); + expect({ status, got: res.status }).toEqual({ status, got: status }); + } + }); + + it("an HTTPException is NOT logged as an unhandled error — it is an outcome, not a fault", async () => { + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + await appWithErrors().request("/http-exception/401"); + expect(errors).toEqual([]); + warn.mockRestore(); + }); + + it("REGRESSION: a thrown NON-Error is contained — Hono re-throws those, so onError alone never sees them", async () => { + // Verified empirically: without nonErrorBoundary the request REJECTS rather than responding, because + // compose() only routes `instanceof Error` to onError and re-throws everything else. + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + const res = await appWithErrors().request("/non-error"); + expect(res.status).toBe(500); + expect(await res.json()).toEqual(INTERNAL_ERROR_BODY); + const logged = JSON.parse(errors[0] as string) as Record; + expect(logged).toMatchObject({ level: "error", event: "unhandled_non_error_throw", method: "GET", path: "/non-error" }); + expect(String(logged["thrown"])).toContain("a bare string"); + warn.mockRestore(); + }); + + it("the non-Error boundary does NOT duplicate onError: an Error thrown below is handled once, by onError", async () => { + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + await appWithErrors().request("/boom"); + // Exactly one log line, and it is onError's -- the boundary re-throws Errors rather than handling them + // a second time, so a change in Hono's own routing would surface instead of being masked. + expect(errors).toHaveLength(1); + expect(JSON.parse(errors[0] as string).event).toBe("unhandled_route_error"); + warn.mockRestore(); + }); + + it("a huge or circular thrown value still produces a bounded, loggable line", async () => { + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + const app = new Hono(); + app.onError(handleAppError); + app.use("*", nonErrorBoundary()); + const circular: Record = { big: "x".repeat(5000) }; + circular["self"] = circular; + app.get("/circular", () => { + throw circular; + }); + const res = await app.request("/circular"); + expect(res.status).toBe(500); + expect(String(JSON.parse(errors[0] as string).thrown).length).toBeLessThanOrEqual(500); + warn.mockRestore(); + }); + + it("INVARIANT: a successful route is untouched — the boundary costs nothing on the happy path", async () => { + const res = await appWithErrors().request("/ok"); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ ok: true }); + }); + + it("the boundary's own contract: it re-throws an Error rather than handling it a second time", async () => { + // Exercised directly rather than through dispatch, because Hono converts an Error to a response several + // levels below and one can never reach the boundary in normal operation. The guard exists so that if + // Hono's routing ever changes, the Error surfaces to onError instead of being silently relabelled as an + // `unhandled_non_error_throw` -- so the contract is worth pinning even though today nothing triggers it. + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + const context = { req: { method: "GET", path: "/direct" }, json: () => new Response("unused") } as unknown as Parameters>[0]; + const boundary = nonErrorBoundary(); + const raised = new Error("an Error reached the boundary"); + await expect( + boundary(context, () => { + throw raised; + }), + ).rejects.toBe(raised); + // Re-thrown untouched, and NOT logged as a non-Error throw. + expect(errors).toEqual([]); + warn.mockRestore(); + }); + + it("carriesOwnResponse duck-types on getResponse, so a second hono copy cannot collapse a 4xx into a 500", async () => { + // `instanceof HTTPException` would fail across two copies of hono in one module graph -- a real hazard + // in a workspace with several packages depending on it. A hand-rolled object carrying getResponse is + // treated exactly like a real HTTPException. + const app = new Hono(); + app.onError(handleAppError); + app.use("*", nonErrorBoundary()); + app.get("/foreign", () => { + const foreign = new Error("from another hono copy") as Error & { getResponse: () => Response }; + foreign.getResponse = () => new Response("teapot", { status: 418 }); + throw foreign; + }); + const errors: string[] = []; + const warn = vi.spyOn(console, "error").mockImplementation((line: unknown) => { + errors.push(String(line)); + }); + const res = await app.request("/foreign"); + expect(res.status).toBe(418); + expect(errors).toEqual([]); // an intentional status is an outcome, not a fault + warn.mockRestore(); + }); + + it("REGRESSION: `c.error` is still set, so the PostHog capture middleware keeps seeing failures", async () => { + // Hono's compose() assigns `context.error` BEFORE dispatching to whichever handler is installed, so a + // custom onError must not break error capture. Asserted through a middleware reading c.error exactly + // the way worker-posthog.ts does. + const seen: string[] = []; + const app = new Hono(); + app.onError(handleAppError); + app.use("*", async (c, next) => { + await next(); + if (c.error) seen.push(c.error.message); + }); + app.get("/boom", () => { + throw new Error("captured"); + }); + const warn = vi.spyOn(console, "error").mockImplementation(() => undefined); + const res = await app.request("/boom"); + expect(res.status).toBe(500); + expect(seen).toEqual(["captured"]); + warn.mockRestore(); + }); +});