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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA
This commit is contained in:
@@ -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;
|
||||
}
|
||||
};
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
cacheClientId,
|
||||
invalidateCachedClient,
|
||||
parseOidcCallbackParams,
|
||||
isStaleClientError,
|
||||
} from './oidcState';
|
||||
|
||||
const installStorage = (): Map<string, string> => {
|
||||
@@ -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
|
||||
});
|
||||
|
||||
@@ -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 }
|
||||
|
||||
Reference in New Issue
Block a user