From 029c5262528c1f3c6851ec92c1012ac326791ac3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 3 Aug 2026 20:49:27 +0200 Subject: [PATCH] Give the intake purge a tombstone, and two health writes their audit row The dose-history purge was the one destructive verb in the intake family that removed rows outright. Both siblings, the single-event DELETE and the bulk delete, stamp `deletedAt` and bump `syncVersion`; the purge called `deleteMany`, so a mis-tap was final and a client offline at purge time kept every row forever, because a row that no longer exists produces no tombstone for the sync feed to carry. It tombstones now, scoped to live rows so a second purge counts zero. No inventory refund: a purge wipes a history as bookkeeping, and refunding every dose ever taken would inflate the current container count instead. The manual water quick-add and the lab-result un-delete both wrote health data and reached only the wide event, which expires. Months later nothing could answer who put a day's total there or who brought a reading back, and with a second person able to act on a shared record that question has a second possible answer. Both write a durable row now, named after the neighbours they sit beside. The refused-delegation audit comment claimed the row was written for the owner's benefit. It is not: it is filed under the caller, it is only written for a header refusal, and a header names an account the caller merely asserts. Filing under that claim would let anyone write rows into any account's activity feed by naming it. The comment says what the row is now, and points at `recordDelegatedAccess`, which is the half the owner does see. Two comments in the biomarker surfaces still claimed the `SetNull` foreign key left the readings behind when a marker is deleted. That stopped being true when delete became a "drop this and its history" action and the confirmation copy was rewritten in six locales to say so. The purge route gains a call-shape assertion rather than a count assertion, because a count comes back identical from a `deleteMany`. The lab restore route had no test at all and now has one. --- .../api/labs/restore/__tests__/route.test.ts | 149 ++++++++++++++++++ src/app/api/labs/restore/route.ts | 12 ++ .../[id]/intake/purge/__tests__/route.test.ts | 35 +++- .../medications/[id]/intake/purge/route.ts | 23 ++- .../nutrients/water/__tests__/route.test.ts | 35 ++++ src/app/api/nutrients/water/route.ts | 13 ++ src/components/labs/biomarker-manager.tsx | 10 +- src/components/labs/lab-biomarker-detail.tsx | 9 +- src/lib/api-handler.ts | 21 ++- 9 files changed, 293 insertions(+), 14 deletions(-) create mode 100644 src/app/api/labs/restore/__tests__/route.test.ts diff --git a/src/app/api/labs/restore/__tests__/route.test.ts b/src/app/api/labs/restore/__tests__/route.test.ts new file mode 100644 index 000000000..5a4e3bcaf --- /dev/null +++ b/src/app/api/labs/restore/__tests__/route.test.ts @@ -0,0 +1,149 @@ +/** + * `POST /api/labs/restore` — the un-tombstone behind the lab-history + * delete-Undo toast. + * + * The route had no test of its own. It writes health data back into the + * record and, until now, wrote no audit row for it: the trail showed a + * deletion that appeared to have been reversed by nobody. With a delegate + * able to act on a shared record that question has a second possible answer, + * so the row is what makes it answerable. + * + * Also pins the ownership scope the affordance depends on — a forged, + * foreign or never-deleted id is a silent no-op rather than a 404 existence + * leak — because that scope lives in the mutation `where` and nothing else + * asserted it. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/db", () => ({ + prisma: { + labResult: { updateMany: vi.fn() }, + }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(), + rateLimitHeaders: () => ({}), +})); +vi.mock("@/lib/idempotency", () => ({ + withIdempotency: + (fn: (...args: Args) => Promise) => + (...args: Args) => + fn(...args), +})); +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserHealthScore: vi.fn(), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { POST } from "../route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; +import { auditLog } from "@/lib/auth/audit"; +import { checkRateLimit } from "@/lib/rate-limit"; +import { invalidateUserHealthScore } from "@/lib/cache/invalidate"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "tester", role: "USER" as const }, +}; + +function postReq(body: unknown): NextRequest { + return new NextRequest("http://localhost/api/labs/restore", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + remaining: 59, + reset: Date.now() + 60_000, + } as never); + vi.mocked(prisma.labResult.updateMany).mockResolvedValue({ + count: 2, + } as never); +}); + +describe("POST /api/labs/restore", () => { + it("clears the tombstone on owned, currently-deleted rows only", async () => { + const res = await POST(postReq({ ids: ["lab-1", "lab-2"] })); + expect(res.status).toBe(200); + + const call = vi.mocked(prisma.labResult.updateMany).mock.calls[0][0] as { + where: Record; + data: Record; + }; + expect(call.where).toEqual({ + id: { in: ["lab-1", "lab-2"] }, + userId: "user-1", + deletedAt: { not: null }, + }); + expect(call.data).toEqual({ deletedAt: null }); + }); + + it("writes an audit row naming how many readings came back", async () => { + await POST(postReq({ ids: ["lab-1", "lab-2"] })); + + expect(auditLog).toHaveBeenCalledTimes(1); + const [action, payload] = vi.mocked(auditLog).mock.calls[0] as [ + string, + { userId: string; details: Record }, + ]; + expect(action).toBe("labResult.restore"); + expect(payload.userId).toBe("user-1"); + expect(payload.details).toMatchObject({ count: 2 }); + }); + + it("reports zero and leaves the health score alone when nothing matched", async () => { + vi.mocked(prisma.labResult.updateMany).mockResolvedValue({ + count: 0, + } as never); + + const res = await POST(postReq({ ids: ["forged"] })); + expect(res.status).toBe(200); + const json = (await res.json()) as { data: { restored: number } }; + expect(json.data.restored).toBe(0); + expect(invalidateUserHealthScore).not.toHaveBeenCalled(); + }); + + it("rejects an empty id list with 422 and touches nothing", async () => { + const res = await POST(postReq({ ids: [] })); + expect(res.status).toBe(422); + expect(prisma.labResult.updateMany).not.toHaveBeenCalled(); + expect(auditLog).not.toHaveBeenCalled(); + }); + + it("returns 429 without restoring when the rate limit is exceeded", async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + reset: Date.now() + 60_000, + } as never); + + const res = await POST(postReq({ ids: ["lab-1"] })); + expect(res.status).toBe(429); + expect(prisma.labResult.updateMany).not.toHaveBeenCalled(); + expect(auditLog).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/labs/restore/route.ts b/src/app/api/labs/restore/route.ts index af4c853dc..24c734675 100644 --- a/src/app/api/labs/restore/route.ts +++ b/src/app/api/labs/restore/route.ts @@ -14,9 +14,11 @@ import { NextRequest } from "next/server"; import { z } from "zod/v4"; import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { auditLog } from "@/lib/auth/audit"; import { apiError, apiSuccess, + getClientIp, returnAllZodIssues, safeJson, } from "@/lib/api-response"; @@ -69,6 +71,16 @@ async function postRestore(request: NextRequest): Promise { }); if (count > 0) invalidateUserHealthScore(user.id); + // Bringing a reading back is a health-data write and earns the same durable + // row its delete already writes (`labResult.delete`). Without this the trail + // showed a deletion that appeared to be reversed by nobody. Mirrors + // `measurement.restore`, which is the same affordance one domain over. + await auditLog("labResult.restore", { + userId: user.id, + ipAddress: getClientIp(request), + details: { count }, + }); + annotate({ action: { name: "labs.restore" }, meta: { restored: count } }); return apiSuccess({ restored: count }); diff --git a/src/app/api/medications/[id]/intake/purge/__tests__/route.test.ts b/src/app/api/medications/[id]/intake/purge/__tests__/route.test.ts index 9a4a76a03..d0aa31c67 100644 --- a/src/app/api/medications/[id]/intake/purge/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/intake/purge/__tests__/route.test.ts @@ -15,7 +15,7 @@ import { NextRequest } from "next/server"; vi.mock("@/lib/db", () => ({ prisma: { medication: { findUnique: vi.fn() }, - medicationIntakeEvent: { deleteMany: vi.fn() }, + medicationIntakeEvent: { updateMany: vi.fn() }, medicationComplianceRollup: { deleteMany: vi.fn() }, }, })); @@ -82,7 +82,7 @@ beforeEach(() => { vi.mocked(prisma.medication.findUnique).mockResolvedValue({ name: "Test", } as never); - vi.mocked(prisma.medicationIntakeEvent.deleteMany).mockResolvedValue({ + vi.mocked(prisma.medicationIntakeEvent.updateMany).mockResolvedValue({ count: 7, } as never); vi.mocked(prisma.medicationComplianceRollup.deleteMany).mockResolvedValue({ @@ -116,7 +116,36 @@ describe("DELETE /api/medications/[id]/intake/purge", () => { ); const res = await DELETE(deleteReq(), ROUTE_PARAMS); expect(res.status).toBe(404); - expect(prisma.medicationIntakeEvent.deleteMany).not.toHaveBeenCalled(); + expect(prisma.medicationIntakeEvent.updateMany).not.toHaveBeenCalled(); expect(invalidateUserMedications).not.toHaveBeenCalled(); }); + + // The purge was the one destructive verb in the intake family that removed + // rows outright: a mis-tap was final, and a client offline at purge time + // kept every row forever because a row that no longer exists produces no + // tombstone. Assert the CALL SHAPE rather than a count — a count comes back + // identical from a `deleteMany`, which is exactly the regression this pins. + it("tombstones the events instead of removing them", async () => { + await DELETE(deleteReq(), ROUTE_PARAMS); + + expect(prisma.medicationIntakeEvent.updateMany).toHaveBeenCalledTimes(1); + const call = vi.mocked(prisma.medicationIntakeEvent.updateMany).mock + .calls[0][0] as { + where: Record; + data: Record; + }; + + // Scoped to the caller's own rows for this medication, and to LIVE rows + // only, so a second purge counts zero rather than re-stamping tombstones + // and re-bumping every syncVersion the clients have already seen. + expect(call.where).toEqual({ + medicationId: "med-1", + userId: "user-1", + deletedAt: null, + }); + + // A tombstone the sync feed can carry: a stamp plus a monotonic bump. + expect(call.data.deletedAt).toBeInstanceOf(Date); + expect(call.data.syncVersion).toEqual({ increment: 1 }); + }); }); diff --git a/src/app/api/medications/[id]/intake/purge/route.ts b/src/app/api/medications/[id]/intake/purge/route.ts index d7f342851..34af5204b 100644 --- a/src/app/api/medications/[id]/intake/purge/route.ts +++ b/src/app/api/medications/[id]/intake/purge/route.ts @@ -30,8 +30,27 @@ export const DELETE = apiHandler( select: { name: true }, }); - const { count } = await prisma.medicationIntakeEvent.deleteMany({ - where: { medicationId: id, userId: user.id }, + // Tombstone, like both siblings (single-event DELETE, bulk-delete). This + // was the one destructive verb in the intake family with nothing to + // reconstruct from: `deleteMany` removed the rows outright, so a purge + // fired by mistake was final, and it was invisible to `/api/sync/changes` + // — a client offline at purge time kept every row forever, because a row + // that no longer exists produces no tombstone to send. + // + // Setting `deletedAt` (+ bumping `syncVersion`) makes the rows invisible + // to every today / compliance / list read (all of which filter + // `deletedAt: null`) while the sync feed emits one tombstone per row. The + // nightly tombstone purge collects them later, so nothing grows without + // bound. Scope `deletedAt: null` so a second purge counts zero rather than + // re-bumping rows already gone. + // + // No inventory refund, deliberately: the single-event and bulk deletes + // refund because "I did not take that dose" puts the pill back in the box. + // A purge wipes an entire history as bookkeeping, and refunding every dose + // ever taken would inflate the current container count instead. + const { count } = await prisma.medicationIntakeEvent.updateMany({ + where: { medicationId: id, userId: user.id, deletedAt: null }, + data: { deletedAt: new Date(), syncVersion: { increment: 1 } }, }); // v1.4.39 W-MED — drop every rollup row for this medication so the diff --git a/src/app/api/nutrients/water/__tests__/route.test.ts b/src/app/api/nutrients/water/__tests__/route.test.ts index 9139fc7e8..443fe2b55 100644 --- a/src/app/api/nutrients/water/__tests__/route.test.ts +++ b/src/app/api/nutrients/water/__tests__/route.test.ts @@ -38,6 +38,9 @@ vi.mock("@/lib/idempotency", () => ({ fn(...args), })); vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); vi.mock("@/lib/db-compat", () => ({ ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), })); @@ -58,6 +61,7 @@ import { checkRateLimit } from "@/lib/rate-limit"; import { requireModuleEnabled } from "@/lib/modules/gate"; import { invalidateUserDashboardSnapshot } from "@/lib/cache/invalidate"; import { NUTRIENT_CATALOG } from "@/lib/nutrients/catalog"; +import { auditLog } from "@/lib/auth/audit"; const SESSION_OK = { session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, @@ -217,6 +221,37 @@ describe("POST /api/nutrients/water", () => { unit: "ml", }); }); + + // A health-data write earns a durable row. This one reached only the wide + // event, which expires, so months later nothing could answer who put a + // day's total there — a question with two possible answers once a delegate + // can act on a shared record. + it("writes an audit row naming the day, the mode and the amount", async () => { + await POST(postReq({ amountMl: 250, mode: "add", day: "2026-07-16" })); + + expect(auditLog).toHaveBeenCalledTimes(1); + const [action, payload] = vi.mocked(auditLog).mock.calls[0] as [ + string, + { userId: string; details: Record }, + ]; + expect(action).toBe("nutrient.water.write"); + expect(payload.userId).toBe("user-1"); + expect(payload.details).toMatchObject({ + day: "2026-07-16", + mode: "add", + amountMl: 250, + resultingAmount: 500, + }); + }); + + it("writes no audit row when the module gate refuses", async () => { + vi.mocked(requireModuleEnabled).mockResolvedValue({ + enabled: false, + response: apiError("Module disabled", 403), + } as never); + await POST(postReq({ amountMl: 250, mode: "add", day: "2026-07-16" })); + expect(auditLog).not.toHaveBeenCalled(); + }); }); describe("POST /api/nutrients/water — day bound and increment cap", () => { diff --git a/src/app/api/nutrients/water/route.ts b/src/app/api/nutrients/water/route.ts index 84044ffba..1ac6e63fa 100644 --- a/src/app/api/nutrients/water/route.ts +++ b/src/app/api/nutrients/water/route.ts @@ -19,9 +19,11 @@ import { NextRequest } from "next/server"; import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; +import { auditLog } from "@/lib/auth/audit"; import { apiError, apiSuccess, + getClientIp, returnAllZodIssues, safeJson, } from "@/lib/api-response"; @@ -135,6 +137,17 @@ async function postWater(request: NextRequest): Promise { // matching the mood / medication / measurement posture. invalidateUserDashboardSnapshot(user.id); + // A health-data write earns a durable row, like every other one. This was + // the last manual write in the nutrients family that only reached the wide + // event, which expires; the audit trail is what answers "who put this here" + // months later, and with a delegate able to act on a shared record that + // question has a second possible answer. + await auditLog("nutrient.water.write", { + userId: user.id, + ipAddress: getClientIp(request), + details: { day, mode, amountMl, resultingAmount: row.amount }, + }); + annotate({ action: { name: "nutrient.water.write" }, meta: { mode, day, amount_ml: amountMl }, diff --git a/src/components/labs/biomarker-manager.tsx b/src/components/labs/biomarker-manager.tsx index f2f024393..699a5d06d 100644 --- a/src/components/labs/biomarker-manager.tsx +++ b/src/components/labs/biomarker-manager.tsx @@ -26,8 +26,14 @@ import type { BiomarkerDto, BiomarkerListResponse } from "./types"; * Lives on the Labs page (not Settings) because the catalog IS the Labs * feature's primary object. Each marker renders as a single compact row * (name + unit inline, reference range / panel on a muted sub-line) with a - * hide toggle, an edit button (reusing the define/edit sheet), and a delete - * (the `onDelete: SetNull` FK keeps existing readings, just unlinks them). + * hide toggle, an edit button (reusing the define/edit sheet), and a delete. + * + * Delete removes the marker AND every reading under it, in one transaction — + * this line used to claim the `onDelete: SetNull` FK left the readings behind + * as unlinked rows, which stopped being true when delete became a "drop this + * and its history" action and the confirmation copy was rewritten in all six + * locales to say so. Hide is the reversible path and is one row above it, so + * the destructive one is deliberately the destructive one. * * "Hide" is a soft remove: a marker the user no longer needs leaves the * active list and the lab-entry pickers but keeps its readings + canonical diff --git a/src/components/labs/lab-biomarker-detail.tsx b/src/components/labs/lab-biomarker-detail.tsx index d57b42bd7..16442a653 100644 --- a/src/components/labs/lab-biomarker-detail.tsx +++ b/src/components/labs/lab-biomarker-detail.tsx @@ -266,9 +266,12 @@ export function LabBiomarkerDetail({ biomarkerId }: { biomarkerId: string }) { } // v1.18.9 (#41/#3) — delete the biomarker directly from its detail page so - // a stray marker is removable without a Settings detour. `onDelete: SetNull` - // keeps the readings and unlinks them. On success, invalidate the catalog + - // result list and return to the labs surface. + // a stray marker is removable without a Settings detour. This removes every + // reading under the marker along with it; the line here used to claim + // `onDelete: SetNull` kept them, which stopped being true when delete became + // a "drop this and its history" action. The confirmation dialog states it in + // all six locales, and Hide is the reversible alternative. On success, + // invalidate the catalog + result list and return to the labs surface. const deleteMarker = useMutation({ mutationFn: () => apiDelete(`/api/biomarkers/${biomarkerId}`), onSuccess: () => { diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 187b362f5..5c711ad16 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -648,10 +648,23 @@ async function readActingCarrier(auth: AuthContext): Promise { /** * Record a refused delegation. * - * Written for the OWNER's benefit as much as the operator's: "someone tried to - * open my record and was turned away" is the question the sharing panel exists - * to answer. The row names the actor (the caller), the account they aimed at, - * and why it failed — none of which reaches the response. + * The row is filed under the CALLER, with no actor, and it is the operator's + * trail rather than the owner's. It names who called, the account they claimed, + * and why the claim was refused — none of which reaches the response. + * + * It is deliberately not filed under the claimed account, and that decision is + * worth stating because the opposite reads as the obvious one. A row is only + * written for a HEADER refusal, and a header is an unverified per-request + * assertion: the value is whatever the caller typed. Filing under it would let + * any authenticated caller write rows into any account's activity feed by + * naming it, which is a spam vector aimed at exactly the panel that exists to + * be trustworthy. Verifying the claim first would mean a grant lookup on a path + * whose whole job is to refuse without touching the record. + * + * What the owner does see is the other half: `recordDelegatedAccess` files a + * per-day row under the OWNER, naming the actor, for every delegated read that + * succeeded. "Who reached my record" is answered there. "Whose client sent a + * malformed claim" is an operator question and stays here. * * Fire-and-forget: an audit write that fails must not convert a 403 into a 500, * because the 403 is the part that protects the record.