From d25e50eaf3e4281529874aeebbaf3a57b0dd59ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Mon, 11 May 2026 02:27:47 +0200 Subject: [PATCH] fix(devices): wrap revoke cascade in a single transaction Both DELETE /api/devices/[id] and DELETE /api/auth/me/devices/[id] performed the same four-write cascade (refresh-token lookup, refresh revoke, access-token revoke, device delete) as separate Prisma calls. A partial failure between the third and fourth write left the device row alive with all its tokens revoked - the user's iPad showed in the device list as "still registered" but every request returned 401. Extract the cascade into a shared revokeDeviceCascade() helper and wrap the writes in prisma.$transaction so they commit atomically. Both routes now call the helper; the audit-log via tag and the wide event action name stay route-local. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../me/devices/[id]/__tests__/route.test.ts | 5 + src/app/api/auth/me/devices/[id]/route.ts | 58 +++---- .../api/devices/[id]/__tests__/route.test.ts | 5 + src/app/api/devices/[id]/route.ts | 57 ++----- src/lib/devices/__tests__/revoke.test.ts | 149 ++++++++++++++++++ src/lib/devices/revoke.ts | 87 ++++++++++ 6 files changed, 281 insertions(+), 80 deletions(-) create mode 100644 src/lib/devices/__tests__/revoke.test.ts create mode 100644 src/lib/devices/revoke.ts diff --git a/src/app/api/auth/me/devices/[id]/__tests__/route.test.ts b/src/app/api/auth/me/devices/[id]/__tests__/route.test.ts index d8a5628f3..556461b84 100644 --- a/src/app/api/auth/me/devices/[id]/__tests__/route.test.ts +++ b/src/app/api/auth/me/devices/[id]/__tests__/route.test.ts @@ -14,6 +14,10 @@ vi.mock("@/lib/db", () => ({ apiToken: { updateMany: vi.fn(), }, + // v1.4.23 W6 — the cascade now runs through `prisma.$transaction` + // (HIGH 6 fix). The mock returns the resolved batch payload so the + // route handler walks past the transaction step. + $transaction: vi.fn().mockResolvedValue([] as never), }, })); @@ -58,6 +62,7 @@ beforeEach(() => { vi.mocked(prisma.refreshToken.findMany).mockResolvedValue([]); vi.mocked(prisma.refreshToken.updateMany).mockResolvedValue({ count: 0 } as never); vi.mocked(prisma.apiToken.updateMany).mockResolvedValue({ count: 0 } as never); + vi.mocked(prisma.$transaction).mockResolvedValue([] as never); }); describe("DELETE /api/auth/me/devices/[id]", () => { diff --git a/src/app/api/auth/me/devices/[id]/route.ts b/src/app/api/auth/me/devices/[id]/route.ts index a5661d7a0..5dfc03aab 100644 --- a/src/app/api/auth/me/devices/[id]/route.ts +++ b/src/app/api/auth/me/devices/[id]/route.ts @@ -16,13 +16,18 @@ * the session lives on `Session.id`, not `Device.id`. Browser logouts * keep using /api/auth/logout. The two surfaces address different * blast radii. + * + * v1.4.23 W6 — the four-write cascade now runs through the shared + * `revokeDeviceCascade` helper so the writes commit atomically inside + * a single `prisma.$transaction`. Previously a partial failure could + * leave the device row alive with all its tokens revoked. */ import { NextRequest } from "next/server"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiError, apiSuccess } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; -import { prisma } from "@/lib/db"; +import { revokeDeviceCascade } from "@/lib/devices/revoke"; interface RouteContext { params: Promise<{ id: string }>; @@ -33,62 +38,35 @@ export const DELETE = apiHandler( const { user } = await requireAuth(); const { id } = await context.params; - const device = await prisma.device.findUnique({ - where: { id }, - select: { id: true, userId: true, model: true, bundleId: true }, - }); - - if (!device || device.userId !== user.id) { + const result = await revokeDeviceCascade(user.id, id); + if (!result) { // 404 on cross-user attempts — leaking "this id exists but isn't // yours" would let an attacker enumerate device ids. return apiError("Device not found", 404); } - // Revoke every refresh token bound to this device + its paired - // access tokens. - const liveRefreshTokens = await prisma.refreshToken.findMany({ - where: { userId: user.id, deviceId: device.id, revokedAt: null }, - select: { accessTokenHash: true }, - }); - const revokedAt = new Date(); - await prisma.refreshToken.updateMany({ - where: { userId: user.id, deviceId: device.id, revokedAt: null }, - data: { revokedAt }, - }); - const accessHashes = liveRefreshTokens - .map((r) => r.accessTokenHash) - .filter((v): v is string => Boolean(v)); - if (accessHashes.length > 0) { - await prisma.apiToken.updateMany({ - where: { tokenHash: { in: accessHashes }, revoked: false }, - data: { revoked: true }, - }); - } - - await prisma.device.delete({ where: { id: device.id } }); - await auditLog("devices.revoke", { userId: user.id, details: { - deviceId: device.id, - label: device.model ?? device.bundleId ?? null, - refreshTokensRevoked: liveRefreshTokens.length, - accessTokensRevoked: accessHashes.length, + deviceId: result.id, + label: result.label, + refreshTokensRevoked: result.refreshTokensRevoked, + accessTokensRevoked: result.accessTokensRevoked, }, }); annotate({ - action: { name: "auth.me.devices.revoke", entity_type: "device", entity_id: device.id }, + action: { name: "auth.me.devices.revoke", entity_type: "device", entity_id: result.id }, meta: { - refresh_tokens_revoked: liveRefreshTokens.length, - access_tokens_revoked: accessHashes.length, + refresh_tokens_revoked: result.refreshTokensRevoked, + access_tokens_revoked: result.accessTokensRevoked, }, }); return apiSuccess({ - id: device.id, + id: result.id, revoked: true, - refreshTokensRevoked: liveRefreshTokens.length, - accessTokensRevoked: accessHashes.length, + refreshTokensRevoked: result.refreshTokensRevoked, + accessTokensRevoked: result.accessTokensRevoked, }); }, ); diff --git a/src/app/api/devices/[id]/__tests__/route.test.ts b/src/app/api/devices/[id]/__tests__/route.test.ts index e2661f97c..436b625f7 100644 --- a/src/app/api/devices/[id]/__tests__/route.test.ts +++ b/src/app/api/devices/[id]/__tests__/route.test.ts @@ -14,6 +14,10 @@ vi.mock("@/lib/db", () => ({ apiToken: { updateMany: vi.fn(), }, + // v1.4.23 W6 — the cascade now runs through `prisma.$transaction` + // (HIGH 6 fix). The mock returns the resolved batch payload so the + // route handler walks past the transaction step. + $transaction: vi.fn().mockResolvedValue([] as never), }, })); @@ -58,6 +62,7 @@ beforeEach(() => { vi.mocked(prisma.refreshToken.findMany).mockResolvedValue([]); vi.mocked(prisma.refreshToken.updateMany).mockResolvedValue({ count: 0 } as never); vi.mocked(prisma.apiToken.updateMany).mockResolvedValue({ count: 0 } as never); + vi.mocked(prisma.$transaction).mockResolvedValue([] as never); }); describe("DELETE /api/devices/[id] (iOS token-rotation endpoint)", () => { diff --git a/src/app/api/devices/[id]/route.ts b/src/app/api/devices/[id]/route.ts index d84533628..b50fbd345 100644 --- a/src/app/api/devices/[id]/route.ts +++ b/src/app/api/devices/[id]/route.ts @@ -9,15 +9,17 @@ * The duplication is deliberate: the W3 open question called out * needing a native-friendly URL for the iOS client's APNs rotation * loop, and the `/api/auth/me/...` path is built for the settings- - * surface listing. Two URLs, one implementation: the underlying - * Prisma calls are identical to keep the audit trail consistent. + * surface listing. Two URLs share one transactional helper + * (`revokeDeviceCascade`) so the cascade stays atomic — the W6 audit + * collapsed the previous duplicated four-write sequence into a single + * `prisma.$transaction` call. */ import { NextRequest } from "next/server"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiError, apiSuccess } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; -import { prisma } from "@/lib/db"; +import { revokeDeviceCascade } from "@/lib/devices/revoke"; interface RouteContext { params: Promise<{ id: string }>; @@ -28,59 +30,34 @@ export const DELETE = apiHandler( const { user } = await requireAuth(); const { id } = await context.params; - const device = await prisma.device.findUnique({ - where: { id }, - select: { id: true, userId: true, model: true, bundleId: true }, - }); - - if (!device || device.userId !== user.id) { + const result = await revokeDeviceCascade(user.id, id); + if (!result) { return apiError("Device not found", 404); } - const liveRefreshTokens = await prisma.refreshToken.findMany({ - where: { userId: user.id, deviceId: device.id, revokedAt: null }, - select: { accessTokenHash: true }, - }); - const revokedAt = new Date(); - await prisma.refreshToken.updateMany({ - where: { userId: user.id, deviceId: device.id, revokedAt: null }, - data: { revokedAt }, - }); - const accessHashes = liveRefreshTokens - .map((r) => r.accessTokenHash) - .filter((v): v is string => Boolean(v)); - if (accessHashes.length > 0) { - await prisma.apiToken.updateMany({ - where: { tokenHash: { in: accessHashes }, revoked: false }, - data: { revoked: true }, - }); - } - - await prisma.device.delete({ where: { id: device.id } }); - await auditLog("devices.revoke", { userId: user.id, details: { - deviceId: device.id, - label: device.model ?? device.bundleId ?? null, - refreshTokensRevoked: liveRefreshTokens.length, - accessTokensRevoked: accessHashes.length, + deviceId: result.id, + label: result.label, + refreshTokensRevoked: result.refreshTokensRevoked, + accessTokensRevoked: result.accessTokensRevoked, via: "ios.rotation", }, }); annotate({ - action: { name: "devices.revoke", entity_type: "device", entity_id: device.id }, + action: { name: "devices.revoke", entity_type: "device", entity_id: result.id }, meta: { - refresh_tokens_revoked: liveRefreshTokens.length, - access_tokens_revoked: accessHashes.length, + refresh_tokens_revoked: result.refreshTokensRevoked, + access_tokens_revoked: result.accessTokensRevoked, }, }); return apiSuccess({ - id: device.id, + id: result.id, revoked: true, - refreshTokensRevoked: liveRefreshTokens.length, - accessTokensRevoked: accessHashes.length, + refreshTokensRevoked: result.refreshTokensRevoked, + accessTokensRevoked: result.accessTokensRevoked, }); }, ); diff --git a/src/lib/devices/__tests__/revoke.test.ts b/src/lib/devices/__tests__/revoke.test.ts new file mode 100644 index 000000000..0d27d6280 --- /dev/null +++ b/src/lib/devices/__tests__/revoke.test.ts @@ -0,0 +1,149 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +// v1.4.23 W6 (HIGH 6) — verify the device-revoke cascade wraps every +// write inside `prisma.$transaction` so a partial failure doesn't +// leave the device row alive with all its tokens revoked. Also pins +// the cross-user 404 path (returns null) and the no-access-token +// short-circuit (no apiToken.updateMany call when there are no live +// tokens). + +vi.mock("@/lib/db", () => ({ + prisma: { + device: { + findUnique: vi.fn(), + delete: vi.fn(), + }, + refreshToken: { + findMany: vi.fn(), + updateMany: vi.fn(), + }, + apiToken: { + updateMany: vi.fn(), + }, + $transaction: vi.fn(), + }, +})); + +import { prisma } from "@/lib/db"; +import { revokeDeviceCascade } from "../revoke"; + +beforeEach(() => { + vi.resetAllMocks(); +}); + +describe("revokeDeviceCascade", () => { + it("returns null and runs no writes when the device id is unknown", async () => { + vi.mocked(prisma.device.findUnique).mockResolvedValue(null); + + const result = await revokeDeviceCascade("user-1", "missing-id"); + + expect(result).toBeNull(); + expect(vi.mocked(prisma.$transaction)).not.toHaveBeenCalled(); + expect(vi.mocked(prisma.refreshToken.updateMany)).not.toHaveBeenCalled(); + }); + + it("returns null on cross-user attempts (caller is not the owner)", async () => { + vi.mocked(prisma.device.findUnique).mockResolvedValue({ + id: "dev-1", + userId: "other-user", + model: "iPhone 15", + bundleId: "io.healthlog.ios", + } as never); + + const result = await revokeDeviceCascade("user-1", "dev-1"); + + expect(result).toBeNull(); + expect(vi.mocked(prisma.$transaction)).not.toHaveBeenCalled(); + }); + + it("wraps the cascade in a single $transaction and reports counts", async () => { + vi.mocked(prisma.device.findUnique).mockResolvedValue({ + id: "dev-1", + userId: "user-1", + model: "iPhone 15", + bundleId: "io.healthlog.ios", + } as never); + vi.mocked(prisma.refreshToken.findMany).mockResolvedValue([ + { accessTokenHash: "hash-a" }, + { accessTokenHash: "hash-b" }, + { accessTokenHash: null }, + ] as never); + + // Track which prisma method calls the helper queues into the + // transaction array. We don't actually execute them; the helper + // hands the array to `$transaction` and the contract is "all of + // these commit atomically or none do". + vi.mocked(prisma.refreshToken.updateMany).mockReturnValue( + "REFRESH_UPDATE" as never, + ); + vi.mocked(prisma.apiToken.updateMany).mockReturnValue( + "API_TOKEN_UPDATE" as never, + ); + vi.mocked(prisma.device.delete).mockReturnValue("DEVICE_DELETE" as never); + vi.mocked(prisma.$transaction).mockResolvedValue([] as never); + + const result = await revokeDeviceCascade("user-1", "dev-1"); + + expect(result).toEqual({ + id: "dev-1", + label: "iPhone 15", + refreshTokensRevoked: 3, + accessTokensRevoked: 2, + }); + expect(vi.mocked(prisma.$transaction)).toHaveBeenCalledTimes(1); + const txArg = vi.mocked(prisma.$transaction).mock.calls[0]?.[0]; + expect(Array.isArray(txArg)).toBe(true); + expect(txArg).toEqual([ + "REFRESH_UPDATE", + "API_TOKEN_UPDATE", + "DEVICE_DELETE", + ]); + }); + + it("omits the apiToken.updateMany write when no access-token hashes exist", async () => { + vi.mocked(prisma.device.findUnique).mockResolvedValue({ + id: "dev-2", + userId: "user-1", + model: null, + bundleId: "io.healthlog.ios", + } as never); + vi.mocked(prisma.refreshToken.findMany).mockResolvedValue([ + { accessTokenHash: null }, + ] as never); + + vi.mocked(prisma.refreshToken.updateMany).mockReturnValue( + "REFRESH_UPDATE" as never, + ); + vi.mocked(prisma.device.delete).mockReturnValue("DEVICE_DELETE" as never); + vi.mocked(prisma.$transaction).mockResolvedValue([] as never); + + const result = await revokeDeviceCascade("user-1", "dev-2"); + + expect(result?.accessTokensRevoked).toBe(0); + expect(vi.mocked(prisma.apiToken.updateMany)).not.toHaveBeenCalled(); + const txArg = vi.mocked(prisma.$transaction).mock.calls[0]?.[0]; + expect(txArg).toEqual(["REFRESH_UPDATE", "DEVICE_DELETE"]); + }); + + it("propagates transaction failure (rollback path) — no swallowed error", async () => { + vi.mocked(prisma.device.findUnique).mockResolvedValue({ + id: "dev-3", + userId: "user-1", + model: "iPad", + bundleId: null, + } as never); + vi.mocked(prisma.refreshToken.findMany).mockResolvedValue([ + { accessTokenHash: "hash-c" }, + ] as never); + vi.mocked(prisma.refreshToken.updateMany).mockReturnValue("R" as never); + vi.mocked(prisma.apiToken.updateMany).mockReturnValue("A" as never); + vi.mocked(prisma.device.delete).mockReturnValue("D" as never); + vi.mocked(prisma.$transaction).mockRejectedValue( + new Error("postgres-blip"), + ); + + await expect( + revokeDeviceCascade("user-1", "dev-3"), + ).rejects.toThrow("postgres-blip"); + }); +}); diff --git a/src/lib/devices/revoke.ts b/src/lib/devices/revoke.ts new file mode 100644 index 000000000..f31db5804 --- /dev/null +++ b/src/lib/devices/revoke.ts @@ -0,0 +1,87 @@ +/** + * v1.4.23 W6 (HIGH 6 / S-01) — shared device-revoke cascade. + * + * Both `DELETE /api/auth/me/devices/[id]` and `DELETE /api/devices/[id]` + * historically performed the same four-write cascade (refresh-token + * lookup → refresh revoke → access-token revoke → device delete) as + * separate Prisma calls. Without a transaction, a partial failure + * (Postgres connection blip, app crash) left the device row alive + * with all its tokens revoked — the user's iPad showed in the device + * list as "still registered" but every request returned 401, and the + * only fix was admin intervention. + * + * `revokeDeviceCascade` wraps the four writes in `prisma.$transaction` + * so the cascade is atomic. Both routes call this helper and only + * differ in the audit-log `details.via` slot + the wide-event action + * name they emit afterwards (those stay route-local because they + * carry surface-specific context). + * + * Returns `null` when the device doesn't exist or belongs to another + * user (the route turns this into a 404 — leaking "this id exists but + * isn't yours" would let an attacker enumerate device ids). + */ +import { prisma } from "@/lib/db"; + +export interface RevokeDeviceCascadeResult { + id: string; + label: string | null; + refreshTokensRevoked: number; + accessTokensRevoked: number; +} + +export async function revokeDeviceCascade( + userId: string, + deviceId: string, +): Promise { + // Ownership lookup outside the transaction — the cascade only runs + // for an owned, existing device, so resolving the row first keeps + // the transactional window tight. + const device = await prisma.device.findUnique({ + where: { id: deviceId }, + select: { id: true, userId: true, model: true, bundleId: true }, + }); + + if (!device || device.userId !== userId) { + return null; + } + + const liveRefreshTokens = await prisma.refreshToken.findMany({ + where: { userId, deviceId: device.id, revokedAt: null }, + select: { accessTokenHash: true }, + }); + const accessHashes = liveRefreshTokens + .map((r) => r.accessTokenHash) + .filter((v): v is string => Boolean(v)); + const revokedAt = new Date(); + + // Build the write set as a `$transaction` array so all writes + // commit together or none of them do. Avoids the "tokens revoked + // but device row alive" half-state the W6 review flagged. + // The array uses `unknown` because Prisma's overloaded + // $transaction type narrows on the literal array shape; the + // operations are still strongly typed at construction. + const writes: unknown[] = [ + prisma.refreshToken.updateMany({ + where: { userId, deviceId: device.id, revokedAt: null }, + data: { revokedAt }, + }), + ]; + if (accessHashes.length > 0) { + writes.push( + prisma.apiToken.updateMany({ + where: { tokenHash: { in: accessHashes }, revoked: false }, + data: { revoked: true }, + }), + ); + } + writes.push(prisma.device.delete({ where: { id: device.id } })); + + await prisma.$transaction(writes as never); + + return { + id: device.id, + label: device.model ?? device.bundleId ?? null, + refreshTokensRevoked: liveRefreshTokens.length, + accessTokensRevoked: accessHashes.length, + }; +}