feat: wire SSO token refresh into sessionAuth

[gcgoncalves]

Closes IBM/mcp-context-forge#6919

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.

[marekdano]

Findings

🔴 Critical

1. Cookie Max-Age never extended on refresh

Location: server/src/plugins/session.ts:47

Issue: refreshRecord() updates the Redis-backed session with new tokens but never re-issues the bff_sid cookie. The cookie's Max-Age is set once at login (sso-callback.ts, from Keycloak's expires_in — typically a short access-token TTL of minutes) and is never refreshed.

Failure scenario: Once the original cookie expiry passes, the browser silently drops bff_sid even though Redis has been kept fresh by successful server-side refreshes the whole time. The next request has no cookie at all → hard 401 unauthenticated → forced full re-login.

Impact: Defeats the entire purpose of the PR's transparent-refresh feature for any session that outlives its first access-token TTL.

🟠 High

2. No locking around concurrent refresh attempts

Location: server/src/plugins/session.ts:86

Issue: sessionAuth runs as a preHandler on every proxied API call and SSE connection. No de-duplication exists for simultaneous refresh attempts on the same session.

Failure scenario: Several requests fire near expiry at once; each independently calls refreshSsoSession() with the same refresh token. If Keycloak rotates/single-uses refresh tokens (a common OIDC security profile), only the first succeeds — the rest get invalid_grant and 401 with session_expired, even though another concurrent request just refreshed successfully.

🟡 Medium

3. Transient refresh failure treated as permanent revocation

Location: server/src/plugins/session.ts:67

Issue: Any error during refresh (network blip, discovery-fetch timeout) is handled identically to a dead/revoked refresh token.

Failure scenario: A momentary Keycloak hiccup during refresh forces a hard 401 session_expired, even though the pre-refresh access token may still be valid for up to ssoTokenRefreshLeewaySeconds more seconds. No fallback to the still-valid token for that one request.

4. Missing expires_in in refresh response causes refresh-storm

Location: server/src/plugins/session.ts:43

Issue: If Keycloak's refresh response omits/has an invalid expires_in, tokenExpiresAt is set to "now," while the Redis TTL still uses the much longer sessionTtlSeconds fallback.

Failure scenario: The session survives in Redis for hours, but needsRefresh() evaluates true on every subsequent request, triggering a live Keycloak round-trip (discovery + refresh, up to ~10s) on every authenticated call instead of once — hammering Keycloak and adding latency to the hot path.

[marekdano]

Findings

🔴 Blocking

