diff --git a/prisma/migrations/0076_v1447_withings_oauth_state/migration.sql b/prisma/migrations/0076_v1447_withings_oauth_state/migration.sql new file mode 100644 index 000000000..9db580c64 --- /dev/null +++ b/prisma/migrations/0076_v1447_withings_oauth_state/migration.sql @@ -0,0 +1,59 @@ +-- v1.4.47 W6 — Withings OAuth state nonce ledger. +-- +-- The v1.4.43 security audit (L-1) flagged the legacy state shape +-- `${user.id}:${random16}` as cosmetic-but-fragile: the user id is +-- recoverable from any request log entry where the cookie isn't +-- redacted, and from a network-traffic capture of the redirect. The +-- cookie is `httpOnly` + `Secure` + `sameSite:lax` today so the XSS +-- exfiltration surface is closed — but a future refactor that flips +-- the cookie to a non-httpOnly variant would silently expose the user +-- id. Audit recommendation was to switch the state to a fully-random +-- 16-byte nonce backed by a short-lived `(nonce → userId)` row. +-- +-- This table is that ledger. Each `withings/connect` mint persists one +-- row; `withings/callback` looks it up, asserts `expiresAt > now()`, +-- and DELETEs the row in a single transaction so a replay of the same +-- state nonce against a fresh user session fails the CSRF check (the +-- row is gone after first consumption). A daily cron sweeps any +-- abandoned rows whose `expiresAt` has passed (user closed the +-- Withings tab without completing the handshake). +-- +-- Schema choices: +-- +-- * `nonce` is the primary key — it's already random and +-- 22-character base64url, so a dedicated `id` column would just +-- duplicate the key without buying anything. +-- * `userId` carries `ON DELETE CASCADE` so removing a user cleans +-- up their in-flight nonces atomically. +-- * `expiresAt` is indexed for the cleanup sweep — without it the +-- daily `DELETE … WHERE expiresAt < now()` would seq-scan the +-- entire table once a day even though the typical row count is +-- bounded by `users × concurrent in-flight handshakes`. +-- +-- Idempotent guards (`IF NOT EXISTS`) match the 0067 / 0070 / 0071 / +-- 0074 / 0075 pattern so reruns are safe. +-- +-- Reversibility: down migration is `DROP TABLE IF EXISTS +-- "withings_oauth_states"`. Additive-only; no existing row is +-- touched. + +CREATE TABLE IF NOT EXISTS "withings_oauth_states" ( + "nonce" TEXT NOT NULL, + "user_id" TEXT NOT NULL, + "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "expires_at" TIMESTAMP(3) NOT NULL, + + CONSTRAINT "withings_oauth_states_pkey" PRIMARY KEY ("nonce") +); + +CREATE INDEX IF NOT EXISTS "withings_oauth_states_expires_at_idx" + ON "withings_oauth_states" ("expires_at"); + +DO $$ BEGIN + ALTER TABLE "withings_oauth_states" + ADD CONSTRAINT "withings_oauth_states_user_id_fkey" + FOREIGN KEY ("user_id") REFERENCES "users"("id") + ON DELETE CASCADE ON UPDATE CASCADE; +EXCEPTION + WHEN duplicate_object THEN NULL; +END $$; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 2b130ce5c..5c94f1d9b 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -234,6 +234,13 @@ model User { refreshTokens RefreshToken[] auditLogs AuditLog[] withingsConnection WithingsConnection? + // v1.4.47 W6 — short-lived `(nonce → userId)` ledger backing the + // Withings OAuth state check. The connect route mints a row with a + // 10-minute TTL; the callback resolves the user via this relation + // instead of parsing the cookie value, closing the L-1 audit gap + // where the legacy `${user.id}:${nonce}` shape leaked the user id + // into request logs / network captures. + withingsOAuthStates WithingsOAuthState[] moodEntries MoodEntry[] notificationChannels NotificationChannel[] pushSubscriptions PushSubscription[] @@ -1192,6 +1199,40 @@ model WithingsConnection { @@map("withings_connections") } +/// v1.4.47 W6 — short-lived ledger of in-flight OAuth state nonces. +/// +/// The legacy state shape `${user.id}:${random16}` (v1.4.42-) persisted +/// the user id in plaintext on both the URL state param and the +/// `withings_state` cookie. The cookie is httpOnly + Secure + +/// sameSite:lax today so XSS exfiltration is closed, but the user id +/// is still recoverable from any request-log entry where the cookie +/// isn't redacted and from a network-traffic capture of the redirect. +/// The v1.4.43 security audit (L-1) recommended decoupling the two by +/// switching the state to a fully-random 16-byte nonce backed by this +/// short-lived row — the callback resolves the user via the row's +/// `userId` instead of the cookie value. +/// +/// Lifecycle: +/// * `connect` mints a row with `expiresAt = now() + 10 min` and +/// sets the cookie to JUST the nonce (no user id). +/// * `callback` looks up the row, asserts `expiresAt > now()`, then +/// DELETEs it in the same transaction — replay of the same nonce +/// fails the second time because the row is gone. +/// * A daily cleanup cron sweeps any abandoned rows whose +/// `expiresAt` has passed (user closed the Withings tab without +/// completing the handshake). +model WithingsOAuthState { + nonce String @id + userId String @map("user_id") + createdAt DateTime @default(now()) @map("created_at") + expiresAt DateTime @map("expires_at") + + user User @relation(fields: [userId], references: [id], onDelete: Cascade) + + @@index([expiresAt]) + @@map("withings_oauth_states") +} + // ─── Mood Entries (moodLog) ─────────────────────────────────── model MoodEntry { diff --git a/src/app/api/withings/callback/route.ts b/src/app/api/withings/callback/route.ts index f733e82d9..9b7ee0319 100644 --- a/src/app/api/withings/callback/route.ts +++ b/src/app/api/withings/callback/route.ts @@ -5,6 +5,7 @@ import { auditLog } from "@/lib/auth/audit"; import { encrypt } from "@/lib/crypto"; import { exchangeCode, WITHINGS_OAUTH_SCOPE } from "@/lib/withings/client"; 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 { NextRequest, NextResponse } from "next/server"; @@ -12,6 +13,20 @@ import { timingSafeEqual } from "node:crypto"; /** * OAuth callback from Withings. Exchanges code for tokens and stores them. + * + * v1.4.47 W6 — `state` is now a fully-random 22-character base64url + * nonce keyed against the `WithingsOAuthState` ledger, rather than the + * legacy `${user.id}:${random16}` shape. The user identity bound to + * the in-flight handshake is resolved via the ledger's `userId` + * column (no longer parsed from the cookie value) and cross-checked + * against the session — both must agree before the token exchange + * runs. The row is consumed (deleted) on every exit branch so a + * replay of the same nonce fails the second time. + * + * 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 + * captures. */ export const GET = apiHandler(async (request: NextRequest) => { const { user } = await requireAuth(); @@ -20,9 +35,11 @@ export const GET = apiHandler(async (request: NextRequest) => { const { searchParams } = new URL(request.url); const code = searchParams.get("code"); const state = searchParams.get("state"); - const storedState = request.cookies.get("withings_state")?.value; + const storedState = request.cookies.get(WITHINGS_OAUTH_STATE_COOKIE)?.value; - // CSRF check (timing-safe comparison to prevent timing attacks) + // 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. if ( !state || !storedState || @@ -37,18 +54,42 @@ export const GET = apiHandler(async (request: NextRequest) => { ); } - // Verify state contains current user's ID - const [stateUserId] = state.split(":"); - if (stateUserId !== user.id) { + // 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(() => {}); + } return NextResponse.redirect( new URL( - "/settings/integrations?withings=error&reason=user", + "/settings/integrations?withings=error&reason=state", process.env.NEXT_PUBLIC_APP_URL!, ), ); } 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", @@ -60,6 +101,9 @@ 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", @@ -112,16 +156,29 @@ 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", process.env.NEXT_PUBLIC_APP_URL!, ), ); - response.cookies.delete("withings_state"); + response.cookies.delete(WITHINGS_OAUTH_STATE_COOKIE); 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", diff --git a/src/app/api/withings/connect/route.ts b/src/app/api/withings/connect/route.ts index a347d65eb..fd8393327 100644 --- a/src/app/api/withings/connect/route.ts +++ b/src/app/api/withings/connect/route.ts @@ -1,14 +1,26 @@ import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiError } from "@/lib/api-response"; +import { prisma } from "@/lib/db"; import { annotate } from "@/lib/logging/context"; import { getAuthorizationUrl } from "@/lib/withings/client"; import { getUserWithingsCredentials } from "@/lib/withings/credentials"; -import { randomBytes } from "node:crypto"; +import { + WITHINGS_OAUTH_STATE_COOKIE, + WITHINGS_OAUTH_STATE_TTL_MS, + mintWithingsOAuthStateNonce, +} from "@/lib/withings/oauth-state"; import { NextResponse } from "next/server"; /** * Redirects the user to Withings OAuth authorization page. - * State param = userId:random for CSRF protection. + * + * v1.4.47 W6 — state is now a fully-random 22-character base64url + * nonce backed by a 10-minute `WithingsOAuthState` row, rather than + * the legacy `${user.id}:${random16}` shape. The cookie carries JUST + * the nonce (no user id), the OAuth state URL param carries JUST the + * nonce, and the callback resolves the user via the row's `userId` + * column. Closes the v1.4.43 security audit L-1 finding where the + * user id was recoverable from log entries / network captures. */ export const GET = apiHandler(async () => { const { user } = await requireAuth(); @@ -22,17 +34,23 @@ export const GET = apiHandler(async () => { ); } - const stateNonce = randomBytes(16).toString("hex"); - const state = `${user.id}:${stateNonce}`; + const nonce = mintWithingsOAuthStateNonce(); + await prisma.withingsOAuthState.create({ + data: { + nonce, + userId: user.id, + expiresAt: new Date(Date.now() + WITHINGS_OAUTH_STATE_TTL_MS), + }, + }); - const url = getAuthorizationUrl(state, creds); + const url = getAuthorizationUrl(nonce, creds); const response = NextResponse.redirect(url); - response.cookies.set("withings_state", state, { + response.cookies.set(WITHINGS_OAUTH_STATE_COOKIE, nonce, { httpOnly: true, secure: process.env.NODE_ENV === "production", sameSite: "lax", - maxAge: 600, + maxAge: Math.floor(WITHINGS_OAUTH_STATE_TTL_MS / 1000), path: "/", }); diff --git a/src/lib/jobs/__tests__/withings-oauth-state-cleanup.test.ts b/src/lib/jobs/__tests__/withings-oauth-state-cleanup.test.ts new file mode 100644 index 000000000..5e4320cc3 --- /dev/null +++ b/src/lib/jobs/__tests__/withings-oauth-state-cleanup.test.ts @@ -0,0 +1,65 @@ +/** + * Unit coverage for the Withings OAuth state ledger sweep. + * + * The cron handler in `reminder-worker.ts` is a thin wrapper around + * `cleanupExpiredWithingsOAuthStates(p)`; the meaningful contract + * (deletes rows whose `expiresAt < now`, returns the deleted count, + * leaves live rows untouched) lives on the helper. These cases pin + * the contract without going near pg-boss or a real Postgres. + */ +import { describe, it, expect, vi, afterEach } from "vitest"; +import type { PrismaClient } from "@/generated/prisma/client"; +import { cleanupExpiredWithingsOAuthStates } from "../withings-oauth-state-cleanup"; + +function makePrismaMock(deletedCount: number) { + return { + withingsOAuthState: { + deleteMany: vi.fn().mockResolvedValue({ count: deletedCount }), + }, + } as unknown as PrismaClient; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("cleanupExpiredWithingsOAuthStates", () => { + it("issues a single `expiresAt < now` deleteMany and returns the deleted count", async () => { + const prisma = makePrismaMock(7); + const now = new Date("2026-05-22T03:20:00Z"); + + const deleted = await cleanupExpiredWithingsOAuthStates(prisma, now); + + expect(deleted).toBe(7); + expect(prisma.withingsOAuthState.deleteMany).toHaveBeenCalledTimes(1); + expect(prisma.withingsOAuthState.deleteMany).toHaveBeenCalledWith({ + where: { expiresAt: { lt: now } }, + }); + }); + + it("defaults `now` to a fresh wall-clock when the caller omits it", async () => { + const prisma = makePrismaMock(0); + const before = Date.now(); + + await cleanupExpiredWithingsOAuthStates(prisma); + + const after = Date.now(); + const call = (prisma.withingsOAuthState.deleteMany as ReturnType) + .mock.calls[0][0]; + const cutoff: Date = call.where.expiresAt.lt; + + expect(cutoff).toBeInstanceOf(Date); + expect(cutoff.getTime()).toBeGreaterThanOrEqual(before); + expect(cutoff.getTime()).toBeLessThanOrEqual(after); + }); + + it("returns 0 when no rows match (idempotent re-run)", async () => { + const prisma = makePrismaMock(0); + const now = new Date("2026-05-22T03:20:00Z"); + + const deleted = await cleanupExpiredWithingsOAuthStates(prisma, now); + + expect(deleted).toBe(0); + expect(prisma.withingsOAuthState.deleteMany).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/lib/jobs/reminder-worker.ts b/src/lib/jobs/reminder-worker.ts index 9ed964804..5e9de6cbc 100644 --- a/src/lib/jobs/reminder-worker.ts +++ b/src/lib/jobs/reminder-worker.ts @@ -31,6 +31,7 @@ import { import { setGlobalBoss } from "@/lib/jobs/boss-instance"; import { cleanupExpiredIdempotencyKeys } from "@/lib/jobs/idempotency-cleanup"; import { cleanupOldAuditLogs } from "@/lib/jobs/audit-log-cleanup"; +import { cleanupExpiredWithingsOAuthStates } from "@/lib/jobs/withings-oauth-state-cleanup"; import { runHostMetricTick } from "@/lib/jobs/host-metric-sampler"; import { aggregateRecommendationFeedback } from "@/lib/jobs/feedback-aggregator"; import { @@ -177,6 +178,14 @@ const IDEMPOTENCY_CLEANUP_QUEUE = "idempotency-cleanup"; const IDEMPOTENCY_CLEANUP_CRON = "0 3 * * *"; // daily at 03:00 (Europe/Berlin) const AUDIT_LOG_CLEANUP_QUEUE = "audit-log-cleanup"; const AUDIT_LOG_CLEANUP_CRON = "15 3 * * *"; // daily at 03:15 (Europe/Berlin) +// v1.4.47 W6 — daily sweep for the Withings OAuth state ledger. Slots +// at :20 between the audit-log cleanup (:15) and the mood-reminder +// cleanup (:25), inside the existing 02:xx-03:xx maintenance window. +// Rows are normally consumed by the callback handler in single-use +// fashion; this sweep picks up the long tail where a user closed the +// Withings approval tab without bouncing back to the callback URL. +const WITHINGS_OAUTH_STATE_CLEANUP_QUEUE = "withings-oauth-state-cleanup"; +const WITHINGS_OAUTH_STATE_CLEANUP_CRON = "20 3 * * *"; const OFFHOST_BACKUP_QUEUE = "data-backup-offhost"; // 02:30 Europe/Berlin — runs after audit-log/idempotency cleanups so old // rows are gone before they're snapshotted, but before the existing @@ -303,6 +312,10 @@ interface AuditLogCleanupPayload { triggeredAt: string; } +interface WithingsOAuthStateCleanupPayload { + triggeredAt: string; +} + interface OffhostBackupPayload { triggeredAt: string; } @@ -1245,6 +1258,23 @@ async function handleAuditLogCleanup(jobs: Job[]) { }); } +async function handleWithingsOAuthStateCleanup( + jobs: Job[], +) { + void jobs; + await withBackgroundEvent("job.withings_oauth_state_cleanup", async (evt) => { + const p = getWorkerPrisma(); + try { + const deleted = await cleanupExpiredWithingsOAuthStates(p); + evt.addMeta("withings_oauth_state_cleanup_deleted", deleted); + } catch (err) { + // The OAuth flow tolerates a stale row sticking around for an + // extra day — log + carry on so the boss queue doesn't retry-loop. + evt.addWarning(`withings-oauth-state-cleanup failed: ${err}`); + } + }); +} + async function handleHostMetricSample(jobs: Job[]) { void jobs; await withBackgroundEvent("job.host_metric_sample", async (evt) => { @@ -1671,6 +1701,11 @@ export async function startReminderWorker() { RATE_LIMIT_CLEANUP_QUEUE, IDEMPOTENCY_CLEANUP_QUEUE, AUDIT_LOG_CLEANUP_QUEUE, + // v1.4.47 W6 — daily sweep for the Withings OAuth state ledger. + // Same pg-boss v12 createQueue contract as the other cleanup + // crons; without this entry the schedule below silently no-ops + // and abandoned rows pile up. + WITHINGS_OAUTH_STATE_CLEANUP_QUEUE, OFFHOST_BACKUP_QUEUE, HOST_METRIC_QUEUE, FEEDBACK_AGGREGATOR_QUEUE, @@ -1744,6 +1779,7 @@ export async function startReminderWorker() { [RATE_LIMIT_CLEANUP_QUEUE, RATE_LIMIT_CLEANUP_CRON], [IDEMPOTENCY_CLEANUP_QUEUE, IDEMPOTENCY_CLEANUP_CRON], [AUDIT_LOG_CLEANUP_QUEUE, AUDIT_LOG_CLEANUP_CRON], + [WITHINGS_OAUTH_STATE_CLEANUP_QUEUE, WITHINGS_OAUTH_STATE_CLEANUP_CRON], [OFFHOST_BACKUP_QUEUE, OFFHOST_BACKUP_CRON], [HOST_METRIC_QUEUE, HOST_METRIC_CRON], [FEEDBACK_AGGREGATOR_QUEUE, FEEDBACK_AGGREGATOR_CRON], @@ -1860,6 +1896,11 @@ export async function startReminderWorker() { { localConcurrency: 1 }, handleAuditLogCleanup, ); + await boss.work( + WITHINGS_OAUTH_STATE_CLEANUP_QUEUE, + { localConcurrency: 1 }, + handleWithingsOAuthStateCleanup, + ); await boss.work( OFFHOST_BACKUP_QUEUE, { localConcurrency: 1 }, diff --git a/src/lib/jobs/withings-oauth-state-cleanup.ts b/src/lib/jobs/withings-oauth-state-cleanup.ts new file mode 100644 index 000000000..2cf9718c8 --- /dev/null +++ b/src/lib/jobs/withings-oauth-state-cleanup.ts @@ -0,0 +1,31 @@ +/** + * Daily cleanup for the `withings_oauth_states` table. + * + * The v1.4.47 W6 OAuth state ledger is single-use — every happy-path + * + error-path branch in `withings/callback` consumes its row, so an + * abandoned row only lingers when the user closed the Withings + * approval tab without ever bouncing back to the callback URL. The + * cookie + row TTL is 10 minutes, so once the timestamp blows the + * row is dead weight on the table. + * + * Runs daily at 03:20 Europe/Berlin (slotted between the audit-log + * cleanup at 03:15 and the mood-reminder cleanup at 03:25) — the + * table is bounded by `users × concurrent in-flight handshakes`, so + * a daily sweep is overkill on volume but cheap and keeps the + * `expires_at` index from accumulating dead tuples. + * + * The handler is idempotent: re-running it within the same window + * matches zero rows the second time because the first pass deleted + * everything older than `now()`. + */ +import type { PrismaClient } from "@/generated/prisma/client"; + +export async function cleanupExpiredWithingsOAuthStates( + prisma: PrismaClient, + now: Date = new Date(), +): Promise { + const { count } = await prisma.withingsOAuthState.deleteMany({ + where: { expiresAt: { lt: now } }, + }); + return count; +} diff --git a/src/lib/withings/__tests__/oauth-state.test.ts b/src/lib/withings/__tests__/oauth-state.test.ts new file mode 100644 index 000000000..a34bb8e3c --- /dev/null +++ b/src/lib/withings/__tests__/oauth-state.test.ts @@ -0,0 +1,39 @@ +/** + * Unit coverage for the Withings OAuth state mint helpers. + * + * The connect route reads these constants verbatim; pinning the + * 22-char base64url shape + cookie name + TTL prevents an accidental + * drift (e.g. someone swapping `randomBytes(16).toString("hex")` back + * in and silently exposing a 32-char hex nonce, which would still + * "work" but is twice the entropy footprint and breaks any reader + * that assumes the v1.4.47 shape). + */ +import { describe, it, expect } from "vitest"; +import { + WITHINGS_OAUTH_STATE_COOKIE, + WITHINGS_OAUTH_STATE_TTL_MS, + mintWithingsOAuthStateNonce, +} from "../oauth-state"; + +describe("Withings OAuth state constants", () => { + it("uses the historical cookie name so an in-flight handshake survives a deploy", () => { + expect(WITHINGS_OAUTH_STATE_COOKIE).toBe("withings_state"); + }); + + it("sets a 10-minute TTL — long enough for the consent prompt, short enough to bound the cleanup", () => { + expect(WITHINGS_OAUTH_STATE_TTL_MS).toBe(10 * 60 * 1000); + }); +}); + +describe("mintWithingsOAuthStateNonce", () => { + it("returns a 22-char base64url string (16 random bytes, no padding)", () => { + const nonce = mintWithingsOAuthStateNonce(); + expect(nonce).toMatch(/^[A-Za-z0-9_-]{22}$/); + }); + + it("returns a fresh value on every call (no collisions over a small batch)", () => { + const nonces = new Set(); + for (let i = 0; i < 100; i += 1) nonces.add(mintWithingsOAuthStateNonce()); + expect(nonces.size).toBe(100); + }); +}); diff --git a/src/lib/withings/oauth-state.ts b/src/lib/withings/oauth-state.ts new file mode 100644 index 000000000..f0303763f --- /dev/null +++ b/src/lib/withings/oauth-state.ts @@ -0,0 +1,48 @@ +/** + * v1.4.47 W6 — shared constants + mint helper for the Withings OAuth + * state nonce ledger. + * + * The legacy implementation (v1.4.42-) built the state as + * `${user.id}:${random16}` and persisted it on both the URL state + * param and the `withings_state` cookie. The cookie is httpOnly + + * Secure + sameSite:lax so XSS exfiltration is closed, but the user + * id still leaks into request logs / network captures (the v1.4.43 + * security audit L-1 finding). The new shape decouples the two: the + * nonce is random + opaque, and a short-lived `WithingsOAuthState` + * row carries the `(nonce → userId)` mapping. + * + * The helpers live on their own module so the connect route, the + * callback route, and the cleanup cron all read the same source of + * truth — and the unit tests can pin the contract without importing + * either route file. + */ +import { randomBytes } from "node:crypto"; + +/** + * Cookie name carrying the in-flight state nonce. Same string as the + * legacy `withings_state` cookie so a deploy mid-flight doesn't + * orphan a user's open Withings tab — the cookie shape is what + * changed (nonce-only instead of `${userId}:${nonce}`), not the + * cookie name. + */ +export const WITHINGS_OAUTH_STATE_COOKIE = "withings_state" as const; + +/** + * 10-minute TTL on the state row. Matches the cookie `maxAge`. Long + * enough to cover a user typing their Withings credentials and + * approving the consent prompt; short enough that an abandoned + * handshake doesn't leave a row stranded for the daily cleanup sweep + * to pick up hours later. + */ +export const WITHINGS_OAUTH_STATE_TTL_MS = 10 * 60 * 1000; + +/** + * Mint a fresh state nonce. 16 random bytes → 22 base64url chars + * (Node's `randomBytes(16).toString("base64url")` returns exactly 22 + * characters without padding for any 16-byte input). 128 bits of + * entropy — well past the CSRF threshold and aligned with the OAuth + * 2.0 §10.10 recommendation of "at least 128 bits". + */ +export function mintWithingsOAuthStateNonce(): string { + return randomBytes(16).toString("base64url"); +} diff --git a/tests/integration/setup.ts b/tests/integration/setup.ts index 4be37416e..24955a0c7 100644 --- a/tests/integration/setup.ts +++ b/tests/integration/setup.ts @@ -69,6 +69,7 @@ export async function truncateAllTables(client: PrismaClient): Promise { "user_achievements", "users", "withings_connections", + "withings_oauth_states", "workout_routes", "workouts", ]; diff --git a/tests/integration/withings-oauth-flow.test.ts b/tests/integration/withings-oauth-flow.test.ts new file mode 100644 index 000000000..51634f139 --- /dev/null +++ b/tests/integration/withings-oauth-flow.test.ts @@ -0,0 +1,348 @@ +/** + * v1.4.47 W6 — end-to-end coverage for the new Withings OAuth state + * ledger. + * + * The audit's L-1 recommendation was a fully-random nonce backed by a + * `(nonce → userId)` ledger row, single-use semantics, with the + * cookie carrying only the opaque nonce. This file pins the + * connect → callback handshake end-to-end against real Postgres: + * + * 1. `withings/connect` mints a row and stamps the cookie. + * 2. `withings/callback` resolves the user via the row's `userId`, + * verifies it matches the session, and deletes the row. + * 3. A second callback with the same nonce hits the "row not + * found" branch and redirects to the error page (replay + * protection). + * 4. A callback whose row has expired hits the same branch. + * 5. The connect handler refuses to start when the user has no + * Withings credentials — no ledger row is minted. + * + * Sibling assertions in `withings-oauth.test.ts` already cover the + * token-exchange happy path + the `disconnect` route; this file + * focuses on the ledger lifecycle the audit flagged. + */ +import { NextRequest } from "next/server"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar, headerJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +process.env.ENCRYPTION_KEY ??= + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +const TEST_USER_ID = "user-withings-oauth-flow"; + +const WITHINGS_OAUTH_URL = + "https://account.withings.com/oauth2_user/authorize2"; +const WITHINGS_TOKEN_URL = "https://wbsapi.withings.net/v2/oauth2"; +const WITHINGS_NOTIFY_URL = "https://wbsapi.withings.net/notify"; + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + cookieJar.clear(); + headerJar.clear(); + + const { encrypt } = await import("@/lib/crypto"); + const prisma = getPrismaClient(); + await prisma.user.create({ + data: { + id: TEST_USER_ID, + username: "withings-flow-user", + email: "withings-flow@example.test", + role: "USER", + withingsClientIdEncrypted: encrypt("client-id-flow"), + withingsClientSecretEncrypted: encrypt("client-secret-flow"), + }, + }); + const session = await prisma.session.create({ + data: { + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); + + process.env.NEXT_PUBLIC_APP_URL = "http://localhost:3000"; +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("Withings OAuth nonce ledger (real Postgres)", () => { + it("connect mints exactly one row keyed on the URL state, with `userId` + 10-min TTL", async () => { + const { GET } = await import("@/app/api/withings/connect/route"); + const res = await (GET as unknown as (req: Request) => Promise)( + new Request("http://localhost/api/withings/connect"), + ); + + expect(res.status).toBe(307); + const location = res.headers.get("location")!; + expect(location).toContain(WITHINGS_OAUTH_URL); + const target = new URL(location); + const state = target.searchParams.get("state")!; + + // 22-char base64url. The legacy `${userId}:${random16}` shape no + // longer reaches the URL — the audit L-1 fix. + expect(state).toMatch(/^[A-Za-z0-9_-]{22}$/); + expect(state).not.toContain(":"); + expect(state).not.toContain(TEST_USER_ID); + + const prisma = getPrismaClient(); + const rows = await prisma.withingsOAuthState.findMany(); + expect(rows).toHaveLength(1); + expect(rows[0].nonce).toBe(state); + expect(rows[0].userId).toBe(TEST_USER_ID); + // TTL is 10 minutes; allow a generous floor so a slow CI box + // doesn't flake on the upper bound. + const ttlMs = rows[0].expiresAt.getTime() - Date.now(); + expect(ttlMs).toBeGreaterThan(9 * 60 * 1000); + expect(ttlMs).toBeLessThanOrEqual(10 * 60 * 1000 + 5_000); + + // Cookie carries the same opaque nonce — no user id. + const setCookie = res.headers.get("set-cookie")!; + expect(setCookie).toContain(`withings_state=${state}`); + expect(setCookie).not.toContain(TEST_USER_ID); + }); + + it("connect refuses (400) when the user has no Withings credentials — no ledger row minted", async () => { + const prisma = getPrismaClient(); + await prisma.user.update({ + where: { id: TEST_USER_ID }, + data: { + withingsClientIdEncrypted: null, + withingsClientSecretEncrypted: null, + }, + }); + + const { GET } = await import("@/app/api/withings/connect/route"); + const res = await (GET as unknown as (req: Request) => Promise)( + new Request("http://localhost/api/withings/connect"), + ); + + expect(res.status).toBe(400); + const rows = await prisma.withingsOAuthState.findMany(); + expect(rows).toHaveLength(0); + }); + + it("connect → callback consumes the row (single-use); replay of the same nonce hits the error page", async () => { + vi.spyOn(globalThis, "fetch").mockImplementation(async (input) => { + const url = typeof input === "string" ? input : (input as Request).url; + if (url === WITHINGS_TOKEN_URL) { + return new Response( + JSON.stringify({ + status: 0, + body: { + userid: "withings-userid-flow", + access_token: "access-flow", + refresh_token: "refresh-flow", + expires_in: 3600, + }, + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (url === WITHINGS_NOTIFY_URL) { + return new Response(JSON.stringify({ status: 0 }), { status: 200 }); + } + throw new Error(`Unexpected fetch in flow test: ${url}`); + }); + + // Leg 1 — connect + const { GET: connect } = await import("@/app/api/withings/connect/route"); + const connectRes = await ( + connect as unknown as (req: Request) => Promise + )(new Request("http://localhost/api/withings/connect")); + const location = connectRes.headers.get("location")!; + const nonce = new URL(location).searchParams.get("state")!; + + const prisma = getPrismaClient(); + const beforeCallback = await prisma.withingsOAuthState.findUnique({ + where: { nonce }, + }); + expect(beforeCallback).not.toBeNull(); + + // Leg 2 — callback (happy path) + const { GET: callback } = await import("@/app/api/withings/callback/route"); + const callbackReq = new NextRequest( + `http://localhost/api/withings/callback?code=auth-code&state=${encodeURIComponent(nonce)}`, + { + method: "GET", + headers: { + cookie: `healthlog_session=${cookieJar.get("healthlog_session")}; withings_state=${nonce}`, + }, + }, + ); + const callbackRes = await callback(callbackReq); + expect(callbackRes.status).toBe(307); + expect(callbackRes.headers.get("location")).toContain( + "withings=connected", + ); + + const afterCallback = await prisma.withingsOAuthState.findUnique({ + where: { nonce }, + }); + expect(afterCallback).toBeNull(); + + // Leg 3 — replay the same nonce. The cookie compare still passes + // (matching strings) but the row lookup misses, so the handler + // bounces to the error page without hitting the token endpoint + // again. + const fetchSpy = vi.mocked(globalThis.fetch); + fetchSpy.mockClear(); + const replayReq = new NextRequest( + `http://localhost/api/withings/callback?code=auth-code&state=${encodeURIComponent(nonce)}`, + { + method: "GET", + headers: { + cookie: `healthlog_session=${cookieJar.get("healthlog_session")}; withings_state=${nonce}`, + }, + }, + ); + const replayRes = await callback(replayReq); + expect(replayRes.status).toBe(307); + expect(replayRes.headers.get("location")).toContain( + "withings=error&reason=state", + ); + // No token-endpoint call on the replay leg. + const replayTokenCalls = fetchSpy.mock.calls.filter( + ([input]) => + (typeof input === "string" ? input : (input as Request).url) === + WITHINGS_TOKEN_URL, + ); + expect(replayTokenCalls).toHaveLength(0); + }); + + it("callback rejects an expired row, deletes it, and never reaches the token exchange", async () => { + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("must not be called", { status: 500 })); + + const nonce = "expired-nonce-22charXX"; + const prisma = getPrismaClient(); + await prisma.withingsOAuthState.create({ + data: { + nonce, + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() - 60_000), // already expired + }, + }); + + const { GET: callback } = await import("@/app/api/withings/callback/route"); + const req = new NextRequest( + `http://localhost/api/withings/callback?code=auth-code&state=${encodeURIComponent(nonce)}`, + { + method: "GET", + headers: { + cookie: `healthlog_session=${cookieJar.get("healthlog_session")}; withings_state=${nonce}`, + }, + }, + ); + const res = await callback(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + + // Token endpoint never hit. + expect(fetchSpy).not.toHaveBeenCalled(); + + // The handler stamps out the expired row so a clock-skewed replay + // can't slip through. + const lingering = await prisma.withingsOAuthState.findUnique({ + where: { nonce }, + }); + expect(lingering).toBeNull(); + }); + + it("callback rejects a row whose userId does not match the session, deletes it, and writes no connection", async () => { + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response("must not be called", { status: 500 })); + + // Seed a second user + a ledger row that points to *them*. The + // callback runs under TEST_USER_ID's session, so the row's userId + // mismatch must trip the rejection branch. + const prisma = getPrismaClient(); + await prisma.user.create({ + data: { + id: "user-other-flow", + username: "other-user", + email: "other@example.test", + role: "USER", + }, + }); + const nonce = "cross-user-noncexxxxxx"; + await prisma.withingsOAuthState.create({ + data: { + nonce, + userId: "user-other-flow", + expiresAt: new Date(Date.now() + 10 * 60 * 1000), + }, + }); + + const { GET: callback } = await import("@/app/api/withings/callback/route"); + const req = new NextRequest( + `http://localhost/api/withings/callback?code=auth-code&state=${encodeURIComponent(nonce)}`, + { + method: "GET", + headers: { + cookie: `healthlog_session=${cookieJar.get("healthlog_session")}; withings_state=${nonce}`, + }, + }, + ); + const res = await callback(req); + + expect(res.status).toBe(307); + expect(res.headers.get("location")).toContain( + "withings=error&reason=state", + ); + expect(fetchSpy).not.toHaveBeenCalled(); + + // The row is stamped out — single-use + cross-user mismatch is a + // CSRF signal, not a recoverable state, so we don't leave the + // row around for a retry. + const lingering = await prisma.withingsOAuthState.findUnique({ + where: { nonce }, + }); + expect(lingering).toBeNull(); + + // No connection landed under EITHER user. + expect( + await prisma.withingsConnection.findUnique({ + where: { userId: TEST_USER_ID }, + }), + ).toBeNull(); + expect( + await prisma.withingsConnection.findUnique({ + where: { userId: "user-other-flow" }, + }), + ).toBeNull(); + }); +}); diff --git a/tests/integration/withings-oauth.test.ts b/tests/integration/withings-oauth.test.ts index 873f5267c..e8a00645a 100644 --- a/tests/integration/withings-oauth.test.ts +++ b/tests/integration/withings-oauth.test.ts @@ -118,15 +118,29 @@ describe("Withings OAuth handshake (real Postgres)", () => { ); expect(target.searchParams.get("scope")).toBe("user.metrics,user.activity"); const state = target.searchParams.get("state"); - expect(state).toMatch(/^user-withings-oauth:[a-f0-9]{32}$/); + // v1.4.47 W6 — state is a fully-random 22-char base64url nonce. + // The legacy `${user.id}:${random16}` shape leaked the user id to + // request logs / network captures (audit L-1); the new shape is + // opaque and resolves the user via the `WithingsOAuthState` row. + expect(state).toMatch(/^[A-Za-z0-9_-]{22}$/); - // The Set-Cookie header carries the same state nonce — that cookie - // is what `callback/route.ts` compares against on the second leg. - // Cookie value is URL-encoded for the `:` separator; compare on the - // encoded form so the assertion is byte-stable. + // The Set-Cookie header carries the same opaque nonce — that + // cookie is what `callback/route.ts` compares against on the + // second leg before resolving the user via the ledger row. const setCookie = res.headers.get("set-cookie"); expect(setCookie).toContain("withings_state="); - expect(setCookie).toContain(encodeURIComponent(state!)); + expect(setCookie).toContain(state!); + + // v1.4.47 W6 — the connect handler also persists a ledger row. + // Asserting on it here closes the contract loop: the row is what + // the callback handler will look up. + const prisma = getPrismaClient(); + const row = await prisma.withingsOAuthState.findUnique({ + where: { nonce: state! }, + }); + expect(row).not.toBeNull(); + expect(row?.userId).toBe(TEST_USER_ID); + expect(row!.expiresAt.getTime()).toBeGreaterThan(Date.now()); }); it("callback exchanges code → encrypted tokens on the WithingsConnection row", async () => { @@ -156,7 +170,19 @@ describe("Withings OAuth handshake (real Postgres)", () => { throw new Error(`Unexpected fetch in withings-oauth test: ${url}`); }); - const state = `${TEST_USER_ID}:${"a".repeat(32)}`; + // v1.4.47 W6 — seed a live ledger row so the callback can resolve + // the user via the row's `userId`. The state on the URL + cookie + // must equal the row's `nonce`. + const state = "abcdefghijklmnopqrstuv"; + const prisma = getPrismaClient(); + await prisma.withingsOAuthState.create({ + data: { + nonce: state, + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 10 * 60 * 1000), + }, + }); + const req = new NextRequest( `http://localhost/api/withings/callback?code=auth-code-123&state=${encodeURIComponent(state)}`, { @@ -175,7 +201,14 @@ describe("Withings OAuth handshake (real Postgres)", () => { "/settings/integrations?withings=connected", ); - const prisma = getPrismaClient(); + // v1.4.47 W6 — the ledger row is consumed on the happy path + // (single-use semantics). A replay of the same nonce would fail + // the state-not-found branch and bounce to the error page. + const consumed = await prisma.withingsOAuthState.findUnique({ + where: { nonce: state }, + }); + expect(consumed).toBeNull(); + const conn = await prisma.withingsConnection.findUnique({ where: { userId: TEST_USER_ID }, }); @@ -210,8 +243,12 @@ describe("Withings OAuth handshake (real Postgres)", () => { .spyOn(globalThis, "fetch") .mockResolvedValue(new Response("must not be called", { status: 500 })); - const validState = `${TEST_USER_ID}:${"a".repeat(32)}`; - const replayedState = `${TEST_USER_ID}:${"b".repeat(32)}`; + // v1.4.47 W6 — 22-char base64url nonces; the cookie value and the + // URL state diverge so the constant-time check short-circuits on + // the first byte mismatch. No ledger row needs to exist because + // the request never reaches the row lookup. + const validState = "aaaaaaaaaaaaaaaaaaaaaa"; + const replayedState = "bbbbbbbbbbbbbbbbbbbbbb"; const req = new NextRequest( `http://localhost/api/withings/callback?code=auth-code-123&state=${encodeURIComponent(replayedState)}`, {