fix(api): address review findings — N+1, idempotency cache safety, device hijack guard

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) <noreply@anthropic.com>

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();

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,

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<string, typeof allEvents>();
+    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({

src/lib/idempotency.ts

@@ -79,9 +79,10 @@ async function findCached(ctx: IdempotencyContext): Promise<NextResponse | null>
 async function persistCached(
   ctx: IdempotencyContext,
   response: Response,
+  preReadBody?: string,
 ): Promise<void> {
-  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;
   };