fix(auth): OIDC callback evicts a cached client id the provider rejects

The redirect back with ?error=invalid_client is the only place a stale
dynamic client id is ever rejected; the callback now resolves the issuer
from the SDK's stored mx_oidc_<state> entry and invalidates the cache so
the next attempt re-registers. Degrades to a no-op if the state entry is
gone. Unit-tested.

Fixes #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA

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).

src/app/pages/auth/oidc/oidcState.test.ts

@@ -6,6 +6,7 @@ import {
   invalidateCachedClient,
   parseOidcCallbackParams,
   isStaleClientError,
+  readStoredOidcIssuer,
 } from './oidcState';
 
 const installStorage = (): Map<string, string> => {
@@ -22,6 +23,20 @@ const installStorage = (): Map<string, string> => {
   return store;
 };
 
+const installSessionStorage = (): Map<string, string> => {
+  const store = new Map<string, string>();
+  (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);
+});

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_<state>` (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;
+  }
+};