From 7b26d57c4d543ebe5b67c12d8700222a3e82c09b Mon Sep 17 00:00:00 2001 From: fireshaper Date: Fri, 2 Oct 2026 21:47:00 -0400 Subject: [PATCH] Step 5.6: validate requests and responses from the route catalog Co-Authored-By: Claude Opus 5.5 --- DECISIONS.md | 8 +++ apps/server/src/app.ts | 19 ++++--- apps/server/src/http/errors.ts | 5 ++ apps/server/src/http/route.ts | 72 +++++++++++++++++++++++++ apps/server/test/validation.test.ts | 84 +++++++++++++++++++++++++++++ memory-bank/architecture.md | 11 ++++ memory-bank/progress.md | 2 +- 7 files changed, 193 insertions(+), 8 deletions(-) create mode 100644 apps/server/src/http/route.ts create mode 100644 apps/server/test/validation.test.ts diff --git a/DECISIONS.md b/DECISIONS.md index a3d4688..f179e7d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -244,3 +244,11 @@ Implementation decisions not spelled out in `memory-bank/implementation-plan.md` - **Errors:** an `ApiError(status, code, message)` thrown anywhere becomes `{ error: { code, message } }`. Unknown routes return `404 not-found`, and unexpected exceptions are logged and return `500 internal-error`. - **`GET /api/state` is implemented now without lazy resolution.** It reads ships, pilots (level and skill points derived with sim), bays, all locations with their discovered flag, tutorial state, in-flight dispatches and unacknowledged summaries. Step 6.1 adds resolution, and Step 6.2 its fuller tests. - **`buildApp` takes an optional `database`.** Tests pass the harness's connection; otherwise the app opens its own and closes it on shutdown. + +## Step 5.6 + +- **The catalog is wired in with a small `registerRoute` helper**, not `fastify-type-provider-zod`. The catalog merges path parameters with the body into one request schema, which the type provider's separate params/body/query slots do not model. +- **Request validation runs before the session check,** so a malformed request always gets `400 validation-error`, even without a session. +- **Requests with no fields reject unexpected ones** (`emptyRequestSchema` is strict), including query parameters. +- **Responses are validated everywhere except production.** A violation is logged and returned as `500 internal-error`, which makes tests fail loudly. Production skips the check to save work. +- **Fastify's own 4xx errors** (malformed JSON, wrong content type) are reported as `400 validation-error` in the documented format. diff --git a/apps/server/src/app.ts b/apps/server/src/app.ts index 357cb1f..f1bdf7a 100644 --- a/apps/server/src/app.ts +++ b/apps/server/src/app.ts @@ -1,6 +1,6 @@ import { randomInt } from "node:crypto"; import { content } from "@deadhead/content"; -import { routes, type PlayerState, type ServerTime } from "@deadhead/schema"; +import { routes } from "@deadhead/schema"; import { realToGame } from "@deadhead/sim"; import { fromNodeHeaders } from "better-auth/node"; import Fastify, { type FastifyInstance } from "fastify"; @@ -11,6 +11,7 @@ import { createDatabase, type Database } from "./db/client"; import { createPlayer } from "./game/new-player"; import { loadPlayerState } from "./game/player-state"; import { ApiError, registerErrorHandler } from "./http/errors"; +import { registerRoute, type RouteOptions } from "./http/route"; import { requirePlayerId } from "./http/session"; export interface AppDeps { @@ -33,15 +34,20 @@ export function buildApp(config: ServerConfig, deps: AppDeps = {}): FastifyInsta if (!deps.database) app.addHook("onClose", () => database.close()); registerErrorHandler(app); + const options: RouteOptions = { + requirePlayer: (request) => requirePlayerId(request, auth, db), + validateResponses: config.env !== "production", + }; + // Under /api so it is reachable through the same-domain proxy (decision 0.5.20). app.get("/api/health", async () => ({ status: "ok" })); - app.get(routes.getServerTime.path, async (): Promise => { + registerRoute(app, routes.getServerTime, options, async () => { const realTime = clock(); return { realTime, gameTime: realToGame(realTime, content.config) }; }); - app.post(routes.createGuest.path, async (request, reply) => { + registerRoute(app, routes.createGuest, options, async (_input, { request, reply }) => { const signIn = await auth.api.signInAnonymous({ headers: fromNodeHeaders(request.headers), returnHeaders: true, @@ -59,10 +65,9 @@ export function buildApp(config: ServerConfig, deps: AppDeps = {}): FastifyInsta return { playerId, state: await loadPlayerState(db, playerId) }; }); - app.get(routes.getPlayerState.path, async (request): Promise => { - const playerId = await requirePlayerId(request, auth, db); - return loadPlayerState(db, playerId); - }); + registerRoute(app, routes.getPlayerState, options, async (_input, { playerId }) => + loadPlayerState(db, playerId), + ); return app; } diff --git a/apps/server/src/http/errors.ts b/apps/server/src/http/errors.ts index 7fc7c33..a0c8dcf 100644 --- a/apps/server/src/http/errors.ts +++ b/apps/server/src/http/errors.ts @@ -22,6 +22,11 @@ export function registerErrorHandler(app: FastifyInstance): void { if (error instanceof ApiError) { return reply.status(error.status).send(errorBody(error.code, error.message)); } + // Fastify's own 4xx errors, e.g. a body that is not valid JSON. + const status = (error as { statusCode?: number }).statusCode; + if (status !== undefined && status >= 400 && status < 500) { + return reply.status(400).send(errorBody("validation-error", (error as Error).message)); + } request.log.error(error); return reply.status(500).send(errorBody("internal-error", "Something went wrong.")); }); diff --git a/apps/server/src/http/route.ts b/apps/server/src/http/route.ts new file mode 100644 index 0000000..097d9b1 --- /dev/null +++ b/apps/server/src/http/route.ts @@ -0,0 +1,72 @@ +import type { RouteDef } from "@deadhead/schema"; +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; +import type { z } from "zod"; +import { ApiError } from "./errors"; + +export interface RouteContext { + /** The signed-in player; only present on routes that require a session. */ + playerId: Auth extends true ? string : null; + request: FastifyRequest; + reply: FastifyReply; +} + +export interface RouteOptions { + /** Resolves the session to a player id, or throws `401 unauthorized`. */ + requirePlayer: (request: FastifyRequest) => Promise; + /** Check responses against their schema (everywhere except production). */ + validateResponses: boolean; +} + +function describeIssues(error: z.ZodError): string { + return error.issues + .map((issue) => (issue.path.length ? `${issue.path.join(".")}: ` : "") + issue.message) + .join("; "); +} + +/** + * Registers one catalog route. The request (path parameters, query and body + * merged) is validated before anything else, then the session is checked if the + * route needs one, and the response is checked against its schema. + */ +export function registerRoute( + app: FastifyInstance, + route: R, + options: RouteOptions, + handler: ( + input: z.output, + context: RouteContext, + ) => Promise>, +): void { + app.route({ + method: route.method, + url: route.path, + handler: async (request, reply) => { + const raw = { + ...(request.query as object), + ...(typeof request.body === "object" && request.body !== null ? request.body : {}), + ...(request.params as object), + }; + const parsed = route.request.safeParse(raw); + if (!parsed.success) { + throw new ApiError(400, "validation-error", describeIssues(parsed.error)); + } + + const playerId = route.auth ? await options.requirePlayer(request) : null; + const result = await handler(parsed.data as z.output, { + playerId: playerId as RouteContext["playerId"], + request, + reply, + }); + + if (options.validateResponses) { + const checked = route.response.safeParse(result); + if (!checked.success) { + throw new Error( + `Response for ${route.method} ${route.path} violates its schema: ${describeIssues(checked.error)}`, + ); + } + } + return result; + }, + }); +} diff --git a/apps/server/test/validation.test.ts b/apps/server/test/validation.test.ts new file mode 100644 index 0000000..f3a11b8 --- /dev/null +++ b/apps/server/test/validation.test.ts @@ -0,0 +1,84 @@ +import { errorResponseSchema, routes } from "@deadhead/schema"; +import Fastify from "fastify"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { buildApp } from "../src/app"; +import { registerErrorHandler } from "../src/http/errors"; +import { registerRoute } from "../src/http/route"; +import { useTestDatabase } from "./db"; +import { testConfig } from "./helpers"; + +const database = useTestDatabase(); + +describe("request validation from the route catalog", () => { + let app: ReturnType; + beforeEach(() => { + app = buildApp(testConfig(), { database: database() }); + }); + afterEach(() => app.close()); + + const expectValidationError = (response: { statusCode: number; json(): unknown }) => { + expect(response.statusCode).toBe(400); + expect(errorResponseSchema.parse(response.json()).error.code).toBe("validation-error"); + }; + + it("rejects unexpected fields on a route that takes none", async () => { + expectValidationError( + await app.inject({ method: "POST", url: "/api/guest", payload: { surprise: true } }), + ); + }); + + it("rejects a body that is not valid JSON", async () => { + expectValidationError( + await app.inject({ + method: "POST", + url: "/api/guest", + headers: { "content-type": "application/json" }, + payload: "{not json", + }), + ); + }); + + it("rejects unexpected query parameters on an authenticated route before checking the session", async () => { + expectValidationError(await app.inject({ method: "GET", url: "/api/state?evil=1" })); + }); + + it("still answers valid requests", async () => { + expect((await app.inject({ method: "GET", url: "/api/time" })).statusCode).toBe(200); + }); + + it("returns not-found in the error format for unknown endpoints", async () => { + const response = await app.inject({ method: "GET", url: "/api/nope" }); + expect(response.statusCode).toBe(404); + expect(errorResponseSchema.parse(response.json()).error.code).toBe("not-found"); + }); +}); + +describe("response validation", () => { + const build = (validateResponses: boolean) => { + const app = Fastify({ logger: false }); + registerErrorHandler(app); + // A handler that breaks the server-time response schema. + registerRoute( + app, + routes.getServerTime, + { requirePlayer: async () => "unused", validateResponses }, + async () => ({ realTime: "not a number" }) as never, + ); + return app; + }; + + it("fails a response that violates its schema in test mode", async () => { + const app = build(true); + const response = await app.inject({ method: "GET", url: "/api/time" }); + expect(response.statusCode).toBe(500); + expect(errorResponseSchema.parse(response.json()).error.code).toBe("internal-error"); + await app.close(); + }); + + it("does not check responses when validation is off (production)", async () => { + const app = build(false); + const response = await app.inject({ method: "GET", url: "/api/time" }); + expect(response.statusCode).toBe(200); + await app.close(); + }); +}); diff --git a/memory-bank/architecture.md b/memory-bank/architecture.md index 7b447d0..1dd5774 100644 --- a/memory-bank/architecture.md +++ b/memory-bank/architecture.md @@ -365,3 +365,14 @@ export const accountRelations = relations(account, ({ one }) => ({ ## Auth and sessions Better Auth (`apps/server/src/auth.ts`) with the anonymous plugin, mounted only through its server API (no `/api/auth/*` HTTP handler yet). `POST /api/guest` calls `signInAnonymous`, forwards its session cookie, and creates the player (`createPlayer`) linked to the new user. Authenticated routes call `requirePlayerId`, which resolves the cookie to a session, then to `players.user_id`; failures are `401 unauthorized`. + +## Request pipeline (`apps/server/src/http`) + +Every catalog endpoint is registered with `registerRoute(app, routes., options, handler)`: + +1. Merge path parameters, query and body, and validate them against the route's `request` schema → `400 validation-error` on failure (also for malformed JSON). +2. If `route.auth`, resolve the session to a player id → `401 unauthorized` on failure. +3. Run the handler with the typed input and `{ playerId, request, reply }`. +4. Outside production, validate the result against the route's `response` schema; a mismatch is logged and returned as `500 internal-error`. + +Handlers throw `ApiError(status, code, message)` for expected failures. Unknown paths return `404 not-found`. diff --git a/memory-bank/progress.md b/memory-bank/progress.md index 9397d62..534936a 100644 --- a/memory-bank/progress.md +++ b/memory-bank/progress.md @@ -50,4 +50,4 @@ - [x] **Step 5.3 — Database schema and migrations.** Drizzle schema (`apps/server/src/db/schema.ts`, copied into `architecture.md`), first migration `drizzle/0000_init.sql`, `db:generate` / `db:migrate` scripts. Migration test runs against an emptied test database twice. CI now has a Postgres 18 service; server tests run one file at a time. - [x] **Step 5.4 — Test database harness.** `apps/server/test/db.ts`: `useTestDatabase()` (migrate once, truncate all tables before each test) and `seedPlayer(db, "tutorial" | "owner")`. New-player creation lives in `src/game/new-player.ts` (`createPlayer`), shared with guest sign-up. - [x] **Step 5.5 — Guest accounts.** Better Auth with the anonymous plugin (`src/auth.ts`, tables in `src/db/auth-schema.ts`, migration `0001_auth`). `POST /api/guest` creates a guest session and a tutorial player; `GET /api/state` returns player state for the session or `401 unauthorized`. Error handler sends the documented error format. -- [ ] Step 5.6 — Request validation from the route catalog +- [x] **Step 5.6 — Request validation from the route catalog.** `registerRoute` (`src/http/route.ts`) validates merged params/query/body before auth, checks sessions for `auth` routes, and checks responses outside production. Malformed JSON and unexpected fields return `400 validation-error`.