diff --git a/src/app/pages/auth/oidc/OidcCallback.tsx b/src/app/pages/auth/oidc/OidcCallback.tsx index a1432b7f7..5037adabd 100644 --- a/src/app/pages/auth/oidc/OidcCallback.tsx +++ b/src/app/pages/auth/oidc/OidcCallback.tsx @@ -5,7 +5,12 @@ import { AsyncStatus, useAsyncCallback } from '../../../hooks/useAsyncCallback'; import { setFallbackSession } from '../../../state/sessions'; import { completeAuthorizationCodeGrant } from './oidcLoginUtil'; import { getOidcCallbackUrl } from './oidcConfig'; -import { parseOidcCallbackParams } from './oidcState'; +import { + invalidateCachedClient, + isStaleClientError, + parseOidcCallbackParams, + readStoredOidcIssuer, +} from './oidcState'; /** * Exchange the authorization code for a Matrix session and persist it. The SDK @@ -76,6 +81,19 @@ export function OidcCallback() { if (params.kind === 'success') complete(params.code, params.state); }, [params, complete]); + useEffect(() => { + // #102 — the login-start path (startOidcLogin) already evicts a cached + // dynamic client id when the provider rejects it, but that only covers + // failures raised before the redirect. A client id can just as well be + // rejected on the way back (`?error=invalid_client` on this callback), + // and until now that case never invalidated the cache — so the same + // stale id gets retried, and rejected again, on every subsequent login. + if (params.kind === 'error' && isStaleClientError(params) && params.state) { + const issuer = readStoredOidcIssuer(params.state); + if (issuer) invalidateCachedClient(issuer); + } + }, [params]); + useEffect(() => { // Session persisted — full-page reload at the app root so it boots the // authenticated client (works for both hash and browser router configs). diff --git a/src/app/pages/auth/oidc/oidcState.test.ts b/src/app/pages/auth/oidc/oidcState.test.ts index dc9df851d..fbb114e3a 100644 --- a/src/app/pages/auth/oidc/oidcState.test.ts +++ b/src/app/pages/auth/oidc/oidcState.test.ts @@ -6,6 +6,7 @@ import { invalidateCachedClient, parseOidcCallbackParams, isStaleClientError, + readStoredOidcIssuer, } from './oidcState'; const installStorage = (): Map => { @@ -22,6 +23,20 @@ const installStorage = (): Map => { return store; }; +const installSessionStorage = (): Map => { + const store = new Map(); + (globalThis as { sessionStorage?: unknown }).sessionStorage = { + getItem: (k: string) => (store.has(k) ? store.get(k) : null), + setItem: (k: string, v: string) => { + store.set(k, String(v)); + }, + removeItem: (k: string) => { + store.delete(k); + }, + }; + return store; +}; + test('registration cache: get / put / invalidate, scoped by issuer + redirectUri', () => { installStorage(); assert.equal(getCachedClientId('iss', 'rd'), undefined); @@ -52,11 +67,21 @@ test('parseOidcCallbackParams classifies success / error / invalid', () => { kind: 'error', error: 'access_denied', errorDescription: 'nope', + state: undefined, }); assert.deepEqual(parseOidcCallbackParams('?error=bad'), { kind: 'error', error: 'bad', errorDescription: undefined, + state: undefined, + }); + // #102 — the provider echoes back `state` on an error redirect too; the + // callback needs it to look up the pending sign-in's issuer. + assert.deepEqual(parseOidcCallbackParams('?error=invalid_client&state=xyz'), { + kind: 'error', + error: 'invalid_client', + errorDescription: undefined, + state: 'xyz', }); assert.deepEqual(parseOidcCallbackParams('?code=only'), { kind: 'invalid' }); assert.deepEqual(parseOidcCallbackParams(''), { kind: 'invalid' }); @@ -82,3 +107,45 @@ test('isStaleClientError: transient / discovery / local failures keep the cache' assert.equal(isStaleClientError(undefined), false); assert.equal(isStaleClientError('invalid_client'), false); // bare strings are not error objects }); + +test('isStaleClientError: also recognises the OidcCallback error-redirect shape (#102)', () => { + // Same field name (`error`) as the ErrorResponse shape, so no special-casing + // is needed — but pin it down since the callback now depends on this. + assert.equal( + isStaleClientError({ + kind: 'error', + error: 'invalid_client', + errorDescription: undefined, + state: 'xyz', + }), + true, + ); + assert.equal( + isStaleClientError({ + kind: 'error', + error: 'access_denied', + errorDescription: undefined, + state: 'xyz', + }), + false, + ); +}); + +test('readStoredOidcIssuer reads the issuer persisted by generateOidcAuthorizationUrl', () => { + const store = installSessionStorage(); + assert.equal(readStoredOidcIssuer('xyz'), undefined); // nothing stored yet + store.set( + 'mx_oidc_xyz', + JSON.stringify({ authority: 'https://issuer.example', client_id: 'abc' }), + ); + assert.equal(readStoredOidcIssuer('xyz'), 'https://issuer.example'); + assert.equal(readStoredOidcIssuer('other-state'), undefined); // different state = miss +}); + +test('readStoredOidcIssuer tolerates corrupt or missing storage', () => { + const store = installSessionStorage(); + store.set('mx_oidc_bad', '{ not json'); + assert.equal(readStoredOidcIssuer('bad'), undefined); + store.set('mx_oidc_noauth', JSON.stringify({ client_id: 'abc' })); + assert.equal(readStoredOidcIssuer('noauth'), undefined); +}); diff --git a/src/app/pages/auth/oidc/oidcState.ts b/src/app/pages/auth/oidc/oidcState.ts index b35a13cde..f52d69c97 100644 --- a/src/app/pages/auth/oidc/oidcState.ts +++ b/src/app/pages/auth/oidc/oidcState.ts @@ -45,14 +45,16 @@ export const invalidateCachedClient = (issuer: string): void => { const STALE_CLIENT_ERROR_CODES = new Set(['invalid_client', 'unauthorized_client']); /** - * #67 — pure: does a `startOidcLogin` failure indicate the CACHED client id is + * #67 / #102 — pure: does an OIDC failure indicate the CACHED client id is * bad? Only then is dropping the registration cache justified; a transient * network error, an offline discovery fetch, or a local (`crypto.randomUUID`) * failure must leave it alone, otherwise every retry performs a fresh dynamic * registration and piles throwaway clients onto the provider. Recognised * shapes: an OAuth error response (`{ error: 'invalid_client' }`, as thrown by - * oidc-client-ts `ErrorResponse`), or an HTTP 400/401 carried as - * `httpStatus`/`status` (MatrixError-style) from the registration/authorize step. + * oidc-client-ts `ErrorResponse`, OR as echoed back on the callback redirect's + * `?error=` query param via {@link OidcCallbackParams}), or an HTTP 400/401 + * carried as `httpStatus`/`status` (MatrixError-style) from the + * registration/authorize step. */ export const isStaleClientError = (e: unknown): boolean => { if (!e || typeof e !== 'object') return false; @@ -69,7 +71,7 @@ export const isStaleClientError = (e: unknown): boolean => { /** Parsed shape of the provider's redirect back to our callback URL. */ export type OidcCallbackParams = | { kind: 'success'; code: string; state: string } - | { kind: 'error'; error: string; errorDescription?: string } + | { kind: 'error'; error: string; errorDescription?: string; state?: string } | { kind: 'invalid' }; /** Pure: classify the callback query string into success / error / invalid. */ @@ -77,10 +79,41 @@ export const parseOidcCallbackParams = (search: string): OidcCallbackParams => { const params = new URLSearchParams(search); const error = params.get('error'); if (error) { - return { kind: 'error', error, errorDescription: params.get('error_description') ?? undefined }; + return { + kind: 'error', + error, + errorDescription: params.get('error_description') ?? undefined, + // OAuth error redirects echo back the `state` we sent, same as a + // success redirect would — needed to look up the pending sign-in's + // issuer (see readStoredOidcIssuer) when the client id itself was + // rejected. + state: params.get('state') ?? undefined, + }; } const code = params.get('code'); const state = params.get('state'); if (code && state) return { kind: 'success', code, state }; return { kind: 'invalid' }; }; + +/** + * #102 — the issuer for a pending sign-in isn't available on the callback's + * error branch (we bail out before `completeAuthorizationCodeGrant`, which is + * what would otherwise surface it). oidc-client-ts's `generateOidcAuthorizationUrl` + * persists the pending sign-in's `SigninState` (including `authority` and + * `client_id`) into sessionStorage before redirecting, keyed by + * `mx_oidc_` (WebStorageStateStore prefix `mx_oidc_`, keyed by the same + * `state` value the provider echoes back on redirect). Read it directly so a + * genuinely stale cached client id (`invalid_client`) can be evicted even when + * the callback errors before a token exchange is attempted. + */ +export const readStoredOidcIssuer = (state: string): string | undefined => { + try { + const raw = sessionStorage.getItem(`mx_oidc_${state}`); + if (!raw) return undefined; + const { authority } = JSON.parse(raw) as { authority?: unknown }; + return typeof authority === 'string' ? authority : undefined; + } catch { + return undefined; + } +};