server/src/lib/memory-redis.ts:42 — MemoryRedis (the REDIS_URL=memory:// dev stand-in) has no set(key, value, "PX", ttl, "NX") method, but the new refresh lock in plugins/session.ts calls exactly that signature via request.server.redis.set(...). A cast in plugins/redis.ts (as unknown as FastifyInstance["redis"]) hides the gap from tsc, and only a separate FakeRedis test helper implements set, so no test exercises the real dev path.

Fix: Add the PX/NX overload to MemoryRedis, or type redis.set against the actual client interface so the mismatch surfaces at compile time.

---

🟠 Functionally-impacting

server/src/plugins/session.ts:130 — Lock release uses an unconditional redis.del(lockKey) rather than compare-and-delete against a per-holder token. If a holder's refresh outlasts the lock's 12s PX TTL, Redis auto-expires the lock, a second instance acquires a fresh one, and the first holder's delayed finally deletes that second instance's lock — opening the door to a third instance racing a concurrent refresh_token grant against the same rotating token.

Fix: Store a random value per lock acquisition and release with a Lua script (or equivalent) that only deletes when the stored value matches.

server/src/plugins/session.ts:124 — A waiter that times out after REFRESH_LOCK_MAX_WAIT_MS (3s) doesn't recheck whether the record is itself expired before returning it.

Fix: On timeout, re-fetch and check tokenExpiresAt; if still expired, return a signal that forces re-auth rather than silently handing back a dead token.
Failure scenario: session already expired, refresh takes >3s (e.g. slow Keycloak near its 5s timeout) → all waiters proceed with an expired bearer token → upstream 401 instead of the transparent refresh the feature promises.

server/src/plugins/session.ts:96 — Classifying Keycloak as "unreachable" leaves tokenExpiresAt unchanged, so every subsequent request on that session re-triggers a full discovery+refresh attempt with no backoff.

Fix: Set a short cooldown (e.g. a few seconds) on unreachable classification so repeated requests during an outage don't each pay ~10s of fetch timeouts.
Failure scenario: multi-minute Keycloak outage → every request/tab/poller for a near-expiry session re-acquires the lock and re-attempts discovery+refresh, inflating latency for that user for the outage's duration.

✅ Confirmed correct (not a finding)

oidc-discovery.ts now rewrites tokenEndpoint / jwksUri / endSessionEndpoint to ssoKeycloakBaseUrl instead of leaving them unrewritten or pointed at the public base. This is a genuine fix — sso-back-channel-logout.ts does a server-side fetch() against endSessionEndpoint and was hitting the wrong host before this PR.

[a-effort]

[redacted]

Posted on the wrong PR. This note was meant for #150 and does not apply here. Not approving this PR yet: the MemoryRedis gap marekdano raised is still open at this commit.

[marekdano]

Blocking

1. Stale refreshToken reuse on lock re-acquisition

File: server/src/plugins/session.ts:180-181
Concurrent requests can race: the first refreshes and rotates the token, the second then acquires the freed lock and refreshes with its own stale (already-rotated) token, 401ing a session that was just successfully refreshed.

2. All 4xx responses (including 429) classified "rejected" (permanent)

File: server/src/lib/sso-token-refresh.ts:95
Transient conditions (rate-limiting, momentary client_secret desync) force immediate session termination instead of a retry.

3. Non-JSON 4xx body classified "unreachable"

File: server/src/lib/sso-token-refresh.ts:67
A genuinely dead/rejected token behind a non-JSON error response (proxy/WAF page) gets retried indefinitely instead of cleanly logged out.

These three are correctness bugs in the changed code, reachable in real use (session-scoping/auth-adjacent), so they're the merge blockers.

Suggestions

1. 3s lock-wait timeout shorter than a legitimate refresh

server/src/plugins/session.ts:33-35, 163-177
A refresh can legitimately take up to 12s (REFRESH_LOCK_TTL_MS), but a waiting request only waits 3s (REFRESH_LOCK_MAX_WAIT_MS) before giving up. It now fails cleanly (401) instead of using a stale token, but a session being correctly refreshed by another request can still get force-logged-out just because the wait is too short. Fix: raise the wait time, or signal completion instead of polling on a fixed clock.

2. updateSessionTokens/deleteSession non-atomic race

server/src/lib/session-store.ts:105-131
updateSessionTokens does a plain read-then-write against Redis with no coordination against deleteSession's del. A logout racing a refresh can land between the two and resurrect the just-deleted session. Fix: make the write conditional on the key still existing (e.g. a small Lua script, or SET ... XX).

3. oidc-discovery.ts scope creep

server/src/lib/oidc-discovery.ts:104-140
Before this PR, tokenEndpoint/jwksUri were returned as-discovered. This PR now rewrites them to ssoKeycloakBaseUrl too — but getDiscoveryDocument() is shared, so this silently changes the existing login token-exchange (sso-callback.ts) and JWKS verification (sso-user-resolution.ts), not just the new refresh path. Fix: scope the rewrite to the refresh call site instead of the shared discovery document.

[gcgoncalves]

@marekdano I'll push back on blocker 3 and suggestion 3:

Blocker 3 (non-JSON 4xx → unreachable): Keycloak's token endpoint always returns JSON. A non-JSON 4xx status would be invalid, and the token status, unknown. Also, "retried indefinitely" isn't an accurate post-fix: unreachable now persists a cooldown, and a dead token would still get cleaned up by the revoke-on-401 path once it hits the API.

Suggestion 3 (oidc-discovery scope creep): Every consumer of tokenEndpoint/jwksUri/endSessionEndpoint is a server-side fetch() from inside the BFF. Token exchange, JWKS verification, back-channel logout, and refresh. Only authorizationEndpoint reaches the browser (the one on the public base, not the internal one). The behavior change to sso-callback.ts/JWKS verification isn't scope creep (the BFF couldn't reach localhost:8180 from inside Docker before this). This was already confirmed correct in the prior review round.

