From 6bfd4d9e50bd2ec47198de8f74d857f8249a53a9 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Wed, 5 Aug 2026 12:08:04 -0400 Subject: [PATCH] validation in some areas, maybe move this to schema later --- apps/accounts/src/accounts.app.ts | 38 ++++- apps/accounts/src/openapi.ts | 16 +- .../accounts/src/test/integration/api.test.ts | 147 ++++++++++++++++++ apps/api/src/events-db.ts | 30 ++++ apps/api/src/routes/events.ts | 15 +- apps/api/src/test/integration/api.test.ts | 38 +++++ apps/clubs/src/clubs.app.ts | 27 +++- apps/clubs/src/openapi.ts | 13 +- apps/clubs/src/test/integration/api.test.ts | 13 +- apps/rooms/src/rooms.app.ts | 17 ++ apps/rooms/src/test/integration/api.test.ts | 86 +++++++++- apps/www/src/client/App.tsx | 34 ++-- packages/domain/package.json | 3 + packages/domain/src/index.ts | 1 + packages/domain/src/validation.ts | 96 ++++++++++++ pnpm-lock.yaml | 18 +++ 16 files changed, 553 insertions(+), 39 deletions(-) create mode 100644 packages/domain/src/validation.ts diff --git a/apps/accounts/src/accounts.app.ts b/apps/accounts/src/accounts.app.ts index 53291b1..f1e364e 100644 --- a/apps/accounts/src/accounts.app.ts +++ b/apps/accounts/src/accounts.app.ts @@ -8,6 +8,11 @@ import { getAccount, getAccountByUsername, getAccountsByIds, + isValidBio, + isValidEmail, + MAX_DISPLAY_NAME_LENGTH, + MAX_USERNAME_LENGTH, + nameRejection, searchAccounts, updateAccount, } from '@repo/domain' @@ -422,7 +427,7 @@ const app = new Hono() requestBody: form(DisplayNameRequest, 'The new display name'), responses: { 200: json(SuccessResponse, 'Updated'), - 400: { description: 'Empty display name (empty body)' }, + 400: { description: 'Empty, over 15 characters, or non-alphanumeric (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), @@ -431,6 +436,14 @@ const app = new Hono() 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 account = await updateAccount(c.env.DB, id, { displayName }) await pushAccountUpdate(c, account) return c.json({ success: true }) @@ -446,9 +459,10 @@ const app = new Hono() tags: ['Profile'], summary: 'Change username', description: [ - 'Rejects a name taken by another account and requires a remaining change; on', - 'success the name is persisted and the counter decremented. Always HTTP 200 —', - 'failures carry a message in `error` (see the UsernameResult envelope).', + 'Letters and digits only, at most 50 characters. Rejects a name taken by another', + 'account and requires a remaining change; on success the name is persisted and', + 'the counter decremented. Always HTTP 200 — failures carry a message in `error`', + '(see the UsernameResult envelope).', ].join(' '), security: AUTHED, requestBody: form(UsernameRequest, 'The desired username'), @@ -464,6 +478,12 @@ const app = new Hono() 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) + // Duplicate check first (case-insensitive); keeping your own name is allowed. const existing = await getAccountByUsername(c.env.DB, username) if (existing && existing.accountId !== id) { @@ -497,7 +517,7 @@ const app = new Hono() requestBody: form(EmailRequest, 'The new email'), responses: { 200: json(SuccessResponse, 'Updated'), - 400: { description: 'Email without an “@” (empty body)' }, + 400: { description: 'Not a valid address, or over 255 characters (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), @@ -505,7 +525,7 @@ const app = new Hono() const id = await authedId(c) if (id === null) return unauthorized(c) const email = (await formField(c, 'email')).trim() - if (!email.includes('@')) return c.body(null, 400) + if (!isValidEmail(email)) return c.body(null, 400) await updateAccount(c.env.DB, id, { email }) return c.json({ success: true }) } @@ -605,11 +625,12 @@ const app = new Hono() describeRoute({ tags: ['Profile'], summary: 'Set bio', - description: 'Free text; empty is allowed. Persisted and broadcast.', + 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, }, }), @@ -617,6 +638,9 @@ const app = new Hono() 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 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 c93a50c..4f6bbb3 100644 --- a/apps/accounts/src/openapi.ts +++ b/apps/accounts/src/openapi.ts @@ -124,15 +124,21 @@ export const CreateAccountRequest = z.object({ /** Single-string form bodies, one per profile mutation. */ export const DisplayNameRequest = z.object({ - displayName: z.string().describe('Trimmed; empty is rejected (400)'), + displayName: z + .string() + .describe('Trimmed; letters and digits only, max 15. Empty or invalid is rejected (400)'), }) export const UsernameRequest = z.object({ - username: z.string().describe('Trimmed; must be unique and changes must remain'), + username: z + .string() + .describe( + 'Trimmed; letters and digits only, max 50. Must be unique and changes must remain' + ), }) export const EmailRequest = z.object({ - email: z.string().describe('Must contain "@"; otherwise 400'), + email: z.string().describe('A deliverable-looking address, max 255; otherwise 400'), }) export const PhoneRequest = z.object({ @@ -147,7 +153,9 @@ export const PronounsRequest = z.object({ pronounFlags: z.string().describe('Integer string bitmask; non-numeric is 400'), }) -export const BioRequest = z.object({ bio: z.string().describe('Free text; empty is allowed') }) +export const BioRequest = z.object({ + bio: z.string().describe('Free text, max 255; empty is allowed'), +}) export const ProfileImageRequest = z.object({ imageName: z.string().describe('Avatar object key; empty is rejected (400)'), diff --git a/apps/accounts/src/test/integration/api.test.ts b/apps/accounts/src/test/integration/api.test.ts index 7cb6b70..6185312 100644 --- a/apps/accounts/src/test/integration/api.test.ts +++ b/apps/accounts/src/test/integration/api.test.ts @@ -460,3 +460,150 @@ describe('auth-gated endpoints', () => { } }) }) + +// The names a player chooses are alphanumeric and length-capped, by the same rule the +// `rooms` worker applies (see `nameRejection` in @repo/domain). The three limits come +// from the client's own input boxes rather than a round number, so anything stored is +// something the game can render and re-edit. +// +// Server-generated names go around this deliberately — the seeded "Rec Room" account +// above has a space in its display name, and dorms are called `@'s Dorm`. The +// check belongs at the request handler, not in the db helpers. +describe('name, email and bio validation', () => { + const authed = async (sub: string) => ({ + ...(await bearer(sub)), + 'Content-Type': 'application/x-www-form-urlencoded', + }) + + test('PUT /account/me/username refuses anything but letters and digits, max 50', async () => { + const headers = await authed('8801') + for (const username of ['has space', 'under_score', 'punct!', 'café', 'a'.repeat(51)]) { + const res = await exports.default.fetch(`${ORIGIN}/account/me/username`, { + ...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 } + expect(body.success, username).toBe(false) + expect(body.error).toMatch(/letters and numbers|at most 50 characters/) + } + + // The rationed change must NOT be spent by a refusal: an account starts with one, + // and burning it on a typo would leave the player stuck with a name they never had. + const me = (await ( + await exports.default.fetch(`${ORIGIN}/account/me`, { headers: await bearer('8801') }) + ).json()) as { availableUsernameChanges: number } + expect(me.availableUsernameChanges).toBe(1) + + // 50 is the client's own cap, so a name that long has to be accepted. + const ok = await exports.default.fetch(`${ORIGIN}/account/me/username`, { + ...form({ username: 'a'.repeat(50) }), + headers, + }) + expect(((await ok.json()) as { success: boolean }).success).toBe(true) + }) + + test('PUT /account/me/displayname refuses anything but letters and digits, max 15', async () => { + const headers = await authed('8802') + for (const displayName of ['has space', 'punct!', 'a'.repeat(16)]) { + const res = await exports.default.fetch(`${ORIGIN}/account/me/displayname`, { + ...form({ displayName }), + headers, + }) + // An empty 400, matching what this route already answers for an empty name — + // it acks with a bare `{ success: true }` and has never sent the client a body + // on failure. + expect(res.status, displayName).toBe(400) + } + + // 15 is the client's box, so it must fit. + const ok = await exports.default.fetch(`${ORIGIN}/account/me/displayname`, { + ...form({ displayName: 'a'.repeat(15) }), + headers, + }) + expect(ok.status).toBe(200) + }) + + // Syntax comes from the `isemail` package rather than a pattern written here — this is + // a contact address nothing is ever sent to in order to prove it, so a hand-rolled + // regex only buys more edge cases to get wrong. It enforces the RFC's own + // 254-character maximum, which is why there's no separate length check. + test('POST /account/me/email requires a syntactically valid address', async () => { + const headers = await authed('8803') + const bad = [ + 'nope', // no @ at all — what this route used to be the only check for + '@example.com', // nothing to deliver to + 'someone@', // no domain + 'someone@example.', // empty last label + 'two words@example.com', // whitespace + `${'a'.repeat(250)}@example.com`, // past the RFC's 254 + ] + for (const email of bad) { + const res = await exports.default.fetch(`${ORIGIN}/account/me/email`, { + ...form({ email }), + method: 'POST', + headers, + }) + expect(res.status, email).toBe(400) + } + + // `someone@localhost` is in the ACCEPTED list on purpose: it's valid per the RFC, + // and an undeliverable address costs nothing here. + for (const email of [ + 'someone@example.com', + 'first.last+tag@mail.example.co.uk', + 'someone@localhost', + ]) { + const res = await exports.default.fetch(`${ORIGIN}/account/me/email`, { + ...form({ email }), + method: 'POST', + headers, + }) + expect(res.status, email).toBe(200) + } + }) + + test('PUT /account/me/bio caps the stored text at 255 characters', async () => { + const headers = await authed('8804') + + const ok = await exports.default.fetch(`${ORIGIN}/account/me/bio`, { + ...form({ bio: 'b'.repeat(255) }), + headers, + }) + expect(ok.status).toBe(200) + + // Refused rather than truncated — storing half a sentence reads as data loss. + const tooLong = await exports.default.fetch(`${ORIGIN}/account/me/bio`, { + ...form({ bio: 'b'.repeat(256) }), + headers, + }) + expect(tooLong.status).toBe(400) + + // The refusal changed nothing: the 255-character bio is still what's stored. + const me = await exports.default.fetch(`${ORIGIN}/account/8804/bio`) + expect(((await me.json()) as { bio: string }).bio).toBe('b'.repeat(255)) + }) +}) + +// Phone is deliberately NOT held to the name rule above: the client sends E.164 +// (`+15552223333`), so a letters-and-digits check would reject every real number by +// eating the leading `+`. Pinned here because this route sits between two that DID just +// get stricter, and the obvious next "cleanup" is to make it match them. +test('POST /account/me/phone stores an E.164 number exactly as the client sends it', async () => { + const res = await exports.default.fetch(`${ORIGIN}/account/me/phone`, { + ...form({ phone: '+15552223333' }), + method: 'POST', + headers: { ...(await bearer('8805')), 'Content-Type': 'application/x-www-form-urlencoded' }, + }) + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ success: true }) + + // Read from the row: phone is stored but not surfaced by any DTO, so there's no + // endpoint to check it through. + const row = await env.DB.prepare( + "SELECT json_extract(data, '$.phone') AS phone FROM account WHERE json_extract(data, '$.accountId') = 8805" + ).first<{ phone: string }>() + // Verbatim — no normalising, no stripping of the +. + expect(row?.phone).toBe('+15552223333') +}) diff --git a/apps/api/src/events-db.ts b/apps/api/src/events-db.ts index 81c7a69..e521e66 100644 --- a/apps/api/src/events-db.ts +++ b/apps/api/src/events-db.ts @@ -17,6 +17,12 @@ * relational table rather than a JSON blob. */ +import { + glyphLength, + MAX_EVENT_DESCRIPTION_LENGTH, + MAX_EVENT_NAME_LENGTH, +} from '@repo/domain' + /** * Schema DDL (mirror of migrations/0006_event.sql + 0007_event_attendee.sql, sans any * seed rows). @@ -251,6 +257,30 @@ function asInt(value: unknown): number | undefined { * clear the value. Timestamps are normalized here, so an unparseable one is dropped * rather than stored. */ +/** + * Why a parsed event body can't be stored, or `null` when it's fine. + * + * Length only. An event name is a title, not an identifier — "Building a Better Room + * Using Trigonometry" is a real one — so the alphanumeric rule the account and room + * names carry would be wrong here. Absent fields are skipped: an update posts only what + * it changes, and create defaults a missing name rather than refusing it. + * + * The name is measured AFTER trimming, matching what create/update actually store. + */ +export function eventInputRejection(input: EventInput): string | null { + const name = input.name?.trim() + if (name !== undefined && glyphLength(name) > MAX_EVENT_NAME_LENGTH) { + return `Event names can be at most ${MAX_EVENT_NAME_LENGTH} characters.` + } + if ( + input.description !== undefined && + glyphLength(input.description) > MAX_EVENT_DESCRIPTION_LENGTH + ) { + return `Event descriptions can be at most ${MAX_EVENT_DESCRIPTION_LENGTH} characters.` + } + return null +} + export function parseEventBody(body: unknown): EventInput { const outer = (typeof body === 'object' && body !== null ? body : {}) as Record const nested = outer.PlayerEvent diff --git a/apps/api/src/routes/events.ts b/apps/api/src/routes/events.ts index efb46b9..9f11bb5 100644 --- a/apps/api/src/routes/events.ts +++ b/apps/api/src/routes/events.ts @@ -14,6 +14,7 @@ import { getEventsByIds, getLiveEvents, isEventResponseType, + eventInputRejection, parseEventBody, searchEvents, setEventResponse, @@ -327,6 +328,7 @@ export const eventRoutes = new Hono({ strict: false }) requestBody: jsonBody(PlayerEventRequest, 'The event to schedule'), responses: { 200: json(PlayerEventResultDto, 'The created event'), + 400: { description: 'Name over 64 or description over 512 characters (empty body)' }, 401: UNAUTHORIZED_RESPONSE, }, }), @@ -334,7 +336,13 @@ export const eventRoutes = new Hono({ strict: false }) const id = await authedId(c) if (id === null) return unauthorized(c) const body = await c.req.json().catch(() => ({})) - const event = await createEvent(c.env.DB, id, parseEventBody(body)) + const input = parseEventBody(body) + // The one thing this route isn't lenient about. Everything else here defaults a + // missing or unusable field, but a name or description past the stored length + // can't be defaulted into something sensible — and truncating a player's event + // description silently is worse than refusing it. + if (eventInputRejection(input) !== null) return c.body(null, 400) + const event = await createEvent(c.env.DB, id, input) await notifyEventCreated(c, event) return c.json(toEventResult(event)) } @@ -359,6 +367,7 @@ export const eventRoutes = new Hono({ strict: false }) requestBody: jsonBody(PlayerEventRequest, 'The fields to change'), responses: { 200: json(PlayerEventResultDto, 'The updated event'), + 400: { description: 'Name over 64 or description over 512 characters (empty body)' }, 401: UNAUTHORIZED_RESPONSE, 403: { description: 'Not the event’s creator (empty body)' }, 404: { description: 'No such event (empty body)' }, @@ -373,7 +382,9 @@ export const eventRoutes = new Hono({ strict: false }) if (existing.CreatorPlayerId !== id) return c.body(null, 403) const body = await c.req.json().catch(() => ({})) - const updated = await updateEvent(c.env.DB, eventId, parseEventBody(body)) + const input = parseEventBody(body) + if (eventInputRejection(input) !== null) return c.body(null, 400) + const updated = await updateEvent(c.env.DB, eventId, input) // updateEvent only returns null when the row vanished, which the read above rules out. return c.json(toEventResult(updated!)) } diff --git a/apps/api/src/test/integration/api.test.ts b/apps/api/src/test/integration/api.test.ts index b2bae5b..964d14b 100644 --- a/apps/api/src/test/integration/api.test.ts +++ b/apps/api/src/test/integration/api.test.ts @@ -2420,6 +2420,44 @@ describe('player events', () => { expect(upcoming.StartTime).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$/) }) + // The one thing the event writes are strict about. Everything else here defaults a + // missing or unusable field (a nameless event becomes "Untitled Event"), but a name or + // description past the stored length can't be defaulted into anything sensible, and + // truncating a player's description silently is worse than refusing the write. + // + // Deliberately length ONLY: an event name is a title, not an identifier — the fixture + // above is called "Building a Better Room Using Trigonometry" — so the alphanumeric + // rule that guards usernames and room names would be wrong here. + test('POST /api/playerevents/v2 caps the name at 64 and the description at 512', async () => { + expect((await post('/api/playerevents/v2', { Name: 'n'.repeat(65), RoomId: 3 })).status).toBe( + 400 + ) + expect((await post('/api/playerevents/v2', { Name: 'n'.repeat(64), RoomId: 3 })).status).toBe( + 200 + ) + + const withDescription = (description: string) => + post('/api/playerevents/v2', { Name: 'Described', RoomId: 3, Description: description }) + expect((await withDescription('d'.repeat(513))).status).toBe(400) + expect((await withDescription('d'.repeat(512))).status).toBe(200) + // Counted in code points, so an emoji costs one character rather than two. + expect((await withDescription('🎉'.repeat(512))).status).toBe(200) + + // Spaces and punctuation stay fine — this is a title, not an identifier. + expect( + (await post('/api/playerevents/v2', { Name: "Bob's Big Night (2)!", RoomId: 3 })).status + ).toBe(200) + + // The update path enforces the same limits, and a refusal leaves the event alone. + const event = await create({ Name: 'EditMe', RoomId: 3 }) + const tooLong = await post(`/api/playerevents/v2/${event.PlayerEventId}`, { + Name: 'n'.repeat(65), + }) + expect(tooLong.status).toBe(400) + const after = await get(`/api/playerevents/v1/${event.PlayerEventId}`) + expect(((await after.json()) as PlayerEvent).Name).toBe('EditMe') + }) + test('POST /api/playerevents/v2 answers the write envelope, not the bare event', async () => { const res = await post('/api/playerevents/v2', { Name: 'Enveloped', RoomId: 3 }) const body = (await res.json()) as PlayerEventResult diff --git a/apps/clubs/src/clubs.app.ts b/apps/clubs/src/clubs.app.ts index 6adc848..ab87307 100644 --- a/apps/clubs/src/clubs.app.ts +++ b/apps/clubs/src/clubs.app.ts @@ -2,6 +2,11 @@ import { Hono } from 'hono' import { describeRoute, openAPIRouteHandler } from 'hono-openapi' import { useWorkersLogger } from 'workers-tagged-logger' +import { + glyphLength, + MAX_CLUB_DESCRIPTION_LENGTH, + MAX_CLUB_NAME_LENGTH, +} from '@repo/domain' import { intVar, logger, withCleanSpec, withNotFound, withOnError } from '@repo/hono-helpers' import { validateAndGetAccountId } from '@repo/jwt' @@ -101,8 +106,6 @@ async function authedId(c: Context): Promise { */ const DEFAULT_MAX_CLUBS_PER_ACCOUNT = 10 -/** Longest a club name may be (the reference's MaxNameLength). */ -const MAX_CLUB_NAME_LENGTH = 16 /** * The tiers `members/invite` may grant — the real member roles only. Creator (100) is @@ -665,6 +668,14 @@ const app = new Hono() if ([...name].length > MAX_CLUB_NAME_LENGTH) { return clubError(c, `Club names can be at most ${MAX_CLUB_NAME_LENGTH} characters.`) } + // Counted in code points like the name above, so an emoji-heavy description is + // measured the way a player sees it rather than by UTF-16 units. + if (glyphLength(description) > MAX_CLUB_DESCRIPTION_LENGTH) { + return clubError( + c, + `Club descriptions can be at most ${MAX_CLUB_DESCRIPTION_LENGTH} characters.` + ) + } // The per-account cap, checked after the cheap validations so a rejected name // costs no extra D1 read. const maxClubs = intVar(c.env.MAX_CLUBS_PER_ACCOUNT, DEFAULT_MAX_CLUBS_PER_ACCOUNT) @@ -778,9 +789,19 @@ const app = new Hono() } } + // Same absent-means-unchanged rule as the name, so a club with no description + // isn't forced to grow one just to be edited. + const description = field('description') || undefined + if (description !== undefined && glyphLength(description) > MAX_CLUB_DESCRIPTION_LENGTH) { + return clubError( + c, + `Club descriptions can be at most ${MAX_CLUB_DESCRIPTION_LENGTH} characters.` + ) + } + const updated = await updateClub(c.env.DB, clubId, { name, - description: field('description') || undefined, + description, category: field('category')?.trim() || undefined, visibility: parseVisibility(field('visibility')), joinability: parseJoinability(field('joinability')), diff --git a/apps/clubs/src/openapi.ts b/apps/clubs/src/openapi.ts index 2500998..c88c3e2 100644 --- a/apps/clubs/src/openapi.ts +++ b/apps/clubs/src/openapi.ts @@ -65,7 +65,7 @@ export const EmptyObject = z.object({}) */ export const ClubDto = z.object({ ClubId: z.int(), - Name: z.string().describe('At most 16 characters; letters, digits and basic punctuation'), + Name: z.string().describe('At most 40 characters; letters, digits and basic punctuation'), Description: z.string(), Category: z.string().describe('One of the /club/categoryTags values; defaults to Social'), Visibility: z.int().describe('ClubVisibility: 0 = Private, 1 = Public'), @@ -277,8 +277,8 @@ export const ChatDisabledResponse = z.boolean() export const CreateClubRequest = z.object({ name: z .string() - .describe('Required; at most 16 characters, letters/digits/basic punctuation only'), - description: z.string().optional(), + .describe('Required; at most 40 characters, letters/digits/basic punctuation only'), + description: z.string().optional().describe('At most 512 characters'), category: z.string().optional().describe('Defaults to Social when unset'), visibility: z.string().optional().describe('By name (`Public`/`Private`) or number'), joinability: z @@ -292,8 +292,11 @@ export const CreateClubRequest = z.object({ /** `PUT /club/:clubId/modifydetails` (and `/modify`) form body. */ export const ModifyClubRequest = z.object({ - name: z.string().optional().describe('Empty means unchanged, not "clear it"'), - description: z.string().optional().describe('Empty means unchanged'), + name: z + .string() + .optional() + .describe('At most 40 characters. Empty means unchanged, not "clear it"'), + description: z.string().optional().describe('At most 512 characters. Empty means unchanged'), category: z.string().optional(), visibility: z.string().optional().describe('By name (`Public`/`Private`) or number'), joinability: z diff --git a/apps/clubs/src/test/integration/api.test.ts b/apps/clubs/src/test/integration/api.test.ts index 980d55a..3776726 100644 --- a/apps/clubs/src/test/integration/api.test.ts +++ b/apps/clubs/src/test/integration/api.test.ts @@ -241,9 +241,16 @@ describe('clubs endpoints', () => { expect(emoji.status).toBe(400) expect(await emoji.json()).toMatchObject({ success: false, value: null }) - // Names cap at 16 characters. - expect((await create({ name: 'a'.repeat(17) })).status).toBe(400) - expect((await create({ name: 'a'.repeat(16) })).status).toBe(200) + // Names cap at 40 characters. + expect((await create({ name: 'a'.repeat(41) })).status).toBe(400) + expect((await create({ name: 'a'.repeat(40) })).status).toBe(200) + + // Descriptions cap at 512. Counted in code points, so an emoji-heavy one isn't + // refused at half the length a player can see (the description has no charset rule + // — only the name does). + expect((await create({ name: 'DescTooLong', description: 'd'.repeat(513) })).status).toBe(400) + expect((await create({ name: 'DescAtLimit', description: 'd'.repeat(512) })).status).toBe(200) + expect((await create({ name: 'DescEmoji', description: '🎉'.repeat(512) })).status).toBe(200) // Basic punctuation is allowed. expect((await create({ name: "Bob's Club (2)" })).status).toBe(200) diff --git a/apps/rooms/src/rooms.app.ts b/apps/rooms/src/rooms.app.ts index 2b47cc0..b0e1370 100644 --- a/apps/rooms/src/rooms.app.ts +++ b/apps/rooms/src/rooms.app.ts @@ -31,7 +31,9 @@ import { getSubRoomPermissions, getSubRoomSaves, getVisitedRooms, + MAX_ROOM_NAME_LENGTH, modifySubRoom, + nameRejection, publishSubRoomSave, removeCheer, removeFavorite, @@ -1071,6 +1073,9 @@ const app = new Hono() const name = typeof raw === 'string' ? raw.trim() : '' if (name === '') return roomEnvelope(c, null, 'You must enter a name for your room.') + // Shape before availability, so a rejected name costs no D1 read. + const badName = nameRejection(name, 'room name', MAX_ROOM_NAME_LENGTH) + if (badName !== null) return roomEnvelope(c, null, badName) if (await getRoomByName(c.env.DB, name)) { return roomEnvelope(c, null, 'A room with that name already exists!') } @@ -1195,6 +1200,12 @@ const app = new Hono() Error: 'You must enter a name for your room!', }) } + // Same ErrorId as the empty case — the client keys off it to mark the field, and + // both are the name being unusable. The sentence is what tells them which. + const badName = nameRejection(name, 'room name', MAX_ROOM_NAME_LENGTH) + if (badName !== null) { + return roomResult(c, { Success: false, ErrorId: 'Rooms.InvalidName', Error: badName }) + } // Reject if a different room already uses this name (case-insensitive). const existing = await getRoomByName(c.env.DB, name) @@ -2075,6 +2086,10 @@ const app = new Hono() Error: 'You must enter a name for your room!', }) } + const badName = nameRejection(name, 'subroom name', MAX_ROOM_NAME_LENGTH) + if (badName !== null) { + return roomResult(c, { Success: false, ErrorId: 'Rooms.InvalidName', Error: badName }) + } const maxPlayers = typeof body.maxPlayers === 'string' ? Number.parseInt(body.maxPlayers, 10) : Number.NaN @@ -2383,6 +2398,8 @@ const app = new Hono() const body = (await c.req.parseBody().catch(() => ({}))) as Record const name = typeof body.name === 'string' ? body.name.trim() : '' if (name === '') return roomEnvelope(c, null, 'You must enter a name for your subroom!') + const badName = nameRejection(name, 'subroom name', MAX_ROOM_NAME_LENGTH) + if (badName !== null) return roomEnvelope(c, null, badName) const result = await createSubRoom(c.env.DB, roomId, accountId, name) if (!result) return roomEnvelope(c, null, 'This room does not exist!') diff --git a/apps/rooms/src/test/integration/api.test.ts b/apps/rooms/src/test/integration/api.test.ts index eb66bc4..a0e54dd 100644 --- a/apps/rooms/src/test/integration/api.test.ts +++ b/apps/rooms/src/test/integration/api.test.ts @@ -2026,7 +2026,7 @@ describe('rooms endpoints', () => { }) it('PUT /rooms/:id/subrooms/:sid/modify is auth-gated, owner-only, and persists subroom settings', async () => { - const fields = { name: 'My Cool Subroom', accessibility: '1', maxPlayers: '20' } + const fields = { name: 'MyCoolSubroom', accessibility: '1', maxPlayers: '20' } // No token → 401 (auth gate). expect((await putForm('/rooms/2/subrooms/2/modify', fields)).status).toBe(401) // Not the owner (room 2 is owned by account 1) → NotOwner. @@ -2056,7 +2056,7 @@ describe('rooms endpoints', () => { Accessibility: number MaxPlayers: number } - expect(sub).toMatchObject({ Name: 'My Cool Subroom', Accessibility: 1, MaxPlayers: 20 }) + expect(sub).toMatchObject({ Name: 'MyCoolSubroom', Accessibility: 1, MaxPlayers: 20 }) }) it('PUT /rooms/:id/subrooms/:sid/accessibility takes the enum name the client sends', async () => { @@ -2499,10 +2499,10 @@ describe('rooms endpoints', () => { await SELF.fetch(`${ORIGIN}/rooms/2/subrooms`, { method: 'POST', headers: { ...(await bearer('1')), 'Content-Type': 'application/x-www-form-urlencoded' }, - body: new URLSearchParams({ name: 'to-delete' }).toString(), + body: new URLSearchParams({ name: 'ToDelete' }).toString(), }) ).json()) as { value: { SubRooms: SubRoom[] } } - const newId = created.value.SubRooms.find((s) => s.Name === 'to-delete')!.SubRoomId + const newId = created.value.SubRooms.find((s) => s.Name === 'ToDelete')!.SubRoomId // No token → 401. Not the owner → success:false. Unknown subroom → success:false. expect((await del(2, newId)).status).toBe(401) @@ -2656,3 +2656,81 @@ describe('rooms endpoints', () => { } }) }) + +// Room and subroom names are held to the same rule as usernames — letters and digits, +// at most 32 (see `nameRejection` in @repo/domain). All four routes that take a +// player-supplied name enforce it, and each keeps its OWN refusal shape: the create +// paths answer the lowercase `{ success, error, value }` envelope, the two settings +// routes answer `{ Success, ErrorId, Error }` with the same `Rooms.InvalidName` id they +// already used for an empty name. The client keys off those, so the rule had to fit the +// existing shapes rather than introduce a fifth one. +// +// Names the SERVER generates are exempt on purpose — a dorm is `@'s Dorm`, +// which this rule would reject. That's why the check lives in the handlers. +describe('room name validation', () => { + const bad = ['My Room', 'under_score', 'punct!', 'a'.repeat(33)] + + const post = async (path: string, fields: Record, sub: string) => + SELF.fetch(`${ORIGIN}${path}`, { + method: 'POST', + headers: { ...(await bearer(sub)), 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams(fields).toString(), + }) + + const put = async (path: string, fields: Record, sub: string) => + SELF.fetch(`${ORIGIN}${path}`, { + method: 'PUT', + headers: { ...(await bearer(sub)), 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams(fields).toString(), + }) + + it('refuses a bad name when a player clones a room into existence', async () => { + for (const name of bad) { + const res = await post('/rooms/2/clone', { name }, '1') + const body = (await res.json()) as { success: boolean; error: string; value: unknown } + expect(body.success, name).toBe(false) + expect(body.error).toMatch(/letters and numbers|at most 32 characters/) + expect(body.value).toBeNull() + } + }) + + it('refuses a bad name on rename, with the id the client already handles', async () => { + for (const name of bad) { + const res = await put('/rooms/2/name', { name }, '1') + const body = (await res.json()) as { Success: boolean; ErrorId: string; Error: string } + expect(body.Success, name).toBe(false) + expect(body.ErrorId).toBe('Rooms.InvalidName') + expect(body.Error).toMatch(/letters and numbers|at most 32 characters/) + } + + // Unchanged: the refusals above never reached the write. + const room = (await (await SELF.fetch(`${ORIGIN}/rooms/2`)).json()) as { Name: string } + expect(room.Name).not.toMatch(/[^A-Za-z0-9]/) + }) + + it('refuses a bad name when creating or modifying a subroom', async () => { + for (const name of bad) { + const created = await post('/rooms/2/subrooms', { name }, '1') + const env1 = (await created.json()) as { success: boolean; error: string } + expect(env1.success, name).toBe(false) + expect(env1.error).toMatch(/letters and numbers|at most 32 characters/) + + const modified = await put( + '/rooms/2/subrooms/2/modify', + { name, accessibility: '1', maxPlayers: '20' }, + '1' + ) + const res2 = (await modified.json()) as { Success: boolean; ErrorId: string } + expect(res2.Success, name).toBe(false) + expect(res2.ErrorId).toBe('Rooms.InvalidName') + } + }) + + it('accepts a 32-character alphanumeric name', async () => { + const name = 'a'.repeat(32) + const res = await post('/rooms/2/subrooms', { name }, '1') + const body = (await res.json()) as { success: boolean; value: { SubRooms: Array<{ Name: string }> } } + expect(body.success).toBe(true) + expect(body.value.SubRooms.some((s) => s.Name === name)).toBe(true) + }) +}) diff --git a/apps/www/src/client/App.tsx b/apps/www/src/client/App.tsx index d14f844..eaf4849 100644 --- a/apps/www/src/client/App.tsx +++ b/apps/www/src/client/App.tsx @@ -150,6 +150,12 @@ interface CallOptions { json?: unknown /** Send the session token. */ authed?: boolean + /** + * What to say when the worker refuses with a 400 and NO body. Several accounts routes + * do exactly that (email, display name, bio), so without this the player reads + * "Request failed (400)" — the status, not the reason. + */ + refusal?: string } /** Call a worker. Returns the parsed body, or throws with something worth showing. */ @@ -178,6 +184,10 @@ async function call>(url: string, opts: CallOptions setToken(null) throw new Error('Your session has expired. Please sign in again.') } + // Only when the body really is empty — a worker that did send a reason keeps it. + if (opts.refusal !== undefined && res.status === 400 && Object.keys(data).length === 0) { + throw new Error(opts.refusal) + } throw new Error(errorMessage(data, res.status)) } return data as T @@ -254,10 +264,20 @@ async function changeUsername(username: string): Promise { return fetchMe() } -/** Set the account's email. `accounts` refuses an address with no `@` — with an empty - * 400 body, so the check is made here too rather than showing a bare status. */ +/** + * Set the account's email. + * + * The address is NOT checked here first. `accounts` validates it with `isemail`, which + * can't come along into the browser (it reaches for node's `util`, which vite stubs with + * a throwing Proxy in dev) — and a second, looser copy of the rule would only disagree + * with the real one. The server decides; this just names the refusal it answers with. + */ const saveEmail = (email: string): Promise => - call(`${where().accounts}/account/me/email`, { form: { email }, authed: true }) + call(`${where().accounts}/account/me/email`, { + form: { email }, + authed: true, + refusal: 'That email address looks wrong.', + }) /** Change the account's password. Lives on `auth`, not `accounts`. */ const changePassword = (oldPassword: string, newPassword: string): Promise => @@ -950,15 +970,7 @@ function SignupForm({ onSubmit={(e) => { e.preventDefault() void run(async () => { - // Checked before anything is created, because `accounts` rejects an address - // with no `@` and by then the account would exist: better to fail the form - // than to hand back an account whose email silently didn't save. Same rule - // accounts applies, deliberately no stricter — this is a contact address, not - // an identity, and nothing is sent to it to prove it. const wanted = email.trim() - if (wanted !== '' && !wanted.includes('@')) { - throw new Error('That email address looks wrong.') - } try { await signUp(password, widgetToken) diff --git a/packages/domain/package.json b/packages/domain/package.json index 86c26b0..1a71706 100644 --- a/packages/domain/package.json +++ b/packages/domain/package.json @@ -13,5 +13,8 @@ "@cloudflare/workers-types": "4.20260630.1", "@repo/tools": "workspace:*", "@repo/typescript-config": "workspace:*" + }, + "dependencies": { + "isemail": "^3.2.0" } } diff --git a/packages/domain/src/index.ts b/packages/domain/src/index.ts index f2903ba..448fa4c 100644 --- a/packages/domain/src/index.ts +++ b/packages/domain/src/index.ts @@ -9,3 +9,4 @@ export * from './presence-db' export * from './gifts-db' export * from './inventory-invention-db' export * from './relationships-db' +export * from './validation' diff --git a/packages/domain/src/validation.ts b/packages/domain/src/validation.ts new file mode 100644 index 0000000..ce8ed62 --- /dev/null +++ b/packages/domain/src/validation.ts @@ -0,0 +1,96 @@ +import isEmail from 'isemail' + +/** + * Limits on the free text a player can put into their account and their rooms. + * + * Shared by `accounts` and `rooms` so one rule can't drift from the other — a username + * and a room name are held to the same shape, and both are typed into the same client. + * + * These check only what a player SUPPLIES. Names the server generates go around them: + * a dorm is called `@'s Dorm` (see `rooms-db.ts`), which the name rule below + * would reject, and auto-assigned usernames (`SwiftFox4821`, `Player42`) happen to + * satisfy it. So validate at the request handler, never inside the db helpers. + * + * Emptiness is deliberately NOT checked here. Every caller already rejects an empty + * value in its own words, and those sentences reach players through response envelopes + * the client renders verbatim — see the client-contract notes in CLAUDE.md. + */ + +/** + * Name lengths. All three come from what the CLIENT will accept in the matching input + * box, not from a round number: accepting more here would store a name the game can't + * re-enter or edit, so the server matches the box rather than being generous. + */ +export const MAX_USERNAME_LENGTH = 50 +export const MAX_DISPLAY_NAME_LENGTH = 15 +export const MAX_ROOM_NAME_LENGTH = 32 + +/** + * Club and event limits. Longer than the name limits above because these aren't + * identifiers — a club name and an event name are titles, and both allow the + * punctuation and spaces a title needs (clubs enforce their own charset rule; events + * enforce none at all, since an event is called things like "Building a Better Room + * Using Trigonometry"). + */ +export const MAX_CLUB_NAME_LENGTH = 40 +export const MAX_CLUB_DESCRIPTION_LENGTH = 512 +export const MAX_EVENT_NAME_LENGTH = 64 +export const MAX_EVENT_DESCRIPTION_LENGTH = 512 + +/** + * Length in code points rather than UTF-16 units, so an emoji or other astral character + * counts once instead of twice — the way a player counts what they typed. + */ +export const glyphLength = (value: string): number => Array.from(value).length + +/** Max length of a profile bio. */ +export const MAX_BIO_LENGTH = 255 + +/** + * Letters and digits only — no spaces, punctuation, or accents. + * + * Deliberately narrow: these names are shown to other players, used to search, and (for + * usernames) typed into a sign-in box, so anything that can be confused for another name + * is worth refusing. It also rules out the homoglyph and right-to-left tricks that come + * with allowing arbitrary Unicode. + */ +const NAME_PATTERN = /^[A-Za-z0-9]+$/ + +/** + * Why a player-supplied name is unacceptable, or `null` when it's fine. + * + * `label` names the thing in the returned sentence ('username', 'room name'), so the + * message reads correctly wherever it's surfaced. `max` is required rather than + * defaulted: the three limits differ, and a caller that forgets which one it wants + * should have to say so instead of silently taking someone else's. + */ +export function nameRejection(value: string, label: string, max: number): string | null { + if (value.length > max) { + return `Your ${label} can be at most ${max} characters.` + } + if (!NAME_PATTERN.test(value)) { + return `Your ${label} can only contain letters and numbers.` + } + return null +} + +/** + * Whether a supplied email is one worth storing — RFC 5321/5322 syntax, via `isemail`. + * + * A hand-rolled pattern is the wrong shape of work here: this is a contact address + * nothing is ever sent to in order to prove it, so the only thing a stricter regex buys + * is more edge cases to get wrong. Note it also enforces the RFC's 254-character maximum + * itself, which is why there's no separate length cap. + * + * It accepts a dotless domain (`someone@localhost`), which a dotted-domain rule would + * refuse. That's the RFC being right and the shortcut being wrong, and an undeliverable + * address costs nothing here. + */ +export function isValidEmail(value: string): boolean { + return isEmail.validate(value) +} + +/** Whether a supplied bio is within the stored length. */ +export function isValidBio(value: string): boolean { + return value.length <= MAX_BIO_LENGTH +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 403e1ef..a165bf4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -864,6 +864,10 @@ importers: version: 4.105.0(@cloudflare/workers-types@4.20260630.1) packages/domain: + dependencies: + isemail: + specifier: ^3.2.0 + version: 3.2.0 devDependencies: '@cloudflare/workers-types': specifier: 4.20260630.1 @@ -3174,6 +3178,10 @@ packages: resolution: {integrity: sha512-rbku49cWloU5bSMI+zaRaXdQHXnthP6DZ/vLnfdSKyL4zUzuWnomtOEiZZOd+ioQ+avFo/qau3KPTc7Fjy1uPA==} engines: {node: '>=12'} + isemail@3.2.0: + resolution: {integrity: sha512-zKqkK+O+dGqevc93KNsbZ/TqTUFd46MwWjYOoMrjIMZ51eU7DtQG3Wmd9SQQT7i7RVnuTPEiYEWHU3MSbxC1Tg==} + engines: {node: '>=4.0.0'} + jiti@2.6.1: resolution: {integrity: sha512-ekilCSN1jwRvIbgeg/57YFh8qQDNbwDb9xT/qu2DAHbFFZUicIl4ygVaAvzveMhMVr3LnpSKTNnwt8PoOfmKhQ==} hasBin: true @@ -3542,6 +3550,10 @@ packages: property-information@7.2.0: resolution: {integrity: sha512-IAtzIB6sUiWaJYrX9smp3V46pBGbBeLFRGdh25kg1334VcBlD8HzhPeNIWQH9zhGmo2itIe25EHt9dQP7G5hmg==} + punycode@2.3.1: + resolution: {integrity: sha512-vYt7UD1U9Wg6138shLtLOvdAu+8DsC/ilFtEVHcH+wydcSpNE20AfSOduf6MkRFahL5FY7X1oU7nKVZFtfq8Fg==} + engines: {node: '>=6'} + quansync@0.2.11: resolution: {integrity: sha512-AifT7QEbW9Nri4tAwR5M/uzpBuqfZf+zwaEM/QkzEjj7NBuFD2rBuy0K3dE+8wltbezDV7JMA0WfnCPYRSYbXA==} @@ -6154,6 +6166,10 @@ snapshots: is-regexp@3.1.0: {} + isemail@3.2.0: + dependencies: + punycode: 2.3.1 + jiti@2.6.1: optional: true @@ -6677,6 +6693,8 @@ snapshots: property-information@7.2.0: {} + punycode@2.3.1: {} + quansync@0.2.11: {} radix-vue@1.9.17(vue@3.5.40(typescript@6.0.3)):