From 77b5c9d5eafc02cff952e2cc859d2fd95d8e0576 Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 16:52:04 -0400 Subject: [PATCH 1/2] refactor(actions): parse server-action input with zod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .coderabbit.yaml states the rule twice — validate input with zod before touching a service, and parse rather than cast — and no server action followed it. All five action modules now do. Behaviour does not move. Every parse failure maps back to the code the page already renders (`readValidationCode` throws if a schema emits one the destination's error map lacks), and the two error channels stay as they were: payouts returns typed codes as state, the other four throw, matching the posture that page-unreachable input earns no notice copy. `parseYmd` and `battleReportUrlProblem` move to a new payouts/validation module because a "use server" file may only export async functions. The schemas keep every ordering the `if` chains they replace had — declaration order inside `z.object`, refine order inside a chain — and `date_invalid` / `date_future` stay separately reachable rather than collapsing into one refinement. URL validation still precedes the appraisal call, so a bad scheme costs no network round trip. Bound arguments are caller-controlled over the wire, so they are parsed too — after the auth guard, never before, so a caller who fails the guard learns nothing about the shape of what they sent. --- src/app/account/actions.ts | 23 ++ src/app/admin/access-lists/actions.ts | 21 +- src/app/admin/accounts/actions.ts | 73 +++- src/app/admin/sync/actions.ts | 15 +- src/app/payouts/actions.ts | 251 ++++++------- src/app/payouts/validation.ts | 223 ++++++++++++ tests/account-actions-validation.test.ts | 34 ++ tests/actions-guard-before-validation.test.ts | 76 ++++ ...in-access-lists-actions-validation.test.ts | 35 ++ .../admin-accounts-actions-validation.test.ts | 119 ++++++ ...dmin-accounts-save-note-validation.test.ts | 30 ++ tests/admin-sync-actions-validation.test.ts | 25 ++ tests/payouts-validation.test.ts | 339 ++++++++++++++++++ 13 files changed, 1121 insertions(+), 143 deletions(-) create mode 100644 src/app/payouts/validation.ts create mode 100644 tests/account-actions-validation.test.ts create mode 100644 tests/actions-guard-before-validation.test.ts create mode 100644 tests/admin-access-lists-actions-validation.test.ts create mode 100644 tests/admin-accounts-actions-validation.test.ts create mode 100644 tests/admin-accounts-save-note-validation.test.ts create mode 100644 tests/admin-sync-actions-validation.test.ts create mode 100644 tests/payouts-validation.test.ts diff --git a/src/app/account/actions.ts b/src/app/account/actions.ts index ca7376ae..ab274d75 100644 --- a/src/app/account/actions.ts +++ b/src/app/account/actions.ts @@ -1,5 +1,6 @@ "use server"; +import { z } from "zod"; import { and, eq } from "drizzle-orm"; import { revalidatePath } from "next/cache"; import { redirect } from "next/navigation"; @@ -12,6 +13,26 @@ import { setMainCharacter, unlinkCharacter, wakeSelf } from "@/services/accounts import { unlinkDiscord } from "@/services/discord-link"; import { getSessionAccount } from "@/services/session"; +/** `setMainAction`/`unlinkAction`'s one bound argument. Neither action reads + * any FormData at all — `account/page.tsx` renders zero named controls — but + * a bound server-action argument is still caller input on the wire, not + * trusted state, so it is parsed rather than cast. Unreachable from the + * rendered page with anything but a real character id, so a bad value throws + * rather than earning notice copy — the same posture the admin actions take + * on their own unreachable inputs (`access-lists/actions.ts`'s `parseId`, + * `sync/actions.ts`'s `jobTypeSchema`). Both callers parse AFTER + * `requireAccount()`, not before — same order those two siblings use — so a + * caller who is not signed in gets `requireAccount`'s own redirect regardless + * of what they sent, rather than a thrown validation error telling an + * unauthenticated request whether its argument even had the right shape. */ +const characterIdSchema = z.number().int().positive({ error: "invalid_character_id" }); + +function parseCharacterId(value: number): number { + const parsed = characterIdSchema.safeParse(value); + if (!parsed.success) throw new Error("invalid_character_id"); + return parsed.data; +} + async function requireAccount(): Promise { const cfg = getConfig(); const sid = (await cookies()).get(cfg.sessionCookieName)?.value; @@ -26,6 +47,7 @@ async function requireAccount(): Promise { export async function setMainAction(characterId: number): Promise { const accountId = await requireAccount(); + characterId = parseCharacterId(characterId); const result = await getDb().transaction((dbtx) => setMainCharacter(dbtx, accountId, accountId, characterId), ); @@ -52,6 +74,7 @@ export async function setMainAction(characterId: number): Promise { export async function unlinkAction(characterId: number): Promise { const accountId = await requireAccount(); + characterId = parseCharacterId(characterId); const db = getDb(); const cfg = getConfig(); // members may only unlink their own characters. This is a fast, non-locking diff --git a/src/app/admin/access-lists/actions.ts b/src/app/admin/access-lists/actions.ts index afcdc394..185201a4 100644 --- a/src/app/admin/access-lists/actions.ts +++ b/src/app/admin/access-lists/actions.ts @@ -1,5 +1,6 @@ "use server"; +import { z } from "zod"; import { revalidatePath } from "next/cache"; import { redirect } from "next/navigation"; import { getDb } from "@/db"; @@ -17,14 +18,22 @@ import { type ActionOutcome } from "@/app/_components/confirm-group"; * worker performs every read. */ -/** A server action takes whatever the wire sends, so an id that will become a - * bigint column and an audit target is parsed rather than trusted. - * Unreachable from the rendered page, so a bad value throws rather than +/** An id that will become a bigint column and an audit target, parsed with + * zod rather than cast — a server action takes whatever the wire sends. + * `removeWatchAction`'s `accessListId` arrives as a submit button's own + * name/value, not a hidden input, so a scripted POST with no submitter gives + * `null`; the input type is `FormDataEntryValue | null`, never bare `string`. */ +const idSchema = z.preprocess( + (value) => Number(value), + z.number().refine((n) => Number.isSafeInteger(n) && n > 0, { error: "invalid_id" }), +); + +/** Unreachable from the rendered page, so a bad value throws rather than * earning notice copy — the same posture `syncJobAction` takes on `jobType`. */ function parseId(value: FormDataEntryValue | null): number { - const n = Number(value); - if (!Number.isSafeInteger(n) || n <= 0) throw new Error("invalid_id"); - return n; + const parsed = idSchema.safeParse(value); + if (!parsed.success) throw new Error("invalid_id"); + return parsed.data; } export async function designateHolderAction(formData: FormData): Promise { diff --git a/src/app/admin/accounts/actions.ts b/src/app/admin/accounts/actions.ts index d8ff5010..079f66f8 100644 --- a/src/app/admin/accounts/actions.ts +++ b/src/app/admin/accounts/actions.ts @@ -1,5 +1,6 @@ "use server"; +import { z } from "zod"; import { revalidatePath } from "next/cache"; import { redirect } from "next/navigation"; import { getDb } from "@/db"; @@ -27,6 +28,43 @@ import { type AdminAccountsDoneCode, } from "./view"; +/** + * Every bound argument below is caller input on the wire, not trusted state — + * a server action's bound arguments round-trip through the client the same + * way a form field does — so each is parsed with zod before touching a + * service, mirroring `saveNoteAction`'s own `invalid_note` throw (the one + * FormData field this file reads directly). None of these are reachable with + * a bad value from the rendered page itself, so every rejection throws rather + * than earning notice copy — the same posture `access-lists/actions.ts`'s + * `parseId` and `sync/actions.ts`'s `jobTypeSchema` take on their own + * unreachable inputs. + * + * Every `assertValid` call below sits AFTER `requireAdminAction()`, not + * before — the same order `access-lists/actions.ts`'s `parseId` and + * `sync/actions.ts`'s `jobTypeSchema` already use. Authorization is checked + * before input shape: a caller who fails the guard gets the guard's own + * redirect regardless of what they sent, rather than a thrown validation + * error handing an unauthenticated request a way to distinguish a malformed + * argument from a well-formed one. + */ +const accountIdSchema = z.uuid({ error: "invalid_account_id" }); +const identitySchema = z.string().min(1, { error: "invalid_identity" }); +const listSearchSchema = z.string({ error: "invalid_list_search" }); +const tierSchema = z.enum(["member", "associate", "alumni"], { error: "invalid_tier" }); +const approveTierSchema = z.enum(["alumni", "associate"], { error: "invalid_tier" }); +const statusSchema = z.enum(["active", "cryo"], { error: "invalid_status" }); +const characterIdSchema = z.number().int().positive({ error: "invalid_character_id" }); +const noteSchema = z.string({ error: "invalid_note" }); + +/** Parses `value` against `schema`, throwing `Error(message)` — not a + * `ZodError` — on rejection, matching every other unreachable-input throw in + * this file's siblings. */ +function assertValid(schema: z.ZodType, value: unknown, message: string): T { + const result = schema.safeParse(value); + if (!result.success) throw new Error(message); + return result.data; +} + /** * `/admin/accounts?[&]done=&name=&at=`. * @@ -171,6 +209,10 @@ export async function setTierAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + tier = assertValid(tierSchema, tier, "invalid_tier"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => setTierManual(tx, actor, accountId, tier), ); @@ -203,6 +245,10 @@ export async function approveAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + tier = assertValid(approveTierSchema, tier, "invalid_tier"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => approveAccount(tx, actor, accountId, tier), ); @@ -221,6 +267,9 @@ export async function returnToAutoAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => returnTierToAuto(tx, actor, accountId), ); @@ -246,6 +295,10 @@ export async function setStatusAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + status = assertValid(statusSchema, status, "invalid_status"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => setAccountStatus(tx, actor, accountId, status), ); @@ -284,14 +337,15 @@ export async function saveNoteAction( formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - const raw = formData.get("note"); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); // FormData.get() is string | File | null. Coercing a File or a missing field // to "" would silently CLEAR the note (setStatusNote maps "" to null) and // write a status.note_changed audit entry for an edit nobody requested. // Reject the malformed request instead; "" itself stays valid — that is how // the form asks for the note to be cleared. This can only happen if the // form itself is tampered with, so it stays a throw rather than a race. - if (typeof raw !== "string") throw new Error("invalid_note"); + const raw = assertValid(noteSchema, formData.get("note"), "invalid_note"); const result = await getDb().transaction((tx) => setStatusNote(tx, actor, accountId, raw), @@ -313,6 +367,9 @@ export async function syncAccountAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); await getDb().transaction(async (tx) => { await logAudit(tx, { actor, action: "sync.requested", target: accountId }); await enqueueSync(tx, { kind: "account", accountId }); @@ -327,6 +384,9 @@ export async function promoteAdminAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => promoteAdmin(tx, actor, accountId)); if (!result.ok) { // promoteAdmin predates AdminMutationResult and returns `error` as @@ -349,6 +409,9 @@ export async function demoteAdminAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => demoteAdmin(tx, actor, accountId)); if (!result.ok && result.error === "last_admin") { // Surface the service's protection instead of a 500 (carry-over). @@ -389,6 +452,9 @@ export async function unlinkDiscordAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + identity = assertValid(identitySchema, identity, "invalid_identity"); const result = await getDb().transaction((tx) => unlinkDiscord(tx, actor, accountId, "admin"), ); @@ -449,6 +515,9 @@ export async function setMainAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); + accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); + characterId = assertValid(characterIdSchema, characterId, "invalid_character_id"); + listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); const result = await getDb().transaction((tx) => setMainCharacterAsAdmin(tx, actor, accountId, characterId), ); diff --git a/src/app/admin/sync/actions.ts b/src/app/admin/sync/actions.ts index a525c353..36e0f0bc 100644 --- a/src/app/admin/sync/actions.ts +++ b/src/app/admin/sync/actions.ts @@ -1,9 +1,10 @@ "use server"; +import { z } from "zod"; import { revalidatePath } from "next/cache"; import { redirect } from "next/navigation"; import { getDb } from "@/db"; -import { isJobType } from "@/core/schedules"; +import { isJobType, type JobType } from "@/core/schedules"; import { HEARTBEAT_STALE_AFTER_MS, evaluateFreshness } from "@/core/health"; import { requireAdminAction } from "@/lib/admin-guard"; import { logAudit } from "@/services/audit"; @@ -13,6 +14,12 @@ import { elapsedShort } from "@/app/_components/format-ago"; import { type ActionOutcome } from "@/app/_components/confirm-group"; import { queuedNotice } from "./view"; +/** Derives its enum from `isJobType` (`@/core/schedules`) — the one place a + * job's schedule is written down — rather than duplicating that job-type + * list here. `z.custom` is the v4 way to lift an existing type guard into a + * schema without re-deriving the literals it checks. */ +const jobTypeSchema = z.custom(isJobType, { error: "invalid_job_type" }); + export async function syncAllAction(): Promise { const { accountId: actor } = await requireAdminAction(); await getDb().transaction(async (tx) => { @@ -69,16 +76,18 @@ export async function syncJobAction( formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - const jobType = formData.get("jobType"); + const rawJobType = formData.get("jobType"); // A server action takes whatever the wire sends, and `jobType` becomes a // queue name downstream. Only the schedules table's own keys are accepted, // so a tampered form cannot enqueue against an arbitrary queue. The dispatch // side checks again; this one keeps the bad row out of the outbox and the // audit log entirely. Unreachable from the rendered page, so it throws // rather than earning notice copy. - if (!isJobType(jobType)) { + const parsed = jobTypeSchema.safeParse(rawJobType); + if (!parsed.success) { throw new Error("invalid_job_type"); } + const jobType = parsed.data; const db = getDb(); await db.transaction(async (tx) => { diff --git a/src/app/payouts/actions.ts b/src/app/payouts/actions.ts index 248e52a7..aec6f0ed 100644 --- a/src/app/payouts/actions.ts +++ b/src/app/payouts/actions.ts @@ -13,7 +13,6 @@ import { setItemPrice, } from "@/services/payout-loot"; import { - MAX_SHARES_HUNDREDTHS, PayoutDuplicateParticipantError, PayoutHasPaidError, PayoutLockedError, @@ -44,12 +43,24 @@ import { classifyOpenInfoFailure } from "@/core/open-info-error"; import { createTriffClient, TriffError } from "@/lib/triff/client"; import type { PricingMode } from "@/core/pricing"; import { parseRosterPaste } from "@/core/roster-paste"; -import { iskToCents } from "@/core/payout-split"; import { encodeDropped } from "./dropped"; import { encodeUnresolved } from "./unresolved"; +import { NEW_OPERATION_ERRORS, OPERATION_ERRORS } from "./errors"; import type { NewOperationErrorCode, OperationErrorCode } from "./errors"; import type { AppraisalResult } from "@/services/appraisal"; import { unresolvedRosterNames } from "./new/unresolved-roster"; +import { + battleReportUrlFieldSchema, + buildCreateOperationSchema, + corpSharePctFieldSchema, + flatPoolFieldSchema, + nameFieldSchema, + occurredAtFieldSchema, + participantNameFieldSchema, + readValidationCode, + sharesFieldSchema, + unitPriceFieldSchema, +} from "./validation"; /** * `addAppraisedPoolAction` used to read these from the form: a "Pricing" @@ -112,72 +123,11 @@ function operationFailed(operationId: string, code: OperationErrorCode): never { redirect(`/payouts/${operationId}?error=${code}`); } -/** - * The one definition of what a battle report link is allowed to be: an - * absolute http(s) URL, or nothing. - * - * The rule matters because the value is rendered as a plain `` on the - * operation's own page, so a `javascript:` (or `data:`, or any other) scheme - * reaching the database is stored XSS. `URL.protocol` is lowercase-normalized - * by the URL spec, so an allowlist compare on it is not case-bypassable, and - * anything `new URL` cannot parse at all — a bare `zkillboard.com`, say — is - * not a link this can store either. - * - * Extracted rather than written twice. Both entry points need it — the create - * form and the inline edit on the operation page — and both now RETURN the code - * rather than redirecting, so the value the operator typed survives the - * rejection (the loot paste beside the field in the composer's case, the field's - * own text in the editor's). This returns the code and lets each caller shape it - * into its own state type. When the two checks were written out separately, the - * comment in each claiming to match the other went stale inside one change. - * - * Returns null when there is nothing to object to, including for a null or - * empty value — the field is optional at both call sites. - */ -function battleReportUrlProblem( - value: string | null, -): "url_invalid" | "url_scheme" | null { - if (!value) return null; - let scheme: string; - try { - scheme = new URL(value).protocol; - } catch { - return "url_invalid"; - } - return scheme === "http:" || scheme === "https:" ? null : "url_scheme"; -} - -/** - * The `` wire format, parsed strictly. Shared by the two - * places an operation date arrives (`createOperationAction` and - * `setOccurredAtAction`) for the same reason `battleReportUrlProblem` is - * shared: two copies of a check drift. - * - * `new Date(...)` alone is not enough. It rejects "not-a-date" and month 13, - * but it *normalizes* a day past the end of the month rather than refusing it - * — `new Date("2026-02-30")` is 2026-03-02 — so a hand-built request (the - * browser's own date picker cannot produce one) would store a different day - * than it submitted, silently, on a record operators reconcile against their - * own logs. Comparing the parsed UTC components back against the submitted - * digits is what catches the rollover; the format guard in front of it is what - * keeps locale-ish spellings like "2026-2-3" out, since those parse in local - * time and would shift the stored day by a timezone. - */ -function parseYmd(raw: string): Date | null { - const m = /^(\d{4})-(\d{2})-(\d{2})$/.exec(raw.trim()); - if (!m) return null; - const [, y, mo, d] = m; - const parsed = new Date(`${y}-${mo}-${d}T00:00:00.000Z`); - if (Number.isNaN(parsed.getTime())) return null; - if ( - parsed.getUTCFullYear() !== Number(y) || - parsed.getUTCMonth() + 1 !== Number(mo) || - parsed.getUTCDate() !== Number(d) - ) { - return null; - } - return parsed; -} +/** `parseYmd` and `battleReportUrlProblem` now live in `./validation`, wrapped + * by the zod field schemas there (`occurredAtFieldSchema`, + * `battleReportUrlFieldSchema`) — see that module's docblocks for both. Every + * caller in this file now goes through those schemas rather than either bare + * function directly. */ /** The composer's own state. `null` is `useActionState`'s initial value, * matching `AppraiseActionState`'s own convention: `state === null` never @@ -253,20 +203,18 @@ export async function createOperationAction( formData: FormData, ): Promise { const actor = await requireOperatorAccount(); - const name = field(formData, "name").trim(); - if (!name) return { ok: false, code: "name_required" }; - const occurredAt = parseYmd(field(formData, "occurredAt")); - if (occurredAt === null) return { ok: false, code: "date_invalid" }; - // `max={today}` on the form declares this rule; until now the browser was the - // only thing enforcing it, so a hand-built request could always date an - // operation into the future. That became load-bearing the moment this form - // took `noValidate` (see new-operation-form.tsx): with native validation off, - // the attribute stops being enforcement at all and this check is the rule. + // `max={today}` on the form declares the future-date rule; until now the + // browser was the only thing enforcing it, so a hand-built request could + // always date an operation into the future. That became load-bearing the + // moment this form took `noValidate` (see new-operation-form.tsx): with + // native validation off, the attribute stops being enforcement at all and + // this schema's `date_future` refine is the rule. // // Compared against the same UTC-midnight boundary `parseYmd` produces, not // against `now`: both sides are then EVE-day granular, so an operation // recorded during today's downtime is not rejected for being "ahead" of an - // instant a few hours later in the same day. + // instant a few hours later in the same day. Computed fresh per call (not a + // module-level constant) — see `buildCreateOperationSchema`'s own comment. // // `/payouts/[id]`'s own date field is deliberately NOT changed to match. It // still runs native validation, so its `max={today}` still holds for anyone @@ -275,15 +223,22 @@ export async function createOperationAction( // `OPERATION_ERRORS` for a path this task did not touch. const todayUtc = new Date(); todayUtc.setUTCHours(0, 0, 0, 0); - if (occurredAt.getTime() > todayUtc.getTime()) { - return { ok: false, code: "date_future" }; - } - // Checked before any network call, alongside name and date, so a bad scheme - // never triggers an appraisal only to be thrown away. - const battleReportUrl = field(formData, "battleReportUrl").trim() || null; - const urlProblem = battleReportUrlProblem(battleReportUrl); - if (urlProblem) return { ok: false, code: urlProblem }; + // One parse, not three sequential `if`s — see `buildCreateOperationSchema`'s + // own comment for why declaration order (name, occurredAt, battleReportUrl) + // is what keeps a blank name and a bad URL both present landing on + // `name_required`, matching the `if` chain this replaces. Every field is + // checked before any network call, so a typo or a bad scheme never triggers + // an appraisal only to be thrown away. + const parsed = buildCreateOperationSchema(todayUtc).safeParse({ + name: field(formData, "name").trim(), + occurredAt: field(formData, "occurredAt"), + battleReportUrl: field(formData, "battleReportUrl").trim(), + }); + if (!parsed.success) { + return { ok: false, code: readValidationCode(parsed.error, NEW_OPERATION_ERRORS) }; + } + const { name, occurredAt, battleReportUrl } = parsed.data; const lootPaste = field(formData, "lootPaste").trim(); const rosterPaste = field(formData, "rosterPaste").trim(); @@ -514,12 +469,7 @@ export async function addFlatPoolAction( formData: FormData, ): Promise { const actor = await requireOperatorAccount(); - const totalValue = field(formData, "totalValue").trim(); - const notes = field(formData, "notes").trim(); const rawPaste = field(formData, "rawPaste").trim(); - if (!notes) { - return { ok: false, code: "note_required" }; - } // accepts scientific notation like "1e5" client-side; // iskToCents' regex rejects it, but let this action fail with the same // readable message the other numeric fields above use, rather than relying @@ -531,9 +481,18 @@ export async function addFlatPoolAction( // form to fix the number. Rejecting here keeps them on the form, with // `FlatPoolForm`'s controlled inputs — not this state — holding what they // typed. - if (!/^\d+(\.\d{1,2})?$/.test(totalValue)) { - return { ok: false, code: "total_invalid" }; + // + // `notes` declared before `totalValue` in `flatPoolFieldSchema` (`./validation`) + // is what keeps a blank note taking priority over a malformed total, matching + // the `if` chain this replaces. + const parsed = flatPoolFieldSchema.safeParse({ + notes: field(formData, "notes").trim(), + totalValue: field(formData, "totalValue").trim(), + }); + if (!parsed.success) { + return { ok: false, code: readValidationCode(parsed.error, OPERATION_ERRORS) }; } + const { notes, totalValue } = parsed.data; await getDb().transaction((dbtx) => addFlatPool(dbtx, actor, operationId, { @@ -568,10 +527,15 @@ export async function setItemPriceAction( // silent round to 0.01 would inflate the line 2.5x with no sign of it. The // escape hatch for genuinely sub-cent heaps is the flat-total pool, which // takes a pool value directly and skips per-item pricing. - if (!/^\d+(\.\d{1,2})?$/.test(unitPrice)) { - return { ok: false, code: "price_invalid", value: unitPrice }; + const parsed = unitPriceFieldSchema.safeParse(unitPrice); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: unitPrice, + }; } - await getDb().transaction((dbtx) => setItemPrice(dbtx, actor, itemId, unitPrice)); + await getDb().transaction((dbtx) => setItemPrice(dbtx, actor, itemId, parsed.data)); revalidateOperation(operationId); return { ok: true }; } @@ -605,7 +569,14 @@ export async function addParticipantAction( ): Promise { const actor = await requireOperatorAccount(); const name = field(formData, "name").trim(); - if (!name) return { ok: false, code: "participant_name_required", value: name }; + const parsed = participantNameFieldSchema.safeParse(name); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: name, + }; + } try { await getDb().transaction((dbtx) => addParticipant(dbtx, actor, operationId, name)); } catch (err) { @@ -626,31 +597,24 @@ export async function setParticipantSharesAction( ): Promise { const actor = await requireOperatorAccount(); const shares = field(formData, "shares").trim(); - if (!shares) return { ok: false, code: "shares_required", value: shares }; // Format first, positivity second, and in that order deliberately: iskToCents // *throws* on anything its regex rejects (core/payout-split.ts), so calling it // on "abc" would escape past the checks below — and text in a numeric field // is the likeliest bad input this control gets. Mirrors the regex-then-parse - // order addFlatPoolAction already uses for totalValue. - if (!/^-?\d+(\.\d{1,2})?$/.test(shares)) { - return { ok: false, code: "shares_invalid", value: shares }; - } - // Mirrors payout_participant_shares_ck (shares > 0) with a readable message - // before the raw string reaches the numeric(6,2) column. - if (iskToCents(shares) <= 0n) { - return { ok: false, code: "shares_positive", value: shares }; - } - // The numeric(6, 2) column's own range, mirrored here for the same reason the - // three checks above mirror the format and payout_participant_shares_ck: an - // unbounded "10000" reaches Postgres as a raw numeric overflow. assertSharesInRange - // in the service enforces this for every caller; this copy is the one that can - // give the operator a page with their roster still on it. Same constant, so the - // two cannot drift. - if (iskToCents(shares) > MAX_SHARES_HUNDREDTHS) { - return { ok: false, code: "shares_range", value: shares }; + // order addFlatPoolAction already uses for totalValue. `sharesFieldSchema` + // (`./validation`) is what keeps that ordering: its own `.transform` calling + // `iskToCents` never runs once an earlier `.refine` in the same chain has + // already failed. + const parsed = sharesFieldSchema.safeParse(shares); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: shares, + }; } await getDb().transaction((dbtx) => - setParticipantShares(dbtx, actor, participantId, shares), + setParticipantShares(dbtx, actor, participantId, parsed.data), ); revalidateOperation(operationId); return { ok: true }; @@ -677,8 +641,17 @@ export async function setNameAction( ): Promise { const actor = await requireOperatorAccount(); const name = field(formData, "name").trim(); - if (!name) return { ok: false, code: "name_required", value: name }; - await getDb().transaction((dbtx) => setOperationName(dbtx, actor, operationId, name)); + const parsed = nameFieldSchema.safeParse(name); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: name, + }; + } + await getDb().transaction((dbtx) => + setOperationName(dbtx, actor, operationId, parsed.data), + ); revalidateOperation(operationId); return { ok: true }; } @@ -690,12 +663,16 @@ export async function setOccurredAtAction( ): Promise { const actor = await requireOperatorAccount(); const raw = field(formData, "occurredAt"); - const occurredAt = parseYmd(raw); - if (occurredAt === null) { - return { ok: false, code: "date_invalid", value: raw }; + const parsed = occurredAtFieldSchema.safeParse(raw); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: raw, + }; } await getDb().transaction((dbtx) => - setOccurredAt(dbtx, actor, operationId, occurredAt), + setOccurredAt(dbtx, actor, operationId, parsed.data), ); revalidateOperation(operationId); return { ok: true }; @@ -707,10 +684,18 @@ export async function setBattleReportUrlAction( formData: FormData, ): Promise { const actor = await requireOperatorAccount(); - const raw = field(formData, "battleReportUrl").trim() || null; - const problem = battleReportUrlProblem(raw); - if (problem) return { ok: false, code: problem, value: raw ?? "" }; - await getDb().transaction((dbtx) => setBattleReportUrl(dbtx, actor, operationId, raw)); + const raw = field(formData, "battleReportUrl").trim(); + const parsed = battleReportUrlFieldSchema.safeParse(raw); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: raw, + }; + } + await getDb().transaction((dbtx) => + setBattleReportUrl(dbtx, actor, operationId, parsed.data), + ); revalidateOperation(operationId); return { ok: true }; } @@ -775,14 +760,16 @@ export async function setCorpShareAction( ): Promise { const actor = await requireOperatorAccount(); const corpSharePct = field(formData, "corpSharePct").trim(); - if (!/^\d+(\.\d{1,2})?$/.test(corpSharePct)) { - return { ok: false, code: "share_format", value: corpSharePct }; - } - if (Number(corpSharePct) > 100) { - return { ok: false, code: "share_range", value: corpSharePct }; + const parsed = corpSharePctFieldSchema.safeParse(corpSharePct); + if (!parsed.success) { + return { + ok: false, + code: readValidationCode(parsed.error, OPERATION_ERRORS), + value: corpSharePct, + }; } await getDb().transaction((dbtx) => - setCorpSharePct(dbtx, actor, operationId, corpSharePct), + setCorpSharePct(dbtx, actor, operationId, parsed.data), ); revalidateOperation(operationId); return { ok: true }; diff --git a/src/app/payouts/validation.ts b/src/app/payouts/validation.ts new file mode 100644 index 00000000..3618993e --- /dev/null +++ b/src/app/payouts/validation.ts @@ -0,0 +1,223 @@ +import { z } from "zod"; +import { MAX_SHARES_HUNDREDTHS } from "@/services/payouts"; +import { iskToCents } from "@/core/payout-split"; + +/** + * The `` wire format, parsed strictly. Shared by the two + * places an operation date arrives (`createOperationAction` and + * `setOccurredAtAction`) for the same reason `battleReportUrlProblem` is + * shared: two copies of a check drift. + * + * `new Date(...)` alone is not enough. It rejects "not-a-date" and month 13, + * but it *normalizes* a day past the end of the month rather than refusing it + * — `new Date("2026-02-30")` is 2026-03-02 — so a hand-built request (the + * browser's own date picker cannot produce one) would store a different day + * than it submitted, silently, on a record operators reconcile against their + * own logs. Comparing the parsed UTC components back against the submitted + * digits is what catches the rollover; the format guard in front of it is what + * keeps locale-ish spellings like "2026-2-3" out, since those parse in local + * time and would shift the stored day by a timezone. + */ +export function parseYmd(raw: string): Date | null { + const m = /^(\d{4})-(\d{2})-(\d{2})$/.exec(raw.trim()); + if (!m) return null; + const [, y, mo, d] = m; + const parsed = new Date(`${y}-${mo}-${d}T00:00:00.000Z`); + if (Number.isNaN(parsed.getTime())) return null; + if ( + parsed.getUTCFullYear() !== Number(y) || + parsed.getUTCMonth() + 1 !== Number(mo) || + parsed.getUTCDate() !== Number(d) + ) { + return null; + } + return parsed; +} + +/** + * The one definition of what a battle report link is allowed to be: an + * absolute http(s) URL, or nothing. + * + * The rule matters because the value is rendered as a plain `` on the + * operation's own page, so a `javascript:` (or `data:`, or any other) scheme + * reaching the database is stored XSS. `URL.protocol` is lowercase-normalized + * by the URL spec, so an allowlist compare on it is not case-bypassable, and + * anything `new URL` cannot parse at all — a bare `zkillboard.com`, say — is + * not a link this can store either. + * + * Extracted rather than written twice. Both entry points need it — the create + * form and the inline edit on the operation page — and both now RETURN the code + * rather than redirecting, so the value the operator typed survives the + * rejection (the loot paste beside the field in the composer's case, the field's + * own text in the editor's). This returns the code and lets each caller shape it + * into its own state type. When the two checks were written out separately, the + * comment in each claiming to match the other went stale inside one change. + * + * Returns null when there is nothing to object to, including for a null or + * empty value — the field is optional at both call sites. + */ +export function battleReportUrlProblem( + value: string | null, +): "url_invalid" | "url_scheme" | null { + if (!value) return null; + let scheme: string; + try { + scheme = new URL(value).protocol; + } catch { + return "url_invalid"; + } + return scheme === "http:" || scheme === "https:" ? null : "url_scheme"; +} + +/** + * Reads the rejected field's code back off a failed zod parse and proves it is + * one the destination page's own error map can render — the same "unmapped + * code fails loudly" property `operationFailed`'s `keyof typeof` parameter + * already gives every throw-based redirect (`actions.ts`), applied here to a + * returned parse failure instead. Every schema below carries its code as the + * issue's own `message` (via `error:`/`ctx.addIssue({ message })`) rather than + * zod's own generated wording, deliberately: this reader is what proves that + * carried string is one the caller can actually show, and a schema that typos + * a code, or a page whose map drops one a schema still emits, throws here at + * the first parse that hits it rather than silently rendering nothing. + * + * Reads `issues[0]` only — first-check-wins, the same convention every schema + * below is written to preserve (declaration order inside a `z.object`, or + * `.refine`/`.transform` order inside a single field's own chain). + */ +export function readValidationCode( + error: z.ZodError, + errors: Record, +): Code { + const code = error.issues[0]?.message; + if (code === undefined || !Object.hasOwn(errors, code)) { + throw new Error(`payouts/validation: unmapped code from zod issue: ${String(code)}`); + } + return code as Code; +} + +/** `NEW_OPERATION_ERRORS`' `name_required`, shared by `createOperationAction` + * (as the `name` key of `buildCreateOperationSchema`) and `setNameAction`, + * whose `OPERATION_ERRORS` entry carries the same code — see `errors.ts`'s + * own docblock for why the two maps stay separate even though this one code + * spells identically in both. Expects an already-trimmed string, matching + * every field schema below: trimming is `field()`'s caller's job (see + * `actions.ts`), not this schema's. */ +export const nameFieldSchema = z.string().refine((s) => s.length > 0, { + error: "name_required", +}); + +/** Parses to a `Date`, or fails with `date_invalid` — no future check. Shared + * by `setOccurredAtAction` directly and by `buildCreateOperationSchema` below + * (as its base, with a `date_future` refine layered on top only there): the + * detail page's own date editor has no future check (pre-existing gap, out + * of scope — see `actions.ts`'s own comment on `setOccurredAtAction`), so + * sharing this schema rather than `buildCreateOperationSchema`'s whole + * `occurredAt` key is what keeps that gap from silently closing here. */ +export const occurredAtFieldSchema = z.string().transform((raw, ctx) => { + const d = parseYmd(raw); + if (!d) { + ctx.addIssue({ code: "custom", message: "date_invalid" }); + return z.NEVER; + } + return d; +}); + +/** `url_invalid` / `url_scheme` are spelled identically in both error maps + * (see `errors.ts`), so one schema serves both `createOperationAction` and + * `setBattleReportUrlAction`. Expects an already-trimmed string; empty means + * "nothing submitted", the same optional-field reading `battleReportUrlProblem` + * itself takes. */ +export const battleReportUrlFieldSchema = z.string().transform((raw, ctx) => { + const value = raw || null; + const problem = battleReportUrlProblem(value); + if (problem) { + ctx.addIssue({ code: "custom", message: problem }); + return z.NEVER; + } + return value; +}); + +/** + * `createOperationAction`'s one schema, folding the name/date/battle-report-url + * checks that used to be three separate `if`s into a single parse — see that + * action's own comment for why all three still have to run before the paste is + * ever appraised. Declared in `name`, `occurredAt`, `battleReportUrl` order + * because zod (v4) walks a `z.object`'s keys in declaration order and this + * reader only ever looks at `issues[0]`: that order is what keeps "a blank name + * plus a bad URL" landing on `name_required`, matching the sequential `if` + * chain this replaces. + * + * A factory rather than a module-level constant because `date_future` depends + * on the request's own instant — see `createOperationAction`'s comment on why + * `todayUtc` is computed once per call rather than at import time. + */ +export function buildCreateOperationSchema(todayUtc: Date) { + return z.object({ + name: nameFieldSchema, + occurredAt: occurredAtFieldSchema.refine((d) => d.getTime() <= todayUtc.getTime(), { + error: "date_future", + }), + battleReportUrl: battleReportUrlFieldSchema, + }); +} + +/** `addFlatPoolAction`'s two required fields — `note_required` before + * `total_invalid`, matching the `if` chain this replaces: `notes` declared + * first is what keeps a blank note taking priority over a malformed total. */ +export const flatPoolFieldSchema = z.object({ + notes: z.string().refine((s) => s.length > 0, { error: "note_required" }), + totalValue: z + .string() + .refine((s) => /^\d+(\.\d{1,2})?$/.test(s), { error: "total_invalid" }), +}); + +/** `setItemPriceAction`'s one field. Two decimal places, matching numeric(20,2). */ +export const unitPriceFieldSchema = z + .string() + .refine((s) => /^\d+(\.\d{1,2})?$/.test(s), { error: "price_invalid" }); + +/** `addParticipantAction`'s one field. `participant_duplicate` is not part of + * this schema — it can only be known once the insert itself races against the + * roster, so `addParticipantAction` still catches `PayoutDuplicateParticipantError` + * after this parse succeeds. */ +export const participantNameFieldSchema = z.string().refine((s) => s.length > 0, { + error: "participant_name_required", +}); + +/** + * `setParticipantSharesAction`'s one field, folding all four of its checks + * into a single chain in the same order the `if` chain this replaces used: + * `shares_required`, then `shares_invalid` (format), then — only once the + * format is known to be safe for it — `iskToCents`, whose own + * `shares_positive` / `shares_range` checks close the chain. The order is load + * -bearing: `iskToCents` *throws* on anything its own regex rejects + * (`core/payout-split.ts`), so it must never run on a string the format refine + * above already rejected — and it does not, because a zod `.transform` does + * not run once an earlier step in the same chain has already failed (unlike a + * later `.refine`, which still evaluates against the original input; verified + * empirically for this codebase's zod v4). + */ +export const sharesFieldSchema = z + .string() + .refine((s) => s.length > 0, { error: "shares_required" }) + .refine((s) => /^-?\d+(\.\d{1,2})?$/.test(s), { error: "shares_invalid" }) + .transform((s, ctx) => { + const cents = iskToCents(s); + if (cents <= 0n) { + ctx.addIssue({ code: "custom", message: "shares_positive" }); + return z.NEVER; + } + if (cents > MAX_SHARES_HUNDREDTHS) { + ctx.addIssue({ code: "custom", message: "shares_range" }); + return z.NEVER; + } + return s; + }); + +/** `setCorpShareAction`'s one field: format before range, matching the `if` + * chain this replaces. */ +export const corpSharePctFieldSchema = z + .string() + .refine((s) => /^\d+(\.\d{1,2})?$/.test(s), { error: "share_format" }) + .refine((s) => Number(s) <= 100, { error: "share_range" }); diff --git a/tests/account-actions-validation.test.ts b/tests/account-actions-validation.test.ts new file mode 100644 index 00000000..4f31cace --- /dev/null +++ b/tests/account-actions-validation.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it, vi } from "vitest"; + +// setMainAction/unlinkAction now validate characterId AFTER requireAccount(), +// matching every sibling file's guard-before-validation order — see +// account/actions.ts's own comment on parseCharacterId. That means this test +// needs requireAccount's own dependencies (next/headers's cookies and +// getSessionAccount) mocked to resolve, the same way +// admin-sync-actions-validation.test.ts mocks admin-guard for the equivalent +// reason. Nothing below this mock touches a real database: the schema +// rejection throws before setMainCharacter/unlinkCharacter or getDb are ever +// called. +vi.mock("next/headers", () => ({ + cookies: async () => ({ get: () => ({ value: "session-id" }) }), +})); +vi.mock("@/services/session", () => ({ + getSessionAccount: async () => ({ accountId: "00000000-0000-0000-0000-000000000000" }), +})); + +const { setMainAction, unlinkAction } = await import("@/app/account/actions"); + +describe("account actions — bound characterId validation", () => { + it("setMainAction rejects a non-positive characterId with invalid_character_id", async () => { + await expect(setMainAction(0)).rejects.toThrow("invalid_character_id"); + await expect(setMainAction(-1)).rejects.toThrow("invalid_character_id"); + }); + + it("setMainAction rejects a non-integer characterId with invalid_character_id", async () => { + await expect(setMainAction(1.5)).rejects.toThrow("invalid_character_id"); + }); + + it("unlinkAction rejects a non-positive characterId with invalid_character_id", async () => { + await expect(unlinkAction(0)).rejects.toThrow("invalid_character_id"); + }); +}); diff --git a/tests/actions-guard-before-validation.test.ts b/tests/actions-guard-before-validation.test.ts new file mode 100644 index 00000000..a1d1d251 --- /dev/null +++ b/tests/actions-guard-before-validation.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it, vi } from "vitest"; + +/** + * The other `*-validation.test.ts` files mock their guard so it *resolves*, + * which is what lets them reach the schema rejections at all — but a resolving + * guard cannot tell guard-first from validation-first apart, because both + * orders reach the same throw. This file mocks the guards so they *reject*, + * which is the only arrangement where the two orders differ: with the guard + * first, a caller who fails it gets the guard's own outcome no matter how + * malformed their arguments were; with validation first, a malformed argument + * throws `invalid_*` and tells an unauthorized caller something about the + * shape of the input it sent. + * + * That order was written the wrong way round once already in this change, and + * every assertion here fails if it is written that way again. + */ +vi.mock("@/lib/admin-guard", () => ({ + requireAdminAction: async () => { + throw new Error("guard_denied"); + }, +})); + +// `requireAccount` (account/actions.ts) is not a shared helper — it is local to +// that module and redirects rather than throwing, so its denial is staged by +// giving it no session cookie and capturing the `redirect()` it reaches for. +vi.mock("next/headers", () => ({ + cookies: async () => ({ get: () => undefined }), +})); +vi.mock("next/navigation", () => ({ + redirect: (url: string) => { + throw new Error(`redirected:${url}`); + }, +})); + +const { setTierAction, saveNoteAction, syncAccountAction } = + await import("@/app/admin/accounts/actions"); +const { setMainAction, unlinkAction } = await import("@/app/account/actions"); + +describe("admin/accounts actions — the admin guard runs before any argument is parsed", () => { + it("setTierAction denies an unauthorized caller rather than reporting invalid_account_id", async () => { + await expect( + setTierAction( + "not-a-uuid", + "bogus" as never, + "", + "Some Pilot", + null, + new FormData(), + ), + ).rejects.toThrow("guard_denied"); + }); + + it("saveNoteAction denies before it reads the note field at all", async () => { + const formData = new FormData(); + formData.set("note", new Blob(["x"])); + await expect( + saveNoteAction("not-a-uuid", "", { seq: 0, changed: false }, formData), + ).rejects.toThrow("guard_denied"); + }); + + it("syncAccountAction denies before enqueueing or parsing", async () => { + await expect(syncAccountAction("not-a-uuid", "", "Some Pilot")).rejects.toThrow( + "guard_denied", + ); + }); +}); + +describe("account actions — requireAccount runs before characterId is parsed", () => { + it("setMainAction redirects a signed-out caller rather than reporting invalid_character_id", async () => { + await expect(setMainAction(0)).rejects.toThrow("redirected:/login"); + }); + + it("unlinkAction redirects a signed-out caller rather than reporting invalid_character_id", async () => { + await expect(unlinkAction(-1)).rejects.toThrow("redirected:/login"); + }); +}); diff --git a/tests/admin-access-lists-actions-validation.test.ts b/tests/admin-access-lists-actions-validation.test.ts new file mode 100644 index 00000000..0b68f8ea --- /dev/null +++ b/tests/admin-access-lists-actions-validation.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it, vi } from "vitest"; + +// designateHolderAction/addWatchAction/removeWatchAction all call +// requireAdminAction before parseId, so it must resolve for the test to reach +// the id validation at all. Nothing below this mock touches a real database: +// a bad id throws before designateHolder/addWatch/removeWatch or getDb are +// ever called. +vi.mock("@/lib/admin-guard", () => ({ + requireAdminAction: async () => ({ accountId: "00000000-0000-0000-0000-000000000000" }), +})); + +const { addWatchAction, designateHolderAction, removeWatchAction } = + await import("@/app/admin/access-lists/actions"); + +describe("access-lists actions — id validation", () => { + it("designateHolderAction rejects a non-numeric characterId with invalid_id", async () => { + const formData = new FormData(); + formData.set("characterId", "not-a-number"); + await expect(designateHolderAction(formData)).rejects.toThrow("invalid_id"); + }); + + it("addWatchAction rejects a zero/negative accessListId with invalid_id", async () => { + const formData = new FormData(); + formData.set("accessListId", "0"); + await expect(addWatchAction(formData)).rejects.toThrow("invalid_id"); + }); + + it("removeWatchAction rejects a missing accessListId (a scripted POST with no submitter) with invalid_id", async () => { + // The one call site where accessListId arrives as a submit button's own + // name/value rather than a hidden input, so a scripted POST with no + // submitter gives FormData.get(...) === null — parseId(null) must still + // throw invalid_id, not a TypeError coercing null. + await expect(removeWatchAction(null, new FormData())).rejects.toThrow("invalid_id"); + }); +}); diff --git a/tests/admin-accounts-actions-validation.test.ts b/tests/admin-accounts-actions-validation.test.ts new file mode 100644 index 00000000..57e50bea --- /dev/null +++ b/tests/admin-accounts-actions-validation.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it, vi } from "vitest"; + +// Every action below now validates its bound arguments (via `assertValid`) +// AFTER calling `requireAdminAction()`, not before — see this file's own +// docblock. That means the guard has to resolve for any of these throws to be +// reached at all, mocked rather than exercised for real, the same pattern +// admin-sync-actions-validation.test.ts and +// admin-access-lists-actions-validation.test.ts already use. Nothing below +// this mock touches a real database: every rejection throws before +// getDb/logAudit/enqueueSync or any service call. +vi.mock("@/lib/admin-guard", () => ({ + requireAdminAction: async () => ({ accountId: "00000000-0000-0000-0000-000000000000" }), +})); + +const { + approveAction, + demoteAdminAction, + promoteAdminAction, + returnToAutoAction, + saveNoteAction, + setMainAction, + setStatusAction, + setTierAction, + syncAccountAction, + unlinkDiscordAction, +} = await import("@/app/admin/accounts/actions"); + +const VALID_UUID = "00000000-0000-0000-0000-000000000000"; + +describe("admin/accounts actions — bound-argument validation", () => { + it("setTierAction rejects a malformed accountId with invalid_account_id", async () => { + await expect( + setTierAction("not-a-uuid", "member", "", "Some Pilot", null, new FormData()), + ).rejects.toThrow("invalid_account_id"); + }); + + it("setTierAction rejects a tier outside its own enum with invalid_tier", async () => { + await expect( + setTierAction(VALID_UUID, "bogus" as never, "", "Some Pilot", null, new FormData()), + ).rejects.toThrow("invalid_tier"); + }); + + it("approveAction rejects the wider setTierAction tier union with invalid_tier", async () => { + // approveAction's own tier union is narrower ("alumni" | "associate") than + // setTierAction's three-tier union — "member" is valid for its sibling but + // not here. + await expect( + approveAction( + VALID_UUID, + "member" as never, + "", + "Some Pilot", + null, + new FormData(), + ), + ).rejects.toThrow("invalid_tier"); + }); + + it("returnToAutoAction rejects a malformed accountId with invalid_account_id", async () => { + await expect( + returnToAutoAction("not-a-uuid", "", "Some Pilot", null, new FormData()), + ).rejects.toThrow("invalid_account_id"); + }); + + it("setStatusAction rejects a status outside its own enum with invalid_status", async () => { + await expect( + setStatusAction( + VALID_UUID, + "bogus" as never, + "", + "Some Pilot", + null, + new FormData(), + ), + ).rejects.toThrow("invalid_status"); + }); + + it("saveNoteAction rejects a malformed accountId before ever reading the note", async () => { + await expect( + saveNoteAction("not-a-uuid", "", { seq: 0, changed: false }, new FormData()), + ).rejects.toThrow("invalid_account_id"); + }); + + it("syncAccountAction rejects a malformed accountId with invalid_account_id", async () => { + await expect(syncAccountAction("not-a-uuid", "", "Some Pilot")).rejects.toThrow( + "invalid_account_id", + ); + }); + + it("promoteAdminAction rejects a malformed accountId with invalid_account_id", async () => { + await expect(promoteAdminAction("not-a-uuid", "", "Some Pilot")).rejects.toThrow( + "invalid_account_id", + ); + }); + + it("demoteAdminAction rejects a malformed accountId with invalid_account_id", async () => { + await expect(demoteAdminAction("not-a-uuid", "", "Some Pilot")).rejects.toThrow( + "invalid_account_id", + ); + }); + + it("unlinkDiscordAction rejects a malformed accountId with invalid_account_id", async () => { + await expect( + unlinkDiscordAction("not-a-uuid", "", "Some Pilot", null, new FormData()), + ).rejects.toThrow("invalid_account_id"); + }); + + it("setMainAction rejects a non-positive characterId with invalid_character_id", async () => { + await expect(setMainAction(VALID_UUID, 0, "", null, new FormData())).rejects.toThrow( + "invalid_character_id", + ); + }); + + it("setMainAction rejects a malformed accountId before the characterId is even considered", async () => { + await expect( + setMainAction("not-a-uuid", 0, "", null, new FormData()), + ).rejects.toThrow("invalid_account_id"); + }); +}); diff --git a/tests/admin-accounts-save-note-validation.test.ts b/tests/admin-accounts-save-note-validation.test.ts new file mode 100644 index 00000000..6f71bbdf --- /dev/null +++ b/tests/admin-accounts-save-note-validation.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from "vitest"; + +// saveNoteAction calls requireAdminAction() first, then validates +// accountId/listSearch, and only then reads the note — so this rejection, like +// every other one in admin-accounts-actions-validation.test.ts, needs the +// guard mocked to be reached at all. Nothing below this mock touches a real +// database: the note rejection throws before setStatusNote/getDb are called. +vi.mock("@/lib/admin-guard", () => ({ + requireAdminAction: async () => ({ accountId: "00000000-0000-0000-0000-000000000000" }), +})); + +const { saveNoteAction } = await import("@/app/admin/accounts/actions"); + +const VALID_UUID = "00000000-0000-0000-0000-000000000000"; + +describe("saveNoteAction — note validation", () => { + it("rejects a non-string note (a File field) with invalid_note, same as the original typeof check", async () => { + const formData = new FormData(); + formData.set("note", new Blob(["x"])); + await expect( + saveNoteAction(VALID_UUID, "", { seq: 0, changed: false }, formData), + ).rejects.toThrow("invalid_note"); + }); + + it("rejects a missing note field with invalid_note", async () => { + await expect( + saveNoteAction(VALID_UUID, "", { seq: 0, changed: false }, new FormData()), + ).rejects.toThrow("invalid_note"); + }); +}); diff --git a/tests/admin-sync-actions-validation.test.ts b/tests/admin-sync-actions-validation.test.ts new file mode 100644 index 00000000..d43fa0c4 --- /dev/null +++ b/tests/admin-sync-actions-validation.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it, vi } from "vitest"; + +// The guard runs before jobType validation in syncJobAction, so it must +// resolve for this test to reach that check at all — mocked rather than +// exercised for real, the same pattern health-routes-db-down.test.ts uses for +// a different dependency. Nothing below this mock touches a real database: +// the schema rejection throws before enqueueSync/logAudit/getDb are ever +// called. +vi.mock("@/lib/admin-guard", () => ({ + requireAdminAction: async () => ({ accountId: "00000000-0000-0000-0000-000000000000" }), +})); + +const { syncJobAction } = await import("@/app/admin/sync/actions"); + +describe("syncJobAction — jobType validation", () => { + it("rejects a jobType outside @/core/schedules's own JOB_CRON with invalid_job_type", async () => { + const formData = new FormData(); + formData.set("jobType", "not-a-real-job"); + await expect(syncJobAction(null, formData)).rejects.toThrow("invalid_job_type"); + }); + + it("rejects a missing jobType field with invalid_job_type", async () => { + await expect(syncJobAction(null, new FormData())).rejects.toThrow("invalid_job_type"); + }); +}); diff --git a/tests/payouts-validation.test.ts b/tests/payouts-validation.test.ts new file mode 100644 index 00000000..8d2b089a --- /dev/null +++ b/tests/payouts-validation.test.ts @@ -0,0 +1,339 @@ +import { describe, expect, it } from "vitest"; +import { z } from "zod"; +import { NEW_OPERATION_ERRORS, OPERATION_ERRORS } from "@/app/payouts/errors"; +import { + battleReportUrlFieldSchema, + battleReportUrlProblem, + buildCreateOperationSchema, + corpSharePctFieldSchema, + flatPoolFieldSchema, + nameFieldSchema, + occurredAtFieldSchema, + parseYmd, + participantNameFieldSchema, + readValidationCode, + sharesFieldSchema, + unitPriceFieldSchema, +} from "@/app/payouts/validation"; + +// `todayUtc` fixed at a known instant so `date_future` is deterministic +// against it, rather than against whatever day the suite happens to run on. +const TODAY_UTC = new Date("2026-08-10T00:00:00.000Z"); + +describe("readValidationCode", () => { + it("reads the first issue's message as the code when it is mapped", () => { + const result = nameFieldSchema.safeParse(""); + if (result.success) throw new Error("expected rejection"); + expect(readValidationCode(result.error, NEW_OPERATION_ERRORS)).toBe("name_required"); + }); + + it("throws on a code the destination page's map has no entry for", () => { + // A schema that carries a code outside the target map at all — the + // situation this reader exists to catch loudly rather than render nothing. + const rogue = z.string().refine(() => false, { error: "not_a_real_code" }); + const result = rogue.safeParse("x"); + if (result.success) throw new Error("expected rejection"); + expect(() => readValidationCode(result.error, NEW_OPERATION_ERRORS)).toThrow( + /unmapped code/, + ); + }); +}); + +describe("parseYmd / battleReportUrlProblem (still the single definitions, only reached via schemas below)", () => { + it("parseYmd rejects a rollover date rather than normalizing it", () => { + expect(parseYmd("2026-02-30")).toBeNull(); + }); + + it("battleReportUrlProblem accepts a plain https link", () => { + expect(battleReportUrlProblem("https://zkillboard.com/kill/1/")).toBeNull(); + }); +}); + +describe("nameFieldSchema", () => { + it("rejects an empty name with name_required", () => { + const result = nameFieldSchema.safeParse(""); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("name_required"); + }); + + it("accepts a non-empty name", () => { + const result = nameFieldSchema.safeParse("Operation Foo"); + expect(result).toEqual({ success: true, data: "Operation Foo" }); + }); +}); + +describe("occurredAtFieldSchema", () => { + it("rejects a malformed date with date_invalid", () => { + const result = occurredAtFieldSchema.safeParse("not-a-date"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("date_invalid"); + }); + + it("rejects a rollover date (Feb 30) with date_invalid, not a normalized date", () => { + const result = occurredAtFieldSchema.safeParse("2026-02-30"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("date_invalid"); + }); + + it("parses a real calendar date to a Date with no future check on its own", () => { + // This bare schema is what setOccurredAtAction uses directly — the + // detail page's date editor has no future check, a pre-existing, + // out-of-scope gap (see setOccurredAtAction's own comment) — so a date far + // in the future must still parse successfully here. + const result = occurredAtFieldSchema.safeParse("2099-01-01"); + expect(result.success).toBe(true); + }); +}); + +describe("buildCreateOperationSchema — date_invalid and date_future stay separately reachable", () => { + it("rejects a malformed date with date_invalid", () => { + const result = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "not-a-date", + battleReportUrl: "", + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(readValidationCode(result.error, NEW_OPERATION_ERRORS)).toBe("date_invalid"); + } + }); + + it("rejects a real, well-formed date in the future with date_future, not date_invalid", () => { + const result = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "2026-08-11", + battleReportUrl: "", + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(readValidationCode(result.error, NEW_OPERATION_ERRORS)).toBe("date_future"); + } + }); + + it("accepts today's own date (the boundary itself is not future)", () => { + const result = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "2026-08-10", + battleReportUrl: "", + }); + expect(result.success).toBe(true); + }); + + it("keeps declaration order: a blank name with a bad URL still lands on name_required", () => { + const result = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "", + occurredAt: "2026-08-01", + battleReportUrl: "javascript:alert(1)", + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(readValidationCode(result.error, NEW_OPERATION_ERRORS)).toBe( + "name_required", + ); + } + }); + + it("battleReportUrl: an unparseable value is url_invalid, a bad scheme is url_scheme", () => { + const invalid = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "2026-08-01", + battleReportUrl: "zkillboard.com/related/1", + }); + expect(invalid.success).toBe(false); + if (!invalid.success) { + expect(readValidationCode(invalid.error, NEW_OPERATION_ERRORS)).toBe("url_invalid"); + } + + const scheme = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "2026-08-01", + battleReportUrl: "javascript:alert(1)", + }); + expect(scheme.success).toBe(false); + if (!scheme.success) { + expect(readValidationCode(scheme.error, NEW_OPERATION_ERRORS)).toBe("url_scheme"); + } + }); + + it("an empty battle report URL is optional and does not reject", () => { + const result = buildCreateOperationSchema(TODAY_UTC).safeParse({ + name: "Op", + occurredAt: "2026-08-01", + battleReportUrl: "", + }); + expect(result.success).toBe(true); + }); +}); + +describe("battleReportUrlFieldSchema", () => { + it("accepts an empty string as 'nothing submitted'", () => { + const result = battleReportUrlFieldSchema.safeParse(""); + expect(result).toEqual({ success: true, data: null }); + }); + + it("rejects a bare hostname with url_invalid", () => { + const result = battleReportUrlFieldSchema.safeParse("zkillboard.com"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("url_invalid"); + }); + + it("rejects a non-http(s) scheme with url_scheme", () => { + const result = battleReportUrlFieldSchema.safeParse("javascript:alert(1)"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("url_scheme"); + }); +}); + +describe("flatPoolFieldSchema — note_required before total_invalid", () => { + it("rejects a blank note first even when totalValue is also bad", () => { + const result = flatPoolFieldSchema.safeParse({ notes: "", totalValue: "abc" }); + expect(result.success).toBe(false); + if (!result.success) { + expect(readValidationCode(result.error, OPERATION_ERRORS)).toBe("note_required"); + } + }); + + it("rejects a malformed total once the note is present", () => { + const result = flatPoolFieldSchema.safeParse({ + notes: "from loot log", + totalValue: "abc", + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(readValidationCode(result.error, OPERATION_ERRORS)).toBe("total_invalid"); + } + }); + + it("accepts a plain two-decimal total", () => { + const result = flatPoolFieldSchema.safeParse({ + notes: "note", + totalValue: "12345.67", + }); + expect(result).toEqual({ + success: true, + data: { notes: "note", totalValue: "12345.67" }, + }); + }); +}); + +describe("unitPriceFieldSchema", () => { + it("rejects more than two decimal places with price_invalid", () => { + const result = unitPriceFieldSchema.safeParse("1.234"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("price_invalid"); + }); + + it("accepts a plain two-decimal price", () => { + expect(unitPriceFieldSchema.safeParse("12.34")).toEqual({ + success: true, + data: "12.34", + }); + }); +}); + +describe("participantNameFieldSchema", () => { + it("rejects a blank name with participant_name_required", () => { + const result = participantNameFieldSchema.safeParse(""); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues[0]?.message).toBe("participant_name_required"); + } + }); +}); + +describe("sharesFieldSchema — order: shares_required, shares_invalid, shares_positive, shares_range", () => { + it("rejects a blank value with shares_required", () => { + const result = sharesFieldSchema.safeParse(""); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("shares_required"); + }); + + it("rejects a non-numeric value with shares_invalid, never calling iskToCents", () => { + // iskToCents throws on anything its own regex rejects; if the transform + // ran anyway on "abc" this would throw an uncaught error instead of + // producing a clean zod rejection. + const result = sharesFieldSchema.safeParse("abc"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("shares_invalid"); + }); + + it("rejects zero with shares_positive", () => { + const result = sharesFieldSchema.safeParse("0"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("shares_positive"); + }); + + it("rejects a negative value with shares_positive", () => { + const result = sharesFieldSchema.safeParse("-5"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("shares_positive"); + }); + + it("rejects a value past the max with shares_range", () => { + const result = sharesFieldSchema.safeParse("10000"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("shares_range"); + }); + + it("accepts an ordinary positive value within range", () => { + expect(sharesFieldSchema.safeParse("1.5")).toEqual({ success: true, data: "1.5" }); + }); +}); + +describe("corpSharePctFieldSchema — share_format before share_range", () => { + it("rejects a malformed percentage with share_format", () => { + const result = corpSharePctFieldSchema.safeParse("12,5"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("share_format"); + }); + + it("rejects a well-formed percentage over 100 with share_range", () => { + const result = corpSharePctFieldSchema.safeParse("120"); + expect(result.success).toBe(false); + if (!result.success) expect(result.error.issues[0]?.message).toBe("share_range"); + }); + + it("accepts a plain in-range percentage", () => { + expect(corpSharePctFieldSchema.safeParse("12.5")).toEqual({ + success: true, + data: "12.5", + }); + }); +}); + +describe("every code emitted by these schemas is mapped in the destination page's own error map", () => { + it("NEW_OPERATION_ERRORS covers every code buildCreateOperationSchema can emit", () => { + const codes = [ + "name_required", + "date_invalid", + "date_future", + "url_invalid", + "url_scheme", + ]; + for (const code of codes) { + expect(Object.hasOwn(NEW_OPERATION_ERRORS, code)).toBe(true); + } + }); + + it("OPERATION_ERRORS covers every code the field schemas can emit", () => { + const codes = [ + "name_required", + "date_invalid", + "url_invalid", + "url_scheme", + "note_required", + "total_invalid", + "price_invalid", + "participant_name_required", + "shares_required", + "shares_invalid", + "shares_positive", + "shares_range", + "share_format", + "share_range", + ]; + for (const code of codes) { + expect(Object.hasOwn(OPERATION_ERRORS, code)).toBe(true); + } + }); +}); From af1521b1724df77523548548e2b303fb7452459f Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 17:33:16 -0400 Subject: [PATCH 2/2] refactor(actions): read each rejection code off the schema that emitted it Unifies parseId/parseCharacterId onto the convention assertValid already uses: throw the code taken off the rejected issue rather than restating it at the call site, so each code has one spelling. Spells the code on every step a schema can reject through, since `.positive({ error })` attaches it to `positive` alone and a non-integer would otherwise surface zod's own wording. Comment corrections throughout. --- src/app/account/actions.ts | 17 ++- src/app/admin/access-lists/actions.ts | 19 ++- src/app/admin/accounts/actions.ts | 109 +++++++++++------- src/app/admin/sync/actions.ts | 9 +- tests/account-actions-validation.test.ts | 7 +- .../admin-accounts-actions-validation.test.ts | 7 +- tests/payouts-validation.test.ts | 97 +++++++++++----- 7 files changed, 173 insertions(+), 92 deletions(-) diff --git a/src/app/account/actions.ts b/src/app/account/actions.ts index ab274d75..55c42178 100644 --- a/src/app/account/actions.ts +++ b/src/app/account/actions.ts @@ -24,12 +24,23 @@ import { getSessionAccount } from "@/services/session"; * `requireAccount()`, not before — same order those two siblings use — so a * caller who is not signed in gets `requireAccount`'s own redirect regardless * of what they sent, rather than a thrown validation error telling an - * unauthenticated request whether its argument even had the right shape. */ -const characterIdSchema = z.number().int().positive({ error: "invalid_character_id" }); + * unauthenticated request whether its argument even had the right shape. + * + * The code is spelled on every step rather than only the last: + * `parseCharacterId` throws what it reads off the rejected issue, and + * `z.number().int().positive({ error })` attaches the code to `positive` + * alone — so a non-integer would otherwise surface zod's own wording + * ("Invalid input: expected int, received number") as the thrown message. */ +const characterIdSchema = z + .number({ error: "invalid_character_id" }) + .int({ error: "invalid_character_id" }) + .positive({ error: "invalid_character_id" }); function parseCharacterId(value: number): number { const parsed = characterIdSchema.safeParse(value); - if (!parsed.success) throw new Error("invalid_character_id"); + if (!parsed.success) { + throw new Error(parsed.error.issues[0]?.message ?? "invalid_character_id"); + } return parsed.data; } diff --git a/src/app/admin/access-lists/actions.ts b/src/app/admin/access-lists/actions.ts index 185201a4..fdc9c9da 100644 --- a/src/app/admin/access-lists/actions.ts +++ b/src/app/admin/access-lists/actions.ts @@ -25,14 +25,27 @@ import { type ActionOutcome } from "@/app/_components/confirm-group"; * `null`; the input type is `FormDataEntryValue | null`, never bare `string`. */ const idSchema = z.preprocess( (value) => Number(value), - z.number().refine((n) => Number.isSafeInteger(n) && n > 0, { error: "invalid_id" }), + // The `error` on the type gate is not redundant with the refine's, and both + // are read: `parseId` below throws the code it takes off the rejected issue, + // so a path left without one would surface zod's own generated wording as + // the thrown message. `Number()` runs first and maps a non-numeric spelling + // to `NaN`, which `z.number()` rejects at the gate — the refine never runs — + // so the most likely bad input ("12abc") rejects through the gate while a + // well-formed-but-out-of-range one ("-1") rejects through the refine. Both + // spell it the same way, so the caller gets `invalid_id` either way. + z + .number({ error: "invalid_id" }) + .refine((n) => Number.isSafeInteger(n) && n > 0, { error: "invalid_id" }), ); /** Unreachable from the rendered page, so a bad value throws rather than - * earning notice copy — the same posture `syncJobAction` takes on `jobType`. */ + * earning notice copy — the same posture `syncJobAction` takes on `jobType`. + * The code comes off the rejected issue rather than being restated here, so + * `invalid_id` has one spelling (the schema's) rather than two that can + * drift; same shape as `admin/accounts/actions.ts`'s `assertValid`. */ function parseId(value: FormDataEntryValue | null): number { const parsed = idSchema.safeParse(value); - if (!parsed.success) throw new Error("invalid_id"); + if (!parsed.success) throw new Error(parsed.error.issues[0]?.message ?? "invalid_id"); return parsed.data; } diff --git a/src/app/admin/accounts/actions.ts b/src/app/admin/accounts/actions.ts index 079f66f8..4a4bf916 100644 --- a/src/app/admin/accounts/actions.ts +++ b/src/app/admin/accounts/actions.ts @@ -46,22 +46,41 @@ import { * redirect regardless of what they sent, rather than a thrown validation * error handing an unauthenticated request a way to distinguish a malformed * argument from a well-formed one. + * + * Each schema spells its code on EVERY path it can reject through, not just + * the last one: `assertValid` throws the code it reads back off the issue, so + * a path left carrying zod's own generated wording ("Invalid input: expected + * number, received string") would surface that wording as the thrown message + * instead of this file's code. `z.number().int().positive({ error })` attaches + * the code to `positive` alone — hence the repetition on the two schemas that + * reject through more than one step. */ const accountIdSchema = z.uuid({ error: "invalid_account_id" }); -const identitySchema = z.string().min(1, { error: "invalid_identity" }); +const identitySchema = z + .string({ error: "invalid_identity" }) + .min(1, { error: "invalid_identity" }); const listSearchSchema = z.string({ error: "invalid_list_search" }); const tierSchema = z.enum(["member", "associate", "alumni"], { error: "invalid_tier" }); const approveTierSchema = z.enum(["alumni", "associate"], { error: "invalid_tier" }); const statusSchema = z.enum(["active", "cryo"], { error: "invalid_status" }); -const characterIdSchema = z.number().int().positive({ error: "invalid_character_id" }); +const characterIdSchema = z + .number({ error: "invalid_character_id" }) + .int({ error: "invalid_character_id" }) + .positive({ error: "invalid_character_id" }); const noteSchema = z.string({ error: "invalid_note" }); -/** Parses `value` against `schema`, throwing `Error(message)` — not a - * `ZodError` — on rejection, matching every other unreachable-input throw in - * this file's siblings. */ -function assertValid(schema: z.ZodType, value: unknown, message: string): T { +/** Parses `value` against `schema`, throwing `Error(code)` — not a `ZodError` + * — on rejection, matching every other unreachable-input throw in this file's + * siblings. The code comes from the rejected issue's own message, which is + * what each schema's `error:` option puts there, so the code has exactly one + * spelling per schema rather than one on the schema and another repeated at + * each call site. (`error:` is zod v4's spelling of v3's `message:`, which is + * what the older schemas in `src/config.ts` and `src/lib/wanderer/client.ts` + * still use; new schemas here take the v4 form.) */ +function assertValid(schema: z.ZodType, value: unknown): T { const result = schema.safeParse(value); - if (!result.success) throw new Error(message); + if (!result.success) + throw new Error(result.error.issues[0]?.message ?? "invalid_input"); return result.data; } @@ -209,10 +228,10 @@ export async function setTierAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - tier = assertValid(tierSchema, tier, "invalid_tier"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + tier = assertValid(tierSchema, tier); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => setTierManual(tx, actor, accountId, tier), ); @@ -245,10 +264,10 @@ export async function approveAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - tier = assertValid(approveTierSchema, tier, "invalid_tier"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + tier = assertValid(approveTierSchema, tier); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => approveAccount(tx, actor, accountId, tier), ); @@ -267,9 +286,9 @@ export async function returnToAutoAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => returnTierToAuto(tx, actor, accountId), ); @@ -295,10 +314,10 @@ export async function setStatusAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - status = assertValid(statusSchema, status, "invalid_status"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + status = assertValid(statusSchema, status); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => setAccountStatus(tx, actor, accountId, status), ); @@ -337,15 +356,15 @@ export async function saveNoteAction( formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); // FormData.get() is string | File | null. Coercing a File or a missing field // to "" would silently CLEAR the note (setStatusNote maps "" to null) and // write a status.note_changed audit entry for an edit nobody requested. // Reject the malformed request instead; "" itself stays valid — that is how // the form asks for the note to be cleared. This can only happen if the // form itself is tampered with, so it stays a throw rather than a race. - const raw = assertValid(noteSchema, formData.get("note"), "invalid_note"); + const raw = assertValid(noteSchema, formData.get("note")); const result = await getDb().transaction((tx) => setStatusNote(tx, actor, accountId, raw), @@ -367,9 +386,9 @@ export async function syncAccountAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); await getDb().transaction(async (tx) => { await logAudit(tx, { actor, action: "sync.requested", target: accountId }); await enqueueSync(tx, { kind: "account", accountId }); @@ -384,9 +403,9 @@ export async function promoteAdminAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => promoteAdmin(tx, actor, accountId)); if (!result.ok) { // promoteAdmin predates AdminMutationResult and returns `error` as @@ -409,9 +428,9 @@ export async function demoteAdminAction( identity: string, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => demoteAdmin(tx, actor, accountId)); if (!result.ok && result.error === "last_admin") { // Surface the service's protection instead of a 500 (carry-over). @@ -452,9 +471,9 @@ export async function unlinkDiscordAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); - identity = assertValid(identitySchema, identity, "invalid_identity"); + accountId = assertValid(accountIdSchema, accountId); + listSearch = assertValid(listSearchSchema, listSearch); + identity = assertValid(identitySchema, identity); const result = await getDb().transaction((tx) => unlinkDiscord(tx, actor, accountId, "admin"), ); @@ -499,10 +518,12 @@ export async function unlinkDiscordAction( /** * Promote a character to main from the drawer's crew table. * - * `characterId` is bound at render time from the row's own crew list, so it is - * not user input in the way a form field would be — but the service still - * verifies the character belongs to the account, which is what makes a forged - * request land on `not_found` rather than on someone else's account. + * `characterId` is bound at render time from the row's own crew list, but a + * bound argument round-trips through the client exactly as a form field does, + * so it is parsed here like every other argument (see this file's docblock). + * The service independently verifies the character belongs to the account, + * which is what makes a forged request that clears the parse land on + * `not_found` rather than on someone else's account. * * Returns an `ActionOutcome` rather than redirecting: the control lives inside * the drawer, and a redirect would collapse it. @@ -515,9 +536,9 @@ export async function setMainAction( _formData: FormData, ): Promise { const { accountId: actor } = await requireAdminAction(); - accountId = assertValid(accountIdSchema, accountId, "invalid_account_id"); - characterId = assertValid(characterIdSchema, characterId, "invalid_character_id"); - listSearch = assertValid(listSearchSchema, listSearch, "invalid_list_search"); + accountId = assertValid(accountIdSchema, accountId); + characterId = assertValid(characterIdSchema, characterId); + listSearch = assertValid(listSearchSchema, listSearch); const result = await getDb().transaction((tx) => setMainCharacterAsAdmin(tx, actor, accountId, characterId), ); diff --git a/src/app/admin/sync/actions.ts b/src/app/admin/sync/actions.ts index 36e0f0bc..e069784b 100644 --- a/src/app/admin/sync/actions.ts +++ b/src/app/admin/sync/actions.ts @@ -14,10 +14,11 @@ import { elapsedShort } from "@/app/_components/format-ago"; import { type ActionOutcome } from "@/app/_components/confirm-group"; import { queuedNotice } from "./view"; -/** Derives its enum from `isJobType` (`@/core/schedules`) — the one place a - * job's schedule is written down — rather than duplicating that job-type - * list here. `z.custom` is the v4 way to lift an existing type guard into a - * schema without re-deriving the literals it checks. */ +/** Defers to `isJobType` (`@/core/schedules`) — the one place a job's schedule + * is written down — rather than restating that job-type list here as literals + * that could drift from it. `z.custom` lifts an existing type guard into a + * schema; it derives no literals of its own, so there is nothing here to keep + * in step. */ const jobTypeSchema = z.custom(isJobType, { error: "invalid_job_type" }); export async function syncAllAction(): Promise { diff --git a/tests/account-actions-validation.test.ts b/tests/account-actions-validation.test.ts index 4f31cace..7a73a0ec 100644 --- a/tests/account-actions-validation.test.ts +++ b/tests/account-actions-validation.test.ts @@ -6,9 +6,10 @@ import { describe, expect, it, vi } from "vitest"; // needs requireAccount's own dependencies (next/headers's cookies and // getSessionAccount) mocked to resolve, the same way // admin-sync-actions-validation.test.ts mocks admin-guard for the equivalent -// reason. Nothing below this mock touches a real database: the schema -// rejection throws before setMainCharacter/unlinkCharacter or getDb are ever -// called. +// reason. Nothing below this mock reaches a real database: `getDb()` is +// evaluated (it is the argument to the mocked `getSessionAccount`), but it +// only builds a client — no query is issued, and the schema rejection throws +// before setMainCharacter/unlinkCharacter are ever called. vi.mock("next/headers", () => ({ cookies: async () => ({ get: () => ({ value: "session-id" }) }), })); diff --git a/tests/admin-accounts-actions-validation.test.ts b/tests/admin-accounts-actions-validation.test.ts index 57e50bea..1b3ecaa3 100644 --- a/tests/admin-accounts-actions-validation.test.ts +++ b/tests/admin-accounts-actions-validation.test.ts @@ -1,9 +1,10 @@ import { describe, expect, it, vi } from "vitest"; // Every action below now validates its bound arguments (via `assertValid`) -// AFTER calling `requireAdminAction()`, not before — see this file's own -// docblock. That means the guard has to resolve for any of these throws to be -// reached at all, mocked rather than exercised for real, the same pattern +// AFTER calling `requireAdminAction()`, not before — see the docblock at the +// top of src/app/admin/accounts/actions.ts. That means the guard has to +// resolve for any of these throws to be reached at all, mocked rather than +// exercised for real, the same pattern // admin-sync-actions-validation.test.ts and // admin-access-lists-actions-validation.test.ts already use. Nothing below // this mock touches a real database: every rejection throws before diff --git a/tests/payouts-validation.test.ts b/tests/payouts-validation.test.ts index 8d2b089a..8df27bf5 100644 --- a/tests/payouts-validation.test.ts +++ b/tests/payouts-validation.test.ts @@ -39,7 +39,7 @@ describe("readValidationCode", () => { }); }); -describe("parseYmd / battleReportUrlProblem (still the single definitions, only reached via schemas below)", () => { +describe("parseYmd / battleReportUrlProblem (still the single definitions the schemas below build on)", () => { it("parseYmd rejects a rollover date rather than normalizing it", () => { expect(parseYmd("2026-02-30")).toBeNull(); }); @@ -301,39 +301,72 @@ describe("corpSharePctFieldSchema — share_format before share_range", () => { }); }); +/** + * Reads the code a schema actually emits rather than restating it, so that a + * schema whose `error:` string is typo'd fails the coverage assertions below + * instead of passing a hand-copied list against a map it no longer matches. + */ +function emittedCode(schema: z.ZodType, input: unknown): string { + const result = schema.safeParse(input); + if (result.success) throw new Error(`expected ${JSON.stringify(input)} to be rejected`); + const code = result.error.issues[0]?.message; + if (code === undefined) throw new Error("rejection carried no issue"); + return code; +} + +const CREATE_REJECTIONS: ReadonlyArray<[string, unknown]> = [ + ["blank name", { name: "", occurredAt: "2026-01-01", battleReportUrl: "" }], + ["unparseable date", { name: "Op", occurredAt: "2026-02-30", battleReportUrl: "" }], + ["future date", { name: "Op", occurredAt: "2099-01-01", battleReportUrl: "" }], + [ + "unparseable url", + { name: "Op", occurredAt: "2026-01-01", battleReportUrl: "zkillboard.com" }, + ], + [ + "non-http scheme", + { name: "Op", occurredAt: "2026-01-01", battleReportUrl: "javascript:alert(1)" }, + ], +]; + +const FIELD_REJECTIONS: ReadonlyArray<[string, z.ZodType, unknown]> = [ + ["nameFieldSchema", nameFieldSchema, ""], + ["occurredAtFieldSchema", occurredAtFieldSchema, "2026-02-30"], + [ + "battleReportUrlFieldSchema (unparseable)", + battleReportUrlFieldSchema, + "zkillboard.com", + ], + [ + "battleReportUrlFieldSchema (scheme)", + battleReportUrlFieldSchema, + "javascript:alert(1)", + ], + ["flatPoolFieldSchema (note)", flatPoolFieldSchema, { notes: "", totalValue: "1" }], + ["flatPoolFieldSchema (total)", flatPoolFieldSchema, { notes: "n", totalValue: "x" }], + ["unitPriceFieldSchema", unitPriceFieldSchema, "x"], + ["participantNameFieldSchema", participantNameFieldSchema, ""], + ["sharesFieldSchema (blank)", sharesFieldSchema, ""], + ["sharesFieldSchema (format)", sharesFieldSchema, "abc"], + ["sharesFieldSchema (zero)", sharesFieldSchema, "0"], + ["sharesFieldSchema (over max)", sharesFieldSchema, "10000"], + ["corpSharePctFieldSchema (format)", corpSharePctFieldSchema, "x"], + ["corpSharePctFieldSchema (range)", corpSharePctFieldSchema, "120"], +]; + describe("every code emitted by these schemas is mapped in the destination page's own error map", () => { - it("NEW_OPERATION_ERRORS covers every code buildCreateOperationSchema can emit", () => { - const codes = [ - "name_required", - "date_invalid", - "date_future", - "url_invalid", - "url_scheme", - ]; - for (const code of codes) { + it.each(CREATE_REJECTIONS)( + "NEW_OPERATION_ERRORS maps what buildCreateOperationSchema emits for %s", + (_label, input) => { + const code = emittedCode(buildCreateOperationSchema(TODAY_UTC), input); expect(Object.hasOwn(NEW_OPERATION_ERRORS, code)).toBe(true); - } - }); + }, + ); - it("OPERATION_ERRORS covers every code the field schemas can emit", () => { - const codes = [ - "name_required", - "date_invalid", - "url_invalid", - "url_scheme", - "note_required", - "total_invalid", - "price_invalid", - "participant_name_required", - "shares_required", - "shares_invalid", - "shares_positive", - "shares_range", - "share_format", - "share_range", - ]; - for (const code of codes) { + it.each(FIELD_REJECTIONS)( + "OPERATION_ERRORS maps what %s emits", + (_label, schema, input) => { + const code = emittedCode(schema, input); expect(Object.hasOwn(OPERATION_ERRORS, code)).toBe(true); - } - }); + }, + ); });