From 8210ee704687ad3bab0ba0e418d6217395bc19f9 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 19:46:05 -0400 Subject: [PATCH] fix(auth): only drop the cached OIDC client id when the client is rejected A transient network/discovery failure invalidated the cached dynamic client and registered a fresh one on every retry. Invalidate only on invalid_client / unauthorized_client or a 400/401 from the provider. Unit-tested. Fixes #67 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA --- src/app/pages/auth/oidc/oidcLoginUtil.ts | 14 +++++++++--- src/app/pages/auth/oidc/oidcState.test.ts | 22 +++++++++++++++++++ src/app/pages/auth/oidc/oidcState.ts | 26 +++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/app/pages/auth/oidc/oidcLoginUtil.ts b/src/app/pages/auth/oidc/oidcLoginUtil.ts index 73c2e4a16..a23f8739a 100644 --- a/src/app/pages/auth/oidc/oidcLoginUtil.ts +++ b/src/app/pages/auth/oidc/oidcLoginUtil.ts @@ -6,7 +6,12 @@ import { registerOidcClient, } from 'matrix-js-sdk'; import { getOidcCallbackUrl, getOidcClientMetadata } from './oidcConfig'; -import { cacheClientId, getCachedClientId, invalidateCachedClient } from './oidcState'; +import { + cacheClientId, + getCachedClientId, + invalidateCachedClient, + isStaleClientError, +} from './oidcState'; export { completeAuthorizationCodeGrant }; @@ -42,8 +47,11 @@ export const startOidcLogin = async (issuer: string, homeserverBaseUrl: string): }); window.location.assign(url); } catch (e) { - // Drop a possibly-stale cached client so the next attempt re-registers. - invalidateCachedClient(issuer); + // #67 — drop the cached client so the next attempt re-registers, but ONLY + // when the failure says the client id itself was rejected. Network and + // discovery failures keep the cache, otherwise each retry would register + // yet another throwaway dynamic client at the provider. + if (isStaleClientError(e)) invalidateCachedClient(issuer); throw e; } }; diff --git a/src/app/pages/auth/oidc/oidcState.test.ts b/src/app/pages/auth/oidc/oidcState.test.ts index 9a4276764..dc9df851d 100644 --- a/src/app/pages/auth/oidc/oidcState.test.ts +++ b/src/app/pages/auth/oidc/oidcState.test.ts @@ -5,6 +5,7 @@ import { cacheClientId, invalidateCachedClient, parseOidcCallbackParams, + isStaleClientError, } from './oidcState'; const installStorage = (): Map => { @@ -60,3 +61,24 @@ test('parseOidcCallbackParams classifies success / error / invalid', () => { assert.deepEqual(parseOidcCallbackParams('?code=only'), { kind: 'invalid' }); assert.deepEqual(parseOidcCallbackParams(''), { kind: 'invalid' }); }); + +test('isStaleClientError: only client-rejection shapes invalidate the cache', () => { + // OAuth error responses naming the client (oidc-client-ts ErrorResponse shape). + assert.equal(isStaleClientError({ error: 'invalid_client' }), true); + assert.equal(isStaleClientError({ error: 'unauthorized_client' }), true); + // HTTP 400/401 from the registration/authorize step (MatrixError-style). + assert.equal(isStaleClientError({ httpStatus: 400 }), true); + assert.equal(isStaleClientError({ httpStatus: 401 }), true); + assert.equal(isStaleClientError({ status: 401 }), true); + assert.equal(isStaleClientError({ httpStatus: 500 }), false); + assert.equal(isStaleClientError({ httpStatus: 400, status: 200 }), true); // httpStatus wins +}); + +test('isStaleClientError: transient / discovery / local failures keep the cache', () => { + assert.equal(isStaleClientError(new TypeError('Failed to fetch')), false); + assert.equal(isStaleClientError(new Error('Something went wrong with OIDC discovery')), false); + assert.equal(isStaleClientError({ error: 'server_error' }), false); + assert.equal(isStaleClientError(new Error('crypto.randomUUID is not a function')), false); + assert.equal(isStaleClientError(undefined), false); + assert.equal(isStaleClientError('invalid_client'), false); // bare strings are not error objects +}); diff --git a/src/app/pages/auth/oidc/oidcState.ts b/src/app/pages/auth/oidc/oidcState.ts index cb1300157..b35a13cde 100644 --- a/src/app/pages/auth/oidc/oidcState.ts +++ b/src/app/pages/auth/oidc/oidcState.ts @@ -40,6 +40,32 @@ export const invalidateCachedClient = (issuer: string): void => { } }; +// OAuth error codes that mean the provider no longer recognises our client id +// (deleted/expired dynamic registration, or one issued by a different deployment). +const STALE_CLIENT_ERROR_CODES = new Set(['invalid_client', 'unauthorized_client']); + +/** + * #67 — pure: does a `startOidcLogin` 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. + */ +export const isStaleClientError = (e: unknown): boolean => { + if (!e || typeof e !== 'object') return false; + const { error, httpStatus, status } = e as { + error?: unknown; + httpStatus?: unknown; + status?: unknown; + }; + if (typeof error === 'string' && STALE_CLIENT_ERROR_CODES.has(error)) return true; + const code = typeof httpStatus === 'number' ? httpStatus : status; + return code === 400 || code === 401; +}; + /** Parsed shape of the provider's redirect back to our callback URL. */ export type OidcCallbackParams = | { kind: 'success'; code: string; state: string }