From acb3d02af087d0fb073f706636bf6dbaf533b4f3 Mon Sep 17 00:00:00 2001 From: Echo Date: Tue, 1 Sep 2026 14:01:21 -0400 Subject: [PATCH] feat: structured JSON errors --- src/__tests__/handlers.test.ts | 72 ++++++++++++++- src/__tests__/http-errors.test.ts | 105 ++++++++++++++++++++++ src/__tests__/swagger-generator.test.ts | 36 ++++++++ src/handlers/index.ts | 49 +++++++++-- src/index.ts | 14 +++ src/lib/analytics-wrapper.ts | 21 ++++- src/lib/http-errors.ts | 112 ++++++++++++++++++++++++ src/lib/swagger-generator.ts | 45 +++++++++- 8 files changed, 436 insertions(+), 18 deletions(-) create mode 100644 src/__tests__/http-errors.test.ts create mode 100644 src/__tests__/swagger-generator.test.ts create mode 100644 src/lib/http-errors.ts diff --git a/src/__tests__/handlers.test.ts b/src/__tests__/handlers.test.ts index b7a4b57..4b71152 100644 --- a/src/__tests__/handlers.test.ts +++ b/src/__tests__/handlers.test.ts @@ -88,13 +88,50 @@ describe("handlers", () => { expect(body.checks.database.status).toBe(true); }); - it("returns 503 when unhealthy", async () => { + it("returns a structured JSON error for an unhealthy detailed check", async () => { + const cache = createMockCache({ + detailedHealthCheck: mock(async () => ({ + status: "unhealthy" as const, + checks: { + database: { status: false, latency: 1 }, + slackApi: { status: true }, + queueDepth: 0, + queueDetail: { newUser: 0, refresh: 0 }, + memoryUsage: { heapUsed: 50, heapTotal: 100, percentage: 50 }, + }, + uptime: 1234, + })), + }); + const handlers = createHandlers(cache); + const response = await handlers.handleHealthCheck( + new Request("http://localhost/health?detailed=true"), + noopAnalytics, + ); + const body = await jsonBody<{ + error: { code: string; hint: string }; + checks: { database: { status: boolean } }; + }>(response); + + expect(response.status).toBe(503); + expect(body.error.code).toBe("SERVICE_UNHEALTHY"); + expect(body.checks.database.status).toBe(false); + }); + + it("returns a structured JSON error when unhealthy", async () => { const cache = createMockCache({ healthCheck: mock(async () => false) }); const handlers = createHandlers(cache); const request = new Request("http://localhost/health"); const response = await handlers.handleHealthCheck(request, noopAnalytics); + const body = await jsonBody<{ + error: { code: string; message: string; hint: string }; + }>(response); expect(response.status).toBe(503); + expect(response.headers.get("content-type")).toContain( + "application/json", + ); + expect(body.error.code).toBe("CACHE_UNAVAILABLE"); + expect(body.error.hint.length).toBeGreaterThan(0); }); }); @@ -154,8 +191,13 @@ describe("handlers", () => { const handlers = createHandlers(cache); const request = new Request("http://localhost/emojis/nonexistent"); const response = await handlers.handleGetEmoji(request, noopAnalytics); + const body = await jsonBody<{ error: { code: string; hint: string } }>( + response, + ); expect(response.status).toBe(404); + expect(body.error.code).toBe("EMOJI_NOT_FOUND"); + expect(body.error.hint.length).toBeGreaterThan(0); }); it("returns native emoji when not cached", async () => { @@ -172,6 +214,21 @@ describe("handlers", () => { }); describe("handleEmojiRedirect", () => { + it("returns a structured 404 when no redirect target exists", async () => { + const handlers = createHandlers(createMockCache()); + const response = await handlers.handleEmojiRedirect( + new Request("http://localhost/emojis/definitely_not_an_emoji/r"), + noopAnalytics, + ); + const body = await jsonBody<{ error: { code: string; hint: string } }>( + response, + ); + + expect(response.status).toBe(404); + expect(body.error.code).toBe("EMOJI_NOT_FOUND"); + expect(body.error.hint.length).toBeGreaterThan(0); + }); + it("redirects to native emoji when not cached", async () => { const cache = createMockCache(); const handlers = createHandlers(cache); @@ -213,9 +270,18 @@ describe("handlers", () => { }); const response = await handlers.handlePurgeUser(request, noopAnalytics); - // This will depend on whether BEARER_TOKEN is set in the test env - // The important thing is it doesn't crash + // This depends on whether BEARER_TOKEN was set before config was imported. expect(response.status).toBeOneOf([200, 401, 500]); + if (response.status >= 400) { + const body = await jsonBody<{ + error: { code: string; message: string; hint: string }; + }>(response); + expect(body.error.code).toBeOneOf([ + "UNAUTHORIZED", + "AUTH_NOT_CONFIGURED", + ]); + expect(body.error.hint.length).toBeGreaterThan(0); + } if (origToken) process.env.BEARER_TOKEN = origToken; }); diff --git a/src/__tests__/http-errors.test.ts b/src/__tests__/http-errors.test.ts new file mode 100644 index 0000000..7be3668 --- /dev/null +++ b/src/__tests__/http-errors.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, it, mock } from "bun:test"; +import type { SlackCache } from "../cache"; +import { createAnalyticsWrapper } from "../lib/analytics-wrapper"; +import { + createFallbackHandler, + internalErrorResponse, + jsonError, +} from "../lib/http-errors"; + +type ErrorBody = { + error: { code: string; message: string; hint: string }; +}; + +async function body(response: Response): Promise { + return (await response.json()) as ErrorBody; +} + +describe("structured HTTP errors", () => { + it("returns a JSON error envelope with a resolution hint", async () => { + const response = jsonError( + 404, + "THING_NOT_FOUND", + "Missing.", + "Try again.", + ); + + expect(response.status).toBe(404); + expect(response.headers.get("content-type")).toContain("application/json"); + expect(await body(response)).toEqual({ + error: { + code: "THING_NOT_FOUND", + message: "Missing.", + hint: "Try again.", + }, + }); + }); + + it("returns a structured 404 for an unknown route", async () => { + const fallback = createFallbackHandler({ "/health": { GET: () => {} } }); + const response = fallback(new Request("http://localhost/missing")); + + expect(response.status).toBe(404); + expect((await body(response)).error.code).toBe("ROUTE_NOT_FOUND"); + }); + + it("returns a structured 405 and Allow header for a known route", async () => { + const fallback = createFallbackHandler({ + "/users/:id": { GET: () => {} }, + "/users/:id/purge": { POST: () => {} }, + }); + const response = fallback( + new Request("http://localhost/users/U123", { method: "POST" }), + ); + + expect(response.status).toBe(405); + expect(response.headers.get("allow")).toBe("GET"); + expect((await body(response)).error.code).toBe("METHOD_NOT_ALLOWED"); + }); + + it("prefers an exact static route over an earlier dynamic pattern", async () => { + const fallback = createFallbackHandler({ + "/emojis/:name": { GET: () => {} }, + "/emojis/purge": { POST: () => {} }, + }); + const response = fallback( + new Request("http://localhost/emojis/purge", { method: "DELETE" }), + ); + + expect(response.status).toBe(405); + expect(response.headers.get("allow")).toBe("POST"); + expect((await body(response)).error.code).toBe("METHOD_NOT_ALLOWED"); + }); + + it("returns a safe structured 500 without leaking exception details", async () => { + const response = internalErrorResponse(); + const error = (await body(response)).error; + + expect(response.status).toBe(500); + expect(error.code).toBe("INTERNAL_ERROR"); + expect(error.message).not.toContain("stack"); + }); + + it("converts thrown API handler errors into structured JSON", async () => { + const recordRequest = mock(() => {}); + const withAnalytics = createAnalyticsWrapper({ + recordRequest, + } as unknown as SlackCache); + const handler = withAnalytics("/example", "GET", async () => { + throw new Error("database password must not leak"); + }); + const originalConsoleError = console.error; + console.error = mock(() => {}); + try { + const response = await handler(new Request("http://localhost/example")); + const error = (await body(response)).error; + + expect(response.status).toBe(500); + expect(error.code).toBe("INTERNAL_ERROR"); + expect(JSON.stringify(error)).not.toContain("password"); + expect(recordRequest).toHaveBeenCalled(); + } finally { + console.error = originalConsoleError; + } + }); +}); diff --git a/src/__tests__/swagger-generator.test.ts b/src/__tests__/swagger-generator.test.ts new file mode 100644 index 0000000..5bde442 --- /dev/null +++ b/src/__tests__/swagger-generator.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from "bun:test"; +import { SwaggerGenerator } from "../lib/swagger-generator"; +import { apiResponse, createRoute } from "../types/routes"; + +describe("SwaggerGenerator error responses", () => { + it("publishes the structured error schema for declared and unexpected errors", () => { + const generator = new SwaggerGenerator(); + generator.addRoutes({ + "/example": { + GET: createRoute(() => Response.json({ ok: true }), { + summary: "Example", + responses: Object.fromEntries([ + apiResponse(200, "Success", { type: "object" }), + apiResponse(404, "Not found"), + ]), + }), + }, + }); + + type ErrorResponses = Record< + string, + { content?: { "application/json": { schema: { $ref?: string } } } } + >; + const spec = generator.getSpec() as unknown as { + paths: Record>; + components: { schemas: Record }; + }; + const responses = spec.paths["/example"]?.get?.responses; + const errorRef = (status: string) => + responses?.[status]?.content?.["application/json"].schema.$ref; + + expect(spec.components.schemas.ErrorResponse).toBeDefined(); + expect(errorRef("404")).toBe("#/components/schemas/ErrorResponse"); + expect(errorRef("500")).toBe("#/components/schemas/ErrorResponse"); + }); +}); diff --git a/src/handlers/index.ts b/src/handlers/index.ts index 8d35e96..132b62d 100644 --- a/src/handlers/index.ts +++ b/src/handlers/index.ts @@ -7,6 +7,7 @@ import type { SlackCache } from "../cache"; import { config } from "../config"; import type { RouteHandlerWithAnalytics } from "../lib/analytics-wrapper"; import { lastSegment, pathSegment, queryParam } from "../lib/fast-url"; +import { jsonError } from "../lib/http-errors"; /** * Parse a string to a positive integer, returning a fallback if invalid @@ -40,12 +41,22 @@ export function createHandlers(cache: SlackCache) { if (!token) { console.error("BEARER_TOKEN is not configured"); recordAnalytics(500); - return new Response("Server misconfigured", { status: 500 }); + return jsonError( + 500, + "AUTH_NOT_CONFIGURED", + "Administrative authentication is not configured.", + "Ask the service operator to configure BEARER_TOKEN.", + ); } const authHeader = request.headers.get("authorization") || ""; if (authHeader !== `Bearer ${token}`) { recordAnalytics(401); - return new Response("Unauthorized", { status: 401 }); + return jsonError( + 401, + "UNAUTHORIZED", + "A valid bearer token is required for this endpoint.", + "Send the service's token in the Authorization: Bearer header.", + ); } return null; } @@ -65,7 +76,16 @@ export function createHandlers(cache: SlackCache) { ? 200 : 200; recordAnalytics(statusCode); - return Response.json(health, { status: statusCode }); + if (statusCode === 503) { + return jsonError( + 503, + "SERVICE_UNHEALTHY", + "One or more required service checks failed.", + "Inspect the checks object, resolve failed dependencies, and retry.", + { extra: health }, + ); + } + return Response.json(health); } const isHealthy = await cache.healthCheck(); @@ -78,9 +98,12 @@ export function createHandlers(cache: SlackCache) { }); } else { recordAnalytics(503); - return Response.json( - { status: "unhealthy", error: "Cache connection failed" }, - { status: 503 }, + return jsonError( + 503, + "CACHE_UNAVAILABLE", + "The cache database is unavailable.", + "Retry later or inspect /health?detailed=true for diagnostic checks.", + { extra: { status: "unhealthy", cache: false } }, ); } }; @@ -191,7 +214,12 @@ export function createHandlers(cache: SlackCache) { const nativeEmojiUrl = getEmojiUrl(emojiName); if (!nativeEmojiUrl) { recordAnalytics(404); - return Response.json({ message: "Emoji not found" }, { status: 404 }); + return jsonError( + 404, + "EMOJI_NOT_FOUND", + `No cached or native emoji named "${emojiName}" was found.`, + "Check the name with GET /emojis and retry without surrounding colons.", + ); } recordAnalytics(200); @@ -220,7 +248,12 @@ export function createHandlers(cache: SlackCache) { const nativeEmojiUrl = getEmojiUrl(emojiName); if (!nativeEmojiUrl) { recordAnalytics(404); - return Response.json({ message: "Emoji not found" }, { status: 404 }); + return jsonError( + 404, + "EMOJI_NOT_FOUND", + `No cached or native emoji named "${emojiName}" was found.`, + "Check the name with GET /emojis and retry without surrounding colons.", + ); } recordAnalytics(302); diff --git a/src/index.ts b/src/index.ts index 503e921..89bc119 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,6 +5,10 @@ import { SlackCache } from "./cache"; import { config } from "./config"; import dashboard from "./dashboard.html"; import { addCorsHeaders, corsPreflightResponse } from "./lib/cors"; +import { + createFallbackHandler, + internalErrorResponse, +} from "./lib/http-errors"; import { swaggerGenerator } from "./lib/swagger-generator"; import { createApiRoutes } from "./routes/api-routes"; import { SlackWrapper } from "./slackWrapper"; @@ -135,9 +139,19 @@ const allRoutes = { ...typedRoutes, }; +const fallbackHandler = createFallbackHandler(allRoutes); + // Start the server const server = serve({ routes: allRoutes, + fetch(request) { + if (request.method === "OPTIONS") return corsPreflightResponse(); + return addCorsHeaders(fallbackHandler(request)); + }, + error(error) { + console.error("Unhandled request error:", error); + return addCorsHeaders(internalErrorResponse()); + }, port: config.port, development: config.development, }); diff --git a/src/lib/analytics-wrapper.ts b/src/lib/analytics-wrapper.ts index aefc296..42badfa 100644 --- a/src/lib/analytics-wrapper.ts +++ b/src/lib/analytics-wrapper.ts @@ -5,6 +5,7 @@ import type { SlackCache } from "../cache"; import { addCorsHeaders, corsPreflightResponse } from "./cors"; import { fastPathname } from "./fast-url"; +import { internalErrorResponse } from "./http-errors"; export type AnalyticsRecorder = (statusCode: number) => void; export type RouteHandlerWithAnalytics = ( @@ -12,6 +13,20 @@ export type RouteHandlerWithAnalytics = ( recordAnalytics: AnalyticsRecorder, ) => Promise | Response; +async function runHandler( + handler: RouteHandlerWithAnalytics, + request: Request, + recordAnalytics: AnalyticsRecorder, +): Promise { + try { + return await handler(request, recordAnalytics); + } catch (error) { + console.error("API request failed:", error); + recordAnalytics(500); + return internalErrorResponse(); + } +} + /** * Creates analytics wrapper with injected cache. * Pre-computes static values at registration time to minimize per-request work. @@ -44,7 +59,7 @@ export function createAnalyticsWrapper(cache: SlackCache) { ); }; - const response = await handler(request, recordAnalytics); + const response = await runHandler(handler, request, recordAnalytics); return addCorsHeaders(response); }; } @@ -54,7 +69,7 @@ export function createAnalyticsWrapper(cache: SlackCache) { return async (request: Request): Promise => { if (request.method === "OPTIONS") return corsPreflightResponse(); const noop: AnalyticsRecorder = () => {}; - const response = await handler(request, noop); + const response = await runHandler(handler, request, noop); return addCorsHeaders(response); }; } @@ -77,7 +92,7 @@ export function createAnalyticsWrapper(cache: SlackCache) { ); }; - const response = await handler(request, recordAnalytics); + const response = await runHandler(handler, request, recordAnalytics); return addCorsHeaders(response); }; }; diff --git a/src/lib/http-errors.ts b/src/lib/http-errors.ts new file mode 100644 index 0000000..9a610a6 --- /dev/null +++ b/src/lib/http-errors.ts @@ -0,0 +1,112 @@ +import { fastPathname } from "./fast-url"; + +export interface ApiErrorBody { + error: { + code: string; + message: string; + hint: string; + }; +} + +export function jsonError( + status: number, + code: string, + message: string, + hint: string, + init: { headers?: Record; extra?: object } = {}, +): Response { + return Response.json( + { + ...init.extra, + error: { code, message, hint }, + } satisfies ApiErrorBody, + { status, headers: init.headers }, + ); +} + +export function notFoundResponse(request: Request): Response { + const pathname = fastPathname(request.url); + return jsonError( + 404, + "ROUTE_NOT_FOUND", + `No endpoint exists at ${pathname}.`, + "See /swagger.json for the machine-readable API specification.", + ); +} + +export function methodNotAllowedResponse( + request: Request, + allowedMethods: string[], +): Response { + return jsonError( + 405, + "METHOD_NOT_ALLOWED", + `${request.method} is not supported for ${fastPathname(request.url)}.`, + `Retry with one of the supported methods: ${allowedMethods.join(", ")}.`, + { headers: { Allow: allowedMethods.join(", ") } }, + ); +} + +export function internalErrorResponse(): Response { + return jsonError( + 500, + "INTERNAL_ERROR", + "The server could not complete the request.", + "Retry later. If the problem persists, report the endpoint and request time to the service operator.", + ); +} + +function routeMatches(pattern: string, pathname: string): boolean { + const patternSegments = pattern.split("/"); + const pathSegments = pathname.split("/"); + return ( + patternSegments.length === pathSegments.length && + patternSegments.every( + (segment, index) => + segment.startsWith(":") || segment === pathSegments[index], + ) + ); +} + +const HTTP_METHODS = new Set([ + "GET", + "POST", + "PUT", + "DELETE", + "PATCH", + "HEAD", + "OPTIONS", +]); + +function methodsForRoute(route: unknown): string[] { + if (typeof route !== "object" || route === null) return ["GET"]; + + const methods = Object.keys(route).filter((key) => HTTP_METHODS.has(key)); + return methods.length > 0 ? methods : ["GET"]; +} + +export function createFallbackHandler( + routes: Record, +): (request: Request) => Response { + const routeMethods = Object.fromEntries( + Object.entries(routes).map(([path, route]) => [ + path, + methodsForRoute(route), + ]), + ); + const dynamicRoutes = Object.entries(routeMethods).filter(([pattern]) => + pattern.includes(":"), + ); + + return (request: Request) => { + const pathname = fastPathname(request.url); + const allowedMethods = + routeMethods[pathname] ?? + dynamicRoutes.find(([pattern]) => routeMatches(pattern, pathname))?.[1]; + + if (allowedMethods) { + return methodNotAllowedResponse(request, allowedMethods); + } + return notFoundResponse(request); + }; +} diff --git a/src/lib/swagger-generator.ts b/src/lib/swagger-generator.ts index a310fb1..1fd8710 100644 --- a/src/lib/swagger-generator.ts +++ b/src/lib/swagger-generator.ts @@ -33,6 +33,7 @@ interface SwaggerSpec { paths: Record>; components?: { securitySchemes?: Record; + schemas?: Record>; }; } @@ -64,6 +65,29 @@ export class SwaggerGenerator { scheme: "bearer", }, }, + schemas: { + ErrorResponse: { + type: "object", + required: ["error"], + properties: { + error: { + type: "object", + required: ["code", "message", "hint"], + properties: { + code: { type: "string", example: "ROUTE_NOT_FOUND" }, + message: { + type: "string", + example: "No endpoint exists at /example.", + }, + hint: { + type: "string", + example: "See /swagger.json for available endpoints.", + }, + }, + }, + }, + }, + }, }, }; } @@ -176,18 +200,31 @@ export class SwaggerGenerator { // Add responses Object.entries(metadata.responses).forEach(([status, response]) => { + const isError = Number(status) >= 400; + const schema = + response.schema ?? + (isError ? { $ref: "#/components/schemas/ErrorResponse" } : undefined); (spec.responses as Record)[status] = { description: response.description, - ...(response.schema && { + ...(schema && { content: { - "application/json": { - schema: response.schema, - }, + "application/json": { schema }, }, }), }; }); + if (!("500" in (spec.responses as Record))) { + (spec.responses as Record)["500"] = { + description: "Unexpected server error", + content: { + "application/json": { + schema: { $ref: "#/components/schemas/ErrorResponse" }, + }, + }, + }; + } + // Add security if required if (metadata.requiresAuth) { spec.security = [{ bearerAuth: [] }]; -- 2.51.2