From 60d5416cf8c82d74820ba892e6b777370ccab4d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 23 Jul 2026 15:08:31 +0200 Subject: [PATCH] fix(oidc): sign-in no longer fails on a duplicated service-worker callback A browser SSO sign-in landed on /auth/login?error=oidc_failed even though it had succeeded. Two layers conspired: - The service worker enables navigation preload, but its fetch handler returned early for /api/ paths without calling event.respondWith(). For the OIDC login/callback navigations that left the preload unconsumed, so the browser also ran its own default fetch and the callback was requested twice. The first request redeemed the single-use authorization code and set the session; the second tried to redeem the now-consumed code, failed at the IdP token endpoint, and that 307 to the error page is what the user saw. - Even with the double request gone, a legitimate retry (proxy, double navigation) hit the same wall on the server. Fix both layers: - The worker settles /api/auth/* navigations itself, consuming the preload response (or fetching once when there is none), so exactly one request reaches the server. These responses are never cached. - The callback is idempotent. When the single-use state cookie is already consumed but the request carries a database-validated, unexpired session, the callback treats it as a duplicate of a completed login and redirects into the app instead of surfacing an error. The authorization code is never exchanged on this branch, and a consumed/forged/expired state from a caller with no valid session still fails closed with the unchanged oidc_state error. Refs #600 --- public/sw.js | 20 +++++ src/__tests__/service-worker.test.ts | 89 +++++++++++++++++++ .../oidc/callback/__tests__/route.test.ts | 57 +++++++++++- src/app/api/auth/oidc/callback/route.ts | 40 ++++++++- src/lib/auth/session.ts | 30 +++++++ 5 files changed, 232 insertions(+), 4 deletions(-) diff --git a/public/sw.js b/public/sw.js index f6fe71355..bc286f7b1 100644 --- a/public/sw.js +++ b/public/sw.js @@ -151,6 +151,26 @@ self.addEventListener("fetch", (event) => { // Only handle same-origin GET requests if (request.method !== "GET" || url.origin !== self.location.origin) return; + // One-shot auth navigations must reach the server EXACTLY ONCE. The OIDC + // login + callback endpoints are top-level browser navigations (not + // `fetch`/XHR), so with navigation preload enabled (see `activate`) the + // browser has ALREADY dispatched the request by the time this handler runs. + // Returning early WITHOUT `event.respondWith()` — the general `/api/` path + // below does exactly that — makes the browser ALSO run its own default + // navigation fetch, so the request is sent twice. The OIDC authorization + // code is single-use: the first callback redeems it and sets the session, + // the second fails at the IdP and redirects to + // `/auth/login?error=oidc_failed` on an otherwise-successful sign-in. + // Consuming the preload response here (or fetching once when there is none) + // settles the event with a single network request. These responses are + // never cached. + if (url.pathname.startsWith("/api/auth/")) { + event.respondWith( + (async () => (await event.preloadResponse) || fetch(request))(), + ); + return; + } + // API calls. Allowlisted safe GET reads use network-first so the installed // PWA always renders fresh server data online (and falls back to the last // cached copy only when the network fails); everything else (auth, diff --git a/src/__tests__/service-worker.test.ts b/src/__tests__/service-worker.test.ts index c6fe7bd13..7c2a56db0 100644 --- a/src/__tests__/service-worker.test.ts +++ b/src/__tests__/service-worker.test.ts @@ -248,6 +248,95 @@ function dispatchAssetFetch( return captured; } +/** + * Dispatch a top-level navigation the way the browser does when navigation + * preload is enabled — with a `preloadResponse` promise already in flight. + * Unlike the other dispatchers this one does NOT throw when the handler + * declines to respond: whether `respondWith` was called is exactly what the + * one-shot-auth-navigation test asserts (no `respondWith` ⇒ the browser also + * runs its own default fetch ⇒ the request is sent twice). + */ +function dispatchAuthNavigation( + harness: SwHarness, + path: string, + preloadResponse?: Promise, +): { responded: boolean; response: Promise | null } { + let captured: Promise | null = null; + let responded = false; + harness.listeners.get("fetch")!({ + request: new Request(`${ORIGIN}${path}`, { + headers: { accept: "text/html" }, + }), + respondWith: (p: Promise) => { + responded = true; + captured = p; + }, + preloadResponse, + waitUntil: (p: Promise) => { + harness.lifetimePromises.push(p); + }, + }); + return { responded, response: captured }; +} + +describe("sw.js — one-shot auth navigations", () => { + it("settles an OIDC callback navigation itself, consuming the preload (single request)", async () => { + const harness = bootServiceWorker(); + let fetchCalls = 0; + harness.context.fetch = async () => { + fetchCalls += 1; + return new Response("network callback"); + }; + + const { responded, response } = dispatchAuthNavigation( + harness, + "/api/auth/oidc/callback?code=abc&state=xyz", + Promise.resolve(new Response("preloaded callback")), + ); + + // The SW settles the event, so the browser does NOT also run its own + // default navigation fetch — the single-use OIDC code reaches the server + // exactly once. (Without this the handler returned early with no + // `respondWith`, and the preload + default fetch redeemed the code twice.) + expect(responded).toBe(true); + // The already-in-flight preload response is the one used; no extra fetch. + expect(await (await response!).text()).toBe("preloaded callback"); + expect(fetchCalls).toBe(0); + }); + + it("fetches an auth navigation exactly once when no preload is supplied", async () => { + const harness = bootServiceWorker(); + let fetchCalls = 0; + harness.context.fetch = async () => { + fetchCalls += 1; + return new Response("network callback"); + }; + + const { responded, response } = dispatchAuthNavigation( + harness, + "/api/auth/oidc/login?next=%2F", + ); + + expect(responded).toBe(true); + expect(await (await response!).text()).toBe("network callback"); + expect(fetchCalls).toBe(1); + }); + + it("never caches an auth navigation response", async () => { + const harness = bootServiceWorker(); + harness.context.fetch = async () => new Response("network callback"); + + const url = "/api/auth/oidc/callback?code=abc&state=xyz"; + await dispatchAuthNavigation(harness, url).response; + await Promise.all(harness.lifetimePromises); + + const pages = await harness.cacheStorage.open(CURRENT_PAGE_CACHE); + expect(await pages.match(`${ORIGIN}${url}`)).toBeUndefined(); + const data = await harness.cacheStorage.open(CURRENT_DATA_CACHE); + expect(await data.match(`${ORIGIN}${url}`)).toBeUndefined(); + }); +}); + describe("sw.js — best-effort cache writes", () => { it("returns the original successful API response when cache.put rejects", async () => { const harness = bootServiceWorker(); diff --git a/src/app/api/auth/oidc/callback/__tests__/route.test.ts b/src/app/api/auth/oidc/callback/__tests__/route.test.ts index aa41f7b01..769eeff35 100644 --- a/src/app/api/auth/oidc/callback/__tests__/route.test.ts +++ b/src/app/api/auth/oidc/callback/__tests__/route.test.ts @@ -38,7 +38,11 @@ vi.mock("@/lib/db", () => ({ })); vi.mock("@/lib/auth/audit", () => ({ auditLog: vi.fn() })); -vi.mock("@/lib/auth/session", () => ({ createSession: vi.fn() })); +vi.mock("@/lib/auth/session", () => ({ + createSession: vi.fn(), + validateSessionFromCookieValue: vi.fn(), + SESSION_COOKIE_NAME: "healthlog_session", +})); vi.mock("@/lib/auth/login-alert", () => ({ recordSignInDevice: vi.fn() })); vi.mock("@/lib/auth/mfa/challenge", () => ({ createMfaChallenge: vi.fn(async () => ({ @@ -84,7 +88,10 @@ import { GET } from "../route"; import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; -import { createSession } from "@/lib/auth/session"; +import { + createSession, + validateSessionFromCookieValue, +} from "@/lib/auth/session"; import { createMfaChallenge } from "@/lib/auth/mfa/challenge"; import { getOidcConfig, @@ -591,6 +598,52 @@ describe("GET /api/auth/oidc/callback", () => { }); }); +describe("GET /api/auth/oidc/callback — duplicate-callback idempotency", () => { + // A duplicated callback for the SAME sign-in: the first callback already + // redeemed the single-use code, set the session cookie, and deleted the + // single-use state cookie — so the duplicate arrives with a live `code` + + // `state` in the query but NO state cookie. Whether it also fails closed + // turns entirely on whether a VALIDATED session is present. + function duplicateRequest(withSession: boolean): NextRequest { + const req = new NextRequest( + "http://localhost/api/auth/oidc/callback?code=abc&state=STATE", + ); + if (withSession) req.cookies.set("healthlog_session", "hls_valid_secret"); + return req; + } + + it("redirects a duplicate into the app when a valid session is present, WITHOUT re-redeeming the code", async () => { + vi.mocked(validateSessionFromCookieValue).mockResolvedValue({ + session: { id: "sess-1", expiresAt: new Date(Date.now() + 1_000_000) }, + user: userRow() as never, + } as never); + + const res = await GET(duplicateRequest(true)); + + const location = res.headers.get("location")!; + // Lands in the app, not on the error page. + expect(location).not.toContain("error="); + expect(new URL(location).pathname).toBe("/"); + // The now-consumed authorization code is NEVER exchanged again, and no + // second session is minted — the redirect is inert. + expect(exchangeCodeForTokens).not.toHaveBeenCalled(); + expect(createSession).not.toHaveBeenCalled(); + }); + + it("still fails closed (oidc_state) for a consumed/forged state with NO valid session", async () => { + // The security assertion: a caller who presents a real-looking callback + // but holds no valid session gets the unchanged error and the code is + // never redeemed. This must stay red if the fail-closed path is weakened. + vi.mocked(validateSessionFromCookieValue).mockResolvedValue(null); + + const res = await GET(duplicateRequest(false)); + + expect(res.headers.get("location")).toContain("error=oidc_state"); + expect(exchangeCodeForTokens).not.toHaveBeenCalled(); + expect(createSession).not.toHaveBeenCalled(); + }); +}); + describe("GET /api/auth/oidc/callback — native branch", () => { beforeEach(() => { vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ diff --git a/src/app/api/auth/oidc/callback/route.ts b/src/app/api/auth/oidc/callback/route.ts index 5832c23a6..efa1221b5 100644 --- a/src/app/api/auth/oidc/callback/route.ts +++ b/src/app/api/auth/oidc/callback/route.ts @@ -6,7 +6,11 @@ import { checkAuthSurfaceRateLimit } from "@/lib/rate-limit"; import { decrypt } from "@/lib/crypto"; import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; -import { createSession } from "@/lib/auth/session"; +import { + createSession, + validateSessionFromCookieValue, + SESSION_COOKIE_NAME, +} from "@/lib/auth/session"; import { recordSignInDevice } from "@/lib/auth/login-alert"; import { deriveUniqueUsername, @@ -195,7 +199,39 @@ export const GET = apiHandler(async (req: NextRequest) => { // and there is nothing safe to hand the app (spec §1 web-fallback). if (!code || !state) return errorRedirect("oidc_state"); - if (!stored) return errorRedirect("oidc_state"); + if (!stored) { + // Defence-in-depth idempotency for a duplicated callback. The single-use + // state cookie is set with `path: "/api/auth/oidc"` and DELETED by the + // first callback the moment it succeeds. A second callback for the same + // sign-in (a client/proxy retry, a double navigation, or the + // service-worker double-request this release also fixes) therefore arrives + // with the state cookie already gone — `stored` is null — and, because the + // first callback set the session, it carries a live session cookie. + // Redeeming the now-consumed authorization code again fails at the IdP and + // surfaces `?error=oidc_failed` on a sign-in that actually succeeded. So + // when a VALIDATED session is present, treat this as a duplicate of a + // completed login and send the user into the app instead of erroring. + // + // Fail-closed is preserved for the real attack case: the code is NEVER + // exchanged on this branch, and the session is resolved against the + // database + its expiry (not merely "a cookie is present"). A forged, + // replayed, or expired state from a caller with no valid session gets the + // unchanged `oidc_state` error; a caller who already holds their own valid + // session gains nothing here but a redirect to a page they could already + // reach — no code redemption, no session mint, no identity change. + const existingSession = await validateSessionFromCookieValue( + req.cookies.get(SESSION_COOKIE_NAME)?.value, + ); + if (existingSession) { + annotate({ meta: { reason: "duplicate_callback_authenticated" } }); + const response = NextResponse.redirect(oidcAppUrl("/")); + // The state cookie is already consumed; delete defensively so a stray + // blob can never outlive this response. + deleteStateCookie(response); + return response; + } + return errorRedirect("oidc_state"); + } // CSRF check — timing-safe, byte-for-byte, mirroring the Withings callback. if ( diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index cdc22a74a..0c0a55755 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -13,6 +13,10 @@ import { LOCALE_COOKIE, setLocaleCookie } from "@/lib/i18n/locale-cookie"; const SESSION_COOKIE = "healthlog_session"; const SESSION_MAX_AGE_MS = 30 * 24 * 60 * 60 * 1000; // 30 days +/** Public name of the browser session cookie, for routes that read it off a + * `NextRequest` directly (rather than through `getSession()`'s `cookies()`). */ +export const SESSION_COOKIE_NAME = SESSION_COOKIE; + /** * Marks a cookie value as a v1.30.32 session secret. Doubles as the branch * between the two resolution paths below, so a cookie is only ever looked up @@ -84,6 +88,32 @@ async function findSessionByCookie(cookieValue: string) { return legacy && legacy.tokenHash === null ? legacy : null; } +/** + * Validate a session straight from a raw cookie value — no `next/headers`, + * no sliding-expiry write, no cookie mutation. Runs the same hardened lookup + * `getSession()` uses (`hashToken` secret path or the passive legacy-cuid + * path) plus an expiry check, and returns the resolved user, or null for a + * missing / unknown / expired session. + * + * Exists for routes that must confirm an already-established session from a + * `NextRequest` before they mint anything — notably the OIDC callback's + * duplicate-detection path, which runs before session creation and must not + * touch the cookie store. Presence of a cookie string is NOT enough: an + * attacker controls that string, so the value is resolved against the + * database and its expiry, exactly like the login path. + */ +export async function validateSessionFromCookieValue( + cookieValue: string | undefined | null, +): Promise<{ session: { id: string; expiresAt: Date }; user: User } | null> { + if (!cookieValue) return null; + const session = await findSessionByCookie(cookieValue).catch(() => null); + if (!session || session.expiresAt < new Date()) return null; + return { + session: { id: session.id, expiresAt: session.expiresAt }, + user: session.user, + }; +} + // v1.23 — throttle window for the user-facing active-session list's "last // seen" stamp. A write only happens when the prior stamp is older than this, // so the high-churn `getSession` path costs at most one extra UPDATE per