From fabed40906d641de3fdcafda548ae78ebd67d7a8 Mon Sep 17 00:00:00 2001 From: Daniel Covington Date: Wed, 6 May 2026 11:24:39 -0400 Subject: [PATCH] fix: deduplicate OIDC callback sequence to prevent StrictMode race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit React StrictMode's double-invocation of useEffect caused the first run to remove oidcStateStorageKey from sessionStorage before the async code exchange began. The second run would see the missing state, throw, and display "Authentication failed" — while the first run completed in the background (storing tokens and replacing the URL), making a page refresh appear to work. Introduces a module-scope Promise (pendingCallbackSequence) that covers the full callback sequence: token exchange, session fetch, and URL navigation. The second effect run shares the same Promise and awaits its result, so no re- validation or re-fetching occurs, and only the non-cancelled run sets state. Co-Authored-By: Claude Sonnet 4.6 --- .../implementation-artifacts/deferred-work.md | 8 ++ .../src/auth/useOidcSession.ts | 101 +++++++++++++----- 2 files changed, 84 insertions(+), 25 deletions(-) create mode 100644 _bmad-output/implementation-artifacts/deferred-work.md diff --git a/_bmad-output/implementation-artifacts/deferred-work.md b/_bmad-output/implementation-artifacts/deferred-work.md new file mode 100644 index 0000000..7859bf5 --- /dev/null +++ b/_bmad-output/implementation-artifacts/deferred-work.md @@ -0,0 +1,8 @@ +## Deferred from: fix-strictmode-oidc-callback-race (2026-05-06) + +- `pendingCallbackSequence` is not scoped to a specific callback invocation — if `useOidcSession` were ever mounted twice simultaneously, the second instance would skip CSRF validation and piggyback on the first's exchange. Pre-existing architectural assumption; low risk given single-mount usage, but worth an assertion if the hook gains wider use. +- `user.workspacePath` is used in `window.history.replaceState` without validating it is a relative path. Server currently returns only hard-coded relative paths, but an open-redirect risk exists if the return value ever comes from user-controlled input. + +## Deferred from: code review of 1-4-keycloak-role-mapping-application-authorization.md (2026-05-06) + +- AuthorizationProbeController ships canned operational routes in the production controller surface. Evidence: Campaign_Tracker.Server/Controllers/AuthorizationProbeController.cs:8. Reason: deferred by user choice during review. diff --git a/campaign-tracker-client/src/auth/useOidcSession.ts b/campaign-tracker-client/src/auth/useOidcSession.ts index bca3f4e..c513475 100644 --- a/campaign-tracker-client/src/auth/useOidcSession.ts +++ b/campaign-tracker-client/src/auth/useOidcSession.ts @@ -1,13 +1,16 @@ import { useEffect, useMemo, useState } from 'react' import { buildKeycloakAuthorizationUrl, - decodeAuthenticatedUser, + clearStoredAuthTokenSet, exchangeAuthorizationCode, + fetchAuthenticatedSession, getAuthCallbackPath, getKeycloakClientConfig, isAuthCallbackPath, isTokenRefreshRequired, + mergeRefreshedTokenSet, oidcReturnPathStorageKey, + oidcStateStorageKey, readStoredAuthTokenSet, refreshAccessToken, shouldRedirectToLogin, @@ -16,6 +19,17 @@ import { type AuthenticatedUser, } from './authContracts' +// React StrictMode mounts effects twice (mount → cleanup → remount). The first run +// removes the OIDC state key from sessionStorage before the async exchange completes, +// so the second run sees a missing state and throws. This module-scope promise covers +// the full callback sequence (exchange + session fetch + navigate) so the second run +// simply awaits the result and applies it, instead of re-validating or re-fetching. +let pendingCallbackSequence: Promise<{ + tokens: AuthTokenSet + user: AuthenticatedUser + returnPath: string +}> | null = null + type OidcSessionState = | { status: 'checking'; user: null; tokens: null; error: null } | { status: 'redirecting'; user: null; tokens: null; error: null } @@ -42,37 +56,66 @@ export function useOidcSession(): OidcSessionState { if (isAuthCallbackPath(currentUrl.pathname, callbackPath)) { const code = currentUrl.searchParams.get('code') + if (!code) { throw new Error('Missing Keycloak authorization code') } - const tokens = await exchangeAuthorizationCode(config, code) - storeAuthTokenSet(tokens) - const user = decodeAuthenticatedUser(tokens.accessToken) - const returnPath = - window.sessionStorage.getItem(oidcReturnPathStorageKey) ?? user.workspacePath - window.sessionStorage.removeItem(oidcReturnPathStorageKey) - window.history.replaceState({}, document.title, returnPath) + if (!pendingCallbackSequence) { + const returnedState = currentUrl.searchParams.get('state') + const expectedState = window.sessionStorage.getItem(oidcStateStorageKey) + + if (!returnedState || returnedState !== expectedState) { + throw new Error('Invalid Keycloak authorization state') + } + + window.sessionStorage.removeItem(oidcStateStorageKey) + pendingCallbackSequence = (async () => { + const tokens = await exchangeAuthorizationCode(config, code) + storeAuthTokenSet(tokens) + const user = await fetchAuthenticatedSession(tokens.accessToken) + const returnPath = user.workspacePath + window.sessionStorage.removeItem(oidcReturnPathStorageKey) + window.history.replaceState({}, document.title, returnPath) + return { tokens, user, returnPath } + })() + } + + const result = await pendingCallbackSequence + pendingCallbackSequence = null if (!cancelled) { - setState({ status: 'authenticated', user, tokens, error: null }) + setState({ status: 'authenticated', user: result.user, tokens: result.tokens, error: null }) } return } if (storedTokens) { - const tokens = isTokenRefreshRequired(storedTokens) - ? await refreshAccessToken(config, storedTokens.refreshToken) - : storedTokens + let tokens = storedTokens + + if (isTokenRefreshRequired(storedTokens)) { + try { + tokens = mergeRefreshedTokenSet( + storedTokens, + await refreshAccessToken(config, storedTokens.refreshToken), + ) + } catch { + clearStoredAuthTokenSet() + redirectToLogin() + return + } + } if (tokens !== storedTokens) { storeAuthTokenSet(tokens) } + const user = await fetchAuthenticatedSession(tokens.accessToken) + if (!cancelled) { setState({ status: 'authenticated', - user: decodeAuthenticatedUser(tokens.accessToken), + user, tokens, error: null, }) @@ -81,20 +124,11 @@ export function useOidcSession(): OidcSessionState { } if (shouldRedirectToLogin(window.location.pathname, null, config)) { - window.sessionStorage.setItem( - oidcReturnPathStorageKey, - `${window.location.pathname}${window.location.search}`, - ) - setState({ status: 'redirecting', user: null, tokens: null, error: null }) - window.location.assign( - buildKeycloakAuthorizationUrl( - config, - crypto.randomUUID(), - crypto.randomUUID(), - ), - ) + redirectToLogin() } } catch { + pendingCallbackSequence = null + clearStoredAuthTokenSet() if (!cancelled) { setState({ status: 'error', @@ -106,6 +140,23 @@ export function useOidcSession(): OidcSessionState { } } + function redirectToLogin() { + const state = crypto.randomUUID() + window.sessionStorage.setItem( + oidcReturnPathStorageKey, + `${window.location.pathname}${window.location.search}`, + ) + window.sessionStorage.setItem(oidcStateStorageKey, state) + setState({ status: 'redirecting', user: null, tokens: null, error: null }) + window.location.assign( + buildKeycloakAuthorizationUrl( + config, + state, + crypto.randomUUID(), + ), + ) + } + void syncSession() return () => {