From 7592c739470642abd850a230bb1632d7c99ee2f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Tue, 14 Jul 2026 11:53:53 +0200 Subject: [PATCH] fix(auth): harden the OIDC token path and state-cookie lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Token verification: jwtVerify now carries a closed asymmetric algorithm allowlist (RS256/PS256/ES256/EdDSA — an HMAC alg would let anyone with the client secret forge identities) and a 60 s clock tolerance for the usual self-host drift between IdP and app box. The JWKS fetch — jose's own, outside safeFetch by design — is disciplined instead by pinning the jwks_uri host to the configured issuer's host byte-exact and passing the same 15 s timeout safeFetch defaults to. requirePublicHost stays deliberately unused on this surface: LAN/Tailscale-only IdPs are a legitimate topology and the issuer is operator config, not user input. Discovery: the issuer comparison now tolerates exactly one trailing slash on either side (a recurring IdP/operator mismatch); token verification still matches the provider-sent issuer byte-exact. State cookie: the cookie is set under Path=/api/auth/oidc but was deleted without a path — RFC 6265 keys cookies by (name, domain, path), so the deletion never matched and the single-use blob survived its whole TTL. Every delete now repeats the exact path, via a shared constant. Also: the IdP error code recorded in the wide event is whitelisted against the closed OAuth error-code set (unknown values collapse to "other"); the status route drops an unreachable try/catch and a duplicated annotate; the passkey conditional-UI autofill stays unarmed under OIDC_ONLY (passkey login is a server-enforced dead end there); env-manifest gains the three optional OIDC_* tuning vars; the env example notes why the issuer should be HTTPS. --- .env.production.example | 4 + scripts/env-manifest.json | 19 +++++ .../oidc/callback/__tests__/route.test.ts | 21 +++++ src/app/api/auth/oidc/callback/route.ts | 51 +++++++++++- src/app/api/auth/oidc/login/route.ts | 10 ++- src/app/api/auth/oidc/status/route.ts | 23 ++--- src/app/auth/login/page.tsx | 12 ++- src/lib/auth/__tests__/oidc.test.ts | 83 ++++++++++++++++++- src/lib/auth/oidc-cookie.ts | 7 ++ src/lib/auth/oidc.ts | 54 +++++++++--- 10 files changed, 252 insertions(+), 32 deletions(-) diff --git a/.env.production.example b/.env.production.example index 7958d328a..ba8c95224 100644 --- a/.env.production.example +++ b/.env.production.example @@ -119,6 +119,10 @@ API_TOKEN_HMAC_KEY="" # flow for it yet -- so OIDC_ONLY="true" locks the iOS app out of sign-in # entirely until a native SSO flow ships. Do not enable it on a deployment # with active iOS users. +# Serve the issuer over HTTPS wherever possible: sign-in material (the +# authorization code, the token-endpoint response carrying the ID token) +# crosses this connection. Plain HTTP is defensible only on a fully trusted +# private network (IdP and app on the same box, a Tailscale mesh, a VPN). # OIDC_ISSUER_URL="" # OIDC_CLIENT_ID="" # OIDC_CLIENT_SECRET="" diff --git a/scripts/env-manifest.json b/scripts/env-manifest.json index effc45fda..f65d9e554 100644 --- a/scripts/env-manifest.json +++ b/scripts/env-manifest.json @@ -98,6 +98,25 @@ } ] }, + { + "name": "OIDC SSO options", + "description": "Optional tuning for an enabled OIDC provider. Each stands alone (deliberately NOT allOrNone — any subset is valid) and each has a safe default when unset.", + "required": false, + "variables": [ + { + "name": "OIDC_SCOPES", + "purpose": "Space-separated scopes requested at authorization. Default: \"openid email profile\"." + }, + { + "name": "OIDC_BUTTON_LABEL", + "purpose": "Label on the login page's SSO button. Default: \"Single Sign-On\"." + }, + { + "name": "OIDC_ONLY", + "purpose": "\"true\" disables password/passkey login server-side (only effective when the OIDC provider group is fully configured). Default: additive login. Locks the native iOS app out of sign-in." + } + ] + }, { "name": "APNs (iOS push)", "description": "All four core APNS_* vars together. Missing any disables APNs silently — the v1.4.40 AP-2 gap that motivated this manifest. allOrNone trips the synthetic required row whenever 1-3 of the 4 are set.", 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 fd48ea383..fa4dec016 100644 --- a/src/app/api/auth/oidc/callback/__tests__/route.test.ts +++ b/src/app/api/auth/oidc/callback/__tests__/route.test.ts @@ -459,4 +459,25 @@ describe("GET /api/auth/oidc/callback", () => { const res = await GET(validRequest()); expect(res.headers.get("location")).toContain("error=oidc_failed"); }); + + it("deletes the state cookie with the exact path it was set under", async () => { + // RFC 6265 keys cookies by (name, domain, path) — a deletion without + // `Path=/api/auth/oidc` would target a `/`-scoped cookie that does not + // exist and leave the real single-use blob alive. + const errorRes = await GET(makeRequest({ query: "?error=access_denied" })); + const errorCookie = errorRes.headers + .getSetCookie() + .find((c) => c.startsWith("oidc_auth_state="))!; + expect(errorCookie.toLowerCase()).toContain("path=/api/auth/oidc"); + + mockIdentity(); + mockUserLookups({ + byIdentity: userRow({ oidcIssuer: METADATA.issuer, oidcSub: "sub-1" }), + }); + const successRes = await GET(validRequest()); + const successCookie = successRes.headers + .getSetCookie() + .find((c) => c.startsWith("oidc_auth_state="))!; + expect(successCookie.toLowerCase()).toContain("path=/api/auth/oidc"); + }); }); diff --git a/src/app/api/auth/oidc/callback/route.ts b/src/app/api/auth/oidc/callback/route.ts index f1d8bca08..0311a179e 100644 --- a/src/app/api/auth/oidc/callback/route.ts +++ b/src/app/api/auth/oidc/callback/route.ts @@ -22,6 +22,7 @@ import { OIDC_MFA_COOKIE_PATH, OIDC_MFA_TTL_MS, OIDC_STATE_COOKIE, + OIDC_STATE_COOKIE_PATH, } from "@/lib/auth/oidc-cookie"; import { createMfaChallenge } from "@/lib/auth/mfa/challenge"; import { syncMfaEnrollCookie } from "@/lib/auth/mfa-enrollment"; @@ -34,10 +35,47 @@ function errorRedirect(req: NextRequest, reason: string): NextResponse { const response = NextResponse.redirect( new URL(`${LOGIN_ERROR_URL}?error=${reason}`, req.url), ); - response.cookies.delete(OIDC_STATE_COOKIE); + deleteStateCookie(response); return response; } +/** + * The state cookie is set with `path: "/api/auth/oidc"`; the delete must + * repeat that path (RFC 6265 keys cookies by name+domain+path — a bare + * delete would target `/` and never match, leaving the single-use blob + * alive for the rest of its TTL). + */ +function deleteStateCookie(response: NextResponse): void { + response.cookies.delete({ + name: OIDC_STATE_COOKIE, + path: OIDC_STATE_COOKIE_PATH, + }); +} + +/** + * Closed set of RFC 6749 §4.1.2.1 / OIDC Core §3.1.2.6 authorization error + * codes. The wide-event meta only ever carries a member of this set (or + * "other") — never the raw, attacker-influenceable query value. + */ +const KNOWN_IDP_ERROR_CODES = new Set([ + "access_denied", + "invalid_request", + "unauthorized_client", + "unsupported_response_type", + "invalid_scope", + "server_error", + "temporarily_unavailable", + "interaction_required", + "login_required", + "account_selection_required", + "consent_required", + "invalid_request_uri", + "invalid_request_object", + "request_not_supported", + "request_uri_not_supported", + "registration_not_supported", +]); + interface StoredOidcState { state: string; nonce: string; @@ -82,7 +120,12 @@ export const GET = apiHandler(async (req: NextRequest) => { const { searchParams } = req.nextUrl; const idpError = searchParams.get("error"); if (idpError) { - annotate({ meta: { reason: "idp_denied", idpError } }); + annotate({ + meta: { + reason: "idp_denied", + idpError: KNOWN_IDP_ERROR_CODES.has(idpError) ? idpError : "other", + }, + }); return errorRedirect(req, "oidc_denied"); } @@ -299,7 +342,7 @@ export const GET = apiHandler(async (req: NextRequest) => { loginUrl.searchParams.set("next", stored.next); } const response = NextResponse.redirect(loginUrl); - response.cookies.delete(OIDC_STATE_COOKIE); + deleteStateCookie(response); response.cookies.set( OIDC_MFA_COOKIE, JSON.stringify({ ticket: challenge.ticket, methods }), @@ -341,7 +384,7 @@ export const GET = apiHandler(async (req: NextRequest) => { await auditLog("auth.oidc.login", { userId: user.id, ipAddress: ip }); const response = NextResponse.redirect(new URL(stored.next, req.url)); - response.cookies.delete(OIDC_STATE_COOKIE); + deleteStateCookie(response); return response; } catch (err) { getEvent()?.setError(err); diff --git a/src/app/api/auth/oidc/login/route.ts b/src/app/api/auth/oidc/login/route.ts index 8d21cc3e6..2c4987ade 100644 --- a/src/app/api/auth/oidc/login/route.ts +++ b/src/app/api/auth/oidc/login/route.ts @@ -13,7 +13,11 @@ import { getOidcRedirectUri, sanitizeOidcNextPath, } from "@/lib/auth/oidc"; -import { OIDC_STATE_COOKIE, OIDC_STATE_TTL_MS } from "@/lib/auth/oidc-cookie"; +import { + OIDC_STATE_COOKIE, + OIDC_STATE_COOKIE_PATH, + OIDC_STATE_TTL_MS, +} from "@/lib/auth/oidc-cookie"; /** * Redirects the browser to the configured OIDC provider's authorization @@ -84,7 +88,9 @@ export const GET = apiHandler(async (req: NextRequest) => { // the session cookie and the Withings OAuth state cookie. sameSite: "lax", maxAge: Math.floor(OIDC_STATE_TTL_MS / 1000), - path: "/api/auth/oidc", + // Shared constant — the callback's delete must repeat this exact path + // (RFC 6265 keys cookies by name+domain+path). + path: OIDC_STATE_COOKIE_PATH, }); return response; diff --git a/src/app/api/auth/oidc/status/route.ts b/src/app/api/auth/oidc/status/route.ts index a51b115f7..aa1178609 100644 --- a/src/app/api/auth/oidc/status/route.ts +++ b/src/app/api/auth/oidc/status/route.ts @@ -6,19 +6,14 @@ import { getOidcConfig, isOidcOnly } from "@/lib/auth/oidc"; export const dynamic = "force-dynamic"; /** Public — the login page reads this pre-session to decide whether to - * render the SSO button and whether to hide password/passkey login. */ + * render the SSO button and whether to hide password/passkey login. Pure + * env reads; nothing here can throw. */ export const GET = apiHandler(async () => { - try { - const config = getOidcConfig(); - annotate({ action: { name: "auth.oidc.status" } }); - return apiSuccess({ - enabled: config !== null, - buttonLabel: config?.buttonLabel ?? null, - only: isOidcOnly(), - }); - } catch { - // Fail closed, matching registration-status. - annotate({ action: { name: "auth.oidc.status" } }); - return apiSuccess({ enabled: false, buttonLabel: null, only: false }); - } + annotate({ action: { name: "auth.oidc.status" } }); + const config = getOidcConfig(); + return apiSuccess({ + enabled: config !== null, + buttonLabel: config?.buttonLabel ?? null, + only: isOidcOnly(), + }); }); diff --git a/src/app/auth/login/page.tsx b/src/app/auth/login/page.tsx index 87e6a6ead..274fb870b 100644 --- a/src/app/auth/login/page.tsx +++ b/src/app/auth/login/page.tsx @@ -238,6 +238,11 @@ export default function LoginPage() { methods?: MfaMethod[]; }; if (parsed.ticket) { + // A lazy useState initializer cannot replace this: `document` does + // not exist during the server render, and seeding client-only state + // there would desync hydration. Mount-time pickup of an external + // one-shot store is exactly the effect escape hatch. + // eslint-disable-next-line react-hooks/set-state-in-effect setMfaChallenge({ ticket: parsed.ticket, methods: parsed.methods ?? ["totp", "recovery"], @@ -252,7 +257,12 @@ export default function LoginPage() { // arm a discoverable-credential assertion bound to the username field so a // returning user can pick their passkey straight from the autofill prompt. // Best-effort: any abort (the user types a password instead) is swallowed. + // Waits for the OIDC status and stays off entirely under OIDC_ONLY — + // passkey login is a dead end there (server-enforced), so arming the + // browser's credential prompt would only dangle a door that is locked. useEffect(() => { + if (oidcStatus === undefined) return; + if (oidcStatus.only === true) return; if (autofillStarted.current) return; autofillStarted.current = true; let cancelled = false; @@ -291,7 +301,7 @@ export default function LoginPage() { cancelled = true; }; // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [oidcStatus]); return (
diff --git a/src/lib/auth/__tests__/oidc.test.ts b/src/lib/auth/__tests__/oidc.test.ts index 3cf9c37a0..233129fc6 100644 --- a/src/lib/auth/__tests__/oidc.test.ts +++ b/src/lib/auth/__tests__/oidc.test.ts @@ -7,7 +7,10 @@ vi.mock("jose", () => ({ })); import { safeFetch } from "@/lib/safe-fetch"; -import { jwtVerify as mockJwtVerify } from "jose"; +import { + createRemoteJWKSet as mockCreateRemoteJWKSet, + jwtVerify as mockJwtVerify, +} from "jose"; import { _resetOidcCacheForTests, buildAuthorizationUrl, @@ -195,6 +198,28 @@ describe("discoverOidcMetadata", () => { ); }); + it("tolerates exactly one trailing slash between config and discovery issuer", async () => { + // Several IdPs mint their issuer with a canonical trailing slash while + // the operator configures without one. The provider-sent form is kept + // in the metadata — the ID token's `iss` must match IT byte-exact. + vi.mocked(safeFetch).mockResolvedValue( + jsonResponse({ ...METADATA_DOC, issuer: "https://idp.example.com/" }), + ); + const config = getOidcConfig()!; + const metadata = await discoverOidcMetadata(config); + expect(metadata.issuer).toBe("https://idp.example.com/"); + }); + + it("does not stretch the slash tolerance beyond a single trailing slash", async () => { + vi.mocked(safeFetch).mockResolvedValue( + jsonResponse({ ...METADATA_DOC, issuer: "https://idp.example.com//" }), + ); + const config = getOidcConfig()!; + await expect(discoverOidcMetadata(config)).rejects.toThrow( + /issuer mismatch/, + ); + }); + it("rejects a discovery doc missing required fields", async () => { vi.mocked(safeFetch).mockResolvedValue( jsonResponse({ issuer: METADATA_DOC.issuer }), @@ -344,6 +369,62 @@ describe("verifyIdToken", () => { }), ).rejects.toThrow(/missing sub claim/); }); + + it("verifies with a closed asymmetric algorithm allowlist and bounded clock tolerance", async () => { + vi.mocked(mockJwtVerify).mockResolvedValue({ + payload: { sub: "user-sub-1", nonce: "nonce-1" }, + } as never); + const config = getOidcConfig()!; + await verifyIdToken({ + metadata: METADATA_DOC, + config, + idToken: "id-token", + nonce: "nonce-1", + }); + expect(mockJwtVerify).toHaveBeenCalledWith( + "id-token", + expect.anything(), + expect.objectContaining({ + issuer: METADATA_DOC.issuer, + audience: config.clientId, + algorithms: ["RS256", "PS256", "ES256", "EdDSA"], + clockTolerance: "60s", + }), + ); + }); + + it("pins the JWKS fetch to the issuer's host with a bounded timeout", async () => { + vi.mocked(mockJwtVerify).mockResolvedValue({ + payload: { sub: "user-sub-1", nonce: "nonce-1" }, + } as never); + const config = getOidcConfig()!; + await verifyIdToken({ + metadata: METADATA_DOC, + config, + idToken: "id-token", + nonce: "nonce-1", + }); + expect(mockCreateRemoteJWKSet).toHaveBeenCalledWith( + new URL(METADATA_DOC.jwks_uri), + { timeoutDuration: 15_000 }, + ); + }); + + it("rejects a discovery doc whose jwks_uri points at a foreign host", async () => { + const config = getOidcConfig()!; + await expect( + verifyIdToken({ + metadata: { + ...METADATA_DOC, + jwks_uri: "https://evil.example.com/jwks", + }, + config, + idToken: "id-token", + nonce: "nonce-1", + }), + ).rejects.toThrow(/jwks_uri host mismatch/); + expect(mockCreateRemoteJWKSet).not.toHaveBeenCalled(); + }); }); describe("fetchUserinfoEmail", () => { diff --git a/src/lib/auth/oidc-cookie.ts b/src/lib/auth/oidc-cookie.ts index 8a860905a..32da00289 100644 --- a/src/lib/auth/oidc-cookie.ts +++ b/src/lib/auth/oidc-cookie.ts @@ -4,6 +4,13 @@ * codeVerifier, next}` across the browser round-trip to the IdP and back. */ export const OIDC_STATE_COOKIE = "oidc_auth_state"; +/** + * RFC 6265 keys a cookie by (name, domain, path) — a delete must repeat the + * exact path the set used or it silently targets a different (non-existent) + * cookie and the stale state blob survives. Every set AND delete goes + * through this constant. + */ +export const OIDC_STATE_COOKIE_PATH = "/api/auth/oidc"; export const OIDC_STATE_TTL_MS = 10 * 60 * 1000; /** diff --git a/src/lib/auth/oidc.ts b/src/lib/auth/oidc.ts index 5401a8c5f..4a0b011c3 100644 --- a/src/lib/auth/oidc.ts +++ b/src/lib/auth/oidc.ts @@ -7,6 +7,11 @@ * OAuth server) rather than pulling in a full OAuth/OIDC framework; `jose` * is used only for JWKS fetch + ID-token signature/claims verification, * which is not worth hand-rolling. + * + * None of the outbound calls opt into `requirePublicHost`: a LAN- or + * Tailscale-only IdP (Authentik/Keycloak/Authelia on the same box) is a + * legitimate, common self-host topology, and the issuer comes from operator + * env config — never from user input. */ import { createRemoteJWKSet, jwtVerify } from "jose"; import { safeFetch } from "@/lib/safe-fetch"; @@ -117,7 +122,13 @@ function isRecord(value: unknown): value is Record { * logins don't stampede the discovery endpoint. The doc's own `issuer` * claim must match the configured issuer URL (RFC 8414 §3.3) — a * mismatch means the provider is misconfigured or something is spoofing - * the discovery response. + * the discovery response. The comparison tolerates exactly ONE trailing + * slash on either side (several IdPs mint their issuer with a canonical + * trailing slash while operators configure without one, and vice versa); + * anything beyond that single-slash normalisation is still a hard reject. + * The provider-sent `issuer` is what gets cached and later matched against + * the ID token's `iss` claim byte-exact — the tolerance applies only to + * the operator-config comparison, never to token verification. */ export async function discoverOidcMetadata( config: OidcConfig, @@ -147,7 +158,12 @@ export async function discoverOidcMetadata( ) { throw new Error("discovery document missing required fields"); } - if (json.issuer !== config.issuerUrl) { + const stripOneTrailingSlash = (value: string): string => + value.endsWith("/") ? value.slice(0, -1) : value; + if ( + stripOneTrailingSlash(json.issuer) !== + stripOneTrailingSlash(config.issuerUrl) + ) { throw new Error( `discovery issuer mismatch: expected ${config.issuerUrl}, got ${json.issuer}`, ); @@ -172,16 +188,25 @@ export async function discoverOidcMetadata( return metadataInFlight; } -function getJwks(metadata: OidcMetadata) { +function getJwks(metadata: OidcMetadata, config: OidcConfig) { if (jwksCache && jwksCache.issuerUrl === metadata.issuer) { return jwksCache.jwks; } - // `jose`'s own fetch to the trusted, discovery-supplied `jwks_uri` is a - // deliberate, scoped exception to the safe-fetch-required convention — - // the target host comes from the operator-configured issuer, never from - // user input, and the ESLint rule only lints project source, not - // library internals. - const jwks = createRemoteJWKSet(new URL(metadata.jwks_uri)); + // `jose`'s own fetch to the discovery-supplied `jwks_uri` is a + // deliberate, scoped exception to the safe-fetch-required convention + // (the ESLint rule only lints project source, not library internals). + // It is disciplined two ways instead: the `jwks_uri` host must exactly + // equal the operator-configured issuer's host — a spoofed or sloppy + // discovery doc cannot point key fetching at a third party — and the + // fetch carries the same 15 s timeout `safeFetch` defaults to. + const jwksUrl = new URL(metadata.jwks_uri); + const issuerHost = new URL(config.issuerUrl).host; + if (jwksUrl.host !== issuerHost) { + throw new Error( + `jwks_uri host mismatch: expected ${issuerHost}, got ${jwksUrl.host}`, + ); + } + const jwks = createRemoteJWKSet(jwksUrl, { timeoutDuration: 15_000 }); jwksCache = { issuerUrl: metadata.issuer, jwks }; return jwks; } @@ -268,10 +293,19 @@ export async function verifyIdToken(params: { idToken: string; nonce: string; }): Promise { - const jwks = getJwks(params.metadata); + const jwks = getJwks(params.metadata, params.config); const { payload } = await jwtVerify(params.idToken, jwks, { issuer: params.metadata.issuer, audience: params.config.clientId, + // Closed allowlist of asymmetric signature algorithms — every ID token + // must be provider-key signed. Never extend this with an HMAC (HS*) + // entry: a symmetric alg would let anyone who knows the client secret + // forge identities. + algorithms: ["RS256", "PS256", "ES256", "EdDSA"], + // Self-hosted IdP + app on different boxes routinely drift a few + // seconds apart; a minute of tolerance on exp/iat/nbf absorbs that + // without meaningfully extending a token's life. + clockTolerance: "60s", }); if (typeof payload.sub !== "string" || !payload.sub) {