From 857350012184157a58d6f88145f049b621a6c16e Mon Sep 17 00:00:00 2001 From: mptyl Date: Mon, 17 Aug 2026 06:18:49 +0200 Subject: [PATCH] fix(auth): harden OIDC browser transactions --- backend/src/auth/oidc-client.ts | 67 ++++++++++- backend/src/auth/routes.ts | 94 ++++++++++++--- backend/src/auth/session-store.ts | 4 + backend/src/auth/types.ts | 1 + backend/test/auth-routes-oidc.test.ts | 151 ++++++++++++++++++++---- backend/test/auth-session-store.test.ts | 10 +- backend/test/oidc-client.test.ts | 69 ++++++++++- frontend/src/auth/LoginPage.test.tsx | 37 ++++++ frontend/src/auth/LoginPage.tsx | 10 -- 9 files changed, 384 insertions(+), 59 deletions(-) diff --git a/backend/src/auth/oidc-client.ts b/backend/src/auth/oidc-client.ts index c039919f..5af6b7b8 100644 --- a/backend/src/auth/oidc-client.ts +++ b/backend/src/auth/oidc-client.ts @@ -37,12 +37,15 @@ export interface OidcProtocolOptions { scopes: readonly string[]; groupsClaim: string; fetch?: typeof globalThis.fetch; + jwksTimeoutMs?: number; } const MAX_GROUPS = 128; const MAX_GROUP_LENGTH = 256; const MAX_ID_TOKEN_LENGTH = 16 * 1024; const MAX_JWKS_BYTES = 1024 * 1024; +const DEFAULT_JWKS_TIMEOUT_MS = 5_000; +const MAX_JWKS_TIMEOUT_MS = 30_000; const text = (value: unknown, maximum = 2048): value is string => typeof value === "string" && value.length > 0 && value.length <= maximum && !/\p{Cc}/u.test(value); @@ -75,7 +78,7 @@ function groupsFromClaims(claims: Record, name: string): string && Object.prototype.hasOwnProperty.call(indirect, name)) || claims.hasgroups === true) throw new OidcProtocolError(); const raw = claims[name]; - if (!Array.isArray(raw) || raw.length > MAX_GROUPS) throw new OidcProtocolError(); + if (!Array.isArray(raw) || raw.length === 0 || raw.length > MAX_GROUPS) throw new OidcProtocolError(); const groups: string[] = []; const unique = new Set(); for (const group of raw) { @@ -161,6 +164,54 @@ function joseEcdsaSignatureToDer(signature: Buffer, partLength: number): Buffer return Buffer.concat([Buffer.from([0x30]), derLength(sequence.length), sequence]); } +async function readWithAbort( + reader: ReadableStreamDefaultReader, + signal: AbortSignal, +): Promise> { + signal.throwIfAborted(); + return await new Promise((resolve, reject) => { + const aborted = () => { + void reader.cancel().catch(() => undefined); + reject(signal.reason); + }; + signal.addEventListener("abort", aborted, { once: true }); + reader.read().then(resolve, reject).finally(() => signal.removeEventListener("abort", aborted)); + }); +} + +async function boundedJwksBody(response: Response, signal: AbortSignal): Promise { + const declaredLength = response.headers.get("content-length"); + if (declaredLength !== null) { + if (!/^\d+$/.test(declaredLength)) throw new OidcProtocolError(); + const length = Number(declaredLength); + if (!Number.isSafeInteger(length) || length > MAX_JWKS_BYTES) throw new OidcProtocolError(); + } + if (!response.body) throw new OidcProtocolError(); + const reader = response.body.getReader(); + const chunks: Buffer[] = []; + let total = 0; + try { + while (true) { + const { done, value } = await readWithAbort(reader, signal); + if (done) break; + if (value.byteLength > MAX_JWKS_BYTES - total) { + await reader.cancel().catch(() => undefined); + throw new OidcProtocolError(); + } + total += value.byteLength; + chunks.push(Buffer.from(value)); + } + } finally { + reader.releaseLock(); + } + signal.throwIfAborted(); + try { + return new TextDecoder("utf-8", { fatal: true }).decode(Buffer.concat(chunks, total)); + } catch { + throw new OidcProtocolError(); + } +} + async function verifyIdTokenSignature( idToken: unknown, config: Configuration, @@ -176,14 +227,16 @@ async function verifyIdTokenSignature( || !metadata.id_token_signing_alg_values_supported.includes(header.alg) || !text(metadata.jwks_uri, 2048)) throw new OidcProtocolError(); const jwksUrl = httpsEndpoint(metadata.jwks_uri); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), options.jwksTimeoutMs ?? DEFAULT_JWKS_TIMEOUT_MS); + timeout.unref(); let response: Response; try { response = await (options.fetch ?? globalThis.fetch)(jwksUrl, { - headers: { accept: "application/json" }, redirect: "error", + headers: { accept: "application/json" }, redirect: "error", signal: controller.signal, }); if (!response.ok) throw new OidcProtocolError(); - const body = await response.text(); - if (Buffer.byteLength(body, "utf8") > MAX_JWKS_BYTES) throw new OidcProtocolError(); + const body = await boundedJwksBody(response, controller.signal); const parsed = JSON.parse(body) as { keys?: unknown }; if (!Array.isArray(parsed.keys) || parsed.keys.length === 0 || parsed.keys.length > 16) throw new OidcProtocolError(); const matching = parsed.keys.filter((key): key is Record => @@ -209,6 +262,8 @@ async function verifyIdTokenSignature( } catch (error) { if (error instanceof OidcProtocolError) throw error; throw new OidcProtocolError(); + } finally { + clearTimeout(timeout); } } @@ -217,7 +272,9 @@ export function createOidcProtocol(options: OidcProtocolOptions): OidcProtocol { const callbackUrl = configuredUrl(options.callbackUrl); if (!text(options.clientId, 512) || !text(options.clientSecret, 4096) || !text(options.groupsClaim, 128) || options.scopes.length === 0 || options.scopes.length > 16 - || options.scopes.some((scope) => !text(scope, 128))) throw new OidcProtocolError(); + || options.scopes.some((scope) => !text(scope, 128)) + || (options.jwksTimeoutMs !== undefined && (!Number.isSafeInteger(options.jwksTimeoutMs) + || options.jwksTimeoutMs < 1 || options.jwksTimeoutMs > MAX_JWKS_TIMEOUT_MS))) throw new OidcProtocolError(); let discovered: Promise | undefined; const configuration = async (): Promise => { diff --git a/backend/src/auth/routes.ts b/backend/src/auth/routes.ts index 9fd80297..9c414b61 100644 --- a/backend/src/auth/routes.ts +++ b/backend/src/auth/routes.ts @@ -1,5 +1,5 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; -import { randomBytes } from "node:crypto"; +import { createHash, randomBytes, timingSafeEqual } from "node:crypto"; import type { AuthenticationConfigProvider, LoadedAuthConfig, OidcAuthenticationConfig, Role } from "./types.js"; import type { LocalUserRecord, LocalUserRegistry } from "./local-registry.js"; import type { AuthSessionStore } from "./session-store.js"; @@ -17,6 +17,9 @@ const MAX_PASSWORD_LENGTH = 1024; const MAX_LIMIT_ENTRIES = 10_000; const MAX_OIDC_CALLBACK_QUERY_LENGTH = 4096; const OIDC_CALLBACK_PATH = "/api/auth/oidc/callback"; +const OIDC_TRANSACTION_COOKIE = "__Host-thothii_oidc_tx"; +const OIDC_TRANSACTION_COOKIE_SECONDS = TEN_MINUTES_MS / 1000; +const OIDC_VALUE_PATTERN = /^[A-Za-z0-9_-]{43}$/; export interface AuthRouteDependencies { authMode: "local" | "oidc" | "upstream" | "none" | "mock"; @@ -188,9 +191,13 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDependen app.get("/auth/oidc/login", async (request, reply) => { const loaded = captureAuthConfigSnapshot(request, deps.authentication); const configured = currentOidcConfig(loaded, deps); - if (!configured || !deps.sessionStore) return unavailable(reply); + if (!configured || !deps.sessionStore) { + clearOidcTransactionCookie(reply); + return unavailable(reply); + } const nonce = randomOidcValue(); const codeVerifier = randomOidcValue(); + const browserTransaction = randomOidcValue(); try { const created = await deps.sessionStore.createOidcState({ nonce, @@ -198,24 +205,28 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDependen returnTo: "/", authConfigRevision: configured.loaded.revision, issuer: configured.config.oidc.issuer, + browserTransactionDigest: oidcTransactionDigest(browserTransaction).toString("hex"), }); try { const location = await configured.protocol.authorizationUrl({ state: created.state, nonce, codeVerifier }); + reply.setCookie(OIDC_TRANSACTION_COOKIE, browserTransaction, oidcTransactionCookieOptions()); return reply.redirect(location.href); } catch { await deps.sessionStore.consumeOidcState(created.state).catch(() => undefined); + clearOidcTransactionCookie(reply); return unavailable(reply); } } catch { + clearOidcTransactionCookie(reply); return unavailable(reply); } }); app.get("/auth/oidc/callback", async (request, reply) => { const loaded = captureAuthConfigSnapshot(request, deps.authentication); - if (!loaded || !deps.sessionStore) return oidcCallbackFailed(reply); - const callback = oidcCallbackUrl(request, loaded.value.publicUrl); - if (!callback) return oidcCallbackFailed(reply); + clearOidcTransactionCookie(reply); + const callback = oidcCallbackUrl(request, loaded?.value.publicUrl); + if (!deps.sessionStore || !callback.state) return oidcCallbackFailed(reply); let state; try { state = await deps.sessionStore.consumeOidcState(callback.state); @@ -223,7 +234,9 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDependen return oidcCallbackFailed(reply); } const configured = currentOidcConfig(loaded, deps); - if (!configured || !state || state.returnTo !== "/" || state.authConfigRevision !== configured.loaded.revision + if (!callback.currentUrl || !configured || !state || state.returnTo !== "/" + || !oidcTransactionMatches(request.cookies[OIDC_TRANSACTION_COOKIE], state.browserTransactionDigest) + || state.authConfigRevision !== configured.loaded.revision || state.issuer !== configured.config.oidc.issuer) { return oidcCallbackFailed(reply); } @@ -234,11 +247,13 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDependen nonce: state.nonce, codeVerifier: state.codeVerifier, }); - if (identity.issuer !== configured.config.oidc.issuer) return oidcCallbackFailed(reply); + if (identity.issuer !== configured.config.oidc.issuer || !Array.isArray(identity.groups) + || identity.groups.length === 0) return oidcCallbackFailed(reply); const roles = oidcRoles(identity.groups, configured.config); + const now = new Date(); const absoluteTtlMs = Math.min( configured.config.session.oidcTtlSeconds * 1000, - identity.tokenExpiresAt.getTime() - Date.now(), + identity.tokenExpiresAt.getTime() - now.getTime(), ); if (!Number.isSafeInteger(absoluteTtlMs) || absoluteTtlMs <= 0) return oidcCallbackFailed(reply); const created = await deps.sessionStore.create({ @@ -255,7 +270,7 @@ export function registerAuthRoutes(app: FastifyInstance, deps: AuthRouteDependen authConfigRevision: configured.loaded.revision, idleTtlMs: configured.config.session.regularIdleSeconds * 1000, absoluteTtlMs, - }); + }, now); reply.setCookie(sessionCookieName(), created.token, cookieOptions(configured.loaded, false)); return reply.redirect(state.returnTo); } catch { @@ -370,26 +385,69 @@ function randomOidcValue(): string { return randomBytes(32).toString("base64url"); } -function oidcCallbackUrl(request: FastifyRequest, publicUrl: string): { currentUrl: URL; state: string } | undefined { - if (request.url.length > MAX_OIDC_CALLBACK_QUERY_LENGTH) return undefined; +function oidcTransactionDigest(value: string): Buffer { + return createHash("sha256").update(value, "utf8").digest(); +} + +function oidcTransactionMatches(value: string | undefined, expectedDigest: string): boolean { + const canonical = typeof value === "string" && OIDC_VALUE_PATTERN.test(value); + const expectedCanonical = /^[a-f0-9]{64}$/.test(expectedDigest); + const supplied = oidcTransactionDigest(canonical ? value : ""); + const expected = expectedCanonical ? Buffer.from(expectedDigest, "hex") : Buffer.alloc(32); + const matches = timingSafeEqual(supplied, expected); + return canonical && expectedCanonical && matches; +} + +function oidcTransactionCookieOptions() { + return { + httpOnly: true, + sameSite: "lax" as const, + path: "/", + secure: true, + maxAge: OIDC_TRANSACTION_COOKIE_SECONDS, + }; +} + +function clearOidcTransactionCookie(reply: FastifyReply): void { + reply.clearCookie(OIDC_TRANSACTION_COOKIE, { + httpOnly: true, + sameSite: "lax", + path: "/", + secure: true, + }); +} + +function oidcCallbackUrl( + request: FastifyRequest, + publicUrl: string | undefined, +): { currentUrl?: URL; state?: string } { + if (request.url.length > MAX_OIDC_CALLBACK_QUERY_LENGTH) return {}; let supplied: URL; - let target: URL; try { supplied = new URL(request.url, "http://callback.invalid"); - target = new URL(OIDC_CALLBACK_PATH, publicUrl); } catch { - return undefined; + return {}; } - if (supplied.pathname !== "/auth/oidc/callback") return undefined; + if (supplied.pathname !== "/auth/oidc/callback") return {}; const allowed = new Set(["code", "state", "error", "error_description", "error_uri", "iss"]); const copied = new URLSearchParams(); let state: string | undefined; + let valid = true; for (const [key, value] of supplied.searchParams) { - if (!allowed.has(key) || value.length > 2048 || copied.has(key)) return undefined; + if (key === "state" && state === undefined && OIDC_VALUE_PATTERN.test(value)) state = value; + if (!allowed.has(key) || value.length > 2048 || /\p{Cc}/u.test(value) || copied.has(key)) { + valid = false; + continue; + } copied.set(key, value); - if (key === "state") state = value; } - if (!state || !/^[A-Za-z0-9_-]{43}$/.test(state)) return undefined; + if (!state || !valid || copied.get("state") !== state || publicUrl === undefined) return { state }; + let target: URL; + try { + target = new URL(OIDC_CALLBACK_PATH, publicUrl); + } catch { + return { state }; + } target.search = copied.toString(); return { currentUrl: target, state }; } diff --git a/backend/src/auth/session-store.ts b/backend/src/auth/session-store.ts index 4cd6d529..af6ceae7 100644 --- a/backend/src/auth/session-store.ts +++ b/backend/src/auth/session-store.ts @@ -70,6 +70,7 @@ export interface OidcStateCreateInput { returnTo: "/"; authConfigRevision: string; issuer: string; + browserTransactionDigest: string; } export interface CreatedOidcState { @@ -192,6 +193,7 @@ const oidcStateRecordSchema = z.strictObject({ returnTo: z.literal("/"), authConfigRevision: z.string().regex(/^[a-f0-9]{64}$/), issuer: z.string().min(1).max(2048).refine((value) => !/\p{Cc}/u.test(value)), + browserTransactionDigest: z.string().regex(/^[a-f0-9]{64}$/), createdAt: timestamp, expiresAt: timestamp, }).superRefine((record, context) => { @@ -232,6 +234,7 @@ const oidcStateInputSchema = z.strictObject({ returnTo: z.literal("/"), authConfigRevision: z.string().regex(/^[a-f0-9]{64}$/), issuer: z.string().min(1).max(2048).refine((value) => !/\p{Cc}/u.test(value)), + browserTransactionDigest: z.string().regex(/^[a-f0-9]{64}$/), }); function sameFileIdentity(left: FileIdentity, right: FileIdentity): boolean { @@ -935,6 +938,7 @@ export function createFileAuthSessionStore( returnTo: validated.returnTo, authConfigRevision: validated.authConfigRevision, issuer: validated.issuer, + browserTransactionDigest: validated.browserTransactionDigest, createdAt: isoAt(nowMs), expiresAt: isoAt(expiresMs), }; diff --git a/backend/src/auth/types.ts b/backend/src/auth/types.ts index ad4f4f5d..187d1092 100644 --- a/backend/src/auth/types.ts +++ b/backend/src/auth/types.ts @@ -81,6 +81,7 @@ export interface OidcStateRecord { returnTo: "/"; authConfigRevision: string; issuer: string; + browserTransactionDigest: string; createdAt: string; expiresAt: string; } diff --git a/backend/test/auth-routes-oidc.test.ts b/backend/test/auth-routes-oidc.test.ts index 55af1294..af757929 100644 --- a/backend/test/auth-routes-oidc.test.ts +++ b/backend/test/auth-routes-oidc.test.ts @@ -11,8 +11,25 @@ const issuer = "https://issuer.example.test"; const state = "s".repeat(43); const nonce = "n".repeat(43); const verifier = "v".repeat(43); +const transactionCookieName = "__Host-thothii_oidc_tx"; const createdApps: Array> = []; +function setCookieHeaders(response: { headers: Record }): string[] { + const header = response.headers["set-cookie"]; + return header === undefined ? [] : Array.isArray(header) ? header : [header]; +} + +function transactionCookie(response: { headers: Record }): string | undefined { + return setCookieHeaders(response) + .find((header) => header.startsWith(`${transactionCookieName}=`)) + ?.split(";", 1)[0]; +} + +function expectTransactionCleared(response: { headers: Record }): void { + expect(setCookieHeaders(response).some((header) => + header.startsWith(`${transactionCookieName}=`) && header.includes("Max-Age=0"))).toBe(true); +} + function config(overrides: Partial = {}): LoadedAuthConfig { return { revision, @@ -40,6 +57,7 @@ function stateRecord(extra: Partial = {}): OidcStateRecord { return { version: 1, nonce, codeVerifier: verifier, returnTo: "/", authConfigRevision: revision, issuer, + browserTransactionDigest: "b".repeat(64), createdAt: "2030-01-01T00:00:00.000Z", expiresAt: "2030-01-01T00:10:00.000Z", ...extra, } as OidcStateRecord; @@ -56,6 +74,7 @@ function fixture(options: { let storedState: OidcStateRecord | undefined; const stateInputs: Array> = []; const creates: Array> = []; + const createTimes: Array = []; const callbacks: URL[] = []; const protocol: OidcProtocol = { authorizationUrl: async ({ state: received, nonce: receivedNonce, codeVerifier }) => { @@ -83,6 +102,8 @@ function fixture(options: { authConfigRevision: input.authConfigRevision as string, issuer: input.issuer as string, }); + (record as OidcStateRecord & { browserTransactionDigest: string }).browserTransactionDigest = + input.browserTransactionDigest as string; if (options.stateReturnTo) (record as { returnTo: string }).returnTo = options.stateReturnTo; storedState = record; return { state, record: storedState }; @@ -93,8 +114,9 @@ function fixture(options: { storedState = undefined; return consumed; }, - create: async (input: Record) => { + create: async (input: Record, now?: Date) => { creates.push(input); + createTimes.push(now); return { token: "opaque-session-token", csrfToken: "c".repeat(43), record: {} }; }, } as unknown as AuthSessionStore; @@ -111,7 +133,7 @@ function fixture(options: { }); createdApps.push(app); return { - app, creates, callbacks, stateInputs, + app, creates, createTimes, callbacks, stateInputs, setConfig(next: LoadedAuthConfig) { loaded = next; }, setProtocolAvailable(available: boolean) { protocolAvailable = available; }, stateWasConsumed: () => storedState === undefined, @@ -122,23 +144,50 @@ afterEach(async () => { await Promise.all(createdApps.splice(0).map((app) => app.close())); }); +async function beginOidcLogin(subject: ReturnType) { + const response = await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); + return { response, cookie: transactionCookie(response) }; +} + +async function finishOidcLogin( + subject: ReturnType, + cookie: string | undefined, + query = `code=good&state=${state}`, +) { + return await subject.app.inject({ + method: "GET", + url: `/auth/oidc/callback?${query}`, + ...(cookie ? { headers: { cookie } } : {}), + }); +} + test("creates digest-only bound state, maps exact groups, creates a cookie session, and redirects safely", async () => { const subject = fixture(); - const start = await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); + const { response: start, cookie } = await beginOidcLogin(subject); expect(start.statusCode).toBe(302); expect(new URL(start.headers.location ?? "").searchParams.get("state")).toBe(state); - expect(subject.stateInputs[0]).toMatchObject({ returnTo: "/", authConfigRevision: revision, issuer }); + expect(subject.stateInputs[0]).toMatchObject({ + returnTo: "/", authConfigRevision: revision, issuer, + browserTransactionDigest: expect.stringMatching(/^[a-f0-9]{64}$/), + }); + expect(cookie).toMatch(new RegExp(`^${transactionCookieName}=[A-Za-z0-9_-]{43}$`)); + expect(setCookieHeaders(start).join("\n")).toContain("HttpOnly"); + expect(setCookieHeaders(start).join("\n")).toContain("SameSite=Lax"); + expect(setCookieHeaders(start).join("\n")).toContain("Secure"); + expect(setCookieHeaders(start).join("\n")).toContain("Path=/"); + expect(JSON.stringify(subject.stateInputs)).not.toContain(cookie?.split("=", 2)[1] ?? "missing-cookie"); const callback = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}`, - headers: { host: "attacker.example.test" }, + headers: { host: "attacker.example.test", cookie: cookie ?? "" }, }); expect(callback.statusCode).toBe(302); expect(callback.headers.location).toBe("/"); - expect(callback.headers["set-cookie"]).toContain("HttpOnly"); - expect(callback.headers["set-cookie"]).toContain("SameSite=Lax"); - expect(callback.headers["set-cookie"]).toContain("Secure"); + expect(setCookieHeaders(callback).join("\n")).toContain("HttpOnly"); + expect(setCookieHeaders(callback).join("\n")).toContain("SameSite=Lax"); + expect(setCookieHeaders(callback).join("\n")).toContain("Secure"); + expectTransactionCleared(callback); expect(subject.callbacks[0]?.href).toBe(`https://thothii.example.test/api/auth/oidc/callback?code=good&state=${state}`); expect(subject.creates).toHaveLength(1); expect(subject.creates[0]).toMatchObject({ @@ -156,27 +205,28 @@ test("creates digest-only bound state, maps exact groups, creates a cookie sessi test("consumes state on callback failure and refuses replay", async () => { const subject = fixture({ callbackFailure: true }); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); - const failed = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const { cookie } = await beginOidcLogin(subject); + const failed = await finishOidcLogin(subject, cookie); expect(failed.statusCode).toBe(401); - const replay = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + expectTransactionCleared(failed); + const replay = await finishOidcLogin(subject, cookie); expect(replay.statusCode).toBe(401); expect(subject.callbacks).toHaveLength(1); }); test("consumes state when the protocol becomes unavailable before callback", async () => { const subject = fixture(); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); + const { cookie } = await beginOidcLogin(subject); subject.setProtocolAvailable(false); - const failed = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const failed = await finishOidcLogin(subject, cookie); expect(failed.statusCode).toBe(401); expect(subject.stateWasConsumed()).toBe(true); }); test("rejects a consumed state with a non-root return target", async () => { const subject = fixture({ stateReturnTo: "https://attacker.example.test" }); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); - const callback = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const { cookie } = await beginOidcLogin(subject); + const callback = await finishOidcLogin(subject, cookie); expect(callback.statusCode).toBe(401); expect(subject.callbacks).toEqual([]); expect(subject.creates).toEqual([]); @@ -184,22 +234,22 @@ test("rejects a consumed state with a non-root return target", async () => { test("rejects an OIDC state when its configuration revision changes before callback", async () => { const subject = fixture(); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); + const { cookie } = await beginOidcLogin(subject); subject.setConfig({ ...config(), revision: "b".repeat(64) }); - const callback = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const callback = await finishOidcLogin(subject, cookie); expect(callback.statusCode).toBe(401); expect(subject.callbacks).toEqual([]); }); test("rejects an OIDC state when its issuer changes before callback", async () => { const subject = fixture(); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); + const { cookie } = await beginOidcLogin(subject); const previous = config(); subject.setConfig({ ...previous, value: { ...previous.value, oidc: { ...previous.value.oidc, issuer: "https://other.example.test" } }, } as LoadedAuthConfig); - const callback = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const callback = await finishOidcLogin(subject, cookie); expect(callback.statusCode).toBe(401); expect(subject.callbacks).toEqual([]); }); @@ -208,8 +258,67 @@ test("creates an authenticated but forbidden principal for extra unmapped groups const subject = fixture({ identity: { issuer, subject: "user-123", groups: ["Unmapped"], tokenExpiresAt: new Date(Date.now() + 60_000), } }); - await subject.app.inject({ method: "GET", url: "/auth/oidc/login" }); - const callback = await subject.app.inject({ method: "GET", url: `/auth/oidc/callback?code=good&state=${state}` }); + const { cookie } = await beginOidcLogin(subject); + const callback = await finishOidcLogin(subject, cookie); expect(callback.statusCode).toBe(302); expect(subject.creates[0]).toMatchObject({ principal: { roles: [], permissions: [], isAdmin: false } }); }); + +test("rejects a callback from a different browser and clears the transaction cookie", async () => { + const subject = fixture(); + const { cookie } = await beginOidcLogin(subject); + expect(cookie).toBeDefined(); + + const failed = await finishOidcLogin(subject, `${transactionCookieName}=${"x".repeat(43)}`); + expect(failed.statusCode).toBe(401); + expectTransactionCleared(failed); + expect(subject.stateWasConsumed()).toBe(true); + expect(subject.creates).toEqual([]); + + const replay = await finishOidcLogin(subject, cookie); + expect(replay.statusCode).toBe(401); + expect(subject.creates).toEqual([]); +}); + +test.each([ + ["unknown", `code=good&state=${state}&unexpected=value`], + ["duplicate", `code=good&code=other&state=${state}`], + ["malformed", `code=good&state=${state}&error_description=%00bad`], +])("burns canonical state before rejecting %s callback parameters", async (_label, query) => { + const subject = fixture(); + const { cookie } = await beginOidcLogin(subject); + const failed = await finishOidcLogin(subject, cookie, query); + expect(failed.statusCode).toBe(401); + expectTransactionCleared(failed); + expect(subject.stateWasConsumed()).toBe(true); + expect(subject.callbacks).toEqual([]); + + const replay = await finishOidcLogin(subject, cookie); + expect(replay.statusCode).toBe(401); + expect(subject.callbacks).toEqual([]); +}); + +test("rejects an empty direct groups claim without creating a session cookie", async () => { + const subject = fixture({ identity: { + issuer, subject: "user-123", groups: [], tokenExpiresAt: new Date(Date.now() + 60_000), + } }); + const { cookie } = await beginOidcLogin(subject); + const callback = await finishOidcLogin(subject, cookie); + expect(callback.statusCode).toBe(401); + expectTransactionCleared(callback); + expect(subject.creates).toEqual([]); + expect(setCookieHeaders(callback).join("\n")).not.toContain("opaque-session-token"); +}); + +test("uses one captured instant for token TTL derivation and session creation", async () => { + const tokenExpiresAt = new Date(Date.now() + 120_000); + const subject = fixture({ identity: { + issuer, subject: "user-123", groups: ["Users"], tokenExpiresAt, + } }); + const { cookie } = await beginOidcLogin(subject); + const callback = await finishOidcLogin(subject, cookie); + expect(callback.statusCode).toBe(302); + expect(subject.createTimes[0]).toBeInstanceOf(Date); + expect((subject.createTimes[0] as Date).getTime() + (subject.creates[0]?.absoluteTtlMs as number)) + .toBe(tokenExpiresAt.getTime()); +}); diff --git a/backend/test/auth-session-store.test.ts b/backend/test/auth-session-store.test.ts index e5687cbe..8c800134 100644 --- a/backend/test/auth-session-store.test.ts +++ b/backend/test/auth-session-store.test.ts @@ -70,7 +70,14 @@ const revision = "a".repeat(64); const validLocalUser = { enabled: true, authRevision: 7, roles: ["admin"] as const }; function oidcInput(nonce: string, codeVerifier: string) { - return { nonce, codeVerifier, returnTo: "/" as const, authConfigRevision: revision, issuer: "https://issuer.example.test" }; + return { + nonce, + codeVerifier, + returnTo: "/" as const, + authConfigRevision: revision, + issuer: "https://issuer.example.test", + browserTransactionDigest: "b".repeat(64), + }; } afterEach(() => { @@ -285,6 +292,7 @@ describe("file-backed auth session store", () => { .resolves.toMatchObject({ nonce: "n".repeat(43), codeVerifier: "v".repeat(43), returnTo: "/", authConfigRevision: revision, issuer: "https://issuer.example.test", + browserTransactionDigest: "b".repeat(64), }); await expect(store.consumeOidcState(created.state)).resolves.toBeUndefined(); diff --git a/backend/test/oidc-client.test.ts b/backend/test/oidc-client.test.ts index d7c87333..a800335c 100644 --- a/backend/test/oidc-client.test.ts +++ b/backend/test/oidc-client.test.ts @@ -28,6 +28,8 @@ function protocol(options: { discoveryIssuer?: string; invalidSignature?: boolean; seen?: URL[]; + jwksResponse?: (init?: RequestInit) => Response | Promise; + jwksTimeoutMs?: number; } = {}) { const now = Math.floor(Date.now() / 1000); const claims = { @@ -41,7 +43,7 @@ function protocol(options: { groups: ["TOT Users", "Unmapped group"], ...options.claims, }; - const fetch = async (input: RequestInfo | URL) => { + const fetch = async (input: RequestInfo | URL, init?: RequestInit) => { const url = new URL(input instanceof Request ? input.url : typeof input === "string" ? input : input.toString()); options.seen?.push(url); if (url.pathname.includes(".well-known/")) { @@ -56,7 +58,7 @@ function protocol(options: { id_token_signing_alg_values_supported: ["RS256"], }); } - if (url.pathname === "/jwks") return Response.json({ keys: [jwk] }); + if (url.pathname === "/jwks") return options.jwksResponse ? await options.jwksResponse(init) : Response.json({ keys: [jwk] }); if (url.pathname === "/token") { return Response.json({ token_type: "Bearer", @@ -67,7 +69,7 @@ function protocol(options: { } return new Response(null, { status: 404 }); }; - return createOidcProtocol({ + const protocolOptions = { issuer, clientId, clientSecret: "client-secret-must-not-be-persisted", @@ -75,7 +77,9 @@ function protocol(options: { scopes: ["openid", "profile"], groupsClaim: "groups", fetch, - }); + ...(options.jwksTimeoutMs === undefined ? {} : { jwksTimeoutMs: options.jwksTimeoutMs }), + } as Parameters[0]; + return createOidcProtocol(protocolOptions); } async function callback(subject = protocol()) { @@ -128,9 +132,66 @@ test("rejects invalid ID-token signatures", async () => { await expect(callback(protocol({ invalidSignature: true }))).rejects.toThrow(OidcProtocolError); }); +test("aborts a hanging JWKS request at the configured timeout", async () => { + let aborted = false; + const subject = protocol({ + jwksTimeoutMs: 20, + jwksResponse: (init) => new Promise((_resolve, reject) => { + const fallback = setTimeout(() => reject(new Error("JWKS fixture was not aborted")), 200); + init?.signal?.addEventListener("abort", () => { + aborted = true; + clearTimeout(fallback); + reject(new DOMException("aborted", "AbortError")); + }, { once: true }); + }), + }); + await expect(callback(subject)).rejects.toThrow(OidcProtocolError); + expect(aborted).toBe(true); +}); + +test("rejects an oversized JWKS Content-Length before reading the body", async () => { + let pulls = 0; + const body = new ReadableStream({ + type: "bytes", + pull(controller) { + pulls += 1; + controller.enqueue(new TextEncoder().encode("{}")); + controller.close(); + }, + }); + const subject = protocol({ + jwksResponse: () => new Response(body, { headers: { "content-length": String(1024 * 1024 + 1) } }), + }); + await expect(callback(subject)).rejects.toThrow(OidcProtocolError); + expect(pulls).toBe(0); +}); + +test("stops streaming a JWKS response as soon as the byte limit is exceeded", async () => { + let pulls = 0; + let cancelled = false; + const body = new ReadableStream({ + type: "bytes", + pull(controller) { + pulls += 1; + if (pulls === 1) controller.enqueue(new Uint8Array(700_000)); + else if (pulls === 2) controller.enqueue(new Uint8Array(400_000)); + else { + controller.enqueue(new Uint8Array([1])); + controller.close(); + } + }, + cancel() { cancelled = true; }, + }); + const subject = protocol({ jwksResponse: () => new Response(body) }); + await expect(callback(subject)).rejects.toThrow(OidcProtocolError); + expect(pulls).toBe(2); + expect(cancelled).toBe(true); +}); + test.each([ ["absent", { groups: undefined }], ["non-array", { groups: "TOT Users" }], + ["empty array", { groups: [] }], ["empty", { groups: [""] }], ["duplicate", { groups: ["TOT Users", "TOT Users"] }], ["control", { groups: ["TOT\u0000Users"] }], diff --git a/frontend/src/auth/LoginPage.test.tsx b/frontend/src/auth/LoginPage.test.tsx index 7ff73e9f..faddf437 100644 --- a/frontend/src/auth/LoginPage.test.tsx +++ b/frontend/src/auth/LoginPage.test.tsx @@ -84,6 +84,43 @@ describe("LoginPage", () => { expect(screen.getByRole("button", { name: /single sign-on/i })).toBeEnabled(); }); + test("routes every rendered SSO affordance through the held logout barrier", async () => { + let releaseLogout!: () => void; + let logoutStarted!: () => void; + const logoutGate = new Promise((resolve) => { releaseLogout = resolve; }); + const logoutRequest = new Promise((resolve) => { logoutStarted = resolve; }); + server.use(http.post("/api/auth/logout", async () => { + logoutStarted(); + await logoutGate; + return new HttpResponse(null, { status: 204 }); + })); + setAuthState({ ...authenticated, subject: "user-a" }); + const logoutPromise = authApi.logout(); + await logoutRequest; + + const navigate = vi.fn(); + const beginOidcLogin = authApi.beginOidcLogin; + const begin = vi.spyOn(authApi, "beginOidcLogin") + .mockImplementation(() => beginOidcLogin(navigate)); + try { + render(); + const buttons = screen.getAllByRole("button", { name: /single sign-on/i }); + expect(screen.queryAllByRole("link", { name: /single sign-on/i })).toHaveLength(0); + expect(buttons).toHaveLength(1); + + await userEvent.click(buttons[0]); + await Promise.resolve(); + expect(begin).toHaveBeenCalledOnce(); + expect(navigate).not.toHaveBeenCalled(); + + releaseLogout(); + await Promise.all([logoutPromise, vi.waitFor(() => expect(navigate).toHaveBeenCalledWith("/api/auth/oidc/login"))]); + } finally { + releaseLogout(); + begin.mockRestore(); + } + }); + test("does not dispatch local login until an in-flight logout response settles", async () => { let releaseLogout!: () => void; let logoutStarted!: () => void; diff --git a/frontend/src/auth/LoginPage.tsx b/frontend/src/auth/LoginPage.tsx index a1be1c6e..4aafd069 100644 --- a/frontend/src/auth/LoginPage.tsx +++ b/frontend/src/auth/LoginPage.tsx @@ -150,16 +150,6 @@ export function LoginPage({ config, onAuthenticated, onRetry }: LoginPageProps) )} - {config.oidcLogin && ( - - Continue with single sign-on - - )} - {!localLogin && !config.oidcLogin && (

No browser sign-in method is enabled for this installation.