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

@@ -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,

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<Response | undefined>,
+): { responded: boolean; response: Promise<Response> | null } {
+  let captured: Promise<Response> | null = null;
+  let responded = false;
+  harness.listeners.get("fetch")!({
+    request: new Request(`${ORIGIN}${path}`, {
+      headers: { accept: "text/html" },
+    }),
+    respondWith: (p: Promise<Response>) => {
+      responded = true;
+      captured = p;
+    },
+    preloadResponse,
+    waitUntil: (p: Promise<unknown>) => {
+      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();

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({

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 (

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