diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ec4e1475..7d3e79705 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ ## [Unreleased] +## [1.32.5] — 2026-07-23 + +- **Browser SSO sign-in no longer fails on a duplicated callback.** With the + service worker's navigation preload enabled, a single-use OIDC callback could + be requested twice — the first redeemed the authorization code and set the + session, the second hit the identity provider with the now-consumed code and + failed, landing every successful sign-in on an error page. The service worker + now settles one-shot auth navigations itself so the callback reaches the + server exactly once, and the callback is idempotent: a duplicate that already + holds a database-valid session is sent into the app instead of erroring, + while a forged or session-less state still fails closed and never redeems a + code. + +No migrations. No breaking changes. + ## [1.32.4] — 2026-07-23 - **The Coach's numeric verifier stops flagging figures it was actually given.** diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 0992bcce2..84d9f13c5 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: HealthLog API - version: 1.32.4 + version: 1.32.5 description: >- Self-hosted personal-health-tracking PWA — public API surface for the iOS native client and external ingest. diff --git a/package.json b/package.json index f989be492..c47434ba7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "healthlog", - "version": "1.32.4", + "version": "1.32.5", "description": "Self-hosted personal-health-tracking PWA with Withings integration, AI insights, and doctor-report PDF export.", "license": "PolyForm-Noncommercial-1.0.0", "homepage": "https://healthlog.dev", diff --git a/public/sw.js b/public/sw.js index f6fe71355..a67fb5689 100644 --- a/public/sw.js +++ b/public/sw.js @@ -36,7 +36,7 @@ try { // v1.4.38.4 → v1.4.42. Do not hand-edit; bump `package.json` and rebuild. const CACHE_VERSION = (typeof self !== "undefined" && self.__APP_VERSION__) || - /* @sw-version-fallback */ "v1.32.4"; + /* @sw-version-fallback */ "v1.32.5"; const STATIC_CACHE = `healthlog-static-${CACHE_VERSION}`; const PAGE_CACHE = `healthlog-pages-${CACHE_VERSION}`; // v1.18.6 — read-only data cache for a curated allowlist of safe GET `/api/*` @@ -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 diff --git a/tests/integration/session-cookie-validation.test.ts b/tests/integration/session-cookie-validation.test.ts new file mode 100644 index 000000000..65139d918 --- /dev/null +++ b/tests/integration/session-cookie-validation.test.ts @@ -0,0 +1,84 @@ +/** + * Integration guard for `validateSessionFromCookieValue` against a real + * Postgres. + * + * The OIDC callback's duplicate-detection path relies on this helper to tell a + * duplicate of a completed sign-in (a live session → redirect into the app) + * apart from a forged/expired callback (no session → fail closed). The unit + * suite stubs the resolver out, so the hashing + row lookup + expiry that + * actually decide that branch are only exercised here. + */ +import { beforeEach, describe, expect, it } from "vitest"; + +process.env.ENCRYPTION_KEY ??= + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + +import { getPrismaClient, truncateAllTables } from "./setup"; +import { hashToken } from "@/lib/auth/hmac"; +import { validateSessionFromCookieValue } from "@/lib/auth/session"; + +const prisma = getPrismaClient(); + +beforeEach(async () => { + await truncateAllTables(prisma); +}); + +async function makeUser(username: string) { + return prisma.user.create({ + data: { username, email: `${username}@example.test`, role: "USER" }, + }); +} + +describe("validateSessionFromCookieValue (integration)", () => { + it("resolves the user for a live secret-backed session", async () => { + const user = await makeUser("oidc-dup-live"); + const secret = "hls_" + "a".repeat(64); + await prisma.session.create({ + data: { + userId: user.id, + tokenHash: hashToken(secret), + expiresAt: new Date(Date.now() + 60_000), + }, + }); + + const resolved = await validateSessionFromCookieValue(secret); + expect(resolved?.user.id).toBe(user.id); + }); + + it("returns null for an expired secret-backed session (fail closed)", async () => { + const user = await makeUser("oidc-dup-expired"); + const secret = "hls_" + "b".repeat(64); + await prisma.session.create({ + data: { + userId: user.id, + tokenHash: hashToken(secret), + expiresAt: new Date(Date.now() - 1_000), + }, + }); + + expect(await validateSessionFromCookieValue(secret)).toBeNull(); + }); + + it("returns null for an unknown / forged / empty cookie value", async () => { + expect( + await validateSessionFromCookieValue("hls_" + "c".repeat(64)), + ).toBeNull(); + expect(await validateSessionFromCookieValue(undefined)).toBeNull(); + expect(await validateSessionFromCookieValue("")).toBeNull(); + }); + + it("resolves a legacy cuid-cookie session while its tokenHash is null", async () => { + const user = await makeUser("oidc-dup-legacy"); + const legacy = await prisma.session.create({ + data: { + userId: user.id, + expiresAt: new Date(Date.now() + 60_000), + }, + }); + + // The pre-upgrade cookie carries the row id; it is honoured only while the + // row has not yet retired its id in favour of a hashed secret. + const resolved = await validateSessionFromCookieValue(legacy.id); + expect(resolved?.user.id).toBe(user.id); + }); +});