feat: SSO callback route and session wiring

[gcgoncalves]

Closes IBM/mcp-context-forge#6916
Closes IBM/mcp-context-forge#6920 (Added as a code review request, revoking SSO token on logout)

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

[marekdano]

1. 🔴 High — Logout doesn't revoke Keycloak SSO session

File: server/src/routes/auth/logout.ts:46

Only the BFF bearer token is revoked; the Keycloak refresh_token/id_token this PR starts storing in SessionRecord are never revoked, and Keycloak's revoke/end_session endpoint is never called.

Failure scenario: A user logs in via SSO (session now carries refreshToken/idToken per establish-session.ts's SsoTokens param) and clicks Logout. The BFF session is deleted, but the Keycloak refresh token stays valid and the IdP session stays live — re-visiting /auth/sso/login silently re-authenticates the user.

---

2. 🟠 Medium-High — Error path skips single-use state invalidation

File: server/src/routes/auth/sso-callback.ts:56

The error query-param branch returns before consumeSsoLoginState() is called.

Failure scenario: Keycloak redirects back with ?error=access_denied&state=<S> (per RFC 6749, state is echoed on error responses). The handler returns early without invalidating the state, leaving the Redis entry (and its bound codeVerifier/nonce/returnTo) live for the full SSO_LOGIN_STATE_TTL_SECONDS window instead of single-use as documented.

---

3. 🟡 Medium — Callback error redirect loses the next deep link

File: server/src/routes/auth/sso-callback.ts:34

sso-login.ts has a near-identical helper that preserves returnTo via a next param; this callback's copy drops it.

Failure scenario: User requests /auth/sso/login?next=/app/tools, stored in loginState.returnTo. If the token exchange or nonce check later fails in the callback, loginErrorRedirect sends them to bare /app/login instead of back to /app/tools.

---

4. 🟡 Medium — Missing-email vs unverified-email errors conflated

File: server/src/routes/auth/sso-callback.ts:118

resolveSsoUser() distinguishes "no email claim" from "email present but unverified" (the latter is the account-takeover-prevention case its own comment flags), but both are logged as "missing required claims" and mapped to the same sso_email_missing code.

Failure scenario: A Keycloak account with email_verified=false fails for a security-sensitive reason, but logs/error codes make it indistinguishable from a genuinely absent email claim — harder to triage.

[gcgoncalves]

As logout not revokes the token, this task also closes IBM/mcp-context-forge#6920.

[marekdano]

Findings

1. 🔴 High — Unhandled throw in back-channel logout breaks the BFF logout invariant

File: server/src/lib/sso-back-channel-logout.ts:33

new URL(endSessionEndpoint) is called outside any try/catch. If the discovery document returns an end_session_endpoint that isn't URL-parseable — e.g. a relative path from a misconfigured/non-standard OIDC provider, since SSO_KEYCLOAK_PUBLIC_BASE_URL being unset means oidc-discovery.ts never validates it as a URL, only checks it's a non-empty string — the function throws synchronously and the promise rejects uncaught.

Failure scenario: logout.ts's await Promise.all([...]) re-throws that rejection with no surrounding try/catch, so the route handler throws before clearSessionCookie/reply.clearCookie/reply.send({ok:true}) run. The user gets a 500 on logout and their cookies are never cleared client-side, even though the Redis session was already deleted. Since the discovery document is cached for 1h, every SSO logout fails the same way for that whole window — directly violating the file's own documented "must not block the BFF-side logout" invariant.

---

2. 🟠 Medium — tokenExpiresAt silently falls back to the wrong TTL

File: server/src/lib/establish-session.ts:86

When Keycloak's token response omits expires_in (or sends an invalid value), ttlSeconds silently falls back to the BFF's own default session TTL rather than the real token lifetime, and tokenExpiresAt = now + ttlSeconds is stored as if it reflected the actual access token's expiry.

Failure scenario: Any future consumer of tokenExpiresAt — e.g. a refresh scheduler, which appears to be exactly what this field exists to support — would treat an already-expired Keycloak access token as valid for the whole fallback window.

---

3. 🟡 Medium — Duplicated loginErrorRedirect helper with swapped argument order

File: server/src/routes/auth/sso-callback.ts:42

This file defines its own loginErrorRedirect(code, returnTo), while sso-login.ts has a near-identical helper of the same name but with the arguments reversed: loginErrorRedirect(returnTo, code).

Failure scenario: A future edit copies a call from one file to the other without noticing the swapped signature, silently writing error=<path>&next=sso_<code> into the redirect URL instead of error=sso_<code>&next=<path>, breaking the login error UX without any type error (both params are string).

---

4. 🟢 Low — Repeated ?error= query param silently treated as "no error"

File: server/src/routes/auth/sso-callback.ts:73

firstString(request.query.error) only accepts a plain string. Fastify parses a repeated query param (?error=a&error=b) as an array, so firstString returns undefined for it.

Failure scenario: A crafted or double-encoded callback URL with error repeated causes errorParam to be undefined, so the explicit Keycloak-error branch is skipped and the request falls through to the generic callback_invalid/state_invalid paths, losing the specific error code that would help diagnose the failure.

[gcgoncalves]

@marekdano Updated

[marekdano]

Findings

1. 🔴 High — Unhandled throw in back-channel logout still breaks the BFF logout invariant

File: server/src/lib/sso-back-channel-logout.ts:33

new URL(endSessionEndpoint) is still called outside any try/catch. If the discovery document returns an end_session_endpoint that isn't URL-parseable, the function throws synchronously and the promise rejects uncaught.

Failure scenario: logout.ts's await Promise.all([...]) has no surrounding try/catch either, so the rejection propagates and the route handler throws before clearSessionCookie/reply.clearCookie/reply.send({ok:true}) run. The user gets a 500 on logout and their cookies are never cleared client-side, even though the Redis session was already deleted. Since the discovery document is cached for 1h, every SSO logout would fail the same way for that whole window. This was flagged in the previous review round and does not appear to have been addressed by the latest commit.

---

2. 🟡 Medium — Duplicated loginErrorRedirect helper still has swapped argument order

File: server/src/routes/auth/sso-callback.ts:42 vs server/src/routes/auth/sso-login.ts:46

sso-login.ts defines loginErrorRedirect(returnTo, code); sso-callback.ts defines its own near-identical helper with the arguments reversed: loginErrorRedirect(code, returnTo).

Failure scenario: A future edit copies a call from one file to the other without noticing the swapped signature, silently writing error=<path>&next=sso_<code> into the redirect URL instead of error=sso_<code>&next=<path>, breaking the login error UX without any type error (both params are string). Still present as of the latest commit.

---

3. 🟢 Low — Repeated ?error= query param still silently treated as "no error"

File: server/src/routes/auth/sso-callback.ts:33

firstString(request.query.error) only accepts a plain string. Fastify parses a repeated query param (?error=a&error=b) as an array, so firstString returns undefined for it.

Failure scenario: A crafted or double-encoded callback URL with error repeated causes errorParam to be undefined, so the explicit Keycloak-error branch is skipped and the request falls through to the generic callback_invalid/state_invalid paths, losing the specific error code that would help diagnose the failure. Still present as of the latest commit.

[gcgoncalves]

@marekdano Have you pulled the latest changes? I've addressed these three issues in my latest commit.

• item 1: on sso-back-channel-logout.ts:36-42, the new URL (endSessionEndpoint) is wrapped in its own try/catch (logs + returns on failure).
• item 2: sso-callback.ts:15 imports loginErrorRedirect from the shared lib/sso-login-error-redirect.js. sso-login.ts imports the same one.
• item 3: sso-callback.ts:62-65 checks request.query.error !== undefined (catches the array-from-repeated-param case) before falling back to firstString().

[marekdano]

Checked the latest commit.

All three findings from the last round are fixed in e93fe8c, verified against the diff and the test suite:

1. sso-back-channel-logout.ts:33 — new URL(endSessionEndpoint) now has its own try/catch, logs, and returns instead of throwing.
2. sso-callback.ts / sso-login.ts — loginErrorRedirect is consolidated into one shared implementation with one argument order.
3. sso-callback.ts:33 — a repeated ?error= param is now caught by key presence, not just firstString().

The establish-session.ts tokenExpiresAt fallback fix is a good catch too — it now reads as already-expired instead of borrowing the BFF session-TTL default when expires_in is missing or invalid.

New tests cover all four cases, and the full affected suite (65 tests) passes.

LGTM 🚀