From b9ade5b519faaf928ab673acc34708e08b1d5236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 3 May 2026 22:08:53 +0200 Subject: [PATCH] =?UTF-8?q?fix(api):=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20N+1,=20idempotency=20cache=20safety,=20device=20hij?= =?UTF-8?q?ack=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes from the 5-way review on PR #117: 1. Insights cards: replace per-medication N+1 query with single batched findMany + JS bucketing. For users with many active medications this collapses 10+ sequential round-trips into one. 2. Idempotency cache: only persist 2xx/3xx and validation 4xx responses. 401/403/408/429/5xx are now skipped — a stale 401 (token expired mid-flight) or transient 5xx must not be replayed for 24h. Adds a defence-in-depth check that refuses to cache any body containing "hlk_" so a future caller wrapping an auth route in withIdempotency cannot leak a freshly-issued bearer token through the cache. 3. Devices: reject cross-user token re-registration with 409. Prior behaviour silently transferred ownership on findUnique-by-token, which would let anyone who learns/guesses an APNs token redirect another user's pushes. Audit-logs the denial with reason. Tests adjusted: device-hijack test now expects 409, plus a new test for same-user re-registration. 279/279 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/devices/__tests__/route.test.ts | 17 +++++++- src/app/api/devices/route.ts | 12 +++++- src/app/api/insights/cards/route.ts | 47 ++++++++++++--------- src/lib/idempotency.ts | 32 +++++++++++--- 4 files changed, 81 insertions(+), 27 deletions(-) diff --git a/src/app/api/devices/__tests__/route.test.ts b/src/app/api/devices/__tests__/route.test.ts index e260f6d10..06156b3e6 100644 --- a/src/app/api/devices/__tests__/route.test.ts +++ b/src/app/api/devices/__tests__/route.test.ts @@ -86,7 +86,7 @@ describe("POST /api/devices", () => { expect(prisma.device.update).not.toHaveBeenCalled(); }); - it("upserts when token already exists (transfers ownership)", async () => { + it("rejects cross-user re-registration with 409", async () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(prisma.device.findUnique).mockResolvedValue({ id: "dev-1", @@ -96,6 +96,21 @@ describe("POST /api/devices", () => { const res = await POST( req({ token: "abcd1234efgh5678", bundleId: "io.healthlog.app" }), ); + expect(res.status).toBe(409); + expect(prisma.device.update).not.toHaveBeenCalled(); + expect(prisma.device.create).not.toHaveBeenCalled(); + }); + + it("updates in place when same user re-registers their own token", async () => { + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.device.findUnique).mockResolvedValue({ + id: "dev-1", + userId: "user-1", + token: "abcd1234efgh5678", + } as never); + const res = await POST( + req({ token: "abcd1234efgh5678", bundleId: "io.healthlog.app" }), + ); expect(res.status).toBe(201); expect(prisma.device.update).toHaveBeenCalledTimes(1); expect(prisma.device.create).not.toHaveBeenCalled(); diff --git a/src/app/api/devices/route.ts b/src/app/api/devices/route.ts index b732d62d1..b5e5e1558 100644 --- a/src/app/api/devices/route.ts +++ b/src/app/api/devices/route.ts @@ -43,10 +43,20 @@ export const POST = apiHandler(async (request: NextRequest) => { const existing = await prisma.device.findUnique({ where: { token } }); let id: string; if (existing) { + // Cross-user-hijack guard: a device-token belongs to exactly one user. + // Re-registering the same token under a different account is rejected + // — APNs tokens aren't a secret, so trusting the wire input would let + // anyone who learns/guesses a token redirect another user's pushes. + if (existing.userId !== user.id) { + await auditLog("device.register.denied", { + userId: user.id, + details: { reason: "token_owned_by_other_user", deviceId: existing.id }, + }); + return apiError("Device token already registered to another account", 409); + } const updated = await prisma.device.update({ where: { token }, data: { - userId: user.id, platform: "ios", bundleId, locale: locale ?? null, diff --git a/src/app/api/insights/cards/route.ts b/src/app/api/insights/cards/route.ts index 645b68873..bb2d2a705 100644 --- a/src/app/api/insights/cards/route.ts +++ b/src/app/api/insights/cards/route.ts @@ -133,31 +133,38 @@ export const GET = apiHandler(async () => { compliance7: number; compliance30: number; }> = []; - for (const med of medications) { - if (med.schedules.length === 0) continue; - const events = await prisma.medicationIntakeEvent.findMany({ + // Single batched query — bucket by medicationId in JS (fix N+1). + const activeMeds = medications.filter((m) => m.schedules.length > 0); + if (activeMeds.length > 0) { + const allEvents = await prisma.medicationIntakeEvent.findMany({ where: { userId: user.id, - medicationId: med.id, + medicationId: { in: activeMeds.map((m) => m.id) }, scheduledFor: { gte: new Date(Date.now() - 30 * 86_400_000) }, }, - select: { takenAt: true, skipped: true, scheduledFor: true }, - }); - const takenLast7 = events.filter( - (e) => - e.takenAt !== null && - !e.skipped && - e.scheduledFor.getTime() >= Date.now() - 7 * 86_400_000, - ).length; - const taken30 = events.filter((e) => e.takenAt !== null && !e.skipped).length; - const expected7 = med.schedules.length * 7; - const expected30 = med.schedules.length * 30; - medicationCompliance.push({ - name: med.name, - compliance7: expected7 > 0 ? Math.round((takenLast7 / expected7) * 100) : 0, - compliance30: - expected30 > 0 ? Math.round((taken30 / expected30) * 100) : 0, + select: { medicationId: true, takenAt: true, skipped: true, scheduledFor: true }, }); + const eventsByMed = new Map(); + for (const e of allEvents) { + const arr = eventsByMed.get(e.medicationId) ?? []; + arr.push(e); + eventsByMed.set(e.medicationId, arr); + } + const cutoff7 = Date.now() - 7 * 86_400_000; + for (const med of activeMeds) { + const events = eventsByMed.get(med.id) ?? []; + const takenLast7 = events.filter( + (e) => e.takenAt !== null && !e.skipped && e.scheduledFor.getTime() >= cutoff7, + ).length; + const taken30 = events.filter((e) => e.takenAt !== null && !e.skipped).length; + const expected7 = med.schedules.length * 7; + const expected30 = med.schedules.length * 30; + medicationCompliance.push({ + name: med.name, + compliance7: expected7 > 0 ? Math.round((takenLast7 / expected7) * 100) : 0, + compliance30: expected30 > 0 ? Math.round((taken30 / expected30) * 100) : 0, + }); + } } const alerts = generateAlerts({ diff --git a/src/lib/idempotency.ts b/src/lib/idempotency.ts index 7324a50c4..d1fe6a612 100644 --- a/src/lib/idempotency.ts +++ b/src/lib/idempotency.ts @@ -79,9 +79,10 @@ async function findCached(ctx: IdempotencyContext): Promise async function persistCached( ctx: IdempotencyContext, response: Response, + preReadBody?: string, ): Promise { - const cloned = response.clone(); - const body = await cloned.text().catch(() => ""); + const body = + preReadBody ?? (await response.clone().text().catch(() => "")); await prisma.idempotencyKey .create({ @@ -142,9 +143,30 @@ export function withIdempotency< const response = await handler(...args); - // Persist regardless of status so error retries don't fork into - // two different responses (matches Stripe/Square semantics). - await persistCached(ctx, response); + // Cache only client-stable responses. Replaying a stale 401/403 + // (token expired mid-flight) or a 5xx would lock the user into a + // bogus result for the TTL window. 4xx-validation responses are + // intentionally cached so the same broken request doesn't hit the + // DB twice — but auth/throttle/server faults must not poison. + const cachable = + response.status < 400 || + (response.status >= 400 && + response.status < 500 && + response.status !== 401 && + response.status !== 403 && + response.status !== 408 && + response.status !== 429); + + if (cachable) { + // Defence-in-depth: never persist a body that carries a freshly-issued + // bearer token. Auth routes shouldn't be wrapped in withIdempotency to + // begin with, but if a future caller forgets, we refuse to leak. + const cloned = response.clone(); + const text = await cloned.text(); + if (!text.includes("hlk_")) { + await persistCached(ctx, response, text); + } + } return response; };