From b82a5e1dc0069cdcbb0771383230619da8e1fc6e Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Wed, 5 Aug 2026 14:00:31 -0400 Subject: [PATCH] add some basic validation --- apps/accounts/src/accounts.app.ts | 81 +++++++++---------- apps/accounts/src/openapi.ts | 75 +++++++++++++---- .../accounts/src/test/integration/api.test.ts | 40 +++++++-- packages/hono-helpers/src/helpers/openapi.ts | 38 ++++++++- 4 files changed, 168 insertions(+), 66 deletions(-) diff --git a/apps/accounts/src/accounts.app.ts b/apps/accounts/src/accounts.app.ts index f1e364e..9c2afd7 100644 --- a/apps/accounts/src/accounts.app.ts +++ b/apps/accounts/src/accounts.app.ts @@ -1,5 +1,5 @@ import { Hono } from 'hono' -import { describeRoute, openAPIRouteHandler } from 'hono-openapi' +import { describeRoute, openAPIRouteHandler, validator } from 'hono-openapi' import { useWorkersLogger } from 'workers-tagged-logger' import { @@ -8,11 +8,6 @@ import { getAccount, getAccountByUsername, getAccountsByIds, - isValidBio, - isValidEmail, - MAX_DISPLAY_NAME_LENGTH, - MAX_USERNAME_LENGTH, - nameRejection, searchAccounts, updateAccount, } from '@repo/domain' @@ -74,12 +69,18 @@ function unauthorized(c: Context) { const DEFAULT_USERNAME_CHANGES = 1 /** - * Username-change result envelope: `{ success, error, value }`, always HTTP 200. - * On success `value` is the updated account; on error `error` carries the message - * and `value` is an empty string. + * Username-change result envelope: `{ success, error, value }`. On success `value` is + * the updated account; on a refusal `error` carries the message and `value` is an empty + * string. + * + * A refusal is a 400. The body shape is unchanged — anything reading `error` still + * works — but it used to come back at HTTP 200, which meant a caller keying off the + * status read every refusal as a success. That envelope-at-200 was the reference's + * (`RecNet`) convention and is kept by `POST /account/create`; here it was traded for a + * status a client can actually branch on. */ function usernameResult(c: Context, error = '', value: unknown = '') { - return c.json({ success: error === '', error, value }) + return c.json({ success: error === '', error, value }, error === '' ? 200 : 400) } /** Read a single string field from a form-urlencoded / multipart body. */ @@ -424,26 +425,20 @@ const app = new Hono() summary: 'Set display name', description: 'Persisted and broadcast via an AccountUpdate notification.', security: AUTHED, - requestBody: form(DisplayNameRequest, 'The new display name'), responses: { 200: json(SuccessResponse, 'Updated'), 400: { description: 'Empty, over 15 characters, or non-alphanumeric (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), + // An EMPTY 400, which is what this route already answered for an empty name: it + // acks with a bare SuccessResponse and has never sent the client a body on + // failure, so enforcing the schema doesn't change what a refusal looks like. + validator('form', DisplayNameRequest, (r, c) => (r.success ? undefined : c.body(null, 400))), async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const displayName = (await formField(c, 'displayName')).trim() - if (displayName === '') return c.body(null, 400) - // Held to the same shape as the username — it's the name other players actually - // see, so it can't be the place where spaces and punctuation get in. Refused with - // an empty 400 like the empty case above: this route answers a bare - // SuccessResponse, and giving it a body the client has never been sent is a - // bigger change than the rule is worth. - if (nameRejection(displayName, 'display name', MAX_DISPLAY_NAME_LENGTH) !== null) { - return c.body(null, 400) - } + const { displayName } = c.req.valid('form') const account = await updateAccount(c.env.DB, id, { displayName }) await pushAccountUpdate(c, account) return c.json({ success: true }) @@ -465,24 +460,28 @@ const app = new Hono() '(see the UsernameResult envelope).', ].join(' '), security: AUTHED, - requestBody: form(UsernameRequest, 'The desired username'), responses: { - 200: json(UsernameResult, 'Result envelope (success or a validation error)'), + 200: json(UsernameResult, 'The updated account, in the result envelope'), + 400: json(UsernameResult, 'Refused — `error` carries the reason, `value` is ""'), 401: UNAUTHORIZED_RESPONSE, }, }), + // Shape is checked before the handler runs, so a rejected name costs no D1 read and + // — the part that matters — can never spend one of the account's rationed changes. + // The message is relayed rather than zod's issue array: `nameRejection` writes the + // sentence the player reads, and nothing can render an array of issues. + // `c` is annotated so the hook's context matches this app's bindings, and `error` is + // Standard Schema's flat issue list rather than a zod error object. + validator('form', UsernameRequest, (r, c: Context) => + r.success + ? undefined + : usernameResult(c, r.error[0]?.message ?? 'That username cannot be used.') + ), async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const username = (await formField(c, 'username')).trim() - if (username === '') return usernameResult(c, 'You must enter a username.') - - // Shape before availability: a rejected name shouldn't cost a D1 read, and it - // must never cost the account one of its rationed changes. The envelope carries - // the sentence straight to the player. - const rejection = nameRejection(username, 'username', MAX_USERNAME_LENGTH) - if (rejection !== null) return usernameResult(c, rejection) + const { username } = c.req.valid('form') // Duplicate check first (case-insensitive); keeping your own name is allowed. const existing = await getAccountByUsername(c.env.DB, username) @@ -514,18 +513,17 @@ const app = new Hono() summary: 'Set email', description: 'Persisted; surfaced only by `/account/me`. Not broadcast.', security: AUTHED, - requestBody: form(EmailRequest, 'The new email'), responses: { 200: json(SuccessResponse, 'Updated'), - 400: { description: 'Not a valid address, or over 255 characters (empty body)' }, + 400: { description: 'Not a syntactically valid address (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), + validator('form', EmailRequest, (r, c) => (r.success ? undefined : c.body(null, 400))), async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const email = (await formField(c, 'email')).trim() - if (!isValidEmail(email)) return c.body(null, 400) + const { email } = c.req.valid('form') await updateAccount(c.env.DB, id, { email }) return c.json({ success: true }) } @@ -539,18 +537,17 @@ const app = new Hono() summary: 'Set phone number', description: 'Persisted on the account row. Not broadcast.', security: AUTHED, - requestBody: form(PhoneRequest, 'The new phone number'), responses: { 200: json(SuccessResponse, 'Updated'), 400: { description: 'Empty phone (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), + validator('form', PhoneRequest, (r, c) => (r.success ? undefined : c.body(null, 400))), async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const phone = (await formField(c, 'phone')).trim() - if (phone === '') return c.body(null, 400) + const { phone } = c.req.valid('form') await updateAccount(c.env.DB, id, { phone }) return c.json({ success: true }) } @@ -627,20 +624,18 @@ const app = new Hono() summary: 'Set bio', description: 'Free text up to 255 characters; empty is allowed. Persisted and broadcast.', security: AUTHED, - requestBody: form(BioRequest, 'The new bio'), responses: { 200: json(SuccessResponse, 'Updated'), 400: { description: 'Bio over 255 characters (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), + // Refused rather than truncated: silently storing half a sentence reads as data loss. + validator('form', BioRequest, (r, c) => (r.success ? undefined : c.body(null, 400))), async (c) => { const id = await authedId(c) if (id === null) return unauthorized(c) - const bio = await formField(c, 'bio') - // Length only — a bio is free text by design (see the route summary). Refused - // rather than truncated: silently storing half a sentence reads as data loss. - if (!isValidBio(bio)) return c.body(null, 400) + const { bio } = c.req.valid('form') const account = await updateAccount(c.env.DB, id, { bio }) await pushAccountUpdate(c, account) return c.json({ success: true }) diff --git a/apps/accounts/src/openapi.ts b/apps/accounts/src/openapi.ts index 4f6bbb3..a5f501f 100644 --- a/apps/accounts/src/openapi.ts +++ b/apps/accounts/src/openapi.ts @@ -1,20 +1,35 @@ import { resolver } from 'hono-openapi' import { z } from 'zod' +import { + isValidBio, + isValidEmail, + MAX_DISPLAY_NAME_LENGTH, + MAX_USERNAME_LENGTH, + nameRejection, +} from '@repo/domain' + import type { OpenAPIV3_1 } from 'openapi-types' /** * OpenAPI schemas for the accounts worker. * - * IMPORTANT: these are DESCRIPTIVE ONLY. They are passed to `describeRoute` to - * generate the spec and are never wired into `hono-openapi`'s `validator()`. + * Most of these are DESCRIPTIVE ONLY: they are passed to `describeRoute` to generate the + * spec, and the handler stays lenient. That is deliberate — the Rec Room client is the + * real consumer, form fields are read as `typeof value === 'string' ? value : ''`, and + * missing or malformed input falls through to a graceful path (or a synthesized default + * account) rather than a hard error. A schema that rejected what the client actually + * sends would break the game, not protect it. * - * As with the auth worker, this is deliberate. The Rec Room client is the only real - * consumer and the handlers are intentionally lenient — form fields are read as - * `typeof value === 'string' ? value : ''` and missing/malformed input falls through - * to a graceful path (or a synthesized default account) rather than a hard error. - * These schemas record what the client is observed to send and what we send back; to - * enforce one, do it per-route and land a test with it. + * The EXCEPTION is the profile mutations a player types into a box — displayName, + * username, email, phone, bio. Those carry real rules (see `@repo/domain`), and each is + * wired into `hono-openapi`'s `validator()` per route, with tests, exactly as the older + * version of this note prescribed. Wiring one up means the schema both validates the + * request and generates the spec, so a limit can't be changed in one and not the other — + * which is precisely how the documented email limit came to disagree with the real one. + * + * A validated route drops `requestBody: form(...)` from its `describeRoute`: the + * validator registers the body itself, and declaring it twice would emit it twice. */ /** Emit a zod schema as an `application/json` response body. */ @@ -122,27 +137,54 @@ export const CreateAccountRequest = z.object({ platformId: z.string().optional().describe('Parsed for fidelity; currently unused'), }) -/** Single-string form bodies, one per profile mutation. */ +/** + * Single-string form bodies, one per profile mutation. + * + * These are ENFORCED, not just described: each is handed to hono-openapi's `validator`, + * so the same schema both validates the request and generates the spec. Before this they + * were documentation only, and the real rule lived in the handler — which meant every + * limit had to be edited in two places and nothing caught them disagreeing. + * + * The rules themselves come from `@repo/domain` so `rooms` and `clubs` can't drift from + * `accounts`; `superRefine` is used where the message matters, because `nameRejection` + * writes the player-facing sentence and there's no reason to write it twice. + */ + +/** Zod check that defers to the shared name rule, message and all. */ +const nameCheck = (label: string, max: number) => + z.string() + .trim() + .superRefine((value, ctx) => { + const rejection = nameRejection(value, label, max) + if (rejection !== null) ctx.addIssue({ code: 'custom', message: rejection }) + }) + export const DisplayNameRequest = z.object({ - displayName: z - .string() + displayName: nameCheck('display name', MAX_DISPLAY_NAME_LENGTH) + .min(1) .describe('Trimmed; letters and digits only, max 15. Empty or invalid is rejected (400)'), }) export const UsernameRequest = z.object({ - username: z - .string() + username: nameCheck('username', MAX_USERNAME_LENGTH) + .min(1, 'You must enter a username.') .describe( 'Trimmed; letters and digits only, max 50. Must be unique and changes must remain' ), }) export const EmailRequest = z.object({ - email: z.string().describe('A deliverable-looking address, max 255; otherwise 400'), + email: z + .string() + .trim() + .refine(isValidEmail, 'That email address looks wrong.') + .describe('A syntactically valid address (RFC 5321/5322, so at most 254); otherwise 400'), }) export const PhoneRequest = z.object({ - phone: z.string().describe('Trimmed; empty is rejected (400)'), + // No shape rule on purpose: the client sends E.164 (`+15552223333`), which the name + // rule above would reject outright by eating the leading `+`. + phone: z.string().trim().min(1).describe('Trimmed; empty is rejected (400)'), }) export const IdentityFlagsRequest = z.object({ @@ -154,7 +196,8 @@ export const PronounsRequest = z.object({ }) export const BioRequest = z.object({ - bio: z.string().describe('Free text, max 255; empty is allowed'), + // Not trimmed — a bio is free text, and leading whitespace is the player's business. + bio: z.string().refine(isValidBio).describe('Free text, max 255; empty is allowed'), }) export const ProfileImageRequest = z.object({ diff --git a/apps/accounts/src/test/integration/api.test.ts b/apps/accounts/src/test/integration/api.test.ts index 6185312..9eba46d 100644 --- a/apps/accounts/src/test/integration/api.test.ts +++ b/apps/accounts/src/test/integration/api.test.ts @@ -220,8 +220,9 @@ describe('auth-gated endpoints', () => { ...form({ username: 'Coach' }), headers: { ...(await bearer('893')), 'Content-Type': 'application/x-www-form-urlencoded' }, }) - // Business errors are HTTP 200 with the { success, error, value } envelope. - expect(res.status).toBe(200) + // A refusal is a 400 carrying the same { success, error, value } envelope. It used + // to be HTTP 200, which read as a success to anything branching on the status. + expect(res.status).toBe(400) const body = (await res.json()) as { success: boolean; error: string; value: string } expect(body.success).toBe(false) expect(body.error).toMatch(/already taken/i) @@ -260,7 +261,7 @@ describe('auth-gated endpoints', () => { ...form({ username: 'coachy' }), headers, }) - expect(blocked.status).toBe(200) + expect(blocked.status).toBe(400) const blockedBody = (await blocked.json()) as { success: boolean; error: string } expect(blockedBody.success).toBe(false) expect(blockedBody.error).toMatch(/no username changes/i) @@ -459,6 +460,31 @@ describe('auth-gated endpoints', () => { for (const op of Object.values(ops)) expect(op.summary).toBeTruthy() } }) + + // hono-openapi registers a validated form body under `multipart/form-data` only, and + // its `media` option can't say otherwise (a precedence bug — see `withCleanSpec`). The + // real callers post `application/x-www-form-urlencoded`, so a spec that named only + // multipart would tell an integrator to send the one thing nothing here sends. + test('GET /openapi.json documents both form content types on validated routes', async () => { + const res = await exports.default.fetch(`${ORIGIN}/openapi.json`) + const spec = (await res.json()) as { + paths: Record } }>> + } + + for (const [path, method] of [ + ['/account/me/email', 'post'], + ['/account/me/username', 'put'], + ['/account/me/displayname', 'put'], + ['/account/me/bio', 'put'], + ['/account/me/phone', 'post'], + ] as const) { + const content = spec.paths[path]?.[method]?.requestBody?.content ?? {} + expect(Object.keys(content).sort(), path).toEqual([ + 'application/x-www-form-urlencoded', + 'multipart/form-data', + ]) + } + }) }) // The names a player chooses are alphanumeric and length-capped, by the same rule the @@ -482,11 +508,13 @@ describe('name, email and bio validation', () => { ...form({ username }), headers, }) - // Still the envelope at HTTP 200, like every other refusal on this route. - expect(res.status).toBe(200) - const body = (await res.json()) as { success: boolean; error: string } + // Refused by the SCHEMA (see openapi.ts `UsernameRequest`) before the handler + // runs — but still in this route's envelope, because the hook puts it there. + expect(res.status, username).toBe(400) + const body = (await res.json()) as { success: boolean; error: string; value: string } expect(body.success, username).toBe(false) expect(body.error).toMatch(/letters and numbers|at most 50 characters/) + expect(body.value).toBe('') } // The rationed change must NOT be spent by a refusal: an account starts with one, diff --git a/packages/hono-helpers/src/helpers/openapi.ts b/packages/hono-helpers/src/helpers/openapi.ts index ee92c43..9fe4ade 100644 --- a/packages/hono-helpers/src/helpers/openapi.ts +++ b/packages/hono-helpers/src/helpers/openapi.ts @@ -33,9 +33,44 @@ function addIntegerExamples(node: unknown): void { for (const value of Object.values(obj)) addIntegerExamples(value) } +/** + * hono-openapi registers a `validator('form', …)` body under `multipart/form-data` ONLY, + * and there is no way to ask it for another media type: its media selection reads + * `options?.media ?? target === 'json' ? 'application/json' : 'multipart/form-data'`, + * which by JS precedence is `((options?.media ?? target) === 'json') ? … : …` — so the + * `media` option can never produce anything else. + * + * That leaves the spec claiming a validated route accepts only multipart, when the real + * callers (the Rec Room client and the website) post `application/x-www-form-urlencoded` + * and Hono's `parseBody()` reads both. So the urlencoded variant is mirrored back in. + * + * Safe because nothing here documents a genuinely multipart-only body — there are no file + * uploads on these workers, and hand-written form bodies already declare both types. If + * one is ever added, it will need to opt out of this. + */ +function mirrorFormBodies(node: unknown): void { + if (Array.isArray(node)) { + for (const item of node) mirrorFormBodies(item) + return + } + if (node === null || typeof node !== 'object') return + + const obj = node as Record + const content = obj.content + if (content !== null && typeof content === 'object') { + const media = content as Record + const multipart = media['multipart/form-data'] + if (multipart !== undefined && media['application/x-www-form-urlencoded'] === undefined) { + media['application/x-www-form-urlencoded'] = multipart + } + } + for (const value of Object.values(obj)) mirrorFormBodies(value) +} + /** * Wrap `openAPIRouteHandler(...)` so the generated document gets example values for its - * integer fields. Purely cosmetic — nothing about the documented shapes changes. + * integer fields, and so a validated form body documents both content types it really + * accepts. Nothing about the runtime behaviour changes — this only corrects the document. * * ```ts * app.get('/openapi.json', describeRoute({ hide: true }), withCleanSpec(openAPIRouteHandler(app, { ... }))) @@ -47,6 +82,7 @@ export function withCleanSpec(handler: Handler | MiddlewareHandler): Handler { if (!(res instanceof Response)) return res as never const spec: unknown = await res.json() addIntegerExamples(spec) + mirrorFormBodies(spec) return c.json(spec as Record) } }