From d1732fe63e97459842f7ec0ba276b7378502a4c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 23 May 2026 10:11:48 +0200 Subject: [PATCH] refactor(withings): atomic callback nonce consumption + surfaced delete failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The callback previously read the ledger row then deleted it in two round-trips, contradicting its own docstring's atomicity promise. Concurrent callbacks with the same nonce could both pass findUnique before either issued delete. Replace with delete-first + catch P2025 for the replay branch — atomic at the Postgres level. Silent .catch(() => {}) on delete failures now surfaces via Wide-Event warning. Closes M3 + M7 in v1.4.48 backlog. --- .../withings/callback/__tests__/route.test.ts | 257 ++++++++++++++++++ src/app/api/withings/callback/route.ts | 103 ++++--- 2 files changed, 317 insertions(+), 43 deletions(-) create mode 100644 src/app/api/withings/callback/__tests__/route.test.ts diff --git a/src/app/api/withings/callback/__tests__/route.test.ts b/src/app/api/withings/callback/__tests__/route.test.ts new file mode 100644 index 000000000..a671c4bdc --- /dev/null +++ b/src/app/api/withings/callback/__tests__/route.test.ts @@ -0,0 +1,257 @@ +/** + * v1.4.48 M3 — unit-level coverage for the atomic nonce-consumption + * branch. + * + * The integration tests in `tests/integration/withings-oauth*.test.ts` + * already pin the full handshake against real Postgres (happy path, + * replay, expired, cross-user). This file specifically targets the + * delete-first race window the M3 refactor closed: two concurrent + * callbacks with the same nonce — the first wins, the second hits + * Prisma P2025 and lands on the replay branch. + * + * Co-tests M7 — non-P2025 delete failures (DB-connection drop / + * integrity violation) surface via Wide-Event `addWarning` instead + * of being silently swallowed. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, + requireAuth: vi.fn(async () => ({ + user: { id: "user-1" }, + session: { id: "sess-1" }, + })), + HttpError: class HttpError extends Error { + constructor( + public status: number, + message: string, + ) { + super(message); + } + }, +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + withingsOAuthState: { + delete: vi.fn(), + }, + withingsConnection: { + upsert: vi.fn(), + }, + }, +})); + +vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); + +vi.mock("@/lib/crypto", () => ({ + encrypt: vi.fn((s: string) => `enc(${s})`), +})); + +vi.mock("@/lib/withings/client", () => ({ + exchangeCode: vi.fn(), + WITHINGS_OAUTH_SCOPE: "user.metrics,user.activity", +})); + +vi.mock("@/lib/withings/credentials", () => ({ + getUserWithingsCredentials: vi.fn(), +})); + +vi.mock("@/lib/withings/sync", () => ({ + setupWebhook: vi.fn(() => Promise.resolve()), +})); + +vi.mock("@/lib/integrations/status", () => ({ + markReconnected: vi.fn(), +})); + +const addWarning = vi.fn(); +vi.mock("@/lib/logging/context", () => ({ + annotate: vi.fn(), + getEvent: vi.fn(() => ({ addWarning, setError: vi.fn() })), +})); + +import { GET } from "../route"; +import { prisma } from "@/lib/db"; +import { Prisma } from "@/generated/prisma/client"; + +const NONCE = "abcdefghijklmnopqrstuv"; // 22-char base64url +const APP_URL = "http://localhost:3000"; + +function callbackRequest(state: string, cookieState: string): NextRequest { + return new NextRequest( + `http://localhost/api/withings/callback?code=auth-code-1&state=${encodeURIComponent(state)}`, + { + method: "GET", + headers: { + cookie: `withings_state=${cookieState}`, + }, + }, + ); +} + +beforeEach(() => { + vi.clearAllMocks(); + process.env.NEXT_PUBLIC_APP_URL = APP_URL; +}); + +describe("withings/callback atomic nonce consumption (M3)", () => { + it("concurrent callbacks with the same nonce — only one succeeds, the second hits the P2025 replay branch", async () => { + // First callback: delete returns the row (win the race). + // Second callback: delete throws P2025 (row already consumed). + const winningRow = { + nonce: NONCE, + userId: "user-1", + expiresAt: new Date(Date.now() + 10 * 60 * 1000), + createdAt: new Date(), + }; + const p2025 = new Prisma.PrismaClientKnownRequestError( + "An operation failed because it depends on one or more records that were required but not found.", + { code: "P2025", clientVersion: "x" }, + ); + + const deleteMock = vi.mocked(prisma.withingsOAuthState.delete); + deleteMock + .mockResolvedValueOnce(winningRow as never) + .mockRejectedValueOnce(p2025); + + // Stub the rest of the happy path for the winning request — code + // exchange returns plausible tokens so the upsert + redirect run. + const { getUserWithingsCredentials } = await import( + "@/lib/withings/credentials" + ); + vi.mocked(getUserWithingsCredentials).mockResolvedValue({ + clientId: "cid", + clientSecret: "csec", + } as never); + const { exchangeCode } = await import("@/lib/withings/client"); + vi.mocked(exchangeCode).mockResolvedValue({ + userid: "wuid", + access_token: "atok", + refresh_token: "rtok", + expires_in: 3600, + } as never); + + // Fire both legs concurrently. The mock's call ordering is FIFO, + // so whichever `await` hits the Prisma mock first gets the + // success result. Both legs use the same nonce + cookie value. + const reqA = callbackRequest(NONCE, NONCE); + const reqB = callbackRequest(NONCE, NONCE); + const [resA, resB] = await Promise.all([GET(reqA), GET(reqB)]); + + // Both responses are 307 redirects. + expect(resA.status).toBe(307); + expect(resB.status).toBe(307); + + // Exactly one delete-attempt per leg — the atomic shape collapses + // findUnique + delete into a single round-trip. + expect(deleteMock).toHaveBeenCalledTimes(2); + + // Exactly one leg landed on `withings=connected`, exactly one + // landed on the replay branch (`withings=error&reason=state`). + const locations = [resA, resB] + .map((r) => r.headers.get("location")) + .sort(); + expect(locations).toHaveLength(2); + expect( + locations.some((l) => l?.includes("withings=connected")), + ).toBe(true); + expect( + locations.some((l) => l?.includes("withings=error&reason=state")), + ).toBe(true); + + // P2025 must NOT surface as a warning — it's the expected + // replay-branch signal, not a real infra problem. + expect(addWarning).not.toHaveBeenCalled(); + }); + + it("non-P2025 delete failure (e.g. connection drop) surfaces a Wide-Event warning and bounces to the error page", async () => { + // M7 — a real infra failure on the consume-delete must not be + // silently swallowed. + const infraError = new Error("connection terminated unexpectedly"); + vi.mocked(prisma.withingsOAuthState.delete).mockRejectedValueOnce( + infraError, + ); + + const req = callbackRequest(NONCE, NONCE); + const res = await GET(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + + // The warning carries the error string so the audit trail can + // distinguish "row was gone" (P2025, no warning) from "DB unhappy" + // (warning, infra signal). + expect(addWarning).toHaveBeenCalledTimes(1); + expect(addWarning).toHaveBeenCalledWith( + expect.stringContaining("oauth-state-delete failed"), + ); + expect(addWarning).toHaveBeenCalledWith( + expect.stringContaining("connection terminated unexpectedly"), + ); + }); + + it("delete succeeds but row has expired — validity check trips, no token exchange", async () => { + const expiredRow = { + nonce: NONCE, + userId: "user-1", + expiresAt: new Date(Date.now() - 60_000), + createdAt: new Date(), + }; + vi.mocked(prisma.withingsOAuthState.delete).mockResolvedValueOnce( + expiredRow as never, + ); + + const { exchangeCode } = await import("@/lib/withings/client"); + + const req = callbackRequest(NONCE, NONCE); + const res = await GET(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + // Single atomic round-trip consumed the row; no token exchange. + expect(exchangeCode).not.toHaveBeenCalled(); + }); + + it("delete succeeds but row's userId mismatches session — validity check trips, no token exchange", async () => { + const crossUserRow = { + nonce: NONCE, + userId: "user-OTHER", + expiresAt: new Date(Date.now() + 10 * 60 * 1000), + createdAt: new Date(), + }; + vi.mocked(prisma.withingsOAuthState.delete).mockResolvedValueOnce( + crossUserRow as never, + ); + + const { exchangeCode } = await import("@/lib/withings/client"); + + const req = callbackRequest(NONCE, NONCE); + const res = await GET(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + expect(exchangeCode).not.toHaveBeenCalled(); + }); + + it("URL state vs cookie state mismatch short-circuits BEFORE the atomic delete fires", async () => { + // CSRF leg 1 — must reject without touching the ledger so a + // probe can't grief legitimate rows by issuing deletes for + // nonces it doesn't legitimately hold the cookie for. + const req = callbackRequest("aaaaaaaaaaaaaaaaaaaaaa", "bbbbbbbbbbbbbbbbbbbbbb"); + const res = await GET(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + expect(prisma.withingsOAuthState.delete).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/withings/callback/route.ts b/src/app/api/withings/callback/route.ts index 9b7ee0319..32fe6c4d5 100644 --- a/src/app/api/withings/callback/route.ts +++ b/src/app/api/withings/callback/route.ts @@ -8,6 +8,7 @@ import { getUserWithingsCredentials } from "@/lib/withings/credentials"; import { WITHINGS_OAUTH_STATE_COOKIE } from "@/lib/withings/oauth-state"; import { setupWebhook } from "@/lib/withings/sync"; import { markReconnected } from "@/lib/integrations/status"; +import { Prisma } from "@/generated/prisma/client"; import { NextRequest, NextResponse } from "next/server"; import { timingSafeEqual } from "node:crypto"; @@ -23,6 +24,17 @@ import { timingSafeEqual } from "node:crypto"; * runs. The row is consumed (deleted) on every exit branch so a * replay of the same nonce fails the second time. * + * v1.4.48 M3 — the row is now consumed via a single delete-first + * round-trip. Prisma `delete` returns the row on success, so the + * post-delete validity checks (`expiresAt`, `userId`) still run + * against the consumed payload. Two concurrent callbacks with the + * same nonce now race at the Postgres row level — only one delete + * wins, the other hits P2025 ("record not found") and is treated as + * a replay, bounced to the error page. Previously the `findUnique + + * separate delete` shape left a tiny RTT window where both legs + * could pass the lookup before either issued the delete, contrary + * to the docstring's atomicity promise. + * * Closes the v1.4.43 security audit L-1 finding: the cookie now * carries only the opaque nonce, so a future refactor flipping it to * non-httpOnly cannot leak the user id into request logs / network @@ -39,7 +51,10 @@ export const GET = apiHandler(async (request: NextRequest) => { // CSRF check, leg 1: URL state must match the cookie state byte-for-byte. // Timing-safe comparison so a probe can't recover the cookie bytes via - // response-time analysis. + // response-time analysis. This runs BEFORE the atomic delete so a + // probe with arbitrary state values can't grief legitimate ledger + // rows by triggering deletes for nonces it doesn't legitimately + // hold the cookie for. if ( !state || !storedState || @@ -54,28 +69,51 @@ export const GET = apiHandler(async (request: NextRequest) => { ); } - // CSRF check, leg 2: the nonce must resolve to a live, unexpired - // ledger row whose `userId` matches the authenticated session. - // Resolving the user from the row (rather than parsing the cookie - // value) is the v1.4.43 audit recommendation — a future refactor - // flipping the cookie to non-httpOnly cannot leak the user id this - // way. - const stateRow = await prisma.withingsOAuthState.findUnique({ - where: { nonce: state }, - }); - if ( - !stateRow || - stateRow.expiresAt <= new Date() || - stateRow.userId !== user.id - ) { - // Single-use: stamp out the row whether the expiry tripped, the - // user mismatched, or we found nothing. The user will retry from - // `withings/connect` with a fresh nonce. - if (stateRow) { - await prisma.withingsOAuthState - .delete({ where: { nonce: state } }) - .catch(() => {}); + // CSRF check, leg 2: atomically consume the ledger row. Prisma + // `delete` returns the row on success so `expiresAt` + `userId` + // checks still run against the consumed payload. P2025 ("record + // not found") means the nonce was already consumed by a prior + // callback (a replay) or never existed — both land on the same + // error redirect. Atomic at the Postgres row level: two concurrent + // callbacks with the same nonce can no longer both pass the + // lookup before either issues the delete. + let stateRow: { userId: string; expiresAt: Date } | null = null; + try { + stateRow = await prisma.withingsOAuthState.delete({ + where: { nonce: state }, + }); + } catch (err) { + if ( + err instanceof Prisma.PrismaClientKnownRequestError && + err.code === "P2025" + ) { + // Row already consumed (replay) or never existed. + return NextResponse.redirect( + new URL( + "/settings/integrations?withings=error&reason=state", + process.env.NEXT_PUBLIC_APP_URL!, + ), + ); } + // A real infra problem (connection drop, integrity violation) + // shouldn't be silently swallowed. Surface it to the Wide Event + // so the audit trail captures it, then bounce to the error + // page. Re-throwing would land on the apiHandler's 500 path and + // strand the user without a clean redirect. + getEvent()?.addWarning(`oauth-state-delete failed: ${err}`); + return NextResponse.redirect( + new URL( + "/settings/integrations?withings=error&reason=state", + process.env.NEXT_PUBLIC_APP_URL!, + ), + ); + } + + // Validity checks against the consumed row. The row is already + // gone from the ledger by this point — single-use semantics — + // whether the expiry tripped, the userId mismatched, or + // everything looked good. + if (stateRow.expiresAt <= new Date() || stateRow.userId !== user.id) { return NextResponse.redirect( new URL( "/settings/integrations?withings=error&reason=state", @@ -85,11 +123,6 @@ export const GET = apiHandler(async (request: NextRequest) => { } if (!code) { - // Consume the row even on the no-code path so the nonce can't - // be replayed by an attacker who scraped the redirect URL. - await prisma.withingsOAuthState - .delete({ where: { nonce: state } }) - .catch(() => {}); return NextResponse.redirect( new URL( "/settings/integrations?withings=error&reason=nocode", @@ -101,9 +134,6 @@ export const GET = apiHandler(async (request: NextRequest) => { try { const creds = await getUserWithingsCredentials(user.id); if (!creds) { - await prisma.withingsOAuthState - .delete({ where: { nonce: state } }) - .catch(() => {}); return NextResponse.redirect( new URL( "/settings/integrations?withings=error&reason=nocreds", @@ -156,13 +186,6 @@ export const GET = apiHandler(async (request: NextRequest) => { // don't write a fresh success-time — that's the next sync's job. await markReconnected(user.id, "withings"); - // Consume the ledger row — single-use semantics. From this point - // on a replay of the same `state` lands on the not-found branch - // above and redirects to the error page. - await prisma.withingsOAuthState - .delete({ where: { nonce: state } }) - .catch(() => {}); - const response = NextResponse.redirect( new URL( "/settings/integrations?withings=connected", @@ -173,12 +196,6 @@ export const GET = apiHandler(async (request: NextRequest) => { return response; } catch (err) { getEvent()?.setError(err); - // Consume the row on the failure path too — the user is going to - // retry from `withings/connect` which mints a fresh nonce, and a - // stranded row buys us nothing. - await prisma.withingsOAuthState - .delete({ where: { nonce: state } }) - .catch(() => {}); return NextResponse.redirect( new URL( "/settings/integrations?withings=error&reason=token",