diff --git a/prisma/schema.prisma b/prisma/schema.prisma index e972d56af..5509b6aeb 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -4560,8 +4560,8 @@ model GoogleHealthOAuthState { /// sends it to the same place instead of to an error page. Holds no user id, /// token or code: `location` is the app's own result URL (`?oura=connected`, /// `?whoop=error&reason=…`). Rows outlive the state they guard by a few -/// minutes and are swept by expiry on the next settle -/// (`src/lib/oauth/callback-claim.ts`). +/// minutes and are swept by expiry when a callback completes and by the daily +/// OAuth-state cleanup (`src/lib/jobs/oauth-callback-claim-cleanup.ts`). model OAuthCallbackClaim { /// sha256(provider + NUL + state), hex. key String @id diff --git a/src/app/api/oura/callback/__tests__/route.test.ts b/src/app/api/oura/callback/__tests__/route.test.ts index 898390087..2806299e4 100644 --- a/src/app/api/oura/callback/__tests__/route.test.ts +++ b/src/app/api/oura/callback/__tests__/route.test.ts @@ -3,13 +3,17 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; // Single-use claiming is covered against Postgres in // tests/integration/oauth-callback-single-use.test.ts; here every callback // is the first and only one. +const { claimSpy } = vi.hoisted(() => ({ claimSpy: vi.fn() })); vi.mock("@/lib/oauth/callback-claim", () => ({ runOAuthCallbackOnce: ( _p: string, _s: string, _o: unknown, run: () => Promise, - ) => run(), + ) => { + claimSpy(); + return run(); + }, settleOAuthCallback: (_p: string, _s: string, run: () => Promise) => run(), consumeLedgerState: async ( @@ -227,3 +231,36 @@ describe("GET /api/oura/callback — connect-time first sync", () => { expect(res.headers.get("location")).toContain("oura=connected"); }); }); + +// A refused callback must not spend the state: Oura sends a denied consent +// back without a code, and the person may return to the same authorize URL +// and allow it. +describe("GET /api/oura/callback — claim only what can be exchanged", () => { + it("does not claim the state for a callback without a code", async () => { + const res = await GET(makeReq({ state: "S", cookie: "S" })); + expect(res.headers.get("location")).toContain("reason=nocode"); + expect(claimSpy).not.toHaveBeenCalled(); + }); + + it("does not claim the state for a foreign issuer or a cross-user session", async () => { + await GET( + makeReq({ + code: "c", + state: "S", + cookie: "S", + iss: "https://evil.example/oauth", + }), + ); + (getSession as ReturnType).mockResolvedValue({ + user: { id: "attacker" }, + }); + await GET(makeReq({ code: "c", state: "S", cookie: "S" })); + expect(claimSpy).not.toHaveBeenCalled(); + }); + + it("claims the state once before exchanging a valid code", async () => { + await GET(makeReq({ code: "c", state: "S", cookie: "S" })); + expect(claimSpy).toHaveBeenCalledTimes(1); + expect(exchangeMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/app/api/oura/callback/route.ts b/src/app/api/oura/callback/route.ts index 8871b3a12..6e7b7ed20 100644 --- a/src/app/api/oura/callback/route.ts +++ b/src/app/api/oura/callback/route.ts @@ -70,23 +70,12 @@ export const GET = apiHandler(async (request: NextRequest) => { } const userId = verified.userId; - return runOAuthCallbackOnce( - "oura", - state, - { - fallbackUrl: SETTINGS_URL(), - stateCookie: oauthStateCookieName("oura"), - }, - () => completeCallback(request, userId), - ); -}); - -async function completeCallback( - request: NextRequest, - userId: string, -): Promise { - const { searchParams } = new URL(request.url); - const code = searchParams.get("code"); + // Everything that can refuse the callback without the code runs before + // the claim, so a refused callback (a denied consent arrives without a + // code) never spends the state: the same authorize URL still works. The + // claim stays strictly before the exchange. + const params = new URL(request.url).searchParams; + const code = params.get("code"); const evt = getEvent(); if (evt) evt.setAuth({ user_id: userId, auth_method: "session" }); @@ -102,12 +91,28 @@ async function completeCallback( // RFC 9207: an authorization server that names itself must be Oura's. The // issuer also tells which token endpoint the code belongs to. Older apps // send none, which is fine. - const iss = searchParams.get("iss"); + const iss = params.get("iss"); if (!isAcceptedOuraIssuer(iss)) { annotate({ meta: { reason: "issuer" } }); return ERR("issuer"); } + return runOAuthCallbackOnce( + "oura", + state, + { + fallbackUrl: SETTINGS_URL(), + stateCookie: oauthStateCookieName("oura"), + }, + () => completeCallback(userId, code, iss), + ); +}); + +async function completeCallback( + userId: string, + code: string, + iss: string | null, +): Promise { try { const creds = await getOuraClientCredentials(userId); if (!creds) return ERR("nocreds"); diff --git a/src/app/api/polar/callback/__tests__/route.test.ts b/src/app/api/polar/callback/__tests__/route.test.ts index 18e5c6847..9addbc134 100644 --- a/src/app/api/polar/callback/__tests__/route.test.ts +++ b/src/app/api/polar/callback/__tests__/route.test.ts @@ -25,13 +25,17 @@ const { // Single-use claiming is covered against Postgres in // tests/integration/oauth-callback-single-use.test.ts; here every callback // is the first and only one. +const { claimSpy } = vi.hoisted(() => ({ claimSpy: vi.fn() })); vi.mock("@/lib/oauth/callback-claim", () => ({ runOAuthCallbackOnce: ( _p: string, _s: string, _o: unknown, run: () => Promise, - ) => run(), + ) => { + claimSpy(); + return run(); + }, settleOAuthCallback: (_p: string, _s: string, run: () => Promise) => run(), consumeLedgerState: async ( @@ -78,9 +82,9 @@ import { GET } from "../route"; process.env.NEXT_PUBLIC_APP_URL = "https://app.example"; -function makeReq(): Parameters[0] { +function makeReq(query = "code=abc&state=s"): Parameters[0] { return { - url: "https://app.example/api/polar/callback?code=abc&state=s", + url: `https://app.example/api/polar/callback?${query}`, cookies: { get: () => ({ value: "s" }) }, } as unknown as Parameters[0]; } @@ -163,3 +167,14 @@ describe("GET /api/polar/callback — connect-time first sync", () => { expect(res.headers.get("location")).toContain("polar=connected"); }); }); + +describe("GET /api/polar/callback — claim only what can be exchanged", () => { + it("does not claim the state for a callback without a code or for another user", async () => { + const denied = await run(makeReq("state=s&error=access_denied")); + expect(denied.headers.get("location")).toContain("reason=nocode"); + getSessionMock.mockResolvedValue({ user: { id: "other" } }); + await run(makeReq()); + expect(claimSpy).not.toHaveBeenCalled(); + expect(exchangeCodeMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/polar/callback/route.ts b/src/app/api/polar/callback/route.ts index b18b1be34..cca2ff691 100644 --- a/src/app/api/polar/callback/route.ts +++ b/src/app/api/polar/callback/route.ts @@ -60,26 +60,10 @@ export const GET = apiHandler(async (request: NextRequest) => { } const userId = verified.userId; - // Single-use: a callback delivered twice exchanges its code once, and the - // duplicate follows the first request's result (src/lib/oauth/callback-claim.ts). - return runOAuthCallbackOnce( - "polar", - state, - { - fallbackUrl: new URL( - "/settings/integrations", - process.env.NEXT_PUBLIC_APP_URL!, - ).toString(), - stateCookie: oauthStateCookieName("polar"), - }, - () => completeCallback(request, userId), - ); -}); - -async function completeCallback( - request: NextRequest, - userId: string, -): Promise { + // Everything that can refuse the callback without the code runs before + // the claim, so a refused callback (a denied consent arrives without a + // code) never spends the state: the same authorize URL still works. The + // claim stays strictly before the exchange. const code = new URL(request.url).searchParams.get("code"); const evt = getEvent(); @@ -94,6 +78,26 @@ async function completeCallback( if (!code) return ERR("nocode"); + // Single-use: a callback delivered twice exchanges its code once, and the + // duplicate follows the first request's result (src/lib/oauth/callback-claim.ts). + return runOAuthCallbackOnce( + "polar", + state, + { + fallbackUrl: new URL( + "/settings/integrations", + process.env.NEXT_PUBLIC_APP_URL!, + ).toString(), + stateCookie: oauthStateCookieName("polar"), + }, + () => completeCallback(userId, code), + ); +}); + +async function completeCallback( + userId: string, + code: string, +): Promise { try { const creds = await getPolarClientCredentials(userId); if (!creds) return ERR("nocreds"); diff --git a/src/app/api/strava/callback/__tests__/route.test.ts b/src/app/api/strava/callback/__tests__/route.test.ts index 407cff3b5..c967bce51 100644 --- a/src/app/api/strava/callback/__tests__/route.test.ts +++ b/src/app/api/strava/callback/__tests__/route.test.ts @@ -28,13 +28,17 @@ const { // Single-use claiming is covered against Postgres in // tests/integration/oauth-callback-single-use.test.ts; here every callback // is the first and only one. +const { claimSpy } = vi.hoisted(() => ({ claimSpy: vi.fn() })); vi.mock("@/lib/oauth/callback-claim", () => ({ runOAuthCallbackOnce: ( _p: string, _s: string, _o: unknown, run: () => Promise, - ) => run(), + ) => { + claimSpy(); + return run(); + }, settleOAuthCallback: (_p: string, _s: string, run: () => Promise) => run(), consumeLedgerState: async ( @@ -85,11 +89,12 @@ import { GET } from "../route"; process.env.NEXT_PUBLIC_APP_URL = "https://app.example"; -function callback(): Promise<{ headers: { get(k: string): string | null } }> { - const req = new NextRequest( - "http://localhost/api/strava/callback?code=abc&state=signed-state", - { headers: { cookie: "strava_oauth_state=signed-state" } }, - ); +function callback( + query = "code=abc&state=signed-state", +): Promise<{ headers: { get(k: string): string | null } }> { + const req = new NextRequest(`http://localhost/api/strava/callback?${query}`, { + headers: { cookie: "strava_oauth_state=signed-state" }, + }); return (GET as unknown as (r: NextRequest) => Promise)(req); } @@ -127,3 +132,12 @@ describe("GET /api/strava/callback — S-1 backfill enqueue", () => { expect(bossSend).not.toHaveBeenCalled(); }); }); + +describe("GET /api/strava/callback — claim only what can be exchanged", () => { + it("does not claim the state for a callback without a code", async () => { + const res = await callback("state=signed-state&error=access_denied"); + expect(res.headers.get("location")).toContain("reason=nocode"); + expect(claimSpy).not.toHaveBeenCalled(); + expect(exchangeCodeMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/app/api/strava/callback/route.ts b/src/app/api/strava/callback/route.ts index 807c36d4c..1fc7ddc2b 100644 --- a/src/app/api/strava/callback/route.ts +++ b/src/app/api/strava/callback/route.ts @@ -60,26 +60,10 @@ export const GET = apiHandler(async (request: NextRequest) => { } const userId = verified.userId; - // Single-use: a callback delivered twice exchanges its code once, and the - // duplicate follows the first request's result (src/lib/oauth/callback-claim.ts). - return runOAuthCallbackOnce( - "strava", - state, - { - fallbackUrl: new URL( - "/settings/integrations", - process.env.NEXT_PUBLIC_APP_URL!, - ).toString(), - stateCookie: oauthStateCookieName("strava"), - }, - () => completeCallback(request, userId), - ); -}); - -async function completeCallback( - request: NextRequest, - userId: string, -): Promise { + // Everything that can refuse the callback without the code runs before + // the claim, so a refused callback (a denied consent arrives without a + // code) never spends the state: the same authorize URL still works. The + // claim stays strictly before the exchange. const code = new URL(request.url).searchParams.get("code"); const evt = getEvent(); @@ -93,6 +77,26 @@ async function completeCallback( if (!code) return ERR("nocode"); + // Single-use: a callback delivered twice exchanges its code once, and the + // duplicate follows the first request's result (src/lib/oauth/callback-claim.ts). + return runOAuthCallbackOnce( + "strava", + state, + { + fallbackUrl: new URL( + "/settings/integrations", + process.env.NEXT_PUBLIC_APP_URL!, + ).toString(), + stateCookie: oauthStateCookieName("strava"), + }, + () => completeCallback(userId, code), + ); +}); + +async function completeCallback( + userId: string, + code: string, +): Promise { try { const creds = await getStravaClientCredentials(userId); if (!creds) return ERR("nocreds"); diff --git a/src/lib/jobs/__tests__/oauth-callback-claim-cleanup.test.ts b/src/lib/jobs/__tests__/oauth-callback-claim-cleanup.test.ts new file mode 100644 index 000000000..b226a850d --- /dev/null +++ b/src/lib/jobs/__tests__/oauth-callback-claim-cleanup.test.ts @@ -0,0 +1,77 @@ +/** + * Expired OAuth callback claims are swept by the daily OAuth-state cleanup, + * not only when a callback completes: a quiet instance may not see another + * connect for weeks. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const h = vi.hoisted(() => ({ + withingsCleanup: vi.fn(), + claimDeleteMany: vi.fn(), + meta: {} as Record, +})); + +vi.mock("@/lib/jobs/withings-oauth-state-cleanup", () => ({ + cleanupExpiredWithingsOAuthStates: h.withingsCleanup, +})); +vi.mock("../reminder/shared", async (importOriginal) => ({ + ...(await importOriginal>()), + getWorkerPrisma: () => ({ + oAuthCallbackClaim: { deleteMany: h.claimDeleteMany }, + }), +})); +vi.mock("@/lib/logging/background", () => ({ + withBackgroundEvent: async ( + _name: string, + fn: (evt: { + addMeta: (k: string, v: unknown) => void; + addWarning: (w: string) => void; + }) => Promise, + ) => + fn({ + addMeta: (k, v) => { + h.meta[k] = v; + }, + addWarning: () => {}, + }), +})); + +import { cleanupExpiredOAuthCallbackClaims } from "../oauth-callback-claim-cleanup"; +import { handleWithingsOAuthStateCleanup } from "../reminder/cleanup-handlers"; + +beforeEach(() => { + vi.clearAllMocks(); + for (const key of Object.keys(h.meta)) delete h.meta[key]; + h.withingsCleanup.mockResolvedValue(0); + h.claimDeleteMany.mockResolvedValue({ count: 3 }); +}); + +describe("cleanupExpiredOAuthCallbackClaims", () => { + it("deletes the claims whose expiry has passed and returns the count", async () => { + const now = new Date("2026-10-21T03:20:00Z"); + const deleted = await cleanupExpiredOAuthCallbackClaims( + { oAuthCallbackClaim: { deleteMany: h.claimDeleteMany } } as never, + now, + ); + expect(deleted).toBe(3); + expect(h.claimDeleteMany).toHaveBeenCalledWith({ + where: { expiresAt: { lt: now } }, + }); + }); +}); + +describe("daily OAuth-state cleanup", () => { + it("sweeps expired callback claims alongside the Withings states", async () => { + await handleWithingsOAuthStateCleanup([]); + expect(h.withingsCleanup).toHaveBeenCalledTimes(1); + expect(h.claimDeleteMany).toHaveBeenCalledTimes(1); + expect(h.meta.oauth_callback_claim_cleanup_deleted).toBe(3); + }); + + it("fails the job when the claim sweep fails", async () => { + h.claimDeleteMany.mockRejectedValue(new Error("statement timeout")); + await expect(handleWithingsOAuthStateCleanup([])).resolves.toMatchObject({ + ok: false, + }); + }); +}); diff --git a/src/lib/jobs/job-outcome.ts b/src/lib/jobs/job-outcome.ts index d75c1b29f..1460c78e4 100644 --- a/src/lib/jobs/job-outcome.ts +++ b/src/lib/jobs/job-outcome.ts @@ -63,6 +63,8 @@ export const JOB_FACT_ALLOWLIST: ReadonlySet = new Set([ "buckets_recomputed", "budget_blocked", "cached", + // The daily Withings OAuth-state cleanup also sweeps the callback claims. + "callback_claims_deleted", "candidates_scanned", // The nightly sign-in ledger cleanup (`auth-ledger-cleanup`). "challenges_deleted", diff --git a/src/lib/jobs/oauth-callback-claim-cleanup.ts b/src/lib/jobs/oauth-callback-claim-cleanup.ts new file mode 100644 index 000000000..cd2ba7a5a --- /dev/null +++ b/src/lib/jobs/oauth-callback-claim-cleanup.ts @@ -0,0 +1,21 @@ +/** + * Daily sweep for `oauth_callback_claims`. + * + * A claim is taken when a connect callback is about to exchange its code + * (`src/lib/oauth/callback-claim.ts`) and is useless once its state has + * expired. The callback itself sweeps expired rows whenever one completes, so + * on a quiet instance the last few claims would stay until the next connect. + * This pass rides the daily Withings OAuth-state cleanup (03:20 Europe/Berlin) + * so the table empties on its own regardless. Idempotent. + */ +import type { PrismaClient } from "@/generated/prisma/client"; + +export async function cleanupExpiredOAuthCallbackClaims( + prisma: Pick, + now: Date = new Date(), +): Promise { + const { count } = await prisma.oAuthCallbackClaim.deleteMany({ + where: { expiresAt: { lt: now } }, + }); + return count; +} diff --git a/src/lib/jobs/reminder/cleanup-handlers.ts b/src/lib/jobs/reminder/cleanup-handlers.ts index e15318865..b5d28bedf 100644 --- a/src/lib/jobs/reminder/cleanup-handlers.ts +++ b/src/lib/jobs/reminder/cleanup-handlers.ts @@ -1,5 +1,5 @@ /** - * Retention and maintenance cleanups: rate-limit rows, idempotency keys, audit logs, OAuth states (Withings / WHOOP), mood-reminder events, push attempts, and measurement tombstones. + * Retention and maintenance cleanups: rate-limit rows, idempotency keys, audit logs, OAuth states (Withings / WHOOP) and OAuth callback claims, mood-reminder events, push attempts, and measurement tombstones. * * Extracted from reminder-worker.ts, which owns the queue names, cron * schedules, and the queue bindings. @@ -33,6 +33,7 @@ import { cleanupOldAuditLogs } from "@/lib/jobs/audit-log-cleanup"; import { cleanupOldCoachMessages } from "@/lib/jobs/coach-message-cleanup"; import { cleanupOldWorkoutInsightClaims } from "@/lib/jobs/workout-insight-claim-cleanup"; import { cleanupExpiredWithingsOAuthStates } from "@/lib/jobs/withings-oauth-state-cleanup"; +import { cleanupExpiredOAuthCallbackClaims } from "@/lib/jobs/oauth-callback-claim-cleanup"; import { cleanupExpiredOidcNativeHandoffs } from "@/lib/jobs/oidc-handoff-cleanup"; import { cleanupExpiredMeasurementTombstones, @@ -462,7 +463,12 @@ export async function handleWithingsOAuthStateCleanup( try { const deleted = await cleanupExpiredWithingsOAuthStates(p); evt.addMeta("withings_oauth_state_cleanup_deleted", deleted); - return jobDone({ deleted }); + // The provider-neutral callback claims ride this daily pass: the + // callbacks sweep them only when one completes, which a quiet + // instance may not see for weeks. + const claimsDeleted = await cleanupExpiredOAuthCallbackClaims(p); + evt.addMeta("oauth_callback_claim_cleanup_deleted", claimsDeleted); + return jobDone({ deleted, callback_claims_deleted: claimsDeleted }); } catch (err) { // The OAuth flow tolerates a stale row for an extra day, which used // to be the argument for swallowing this so the queue would not diff --git a/src/lib/oauth/callback-claim.ts b/src/lib/oauth/callback-claim.ts index dd88588d0..f9b0969a1 100644 --- a/src/lib/oauth/callback-claim.ts +++ b/src/lib/oauth/callback-claim.ts @@ -32,6 +32,7 @@ import { NextResponse } from "next/server"; import { prisma } from "@/lib/db"; import type { Prisma } from "@/generated/prisma/client"; import { annotate, getEvent } from "@/lib/logging/context"; +import { cleanupExpiredOAuthCallbackClaims } from "@/lib/jobs/oauth-callback-claim-cleanup"; /** Providers whose callbacks claim their state here. */ export type OAuthCallbackProvider = @@ -130,9 +131,7 @@ export async function settleOAuthCallback( : null, }, }); - await prisma.oAuthCallbackClaim.deleteMany({ - where: { expiresAt: { lt: now } }, - }); + await cleanupExpiredOAuthCallbackClaims(prisma, now); } catch (err) { getEvent()?.addWarning(`oauth callback claim settle failed: ${err}`); } diff --git a/tests/integration/oauth-callback-single-use.test.ts b/tests/integration/oauth-callback-single-use.test.ts index ec4e181e9..4c5a1b824 100644 --- a/tests/integration/oauth-callback-single-use.test.ts +++ b/tests/integration/oauth-callback-single-use.test.ts @@ -87,9 +87,13 @@ afterEach(() => { vi.restoreAllMocks(); }); -async function ouraCallback(state: string, code = "oura-code") { +async function ouraCallback(state: string, code: string | null = "oura-code") { const { GET } = await import("@/app/api/oura/callback/route"); - const params = new URLSearchParams({ code, state, iss: OURA_ISS }); + const params = new URLSearchParams( + code === null + ? { error: "access_denied", state, iss: OURA_ISS } + : { code, state, iss: OURA_ISS }, + ); return GET( new NextRequest(`http://localhost/api/oura/callback?${params}`, { method: "GET", @@ -221,6 +225,37 @@ describe("Oura callback delivered twice", () => { }); }); +describe("Oura consent denied, then allowed", () => { + it("still exchanges the code when the person returns to the same authorize URL", async () => { + let tokenCalls = 0; + vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { + if (urlOf(input) !== OURA_TOKEN_URL) throw new Error("unexpected"); + tokenCalls++; + return new Response( + JSON.stringify({ + access_token: "oura-access", + refresh_token: "oura-refresh", + expires_in: 86400, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + }); + + const state = await mintOuraState(); + // Deny: Oura returns without a code. Nothing is exchanged or claimed. + const denied = await ouraCallback(state, null); + expect(denied.headers.get("location")).toContain( + "oura=error&reason=nocode", + ); + expect(tokenCalls).toBe(0); + + // Allow on the same authorize URL: same state, now with a code. + const allowed = await ouraCallback(state, "allowed-code"); + expect(tokenCalls).toBe(1); + expect(allowed.headers.get("location")).toContain("oura=connected"); + }); +}); + describe("Withings callback delivered twice", () => { it("exchanges the code once and sends the duplicate where the first went", async () => { const prisma = getPrismaClient();