diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index b852045d7..9b509f22b 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -3099,13 +3099,16 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": &a14 - description: "Too many email-address changes. The address change shares one per-account bucket across both profile - routes — `profile-email:`, 10 requests per 3600 seconds — keyed on the ACTING account, so an - account cannot collect a fresh allowance by probing a different address. Nothing was written. + description: "Too many email-address changes, and the request asked for nothing else. The address change shares one + per-account bucket across both profile routes — `profile-email:`, 10 requests per 3600 seconds — + keyed on the ACTING account, so an account cannot collect a fresh allowance by probing a different address. `meta.errorCode` is `profile.update.emailRateLimited`. Only a request whose `email` actually differs from the address on file is counted: re-saving a form that carries the unchanged address is free, and so is any - update that does not touch `email` at all. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` - and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing." + update that does not touch `email` at all. A request that carries other fields beside the refused address + does NOT get this response — it answers 200, saves those fields, and names `email` in `rejectedFields` with + code `rate_limited`, so one spent budget cannot block an unrelated profile edit for an hour. The + `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that + bucket; back off on `Retry-After` rather than guessing." headers: *a8 content: application/json: @@ -37916,9 +37919,11 @@ components: description: Whether an insurance number is stored. The number itself is encrypted at rest and is never returned — only its presence. rejectedFields: - description: "Present only on a PARTIAL success: the fields that failed validation and were skipped while the rest of - the patch was written. A 200 carrying this key means the save was incomplete — surface it, do not treat the - response as a clean save." + description: "Present only on a PARTIAL success: the fields that were skipped while the rest of the patch was written. A + 200 carrying this key means the save was incomplete — surface it, do not treat the response as a clean save. + `code` is the validator code for a field that failed validation, or `rate_limited` for an email-address + change that has spent the account's hourly budget; that one is not a bad value and the same address will be + accepted once the window rolls over." type: array items: type: object @@ -46812,6 +46817,28 @@ components: hasInsuranceNumber: type: boolean description: Whether a KVNR is on file (the value itself is not echoed). + rejectedFields: + description: "Present only on a PARTIAL success: the fields that were skipped while the rest of the patch was written. A + 200 carrying this key means the save was incomplete — surface it, do not treat the response as a clean save. + `code` is the validator code for a field that failed validation, or `rate_limited` for an email-address + change that has spent the account's hourly budget; that one is not a bad value and the same address will be + accepted once the window rolls over. The handler has always been able to answer with this key on this path; + it was published only on PUT /api/auth/profile." + type: array + items: + type: object + properties: + path: + type: string + code: + type: string + message: + type: string + required: + - path + - code + - message + additionalProperties: false required: - username - email diff --git a/messages/de.json b/messages/de.json index ec56cb656..2da2614a2 100644 --- a/messages/de.json +++ b/messages/de.json @@ -5509,6 +5509,7 @@ "invalidValue": "Das ist keine der Antworten, die dieses Feld annimmt.", "invalidFormat": "Das ist nicht in einer Form geschrieben, die dieses Feld lesen kann.", "invalidType": "Der Wert kam nicht in einer Form an, die dieses Feld speichern kann.", + "rateLimited": "Zu oft geändert. Warte kurz und versuche es dann erneut.", "other": "Dieser Wert wurde nicht angenommen. Bitte prüfen und erneut versuchen." }, "avatar": { diff --git a/messages/en.json b/messages/en.json index 74b9370da..966b3d2d7 100644 --- a/messages/en.json +++ b/messages/en.json @@ -5509,6 +5509,7 @@ "invalidValue": "That is not one of the answers this field accepts.", "invalidFormat": "That is not written in a form this field can read.", "invalidType": "That value did not arrive in a form this field can store.", + "rateLimited": "Changed too often. Wait a little and try again.", "other": "This value was not accepted. Check it and try again." }, "avatar": { diff --git a/messages/es.json b/messages/es.json index 3bfb7f622..7f078e580 100644 --- a/messages/es.json +++ b/messages/es.json @@ -5509,6 +5509,7 @@ "invalidValue": "Esa no es una de las respuestas que acepta este campo.", "invalidFormat": "Eso no está escrito en un formato que este campo pueda leer.", "invalidType": "El valor no llegó en un formato que este campo pueda guardar.", + "rateLimited": "Se ha cambiado demasiadas veces. Espera un poco e inténtalo de nuevo.", "other": "No se aceptó este valor. Revísalo e inténtalo de nuevo." }, "avatar": { diff --git a/messages/fr.json b/messages/fr.json index 479150e26..f9ac58f8a 100644 --- a/messages/fr.json +++ b/messages/fr.json @@ -5509,6 +5509,7 @@ "invalidValue": "Ce n'est pas une des réponses acceptées par ce champ.", "invalidFormat": "Ce n'est pas écrit dans un format que ce champ peut lire.", "invalidType": "La valeur n'est pas arrivée dans un format que ce champ peut enregistrer.", + "rateLimited": "Modifié trop souvent. Patientez un peu et réessayez.", "other": "Cette valeur n'a pas été acceptée. Vérifiez-la et réessayez." }, "avatar": { diff --git a/messages/it.json b/messages/it.json index 17321d575..0d1618d85 100644 --- a/messages/it.json +++ b/messages/it.json @@ -5509,6 +5509,7 @@ "invalidValue": "Questa non è una delle risposte accettate da questo campo.", "invalidFormat": "Non è scritto in un formato che questo campo possa leggere.", "invalidType": "Il valore non è arrivato in un formato che questo campo possa salvare.", + "rateLimited": "Modificato troppe volte. Aspetta un momento e riprova.", "other": "Questo valore non è stato accettato. Controllalo e riprova." }, "avatar": { diff --git a/messages/ko.json b/messages/ko.json index 7c49082af..2bb7cd6dd 100644 --- a/messages/ko.json +++ b/messages/ko.json @@ -5509,6 +5509,7 @@ "invalidValue": "이 항목이 받는 답변이 아니에요.", "invalidFormat": "이 항목이 읽을 수 있는 형식이 아니에요.", "invalidType": "이 항목이 저장할 수 있는 형식으로 오지 않았어요.", + "rateLimited": "너무 자주 바꿨어요. 잠시 후 다시 시도해 주세요.", "other": "이 값은 저장하지 못했어요. 확인한 뒤 다시 시도해 주세요." }, "avatar": { diff --git a/messages/pl.json b/messages/pl.json index 9ac7a0414..b65185705 100644 --- a/messages/pl.json +++ b/messages/pl.json @@ -5509,6 +5509,7 @@ "invalidValue": "To nie jest jedna z odpowiedzi, które przyjmuje to pole.", "invalidFormat": "To nie jest zapisane w formacie, który to pole może odczytać.", "invalidType": "Wartość nie dotarła w formacie, który to pole może zapisać.", + "rateLimited": "Zmieniane zbyt często. Odczekaj chwilę i spróbuj ponownie.", "other": "Ta wartość nie została przyjęta. Sprawdź ją i spróbuj ponownie." }, "avatar": { diff --git a/src/__tests__/profile-email-rate-limit-contract.test.ts b/src/__tests__/profile-email-rate-limit-contract.test.ts index 5245581c1..fbcf6d450 100644 --- a/src/__tests__/profile-email-rate-limit-contract.test.ts +++ b/src/__tests__/profile-email-rate-limit-contract.test.ts @@ -115,6 +115,14 @@ describe("the profile email-address bucket", () => { ); }); + it("says this response is only for a request that asked for nothing else", () => { + // The refusal narrows to the field when the request carries other + // fields, so a client reading the 429 as "the whole save failed" would + // be wrong. The sentence has to carry that or it is worse than silence. + expect(published).toContain("rejectedFields"); + expect(published).toContain("rate_limited"); + }); + it("is published on both operations that answer it", () => { const doc = buildOpenApiDocument() as unknown as { paths: Record< diff --git a/src/lib/auth/__tests__/profile-update-email-limit.test.ts b/src/lib/auth/__tests__/profile-update-email-limit.test.ts index 0a87fadc7..7693c5ca3 100644 --- a/src/lib/auth/__tests__/profile-update-email-limit.test.ts +++ b/src/lib/auth/__tests__/profile-update-email-limit.test.ts @@ -7,12 +7,15 @@ * ceiling of any kind — `apiHandler` supplies none — so one account could * walk an address book at whatever rate the network allowed. * - * Three things are pinned here. The charge happens BEFORE the uniqueness - * probe, so a refused request never learns the answer it was refused for. - * A save that carries the address already on file is not charged at all, so - * a settings form that re-posts the whole profile does not spend an honest - * afternoon of edits on the ceiling. And the refusal carries the code the - * contract publishes. + * What is pinned here. The charge happens BEFORE the uniqueness probe, so a + * refused request never learns the answer it was refused for. A save that + * carries the address already on file is not charged at all, so a settings + * form that re-posts the whole profile does not spend an honest afternoon of + * edits on the ceiling. A refusal drops the address and keeps the rest of the + * save, so one spent budget cannot block somebody setting their timezone; the + * published 429 is for the request that asked for nothing else. And both + * answers worth detecting leave an audit row, because neither reaches the + * ordinary `profile.update` row written after the save. */ import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -32,6 +35,7 @@ vi.mock("@/lib/rate-limit", () => ({ import { applyProfileUpdate } from "../profile-update"; import { prisma } from "@/lib/db"; +import { auditLog } from "@/lib/auth/audit"; import { checkProfileEmailRateLimit } from "@/lib/rate-limit"; const USER_ID = "user-1"; @@ -85,7 +89,7 @@ describe("applyProfileUpdate — the email conflict is metered", () => { expect(chargedAt).toBeLessThan(probedAt); }); - it("refuses with 429 and the published code, without asking about the address", async () => { + it("refuses with 429 and the published code when the address was all that was asked for", async () => { vi.mocked(checkProfileEmailRateLimit).mockResolvedValue({ allowed: false, limit: 10, @@ -126,6 +130,94 @@ describe("applyProfileUpdate — the email conflict is metered", () => { expect(checkProfileEmailRateLimit).not.toHaveBeenCalled(); }); + it("keeps the rest of the save when the budget is spent, dropping only the address", async () => { + // The web form posts the whole profile in one request. Failing all of it + // would stop somebody setting their timezone for an hour and tell them it + // was about email addresses. + vi.mocked(checkProfileEmailRateLimit).mockResolvedValue({ + allowed: false, + limit: 10, + remaining: 0, + resetAt: Date.now() + 3_600_000, + }); + + const result = await applyProfileUpdate(USER_ID, { + email: "someone@example.test", + timezone: "Europe/Lisbon", + heightCm: 180, + }); + + expect(result.ok).toBe(true); + const written = vi.mocked(prisma.user.update).mock.calls[0]![0]! + .data as Record; + expect(Object.keys(written).sort()).toEqual(["heightCm", "timezone"]); + expect("email" in written).toBe(false); + if (result.ok) { + expect(result.rejectedFields).toEqual([ + expect.objectContaining({ path: "email", code: "rate_limited" }), + ]); + } + // Still no probe: the refused caller learns nothing about the address. + expect(emailProbes()).toHaveLength(0); + }); + + it("records both answers worth detecting in the audit ledger", async () => { + vi.mocked(checkProfileEmailRateLimit).mockResolvedValue({ + allowed: false, + limit: 10, + remaining: 0, + resetAt: Date.now() + 3_600_000, + }); + await applyProfileUpdate(USER_ID, { email: "someone@example.test" }); + expect(auditLog).toHaveBeenCalledWith( + "profile.email.rate_limited", + expect.objectContaining({ userId: USER_ID }), + ); + + vi.mocked(auditLog).mockClear(); + vi.mocked(checkProfileEmailRateLimit).mockResolvedValue({ + allowed: true, + limit: 10, + remaining: 9, + resetAt: Date.now() + 3_600_000, + }); + vi.mocked(prisma.user.findUnique).mockImplementation((async (args: { + where: Record; + }) => { + if ("id" in args.where) return { email: ON_FILE }; + return { id: "someone-else" }; + }) as never); + + await applyProfileUpdate(USER_ID, { email: "taken@example.test" }); + expect(auditLog).toHaveBeenCalledWith( + "profile.email.conflict", + expect.objectContaining({ userId: USER_ID }), + ); + // Neither path reaches the ordinary post-save row. + expect(auditLog).not.toHaveBeenCalledWith( + "profile.update", + expect.anything(), + ); + }); + + it("does not charge an account whose stored address is spelled differently", async () => { + // Registration writes the address as typed, so an account created as + // `Mine@Example.Test` has that in the column. Comparing it raw against + // the normalised submission would charge its first ordinary save. + vi.mocked(prisma.user.findUnique).mockImplementation((async (args: { + where: Record; + }) => { + if ("id" in args.where) return { email: "Mine@Example.Test" }; + return null; + }) as never); + + const result = await applyProfileUpdate(USER_ID, { email: ON_FILE }); + + expect(result.ok).toBe(true); + expect(checkProfileEmailRateLimit).not.toHaveBeenCalled(); + expect(emailProbes()).toHaveLength(0); + }); + it("still answers 409 when the bucket has room and the address is taken", async () => { vi.mocked(prisma.user.findUnique).mockImplementation((async (args: { where: Record; diff --git a/src/lib/auth/profile-update.ts b/src/lib/auth/profile-update.ts index 8c5257caf..1891c1575 100644 --- a/src/lib/auth/profile-update.ts +++ b/src/lib/auth/profile-update.ts @@ -14,6 +14,7 @@ import { } from "@/lib/cache/invalidate"; import { sanitiseZodIssues, type SanitisedZodIssue } from "@/lib/api-response"; import { checkProfileEmailRateLimit } from "@/lib/rate-limit"; +import { annotate } from "@/lib/logging/context"; import { z } from "zod/v4"; const extendedProfileSchema = profileSchema.extend({ @@ -245,12 +246,18 @@ export async function applyProfileUpdate( // this caller — so they skip both the uniqueness probe and the budget. // Charging them would spend an honest afternoon of edits on the // ceiling and lock the account out of its own settings page. + // + // The stored side is normalised for the comparison too. Registration + // writes the address exactly as typed, so an account registered as + // `MixedCase@Example.com` would otherwise fail to match its own address + // and pay for its first ordinary save. const current = await prisma.user.findUnique({ where: { id: userId }, select: { email: true }, }); + const addressOnFile = current?.email?.trim().toLowerCase() ?? null; - if (current?.email !== normalizedEmail) { + if (addressOnFile !== normalizedEmail) { // Charged BEFORE the probe, not after: a refused request must not // learn the answer it was refused for. The 409 below tells any // signed-in caller whether an address is registered here, and until @@ -258,25 +265,69 @@ export async function applyProfileUpdate( // any kind — `apiHandler` supplies no default. const rl = await checkProfileEmailRateLimit(userId); if (!rl.allowed) { - return { - ok: false, - status: 429, - message: "Too many email-address changes. Try again later.", - errorCode: "profile.update.emailRateLimited", - }; - } + annotate({ action: { name: "profile.email.throttled" } }); + await auditLog("profile.email.rate_limited", { + userId, + ipAddress: ipAddress ?? null, + }); - const existing = await prisma.user.findUnique({ - where: { email: normalizedEmail }, - select: { id: true }, - }); - if (existing && existing.id !== userId) { - return { - ok: false, - status: 409, - message: "Email already in use", - errorCode: "profile.update.emailInUse", - }; + // Narrow the refusal to the field, the way the demo narrowing above + // does: the web form posts the whole profile in one request, so + // failing all of it would stop somebody setting their timezone for + // an hour and tell them it was about email addresses. The address is + // dropped, everything else is saved, and `rejectedFields` says which + // field did not go through. + const { email: _refusedAddress, ...keepable } = data; + if (Object.keys(keepable).length > 0) { + data = keepable; + rejectedFields = [ + ...rejectedFields, + { + path: "email", + code: "rate_limited", + message: "Too many email-address changes.", + }, + ]; + } else { + // Nothing else was asked for, so there is nothing to salvage and a + // partial success would be a lie — the same rule the all-fields- + // rejected 422 above follows. This is the arm the published 429 + // describes. + return { + ok: false, + status: 429, + message: "Too many email-address changes. Try again later.", + errorCode: "profile.update.emailRateLimited", + }; + } + } else { + // Case-sensitive, because the column is: registration writes the + // address as typed, so `Mixed@Example.com` and `mixed@example.com` + // are two rows this probe cannot see as one. Pre-existing and not + // widened here — the fix is a normalisation pass on the register + // write, which is where the two spellings are created. + const existing = await prisma.user.findUnique({ + where: { email: normalizedEmail }, + select: { id: true }, + }); + if (existing && existing.id !== userId) { + // The informative answer is the one worth recording: it says an + // account holds that address. The ordinary save's `profile.update` + // row is written further down and this path never reaches it, so + // without a row here the only probes in the ledger would be the + // ones that came back negative. + annotate({ action: { name: "profile.email.conflict" } }); + await auditLog("profile.email.conflict", { + userId, + ipAddress: ipAddress ?? null, + }); + return { + ok: false, + status: 409, + message: "Email already in use", + errorCode: "profile.update.emailInUse", + }; + } } } } diff --git a/src/lib/openapi/routes/auth.ts b/src/lib/openapi/routes/auth.ts index da312160e..26a80fa82 100644 --- a/src/lib/openapi/routes/auth.ts +++ b/src/lib/openapi/routes/auth.ts @@ -527,7 +527,7 @@ const authProfileUpdateResponse = z ) .optional() .describe( - "Present only on a PARTIAL success: the fields that failed validation and were skipped while the rest of the patch was written. A 200 carrying this key means the save was incomplete — surface it, do not treat the response as a clean save.", + "Present only on a PARTIAL success: the fields that were skipped while the rest of the patch was written. A 200 carrying this key means the save was incomplete — surface it, do not treat the response as a clean save. `code` is the validator code for a field that failed validation, or `rate_limited` for an email-address change that has spent the account's hourly budget; that one is not a bad value and the same address will be accepted once the window rolls over.", ), }) .meta({ diff --git a/src/lib/openapi/routes/profile.ts b/src/lib/openapi/routes/profile.ts index b230d82e5..96dd3d464 100644 --- a/src/lib/openapi/routes/profile.ts +++ b/src/lib/openapi/routes/profile.ts @@ -483,6 +483,18 @@ const profileUpdateResponse = z hasInsuranceNumber: z .boolean() .describe("Whether a KVNR is on file (the value itself is not echoed)."), + rejectedFields: z + .array( + z.object({ + path: z.string(), + code: z.string(), + message: z.string(), + }), + ) + .optional() + .describe( + "Present only on a PARTIAL success: the fields that were skipped while the rest of the patch was written. A 200 carrying this key means the save was incomplete — surface it, do not treat the response as a clean save. `code` is the validator code for a field that failed validation, or `rate_limited` for an email-address change that has spent the account's hourly budget; that one is not a bad value and the same address will be accepted once the window rolls over. The handler has always been able to answer with this key on this path; it was published only on PUT /api/auth/profile.", + ), }) .meta({ id: "ProfileUpdateResponse", diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index 324e165cd..b0a6149b1 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -338,7 +338,7 @@ export const recordWriteRateLimitResponse = { export const profileEmailRateLimitResponse = { "429": { description: - "Too many email-address changes. The address change shares one per-account bucket across both profile routes — `profile-email:`, 10 requests per 3600 seconds — keyed on the ACTING account, so an account cannot collect a fresh allowance by probing a different address. Nothing was written. `meta.errorCode` is `profile.update.emailRateLimited`. Only a request whose `email` actually differs from the address on file is counted: re-saving a form that carries the unchanged address is free, and so is any update that does not touch `email` at all. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing.", + "Too many email-address changes, and the request asked for nothing else. The address change shares one per-account bucket across both profile routes — `profile-email:`, 10 requests per 3600 seconds — keyed on the ACTING account, so an account cannot collect a fresh allowance by probing a different address. `meta.errorCode` is `profile.update.emailRateLimited`. Only a request whose `email` actually differs from the address on file is counted: re-saving a form that carries the unchanged address is free, and so is any update that does not touch `email` at all. A request that carries other fields beside the refused address does NOT get this response — it answers 200, saves those fields, and names `email` in `rejectedFields` with code `rate_limited`, so one spent budget cannot block an unrelated profile edit for an hour. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing.", content: { "application/json": { schema: errorEnvelope } }, headers: stdResponses["429"].headers, }, diff --git a/src/lib/profile/rejected-fields.ts b/src/lib/profile/rejected-fields.ts index da0a3e604..87225925e 100644 --- a/src/lib/profile/rejected-fields.ts +++ b/src/lib/profile/rejected-fields.ts @@ -73,6 +73,11 @@ export function describeRejectedProfileField( * degrade to something honest instead of to silence. */ const REJECTION_REASON_KEYS: Record = { + // Not a Zod code: the profile helper refuses an email-address change that + // has spent the account's hourly budget this way, so the rest of the save + // still lands. Without an entry it would fall back to "check it and try + // again", which is wrong — the value was fine, the timing was not. + rate_limited: "settings.profileRejection.rateLimited", too_big: "settings.profileRejection.tooBig", too_small: "settings.profileRejection.tooSmall", invalid_value: "settings.profileRejection.invalidValue", diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 5021c28f3..5a8816b4b 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -180,8 +180,11 @@ export async function checkRecordWriteRateLimit( * address actually differs from the one on file, so re-saving a settings * form that carries the unchanged address never spends the budget. What it * ends is the unbounded sweep: two hundred and forty questions a day per - * account, each with an audit row beside it, instead of as many as the - * network will carry. + * account instead of as many as the network will carry. Both answers worth + * detecting write an audit row of their own — `profile.email.conflict` when + * an address turns out to be taken, `profile.email.rate_limited` when the + * budget runs out. The ordinary `profile.update` row is written after the + * save, which neither of those paths reaches. */ export const PROFILE_EMAIL_BUCKET_PREFIX = "profile-email"; export const PROFILE_EMAIL_LIMIT = 10;