From d6bc858899c9f18c33eb3347633029e4236b9c0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 05:01:49 +0200 Subject: [PATCH] fix: refuse an unrecognised request field where the caller can act on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `z.object()` without `.strict()` strips an unknown key and parses clean, so a caller that misspells a field name gets a 200 and loses the value with nothing to tell it. The worst of them was the Coach question-dismiss endpoint, where an omitted `question` means "dismiss all": a typo in that key wiped every pending question and answered as though one had been dismissed. Twenty-four inline schemas across twenty-two endpoints are strict now — the settings toggles, the restore and bulk-delete endpoints, message feedback, the admin user update, the onboarding acknowledgements, the provider chain, the chart overlay preferences, the medication extractor and the two client-side reporters. The strip stays where it carries weight. Sync ingest, device registration and the layout PUTs are built out of fields that shipped as additive and optional, on the promise that a server which does not know one ignores it; an app running ahead of the install it talks to is routine here rather than exceptional, and refusing would turn a dropped field into a refused batch — a year of history lost to one row, a dashboard that will not save, a phone that quietly stops registering for push. A read filter discards nothing when a stray query parameter rides along, the OAuth endpoints are required by RFC 6749 and RFC 7591 to ignore parameters they do not recognise, and an import file comes from another version or another tool by definition. Each is registered with its reason in a new inventory test, so a schema cannot join them without someone making the call. That test parses the tree rather than grepping it: fourteen of these schemas are written `z\n .object({…})` and a `z.object(` sweep never saw them. `sanitiseZodIssues` renders `unrecognized_keys` instead of passing it through. Zod leaves `path` empty and puts the names in `issue.keys`, so the sanitised issue named nothing at all and the caller was told only that its body had failed. The names now ride a bounded `keys` field and the message is rebuilt from them, capped at ten keys of 64 characters and restricted to identifier characters, so a caller-supplied key cannot echo back verbatim through the error path or into the audit ledger. The names survive `stripValuesFromMessage` because a field name is structure, not user-typed content. --- CHANGELOG.md | 27 ++ ...inline-schema-strictness-inventory.test.ts | 304 ++++++++++++++++++ src/app/api/admin/users/[id]/route.ts | 38 +-- src/app/api/auth/check-user/route.ts | 8 +- src/app/api/auth/me/devices/[id]/route.ts | 8 +- src/app/api/auth/me/diabetes/route.ts | 8 +- .../__tests__/unknown-key-strict.test.ts | 120 +++++++ src/app/api/auth/me/disable-coach/route.ts | 2 +- src/app/api/coach/about-me/questions/route.ts | 10 +- .../[id]/entries/restore/route.ts | 8 +- .../dashboard/chart-overlay-prefs/route.ts | 38 ++- .../chat/messages/[id]/feedback/route.ts | 10 +- src/app/api/insights/provider-chain/route.ts | 38 ++- src/app/api/internal/web-vitals/route.ts | 46 +-- src/app/api/labs/restore/route.ts | 8 +- src/app/api/measurements/bulk-delete/route.ts | 8 +- .../api/measurements/by-external-ids/route.ts | 14 +- src/app/api/measurements/restore/route.ts | 8 +- .../unknown-query-param-open.test.ts | 103 ++++++ src/app/api/medications/extract/route.ts | 32 +- src/app/api/monitoring/glitchtip/route.ts | 18 +- src/app/api/mood-entries/bulk-delete/route.ts | 8 +- src/app/api/mood-entries/restore/route.ts | 8 +- src/app/api/notifications/web-push/route.ts | 8 +- src/app/api/onboarding/disclaimer/route.ts | 14 +- src/app/api/onboarding/tour/route.ts | 1 + .../api-response-unrecognized-keys.test.ts | 180 +++++++++++ src/lib/api-response.ts | 93 +++++- 28 files changed, 1008 insertions(+), 160 deletions(-) create mode 100644 src/app/api/__tests__/inline-schema-strictness-inventory.test.ts create mode 100644 src/app/api/auth/me/disable-coach/__tests__/unknown-key-strict.test.ts create mode 100644 src/app/api/measurements/series/__tests__/unknown-query-param-open.test.ts create mode 100644 src/lib/__tests__/api-response-unrecognized-keys.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ab82d4fe4..8877d02f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,33 @@ ## [Unreleased] +### Fixed + +- **A misspelled field in a request is no longer accepted and thrown away.** + Send `disableCoachh` where the server expects `disableCoach` and the answer + used to be 200: the unrecognised key was dropped, whatever else the body + carried was saved, and the caller was told its request had been honoured. + Twenty-two endpoints now refuse a field they do not recognise, and the + 422 names the key that was wrong instead of only saying the body failed. The + settings toggles, the restore and bulk-delete endpoints, message feedback, + the admin user update, the diabetes and Coach switches, the onboarding + acknowledgements, the provider chain, the chart overlay preferences and the + two client-side reporters are the ones affected. A key name is bounded and + stripped of anything that is not a plain identifier before it is echoed, so + a hostile one cannot ride the error message back out. +- Sync, layout, import and the read filters deliberately keep accepting a field + they do not know, and now have a test saying so. The phone app ships optional + fields ahead of the servers that will eventually read them, and a self-hosted + install runs whichever release its operator last pulled, so a server quietly + ignoring a field it has never heard of is what keeps an updated app working + against an older install. Refusing there would turn one dropped field into a + refused batch: a year of history lost to a single row, a dashboard that will + not save, or a phone that silently stops registering for notifications. On a + read filter a stray query parameter discards nothing at all, the OAuth + endpoints are required by their specifications to ignore parameters they do + not recognise, and an import file comes from another version or another tool + by definition. + ### Internal - The column-reader sweep was wrong in both directions and had been for as long as it existed. It answers "does this column have a consumer" for every column in the schema, and it is the tool this project leans on to catch a schema change that ships its writer and forgets its reader. Its idea of a write was any `field:` key anywhere, so `where: { ticketHash }` counted as writing `ticketHash`: all twelve columns it reported were a lookup key or a cron's discovery predicate being filtered on, every one a false alarm. And because it only ever reported a column that HAD a write, a column with no write at all was the one thing it could not see. It now understands which Prisma argument a key sits in, follows a payload assembled into a variable before the call, credits raw SQL against the mapped column name, and reports a column no code touches at all as its own category. Twelve false alarms became nineteen findings that each hold up, four of them worth acting on. The matcher underneath is pinned by a test that was checked by breaking it three ways. diff --git a/src/app/api/__tests__/inline-schema-strictness-inventory.test.ts b/src/app/api/__tests__/inline-schema-strictness-inventory.test.ts new file mode 100644 index 000000000..034df0f81 --- /dev/null +++ b/src/app/api/__tests__/inline-schema-strictness-inventory.test.ts @@ -0,0 +1,304 @@ +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, relative, resolve } from "node:path"; + +import { describe, expect, it } from "vitest"; +import ts from "typescript"; + +/** + * Inline request-schema strictness inventory. + * + * A `z.object()` without `.strict()` STRIPS an unknown key and parses + * clean. A client that sends `measuredAtt` for `measuredAt` gets a 200 + * and loses the value; nobody is told. `.strict()` turns that into a 422 + * that names the key. + * + * It is not a change to make everywhere. `.strict()` flips accept-and- + * strip into reject, and this server is talked to by a native client on + * TestFlight and by self-hosted installs that lag the client by an + * arbitrary number of releases. On the surfaces where the client adds an + * optional field and older servers are expected to ignore it, the strip + * IS the compatibility mechanism, and taking it away turns a graceful + * degrade into a hard failure — a whole 500-entry sync batch refused + * rather than one field dropped. + * + * So each inline schema is a decision, and this test is where the + * decision is written down. Every `z.object(...)` under a route file is + * either `.strict()` or accounted for in {@link DELIBERATELY_OPEN} with + * the reason it stays open. A new route cannot land without making the + * call, and an existing open schema cannot quietly gain a sibling. + * + * Deliberately an inventory, not a policy: it does not claim strict is + * the right default. It claims the choice was made on purpose. + */ + +const API_ROOT = resolve(__dirname, ".."); + +/** + * Route files whose inline schemas stay open, with the count of open + * schemas in each and why. `count` is part of the assertion: adding an + * open schema to a listed file fails until the entry is updated, so the + * reason below always covers everything it claims to cover. + */ +const DELIBERATELY_OPEN: Record = { + // --------------------------------------------------------------- + // 1. Query schemas fed a hand-built object. The handler names each + // param it reads (`{ limit: searchParams.get("limit"), … }`), so an + // unknown query param never reaches the schema at all. `.strict()` + // here asserts nothing — it would be decoration on a parse that + // cannot see the key it is supposed to reject. + // --------------------------------------------------------------- + "admin/app-logs/route.ts": { count: 1, reason: "hand-built query object" }, + "admin/audit-log/route.ts": { count: 1, reason: "hand-built query object" }, + "admin/host-metrics/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "analytics/range/route.ts": { count: 1, reason: "hand-built query object" }, + "insights/biomarker-assessment/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "insights/coach-read/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "insights/derived/batch/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "insights/derived/route.ts": { count: 1, reason: "hand-built query object" }, + "insights/metric-status/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "insights/narrative/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "insights/pulse/intraday/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "medications/[id]/cadence/route.ts": { + count: 1, + reason: "hand-built query object", + }, + "sync/changes/route.ts": { count: 1, reason: "hand-built query object" }, + // Same shape, not a query: the range check the edit handler runs + // against the row's own type. Its input is `{ value: data.value }`, + // built one line above from data the handler already parsed — there is + // no caller-supplied key for a strict parse to reject. + "measurements/[id]/route.ts": { + count: 1, + reason: "hand-built parse input", + }, + + // --------------------------------------------------------------- + // 2. Read filters fed the whole `searchParams`. Here an unknown key + // DOES reach the schema, and strict would reject it — which is the + // wrong trade on a read. A stray param discards nothing: the filter + // the caller asked for is still applied and the response is still + // correct. Meanwhile these are hot paths (`measurements/series` is + // the iOS chart loader), a 422 paints an error banner over a chart + // that would otherwise have rendered, and cache-busters and + // campaign params ride on URLs for reasons no handler controls. + // --------------------------------------------------------------- + "measurements/series/route.ts": { + count: 1, + reason: "read filter over whole searchParams", + }, + "mood/linked-context/route.ts": { + count: 1, + reason: "read filter over whole searchParams", + }, + "sleep/night/route.ts": { + count: 1, + reason: "read filter over whole searchParams", + }, + "medications/[id]/dose-history/route.ts": { + count: 1, + reason: "read filter over whole searchParams", + }, + + // --------------------------------------------------------------- + // 3. Native-client ingest and device registration. This is where the + // strip is load-bearing. Every field on these bodies arrived as an + // additive optional one — `source`, `deviceType`, `valueMin` / + // `valueMax`, `syncTrigger`, `tagKeys`, `liveActivityPushToken` — + // and each shipped on the promise that a server which does not know + // it stays byte-for-byte unchanged. A client build reaches a server + // older than itself as a matter of routine here: the app updates + // through TestFlight and the server updates when its operator gets + // round to it. Strict would turn that skew into a refused batch, + // and the batch endpoints' own contract is the opposite — a bad row + // is `skipped` and the rest lands, so a phone draining a year of + // history does not lose the year to one row. `devices` is the worst + // case of the set: refuse the registration and the account silently + // stops receiving push entirely. + // --------------------------------------------------------------- + "measurements/batch/route.ts": { count: 2, reason: "native ingest skew" }, + "mood-entries/bulk/route.ts": { count: 4, reason: "native ingest skew" }, + "medications/intake/bulk/route.ts": { + count: 2, + reason: "native ingest skew", + }, + "integrations/healthkit/route.ts": { count: 2, reason: "native ingest skew" }, + "devices/route.ts": { count: 1, reason: "native ingest skew" }, + + // --------------------------------------------------------------- + // 4. Layout PUTs. Same skew, same mechanism: `comparisonBaseline`, + // `chartOverlayPrefs`, `tileVisible`, `sections` all landed as + // optional fields at an unchanged `version`, which makes + // "add an optional field, older servers ignore it" the documented + // compatibility contract for these blobs rather than an accident. + // `insights/layout` still accepts `version: 1` specifically because + // the live native client sends it. Strict would refuse a Save from + // any client ahead of the server and surface as a failed-to-save + // toast with no way for the user to act on it. + // --------------------------------------------------------------- + "dashboard/widgets/route.ts": { count: 3, reason: "layout additive-field" }, + "insights/layout/route.ts": { count: 3, reason: "layout additive-field" }, + "medications/layout/route.ts": { count: 1, reason: "layout additive-field" }, + + // --------------------------------------------------------------- + // 5. OAuth. RFC 6749 §3.1 requires the authorization endpoint to + // ignore unrecognised request parameters, and RFC 7591 §2 requires + // the registration endpoint to ignore unrecognised metadata — the + // register schema already carries an accepted-and-ignored block for + // exactly this. Strict would be a spec violation, not a hardening. + // --------------------------------------------------------------- + "mcp/oauth/authorize/route.ts": { count: 1, reason: "RFC requires ignore" }, + "mcp/oauth/register/route.ts": { count: 1, reason: "RFC requires ignore" }, + + // --------------------------------------------------------------- + // 6. Archive and file import. The payload is a document produced + // somewhere else — an export from another version of this app, or a + // CSV from a third-party tracker whose columns nobody here controls. + // Refusing an archive because it carries a column this version does + // not read would make a backup unrestorable on the version that has + // to restore it, which is the one job a backup has. + // --------------------------------------------------------------- + "import/route.ts": { count: 3, reason: "foreign archive" }, + "medications/[id]/intake/import/route.ts": { + count: 1, + reason: "foreign archive", + }, +}; + +function routeFiles(dir: string, acc: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + routeFiles(full, acc); + } else if (entry === "route.ts") { + acc.push(full); + } + } + return acc; +} + +/** `z.object(...)` and `z.strictObject(...)` calls. */ +function zodObjectKind(node: ts.Node): "object" | "strictObject" | null { + if (!ts.isCallExpression(node)) return null; + const callee = node.expression; + if (!ts.isPropertyAccessExpression(callee)) return null; + const name = callee.name.text; + if (name !== "object" && name !== "strictObject") return null; + if (!ts.isIdentifier(callee.expression) || callee.expression.text !== "z") { + return null; + } + return name; +} + +/** + * Whether a `.strict()` sits on the chain wrapping this call. Walking the + * chain rather than matching text is what makes the sweep trustworthy: + * a schema written as `z\n .object({…})\n .strict()` is invisible to a + * `z.object(` grep, and fourteen of the schemas in this tree are written + * exactly that way. + */ +function hasStrictOnChain(node: ts.Node): boolean { + let current: ts.Node = node; + while (current.parent) { + const parent = current.parent; + if ( + ts.isPropertyAccessExpression(parent) && + parent.expression === current + ) { + if (parent.name.text === "strict") return true; + current = parent; + continue; + } + if (ts.isCallExpression(parent) && parent.expression === current) { + current = parent; + continue; + } + return false; + } + return false; +} + +function openSchemaCount(file: string): number { + const source = ts.createSourceFile( + file, + readFileSync(file, "utf8"), + ts.ScriptTarget.Latest, + true, + ); + let open = 0; + const walk = (node: ts.Node): void => { + const kind = zodObjectKind(node); + if (kind === "object" && !hasStrictOnChain(node)) open += 1; + ts.forEachChild(node, walk); + }; + walk(source); + return open; +} + +const inventory = new Map(); +for (const file of routeFiles(API_ROOT)) { + const open = openSchemaCount(file); + if (open > 0) inventory.set(relative(API_ROOT, file), open); +} + +describe("inline request-schema strictness inventory", () => { + it("finds the schemas at all", () => { + // A sweep that matches nothing passes every other assertion in this + // file. Pin a floor so an accidentally-broken matcher fails loudly + // instead of reporting a clean tree. + expect(inventory.size).toBeGreaterThan(20); + }); + + it("accounts for every open schema with a written reason", () => { + const unaccounted: string[] = []; + for (const [file, open] of inventory) { + const entry = DELIBERATELY_OPEN[file]; + if (!entry) { + unaccounted.push( + `${file}: ${open} schema(s) without .strict() and no entry in DELIBERATELY_OPEN`, + ); + continue; + } + if (entry.count !== open) { + unaccounted.push( + `${file}: ${open} open schema(s) but DELIBERATELY_OPEN records ${entry.count}`, + ); + } + } + expect(unaccounted).toEqual([]); + }); + + it("carries no stale register entries", () => { + const stale = Object.keys(DELIBERATELY_OPEN).filter( + (file) => !inventory.has(file), + ); + expect(stale).toEqual([]); + }); + + it("gives every register entry a reason", () => { + const empty = Object.entries(DELIBERATELY_OPEN) + .filter(([, entry]) => entry.reason.trim().length < 10) + .map(([file]) => file); + expect(empty).toEqual([]); + }); +}); diff --git a/src/app/api/admin/users/[id]/route.ts b/src/app/api/admin/users/[id]/route.ts index ea105fcc4..4629f7135 100644 --- a/src/app/api/admin/users/[id]/route.ts +++ b/src/app/api/admin/users/[id]/route.ts @@ -12,24 +12,26 @@ import { annotate } from "@/lib/logging/context"; import { NextRequest } from "next/server"; import { z } from "zod/v4"; -const updateUserSchema = z.object({ - username: z.string().min(3).max(30).optional(), - email: z.string().email().nullable().optional(), - role: z.enum(["ADMIN", "USER"]).optional(), - // v1.23 — per-user "require a second factor" override. Effective requirement - // is the OR of this and the instance-wide AppSettings.mfaRequired policy. - mfaEnforced: z.boolean().optional(), - // Document vault — per-user storage-quota override in bytes. Null clears - // the override (the instance default applies again). Same bounds as the - // instance-wide setting. - documentQuotaBytes: z - .number() - .int() - .min(10_485_760) - .max(1_099_511_627_776) - .nullable() - .optional(), -}); +const updateUserSchema = z + .object({ + username: z.string().min(3).max(30).optional(), + email: z.string().email().nullable().optional(), + role: z.enum(["ADMIN", "USER"]).optional(), + // v1.23 — per-user "require a second factor" override. Effective requirement + // is the OR of this and the instance-wide AppSettings.mfaRequired policy. + mfaEnforced: z.boolean().optional(), + // Document vault — per-user storage-quota override in bytes. Null clears + // the override (the instance default applies again). Same bounds as the + // instance-wide setting. + documentQuotaBytes: z + .number() + .int() + .min(10_485_760) + .max(1_099_511_627_776) + .nullable() + .optional(), + }) + .strict(); export const PUT = apiHandler( async ( diff --git a/src/app/api/auth/check-user/route.ts b/src/app/api/auth/check-user/route.ts index 56694d8e0..cae5d1db0 100644 --- a/src/app/api/auth/check-user/route.ts +++ b/src/app/api/auth/check-user/route.ts @@ -48,9 +48,11 @@ import type { NextRequest } from "next/server"; export const dynamic = "force-dynamic"; -const bodySchema = z.object({ - identifier: z.string().trim().min(1).max(254), -}); +const bodySchema = z + .object({ + identifier: z.string().trim().min(1).max(254), + }) + .strict(); export type CheckUserBranch = "not_found" | "passkey_only" | "email_fallback" | "exists"; diff --git a/src/app/api/auth/me/devices/[id]/route.ts b/src/app/api/auth/me/devices/[id]/route.ts index 89f5ca688..fde54928f 100644 --- a/src/app/api/auth/me/devices/[id]/route.ts +++ b/src/app/api/auth/me/devices/[id]/route.ts @@ -45,9 +45,11 @@ interface RouteContext { * v1.7.0 — per-device medication-delivery override. NULL clears the * override (the device inherits the user-level roaming default). */ -const devicePatchSchema = z.object({ - medicationDelivery: deviceDeliverySchema, -}); +const devicePatchSchema = z + .object({ + medicationDelivery: deviceDeliverySchema, + }) + .strict(); export const PATCH = apiHandler( async (request: NextRequest, context: RouteContext) => { diff --git a/src/app/api/auth/me/diabetes/route.ts b/src/app/api/auth/me/diabetes/route.ts index 2b3e48e15..dcb42f992 100644 --- a/src/app/api/auth/me/diabetes/route.ts +++ b/src/app/api/auth/me/diabetes/route.ts @@ -32,9 +32,11 @@ import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; -const patchBodySchema = z.object({ - hasDiabetes: z.boolean(), -}); +const patchBodySchema = z + .object({ + hasDiabetes: z.boolean(), + }) + .strict(); export const dynamic = "force-dynamic"; diff --git a/src/app/api/auth/me/disable-coach/__tests__/unknown-key-strict.test.ts b/src/app/api/auth/me/disable-coach/__tests__/unknown-key-strict.test.ts new file mode 100644 index 000000000..037b72c43 --- /dev/null +++ b/src/app/api/auth/me/disable-coach/__tests__/unknown-key-strict.test.ts @@ -0,0 +1,120 @@ +/** + * The strict half, end to end on a real route. + * + * `patchBodySchema` is `.strict()`, so a PATCH carrying a key the route + * does not declare is refused rather than trimmed down to the keys it + * does. Before that, `{ disableCoachh: true }` parsed clean as `{}` — + * except `disableCoach` is required, so this particular body already + * 422'd on the missing field. The one worth pinning is the body that + * used to succeed while doing something other than what it said: + * `{ disableCoach: true, disableCoachh: false }`. + * + * The second assertion is the user-visible half. A 422 that says only + * "Validation failed" leaves the caller staring at a body it believes is + * correct, so the envelope has to name the key it refused — and name it + * without reflecting a hostile one back verbatim. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { user: { findUnique: vi.fn(), update: vi.fn() } }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn().mockResolvedValue({ + allowed: true, + remaining: 59, + resetAt: Date.now() + 60_000, + }), + rateLimitHeaders: () => ({}), +})); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { PATCH } from "../route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "testuser", role: "USER" as const }, +}; + +function mkPatch(body: unknown): Request { + return new Request("http://localhost/api/auth/me/disable-coach", { + method: "PATCH", + body: JSON.stringify(body), + headers: { "Content-Type": "application/json" }, + }); +} + +const patch = PATCH as (r: Request) => Promise; + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.update).mockResolvedValue({ + disableCoach: true, + } as never); +}); + +describe("PATCH /api/auth/me/disable-coach — unknown keys", () => { + it("accepts the declared body", async () => { + const res = await patch(mkPatch({ disableCoach: true })); + expect(res.status).toBe(200); + expect(prisma.user.update).toHaveBeenCalledTimes(1); + }); + + it("refuses a body that carries an undeclared key alongside a valid one", async () => { + const res = await patch( + mkPatch({ disableCoach: true, disableCoachh: false }), + ); + expect(res.status).toBe(422); + // Nothing was written. Before `.strict()` this wrote `disableCoach: + // true` and answered 200, so a caller that meant the second field + // was told its request had been honoured. + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it("names the offending key in the 422", async () => { + const res = await patch( + mkPatch({ disableCoach: true, disableCoachh: false }), + ); + const body = (await res.json()) as { + error: string; + details?: { + issues: Array<{ code: string; message: string; keys?: string[] }>; + }; + }; + const issue = body.details?.issues.find( + (i) => i.code === "unrecognized_keys", + ); + expect(issue).toBeDefined(); + expect(issue?.keys).toEqual(["disableCoachh"]); + expect(issue?.message).toContain("disableCoachh"); + }); + + it("does not reflect a hostile key name back verbatim", async () => { + const res = await patch( + mkPatch({ disableCoach: true, "": 1 }), + ); + expect(res.status).toBe(422); + const raw = await res.text(); + expect(raw).not.toContain("