[vishu-bh]

Refresh path cannot handle an actually expired production SSO session, and a refresh can race logout and recreate a revoked session. I also found two paths that can pass a known-expired bearer token downstream.

[vishu-bh]

Production SSO sessions cannot reach this refresh path after the access token expires. establishSession() gives both the Redis record and bff_sid cookie the access token's expires_in, so Redis removes the stored refresh token and the browser drops the cookie at the same time. The expired-token tests use a 900-second session around an already-expired token, which the production login flow never creates.

Please give SSO sessions and cookies a lifetime based on the session or refresh-token lifetime, while keeping tokenExpiresAt as the separate access-token deadline. We should also have an integration test with real TTL behavior that advances past access-token expiry and verifies that one refresh succeeds.

[vishu-bh]

This read and the SETEX below are not atomic. Logout, natural expiry, or the upstream-401 revocation path can delete the session between them, after which SETEX recreates the revoked session. That can leave a copied bff_sid usable after the user logs out.

Please make this a conditional atomic write, for example SET key value EX ttl XX or a Lua CAS, and return false if the key no longer exists. A deterministic test that deletes the session between the read and write would cover the race.

[vishu-bh]

This replaces the bearer's real expiry with a retry cooldown and extends the Redis record to the full session TTL. During that window the middleware can treat a known-expired bearer as current, and BFF-local session checks still trust the Redis record.

Please keep the actual tokenExpiresAt unchanged and store refresh retry state separately, such as a refreshRetryAfter field or a short-lived Redis key. The old bearer should only be returned while its real expiry is still in the future.

[vishu-bh]

A held lock only tells us another request is refreshing; it does not make latest.bearerToken valid. If that token has already expired, forwarding it through /api/* can produce an upstream 401 that deletes the session while the lock holder is still completing a successful refresh.

Please return the old record only while its actual tokenExpiresAt is still in the future. Otherwise wait for the refresh to finish or return a controlled 401/503 without forwarding the expired bearer upstream.

[vishu-bh]

rest all looks good just one small thing to check

[vishu-bh]

Could we base the extended SSO session lifetime on ssoTokens?.refreshToken rather than the ssoTokens object itself?
ssoTokens is always passed by the callback, but refreshToken is optional. If Keycloak omits it, this creates a 24-hour Redis session/cookie around an access token that cannot be refreshed. After the access token expires, protected requests return 401 while /auth/session still reports the user as authenticated, which can cause a login redirect loop.
For sessions without a refresh token, can we keep the cookie/Redis TTL aligned with expires_in, as we do for password sessions? Please also add a test covering an SSO response with an ID token but no refresh token.

[a-effort]

Nice work! AI-assisted comments inline:

[a-effort]

Possible blocker to assess: A rejected refresh returns 401 but leaves the record in Redis and bff_sid in place, and /auth/session (routes/auth/session.ts:37) reports authenticated: true for any record still present, with no tokenExpiresAt check. Now that establishSession gives SSO sessions SESSION_TTL_SECONDS instead of expires_in, that record can outlive the dead refresh token by up to a day.

The SPA then loops. api/client.ts:174 navigates to /app/login on a 401, AuthContext reads /auth/session, which reports authenticated, and Login.tsx:27 navigates back to next, whose first /api call returns 401 again. It stops only when the cookie is cleared or the record's TTL runs out.

Keycloak's SSO Session Idle default of 30 minutes puts this on the common path: leave a tab idle past that and the next request gets invalid_grant. Before this PR the record's TTL matched expires_in, so the record was already gone and /auth/session answered false.

Deleting the session and clearing the cookie here would match what catch-all.ts:178 does on an upstream 401.

[a-effort]

An unreachable IdP past tokenExpiresAt returns 401 with the record still in Redis, so /auth/session keeps reporting authenticated for the duration of the outage. Consider applying an expiry check.