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 () => {