From fffdc83ca4382958cc05cb453ceb8737622692cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 23 Aug 2026 16:22:27 +0200 Subject: [PATCH] Make the reset-every-threshold delete ask first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE /api/user/thresholds without ?metric= erased the account's whole override set on an omitted query parameter alone: no confirmation, no dry run, and no rate limit of its own while the PUT beside it had one. The wide form now wants { "confirm": "RESET_THRESHOLDS" } in the body — the same typed-confirmation shape /api/settings/account and /api/settings/data already use — and refuses 422 without writing anything. The ?metric= form is untouched: it resets one band the person can retype, and the targets sheet fires it in a loop. Both forms now run the PUT's 30 / 5 min limit. The settings card's "reset all" control already opened a confirm dialog; it now sends that answer to the server too. Its token is compared against the route's declaration in the tests, so the two ends cannot drift apart into a permanent 422. --- docs/api/openapi.yaml | 37 ++++- .../user/thresholds/__tests__/route.test.ts | 145 +++++++++++++++++- src/app/api/user/thresholds/route.ts | 57 ++++++- .../settings/thresholds-editor-section.tsx | 17 +- src/lib/openapi/routes/profile.ts | 41 ++++- 5 files changed, 282 insertions(+), 15 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 70df81108..fc02bc56f 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -7701,18 +7701,27 @@ paths: summary: Reset one threshold override, or all of them description: >- With `?metric=` it drops that metric's override; WITHOUT the parameter it drops EVERY override on the account. - There is no confirmation step and no dry run, so a client that means to reset one metric must send the - parameter. + The wide form is not reachable by omitting the parameter alone — it requires `{ "confirm": "RESET_THRESHOLDS" }` + in the body, the same typed-confirmation shape account deletion and the data reset use. The `?metric=` form + takes no body and is unchanged. - Audit-logged with the before and after maps. Unlike the PUT this path runs no rate limit of its own. + Audit-logged with the before and after maps. Rate-limited 30 / 5 min per user, matching the PUT. parameters: - in: query name: metric schema: - description: The single metric to reset. Omit to reset every override on the account. + description: The single metric to reset. Omit to reset every override on the account — which then requires the + confirmation body. $ref: "#/components/schemas/ThresholdMetric" - description: The single metric to reset. Omit to reset every override on the account. + description: The single metric to reset. Omit to reset every override on the account — which then requires the + confirmation body. + requestBody: + required: false + content: + application/json: + schema: + $ref: "#/components/schemas/ThresholdsResetAllRequest" responses: "200": description: The remaining override map. @@ -7727,7 +7736,12 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "401": *a1 - "422": *a4 + "422": + description: '`metric` was omitted and the body did not carry `{ "confirm": "RESET_THRESHOLDS" }`. Nothing was reset.' + content: + application/json: + schema: + $ref: "#/components/schemas/ErrorEnvelope" "429": *a5 /api/auth/me/devices: get: @@ -23030,6 +23044,17 @@ components: - TOTAL_BODY_WATER - BONE_MASS - OXYGEN_SATURATION + ThresholdsResetAllRequest: + type: object + properties: + confirm: + type: string + const: RESET_THRESHOLDS + description: Required when `metric` is omitted. Compared exactly; any other value is refused. + required: + - confirm + description: Confirmation body for the reset-everything form. Send no body at all when resetting a single metric with + `?metric=`. DevicePatchRequest: type: object properties: diff --git a/src/app/api/user/thresholds/__tests__/route.test.ts b/src/app/api/user/thresholds/__tests__/route.test.ts index dca481f73..c8524b19d 100644 --- a/src/app/api/user/thresholds/__tests__/route.test.ts +++ b/src/app/api/user/thresholds/__tests__/route.test.ts @@ -32,6 +32,9 @@ vi.mock("@/lib/analytics/effective-range", async (importOriginal) => { }; }); vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserProfile: vi.fn(), +})); vi.mock("@/lib/db-compat", () => ({ ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), })); @@ -44,7 +47,7 @@ vi.mock("next/headers", () => ({ })), })); -import { GET, PUT } from "../route"; +import { GET, PUT, DELETE } from "../route"; import { getSession } from "@/lib/auth/session"; import { checkRateLimit } from "@/lib/rate-limit"; import { prisma } from "@/lib/db"; @@ -63,6 +66,31 @@ function putReq(body: unknown): NextRequest { }); } +function deleteReq( + options: { metric?: string; body?: unknown } = {}, +): NextRequest { + const url = options.metric + ? `http://localhost/api/user/thresholds?metric=${options.metric}` + : "http://localhost/api/user/thresholds"; + return new NextRequest(url, { + method: "DELETE", + ...(options.body === undefined + ? {} + : { + headers: { "content-type": "application/json" }, + body: JSON.stringify(options.body), + }), + }); +} + +/** The stored override set the DELETE branches read before they write. */ +function armOverrides(overrides: Record) { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + thresholdsJson: overrides, + } as never); + vi.mocked(prisma.user.update).mockResolvedValue({} as never); +} + beforeEach(() => { vi.resetAllMocks(); vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); @@ -111,6 +139,121 @@ describe("PUT /api/user/thresholds — 422 multi-issue (v1.4.43 W6)", () => { }); }); +/** + * The parameterless DELETE erases every band the account has tuned, and it + * used to do it on an omitted query parameter alone — no confirmation, no dry + * run, and no rate limit of its own while the PUT beside it had one. + * + * The confirmation is the same shape the other two wide deletes use + * (`/api/settings/account` wants `DELETE_ACCOUNT`, `/api/settings/data` wants + * `DELETE`): a literal in the body, compared exactly. What is pinned here is + * the pair of properties that make it worth anything — the refusal happens, + * AND nothing was written when it does. A status assertion on its own would + * stay green if the wipe ran first and the 422 came after it. + * + * The single-metric form is pinned as UNCHANGED in the same block, because the + * failure mode of a confirmation is that it spreads to the narrow path and + * breaks the targets sheet, which resets metrics in a loop. + * + * Mutation check: drop the `confirm !== RESET_ALL_CONFIRMATION` branch and the + * first two cases go red; drop the rate-limit call and the fourth goes red; + * extend the confirmation to the `?metric=` arm and the fifth goes red. + */ +describe("DELETE /api/user/thresholds — the wide form asks first", () => { + it("refuses the parameterless form with no body", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + + const res = await DELETE(deleteReq()); + + expect(res.status).toBe(422); + const body = (await res.json()) as { error: string }; + expect(body.error).toContain("RESET_THRESHOLDS"); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it("refuses a body whose confirmation is the wrong token", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + + const res = await DELETE(deleteReq({ body: { confirm: "DELETE" } })); + + expect(res.status).toBe(422); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + it("wipes every override once the confirmation is present", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 }, PULSE: { min: 50, max: 90 } }); + + const res = await DELETE( + deleteReq({ body: { confirm: "RESET_THRESHOLDS" } }), + ); + + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { overrides: unknown } }; + expect(body.data.overrides).toEqual({}); + expect(prisma.user.update).toHaveBeenCalledTimes(1); + }); + + it("runs the sibling's rate limit and honours a refusal", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 } }); + vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false } as never); + + const res = await DELETE( + deleteReq({ body: { confirm: "RESET_THRESHOLDS" } }), + ); + + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledWith( + "thresholds:reset:user-1", + 30, + 5 * 60 * 1000, + ); + expect(prisma.user.update).not.toHaveBeenCalled(); + }); + + /** + * The other end. A confirmation the server demands and the only client that + * sends the wide form does not send is not a safeguard, it is an outage — + * and it would look like a passing suite, because the route tests above + * construct their own requests. So the token is compared across the two + * files rather than assumed to match. + */ + it("is the same token the settings card sends", async () => { + const { readFileSync } = await import("node:fs"); + const { join } = await import("node:path"); + + const route = readFileSync( + join(process.cwd(), "src/app/api/user/thresholds/route.ts"), + "utf8", + ); + const client = readFileSync( + join( + process.cwd(), + "src/components/settings/thresholds-editor-section.tsx", + ), + "utf8", + ); + + const declared = route.match(/RESET_ALL_CONFIRMATION = "([^"]+)"/)?.[1]; + expect(declared, "the route no longer declares a confirmation").toBe( + "RESET_THRESHOLDS", + ); + expect(client).toContain(`confirm: "${declared}"`); + }); + + it("leaves the single-metric form exactly as it was", async () => { + armOverrides({ WEIGHT: { min: 60, max: 80 }, PULSE: { min: 50, max: 90 } }); + + // No body at all — the targets sheet sends none, and must keep working. + const res = await DELETE(deleteReq({ metric: "WEIGHT" })); + + expect(res.status).toBe(200); + const body = (await res.json()) as { + data: { overrides: Record }; + }; + expect(Object.keys(body.data.overrides)).toEqual(["PULSE"]); + }); +}); + /** * The docblock promised a key the handler has never returned. * diff --git a/src/app/api/user/thresholds/route.ts b/src/app/api/user/thresholds/route.ts index 5034ae027..22fcce714 100644 --- a/src/app/api/user/thresholds/route.ts +++ b/src/app/api/user/thresholds/route.ts @@ -9,7 +9,10 @@ * it is worth stating the absence rather than just deleting the word. * PUT accepts a partial map — only the provided metrics are updated, others * keep their existing override or remain on the default. - * DELETE with ?metric=... resets one metric to default. + * DELETE with ?metric=... resets one metric to default. WITHOUT the parameter + * it drops every override the account has, so it demands the typed + * confirmation the other wide-blast-radius deletes demand — see + * `RESET_ALL_CONFIRMATION` below. */ import { apiHandler, requireAuth } from "@/lib/api-handler"; import { @@ -118,6 +121,21 @@ export const PUT = apiHandler(async (request: NextRequest) => { return apiSuccess({ overrides: merged }); }); +/** + * What the parameterless DELETE has to be told before it wipes every override. + * + * Same shape as `/api/settings/account` (`DELETE_ACCOUNT`) and + * `/api/settings/data` (`DELETE`): a literal in the JSON body, compared + * exactly, with no default. The point is that the wide form cannot be reached + * by dropping a query parameter — a request that means "reset everything" has + * to say so in a second place. + * + * NOT applied to `?metric=`. That form resets one band the person can retype + * from the editor beside it, and the targets sheet fires it in a loop over the + * metrics it owns; a confirmation there would be ceremony without a decision. + */ +const RESET_ALL_CONFIRMATION = "RESET_THRESHOLDS"; + export const DELETE = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); const url = new URL(request.url); @@ -128,6 +146,43 @@ export const DELETE = apiHandler(async (request: NextRequest) => { meta: { metric: metric ?? "all" }, }); + // The PUT's limit, on the path that can destroy more in one call than the + // PUT can. It had none at all: an omitted query parameter erased the whole + // override set, and nothing stopped that being repeated. + const rl = await checkRateLimit( + `thresholds:reset:${user.id}`, + 30, + 5 * 60 * 1000, + ); + if (!rl.allowed) { + return apiError("Too many requests, please slow down", 429); + } + + if (!metric) { + const raw = await request.text(); + if (raw.length > 64 * 1024) { + return apiError(`Request body exceeds ${64 * 1024} bytes`, 413); + } + let confirm = ""; + if (raw.length > 0) { + try { + const body = JSON.parse(raw) as { confirm?: unknown }; + confirm = typeof body?.confirm === "string" ? body.confirm : ""; + } catch { + return apiError("Invalid request", 422); + } + } + if (confirm !== RESET_ALL_CONFIRMATION) { + // Names the token rather than saying "confirmation missing", so a client + // that reached here by omitting `?metric=` by accident is told what the + // request it sent actually meant. + return apiError( + `Confirmation missing. Resetting every threshold override requires { confirm: '${RESET_ALL_CONFIRMATION}' }`, + 422, + ); + } + } + const existing = await prisma.user.findUnique({ where: { id: user.id }, select: { thresholdsJson: true }, diff --git a/src/components/settings/thresholds-editor-section.tsx b/src/components/settings/thresholds-editor-section.tsx index 29a9689f1..0d638024b 100644 --- a/src/components/settings/thresholds-editor-section.tsx +++ b/src/components/settings/thresholds-editor-section.tsx @@ -108,10 +108,19 @@ export function ThresholdsEditorSection({ id }: { id: string }) { const resetMutation = useMutation({ mutationFn: async (metric: ThresholdMetric | null) => { - const url = metric - ? `/api/user/thresholds?metric=${metric}` - : "/api/user/thresholds"; - const res = await apiFetchRaw(url, { method: "DELETE" }); + // `metric === null` is the reset-everything arm behind the confirm + // dialog at the foot of the card. The server asks for the same decision + // in the body, so the dialog's answer is what gets sent — the wide form + // is not reachable by dropping the query parameter. + const res = metric + ? await apiFetchRaw(`/api/user/thresholds?metric=${metric}`, { + method: "DELETE", + }) + : await apiFetchRaw("/api/user/thresholds", { + method: "DELETE", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ confirm: "RESET_THRESHOLDS" }), + }); if (!res.ok) throw new Error("reset failed"); }, onSuccess: () => { diff --git a/src/lib/openapi/routes/profile.ts b/src/lib/openapi/routes/profile.ts index f3e50f971..f2d5b8eba 100644 --- a/src/lib/openapi/routes/profile.ts +++ b/src/lib/openapi/routes/profile.ts @@ -653,6 +653,27 @@ const thresholdsOverridesResponse = z .object({ overrides: thresholdOverrideMap }) .meta({ id: "ThresholdsOverridesResponse" }); +/** + * The typed confirmation the parameterless DELETE demands. + * + * Published because it is the difference between "reset one band" and "erase + * every band the account has tuned", and a client that learned the endpoint + * from the contract alone would otherwise discover the requirement as a 422. + */ +const thresholdsResetAllRequest = z + .object({ + confirm: z + .literal("RESET_THRESHOLDS") + .describe( + "Required when `metric` is omitted. Compared exactly; any other value is refused.", + ), + }) + .meta({ + id: "ThresholdsResetAllRequest", + description: + "Confirmation body for the reset-everything form. Send no body at all when resetting a single metric with `?metric=`.", + }); + const thresholdsUpdateRequest = thresholdsUpdateSchema.meta({ id: "ThresholdsUpdateRequest", description: @@ -1716,17 +1737,23 @@ export const profilePaths: NonNullable = { tags: ["Analytics"], summary: "Reset one threshold override, or all of them", description: - "With `?metric=` it drops that metric's override; WITHOUT the parameter it drops EVERY override on the account. There is no confirmation step and no dry run, so a client that means to reset one metric must send the parameter.\n\n" + - "Audit-logged with the before and after maps. Unlike the PUT this path runs no rate limit of its own.", + 'With `?metric=` it drops that metric\'s override; WITHOUT the parameter it drops EVERY override on the account. The wide form is not reachable by omitting the parameter alone — it requires `{ "confirm": "RESET_THRESHOLDS" }` in the body, the same typed-confirmation shape account deletion and the data reset use. The `?metric=` form takes no body and is unchanged.\n\n' + + "Audit-logged with the before and after maps. Rate-limited 30 / 5 min per user, matching the PUT.", requestParams: { query: z.object({ metric: thresholdMetricEnum .optional() .describe( - "The single metric to reset. Omit to reset every override on the account.", + "The single metric to reset. Omit to reset every override on the account — which then requires the confirmation body.", ), }), }, + requestBody: { + required: false, + content: { + "application/json": { schema: thresholdsResetAllRequest }, + }, + }, responses: { "200": { description: "The remaining override map.", @@ -1745,6 +1772,14 @@ export const profilePaths: NonNullable = { content: { "application/json": { schema: errorEnvelope } }, }, ...stdResponses, + // After `stdResponses` on purpose: the generic 422 there would + // otherwise overwrite this one and the confirmation contract would + // vanish from the published operation. + "422": { + description: + '`metric` was omitted and the body did not carry `{ "confirm": "RESET_THRESHOLDS" }`. Nothing was reset.', + content: { "application/json": { schema: errorEnvelope } }, + }, }, }, },