From d053227631a70991feb877919aff68b32da65440 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Thu, 24 Sep 2026 11:33:28 +0100 Subject: [PATCH 1/4] feat: SSO callback route and session wiring Adds GET /auth/sso/callback, completing the browser-facing half of the Keycloak login flow: it consumes the PKCE login state minted by /auth/sso/login, exchanges the authorization code for tokens, verifies and resolves the ID token into a SessionUser, and establishes the same cookie session password login uses. SessionRecord and establishSession() gain optional SSO-only fields, additive and backward-compatible with existing password-login sessions. - server/src/routes/auth/sso-callback.ts: the new route - server/src/lib/session-store.ts: SessionRecord gains optional refreshToken/idToken/tokenExpiresAt - server/src/lib/establish-session.ts: establishSession() accepts an optional ssoTokens param - server/src/index.ts, server/test/helpers/build-app.ts: register the route - server/test/sso-callback.test.ts: full round-trip coverage Signed-off-by: Gabriel Costa --- server/src/index.ts | 2 + server/src/lib/establish-session.ts | 14 +- server/src/lib/session-store.ts | 4 + server/src/routes/auth/sso-callback.ts | 140 ++++++++++ server/test/helpers/build-app.ts | 2 + server/test/sso-callback.test.ts | 356 +++++++++++++++++++++++++ 6 files changed, 517 insertions(+), 1 deletion(-) create mode 100644 server/src/routes/auth/sso-callback.ts create mode 100644 server/test/sso-callback.test.ts diff --git a/server/src/index.ts b/server/src/index.ts index 799c478c..e3eea2ff 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -23,6 +23,7 @@ import changePasswordRequiredRoute from "./routes/auth/change-password-required. import loginRoute from "./routes/auth/login.js"; import logoutRoute from "./routes/auth/logout.js"; import sessionRoute from "./routes/auth/session.js"; +import ssoCallbackRoute from "./routes/auth/sso-callback.js"; import ssoLoginRoute from "./routes/auth/sso-login.js"; import catchAllProxyRoute from "./routes/proxy/catch-all.js"; import oauthAuthorizeProxyRoute from "./routes/proxy/oauth-authorize.js"; @@ -55,6 +56,7 @@ await fastify.register(loginRoute); await fastify.register(logoutRoute); await fastify.register(sessionRoute); await fastify.register(ssoLoginRoute); +await fastify.register(ssoCallbackRoute); await fastify.register(changePasswordRequiredRoute); await fastify.register(sseRoutes); await fastify.register(publicPasswordResetRoute); diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts index f2b7301a..9eda2f31 100644 --- a/server/src/lib/establish-session.ts +++ b/server/src/lib/establish-session.ts @@ -23,6 +23,11 @@ export interface UpstreamAuthenticationResponse { user: SessionUser; } +export interface SsoTokens { + refreshToken?: string; + idToken?: string; +} + /** * Thrown when the upstream auth response still reports * password_change_required=true. This is the single chokepoint every @@ -47,6 +52,7 @@ export async function establishSession( request: FastifyRequest, reply: FastifyReply, auth: UpstreamAuthenticationResponse, // pragma: allowlist secret + ssoTokens?: SsoTokens, ): Promise<{ user: SessionUser; csrfToken: string }> { if (auth.user?.password_change_required === true) { throw new PasswordChangeStillRequiredError(); @@ -72,7 +78,13 @@ export async function establishSession( const sessionId = await createSession( fastify.redis, - { bearerToken: auth.access_token, user: auth.user }, + { + bearerToken: auth.access_token, + user: auth.user, + refreshToken: ssoTokens?.refreshToken, + idToken: ssoTokens?.idToken, + tokenExpiresAt: ssoTokens ? Math.floor(Date.now() / 1000) + ttlSeconds : undefined, + }, ttlSeconds, ); diff --git a/server/src/lib/session-store.ts b/server/src/lib/session-store.ts index c4adb028..3dc9d9f2 100644 --- a/server/src/lib/session-store.ts +++ b/server/src/lib/session-store.ts @@ -42,6 +42,10 @@ export interface SessionUser { export interface SessionRecord { bearerToken: string; user: SessionUser; + // SSO-only; undefined for password-login sessions. + refreshToken?: string; + idToken?: string; + tokenExpiresAt?: number; } export function sessionRedisKey(sessionId: string): string { diff --git a/server/src/routes/auth/sso-callback.ts b/server/src/routes/auth/sso-callback.ts new file mode 100644 index 00000000..9ded799d --- /dev/null +++ b/server/src/routes/auth/sso-callback.ts @@ -0,0 +1,140 @@ +// Location: ./client/server/src/routes/auth/sso-callback.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// GET /auth/sso/callback: Keycloak's redirect target. Exchanges the +// authorization code, resolves the user from the ID token, and establishes +// the same cookie session password login uses. See lib/sso-login-state.ts. + +import type { FastifyInstance, FastifyReply, FastifyRequest } from "fastify"; + +import { config } from "../../config.js"; +import { establishSession, PasswordChangeStillRequiredError } from "../../lib/establish-session.js"; +import { setNoStore } from "../../lib/no-store.js"; +import { getDiscoveryDocument } from "../../lib/oidc-discovery.js"; +import { consumeSsoLoginState, SSO_LOGIN_BINDING_COOKIE } from "../../lib/sso-login-state.js"; +import { exchangeSsoCode } from "../../lib/sso-token-exchange.js"; +import { decodeSsoIdToken, resolveSsoUser } from "../../lib/sso-user-resolution.js"; + +const LOGIN_PATH = "/app/login"; + +interface SsoCallbackQuerystring { + code?: string | string[]; + state?: string | string[]; + error?: string | string[]; +} + +function firstString(value: string | string[] | undefined): string | undefined { + return typeof value === "string" && value ? value : undefined; +} + +// Keycloak's own error codes (access_denied, ...) are short RFC 6749 tokens; +// URLSearchParams encodes whatever we're given either way. +function loginErrorRedirect(code: string): string { + const url = new URL(LOGIN_PATH, "http://placeholder"); + url.searchParams.set("error", `sso_${code}`); + return url.pathname + url.search; +} + +export default async function ssoCallbackRoute(fastify: FastifyInstance): Promise { + fastify.get<{ Querystring: SsoCallbackQuerystring }>( + "/auth/sso/callback", + async ( + request: FastifyRequest<{ Querystring: SsoCallbackQuerystring }>, + reply: FastifyReply, + ) => { + setNoStore(reply); + + const binding = request.cookies[SSO_LOGIN_BINDING_COOKIE]; + reply.clearCookie(SSO_LOGIN_BINDING_COOKIE, { path: "/", domain: config.cookieDomain }); + + if (!config.ssoEnabled) { + return reply.redirect(loginErrorRedirect("disabled")); + } + + const errorParam = firstString(request.query.error); + if (errorParam) { + return reply.redirect(loginErrorRedirect(errorParam)); + } + + const code = firstString(request.query.code); + const state = firstString(request.query.state); + if (!code || !state) { + return reply.redirect(loginErrorRedirect("callback_invalid")); + } + + const loginState = await consumeSsoLoginState(fastify.redis, state, binding); + if (!loginState) { + return reply.redirect(loginErrorRedirect("state_invalid")); + } + + let tokenEndpoint: string; + try { + tokenEndpoint = (await getDiscoveryDocument()).tokenEndpoint; + } catch (err) { + request.log.error({ err }, "SSO discovery failed"); + return reply.redirect(loginErrorRedirect("discovery_failed")); + } + + let tokens; + try { + tokens = await exchangeSsoCode({ + tokenEndpoint, + code, + redirectUri: loginState.redirectUri, + codeVerifier: loginState.codeVerifier, + }); + } catch (err) { + request.log.error({ err }, "SSO token exchange failed"); + return reply.redirect(loginErrorRedirect("token_exchange_failed")); + } + + if (!tokens.idToken) { + request.log.error("SSO token response missing id_token"); + return reply.redirect(loginErrorRedirect("id_token_missing")); + } + + let claims; + try { + claims = decodeSsoIdToken(tokens.idToken); + } catch (err) { + request.log.error({ err }, "SSO ID token decode failed"); + return reply.redirect(loginErrorRedirect("id_token_invalid")); + } + + // Confirms this ID token was issued for the authorization request this + // browser started, not replayed from an unrelated flow. + if (claims.nonce !== loginState.nonce) { + request.log.error("SSO ID token nonce mismatch"); + return reply.redirect(loginErrorRedirect("nonce_mismatch")); + } + + let user; + try { + user = resolveSsoUser(claims); + } catch (err) { + request.log.error({ err }, "SSO ID token missing required claims"); + return reply.redirect(loginErrorRedirect("email_missing")); + } + + try { + await establishSession( + fastify, + request, + reply, + { access_token: tokens.accessToken, expires_in: tokens.expiresIn, user }, + { refreshToken: tokens.refreshToken, idToken: tokens.idToken }, + ); + } catch (err) { + // Can't happen -- resolveSsoUser always sets password_change_required + // false -- but establishSession's own backstop exists for this path. + if (err instanceof PasswordChangeStillRequiredError) { + return reply.redirect(loginErrorRedirect("password_change_required")); + } + throw err; + } + + return reply.redirect(loginState.returnTo); + }, + ); +} diff --git a/server/test/helpers/build-app.ts b/server/test/helpers/build-app.ts index d618880f..02f9d803 100644 --- a/server/test/helpers/build-app.ts +++ b/server/test/helpers/build-app.ts @@ -17,6 +17,7 @@ import changePasswordRequiredRoute from "../../src/routes/auth/change-password-r import loginRoute from "../../src/routes/auth/login.js"; import logoutRoute from "../../src/routes/auth/logout.js"; import sessionRoute from "../../src/routes/auth/session.js"; +import ssoCallbackRoute from "../../src/routes/auth/sso-callback.js"; import ssoLoginRoute from "../../src/routes/auth/sso-login.js"; import catchAllProxyRoute from "../../src/routes/proxy/catch-all.js"; import oauthAuthorizeProxyRoute from "../../src/routes/proxy/oauth-authorize.js"; @@ -72,6 +73,7 @@ export async function buildTestApp(opts: { withProxy?: boolean } = {}): Promise< await fastify.register(logoutRoute); await fastify.register(sessionRoute); await fastify.register(ssoLoginRoute); + await fastify.register(ssoCallbackRoute); await fastify.register(changePasswordRequiredRoute); await fastify.register(publicPasswordResetRoute); diff --git a/server/test/sso-callback.test.ts b/server/test/sso-callback.test.ts new file mode 100644 index 00000000..726b30e0 --- /dev/null +++ b/server/test/sso-callback.test.ts @@ -0,0 +1,356 @@ +// Location: ./client/server/test/sso-callback.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// config.ts reads SSO_* env vars once at import time, so each case resets +// the module registry and re-imports build-app.js fresh -- same pattern as +// sso-login.test.ts. + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { SSO_LOGIN_BINDING_COOKIE } from "../src/lib/sso-login-state.js"; +import type { TestApp } from "./helpers/build-app.js"; + +const ENV_KEYS = [ + "SSO_ENABLED", + "SSO_KEYCLOAK_BASE_URL", + "SSO_KEYCLOAK_PUBLIC_BASE_URL", + "SSO_KEYCLOAK_REALM", + "SSO_KEYCLOAK_CLIENT_ID", + "SSO_KEYCLOAK_CLIENT_SECRET", +] as const; + +const TOKEN_ENDPOINT = + "http://keycloak-internal:8080/realms/mcp-gateway/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(); +}); + +function enableSso(): void { + 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 +} + +function makeIdToken(claims: Record): string { + const header = Buffer.from(JSON.stringify({ alg: "RS256" })).toString("base64url"); + const payload = Buffer.from(JSON.stringify(claims)).toString("base64url"); + return `${header}.${payload}.signature`; +} + +function mockDiscoveryFetch(): void { + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ + issuer: "http://keycloak-internal:8080/realms/mcp-gateway", + authorization_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/auth", + token_endpoint: TOKEN_ENDPOINT, + jwks_uri: "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + }), + })), + ); +} + +// Discovery is cached after the login call, so this only needs to serve the +// token endpoint for the callback -- anything else is a test-setup bug. +function mockTokenExchange(idTokenClaims: Record, ok = true, status = 400): void { + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + if (!String(url).endsWith("/protocol/openid-connect/token")) { + throw new Error(`unexpected fetch during callback test: ${url}`); + } + if (!ok) { + return { ok: false, status, json: async () => ({ error: "invalid_grant" }) }; + } + return { + ok: true, + status: 200, + json: async () => ({ + access_token: "keycloak-access-token", // pragma: allowlist secret + refresh_token: "keycloak-refresh-token", // pragma: allowlist secret + id_token: makeIdToken(idTokenClaims), // pragma: allowlist secret + expires_in: 300, + }), + }; + }), + ); +} + +async function freshBuildTestApp(): Promise { + vi.resetModules(); + const { buildTestApp } = await import("./helpers/build-app.js"); + return buildTestApp; +} + +async function performLogin( + app: TestApp, + next?: string, +): Promise<{ state: string; nonce: string; binding: string }> { + const url = next ? `/auth/sso/login?next=${encodeURIComponent(next)}` : "/auth/sso/login"; + const response = await app.fastify.inject({ method: "GET", url }); + const location = new URL(response.headers.location as string); + const binding = response.cookies.find((c) => c.name === SSO_LOGIN_BINDING_COOKIE)!.value; + return { + state: location.searchParams.get("state")!, + nonce: location.searchParams.get("nonce")!, + binding, + }; +} + +function callbackUrl(params: Record): string { + return `/auth/sso/callback?${new URLSearchParams(params).toString()}`; +} + +describe("GET /auth/sso/callback", () => { + it("exchanges the code, establishes a session, and redirects to the stored next path", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, nonce, binding } = await performLogin(app, "/app/tools"); + + mockTokenExchange({ email: "user@example.com", name: "Test User", nonce }); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.statusCode).toBe(302); + expect(response.headers.location).toBe("/app/tools"); + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie?.value).toBeTruthy(); + + const sessionResponse = await app.fastify.inject({ + method: "GET", + url: "/auth/session", + headers: { cookie: `bff_sid=${sessionCookie!.value}` }, + }); + expect(sessionResponse.json()).toMatchObject({ + authenticated: true, + user: { email: "user@example.com", auth_provider: "sso" }, + }); + }); + + it("clears the binding cookie on success", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, nonce, binding } = await performLogin(app); + + mockTokenExchange({ email: "user@example.com", nonce }); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + const cleared = response.cookies.find((c) => c.name === SSO_LOGIN_BINDING_COOKIE); + expect(cleared?.value).toBe(""); + }); + + it("consumes the state exactly once -- replaying the same callback fails", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, nonce, binding } = await performLogin(app); + + mockTokenExchange({ email: "user@example.com", nonce }); + const headers = { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }; + const first = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers, + }); + const second = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers, + }); + + expect(first.headers.location).not.toContain("error="); + expect(second.headers.location).toBe("/app/login?error=sso_state_invalid"); + expect(second.cookies.find((c) => c.name === "bff_sid")?.value ?? "").toBe(""); + }); + + it("redirects with a recognizable error on Keycloak's own access_denied, without establishing a session", async () => { + enableSso(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ error: "access_denied" }), + }); + + expect(response.statusCode).toBe(302); + expect(response.headers.location).toBe("/app/login?error=sso_access_denied"); + expect(response.cookies.find((c) => c.name === "bff_sid")).toBeUndefined(); + }); + + it("redirects with an error when code or state is missing", async () => { + enableSso(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code" }), + }); + + expect(response.headers.location).toBe("/app/login?error=sso_callback_invalid"); + }); + + it("redirects with an error for an unknown or already-expired state", async () => { + enableSso(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state: "unknown-state" }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=some-binding` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_state_invalid"); + }); + + it("redirects with an error when the binding cookie is missing", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state } = await performLogin(app); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + }); + + expect(response.headers.location).toBe("/app/login?error=sso_state_invalid"); + }); + + it("redirects with an error when the binding cookie doesn't match", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state } = await performLogin(app); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=wrong-binding` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_state_invalid"); + }); + + it("redirects with an error when the token exchange fails", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, binding } = await performLogin(app); + + mockTokenExchange({}, false); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_token_exchange_failed"); + }); + + it("redirects with an error when the token response has no id_token", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, binding } = await performLogin(app); + + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ + ok: true, + status: 200, + json: async () => ({ access_token: "at" }), // pragma: allowlist secret + })), + ); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_id_token_missing"); + }); + + it("redirects with an error when the ID token nonce doesn't match the login attempt", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, binding } = await performLogin(app); + + mockTokenExchange({ email: "user@example.com", nonce: "wrong-nonce" }); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_nonce_mismatch"); + }); + + it("redirects with an error when the ID token has no email claim", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, nonce, binding } = await performLogin(app); + + mockTokenExchange({ nonce }); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_email_missing"); + }); + + it("redirects with an error when SSO is disabled", async () => { + delete process.env.SSO_ENABLED; + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state: "any" }), + }); + + expect(response.headers.location).toBe("/app/login?error=sso_disabled"); + }); +}); From afe7551a0c239b6102ccb1aa5c147fa3ccc0732a Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Thu, 24 Sep 2026 16:28:35 +0100 Subject: [PATCH 2/4] Bugfix + tests Signed-off-by: Gabriel Costa --- server/src/routes/auth/sso-callback.ts | 6 +- server/test/lib/establish-session.test.ts | 201 ++++++++++++++++++++++ server/test/lib/session-store.test.ts | 80 +++++++++ server/test/sso-callback.test.ts | 68 ++++++-- 4 files changed, 337 insertions(+), 18 deletions(-) create mode 100644 server/test/lib/establish-session.test.ts create mode 100644 server/test/lib/session-store.test.ts diff --git a/server/src/routes/auth/sso-callback.ts b/server/src/routes/auth/sso-callback.ts index 9ded799d..f1f993d5 100644 --- a/server/src/routes/auth/sso-callback.ts +++ b/server/src/routes/auth/sso-callback.ts @@ -14,7 +14,7 @@ import { setNoStore } from "../../lib/no-store.js"; import { getDiscoveryDocument } from "../../lib/oidc-discovery.js"; import { consumeSsoLoginState, SSO_LOGIN_BINDING_COOKIE } from "../../lib/sso-login-state.js"; import { exchangeSsoCode } from "../../lib/sso-token-exchange.js"; -import { decodeSsoIdToken, resolveSsoUser } from "../../lib/sso-user-resolution.js"; +import { resolveSsoUser, verifySsoIdToken } from "../../lib/sso-user-resolution.js"; const LOGIN_PATH = "/app/login"; @@ -96,9 +96,9 @@ export default async function ssoCallbackRoute(fastify: FastifyInstance): Promis let claims; try { - claims = decodeSsoIdToken(tokens.idToken); + claims = await verifySsoIdToken(tokens.idToken); } catch (err) { - request.log.error({ err }, "SSO ID token decode failed"); + request.log.error({ err }, "SSO ID token verification failed"); return reply.redirect(loginErrorRedirect("id_token_invalid")); } diff --git a/server/test/lib/establish-session.test.ts b/server/test/lib/establish-session.test.ts new file mode 100644 index 00000000..da6d110e --- /dev/null +++ b/server/test/lib/establish-session.test.ts @@ -0,0 +1,201 @@ +// Location: ./client/server/test/lib/establish-session.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Exercises establishSession() directly through a minimal Fastify app (real +// cookie + CSRF plugins, fake Redis) rather than through a specific route, so +// this stays the one place both callers' shared TTL/CSRF/SSO-field behavior +// is pinned -- routes/auth/login.ts and routes/auth/sso-callback.ts each only +// need their own route-shaped tests on top of this. + +import Fastify, { type FastifyInstance } from "fastify"; +import { type Redis } from "ioredis"; +import { describe, expect, it } from "vitest"; + +import cookiePlugin from "../../src/plugins/cookie.js"; +import csrfPlugin from "../../src/plugins/csrf.js"; +import { + establishSession, + PasswordChangeStillRequiredError, + type SsoTokens, + type UpstreamAuthenticationResponse, +} from "../../src/lib/establish-session.js"; +import { config } from "../../src/config.js"; +import { getSession, sessionRedisKey } from "../../src/lib/session-store.js"; +import { FakeRedis } from "../helpers/build-app.js"; + +interface TestApp { + fastify: FastifyInstance; + redis: FakeRedis; +} + +async function buildEstablishSessionTestApp(): Promise { + const fastify = Fastify(); + const redis = new FakeRedis(); + fastify.decorate("redis", redis as unknown as Redis); + + await fastify.register(cookiePlugin); + await fastify.register(csrfPlugin); + + fastify.post<{ Body: { auth: UpstreamAuthenticationResponse; ssoTokens?: SsoTokens } }>( // pragma: allowlist secret + "/test/establish-session", + async (request, reply) => { + try { + return await establishSession( + fastify, + request, + reply, + request.body.auth, + request.body.ssoTokens, + ); + } catch (err) { + if (err instanceof PasswordChangeStillRequiredError) { + return reply.code(409).send({ error: "password_change_required" }); + } + throw err; + } + }, + ); + + await fastify.ready(); + return { fastify, redis }; +} + +describe("establishSession", () => { + it("persists a password-login session with no SSO fields in Redis", async () => { + const app = await buildEstablishSessionTestApp(); + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "upstream-jwt", // pragma: allowlist secret + expires_in: 1200, + user: { email: "user@example.com", is_admin: false }, + }, + }, + }); + + expect(response.statusCode).toBe(200); + const sessionId = response.cookies.find((c) => c.name === "bff_sid")?.value; + const stored = await getSession(app.redis, sessionId!); + + expect(stored?.refreshToken).toBeUndefined(); + expect(stored?.idToken).toBeUndefined(); + expect(stored?.tokenExpiresAt).toBeUndefined(); + const raw = await app.redis.get(sessionRedisKey(sessionId!)); + expect(Object.keys(JSON.parse(raw!) as object)).toEqual(["bearerToken", "user"]); + }); + + it("persists an SSO-shaped session with refreshToken/idToken/tokenExpiresAt in Redis", async () => { + const app = await buildEstablishSessionTestApp(); + const before = Math.floor(Date.now() / 1000); + + 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", is_admin: false, auth_provider: "sso" }, + }, + ssoTokens: { + refreshToken: "keycloak-refresh-token", // pragma: allowlist secret + idToken: "keycloak-id-token", // pragma: allowlist secret + }, + }, + }); + + expect(response.statusCode).toBe(200); + const sessionId = response.cookies.find((c) => c.name === "bff_sid")?.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. + expect(stored?.tokenExpiresAt).toBeGreaterThanOrEqual(before + 300); + expect(stored?.tokenExpiresAt).toBeLessThanOrEqual(before + 300 + 5); + }); + + it("uses the upstream token's own expires_in as the session TTL, not the BFF default", async () => { + const app = await buildEstablishSessionTestApp(); + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "upstream-jwt", // pragma: allowlist secret + expires_in: 1200, + user: { email: "user@example.com" }, + }, + }, + }); + + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie?.maxAge).toBe(1200); + }); + + it("falls back to the BFF default TTL when expires_in is absent or invalid", async () => { + const app = await buildEstablishSessionTestApp(); + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "upstream-jwt", // pragma: allowlist secret + expires_in: -5, + user: { email: "user@example.com" }, + }, + }, + }); + + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie?.maxAge).toBe(config.sessionTtlSeconds); + }); + + it("throws PasswordChangeStillRequiredError instead of minting a session", async () => { + const app = await buildEstablishSessionTestApp(); + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "upstream-jwt", // pragma: allowlist secret + user: { email: "user@example.com", password_change_required: true }, + }, + }, + }); + + expect(response.statusCode).toBe(409); + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie).toBeUndefined(); + }); + + it("rotates the CSRF secret even if the request already carries one", async () => { + const app = await buildEstablishSessionTestApp(); + const first = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { access_token: "upstream-jwt", user: { email: "user@example.com" } }, // pragma: allowlist secret + }, + }); + const firstCsrfToken = first.json().csrfToken as string; + const priorCookies = first.cookies.map((c) => `${c.name}=${c.value}`).join("; "); + + const second = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + headers: { cookie: priorCookies }, + payload: { + auth: { access_token: "upstream-jwt-2", user: { email: "user@example.com" } }, // pragma: allowlist secret + }, + }); + const secondCsrfToken = second.json().csrfToken as string; + + expect(secondCsrfToken).not.toBe(firstCsrfToken); + }); +}); diff --git a/server/test/lib/session-store.test.ts b/server/test/lib/session-store.test.ts new file mode 100644 index 00000000..6ac08e74 --- /dev/null +++ b/server/test/lib/session-store.test.ts @@ -0,0 +1,80 @@ +// Location: ./client/server/test/lib/session-store.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + createSession, + deleteSession, + getSession, + sessionRedisKey, + type SessionRecord, +} from "../../src/lib/session-store.js"; +import { FakeRedis } from "../helpers/build-app.js"; + +describe("createSession / getSession", () => { + it("round-trips a password-login session with no SSO fields", async () => { + const redis = new FakeRedis(); + const record: SessionRecord = { + bearerToken: "upstream-jwt", // pragma: allowlist secret + user: { email: "user@example.com", is_admin: false }, + }; + + const sessionId = await createSession(redis, record, 900); + const stored = await getSession(redis, sessionId); + + expect(stored).toEqual(record); + // Not just undefined on the JS object -- genuinely absent from what's + // persisted, since JSON has no way to represent `undefined` (JSON.stringify + // drops those keys entirely rather than writing them as null). + const raw = await redis.get(sessionRedisKey(sessionId)); + expect(Object.keys(JSON.parse(raw!) as object)).toEqual(["bearerToken", "user"]); + }); + + it("round-trips an SSO-shaped session (refreshToken/idToken/tokenExpiresAt) through Redis", async () => { + const redis = new FakeRedis(); + const record: SessionRecord = { + bearerToken: "keycloak-access-token", // pragma: allowlist secret + user: { email: "user@example.com", is_admin: false, auth_provider: "sso" }, + refreshToken: "keycloak-refresh-token", // pragma: allowlist secret + idToken: "keycloak-id-token", // pragma: allowlist secret + tokenExpiresAt: 1_700_000_000, + }; + + const sessionId = await createSession(redis, record, 900); + const stored = await getSession(redis, sessionId); + + expect(stored).toEqual(record); + }); + + it("returns null for a session that was never created", async () => { + const redis = new FakeRedis(); + + expect(await getSession(redis, "nonexistent-id")).toBeNull(); + }); + + it("returns null for a corrupted Redis value instead of throwing", async () => { + const redis = new FakeRedis(); + await redis.setex(sessionRedisKey("bad-id"), 900, "not json"); + + expect(await getSession(redis, "bad-id")).toBeNull(); + }); +}); + +describe("deleteSession", () => { + it("removes the session and publishes a revocation event", async () => { + const redis = new FakeRedis(); + const sessionId = await createSession( + redis, + { bearerToken: "t", user: { email: "user@example.com" } }, // pragma: allowlist secret + 900, + ); + + await deleteSession(redis, sessionId); + + expect(await getSession(redis, sessionId)).toBeNull(); + expect(redis.published).toHaveLength(1); + expect(redis.published[0]?.channel).toContain(sessionId); + }); +}); diff --git a/server/test/sso-callback.test.ts b/server/test/sso-callback.test.ts index 726b30e0..f24a6405 100644 --- a/server/test/sso-callback.test.ts +++ b/server/test/sso-callback.test.ts @@ -6,7 +6,9 @@ // the module registry and re-imports build-app.js fresh -- same pattern as // sso-login.test.ts. -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { generateKeyPairSync, sign as signBuffer, type KeyObject } from "node:crypto"; + +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { SSO_LOGIN_BINDING_COOKIE } from "../src/lib/sso-login-state.js"; import type { TestApp } from "./helpers/build-app.js"; @@ -20,8 +22,17 @@ const ENV_KEYS = [ "SSO_KEYCLOAK_CLIENT_SECRET", ] as const; -const TOKEN_ENDPOINT = - "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token"; +const ISSUER = "http://keycloak-internal:8080/realms/mcp-gateway"; +const TOKEN_ENDPOINT = `${ISSUER}/protocol/openid-connect/token`; +const JWKS_ENDPOINT = `${ISSUER}/protocol/openid-connect/certs`; +const CLIENT_ID = "contextforge-web-ui"; // matches enableSso()'s SSO_KEYCLOAK_CLIENT_ID +const KID = "test-kid"; + +let keyPair: { publicKey: KeyObject; privateKey: KeyObject }; + +beforeAll(() => { + keyPair = generateKeyPairSync("rsa", { modulusLength: 2048 }); +}); let savedEnv: Record; @@ -45,10 +56,29 @@ function enableSso(): void { process.env.SSO_KEYCLOAK_CLIENT_SECRET = "dev-secret"; // pragma: allowlist secret } -function makeIdToken(claims: Record): string { - const header = Buffer.from(JSON.stringify({ alg: "RS256" })).toString("base64url"); +function publicJwk(): Record { + return { ...keyPair.publicKey.export({ format: "jwk" }), kid: KID, use: "sig", alg: "RS256" }; +} + +// iss/aud/exp default to values verifySsoIdToken accepts, so call sites only +// need to override the claims their test actually cares about (email, nonce, ...). +function makeIdToken(claimOverrides: Record): string { + const claims = { + iss: ISSUER, + aud: CLIENT_ID, + exp: Math.floor(Date.now() / 1000) + 300, + ...claimOverrides, + }; + const header = Buffer.from(JSON.stringify({ alg: "RS256", typ: "JWT", kid: KID })).toString( + "base64url", + ); const payload = Buffer.from(JSON.stringify(claims)).toString("base64url"); - return `${header}.${payload}.signature`; + const signature = signBuffer( + "RSA-SHA256", + Buffer.from(`${header}.${payload}`), + keyPair.privateKey, + ); + return `${header}.${payload}.${signature.toString("base64url")}`; } function mockDiscoveryFetch(): void { @@ -58,23 +88,26 @@ function mockDiscoveryFetch(): void { ok: true, status: 200, json: async () => ({ - issuer: "http://keycloak-internal:8080/realms/mcp-gateway", - authorization_endpoint: - "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/auth", + issuer: ISSUER, + authorization_endpoint: `${ISSUER}/protocol/openid-connect/auth`, token_endpoint: TOKEN_ENDPOINT, - jwks_uri: "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + jwks_uri: JWKS_ENDPOINT, }), })), ); } // Discovery is cached after the login call, so this only needs to serve the -// token endpoint for the callback -- anything else is a test-setup bug. +// token and JWKS endpoints for the callback -- anything else is a test-setup bug. function mockTokenExchange(idTokenClaims: Record, ok = true, status = 400): void { vi.stubGlobal( "fetch", vi.fn(async (url: string) => { - if (!String(url).endsWith("/protocol/openid-connect/token")) { + const href = String(url); + if (href === JWKS_ENDPOINT) { + return { ok: true, status: 200, json: async () => ({ keys: [publicJwk()] }) }; + } + if (href !== TOKEN_ENDPOINT) { throw new Error(`unexpected fetch during callback test: ${url}`); } if (!ok) { @@ -127,7 +160,12 @@ describe("GET /auth/sso/callback", () => { const app = await buildTestApp(); const { state, nonce, binding } = await performLogin(app, "/app/tools"); - mockTokenExchange({ email: "user@example.com", name: "Test User", nonce }); + mockTokenExchange({ + email: "user@example.com", + email_verified: true, + name: "Test User", + nonce, + }); const response = await app.fastify.inject({ method: "GET", url: callbackUrl({ code: "auth-code", state }), @@ -157,7 +195,7 @@ describe("GET /auth/sso/callback", () => { const app = await buildTestApp(); const { state, nonce, binding } = await performLogin(app); - mockTokenExchange({ email: "user@example.com", nonce }); + mockTokenExchange({ email: "user@example.com", email_verified: true, nonce }); const response = await app.fastify.inject({ method: "GET", url: callbackUrl({ code: "auth-code", state }), @@ -175,7 +213,7 @@ describe("GET /auth/sso/callback", () => { const app = await buildTestApp(); const { state, nonce, binding } = await performLogin(app); - mockTokenExchange({ email: "user@example.com", nonce }); + mockTokenExchange({ email: "user@example.com", email_verified: true, nonce }); const headers = { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }; const first = await app.fastify.inject({ method: "GET", From b1f76e452b4b6a4000efc20bd45f2afac21fcf58 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Fri, 25 Sep 2026 15:19:06 +0100 Subject: [PATCH 3/4] Revoke key on logout Signed-off-by: Gabriel Costa --- server/src/lib/sso-back-channel-logout.ts | 49 +++++++++ server/src/lib/sso-user-resolution.ts | 13 ++- server/src/routes/auth/logout.ts | 12 ++- server/src/routes/auth/sso-callback.ts | 62 +++++++++--- server/test/auth.test.ts | 118 ++++++++++++++++++++++ server/test/sso-callback.test.ts | 64 ++++++++++++ 6 files changed, 298 insertions(+), 20 deletions(-) create mode 100644 server/src/lib/sso-back-channel-logout.ts diff --git a/server/src/lib/sso-back-channel-logout.ts b/server/src/lib/sso-back-channel-logout.ts new file mode 100644 index 00000000..8dcba436 --- /dev/null +++ b/server/src/lib/sso-back-channel-logout.ts @@ -0,0 +1,49 @@ +// Location: ./client/server/src/lib/sso-back-channel-logout.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Best-effort RP-Initiated Logout (OIDC) against Keycloak's +// end_session_endpoint, so logging out of the BFF also invalidates the +// Keycloak refresh_token/IdP session an SSO session's tokens came from. +// Mirrors revoke-upstream-token.ts's "must not block the response" +// convention. Used by routes/auth/logout.ts. + +import type { FastifyRequest } from "fastify"; + +import { getDiscoveryDocument } from "./oidc-discovery.js"; + +// Caller (the logout response) is waiting on this -- cap how long a hung +// (not refused) Keycloak can hold it open. +const SSO_BACK_CHANNEL_LOGOUT_TIMEOUT_MS = 3000; + +export async function backChannelLogoutSso( + request: FastifyRequest, + idToken: string, +): Promise { + let endSessionEndpoint: string | undefined; + try { + ({ endSessionEndpoint } = await getDiscoveryDocument()); + } catch (err) { + request.log.warn({ err }, "SSO back-channel logout: discovery failed"); + return; + } + // Not every realm/provider advertises one (see oidc-discovery.ts) -- nothing to call. + if (!endSessionEndpoint) return; + + const url = new URL(endSessionEndpoint); + url.searchParams.set("id_token_hint", idToken); + + try { + const response = await fetch(url, { + signal: AbortSignal.timeout(SSO_BACK_CHANNEL_LOGOUT_TIMEOUT_MS), + }); + if (!response.ok) { + request.log.warn( + { status: response.status }, + "SSO back-channel logout returned a non-2xx status", + ); + } + } catch (err) { + request.log.warn({ err }, "SSO back-channel logout failed"); + } +} diff --git a/server/src/lib/sso-user-resolution.ts b/server/src/lib/sso-user-resolution.ts index 40dcffce..f934c644 100644 --- a/server/src/lib/sso-user-resolution.ts +++ b/server/src/lib/sso-user-resolution.ts @@ -34,9 +34,16 @@ export interface SsoIdTokenClaims { } export class SsoIdTokenError extends Error { - constructor(message: string, options?: { cause?: unknown }) { + // Set only where the callback route needs to react differently by cause + // (see resolveSsoUser) -- undefined for the generic parse/verify failures. + readonly code?: "email_missing" | "email_unverified"; + constructor( + message: string, + options?: { cause?: unknown; code?: "email_missing" | "email_unverified" }, + ) { super(message, options); this.name = "SsoIdTokenError"; + this.code = options?.code; } } @@ -216,13 +223,13 @@ export async function verifySsoIdToken(idToken: string): Promise { }); expect(followUp.json()).toEqual({ authenticated: false, ssoEnabled: false }); }); + + it("does not attempt a Keycloak back-channel logout for a password-login session", async () => { + const app = await buildTestApp(); + const { cookies, csrfToken } = await login(app); + + const fetchCalls: string[] = []; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + fetchCalls.push(String(url)); + return { ok: true, status: 200, json: async () => ({}), text: async () => "" }; + }), + ); + + await app.fastify.inject({ + method: "POST", + url: "/auth/logout", + headers: { cookie: cookies.join("; "), "x-csrf-token": csrfToken }, + }); + + // Only the upstream revoke call -- a password-login session never has an + // idToken, so there's nothing for a Keycloak back-channel logout to do. + expect(fetchCalls).toEqual([`${config.contextforgeUrl}/auth/logout`]); + }); + + it("revokes the Keycloak SSO session via back-channel logout when the session carries an idToken", async () => { + await withSsoEnabled(async () => { + const { buildTestApp: freshBuildTestApp } = await import("./helpers/build-app.js"); + const { getSession, sessionRedisKey } = await import("../src/lib/session-store.js"); + const app = await freshBuildTestApp(); + const { cookies, csrfToken } = await login(app); + const sessionId = cookies.find((c) => c.startsWith("bff_sid="))!.slice("bff_sid=".length); + + // Reshape the just-created password-login session into an SSO-shaped + // one in place -- this test is about logout's own behavior given an + // idToken, not about re-running the whole SSO login round trip. + const record = await getSession(app.redis, sessionId); + await app.redis.setex( + sessionRedisKey(sessionId), + 900, + JSON.stringify({ ...record, idToken: "keycloak-id-token" }), // pragma: allowlist secret + ); + + const fetchCalls: string[] = []; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + fetchCalls.push(href); + if (href.startsWith("http://keycloak-internal:8080/realms/mcp-gateway/.well-known")) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: "http://keycloak-internal:8080/realms/mcp-gateway", + authorization_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/auth", + token_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token", + jwks_uri: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + end_session_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/logout", + }), + }; + } + return { ok: true, status: 200, json: async () => ({}), text: async () => "" }; + }), + ); + + const response = await app.fastify.inject({ + method: "POST", + url: "/auth/logout", + headers: { cookie: cookies.join("; "), "x-csrf-token": csrfToken }, + }); + + expect(response.statusCode).toBe(200); + const endSessionCall = fetchCalls.find((url) => + url.includes("/protocol/openid-connect/logout"), + ); + expect(endSessionCall).toBeTruthy(); + expect(new URL(endSessionCall!).searchParams.get("id_token_hint")).toBe("keycloak-id-token"); + }); + }); + + it("still responds and clears the BFF session when the Keycloak back-channel logout call fails", async () => { + await withSsoEnabled(async () => { + const { buildTestApp: freshBuildTestApp } = await import("./helpers/build-app.js"); + const { getSession, sessionRedisKey } = await import("../src/lib/session-store.js"); + const app = await freshBuildTestApp(); + const { cookies, csrfToken } = await login(app); + const sessionId = cookies.find((c) => c.startsWith("bff_sid="))!.slice("bff_sid=".length); + + const record = await getSession(app.redis, sessionId); + await app.redis.setex( + sessionRedisKey(sessionId), + 900, + JSON.stringify({ ...record, idToken: "keycloak-id-token" }), // pragma: allowlist secret + ); + + vi.stubGlobal( + "fetch", + vi.fn(async () => { + throw new Error("keycloak unreachable"); + }), + ); + + const response = await app.fastify.inject({ + method: "POST", + url: "/auth/logout", + headers: { cookie: cookies.join("; "), "x-csrf-token": csrfToken }, + }); + + expect(response.statusCode).toBe(200); + const cleared = response.cookies.find((c) => c.name === "bff_sid"); + expect(cleared?.value).toBe(""); + }); + }); }); diff --git a/server/test/sso-callback.test.ts b/server/test/sso-callback.test.ts index f24a6405..547fef34 100644 --- a/server/test/sso-callback.test.ts +++ b/server/test/sso-callback.test.ts @@ -246,6 +246,51 @@ describe("GET /auth/sso/callback", () => { expect(response.cookies.find((c) => c.name === "bff_sid")).toBeUndefined(); }); + it("consumes the state on Keycloak's own error redirect too, not just on success", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, binding } = await performLogin(app); + const headers = { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }; + + const errorResponse = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ error: "access_denied", state }), + headers, + }); + expect(errorResponse.headers.location).toBe("/app/login?error=sso_access_denied"); + + // RFC 6749 has Keycloak echo `state` on the error redirect too -- it must + // be burned there, not left live for the full SSO_LOGIN_STATE_TTL_SECONDS + // window. Replaying it afterwards must fail. + const replay = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers, + }); + expect(replay.headers.location).toBe("/app/login?error=sso_state_invalid"); + }); + + it("preserves the caller's next destination on a mid-flow failure redirect", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, binding } = await performLogin(app, "/app/tools"); + + mockTokenExchange({}, false); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe( + "/app/login?error=sso_token_exchange_failed&next=%2Fapp%2Ftools", + ); + }); + it("redirects with an error when code or state is missing", async () => { enableSso(); const buildTestApp = await freshBuildTestApp(); @@ -379,6 +424,25 @@ describe("GET /auth/sso/callback", () => { expect(response.headers.location).toBe("/app/login?error=sso_email_missing"); }); + it("redirects with a distinct error when the email claim is present but unverified", async () => { + enableSso(); + mockDiscoveryFetch(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + const { state, nonce, binding } = await performLogin(app); + + // Distinct from a missing email claim -- this is the account-takeover- + // prevention case (see resolveSsoUser), and should be triageable as such. + mockTokenExchange({ email: "user@example.com", email_verified: false, nonce }); + const response = await app.fastify.inject({ + method: "GET", + url: callbackUrl({ code: "auth-code", state }), + headers: { cookie: `${SSO_LOGIN_BINDING_COOKIE}=${binding}` }, + }); + + expect(response.headers.location).toBe("/app/login?error=sso_email_unverified"); + }); + it("redirects with an error when SSO is disabled", async () => { delete process.env.SSO_ENABLED; const buildTestApp = await freshBuildTestApp(); From e93fe8c5e5271fcf1ebf04876fe12040677c8b72 Mon Sep 17 00:00:00 2001 From: Gabriel Costa Date: Mon, 28 Sep 2026 10:33:35 +0100 Subject: [PATCH 4/4] fix: SSO callback/logout review findings Fixes four issues found in review of the SSO callback route and logout flow: - sso-back-channel-logout.ts: new URL(endSessionEndpoint) could throw synchronously outside any try/catch, breaking logout's "must not block the response" invariant for the full 1h discovery-cache window whenever a provider's end_session_endpoint isn't a well-formed URL. Now caught and logged like every other failure in that function. - establish-session.ts: tokenExpiresAt was computed from ttlSeconds' BFF-default fallback when Keycloak's expires_in was missing/invalid, falsely marking an SSO access token valid for the whole fallback window. Split into a separate ssoTokenTtlSeconds that defaults to 0 (already-expired) instead of borrowing the session TTL's fallback. - sso-login.ts and sso-callback.ts each had their own copy of loginErrorRedirect(), with the arguments in opposite order. Consolidated into lib/sso-login-error-redirect.ts, one signature, one implementation. - sso-callback.ts: a repeated ?error= query param parses as an array, which firstString() silently treated as absent, letting the request fall through as if Keycloak hadn't reported an error. Now checked by key presence first. Signed-off-by: Gabriel Costa --- server/src/lib/establish-session.ts | 6 ++- server/src/lib/sso-back-channel-logout.ts | 11 +++- server/src/lib/sso-login-error-redirect.ts | 22 ++++++++ server/src/routes/auth/sso-callback.ts | 27 +++------- server/src/routes/auth/sso-login.ts | 35 +++++------- server/test/auth.test.ts | 53 +++++++++++++++++++ server/test/lib/establish-session.test.ts | 33 ++++++++++++ .../test/lib/sso-login-error-redirect.test.ts | 37 +++++++++++++ server/test/sso-callback.test.ts | 17 ++++++ 9 files changed, 199 insertions(+), 42 deletions(-) create mode 100644 server/src/lib/sso-login-error-redirect.ts create mode 100644 server/test/lib/sso-login-error-redirect.test.ts diff --git a/server/src/lib/establish-session.ts b/server/src/lib/establish-session.ts index 9eda2f31..177058fd 100644 --- a/server/src/lib/establish-session.ts +++ b/server/src/lib/establish-session.ts @@ -62,8 +62,12 @@ export async function establishSession( // 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 @@ -83,7 +87,7 @@ export async function establishSession( user: auth.user, refreshToken: ssoTokens?.refreshToken, idToken: ssoTokens?.idToken, - tokenExpiresAt: ssoTokens ? Math.floor(Date.now() / 1000) + ttlSeconds : undefined, + tokenExpiresAt: ssoTokens ? Math.floor(Date.now() / 1000) + ssoTokenTtlSeconds : undefined, }, ttlSeconds, ); diff --git a/server/src/lib/sso-back-channel-logout.ts b/server/src/lib/sso-back-channel-logout.ts index 8dcba436..f93d2261 100644 --- a/server/src/lib/sso-back-channel-logout.ts +++ b/server/src/lib/sso-back-channel-logout.ts @@ -30,7 +30,16 @@ export async function backChannelLogoutSso( // Not every realm/provider advertises one (see oidc-discovery.ts) -- nothing to call. if (!endSessionEndpoint) return; - const url = new URL(endSessionEndpoint); + // oidc-discovery.ts only checks this is a non-empty string, not a + // well-formed URL -- a malformed value from a misconfigured provider must + // not throw out of this function (see the file header). + let url: URL; + try { + url = new URL(endSessionEndpoint); + } catch (err) { + request.log.warn({ err }, "SSO back-channel logout: malformed end_session_endpoint"); + return; + } url.searchParams.set("id_token_hint", idToken); try { diff --git a/server/src/lib/sso-login-error-redirect.ts b/server/src/lib/sso-login-error-redirect.ts new file mode 100644 index 00000000..7776c6af --- /dev/null +++ b/server/src/lib/sso-login-error-redirect.ts @@ -0,0 +1,22 @@ +// Location: ./client/server/src/lib/sso-login-error-redirect.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 +// +// Shared by routes/auth/sso-login.ts and routes/auth/sso-callback.ts -- both +// redirect every SSO failure back to the login page with a `sso_`-prefixed +// error code, preserving the caller's destination when known. One shared +// implementation, one argument order -- two near-identical copies previously +// existed with the arguments swapped between them. + +const APP_PREFIX = "/app"; +export const SSO_DEFAULT_RETURN_TO = `${APP_PREFIX}/`; +export const SSO_LOGIN_PATH = `${APP_PREFIX}/login`; + +// Keycloak's own error codes (access_denied, ...) are short RFC 6749 tokens; +// URLSearchParams encodes whatever we're given either way. +export function loginErrorRedirect(code: string, returnTo?: string): string { + const url = new URL(SSO_LOGIN_PATH, "http://placeholder"); + url.searchParams.set("error", `sso_${code}`); + if (returnTo && returnTo !== SSO_DEFAULT_RETURN_TO) url.searchParams.set("next", returnTo); + return url.pathname + url.search; +} diff --git a/server/src/routes/auth/sso-callback.ts b/server/src/routes/auth/sso-callback.ts index 30ac3f3c..dd30e97c 100644 --- a/server/src/routes/auth/sso-callback.ts +++ b/server/src/routes/auth/sso-callback.ts @@ -12,6 +12,7 @@ import { config } from "../../config.js"; import { establishSession, PasswordChangeStillRequiredError } from "../../lib/establish-session.js"; import { setNoStore } from "../../lib/no-store.js"; import { getDiscoveryDocument } from "../../lib/oidc-discovery.js"; +import { loginErrorRedirect } from "../../lib/sso-login-error-redirect.js"; import { consumeSsoLoginState, SSO_LOGIN_BINDING_COOKIE } from "../../lib/sso-login-state.js"; import { exchangeSsoCode } from "../../lib/sso-token-exchange.js"; import { @@ -20,10 +21,6 @@ import { verifySsoIdToken, } from "../../lib/sso-user-resolution.js"; -const APP_PREFIX = "/app"; -const DEFAULT_RETURN_TO = `${APP_PREFIX}/`; -const LOGIN_PATH = `${APP_PREFIX}/login`; - interface SsoCallbackQuerystring { code?: string | string[]; state?: string | string[]; @@ -34,18 +31,6 @@ function firstString(value: string | string[] | undefined): string | undefined { return typeof value === "string" && value ? value : undefined; } -// Keycloak's own error codes (access_denied, ...) are short RFC 6749 tokens; -// URLSearchParams encodes whatever we're given either way. Carries the -// caller's original destination back through the error redirect (when -// known) so a retry from the login page doesn't lose it -- same pattern as -// sso-login.ts's own loginErrorRedirect. -function loginErrorRedirect(code: string, returnTo?: string): string { - const url = new URL(LOGIN_PATH, "http://placeholder"); - url.searchParams.set("error", `sso_${code}`); - if (returnTo && returnTo !== DEFAULT_RETURN_TO) url.searchParams.set("next", returnTo); - return url.pathname + url.search; -} - export default async function ssoCallbackRoute(fastify: FastifyInstance): Promise { fastify.get<{ Querystring: SsoCallbackQuerystring }>( "/auth/sso/callback", @@ -70,9 +55,13 @@ export default async function ssoCallbackRoute(fastify: FastifyInstance): Promis // live in Redis for the full SSO_LOGIN_STATE_TTL_SECONDS window. const loginState = state ? await consumeSsoLoginState(fastify.redis, state, binding) : null; - const errorParam = firstString(request.query.error); - if (errorParam) { - return reply.redirect(loginErrorRedirect(errorParam, loginState?.returnTo)); + // Presence of the key at all is treated as an error, not just a clean + // single value -- a repeated ?error=a&error=b parses as an array, and + // firstString() would otherwise silently drop it, falling through as + // if Keycloak hadn't reported an error at all. + if (request.query.error !== undefined) { + const errorCode = firstString(request.query.error) ?? "callback_invalid"; + return reply.redirect(loginErrorRedirect(errorCode, loginState?.returnTo)); } const code = firstString(request.query.code); diff --git a/server/src/routes/auth/sso-login.ts b/server/src/routes/auth/sso-login.ts index 41cf201e..fdfa7962 100644 --- a/server/src/routes/auth/sso-login.ts +++ b/server/src/routes/auth/sso-login.ts @@ -12,11 +12,14 @@ import { config } from "../../config.js"; import { setNoStore } from "../../lib/no-store.js"; import { getDiscoveryDocument } from "../../lib/oidc-discovery.js"; import { isForbiddenCrossOrigin, resolvePublicOrigin } from "../../lib/origin-guard.js"; +import { + loginErrorRedirect, + SSO_DEFAULT_RETURN_TO, + SSO_LOGIN_PATH, +} from "../../lib/sso-login-error-redirect.js"; import { mintSsoLoginState, SSO_LOGIN_BINDING_COOKIE } from "../../lib/sso-login-state.js"; const APP_PREFIX = "/app"; -const DEFAULT_RETURN_TO = `${APP_PREFIX}/`; -const LOGIN_ERROR_REDIRECT = `${APP_PREFIX}/login`; interface SsoLoginQuerystring { next?: string | string[]; @@ -26,32 +29,22 @@ interface SsoLoginQuerystring { // across the package boundary, so reimplemented (vectors shared in tests). export function safeReturnTo(next: string | string[] | undefined): string { // Fastify turns a repeated ?next=a&next=b into an array; treat non-string as absent. - if (typeof next !== "string" || !next) return DEFAULT_RETURN_TO; - if (/[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(next)) return DEFAULT_RETURN_TO; - if (next.startsWith("//")) return DEFAULT_RETURN_TO; + if (typeof next !== "string" || !next) return SSO_DEFAULT_RETURN_TO; + if (/[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(next)) return SSO_DEFAULT_RETURN_TO; + if (next.startsWith("//")) return SSO_DEFAULT_RETURN_TO; const [pathname = "", queryString] = next.split("?"); - if (pathname.includes("..")) return DEFAULT_RETURN_TO; + if (pathname.includes("..")) return SSO_DEFAULT_RETURN_TO; const isAppPath = pathname === APP_PREFIX || pathname.startsWith(`${APP_PREFIX}/`); - if (!isAppPath) return DEFAULT_RETURN_TO; + if (!isAppPath) return SSO_DEFAULT_RETURN_TO; // Mirrors resolveNextParam: never bounce the post-login redirect back to // the login page itself. - if (pathname === LOGIN_ERROR_REDIRECT) return DEFAULT_RETURN_TO; + if (pathname === SSO_LOGIN_PATH) return SSO_DEFAULT_RETURN_TO; return queryString ? `${pathname}?${queryString}` : pathname; } -// Preserves the caller's destination across a failure redirect so a retry -// doesn't lose it (resolveNextParam re-validates on read, so round-tripping -// it through the login page's own `next` param is safe). -function loginErrorRedirect(returnTo: string, code: string): string { - const url = new URL(LOGIN_ERROR_REDIRECT, "http://placeholder"); - url.searchParams.set("error", `sso_${code}`); - if (returnTo !== DEFAULT_RETURN_TO) url.searchParams.set("next", returnTo); - return url.pathname + url.search; -} - export default async function ssoLoginRoute(fastify: FastifyInstance): Promise { fastify.get<{ Querystring: SsoLoginQuerystring }>( "/auth/sso/login", @@ -64,14 +57,14 @@ export default async function ssoLoginRoute(fastify: FastifyInstance): Promise { expect(cleared?.value).toBe(""); }); }); + + it("still responds and clears the BFF session when discovery returns a malformed end_session_endpoint", async () => { + await withSsoEnabled(async () => { + const { buildTestApp: freshBuildTestApp } = await import("./helpers/build-app.js"); + const { getSession, sessionRedisKey } = await import("../src/lib/session-store.js"); + const app = await freshBuildTestApp(); + const { cookies, csrfToken } = await login(app); + const sessionId = cookies.find((c) => c.startsWith("bff_sid="))!.slice("bff_sid=".length); + + const record = await getSession(app.redis, sessionId); + await app.redis.setex( + sessionRedisKey(sessionId), + 900, + JSON.stringify({ ...record, idToken: "keycloak-id-token" }), // pragma: allowlist secret + ); + + vi.stubGlobal( + "fetch", + vi.fn(async (url: string) => { + const href = String(url); + if (href.startsWith("http://keycloak-internal:8080/realms/mcp-gateway/.well-known")) { + return { + ok: true, + status: 200, + json: async () => ({ + issuer: "http://keycloak-internal:8080/realms/mcp-gateway", + authorization_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/auth", + token_endpoint: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/token", + jwks_uri: + "http://keycloak-internal:8080/realms/mcp-gateway/protocol/openid-connect/certs", + // oidc-discovery.ts only checks this is a non-empty string, not + // that it's a well-formed URL -- exercises that gap directly. + end_session_endpoint: "not a valid url", + }), + }; + } + return { ok: true, status: 200, json: async () => ({}), text: async () => "" }; + }), + ); + + const response = await app.fastify.inject({ + method: "POST", + url: "/auth/logout", + headers: { cookie: cookies.join("; "), "x-csrf-token": csrfToken }, + }); + + expect(response.statusCode).toBe(200); + const cleared = response.cookies.find((c) => c.name === "bff_sid"); + expect(cleared?.value).toBe(""); + }); + }); }); diff --git a/server/test/lib/establish-session.test.ts b/server/test/lib/establish-session.test.ts index da6d110e..2badf8bf 100644 --- a/server/test/lib/establish-session.test.ts +++ b/server/test/lib/establish-session.test.ts @@ -120,6 +120,39 @@ describe("establishSession", () => { expect(stored?.tokenExpiresAt).toBeLessThanOrEqual(before + 300 + 5); }); + 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); + + const response = await app.fastify.inject({ + method: "POST", + url: "/test/establish-session", + payload: { + auth: { + access_token: "keycloak-access-token", // pragma: allowlist secret + expires_in: -5, // invalid -- session TTL falls back to the BFF default + user: { email: "user@example.com", auth_provider: "sso" }, + }, + ssoTokens: { + refreshToken: "keycloak-refresh-token", // pragma: allowlist secret + idToken: "keycloak-id-token", // pragma: allowlist secret + }, + }, + }); + + expect(response.statusCode).toBe(200); + // The session/cookie itself is fine living out the BFF default (that's + // ttlSeconds' own fallback) -- but tokenExpiresAt must never borrow that + // same fallback and claim a Keycloak access token is valid for hours + // when its real lifetime is unknown. A refresh-aware consumer reading + // tokenExpiresAt needs it to read as already-expired, not BFF-default-valid. + const sessionCookie = response.cookies.find((c) => c.name === "bff_sid"); + expect(sessionCookie?.maxAge).toBe(config.sessionTtlSeconds); + const sessionId = sessionCookie?.value; + const stored = await getSession(app.redis, sessionId!); + expect(stored?.tokenExpiresAt).toBeLessThanOrEqual(before); + }); + it("uses the upstream token's own expires_in as the session TTL, not the BFF default", async () => { const app = await buildEstablishSessionTestApp(); const response = await app.fastify.inject({ diff --git a/server/test/lib/sso-login-error-redirect.test.ts b/server/test/lib/sso-login-error-redirect.test.ts new file mode 100644 index 00000000..0157e16d --- /dev/null +++ b/server/test/lib/sso-login-error-redirect.test.ts @@ -0,0 +1,37 @@ +// Location: ./client/server/test/lib/sso-login-error-redirect.test.ts +// Copyright contributors to the MCP-CONTEXT-FORGE project +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + loginErrorRedirect, + SSO_DEFAULT_RETURN_TO, + SSO_LOGIN_PATH, +} from "../../src/lib/sso-login-error-redirect.js"; + +describe("loginErrorRedirect", () => { + it("prefixes the error code with sso_ and omits next when no returnTo is given", () => { + expect(loginErrorRedirect("disabled")).toBe(`${SSO_LOGIN_PATH}?error=sso_disabled`); + }); + + it("omits next when returnTo is the default return path", () => { + expect(loginErrorRedirect("disabled", SSO_DEFAULT_RETURN_TO)).toBe( + `${SSO_LOGIN_PATH}?error=sso_disabled`, + ); + }); + + it("appends next when returnTo is a real destination", () => { + expect(loginErrorRedirect("token_exchange_failed", "/app/tools")).toBe( + `${SSO_LOGIN_PATH}?error=sso_token_exchange_failed&next=%2Fapp%2Ftools`, + ); + }); + + it("code is always the first argument -- this is the one shared implementation both routes call", () => { + // Regression guard for the bug this module fixed: sso-login.ts and + // sso-callback.ts each had their own copy with the arguments swapped. + const result = loginErrorRedirect("state_invalid", "/app/tools"); + expect(result).toContain("error=sso_state_invalid"); + expect(result).not.toContain("error=%2Fapp%2Ftools"); + }); +}); diff --git a/server/test/sso-callback.test.ts b/server/test/sso-callback.test.ts index 547fef34..43f84b57 100644 --- a/server/test/sso-callback.test.ts +++ b/server/test/sso-callback.test.ts @@ -291,6 +291,23 @@ describe("GET /auth/sso/callback", () => { ); }); + it("treats a repeated ?error= param as an error, not as absent", async () => { + enableSso(); + const buildTestApp = await freshBuildTestApp(); + const app = await buildTestApp(); + + const response = await app.fastify.inject({ + method: "GET", + // Fastify parses a repeated query key as an array -- firstString() + // alone would silently drop it, falling through as if Keycloak hadn't + // reported an error. + url: "/auth/sso/callback?error=access_denied&error=consent_required", + }); + + expect(response.headers.location).toBe("/app/login?error=sso_callback_invalid"); + expect(response.cookies.find((c) => c.name === "bff_sid")).toBeUndefined(); + }); + it("redirects with an error when code or state is missing", async () => { enableSso(); const buildTestApp = await freshBuildTestApp();