From b675c49494f5012bbddbf87ba0c8c09129ebb8a8 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Mon, 28 Sep 2026 14:32:36 +0100 Subject: [PATCH 1/4] feat: wire SSO token refresh into sessionAuth (task 2.2) sessionAuth now checks a loaded session's tokenExpiresAt against a new SSO_TOKEN_REFRESH_LEEWAY_SECONDS config (default 30s) and, when due and a refreshToken is present, refreshes via refreshSsoSession() + updateSessionTokens() in place before populating request.session - same session id, no new cookie. Any failure (rejected grant, unreachable Keycloak, discovery failure, session gone mid-refresh) falls through to the existing 401 session_expired. Password-login sessions are untouched - no refreshToken means no refresh attempt. updateSessionTokens() now takes an explicit tokenExpiresAt instead of deriving it from the Redis-key ttlSeconds, so a refresh response with a missing/invalid expires_in can't make tokenExpiresAt inherit a much longer session-TTL fallback (same bug class fixed earlier in establish-session.ts, applied here before it got a real caller). Signed-off-by: Gabriel Costa --- .env.example | 1 + .env.prod.example | 1 + server/src/config.ts | 10 + server/src/lib/oidc-discovery.ts | 38 +- server/src/lib/session-store.ts | 12 +- server/src/lib/sso-token-refresh.ts | 45 +- server/src/plugins/session.ts | 132 +++++- server/test/helpers/build-app.ts | 12 + server/test/lib/oidc-discovery.test.ts | 65 ++- server/test/lib/session-store.test.ts | 12 +- server/test/lib/sso-token-refresh.test.ts | 5 +- server/test/plugins/session.test.ts | 552 ++++++++++++++++++++++ 12 files changed, 841 insertions(+), 44 deletions(-) create mode 100644 server/test/plugins/session.test.ts diff --git a/.env.example b/.env.example index 4bc76ab2..52f04181 100644 --- a/.env.example +++ b/.env.example @@ -85,6 +85,7 @@ SSO_ENABLED=false # SSO_KEYCLOAK_CLIENT_SECRET= # SSO_KEYCLOAK_SCOPES=openid profile email # SSO_LOGIN_STATE_TTL_SECONDS=300 +# SSO_TOKEN_REFRESH_LEEWAY_SECONDS=30 # Build-time UI feature flags. These are read by Vite when the frontend starts. VITE_ENABLE_VIRTUAL_SERVER_TOOL_TRY_IT=false diff --git a/.env.prod.example b/.env.prod.example index 694f3af8..da07a7b6 100644 --- a/.env.prod.example +++ b/.env.prod.example @@ -69,3 +69,4 @@ SSO_KEYCLOAK_CLIENT_ID= SSO_KEYCLOAK_CLIENT_SECRET= SSO_KEYCLOAK_SCOPES=openid profile email SSO_LOGIN_STATE_TTL_SECONDS=300 +SSO_TOKEN_REFRESH_LEEWAY_SECONDS=30 diff --git a/server/src/config.ts b/server/src/config.ts index 8c1fe2d6..7b13f7b0 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -88,6 +88,9 @@ export const config = { // TTL for the one-time PKCE verifier/state/nonce minted by the login route // and consumed by the callback route — same pattern as the nonce above. ssoLoginStateTtlSeconds: Number(optional("SSO_LOGIN_STATE_TTL_SECONDS", "300")), + // Refresh an SSO session's Keycloak tokens this many seconds before + // tokenExpiresAt, not exactly at it. + ssoTokenRefreshLeewaySeconds: Number(optional("SSO_TOKEN_REFRESH_LEEWAY_SECONDS", "30")), // memory:// (default) = in-process store, no Redis needed — dev only. // See lib/memory-redis.ts. Use a real redis:// URL beyond a single @@ -181,6 +184,13 @@ if (!Number.isSafeInteger(config.ssoLoginStateTtlSeconds) || config.ssoLoginStat throw new Error("SSO_LOGIN_STATE_TTL_SECONDS must be a positive integer"); } +if ( + !Number.isSafeInteger(config.ssoTokenRefreshLeewaySeconds) || + config.ssoTokenRefreshLeewaySeconds < 0 +) { + throw new Error("SSO_TOKEN_REFRESH_LEEWAY_SECONDS must be a non-negative integer"); +} + // Fail fast at boot, not at the first /auth/sso/login request. if (config.ssoEnabled) { const missingSsoVars = [ diff --git a/server/src/lib/oidc-discovery.ts b/server/src/lib/oidc-discovery.ts index c4570b1c..96fa80ea 100644 --- a/server/src/lib/oidc-discovery.ts +++ b/server/src/lib/oidc-discovery.ts @@ -18,11 +18,13 @@ const DISCOVERY_FETCH_TIMEOUT_MS = 5000; export interface OidcDiscoveryDocument { issuer: string; - // Browser-redirect targets -- rewritten to ssoKeycloakPublicBaseUrl when configured. + // Browser-redirect target -- rewritten to ssoKeycloakPublicBaseUrl when configured. authorizationEndpoint: string; - // Not every OIDC provider advertises this. + // Server-to-server only -- rewritten to ssoKeycloakBaseUrl. Not every + // OIDC provider advertises this. endSessionEndpoint?: string; - // Called by the BFF, server-to-server only -- stay on the internal host. + // Server-to-server only -- rewritten to ssoKeycloakBaseUrl, never trusted + // as-discovered (Keycloak reports one hostname for every endpoint). tokenEndpoint: string; jwksUri: string; } @@ -103,11 +105,16 @@ async function fetchDiscoveryDocument(): Promise { return { issuer, - authorizationEndpoint: rewritePublicBaseUrl(authorizationEndpoint, "authorization_endpoint"), - tokenEndpoint, - jwksUri, + authorizationEndpoint: rewriteBaseUrl( + authorizationEndpoint, + config.ssoKeycloakPublicBaseUrl, + "authorization_endpoint", + ), + tokenEndpoint: rewriteBaseUrl(tokenEndpoint, config.ssoKeycloakBaseUrl, "token_endpoint"), + jwksUri: rewriteBaseUrl(jwksUri, config.ssoKeycloakBaseUrl, "jwks_uri"), endSessionEndpoint: - endSessionEndpoint && rewritePublicBaseUrl(endSessionEndpoint, "end_session_endpoint"), + endSessionEndpoint && + rewriteBaseUrl(endSessionEndpoint, config.ssoKeycloakBaseUrl, "end_session_endpoint"), }; } @@ -124,15 +131,18 @@ function optionalStringField(body: Record, field: string): stri return typeof value === "string" && value ? value : undefined; } -// Swaps only scheme+host+port to ssoKeycloakPublicBaseUrl, keeping the -// discovered path -- the browser can't reach the internal host. -function rewritePublicBaseUrl(endpoint: string, fieldName: string): string { - if (!config.ssoKeycloakPublicBaseUrl) return endpoint; +// Swaps only scheme+host+port to targetBase, keeping the discovered path. +function rewriteBaseUrl( + endpoint: string, + targetBase: string | undefined, + fieldName: string, +): string { + if (!targetBase) return endpoint; try { - const publicBase = new URL(config.ssoKeycloakPublicBaseUrl); + const target = new URL(targetBase); const rewritten = new URL(endpoint); - rewritten.protocol = publicBase.protocol; - rewritten.host = publicBase.host; // host includes port + rewritten.protocol = target.protocol; + rewritten.host = target.host; // host includes port return rewritten.toString(); } catch (err) { throw new OidcDiscoveryError( diff --git a/server/src/lib/session-store.ts b/server/src/lib/session-store.ts index fc10823d..39a33aa5 100644 --- a/server/src/lib/session-store.ts +++ b/server/src/lib/session-store.ts @@ -24,6 +24,9 @@ export interface RedisLike { // second concurrent caller's get() can still observe the value. getdel(key: string): Promise; setex(key: string, ttlSeconds: number, value: string): Promise; + // Atomic lock acquire (ioredis's own SET key value PX ms NX signature) -- + // "OK" only if the key was absent; PX auto-releases an abandoned lock. + set(key: string, value: string, mode: "PX", ttlMs: number, flag: "NX"): Promise<"OK" | null>; del(key: string): Promise; publish(channel: string, message: string): Promise; } @@ -57,6 +60,11 @@ export function sessionRevokedChannel(sessionId: string): string { return `${config.redisKeyPrefix}:session:revoked:${sessionId}`; } +/** Cross-instance lock so concurrent requests don't race a token refresh (see plugins/session.ts). */ +export function sessionRefreshLockKey(sessionId: string): string { + return `${config.redisKeyPrefix}:session-refresh-lock:${sessionId}`; +} + // TTL defaults to config.sessionTtlSeconds, but callers should pass the // upstream token's real expires_in (see routes/auth/login.ts) — the BFF // session and cookie must not outlive the bearer token they wrap. A session @@ -95,7 +103,7 @@ export async function getSession( export async function updateSessionTokens( redis: RedisLike, sessionId: string, - tokens: { bearerToken: string; refreshToken?: string; idToken?: string }, + tokens: { bearerToken: string; refreshToken?: string; idToken?: string; tokenExpiresAt: number }, ttlSeconds: number, ): Promise { const existing = await getSession(redis, sessionId); @@ -106,7 +114,7 @@ export async function updateSessionTokens( bearerToken: tokens.bearerToken, refreshToken: tokens.refreshToken ?? existing.refreshToken, idToken: tokens.idToken ?? existing.idToken, - tokenExpiresAt: Math.floor(Date.now() / 1000) + ttlSeconds, + tokenExpiresAt: tokens.tokenExpiresAt, }; await redis.setex(sessionRedisKey(sessionId), ttlSeconds, JSON.stringify(updated)); return true; diff --git a/server/src/lib/sso-token-refresh.ts b/server/src/lib/sso-token-refresh.ts index 8d6ed84b..5092ddc9 100644 --- a/server/src/lib/sso-token-refresh.ts +++ b/server/src/lib/sso-token-refresh.ts @@ -18,9 +18,13 @@ export interface SsoTokenRefreshResult { } export class SsoTokenRefreshError extends Error { - constructor(message: string, options?: { cause?: unknown }) { + // "rejected" = Keycloak said no (4xx, dead token). "unreachable" = + // network/timeout/5xx/malformed -- the refresh token may still be fine. + readonly code: "rejected" | "unreachable"; + constructor(message: string, code: "rejected" | "unreachable", options?: { cause?: unknown }) { super(message, options); this.name = "SsoTokenRefreshError"; + this.code = code; } } @@ -29,7 +33,7 @@ export async function refreshSsoSession(params: { refreshToken: string; }): Promise { if (!config.ssoEnabled || !config.ssoKeycloakClientId || !config.ssoKeycloakClientSecret) { - throw new SsoTokenRefreshError("SSO is not configured"); + throw new SsoTokenRefreshError("SSO is not configured", "unreachable"); } const body = new URLSearchParams({ @@ -51,10 +55,9 @@ export async function refreshSsoSession(params: { redirect: "error", }); } catch (err) { - // Keycloak unreachable (network/timeout) -- not necessarily that the - // refresh token itself is bad. Distinguishable in logs/error handling - // from the non-2xx branch below, which carries Keycloak's own error code. - throw new SsoTokenRefreshError("Keycloak token request failed", { cause: err }); + // Network/timeout -- not necessarily that the refresh token is bad, + // distinct from the non-2xx branch below (Keycloak's own error code). + throw new SsoTokenRefreshError("Keycloak token request failed", "unreachable", { cause: err }); } let json: unknown; @@ -62,32 +65,42 @@ export async function refreshSsoSession(params: { json = await response.json(); } catch (err) { if (!response.ok) { - throw new SsoTokenRefreshError(`Keycloak token endpoint returned ${response.status}`, { - cause: err, - }); + throw new SsoTokenRefreshError( + `Keycloak token endpoint returned ${response.status}`, + "unreachable", + { cause: err }, + ); } - throw new SsoTokenRefreshError("Keycloak token endpoint returned a non-JSON body", { - cause: err, - }); + throw new SsoTokenRefreshError( + "Keycloak token endpoint returned a non-JSON body", + "unreachable", + { cause: err }, + ); } if (json === null || typeof json !== "object") { - throw new SsoTokenRefreshError("Keycloak token endpoint returned a non-object JSON body"); + throw new SsoTokenRefreshError( + "Keycloak token endpoint returned a non-object JSON body", + "unreachable", + ); } const responseBody = json as Record; if (!response.ok) { // Keycloak's own RFC 6749 error code -- invalid_grant means the refresh - // token was revoked/expired (the caller should give up refreshing and - // fall back to a full login), distinct from "Keycloak is unreachable" above. + // token is dead, distinct from "unreachable" above. const errorCode = typeof responseBody.error === "string" ? responseBody.error : "unknown_error"; + // 5xx is Keycloak's own server error (transient); 4xx is Keycloak + // explicitly rejecting this specific grant/token (not transient). + const code = response.status >= 500 ? "unreachable" : "rejected"; throw new SsoTokenRefreshError( `Keycloak token endpoint returned ${response.status} (${errorCode})`, + code, ); } if (typeof responseBody.access_token !== "string" || !responseBody.access_token) { - throw new SsoTokenRefreshError("Keycloak token response missing access_token"); + throw new SsoTokenRefreshError("Keycloak token response missing access_token", "unreachable"); } return { diff --git a/server/src/plugins/session.ts b/server/src/plugins/session.ts index ed466d16..6b4a9a91 100644 --- a/server/src/plugins/session.ts +++ b/server/src/plugins/session.ts @@ -10,7 +10,126 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import fp from "fastify-plugin"; -import { getSession, SESSION_COOKIE_NAME } from "../lib/session-store.js"; +import { config } from "../config.js"; +import { getDiscoveryDocument, OidcDiscoveryError } from "../lib/oidc-discovery.js"; +import { + getSession, + sessionRefreshLockKey, + setSessionCookie, + SESSION_COOKIE_NAME, + updateSessionTokens, + type SessionRecord, +} from "../lib/session-store.js"; +import { refreshSsoSession, SsoTokenRefreshError } from "../lib/sso-token-refresh.js"; + +// Missing expires_in on a refresh isn't "already expired" -- that would +// retry every request (a storm). Back off a short window instead. +const SSO_TOKEN_REFRESH_FALLBACK_SECONDS = 60; + +// Covers the worst-case refresh round trip (discovery fetch + token refresh +// + DB write) so an abandoned lock (crashed holder) self-clears promptly. +const REFRESH_LOCK_TTL_MS = 12_000; +const REFRESH_LOCK_POLL_MS = 100; +const REFRESH_LOCK_MAX_WAIT_MS = 3_000; + +function needsRefresh(record: SessionRecord): boolean { + if (!record.refreshToken || record.tokenExpiresAt === undefined) return false; + const now = Math.floor(Date.now() / 1000); + return record.tokenExpiresAt - config.ssoTokenRefreshLeewaySeconds <= now; +} + +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +// null only for a definite failure (rejected token, session gone). A +// transient failure returns the ORIGINAL record so this request still works. +async function refreshRecord( + request: FastifyRequest, + reply: FastifyReply, + sessionId: string, + record: SessionRecord, +): Promise { + try { + const { tokenEndpoint } = await getDiscoveryDocument(); + const tokens = await refreshSsoSession({ + tokenEndpoint, + refreshToken: record.refreshToken!, + }); + + const validExpiresIn = tokens.expiresIn && tokens.expiresIn > 0 ? tokens.expiresIn : undefined; + const ttlSeconds = validExpiresIn ?? config.sessionTtlSeconds; + const tokenExpiresAt = + Math.floor(Date.now() / 1000) + (validExpiresIn ?? SSO_TOKEN_REFRESH_FALLBACK_SECONDS); + + const wrote = await updateSessionTokens( + request.server.redis, + sessionId, + { + bearerToken: tokens.accessToken, + refreshToken: tokens.refreshToken, + idToken: tokens.idToken, + tokenExpiresAt, + }, + ttlSeconds, + ); + if (!wrote) return null; + + // Without this, the cookie keeps the *original* login's maxAge and the + // browser drops it once that elapses, even with Redis kept fresh. + setSessionCookie(reply, sessionId, ttlSeconds); + + return { + ...record, + bearerToken: tokens.accessToken, + refreshToken: tokens.refreshToken ?? record.refreshToken, + idToken: tokens.idToken ?? record.idToken, + tokenExpiresAt, + }; + } catch (err) { + const isTransient = + (err instanceof SsoTokenRefreshError && err.code === "unreachable") || + err instanceof OidcDiscoveryError; + if (isTransient) { + request.log.warn({ err }, "SSO token refresh unreachable -- using pre-refresh token"); + return record; + } + request.log.warn({ err }, "SSO token refresh failed"); + return null; + } +} + +// Redis-locked so concurrent requests near expiry don't each race their own +// refresh_token grant -- rotating tokens would reject all but the first. +async function refreshRecordWithLock( + request: FastifyRequest, + reply: FastifyReply, + sessionId: string, + record: SessionRecord, +): Promise { + const lockKey = sessionRefreshLockKey(sessionId); + const acquired = await request.server.redis.set(lockKey, "1", "PX", REFRESH_LOCK_TTL_MS, "NX"); + + if (!acquired) { + const deadline = Date.now() + REFRESH_LOCK_MAX_WAIT_MS; + while (Date.now() < deadline) { + await delay(REFRESH_LOCK_POLL_MS); + const current = await getSession(request.server.redis, sessionId); + if (!current) return null; + if (!needsRefresh(current)) return current; + } + // Gave up waiting -- use the pre-refresh record rather than 401 over + // lock contention; the next request gets another chance. + request.log.warn({ sessionId }, "SSO token refresh lock wait timed out"); + return record; + } + + try { + return await refreshRecord(request, reply, sessionId, record); + } finally { + await request.server.redis.del(lockKey); + } +} async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promise { const sessionId = request.cookies[SESSION_COOKIE_NAME]; @@ -19,13 +138,20 @@ async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promis return; } - const record = await getSession(request.server.redis, sessionId); - + let record = await getSession(request.server.redis, sessionId); if (!record) { reply.code(401).send({ error: "session_expired" }); return; } + if (needsRefresh(record)) { + record = await refreshRecordWithLock(request, reply, sessionId, record); + if (!record) { + reply.code(401).send({ error: "session_expired" }); + return; + } + } + request.session = { sessionId, bearerToken: record.bearerToken, user: record.user }; } diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index 02f9d803..82e0dc13 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -45,6 +45,18 @@ export class FakeRedis { return "OK"; } + async set( + key: string, + value: string, + _mode: "PX", + _ttlMs: number, + flag: "NX", + ): Promise<"OK" | null> { + if (flag === "NX" && this.store.has(key)) return null; + this.store.set(key, value); + return "OK"; + } + async del(key: string): Promise { return this.store.delete(key) ? 1 : 0; } diff --git a/server/test/lib/oidc-discovery.test.ts b/server/test/lib/oidc-discovery.test.ts index dbf9b009..b4e22d85 100644 --- a/server/test/lib/oidc-discovery.test.ts +++ b/server/test/lib/oidc-discovery.test.ts @@ -127,13 +127,67 @@ describe("getDiscoveryDocument", () => { expect(doc.authorizationEndpoint).toBe( "https://keycloak.example.com:9443/realms/mcp-gateway/protocol/openid-connect/auth", ); + // Server-to-server endpoints always stay on the internal host, never the public one. + expect(doc.tokenEndpoint).toBe( + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token", + ); expect(doc.endSessionEndpoint).toBe( - "https://keycloak.example.com:9443/realms/mcp-gateway/protocol/openid-connect/logout", + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/logout", + ); + }); + + it("rewrites token/jwks/end_session endpoints to the internal host even when Keycloak reports everything under one public hostname", async () => { + // A real single-hostname Keycloak (just KC_HOSTNAME set, no separate + // internal-facing config) reports every endpoint under that one host -- + // this is the shape a real discovery response actually has. + process.env.SSO_KEYCLOAK_PUBLIC_BASE_URL = "http://localhost:8180"; + mockDiscoveryFetch( + discoveryBody({ + issuer: "http://localhost:8180/realms/mcp-gateway", + authorization_endpoint: + "http://localhost:8180/realms/mcp-gateway/protocol/openid-connect/auth", + token_endpoint: "http://localhost:8180/realms/mcp-gateway/protocol/openid-connect/token", + jwks_uri: "http://localhost:8180/realms/mcp-gateway/protocol/openid-connect/certs", + end_session_endpoint: + "http://localhost:8180/realms/mcp-gateway/protocol/openid-connect/logout", + }), + ); + const { getDiscoveryDocument } = await freshImport(); + + const doc = await getDiscoveryDocument(); + + expect(doc.authorizationEndpoint).toBe( + "http://localhost:8180/realms/mcp-gateway/protocol/openid-connect/auth", ); - // Server-to-server endpoints stay on the internal host. expect(doc.tokenEndpoint).toBe( "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token", ); + expect(doc.jwksUri).toBe( + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + ); + expect(doc.endSessionEndpoint).toBe( + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/logout", + ); + }); + + it("rewrites token/jwks endpoints to the internal host even without a public base URL configured", async () => { + mockDiscoveryFetch( + discoveryBody({ + token_endpoint: + "http://some-other-host:9999/realms/mcp-gateway/protocol/openid-connect/token", + jwks_uri: "http://some-other-host:9999/realms/mcp-gateway/protocol/openid-connect/certs", + }), + ); + const { getDiscoveryDocument } = await freshImport(); + + const doc = await getDiscoveryDocument(); + + expect(doc.tokenEndpoint).toBe( + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token", + ); + expect(doc.jwksUri).toBe( + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + ); }); it("throws OidcDiscoveryError, not a raw TypeError, for a malformed authorization_endpoint when a public base URL is set", async () => { @@ -144,6 +198,13 @@ describe("getDiscoveryDocument", () => { await expect(getDiscoveryDocument()).rejects.toBeInstanceOf(OidcDiscoveryError); }); + it("throws OidcDiscoveryError, not a raw TypeError, for a malformed token_endpoint", async () => { + mockDiscoveryFetch(discoveryBody({ token_endpoint: "not a url" })); + const { getDiscoveryDocument, OidcDiscoveryError } = await freshImport(); + + await expect(getDiscoveryDocument()).rejects.toBeInstanceOf(OidcDiscoveryError); + }); + it("de-dupes concurrent callers into a single in-flight fetch", async () => { const fetchMock = mockDiscoveryFetch(discoveryBody()); const { getDiscoveryDocument } = await freshImport(); diff --git a/server/test/lib/session-store.test.ts b/server/test/lib/session-store.test.ts index 01ba49a3..317dcca3 100644 --- a/server/test/lib/session-store.test.ts +++ b/server/test/lib/session-store.test.ts @@ -77,12 +77,12 @@ describe("updateSessionTokens", () => { }, 900, ); - const before = Math.floor(Date.now() / 1000); + const tokenExpiresAt = Math.floor(Date.now() / 1000) + 300; const wrote = await updateSessionTokens( redis, sessionId, - { bearerToken: "new-at", refreshToken: "new-rt", idToken: "new-idt" }, // pragma: allowlist secret + { bearerToken: "new-at", refreshToken: "new-rt", idToken: "new-idt", tokenExpiresAt }, // pragma: allowlist secret 300, ); @@ -91,8 +91,7 @@ describe("updateSessionTokens", () => { expect(stored?.bearerToken).toBe("new-at"); expect(stored?.refreshToken).toBe("new-rt"); expect(stored?.idToken).toBe("new-idt"); - expect(stored?.tokenExpiresAt).toBeGreaterThanOrEqual(before + 300); - expect(stored?.tokenExpiresAt).toBeLessThanOrEqual(before + 305); + expect(stored?.tokenExpiresAt).toBe(tokenExpiresAt); // user (and everything else on the record) is untouched by a token refresh. expect(stored?.user).toEqual({ email: "user@example.com", auth_provider: "sso" }); }); @@ -110,7 +109,8 @@ describe("updateSessionTokens", () => { 900, ); - await updateSessionTokens(redis, sessionId, { bearerToken: "new-at" }, 300); // pragma: allowlist secret + const tokenExpiresAt = Math.floor(Date.now() / 1000) + 300; + await updateSessionTokens(redis, sessionId, { bearerToken: "new-at", tokenExpiresAt }, 300); // pragma: allowlist secret const stored = await getSession(redis, sessionId); expect(stored?.refreshToken).toBe("stays-the-same-rt"); @@ -123,7 +123,7 @@ describe("updateSessionTokens", () => { const wrote = await updateSessionTokens( redis, "gone-id", - { bearerToken: "new-at" }, // pragma: allowlist secret + { bearerToken: "new-at", tokenExpiresAt: Math.floor(Date.now() / 1000) + 300 }, // pragma: allowlist secret 300, ); diff --git a/server/test/lib/sso-token-refresh.test.ts b/server/test/lib/sso-token-refresh.test.ts index bbc186d0..0badeee9 100644 --- a/server/test/lib/sso-token-refresh.test.ts +++ b/server/test/lib/sso-token-refresh.test.ts @@ -114,6 +114,7 @@ describe("refreshSsoSession", () => { const err = await refreshSsoSession(PARAMS).catch((e: unknown) => e); expect(err).toBeInstanceOf(SsoTokenRefreshError); + expect((err as InstanceType).code).toBe("unreachable"); expect((err as Error).message).not.toContain("invalid_grant"); }); @@ -123,15 +124,17 @@ describe("refreshSsoSession", () => { const err = await refreshSsoSession(PARAMS).catch((e: unknown) => e); expect(err).toBeInstanceOf(SsoTokenRefreshError); + expect((err as InstanceType).code).toBe("rejected"); expect((err as Error).message).toContain("invalid_grant"); }); - it("never leaks the token endpoint URL in the error message", async () => { + it("never leaks the token endpoint URL in the error message, and treats a 5xx as unreachable (not rejected)", async () => { mockTokenFetch({}, false, 500); const { refreshSsoSession, SsoTokenRefreshError } = await freshImport(); const err = await refreshSsoSession(PARAMS).catch((e: unknown) => e); expect(err).toBeInstanceOf(SsoTokenRefreshError); + expect((err as InstanceType).code).toBe("unreachable"); expect((err as Error).message).not.toContain(PARAMS.tokenEndpoint); }); diff --git a/server/test/plugins/session.test.ts b/server/test/plugins/session.test.ts new file mode 100644 index 00000000..867cd308 --- /dev/null +++ b/server/test/plugins/session.test.ts @@ -0,0 +1,552 @@ +// Location: ./client/server/test/plugins/session.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 + +import Fastify, { type FastifyInstance } from "fastify"; +import { type Redis } from "ioredis"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { createSession } from "../../src/lib/session-store.js"; +import cookiePlugin from "../../src/plugins/cookie.js"; +import sessionPlugin from "../../src/plugins/session.js"; +import { FakeRedis } from "../helpers/build-app.js"; + +const ENV_KEYS = [ + "SSO_ENABLED", + "SSO_KEYCLOAK_BASE_URL", + "SSO_KEYCLOAK_REALM", + "SSO_KEYCLOAK_CLIENT_ID", + "SSO_KEYCLOAK_CLIENT_SECRET", +] as const; + +const ISSUER = "http://keycloak-internal:8080/realms/mcp-gateway"; +const TOKEN_ENDPOINT = `${ISSUER}/protocol/openid-connect/token`; + +let savedEnv: Record; + +beforeEach(() => { + savedEnv = Object.fromEntries(ENV_KEYS.map((key) => [key, process.env[key]])); +}); + +afterEach(() => { + for (const key of ENV_KEYS) { + if (savedEnv[key] === undefined) delete process.env[key]; + else process.env[key] = savedEnv[key]; + } + vi.unstubAllGlobals(); +}); + +interface TestApp { + fastify: FastifyInstance; + redis: FakeRedis; +} + +async function buildApp(plugin: typeof sessionPlugin): Promise { + const fastify = Fastify(); + const redis = new FakeRedis(); + fastify.decorate("redis", redis as unknown as Redis); + await fastify.register(cookiePlugin); + await fastify.register(plugin); + fastify.get("/protected", { preHandler: [fastify.sessionAuth] }, async (request) => ({ + session: request.session, + })); + await fastify.ready(); + return { fastify, redis }; +} + +// config.ts reads SSO_* env vars at import time -- same reset-modules +// pattern as auth.test.ts's withSsoEnabled. +async function withSsoEnabled(run: () => Promise): Promise { + process.env.SSO_ENABLED = "true"; + process.env.SSO_KEYCLOAK_BASE_URL = "http://keycloak-internal:8080"; + process.env.SSO_KEYCLOAK_REALM = "mcp-gateway"; + process.env.SSO_KEYCLOAK_CLIENT_ID = "contextforge-web-ui"; + process.env.SSO_KEYCLOAK_CLIENT_SECRET = "dev-secret"; // pragma: allowlist secret + vi.resetModules(); + try { + return await run(); + } finally { + for (const key of ENV_KEYS) delete process.env[key]; + vi.resetModules(); + } +} + +function mockDiscoveryAndRefresh(opts: { refreshOk: boolean; expiresIn?: number }): void { + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + if (href.startsWith(`${ISSUER}/.well-known`)) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/protocol/openid-connect/auth`, + token_endpoint: TOKEN_ENDPOINT, + jwks_uri: `${ISSUER}/protocol/openid-connect/certs`, + }), + }; + } + if (href === TOKEN_ENDPOINT) { + if (!opts.refreshOk) { + return { ok: false, status: 400, json: async () => ({ error: "invalid_grant" }) }; + } + return { + ok: true, + status: 200, + json: async () => ({ + access_token: "new-access-token", // pragma: allowlist secret + refresh_token: "new-refresh-token", // pragma: allowlist secret + expires_in: opts.expiresIn ?? 300, + }), + }; + } + throw new Error(`unexpected fetch url in test: ${href}`); + }), + ); +} + +describe("sessionAuth", () => { + it("401s unauthenticated with no session cookie", async () => { + const app = await buildApp(sessionPlugin); + + const response = await app.fastify.inject({ method: "GET", url: "/protected" }); + + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "unauthenticated" }); + }); + + it("401s session_expired for an unknown session id", async () => { + const app = await buildApp(sessionPlugin); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: "bff_sid=nonexistent" }, + }); + + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "session_expired" }); + }); + + it("populates request.session for a valid password-login session", async () => { + const app = await buildApp(sessionPlugin); + const sessionId = await createSession( + app.redis, + { bearerToken: "upstream-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret + 900, + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toEqual({ + session: { sessionId, bearerToken: "upstream-jwt", user: { email: "user@example.com" } }, + }); + }); + + it("never attempts a refresh for a password-login session (no refreshToken)", async () => { + const app = await buildApp(sessionPlugin); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const sessionId = await createSession( + app.redis, + { bearerToken: "upstream-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret + 900, + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("transparently refreshes a near-expiry SSO session and updates the record in place", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, getSession } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now + 10, // inside the 30s default leeway + }, + 900, + ); + mockDiscoveryAndRefresh({ refreshOk: true, expiresIn: 300 }); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toMatchObject({ + session: { sessionId, bearerToken: "new-access-token" }, + }); + const stored = await getSession(app.redis, sessionId); + expect(stored?.bearerToken).toBe("new-access-token"); + expect(stored?.refreshToken).toBe("new-refresh-token"); + expect(stored?.tokenExpiresAt).toBeGreaterThanOrEqual(now + 300); + }); + }); + + it("re-issues the session cookie with the new TTL on a successful refresh", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, // original cookie's own maxAge, from a much shorter first login + ); + mockDiscoveryAndRefresh({ refreshOk: true, expiresIn: 300 }); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + // Without this, the browser drops bff_sid once the *original* login's + // maxAge elapses, even though Redis has been kept fresh by refreshes. + const cookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(cookie?.value).toBe(sessionId); + expect(cookie?.maxAge).toBe(300); + }); + }); + + it("locks concurrent refresh attempts so only one Keycloak refresh call happens", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + + let tokenEndpointCalls = 0; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + if (href.startsWith(`${ISSUER}/.well-known`)) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/protocol/openid-connect/auth`, + token_endpoint: TOKEN_ENDPOINT, + jwks_uri: `${ISSUER}/protocol/openid-connect/certs`, + }), + }; + } + if (href === TOKEN_ENDPOINT) { + tokenEndpointCalls += 1; + // Simulated latency so the second request genuinely finds the + // lock held, not just wins a race by luck. + await new Promise((resolve) => setTimeout(resolve, 200)); + return { + ok: true, + status: 200, + json: async () => ({ + access_token: "new-access-token", // pragma: allowlist secret + refresh_token: "new-refresh-token", // pragma: allowlist secret + expires_in: 300, + }), + }; + } + throw new Error(`unexpected fetch url in test: ${href}`); + }), + ); + + const [first, second] = await Promise.all([ + app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }), + app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }), + ]); + + expect(first.statusCode).toBe(200); + expect(second.statusCode).toBe(200); + // Not 2 -- a single-use/rotating refresh_token would reject the loser + // of an unlocked race with invalid_grant. + expect(tokenEndpointCalls).toBe(1); + }); + }); + + it("does not attempt a refresh for an SSO session that isn't near expiry", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "id-token", // pragma: allowlist secret + tokenExpiresAt: now + 300, + }, + 900, + ); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); + + it("401s like an unrefreshable session when Keycloak rejects the refresh_token", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "revoked-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + mockDiscoveryAndRefresh({ refreshOk: false }); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "session_expired" }); + }); + }); + + it("falls back to the pre-refresh token when the refresh POST itself is unreachable (discovery succeeds)", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + if (href.startsWith(`${ISSUER}/.well-known`)) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/protocol/openid-connect/auth`, + token_endpoint: TOKEN_ENDPOINT, + jwks_uri: `${ISSUER}/protocol/openid-connect/certs`, + }), + }; + } + throw new Error("keycloak token endpoint unreachable"); + }), + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toMatchObject({ + session: { sessionId, bearerToken: "old-access-token" }, + }); + }); + }); + + it("backs off a short window, not an immediate re-trigger, when a refresh response omits expires_in", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, getSession } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + if (href.startsWith(`${ISSUER}/.well-known`)) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/protocol/openid-connect/auth`, + token_endpoint: TOKEN_ENDPOINT, + jwks_uri: `${ISSUER}/protocol/openid-connect/certs`, + }), + }; + } + if (href === TOKEN_ENDPOINT) { + return { + ok: true, + status: 200, + json: async () => ({ access_token: "new-access-token" }), // pragma: allowlist secret + }; + } + throw new Error(`unexpected fetch url in test: ${href}`); + }), + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + const stored = await getSession(app.redis, sessionId); + // Not "now" (storm on every next request) and not sessionTtlSeconds + // (hours) -- a short, fixed backoff. + expect(stored?.tokenExpiresAt).toBeGreaterThan(now); + expect(stored?.tokenExpiresAt).toBeLessThan(now + 120); + }); + }); + + it("falls back to the pre-refresh token (not 401) when discovery is unreachable during a refresh attempt", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("keycloak unreachable"); + }), + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + // Unreachable discovery is transient -- says nothing about the + // refresh token itself, so this request still succeeds. + expect(response.statusCode).toBe(200); + expect(response.json()).toMatchObject({ + session: { sessionId, bearerToken: "old-access-token" }, + }); + }); + }); + + it("leaves a password-login session unaffected even when SSO is enabled", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const sessionId = await freshCreateSession( + app.redis, + { bearerToken: "upstream-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret + 900, + ); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); +}); From d2e1a8ea4ce5b2d97fbddf8a0b8b5c87e20489e3 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Tue, 29 Sep 2026 15:19:09 +0100 Subject: [PATCH 2/4] fix(session): harden SSO token-refresh lock and error handling - MemoryRedis: add set(PX,NX) and eval(), plus a compile-time RedisLike check so plugins/redis.ts's cast can't hide a signature drift again. - Refresh lock release is now compare-and-delete (per-holder token via a Lua eval), not an unconditional DEL that could wipe a different holder's lock after this one's PX TTL expired mid-refresh. - A timed-out lock wait re-checks the session before returning it; if still expired, forces re-auth instead of handing back a dead token. - Classifying Keycloak as unreachable now persists a short cooldown (past the refresh leeway) so an outage doesn't retrigger discovery on every request Signed-off-by: Gabriel Costa --- server/src/lib/memory-redis.ts | 43 +++++++++++++ server/src/lib/session-store.ts | 7 +++ server/src/plugins/session.ts | 65 ++++++++++++++++++-- server/test/helpers/build-app.ts | 12 ++++ server/test/memory-redis.test.ts | 20 ++++++ server/test/plugins/session.test.ts | 95 +++++++++++++++++++++++++++++ 6 files changed, 236 insertions(+), 6 deletions(-) diff --git a/server/src/lib/memory-redis.ts b/server/src/lib/memory-redis.ts index a8e5c284..1eb332f4 100644 --- a/server/src/lib/memory-redis.ts +++ b/server/src/lib/memory-redis.ts @@ -15,6 +15,8 @@ import { EventEmitter } from "node:events"; +import type { RedisLike } from "./session-store.js"; + export const MEMORY_REDIS_URL_PREFIX = "memory://"; export function isMemoryRedisUrl(url: string): boolean { @@ -61,10 +63,41 @@ export class MemoryRedis extends EventEmitter { return entry.value; } + // Mirrors ioredis's SET key value PX ms NX signature (atomic lock acquire). + async set( + key: string, + value: string, + _mode: "PX", + ttlMs: number, + flag: "NX", + ): Promise<"OK" | null> { + const existing = store.get(key); + if (flag === "NX" && existing && !isExpired(existing)) return null; + store.set(key, { value, expiresAt: Date.now() + ttlMs }); + return "OK"; + } + async del(key: string): Promise { return store.delete(key) ? 1 : 0; } + // ponytail: only implements the one compare-and-delete script this app + // issues (see UNLOCK_SCRIPT in plugins/session.ts), not general Lua -- + // MemoryRedis is dev-only and that's the sole script ever passed here. + // Safe without a real atomic guarantee: no `await` runs between the read + // and the delete, so nothing else in this single-threaded process can + // interleave. + async eval(_script: string, numKeys: number, ...args: Array): Promise { + const key = String(args[0]); + const expected = String(args[numKeys]); + const entry = store.get(key); + if (entry && !isExpired(entry) && entry.value === expected) { + store.delete(key); + return 1; + } + return 0; + } + async publish(channel: string, message: string): Promise { const before = bus.listenerCount("publish"); bus.emit("publish", channel, message); @@ -97,3 +130,13 @@ export class MemoryRedis extends EventEmitter { return "OK"; } } + +// plugins/redis.ts decorates fastify.redis with a MemoryRedis instance via +// `as unknown as FastifyInstance["redis"]`, which bypasses structural +// checking against the real ioredis type -- MemoryRedis can never fully +// satisfy that (hundreds of commands). This is the one place a MemoryRedis +// method actually falling behind RedisLike (session-store.ts's calling +// contract) would otherwise go unnoticed until it 500s at runtime instead of +// failing `tsc`. +const _redisLikeCheck: RedisLike = new MemoryRedis(); +void _redisLikeCheck; diff --git a/server/src/lib/session-store.ts b/server/src/lib/session-store.ts index 39a33aa5..da8cc239 100644 --- a/server/src/lib/session-store.ts +++ b/server/src/lib/session-store.ts @@ -28,6 +28,13 @@ export interface RedisLike { // "OK" only if the key was absent; PX auto-releases an abandoned lock. set(key: string, value: string, mode: "PX", ttlMs: number, flag: "NX"): Promise<"OK" | null>; del(key: string): Promise; + // Atomic Lua eval (ioredis's own `eval(script, numkeys, ...keys, ...args)`) + // -- used for compare-and-delete lock release (see UNLOCK_SCRIPT in + // plugins/session.ts). An unconditional DEL would let a second lock holder + // (whose lock was acquired after this one's PX TTL auto-expired while this + // holder's own refresh ran long) have its lock deleted by this holder's + // delayed release. + eval(script: string, numKeys: number, ...args: Array): Promise; publish(channel: string, message: string): Promise; } diff --git a/server/src/plugins/session.ts b/server/src/plugins/session.ts index 6b4a9a91..a58c3828 100644 --- a/server/src/plugins/session.ts +++ b/server/src/plugins/session.ts @@ -7,6 +7,8 @@ // per-route (proxy/auth/SSE), not globally — SSE routes need different CSRF // treatment, and /healthz and /auth/login must stay unauthenticated. +import { randomUUID } from "node:crypto"; + import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; import fp from "fastify-plugin"; @@ -32,6 +34,24 @@ const REFRESH_LOCK_TTL_MS = 12_000; const REFRESH_LOCK_POLL_MS = 100; const REFRESH_LOCK_MAX_WAIT_MS = 3_000; +// Compare-and-delete: only release a lock this holder itself acquired. An +// unconditional DEL would let a second holder's lock (acquired after this +// one's PX TTL auto-expired while this holder's own refresh ran long) be +// deleted by this holder's delayed `finally`, opening a window for a third +// holder to race a concurrent refresh_token grant against the same rotating +// token. +const UNLOCK_SCRIPT = ` +if redis.call("get", KEYS[1]) == ARGV[1] then + return redis.call("del", KEYS[1]) +else + return 0 +end +`; + +// Short backoff after a confirmed-unreachable IdP so every request during an +// outage doesn't each pay a fresh discovery+refresh round trip. +const UNREACHABLE_COOLDOWN_SECONDS = 5; + function needsRefresh(record: SessionRecord): boolean { if (!record.refreshToken || record.tokenExpiresAt === undefined) return false; const now = Math.floor(Date.now() / 1000); @@ -92,7 +112,30 @@ async function refreshRecord( err instanceof OidcDiscoveryError; if (isTransient) { request.log.warn({ err }, "SSO token refresh unreachable -- using pre-refresh token"); - return record; + // Persist a short cooldown so every request during an outage doesn't + // each retrigger discovery+refresh; without this, needsRefresh() stays + // true on the unchanged tokenExpiresAt and every subsequent request + // pays the full timeout again until Keycloak recovers. needsRefresh() + // fires whenever tokenExpiresAt is within the leeway window, so the + // cooldown has to clear that leeway too, not just add a few seconds + // to the (already-passed) real expiry. + const cooldownExpiresAt = + Math.floor(Date.now() / 1000) + + config.ssoTokenRefreshLeewaySeconds + + UNREACHABLE_COOLDOWN_SECONDS; + const wrote = await updateSessionTokens( + request.server.redis, + sessionId, + { + bearerToken: record.bearerToken, + refreshToken: record.refreshToken, + idToken: record.idToken, + tokenExpiresAt: cooldownExpiresAt, + }, + config.sessionTtlSeconds, + ); + if (!wrote) return null; + return { ...record, tokenExpiresAt: cooldownExpiresAt }; } request.log.warn({ err }, "SSO token refresh failed"); return null; @@ -108,7 +151,14 @@ async function refreshRecordWithLock( record: SessionRecord, ): Promise { const lockKey = sessionRefreshLockKey(sessionId); - const acquired = await request.server.redis.set(lockKey, "1", "PX", REFRESH_LOCK_TTL_MS, "NX"); + const lockToken = randomUUID(); + const acquired = await request.server.redis.set( + lockKey, + lockToken, + "PX", + REFRESH_LOCK_TTL_MS, + "NX", + ); if (!acquired) { const deadline = Date.now() + REFRESH_LOCK_MAX_WAIT_MS; @@ -118,16 +168,19 @@ async function refreshRecordWithLock( if (!current) return null; if (!needsRefresh(current)) return current; } - // Gave up waiting -- use the pre-refresh record rather than 401 over - // lock contention; the next request gets another chance. + // Gave up waiting on the lock holder. Re-check rather than trust the + // pre-refresh record blindly -- if it's still expired, force re-auth + // instead of handing back a token that'll just 401 upstream. request.log.warn({ sessionId }, "SSO token refresh lock wait timed out"); - return record; + const latest = await getSession(request.server.redis, sessionId); + if (!latest) return null; + return needsRefresh(latest) ? null : latest; } try { return await refreshRecord(request, reply, sessionId, record); } finally { - await request.server.redis.del(lockKey); + await request.server.redis.eval(UNLOCK_SCRIPT, 1, lockKey, lockToken); } } diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index 82e0dc13..47dad3c2 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -61,6 +61,18 @@ export class FakeRedis { return this.store.delete(key) ? 1 : 0; } + // Only implements the compare-and-delete script plugins/session.ts issues + // (UNLOCK_SCRIPT), not general Lua -- the sole script this app ever evals. + async eval(_script: string, numKeys: number, ...args: Array): Promise { + const key = String(args[0]); + const expected = String(args[numKeys]); + if (this.store.get(key) === expected) { + this.store.delete(key); + return 1; + } + return 0; + } + async publish(channel: string, message: string): Promise { this.published.push({ channel, message }); return 0; diff --git a/server/test/memory-redis.test.ts b/server/test/memory-redis.test.ts index b476c6e7..da03a220 100644 --- a/server/test/memory-redis.test.ts +++ b/server/test/memory-redis.test.ts @@ -33,6 +33,26 @@ describe("MemoryRedis", () => { } }); + it("set(...,'PX',ttl,'NX') only succeeds when the key is absent or expired", async () => { + const redis = new MemoryRedis(); + expect(await redis.set("lock1", "a", "PX", 60_000, "NX")).toBe("OK"); + expect(await redis.set("lock1", "b", "PX", 60_000, "NX")).toBeNull(); + expect(await redis.get("lock1")).toBe("a"); + }); + + it("eval only deletes when the stored value matches (compare-and-delete lock release)", async () => { + const redis = new MemoryRedis(); + await redis.set("lock2", "holder-a", "PX", 60_000, "NX"); + + // Wrong token -- simulates a second lock holder's value; must survive. + expect(await redis.eval("unused", 1, "lock2", "holder-b")).toBe(0); + expect(await redis.get("lock2")).toBe("holder-a"); + + // Matching token -- this holder's own release. + expect(await redis.eval("unused", 1, "lock2", "holder-a")).toBe(1); + expect(await redis.get("lock2")).toBeNull(); + }); + it("delivers publish() to a matching psubscribe() pattern across instances", async () => { const subscriber = new MemoryRedis(); const publisher = new MemoryRedis(); diff --git a/server/test/plugins/session.test.ts b/server/test/plugins/session.test.ts index 867cd308..cb7f74cd 100644 --- a/server/test/plugins/session.test.ts +++ b/server/test/plugins/session.test.ts @@ -316,6 +316,51 @@ describe("sessionAuth", () => { }); }); + it("forces re-auth, not a stale token, when the lock wait times out and the session is still expired", async () => { + await withSsoEnabled(async () => { + vi.useFakeTimers(); + try { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, sessionRefreshLockKey } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + // Simulate another instance holding the refresh lock for the whole + // wait window -- this request's own SET NX fails immediately, so it + // polls and times out without ever seeing a completed refresh. + await app.redis.set(sessionRefreshLockKey(sessionId), "other-holder", "PX", 60_000, "NX"); + vi.stubGlobal("fetch", vi.fn()); + + const injectPromise = app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + await vi.advanceTimersByTimeAsync(4_000); + const response = await injectPromise; + + // Not the stale pre-refresh record -- the session is still expired + // after giving up on the lock, so this must 401 like any other dead + // session rather than hand back a token that'll just fail upstream. + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "session_expired" }); + } finally { + vi.useRealTimers(); + } + }); + }); + it("does not attempt a refresh for an SSO session that isn't near expiry", async () => { await withSsoEnabled(async () => { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); @@ -526,6 +571,56 @@ describe("sessionAuth", () => { }); }); + it("backs off after an unreachable IdP so a second request doesn't re-attempt discovery", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, getSession } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + const fetchMock = vi.fn(async () => { + throw new Error("keycloak unreachable"); + }); + vi.stubGlobal("fetch", fetchMock); + + const first = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + expect(first.statusCode).toBe(200); + expect(fetchMock).toHaveBeenCalledTimes(1); + + const stored = await getSession(app.redis, sessionId); + // Moved forward past the refresh leeway window (default 30s), not + // left at the old expired value (which would retrigger a refresh + // attempt on every request regardless of a small forward bump). + expect(stored?.tokenExpiresAt).toBeGreaterThan(now + 30); + expect(stored?.tokenExpiresAt).toBeLessThan(now + 45); + + const second = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + expect(second.statusCode).toBe(200); + // Still inside the cooldown window -- must not pay another discovery + // round trip during an ongoing outage. + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + }); + it("leaves a password-login session unaffected even when SSO is enabled", async () => { await withSsoEnabled(async () => { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); From 512dcddea7e8f81b67c38807a9d0588ea7a27a69 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Thu, 1 Oct 2026 13:21:57 +0100 Subject: [PATCH 3/4] fix(session): fix stale-token reuse and errors Re-read the session after acquiring the refresh lock instead of reusing the pre-acquire record -- a holder winning the lock right after another holder rotated the refresh token would otherwise refresh with the now-superseded token and get invalid_grant'd. Classify refresh failures by Keycloak's RFC 6749 error code, not HTTP status class. Only invalid_grant means the refresh token is dead; invalid_client, temporarily_unavailable, and 429 are all 4xx but say nothing about the token, so logging the session out on those was wrong. On a lock-wait timeout, re-check the session and the lock before giving up: still expired with the lock gone means the attempt failed (401); still expired with the lock still held means another holder is still working (a refresh can legitimately take longer than the wait budget), so use the pre-refresh token instead of force-logging out a session on track to succeed. Signed-off-by: Gabriel Costa --- .env.example | 7 +- .secrets.baseline | 15 +- server/src/lib/establish-session.ts | 29 ++- server/src/lib/memory-redis.ts | 13 +- server/src/lib/session-store.ts | 39 +++- server/src/lib/sso-token-refresh.ts | 12 +- server/src/plugins/session.ts | 65 +++++-- server/test/helpers/build-app.ts | 7 +- server/test/lib/establish-session.test.ts | 12 +- server/test/lib/session-store.test.ts | 36 ++++ server/test/lib/sso-token-refresh.test.ts | 20 ++ server/test/plugins/session.test.ts | 217 ++++++++++++++++++++-- 12 files changed, 392 insertions(+), 80 deletions(-) diff --git a/.env.example b/.env.example index 52f04181..9c213255 100644 --- a/.env.example +++ b/.env.example @@ -75,6 +75,11 @@ LOG_LEVEL=info # Keycloak SSO login (BFF's own OIDC client). Disabled by default -- leave # SSO_ENABLED=false to skip all of this. +# +# Values below match mcp-context-forge's own `make compose-sso` stack -- +# both Keycloak clients ("mcp-gateway" for the gateway's own /admin UI, +# "contextforge-web-ui" for this BFF) are seeded directly in that repo's +# infra/keycloak/realm-export.json, no manual admin-console setup needed. SSO_ENABLED=false # SSO_KEYCLOAK_BASE_URL=http://localhost:8180 # Only needed if the browser-facing Keycloak host differs from the one above @@ -82,7 +87,7 @@ SSO_ENABLED=false # SSO_KEYCLOAK_PUBLIC_BASE_URL= # SSO_KEYCLOAK_REALM=mcp-gateway # SSO_KEYCLOAK_CLIENT_ID=contextforge-web-ui -# SSO_KEYCLOAK_CLIENT_SECRET= +# SSO_KEYCLOAK_CLIENT_SECRET=contextforge-web-ui-dev-secret # SSO_KEYCLOAK_SCOPES=openid profile email # SSO_LOGIN_STATE_TTL_SECONDS=300 # SSO_TOKEN_REFRESH_LEEWAY_SECONDS=30 diff --git a/.secrets.baseline b/.secrets.baseline index 12e28b2d..20423006 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -3,7 +3,7 @@ "files": "(?x)(package-lock\\.json$)|^\\.secrets\\.baseline$|src/i18n/locales/*|openapi.json|^.secrets.baseline$", "lines": null }, - "generated_at": "2026-09-10T08:05:24Z", + "generated_at": "2026-10-01T15:44:55Z", "plugins_used": [ { "name": "AWSKeyDetector" @@ -76,7 +76,18 @@ "name": "TwilioKeyDetector" } ], - "results": {}, + "results": { + ".env.example": [ + { + "hashed_secret": "c748440d5f1dd8ee8b1e5b3c96af779d3daa07a9", + "is_secret": false, + "is_verified": false, + "line_number": 90, + "type": "Secret Keyword", + "verified_result": null + } + ] + }, "version": "0.13.1+ibm.64.dss", "word_list": { "file": null, diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts index 177058fd..b1b8e2f1 100644 --- a/server/src/lib/establish-session.ts +++ b/server/src/lib/establish-session.ts @@ -58,28 +58,25 @@ export async function establishSession( throw new PasswordChangeStillRequiredError(); } - // The BFF session/cookie must not outlive the bearer token it wraps — use - // the upstream JWT's own lifetime, not a fixed BFF-side default. See - // createSession's comment in lib/session-store.ts. - let ttlSeconds = config.sessionTtlSeconds; - // Separate from ttlSeconds' BFF-default fallback -- tokenExpiresAt must never - // borrow it and claim an unknown-lifetime access token is still valid. - let ssoTokenTtlSeconds = 0; - if (Number.isFinite(auth.expires_in) && auth.expires_in! > 0) { - ttlSeconds = auth.expires_in!; - ssoTokenTtlSeconds = auth.expires_in!; - } else if (auth.expires_in !== undefined) { - // Upstream sent expires_in, but it's not a usable positive number — fall - // back, but log it: this means the BFF session can outlive the JWT it - // wraps until the proxy's revoke-on-401 catches up (see session-store.ts). - // A simply *absent* expires_in is not logged here — some upstream - // login-shaped endpoints don't send it by design (see upstream-login.ts). + const validExpiresIn = + Number.isFinite(auth.expires_in) && auth.expires_in! > 0 ? auth.expires_in! : undefined; + if (validExpiresIn === undefined && auth.expires_in !== undefined) { request.log.warn( { expires_in: auth.expires_in }, "upstream login returned invalid expires_in, using BFF default session TTL", ); } + // Password-login sessions have no refresh path, so the cookie/Redis TTL + // must match the JWT's own lifetime (session dies when the JWT does). SSO + // sessions can outlive the access token via refreshToken -- tying them to + // the same short expires_in would make sessionAuth's refresh unreachable + // for any idle gap longer than one access-token lifetime. + const ttlSeconds = ssoTokens + ? config.sessionTtlSeconds + : (validExpiresIn ?? config.sessionTtlSeconds); + const ssoTokenTtlSeconds = validExpiresIn ?? 0; + const sessionId = await createSession( fastify.redis, { diff --git a/server/src/lib/memory-redis.ts b/server/src/lib/memory-redis.ts index 1eb332f4..e2660964 100644 --- a/server/src/lib/memory-redis.ts +++ b/server/src/lib/memory-redis.ts @@ -63,16 +63,19 @@ export class MemoryRedis extends EventEmitter { return entry.value; } - // Mirrors ioredis's SET key value PX ms NX signature (atomic lock acquire). + // Mirrors ioredis's SET key value [EX secs|PX ms] [NX|XX] signature. async set( key: string, value: string, - _mode: "PX", - ttlMs: number, - flag: "NX", + mode: "PX" | "EX", + ttl: number, + flag: "NX" | "XX", ): Promise<"OK" | null> { const existing = store.get(key); - if (flag === "NX" && existing && !isExpired(existing)) return null; + const exists = !!existing && !isExpired(existing); + if (flag === "NX" && exists) return null; + if (flag === "XX" && !exists) return null; + const ttlMs = mode === "EX" ? ttl * 1000 : ttl; store.set(key, { value, expiresAt: Date.now() + ttlMs }); return "OK"; } diff --git a/server/src/lib/session-store.ts b/server/src/lib/session-store.ts index da8cc239..e43ee9ac 100644 --- a/server/src/lib/session-store.ts +++ b/server/src/lib/session-store.ts @@ -24,9 +24,15 @@ export interface RedisLike { // second concurrent caller's get() can still observe the value. getdel(key: string): Promise; setex(key: string, ttlSeconds: number, value: string): Promise; - // Atomic lock acquire (ioredis's own SET key value PX ms NX signature) -- - // "OK" only if the key was absent; PX auto-releases an abandoned lock. - set(key: string, value: string, mode: "PX", ttlMs: number, flag: "NX"): Promise<"OK" | null>; + // PX+NX for lock acquire; EX+XX for a conditional update that no-ops if + // the key's gone (see updateSessionTokens). + set( + key: string, + value: string, + mode: "PX" | "EX", + ttl: number, + flag: "NX" | "XX", + ): Promise<"OK" | null>; del(key: string): Promise; // Atomic Lua eval (ioredis's own `eval(script, numkeys, ...keys, ...args)`) // -- used for compare-and-delete lock release (see UNLOCK_SCRIPT in @@ -56,6 +62,8 @@ export interface SessionRecord { refreshToken?: string; idToken?: string; tokenExpiresAt?: number; + // Skip-refresh-until marker after a transient failure; cleared on success. + refreshRetryAfter?: number; } export function sessionRedisKey(sessionId: string): string { @@ -104,13 +112,18 @@ export async function getSession( // Re-persists a session's tokens in place after an SSO token refresh -- same // session id, so the browser's cookie never needs to change. Keycloak doesn't -// always rotate the refresh token on every use, so a field the refresh -// response omits keeps its previous value rather than being wiped. Returns -// false (no write) if the session was deleted (logout, expiry) mid-refresh. +// always rotate the refresh token on every use, so an omitted field keeps +// its previous value instead of being wiped. export async function updateSessionTokens( redis: RedisLike, sessionId: string, - tokens: { bearerToken: string; refreshToken?: string; idToken?: string; tokenExpiresAt: number }, + tokens: { + bearerToken: string; + refreshToken?: string; + idToken?: string; + tokenExpiresAt: number; + refreshRetryAfter?: number; + }, ttlSeconds: number, ): Promise { const existing = await getSession(redis, sessionId); @@ -122,9 +135,17 @@ export async function updateSessionTokens( refreshToken: tokens.refreshToken ?? existing.refreshToken, idToken: tokens.idToken ?? existing.idToken, tokenExpiresAt: tokens.tokenExpiresAt, + refreshRetryAfter: tokens.refreshRetryAfter, }; - await redis.setex(sessionRedisKey(sessionId), ttlSeconds, JSON.stringify(updated)); - return true; + // XX: no-op instead of recreating a session a concurrent logout just deleted. + const result = await redis.set( + sessionRedisKey(sessionId), + JSON.stringify(updated), + "EX", + ttlSeconds, + "XX", + ); + return result === "OK"; } export async function deleteSession(redis: RedisLike, sessionId: string): Promise { diff --git a/server/src/lib/sso-token-refresh.ts b/server/src/lib/sso-token-refresh.ts index 5092ddc9..a25cc730 100644 --- a/server/src/lib/sso-token-refresh.ts +++ b/server/src/lib/sso-token-refresh.ts @@ -87,12 +87,14 @@ export async function refreshSsoSession(params: { const responseBody = json as Record; if (!response.ok) { - // Keycloak's own RFC 6749 error code -- invalid_grant means the refresh - // token is dead, distinct from "unreachable" above. + // Keycloak's own RFC 6749 error code is the actual verdict on the + // token, not the HTTP status class -- a 4xx can be rate-limiting + // (429), a desynced client_secret (invalid_client), or a momentary + // temporarily_unavailable, none of which say the refresh token itself + // is dead. Only invalid_grant does. Treating every 4xx as "rejected" + // would log a user out over conditions that clear on their own. const errorCode = typeof responseBody.error === "string" ? responseBody.error : "unknown_error"; - // 5xx is Keycloak's own server error (transient); 4xx is Keycloak - // explicitly rejecting this specific grant/token (not transient). - const code = response.status >= 500 ? "unreachable" : "rejected"; + const code = errorCode === "invalid_grant" ? "rejected" : "unreachable"; throw new SsoTokenRefreshError( `Keycloak token endpoint returned ${response.status} (${errorCode})`, code, diff --git a/server/src/plugins/session.ts b/server/src/plugins/session.ts index a58c3828..4876b0ab 100644 --- a/server/src/plugins/session.ts +++ b/server/src/plugins/session.ts @@ -55,9 +55,18 @@ const UNREACHABLE_COOLDOWN_SECONDS = 5; function needsRefresh(record: SessionRecord): boolean { if (!record.refreshToken || record.tokenExpiresAt === undefined) return false; const now = Math.floor(Date.now() / 1000); + if (record.refreshRetryAfter !== undefined && now < record.refreshRetryAfter) return false; return record.tokenExpiresAt - config.ssoTokenRefreshLeewaySeconds <= now; } +// True once the access token's real deadline has passed -- never fudged by +// a retry cooldown. Used to refuse forwarding a dead bearer token upstream. +function isActuallyExpired(record: SessionRecord): boolean { + return ( + record.tokenExpiresAt !== undefined && record.tokenExpiresAt <= Math.floor(Date.now() / 1000) + ); +} + function delay(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } @@ -78,7 +87,9 @@ async function refreshRecord( }); const validExpiresIn = tokens.expiresIn && tokens.expiresIn > 0 ? tokens.expiresIn : undefined; - const ttlSeconds = validExpiresIn ?? config.sessionTtlSeconds; + // Always the session TTL, never the refreshed access token's own + // expires_in -- otherwise this bug recurs on every refresh cycle. + const ttlSeconds = config.sessionTtlSeconds; const tokenExpiresAt = Math.floor(Date.now() / 1000) + (validExpiresIn ?? SSO_TOKEN_REFRESH_FALLBACK_SECONDS); @@ -112,17 +123,10 @@ async function refreshRecord( err instanceof OidcDiscoveryError; if (isTransient) { request.log.warn({ err }, "SSO token refresh unreachable -- using pre-refresh token"); - // Persist a short cooldown so every request during an outage doesn't - // each retrigger discovery+refresh; without this, needsRefresh() stays - // true on the unchanged tokenExpiresAt and every subsequent request - // pays the full timeout again until Keycloak recovers. needsRefresh() - // fires whenever tokenExpiresAt is within the leeway window, so the - // cooldown has to clear that leeway too, not just add a few seconds - // to the (already-passed) real expiry. - const cooldownExpiresAt = - Math.floor(Date.now() / 1000) + - config.ssoTokenRefreshLeewaySeconds + - UNREACHABLE_COOLDOWN_SECONDS; + // Don't touch tokenExpiresAt -- it's the real deadline. refreshRetryAfter + // just stops every request from re-paying a discovery+refresh timeout + // during an outage; it never claims an expired token is still valid. + const retryAfter = Math.floor(Date.now() / 1000) + UNREACHABLE_COOLDOWN_SECONDS; const wrote = await updateSessionTokens( request.server.redis, sessionId, @@ -130,12 +134,14 @@ async function refreshRecord( bearerToken: record.bearerToken, refreshToken: record.refreshToken, idToken: record.idToken, - tokenExpiresAt: cooldownExpiresAt, + tokenExpiresAt: record.tokenExpiresAt!, + refreshRetryAfter: retryAfter, }, config.sessionTtlSeconds, ); if (!wrote) return null; - return { ...record, tokenExpiresAt: cooldownExpiresAt }; + const updated = { ...record, refreshRetryAfter: retryAfter }; + return isActuallyExpired(updated) ? null : updated; } request.log.warn({ err }, "SSO token refresh failed"); return null; @@ -148,7 +154,6 @@ async function refreshRecordWithLock( request: FastifyRequest, reply: FastifyReply, sessionId: string, - record: SessionRecord, ): Promise { const lockKey = sessionRefreshLockKey(sessionId); const lockToken = randomUUID(); @@ -174,11 +179,28 @@ async function refreshRecordWithLock( request.log.warn({ sessionId }, "SSO token refresh lock wait timed out"); const latest = await getSession(request.server.redis, sessionId); if (!latest) return null; - return needsRefresh(latest) ? null : latest; + if (!needsRefresh(latest)) return latest; + + // Still expired, but a refresh can legitimately take up to + // REFRESH_LOCK_TTL_MS -- longer than our own wait budget. If the lock + // is still held, someone's actively working; use the pre-refresh token + // for this request rather than force-logout a session on track to + // succeed. Only a vanished lock (holder crashed/released without + // writing fresh tokens) means the attempt genuinely failed. + const stillLocked = await request.server.redis.get(lockKey); + return stillLocked ? latest : null; } try { - return await refreshRecord(request, reply, sessionId, record); + // Re-read now that the lock is held, rather than reusing the `record` + // passed in from before the acquire. Without this, a holder who wins + // the lock right after a prior holder rotated the refresh token would + // refresh with that now-superseded token and get invalid_grant'd, even + // though the session is actually fine. + const fresh = await getSession(request.server.redis, sessionId); + if (!fresh) return null; + if (!needsRefresh(fresh)) return fresh; + return await refreshRecord(request, reply, sessionId, fresh); } finally { await request.server.redis.eval(UNLOCK_SCRIPT, 1, lockKey, lockToken); } @@ -198,13 +220,20 @@ async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promis } if (needsRefresh(record)) { - record = await refreshRecordWithLock(request, reply, sessionId, record); + record = await refreshRecordWithLock(request, reply, sessionId); if (!record) { reply.code(401).send({ error: "session_expired" }); return; } } + // Never forward a bearer past its real deadline, even if a retry backoff + // or a still-held lock skipped refreshing it this request. + if (isActuallyExpired(record)) { + reply.code(401).send({ error: "session_expired" }); + return; + } + request.session = { sessionId, bearerToken: record.bearerToken, user: record.user }; } diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index 47dad3c2..10b09bcf 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -48,11 +48,12 @@ export class FakeRedis { async set( key: string, value: string, - _mode: "PX", - _ttlMs: number, - flag: "NX", + _mode: "PX" | "EX", + _ttl: number, + flag: "NX" | "XX", ): Promise<"OK" | null> { if (flag === "NX" && this.store.has(key)) return null; + if (flag === "XX" && !this.store.has(key)) return null; this.store.set(key, value); return "OK"; } diff --git a/server/test/lib/establish-session.test.ts b/server/test/lib/establish-session.test.ts index 2badf8bf..1c7a0c55 100644 --- a/server/test/lib/establish-session.test.ts +++ b/server/test/lib/establish-session.test.ts @@ -108,16 +108,20 @@ describe("establishSession", () => { }); expect(response.statusCode).toBe(200); - const sessionId = response.cookies.find((c) => c.name === "bff_sid")?.value; + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + const sessionId = sessionCookie?.value; const stored = await getSession(app.redis, sessionId!); expect(stored?.refreshToken).toBe("keycloak-refresh-token"); expect(stored?.idToken).toBe("keycloak-id-token"); - // tokenExpiresAt is derived from the session TTL (expires_in), computed - // at call time -- assert it landed in the expected window rather than an - // exact value. + // tokenExpiresAt tracks the access token's own short expires_in... expect(stored?.tokenExpiresAt).toBeGreaterThanOrEqual(before + 300); expect(stored?.tokenExpiresAt).toBeLessThanOrEqual(before + 300 + 5); + // ...but the session/cookie TTL must not: refreshToken exists so this + // session can outlive the access token, and tying its lifetime to the + // same 300s would make sessionAuth's refresh unreachable for any idle + // gap longer than that. + expect(sessionCookie?.maxAge).toBe(config.sessionTtlSeconds); }); it("treats tokenExpiresAt as already-expired, not BFF-default-valid, when expires_in is missing/invalid", async () => { diff --git a/server/test/lib/session-store.test.ts b/server/test/lib/session-store.test.ts index 317dcca3..993819df 100644 --- a/server/test/lib/session-store.test.ts +++ b/server/test/lib/session-store.test.ts @@ -10,6 +10,7 @@ import { getSession, sessionRedisKey, updateSessionTokens, + type RedisLike, type SessionRecord, } from "../../src/lib/session-store.js"; import { FakeRedis } from "../helpers/build-app.js"; @@ -130,6 +131,41 @@ describe("updateSessionTokens", () => { expect(wrote).toBe(false); expect(await getSession(redis, "gone-id")).toBeNull(); }); + + it("does not resurrect a session deleted between the read and the write (logout/refresh race)", async () => { + const redis = new FakeRedis(); + const sessionId = await createSession( + redis, + { + bearerToken: "old-at", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-rt", // pragma: allowlist secret + tokenExpiresAt: 1_700_000_000, + }, + 900, + ); + // Simulates logout's `del` landing after our read but before our write -- + // the real SET's EX+XX flags make this a no-op server-side regardless of + // what the stale local read already knew. + const raceyRedis: RedisLike = { + get: redis.get.bind(redis), + getdel: redis.getdel.bind(redis), + setex: redis.setex.bind(redis), + set: async () => null, + del: redis.del.bind(redis), + eval: redis.eval.bind(redis), + publish: redis.publish.bind(redis), + }; + + const wrote = await updateSessionTokens( + raceyRedis, + sessionId, + { bearerToken: "new-at", tokenExpiresAt: Math.floor(Date.now() / 1000) + 300 }, // pragma: allowlist secret + 300, + ); + + expect(wrote).toBe(false); + }); }); describe("deleteSession", () => { diff --git a/server/test/lib/sso-token-refresh.test.ts b/server/test/lib/sso-token-refresh.test.ts index 0badeee9..9c232677 100644 --- a/server/test/lib/sso-token-refresh.test.ts +++ b/server/test/lib/sso-token-refresh.test.ts @@ -138,6 +138,26 @@ describe("refreshSsoSession", () => { expect((err as Error).message).not.toContain(PARAMS.tokenEndpoint); }); + it("treats a 429 as unreachable, not rejected -- rate-limiting says nothing about the token", async () => { + mockTokenFetch({ error: "temporarily_unavailable" }, false, 429); + const { refreshSsoSession, SsoTokenRefreshError } = await freshImport(); + + const err = await refreshSsoSession(PARAMS).catch((e: unknown) => e); + expect(err).toBeInstanceOf(SsoTokenRefreshError); + expect((err as InstanceType).code).toBe("unreachable"); + }); + + it("treats a desynced client_secret (invalid_client) as unreachable, not rejected", async () => { + mockTokenFetch({ error: "invalid_client" }, false, 401); + const { refreshSsoSession, SsoTokenRefreshError } = await freshImport(); + + const err = await refreshSsoSession(PARAMS).catch((e: unknown) => e); + expect(err).toBeInstanceOf(SsoTokenRefreshError); + // Not the refresh token's fault -- forcing logout over this would be + // wrong, and it may clear once the secret is fixed. + expect((err as InstanceType).code).toBe("unreachable"); + }); + it("throws SsoTokenRefreshError on a non-JSON body", async () => { vi.stubGlobal( "fetch", diff --git a/server/test/plugins/session.test.ts b/server/test/plugins/session.test.ts index cb7f74cd..6ab66676 100644 --- a/server/test/plugins/session.test.ts +++ b/server/test/plugins/session.test.ts @@ -233,11 +233,12 @@ describe("sessionAuth", () => { }); expect(response.statusCode).toBe(200); - // Without this, the browser drops bff_sid once the *original* login's - // maxAge elapses, even though Redis has been kept fresh by refreshes. + // The session TTL (default 86400s), not the refreshed access token's + // own 300s expires_in -- otherwise the cookie dies at the next access + // token's expiry again, same bug for a different reason. const cookie = response.cookies.find((c) => c.name === "bff_sid"); expect(cookie?.value).toBe(sessionId); - expect(cookie?.maxAge).toBe(300); + expect(cookie?.maxAge).toBe(86400); }); }); @@ -316,7 +317,59 @@ describe("sessionAuth", () => { }); }); - it("forces re-auth, not a stale token, when the lock wait times out and the session is still expired", async () => { + it("re-reads the session after acquiring the lock instead of reusing the stale pre-lock record", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, updateSessionTokens } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "old-refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + // Simulate a concurrent request winning the race: by the time this + // one acquires the (now-free) lock, Redis already holds rotated + // tokens -- the `record` sessionAuth read before the acquire is + // stale. Refreshing with its old, already-superseded refreshToken + // would get invalid_grant'd even though the session is fine. + await updateSessionTokens( + app.redis, + sessionId, + { + bearerToken: "new-access-token", // pragma: allowlist secret + refreshToken: "new-refresh-token", // pragma: allowlist secret + idToken: "new-id-token", // pragma: allowlist secret + tokenExpiresAt: now + 300, + }, + 900, + ); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + expect(response.statusCode).toBe(200); + expect(response.json()).toMatchObject({ + session: { sessionId, bearerToken: "new-access-token" }, + }); + // Already fresh once re-read under the lock -- no refresh attempted. + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); + + it("uses the pre-refresh token when the lock wait times out but the holder is still working", async () => { await withSsoEnabled(async () => { vi.useFakeTimers(); try { @@ -332,13 +385,16 @@ describe("sessionAuth", () => { user: { email: "user@example.com", auth_provider: "sso" }, refreshToken: "refresh-token", // pragma: allowlist secret idToken: "old-id-token", // pragma: allowlist secret - tokenExpiresAt: now - 10, + tokenExpiresAt: now + 10, // within leeway, not yet actually expired }, 900, ); // Simulate another instance holding the refresh lock for the whole // wait window -- this request's own SET NX fails immediately, so it - // polls and times out without ever seeing a completed refresh. + // polls and times out without ever seeing a completed refresh. The + // lock itself is still held the whole time (a refresh can + // legitimately take up to REFRESH_LOCK_TTL_MS, longer than our own + // REFRESH_LOCK_MAX_WAIT_MS wait budget). await app.redis.set(sessionRefreshLockKey(sessionId), "other-holder", "PX", 60_000, "NX"); vi.stubGlobal("fetch", vi.fn()); @@ -350,9 +406,100 @@ describe("sessionAuth", () => { await vi.advanceTimersByTimeAsync(4_000); const response = await injectPromise; - // Not the stale pre-refresh record -- the session is still expired - // after giving up on the lock, so this must 401 like any other dead - // session rather than hand back a token that'll just fail upstream. + // Someone's still actively refreshing (lock held) -- don't + // force-logout a session that's on track to succeed just because + // our own wait budget ran out. + expect(response.statusCode).toBe(200); + expect(response.json()).toMatchObject({ + session: { sessionId, bearerToken: "old-access-token" }, + }); + } finally { + vi.useRealTimers(); + } + }); + }); + + it("401s when the lock wait times out and the token is already actually expired, even with the lock still held", async () => { + await withSsoEnabled(async () => { + vi.useFakeTimers(); + try { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, sessionRefreshLockKey } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, // already past, not just near expiry + }, + 900, + ); + await app.redis.set(sessionRefreshLockKey(sessionId), "other-holder", "PX", 60_000, "NX"); + vi.stubGlobal("fetch", vi.fn()); + + const injectPromise = app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + await vi.advanceTimersByTimeAsync(4_000); + const response = await injectPromise; + + // A held lock means someone's refreshing, not that this bearer is + // still valid -- never forward a token past its real deadline. + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "session_expired" }); + } finally { + vi.useRealTimers(); + } + }); + }); + + it("forces re-auth when the lock wait times out and the holder is gone without refreshing", async () => { + await withSsoEnabled(async () => { + vi.useFakeTimers(); + try { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession, sessionRefreshLockKey } = + await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, + }, + 900, + ); + const lockKey = sessionRefreshLockKey(sessionId); + await app.redis.set(lockKey, "other-holder", "PX", 60_000, "NX"); + vi.stubGlobal("fetch", vi.fn()); + + const injectPromise = app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + // Let a couple of polls happen, then simulate the holder + // crashing/releasing without ever writing fresh tokens -- the lock + // vanishes but the session record is still expired. + await vi.advanceTimersByTimeAsync(500); + await app.redis.del(lockKey); + await vi.advanceTimersByTimeAsync(4_000); + const response = await injectPromise; + + // The lock is gone and nothing refreshed the session -- the attempt + // genuinely failed, so this must 401 rather than hand back a token + // that'll just fail upstream. expect(response.statusCode).toBe(401); expect(response.json()).toEqual({ error: "session_expired" }); } finally { @@ -435,7 +582,7 @@ describe("sessionAuth", () => { user: { email: "user@example.com", auth_provider: "sso" }, refreshToken: "refresh-token", // pragma: allowlist secret idToken: "old-id-token", // pragma: allowlist secret - tokenExpiresAt: now - 10, + tokenExpiresAt: now + 10, // within leeway, not yet actually expired }, 900, ); @@ -545,7 +692,7 @@ describe("sessionAuth", () => { user: { email: "user@example.com", auth_provider: "sso" }, refreshToken: "refresh-token", // pragma: allowlist secret idToken: "old-id-token", // pragma: allowlist secret - tokenExpiresAt: now - 10, + tokenExpiresAt: now + 10, // within leeway, not yet actually expired }, 900, ); @@ -571,6 +718,44 @@ describe("sessionAuth", () => { }); }); + it("401s (not silently served) when an unreachable IdP hits a session that's already actually expired", async () => { + await withSsoEnabled(async () => { + const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); + const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const app = await buildApp(freshSessionPlugin); + const now = Math.floor(Date.now() / 1000); + const sessionId = await freshCreateSession( + app.redis, + { + bearerToken: "old-access-token", // pragma: allowlist secret + user: { email: "user@example.com", auth_provider: "sso" }, + refreshToken: "refresh-token", // pragma: allowlist secret + idToken: "old-id-token", // pragma: allowlist secret + tokenExpiresAt: now - 10, // already past, not just near expiry + }, + 900, + ); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("keycloak unreachable"); + }), + ); + + const response = await app.fastify.inject({ + method: "GET", + url: "/protected", + headers: { cookie: `bff_sid=${sessionId}` }, + }); + + // A transient failure's grace period only covers a token that's still + // technically valid -- once the real deadline has passed, this must + // 401 rather than keep forwarding a dead bearer upstream. + expect(response.statusCode).toBe(401); + expect(response.json()).toEqual({ error: "session_expired" }); + }); + }); + it("backs off after an unreachable IdP so a second request doesn't re-attempt discovery", async () => { await withSsoEnabled(async () => { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); @@ -585,7 +770,7 @@ describe("sessionAuth", () => { user: { email: "user@example.com", auth_provider: "sso" }, refreshToken: "refresh-token", // pragma: allowlist secret idToken: "old-id-token", // pragma: allowlist secret - tokenExpiresAt: now - 10, + tokenExpiresAt: now + 10, // within leeway, not yet actually expired }, 900, ); @@ -603,11 +788,9 @@ describe("sessionAuth", () => { expect(fetchMock).toHaveBeenCalledTimes(1); const stored = await getSession(app.redis, sessionId); - // Moved forward past the refresh leeway window (default 30s), not - // left at the old expired value (which would retrigger a refresh - // attempt on every request regardless of a small forward bump). - expect(stored?.tokenExpiresAt).toBeGreaterThan(now + 30); - expect(stored?.tokenExpiresAt).toBeLessThan(now + 45); + // Real deadline untouched -- only the retry marker moved. + expect(stored?.tokenExpiresAt).toBe(now + 10); + expect(stored?.refreshRetryAfter).toBeGreaterThan(now); const second = await app.fastify.inject({ method: "GET", From 52b5319a7cfce5019703af4b06fa51c0ee400dcf Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Fri, 2 Oct 2026 09:39:01 +0100 Subject: [PATCH 4/4] Address comments Signed-off-by: Gabriel Costa --- server/src/lib/establish-session.ts | 11 ++++----- server/src/plugins/session.ts | 18 ++++++++++++-- server/test/lib/establish-session.test.ts | 30 +++++++++++++++++++++++ server/test/plugins/session.test.ts | 20 ++++++++++++--- 4 files changed, 67 insertions(+), 12 deletions(-) diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts index b1b8e2f1..7d56b39e 100644 --- a/server/src/lib/establish-session.ts +++ b/server/src/lib/establish-session.ts @@ -67,12 +67,11 @@ export async function establishSession( ); } - // Password-login sessions have no refresh path, so the cookie/Redis TTL - // must match the JWT's own lifetime (session dies when the JWT does). SSO - // sessions can outlive the access token via refreshToken -- tying them to - // the same short expires_in would make sessionAuth's refresh unreachable - // for any idle gap longer than one access-token lifetime. - const ttlSeconds = ssoTokens + // A session can only outlive the access token if it actually has a + // refreshToken to extend it with -- otherwise (password login, or an SSO + // response that omitted it) the cookie/Redis TTL must match the access + // token's own lifetime, same as sessionAuth's refresh gate requires. + const ttlSeconds = ssoTokens?.refreshToken ? config.sessionTtlSeconds : (validExpiresIn ?? config.sessionTtlSeconds); const ssoTokenTtlSeconds = validExpiresIn ?? 0; diff --git a/server/src/plugins/session.ts b/server/src/plugins/session.ts index 4876b0ab..efb4e827 100644 --- a/server/src/plugins/session.ts +++ b/server/src/plugins/session.ts @@ -15,6 +15,8 @@ import fp from "fastify-plugin"; import { config } from "../config.js"; import { getDiscoveryDocument, OidcDiscoveryError } from "../lib/oidc-discovery.js"; import { + clearSessionCookie, + deleteSession, getSession, sessionRefreshLockKey, setSessionCookie, @@ -71,6 +73,18 @@ function delay(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +// Matches catch-all.ts's upstream-401 revocation: a dead session must not +// keep looking "authenticated" to /auth/session for the rest of its TTL. +async function endSession( + request: FastifyRequest, + reply: FastifyReply, + sessionId: string, +): Promise { + await deleteSession(request.server.redis, sessionId); + clearSessionCookie(reply); + reply.code(401).send({ error: "session_expired" }); +} + // null only for a definite failure (rejected token, session gone). A // transient failure returns the ORIGINAL record so this request still works. async function refreshRecord( @@ -222,7 +236,7 @@ async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promis if (needsRefresh(record)) { record = await refreshRecordWithLock(request, reply, sessionId); if (!record) { - reply.code(401).send({ error: "session_expired" }); + await endSession(request, reply, sessionId); return; } } @@ -230,7 +244,7 @@ async function sessionAuth(request: FastifyRequest, reply: FastifyReply): Promis // Never forward a bearer past its real deadline, even if a retry backoff // or a still-held lock skipped refreshing it this request. if (isActuallyExpired(record)) { - reply.code(401).send({ error: "session_expired" }); + await endSession(request, reply, sessionId); return; } diff --git a/server/test/lib/establish-session.test.ts b/server/test/lib/establish-session.test.ts index 1c7a0c55..2cac57f8 100644 --- a/server/test/lib/establish-session.test.ts +++ b/server/test/lib/establish-session.test.ts @@ -124,6 +124,36 @@ describe("establishSession", () => { expect(sessionCookie?.maxAge).toBe(config.sessionTtlSeconds); }); + it("aligns the cookie/Redis TTL with expires_in for an SSO response with an idToken but no refreshToken", async () => { + const app = await buildEstablishSessionTestApp(); + + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "keycloak-access-token", // pragma: allowlist secret + expires_in: 300, + user: { email: "user@example.com", auth_provider: "sso" }, + }, + ssoTokens: { + idToken: "keycloak-id-token", // pragma: allowlist secret + // no refreshToken -- nothing can ever extend this session past + // the access token's own lifetime, so it must not get the long + // session-TTL treatment (that would leave /auth/session reporting + // authenticated for hours after the dead access token 401s). + }, + }, + }); + + expect(response.statusCode).toBe(200); + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie?.maxAge).toBe(300); + const stored = await getSession(app.redis, sessionCookie!.value); + expect(stored?.refreshToken).toBeUndefined(); + expect(stored?.idToken).toBe("keycloak-id-token"); + }); + it("treats tokenExpiresAt as already-expired, not BFF-default-valid, when expires_in is missing/invalid", async () => { const app = await buildEstablishSessionTestApp(); const before = Math.floor(Date.now() / 1000); diff --git a/server/test/plugins/session.test.ts b/server/test/plugins/session.test.ts index 6ab66676..40008216 100644 --- a/server/test/plugins/session.test.ts +++ b/server/test/plugins/session.test.ts @@ -465,8 +465,11 @@ describe("sessionAuth", () => { vi.useFakeTimers(); try { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); - const { createSession: freshCreateSession, sessionRefreshLockKey } = - await import("../../src/lib/session-store.js"); + const { + createSession: freshCreateSession, + sessionRefreshLockKey, + getSession, + } = await import("../../src/lib/session-store.js"); const app = await buildApp(freshSessionPlugin); const now = Math.floor(Date.now() / 1000); const sessionId = await freshCreateSession( @@ -502,6 +505,7 @@ describe("sessionAuth", () => { // that'll just fail upstream. expect(response.statusCode).toBe(401); expect(response.json()).toEqual({ error: "session_expired" }); + expect(await getSession(app.redis, sessionId)).toBeNull(); } finally { vi.useRealTimers(); } @@ -542,7 +546,8 @@ describe("sessionAuth", () => { it("401s like an unrefreshable session when Keycloak rejects the refresh_token", async () => { await withSsoEnabled(async () => { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); - const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const { createSession: freshCreateSession, getSession } = + await import("../../src/lib/session-store.js"); const app = await buildApp(freshSessionPlugin); const now = Math.floor(Date.now() / 1000); const sessionId = await freshCreateSession( @@ -566,6 +571,9 @@ describe("sessionAuth", () => { expect(response.statusCode).toBe(401); expect(response.json()).toEqual({ error: "session_expired" }); + // Not just a 401 response -- the record itself must be gone, or + // /auth/session keeps reporting authenticated: true for its full TTL. + expect(await getSession(app.redis, sessionId)).toBeNull(); }); }); @@ -721,7 +729,8 @@ describe("sessionAuth", () => { it("401s (not silently served) when an unreachable IdP hits a session that's already actually expired", async () => { await withSsoEnabled(async () => { const { default: freshSessionPlugin } = await import("../../src/plugins/session.js"); - const { createSession: freshCreateSession } = await import("../../src/lib/session-store.js"); + const { createSession: freshCreateSession, getSession } = + await import("../../src/lib/session-store.js"); const app = await buildApp(freshSessionPlugin); const now = Math.floor(Date.now() / 1000); const sessionId = await freshCreateSession( @@ -753,6 +762,9 @@ describe("sessionAuth", () => { // 401 rather than keep forwarding a dead bearer upstream. expect(response.statusCode).toBe(401); expect(response.json()).toEqual({ error: "session_expired" }); + // And the record itself must be gone, or /auth/session keeps + // reporting authenticated: true for the rest of an outage. + expect(await getSession(app.redis, sessionId)).toBeNull(); }); });