From 18691d455029d430159a755f3a91e6478f71a6e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 25 Jul 2026 18:41:40 +0200 Subject: [PATCH] fix(medications): scope the intake idempotency key to its owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `medication_intake_events.idempotency_key` carried a GLOBAL unique from 0001_init. The value is client-supplied, so two people running the same client can mint the same key — and on a global unique the second one is simply refused by the constraint. Their dose is not written and nothing tells them so. The bulk drain made it worse: its replay probe read the key with no user scope, resolved the first user's row, and reported the second user's entry as a duplicate carrying a stranger's row id. Migration 0273 replaces the global unique with `(user_id, idempotency_key)`, the grain every other client-supplied identity on the table already uses. Both unscoped reads in the bulk drain now key on the composite; the other four reads were already narrowed by user. On existing data this is a relaxation, so nothing needs reconciling: the old constraint is strictly stronger than the new one, and no pair of rows can violate an index the previous one already forbade. The migration still carries a reconciliation step for a database whose global index was dropped by hand at some point. It never deletes or rewrites a dose — it clears the dedup HINT on the later row of a colliding pair and keeps every intake event, timestamp, attribution and inventory stamp intact. The new index is created before the old one is dropped, so a failure aborts with the old protection still in place. A structural guard freezes the read side: no `medicationIntakeEvent` query may filter on the key without narrowing by `userId`. --- .../migration.sql | 47 +++ prisma/schema.prisma | 8 +- .../external-id-stability-guard.test.ts | 76 +++++ src/app/api/medications/intake/bulk/route.ts | 17 +- .../medications-intake-bulk.test.ts | 283 ++++++++++++++++++ 5 files changed, 428 insertions(+), 3 deletions(-) create mode 100644 prisma/migrations/0273_scope_intake_idempotency_key_per_user/migration.sql diff --git a/prisma/migrations/0273_scope_intake_idempotency_key_per_user/migration.sql b/prisma/migrations/0273_scope_intake_idempotency_key_per_user/migration.sql new file mode 100644 index 000000000..c4aba2094 --- /dev/null +++ b/prisma/migrations/0273_scope_intake_idempotency_key_per_user/migration.sql @@ -0,0 +1,47 @@ +-- Scope the medication-intake idempotency key per user. +-- +-- `medication_intake_events.idempotency_key` has carried a GLOBAL unique +-- index since 0001_init. The key is client-supplied, so a global unique +-- means one user's key can block another user's: the second user's write +-- is refused by the constraint and the dose is silently lost, and the +-- bulk route's unscoped replay probe resolves the key to the FIRST user's +-- row and hands its id back to the second user. +-- +-- The correct grain is per user, matching every other client-supplied +-- identity on this table (`(user_id, external_id)`, migration 0239). +-- +-- On existing data this is a RELAXATION: the old constraint is strictly +-- stronger than the new one — if no two rows anywhere share a key, then +-- no two rows of the SAME user share one either — so no row can violate +-- the new index and nothing needs reconciling. Step 1 below is a safety +-- net for a database whose global index was dropped by hand at some +-- point; against an intact schema it matches zero rows. +-- +-- Step 1 never deletes or rewrites a dose. It only clears the dedup HINT +-- on the strictly later row of a colliding pair, keeping every intake +-- event, its timestamps, its attribution and its inventory stamp intact. +-- The cost is that one row loses replay protection it never really had. + +-- 1. Safety net: break any pre-existing same-user key collision by +-- clearing the key on every row but the earliest of its group. +UPDATE "medication_intake_events" AS e +SET "idempotency_key" = NULL +WHERE e."idempotency_key" IS NOT NULL + AND EXISTS ( + SELECT 1 + FROM "medication_intake_events" AS earlier + WHERE earlier."user_id" = e."user_id" + AND earlier."idempotency_key" = e."idempotency_key" + AND ( + earlier."created_at" < e."created_at" + OR (earlier."created_at" = e."created_at" AND earlier."id" < e."id") + ) + ); + +-- 2. Create the per-user unique BEFORE dropping the global one, so a +-- failure here aborts the migration with the old protection intact. +CREATE UNIQUE INDEX "medication_intake_events_user_id_idempotency_key_key" + ON "medication_intake_events" ("user_id", "idempotency_key"); + +-- 3. Retire the global unique. +DROP INDEX IF EXISTS "medication_intake_events_idempotency_key_key"; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index e2d69c31f..fd31c7cf6 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -2757,7 +2757,12 @@ model MedicationIntakeEvent { /// nightly dedup leaves both standalone. attributionSource IntakeAttributionSource @default(AUTO) @map("attribution_source") source IntakeSource @default(WEB) - idempotencyKey String? @unique @map("idempotency_key") + /// v1.32.37 — client-supplied replay anchor, unique PER USER (migration + /// 0273). It carried a GLOBAL unique from 0001_init, which let one + /// user's key refuse another user's write outright and let an unscoped + /// lookup resolve to a stranger's row. Same grain as `(user_id, + /// external_id)` below: every read must key on `(userId, idempotencyKey)`. + idempotencyKey String? @map("idempotency_key") createdAt DateTime @default(now()) @map("created_at") /// v1.4.25 W4d — only populated for GLP-1 intake events (the picker /// is the only surface that writes this column). Existing rows stay @@ -2829,6 +2834,7 @@ model MedicationIntakeEvent { /// unique above, migration 0121), so the schema keeps the full /// `@@unique` for the `userId_externalId` compound where-key. @@unique([userId, externalId]) + @@unique([userId, idempotencyKey]) @@index([userId, medicationId, scheduledFor]) // v1.7.0 sync — keyset support for the `/api/sync/changes` intake feed // (`where userId` ordered by `(updatedAt asc, id asc)`). diff --git a/src/__tests__/external-id-stability-guard.test.ts b/src/__tests__/external-id-stability-guard.test.ts index 88b279fb1..8e3abe525 100644 --- a/src/__tests__/external-id-stability-guard.test.ts +++ b/src/__tests__/external-id-stability-guard.test.ts @@ -298,6 +298,82 @@ describe("identities derived outside a Zod schema stay inside the floor", () => }); }); +describe("the intake idempotency key is never read across users", () => { + /** + * `medication_intake_events.idempotency_key` carried a GLOBAL unique + * from 0001_init until migration 0273. Client-supplied keys on a global + * unique meant one user's key blocked another user's write outright, + * and an unscoped lookup resolved a stranger's row and handed back its + * id. The column is now unique per user; every read has to key on + * `(userId, idempotencyKey)` or the leak comes straight back. + */ + function intakeQueriesFilteringOnTheKey(): Array<{ + file: string; + line: number; + scoped: boolean; + }> { + const out: Array<{ file: string; line: number; scoped: boolean }> = []; + for (const rel of sourceFiles()) { + const source = read(rel); + if (!source.includes("medicationIntakeEvent.")) continue; + const call = /medicationIntakeEvent\.(\w+)\(\s*\{/g; + let m: RegExpExecArray | null; + while ((m = call.exec(source)) !== null) { + // Walk to the matching brace so the window is the call, not a + // fixed slice that could bleed into the next statement. + let depth = 0; + let j = source.indexOf("{", m.index); + for (; j < source.length; j++) { + if (source[j] === "{") depth++; + else if (source[j] === "}" && --depth === 0) break; + } + const body = source.slice(m.index, j + 1); + const whereAt = body.indexOf("where"); + if (whereAt === -1) continue; + const where = body.slice(whereAt, whereAt + 400); + if (!where.includes("idempotencyKey")) continue; + out.push({ + file: rel, + line: source.slice(0, m.index).split("\n").length, + // Keyed on the row's own primary id is scoped by construction — + // that is an update-by-id that happens to WRITE the key. + scoped: where.includes("userId") || /where:\s*\{\s*id:/.test(where), + }); + } + } + return out; + } + + it("finds the reads at all (the matcher still matches)", () => { + const found = intakeQueriesFilteringOnTheKey(); + expect(found.length).toBeGreaterThanOrEqual(6); + expect(found.map((f) => f.file)).toContain( + "app/api/medications/intake/bulk/route.ts", + ); + }); + + it("every query filtering on the key also narrows by userId", () => { + const unscoped = intakeQueriesFilteringOnTheKey().filter((f) => !f.scoped); + expect( + unscoped, + "a medicationIntakeEvent query filters on idempotencyKey without a userId. " + + "The key is client-supplied and unique per USER (migration 0273); an " + + "unscoped read resolves another user's row.", + ).toEqual([]); + }); + + it("the schema keeps the key unique per user, not globally", () => { + const schema = readFileSync( + join(process.cwd(), "prisma", "schema.prisma"), + "utf8", + ); + expect(schema).toContain("@@unique([userId, idempotencyKey])"); + expect(schema).not.toMatch( + /idempotencyKey\s+String\?\s+@unique\s+@map\("idempotency_key"\)/, + ); + }); +}); + describe("the batch surfaces refuse per entry, never whole-batch", () => { // Each of these drains a client sync queue. A schema-level refusal here // would stop a whole batch of good rows because of one bad one, which is diff --git a/src/app/api/medications/intake/bulk/route.ts b/src/app/api/medications/intake/bulk/route.ts index c969f17da..996cab697 100644 --- a/src/app/api/medications/intake/bulk/route.ts +++ b/src/app/api/medications/intake/bulk/route.ts @@ -616,8 +616,16 @@ async function postBulk(request: NextRequest): Promise { // by `source`. The partial unique index carries `source`, so it // cannot catch this duplicate; the probe must. if (entry.idempotencyKey) { + // Scoped to the caller. The key is client-supplied, so an + // unscoped read would resolve another user's row and hand its + // id back here (migration 0273 made the unique per user). const replay = await prisma.medicationIntakeEvent.findUnique({ - where: { idempotencyKey: entry.idempotencyKey }, + where: { + userId_idempotencyKey: { + userId: user.id, + idempotencyKey: entry.idempotencyKey, + }, + }, select: { id: true }, }); if (replay) { @@ -793,7 +801,12 @@ async function postBulk(request: NextRequest): Promise { ) { const existing = entry.idempotencyKey ? await prisma.medicationIntakeEvent.findUnique({ - where: { idempotencyKey: entry.idempotencyKey }, + where: { + userId_idempotencyKey: { + userId: user.id, + idempotencyKey: entry.idempotencyKey, + }, + }, select: { id: true }, }) : // v1.28 — an Apple entry that raced the partial diff --git a/tests/integration/medications-intake-bulk.test.ts b/tests/integration/medications-intake-bulk.test.ts index 8feac204a..886e2dc90 100644 --- a/tests/integration/medications-intake-bulk.test.ts +++ b/tests/integration/medications-intake-bulk.test.ts @@ -483,4 +483,287 @@ describe("POST /api/medications/intake/bulk (real Postgres)", () => { }); expect(stored).toHaveLength(1); }); + + /** + * Migration 0273 — the idempotency key is unique PER USER. + * + * It carried a GLOBAL unique from 0001_init. Because the key is + * client-supplied, two people running the same client could mint the + * same one, and then the global constraint refused the second person's + * write outright: their dose was silently lost. The unscoped replay + * probe made it worse by resolving the key to the FIRST user's row and + * handing that stranger's row id back. + * + * These two cases are the whole contract: different users must not + * collide, and one user replaying their own key must still dedup. + */ + it("lets two different users post the SAME idempotency key — both doses land", async () => { + const { POST } = await import("@/app/api/medications/intake/bulk/route"); + const prisma = getPrismaClient(); + const SHARED_KEY = "ios-outbox-2026-05-16T08:00"; + + // User A posts first. + const first = await POST( + makeRequest({ + entries: [ + { + medicationId: ownedMedId, + scheduledFor: "2026-05-16T08:00:00.000Z", + takenAt: "2026-05-16T08:02:00.000Z", + idempotencyKey: SHARED_KEY, + }, + ], + }), + ); + expect(first.status).toBe(200); + expect( + ((await first.json()) as { data: { inserted: number } }).data.inserted, + ).toBe(1); + + // Now the OTHER user, with their own session and their own medication, + // posts the very same key. + const otherSession = await prisma.session.create({ + data: { + userId: OTHER_USER_ID, + expiresAt: new Date(Date.now() + 365 * 24 * 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", otherSession.id); + + const second = await POST( + makeRequest({ + entries: [ + { + medicationId: foreignMedId, + scheduledFor: "2026-05-16T08:00:00.000Z", + takenAt: "2026-05-16T08:03:00.000Z", + idempotencyKey: SHARED_KEY, + }, + ], + }), + ); + expect(second.status).toBe(200); + const secondJson = (await second.json()) as { + data: { + inserted: number; + duplicates: number; + entries: Array<{ status: string; id?: string }>; + }; + }; + // The second user's dose LANDS. Pre-0273 it was reported `duplicate` + // against the first user's row and never written. + expect(secondJson.data.inserted).toBe(1); + expect(secondJson.data.duplicates).toBe(0); + + const mine = await prisma.medicationIntakeEvent.findMany({ + where: { userId: OTHER_USER_ID }, + select: { id: true, userId: true, idempotencyKey: true }, + }); + const theirs = await prisma.medicationIntakeEvent.findMany({ + where: { userId: TEST_USER_ID }, + select: { id: true }, + }); + expect(mine).toHaveLength(1); + expect(theirs).toHaveLength(1); + expect(mine[0].idempotencyKey).toBe(SHARED_KEY); + // And no id belonging to the first user was echoed to the second. + const echoed = secondJson.data.entries[0]?.id; + if (echoed) expect(echoed).not.toBe(theirs[0].id); + }); + + it("still dedups when the SAME user replays their own key", async () => { + const { POST } = await import("@/app/api/medications/intake/bulk/route"); + const prisma = getPrismaClient(); + const KEY = "ios-outbox-replay-001"; + const body = { + entries: [ + { + medicationId: ownedMedId, + scheduledFor: "2026-05-18T08:00:00.000Z", + takenAt: "2026-05-18T08:02:00.000Z", + idempotencyKey: KEY, + }, + ], + }; + + const first = await POST(makeRequest(body)); + expect( + ((await first.json()) as { data: { inserted: number } }).data.inserted, + ).toBe(1); + + const replay = await POST(makeRequest(body)); + const replayJson = (await replay.json()) as { + data: { + inserted: number; + duplicates: number; + entries: Array<{ status: string; id?: string }>; + }; + }; + expect(replayJson.data.inserted).toBe(0); + expect(replayJson.data.duplicates).toBe(1); + + const stored = await prisma.medicationIntakeEvent.findMany({ + where: { userId: TEST_USER_ID, idempotencyKey: KEY }, + select: { id: true }, + }); + expect(stored).toHaveLength(1); + // The replay resolves to the caller's OWN row. + expect(replayJson.data.entries[0]?.id).toBe(stored[0].id); + }); + + it("holds the per-user unique at the database, not only in the route", async () => { + const prisma = getPrismaClient(); + const KEY = "db-level-shared-key"; + + await prisma.medicationIntakeEvent.create({ + data: { + userId: TEST_USER_ID, + medicationId: ownedMedId, + scheduledFor: new Date("2026-05-19T08:00:00.000Z"), + takenAt: new Date("2026-05-19T08:00:00.000Z"), + source: "WEB", + idempotencyKey: KEY, + }, + }); + + // Same key, different user — allowed now, refused before 0273. + await expect( + prisma.medicationIntakeEvent.create({ + data: { + userId: OTHER_USER_ID, + medicationId: foreignMedId, + scheduledFor: new Date("2026-05-19T08:00:00.000Z"), + takenAt: new Date("2026-05-19T08:00:00.000Z"), + source: "WEB", + idempotencyKey: KEY, + }, + }), + ).resolves.toBeTruthy(); + + // Same key, SAME user — still refused. + await expect( + prisma.medicationIntakeEvent.create({ + data: { + userId: TEST_USER_ID, + medicationId: ownedMedId, + scheduledFor: new Date("2026-05-20T08:00:00.000Z"), + takenAt: new Date("2026-05-20T08:00:00.000Z"), + source: "WEB", + idempotencyKey: KEY, + }, + }), + ).rejects.toMatchObject({ code: "P2002" }); + }); + + /** + * Migration 0273 step 1 — the reconciliation safety net. + * + * Against an intact database it can never fire: the global unique it + * replaces is strictly stronger than the per-user one, so no colliding + * pair can exist. It is there for a database whose global index was + * dropped by hand at some point. That means nothing else in this suite + * exercises it, so this case builds the collision on purpose and runs + * the SHIPPED SQL — read from the migration file, not a copy that could + * drift — against it. + * + * The whole thing runs inside a transaction that is rolled back, so the + * index surgery never escapes into the rest of the run. + */ + it("migration 0273 reconciles a pre-existing same-user collision without losing a dose", async () => { + const prisma = getPrismaClient(); + const { readFileSync } = await import("node:fs"); + const { join } = await import("node:path"); + + const sql = readFileSync( + join( + process.cwd(), + "prisma", + "migrations", + "0273_scope_intake_idempotency_key_per_user", + "migration.sql", + ), + "utf8", + ); + // Strip comments BEFORE splitting: the prose contains semicolons of + // its own, and splitting first would slice a statement in half. + const statements = sql + .split("\n") + .filter((line) => !line.trim().startsWith("--")) + .join("\n") + .split(";") + .map((part) => part.trim()) + .filter(Boolean); + const safetyNet = statements.find((st) => st.startsWith("UPDATE")); + const createIndex = statements.find((st) => + st.startsWith("CREATE UNIQUE INDEX"), + ); + expect( + safetyNet, + "migration must carry the reconciliation UPDATE", + ).toBeTruthy(); + expect(createIndex, "migration must carry the per-user index").toBeTruthy(); + + const COLLIDING_KEY = "legacy-collision-key"; + let observed: Array<{ id: string; idempotencyKey: string | null }> = []; + + class Rollback extends Error {} + await expect( + prisma.$transaction(async (tx) => { + // Simulate the only database on which the collision is possible: + // one whose unique index was dropped by hand. + await tx.$executeRawUnsafe( + 'DROP INDEX "medication_intake_events_user_id_idempotency_key_key"', + ); + + const earlier = await tx.medicationIntakeEvent.create({ + data: { + id: "collide-earlier", + userId: TEST_USER_ID, + medicationId: ownedMedId, + scheduledFor: new Date("2026-05-21T08:00:00.000Z"), + takenAt: new Date("2026-05-21T08:00:00.000Z"), + source: "WEB", + idempotencyKey: COLLIDING_KEY, + createdAt: new Date("2026-05-21T08:00:00.000Z"), + }, + }); + const later = await tx.medicationIntakeEvent.create({ + data: { + id: "collide-later", + userId: TEST_USER_ID, + medicationId: ownedMedId, + scheduledFor: new Date("2026-05-22T08:00:00.000Z"), + takenAt: new Date("2026-05-22T08:00:00.000Z"), + source: "WEB", + idempotencyKey: COLLIDING_KEY, + createdAt: new Date("2026-05-22T08:00:00.000Z"), + }, + }); + + // Run the migration's own reconciliation, then its own index. + await tx.$executeRawUnsafe(safetyNet!); + await tx.$executeRawUnsafe(createIndex!); + + observed = await tx.medicationIntakeEvent.findMany({ + where: { id: { in: [earlier.id, later.id] } }, + orderBy: { createdAt: "asc" }, + select: { id: true, idempotencyKey: true }, + }); + throw new Rollback(); + }), + ).rejects.toBeInstanceOf(Rollback); + + // Both doses survive — the migration never deletes or rewrites a row. + expect(observed).toHaveLength(2); + expect(observed[0]).toEqual({ + id: "collide-earlier", + idempotencyKey: COLLIDING_KEY, + }); + // Only the dedup HINT on the later row is cleared, which is what makes + // the unique index creatable. + expect(observed[1]).toEqual({ + id: "collide-later", + idempotencyKey: null, + }); + }); });