From 53c8e1fb18c69eaac353ebc013b36ba957e35395 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Fri, 28 Aug 2026 16:34:19 -0400 Subject: [PATCH 01/11] first pass --- .../src/Credential/Credential.ts | 31 ++++--- .../src/Credential/CredentialDataSource.ts | 2 + .../auth-foundation/src/http/APIClient.ts | 8 ++ packages/auth-foundation/src/oauth2/client.ts | 8 ++ .../auth-foundation/src/utils/EventEmitter.ts | 35 +++++++- .../test/spec/Credential/Credential.spec.ts | 33 ++++++++ .../src/Credential/CredentialCoordinator.ts | 80 ++++++++++--------- 7 files changed, 148 insertions(+), 49 deletions(-) diff --git a/packages/auth-foundation/src/Credential/Credential.ts b/packages/auth-foundation/src/Credential/Credential.ts index be3a09d7..5814559d 100644 --- a/packages/auth-foundation/src/Credential/Credential.ts +++ b/packages/auth-foundation/src/Credential/Credential.ts @@ -49,7 +49,12 @@ export class Credential implements RequestAuthorizer, JSONSerializable { // unbinds listeners of previous coordinator ( [ - 'credential_added', 'credential_removed', 'credential_refreshed', 'default_changed', 'cleared' + 'credential_added', + 'credential_removed', + 'credential_refreshed', + 'default_changed', + 'cleared', + 'metadata_updated' ] satisfies (keyof CredentialCoordinatorEvents)[] ).forEach((evt) => previousCoordinator.emitter.off(evt)); @@ -63,6 +68,10 @@ export class Credential implements RequestAuthorizer, JSONSerializable { this.coordinator.emitter.on('credential_removed', ({ id }) => { this.emitter.emit('credential_removed', { id }); }); + + this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { + this.emitter.emit('tags_updated', { id, tags: metadata?.tags ?? [] }); + }); } static { @@ -87,6 +96,9 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /** @internal */ protected _userInfo: UserInfo | undefined; + /** @internal */ + #controller = new AbortController(); + /** * @remarks * Do not use directly, use {@link store | Credential.store} instead @@ -321,6 +333,14 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /////// public instances methods /////// + /** + * Cleans up resourece associated with the Credential instance to prevent leaks. + */ + public dispose () { + this.oauth2.dispose(); + this.#controller.abort('dispose'); + } + /** * Updates tags associated with {@link Credential} * @@ -383,14 +403,7 @@ export class Credential implements RequestAuthorizer, JSONSerializable { this.oauth2.emitter.on('token_did_refresh', ({ token }) => { if (Token.isEqual(token, this.token)) { return; } this.token = token; - }); - - // bind listener to Derived class instance - this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { - if (this.id === id) { - Credential.emitter.emit('tags_updated', { id, tags: metadata?.tags ?? [] }); - } - }); + }, { signal: this.#controller.signal }); } // oauth2 methods diff --git a/packages/auth-foundation/src/Credential/CredentialDataSource.ts b/packages/auth-foundation/src/Credential/CredentialDataSource.ts index 2574b03f..665f40fb 100644 --- a/packages/auth-foundation/src/Credential/CredentialDataSource.ts +++ b/packages/auth-foundation/src/Credential/CredentialDataSource.ts @@ -101,12 +101,14 @@ export class DefaultCredentialDataSource implements CredentialDataSource { const id = typeof cred === 'string' ? cred : cred.id; if (this.credentials.has(id)) { const cred = this.credentials.get(id)!; + cred.dispose(); this.credentials.delete(id); this.emitter.emit('credential_removed', { dataSource: this, id: cred.id }); } } public clear () { + this.credentials.forEach(cred => cred.dispose()); this.credentials.clear(); } diff --git a/packages/auth-foundation/src/http/APIClient.ts b/packages/auth-foundation/src/http/APIClient.ts index ca72bf24..c0ddb7d7 100644 --- a/packages/auth-foundation/src/http/APIClient.ts +++ b/packages/auth-foundation/src/http/APIClient.ts @@ -56,6 +56,14 @@ export abstract class APIClient { await this.dpopNonceCache.cacheNonce(this.getDPoPNonceCacheKey(request), nonce); } + /** + * Cleans up resourece associated with the client instance to prevent leaks. + */ + public dispose () { + this.emitter.clear(); + this.interceptors.splice(0, this.interceptors.length); // clears array in place + } + /** * Registers an {@link APIClient.RequestInterceptor} on the {@link APIClient} * diff --git a/packages/auth-foundation/src/oauth2/client.ts b/packages/auth-foundation/src/oauth2/client.ts index 751ac27b..99bd1343 100644 --- a/packages/auth-foundation/src/oauth2/client.ts +++ b/packages/auth-foundation/src/oauth2/client.ts @@ -145,6 +145,14 @@ export class OAuth2Client e return json; } + /** + * Cleans up resourece associated with the client instance to prevent leaks. + */ + public dispose () { + super.dispose(); + this.#httpCache.clear(); + } + /** * Retrieves the Authorization Server's OpenID configuration */ diff --git a/packages/auth-foundation/src/utils/EventEmitter.ts b/packages/auth-foundation/src/utils/EventEmitter.ts index 6487d650..f59721af 100644 --- a/packages/auth-foundation/src/utils/EventEmitter.ts +++ b/packages/auth-foundation/src/utils/EventEmitter.ts @@ -8,6 +8,8 @@ type EventMap = { }; type EventListener = T extends void ? () => void : (event: T) => void; +type EventListenerOptions = { signal: AbortSignal } + /** * @group EventEmitter */ @@ -21,13 +23,28 @@ export interface Emitter { */ export class EventEmitter { listeners: { [K in keyof Events]?: Array> } = {}; + signals: WeakMap void }> = new WeakMap(); + + on( + eventName: K, + handler: EventListener, + options: Partial = {} + ): this { + const { signal } = options; - on(eventName: K, handler: EventListener): this { if (!this.listeners[eventName]) { this.listeners[eventName] = []; } this.listeners[eventName]!.push(handler); + if (signal && !signal?.aborted) { + const abortHandler = () => { + this.off(eventName, handler); + }; + signal.addEventListener('abort', abortHandler, { once: true }); + this.signals.set(handler, { signal, abortHandler }); + } + return this; } @@ -37,14 +54,25 @@ export class EventEmitter { } if (!handler) { + this.listeners[eventName]?.forEach(h => this.detachSignal(h)); delete this.listeners[eventName]; return this; } + this.detachSignal(handler); this.listeners[eventName] = this.listeners[eventName]?.filter(l => l !== handler); return this; } + /** @internal removes the `AbortSignal` registration (if any) associated with `handler` */ + protected detachSignal (handler: EventListener): void { + const entry = this.signals.get(handler); + if (entry) { + entry.signal.removeEventListener('abort', entry.abortHandler); + this.signals.delete(handler); + } + } + emit(eventName: K, data: Events[K]): void; emit(eventName: K): void; emit(eventName: K, data?: Events[K]): void { @@ -82,4 +110,9 @@ export class EventEmitter { emitter.on(event, handler); } } + + clear (): this { + (Object.keys(this.listeners) as (keyof Events)[]).forEach(eventName => this.off(eventName)); + return this; + } } \ No newline at end of file diff --git a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts index af126c96..f43fb108 100644 --- a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts @@ -57,6 +57,39 @@ describe('Credential', () => { expect(onRemoved1).toHaveBeenCalledTimes(1); // was removed, should not be called again expect(onRemoved2).toHaveBeenNthCalledWith(2, { id: cred.id }); }); + + // regression test for the leaked `observeToken()` -> `coordinator.emitter.on('metadata_updated', ...)` + // subscription: every constructed Credential added one more listener to the page-lifetime coordinator + // emitter, and nothing ever removed it, even after `.remove()` + it('should not accumulate metadata_updated listeners on the coordinator emitter across store/remove cycles', async () => { + const emitter = (Credential as any).coordinator.emitter; + const baseline = emitter.listeners['metadata_updated']?.length ?? 0; + + for (let i = 0; i < 25; i++) { + const cred = await Credential.store(makeTestToken()); + await cred.remove(); + } + + expect(emitter.listeners['metadata_updated']?.length ?? 0).toBe(baseline); + }); + + // target behavior for the planned fix: `metadata_updated` should be bound exactly once, from the + // static `coordinator` setter, and cleanly rebound (not duplicated, not left dangling) on reassignment + it('binds exactly one metadata_updated listener via the coordinator setter, and unbinds the previous one on reassignment', () => { + const previousCoordinator = (Credential as any).coordinator; + const newCoordinator = new previousCoordinator.constructor(Credential); + + (Credential as any).coordinator = newCoordinator; + + try { + expect(previousCoordinator.emitter.listeners['metadata_updated']).toBeFalsy(); + expect(newCoordinator.emitter.listeners['metadata_updated']?.length).toBe(1); + } + finally { + // restore original coordinator so later tests aren't affected + (Credential as any).coordinator = previousCoordinator; + } + }); }); describe('getters/setters', () => { diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index b3fc4d59..6f5b073a 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -30,7 +30,7 @@ import { isFirefox } from '../utils/UserAgent.ts'; function log (...args: any[]) {} -type BroadcastMessage = { eventName: string, id: string, source: string, value: JsonRecord }; +type BroadcastMessage = { eventName: string, id: string, source: string }; /** * Browser-specific implementation of {@link CredentialCoordinator} @@ -50,8 +50,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme this.registerTabListeners(); this.emitter.on('credential_refreshed', ({ credential }) => { - const { token } = credential; - this.broadcast('credential_refreshed', { id: token.id, value: token.toJSON() }); + this.broadcast('credential_refreshed', { id: credential.id }); }); } @@ -73,7 +72,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme super.tokenStorage = tokenStorage; this.tokenStorage.emitter.on('token_added', ({ token }) => { - this.broadcast('credential_added', { id: token.id, value: token.toJSON() }); + this.broadcast('credential_added', { id: token.id }); }); this.tokenStorage.emitter.on('token_removed', ({ id }) => { @@ -112,7 +111,7 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme await pause(50); } - const { eventName, id, value, source } = event.data as BroadcastMessage; + const { eventName, id, source } = event.data as BroadcastMessage; log('tab sync event: ', { eventName, source }); if (source == this.id) { return; // do not listen to messages broadcasted by this instance @@ -136,44 +135,47 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme this.emitter.emit('metadata_updated', { storage: this.tokenStorage, id, metadata }); } } + else if (eventName === 'credential_removed') { + log('removal'); + this.credentialDataSource.remove(id); + + // // removal messages never carried a token body; `id` is all `hasCredential` actually reads + // const token = new Token({ id } as TokenInit); + // if (this.credentialDataSource.hasCredential(token)) { + // this.credentialDataSource.remove(id); + // } + // else { + // // TODO: is this needed? + // // ensures removal event is broadcast, regardless of the DataSource knowledge of the Credential + // this.emitter.emit('credential_removed', { dataSource: this.credentialDataSource, id }); + // } + } else { - // TODO: confirm client info - const token = new Token({ ...value, id } as TokenInit); - - if (eventName === 'credential_removed') { - log('removal'); - if (this.credentialDataSource.hasCredential(token)) { - this.credentialDataSource.remove(id); - } - else { - // TODO: is this needed? - // ensures removal event is broadcast, regardless of the DataSource knowledge of the Credential - this.emitter.emit('credential_removed', { dataSource: this.credentialDataSource, id }); - } + const token = await this.tokenStorage.get(id); + if (!token) { + return; } - else { - const credential = this.credentialDataSource.credentialFor(token); - if (eventName === 'credential_added') { - log('added'); - // credentialDataSource.credentialFor() call above handles updating credDataSrc - } - else if (eventName === 'credential_refreshed') { - log('refresh'); - - // when a Credential is updated in a separate tab, the Token passed via the broadcast - // may differ from cred.token via DataSource, so the update should continue. - // If the tokens are equal, this means this DataSource has already updated the token to the new value - // eslint-disable-next-line max-depth - if (Token.isEqual(token, credential.token)) { - log('token has already been updated'); - return; - } - - // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API - credential.token = token; - this.emitter.emit('credential_refreshed', { credential }); + const credential = this.credentialDataSource.credentialFor(token); + + if (eventName === 'credential_added') { + log('added'); + // credentialDataSource.credentialFor() call above handles updating credDataSrc + } + else if (eventName === 'credential_refreshed') { + log('refresh'); + + // when a Credential is updated in a separate tab, the Token read from storage + // may differ from cred.token via DataSource, so the update should continue. + // If the tokens are equal, this means this DataSource has already updated the token to the new value + if (Token.isEqual(token, credential.token)) { + log('token has already been updated'); + return; } + + // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API + credential.token = token; + this.emitter.emit('credential_refreshed', { credential }); } } From 1022fef34a500ee110a1ee3658f2b982e1a43319 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Mon, 31 Aug 2026 10:11:08 -0400 Subject: [PATCH 02/11] test app fixes --- e2e/apps/redirect-model/src/component/Landing.tsx | 11 +++++++++-- e2e/apps/redirect-model/src/component/Token.tsx | 15 ++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/e2e/apps/redirect-model/src/component/Landing.tsx b/e2e/apps/redirect-model/src/component/Landing.tsx index 02614584..80b175a6 100644 --- a/e2e/apps/redirect-model/src/component/Landing.tsx +++ b/e2e/apps/redirect-model/src/component/Landing.tsx @@ -30,12 +30,19 @@ export function Landing () { setCredentialIds(allIDs); }; + const removeHandler = async ({ id }) => { + if (credential?.id === id) { + setCredential(null); + } + await updateHandler(); + } + const defaultHandler = ({ id }) => { setDefault(id); }; Credential.on('credential_added', updateHandler); - Credential.on('credential_removed', updateHandler); + Credential.on('credential_removed', removeHandler); Credential.on('cleared', updateHandler); Credential.on('default_changed', defaultHandler); @@ -45,7 +52,7 @@ export function Landing () { Credential.off('cleared', updateHandler); Credential.off('default_changed', defaultHandler); }; - }, [setCredentialIds, setCredential, setDefault]); + }, [credential, setCredentialIds, setCredential, setDefault]); const clear = async () => { await Credential.clear(); diff --git a/e2e/apps/redirect-model/src/component/Token.tsx b/e2e/apps/redirect-model/src/component/Token.tsx index ca512bba..09f7c638 100644 --- a/e2e/apps/redirect-model/src/component/Token.tsx +++ b/e2e/apps/redirect-model/src/component/Token.tsx @@ -12,22 +12,27 @@ export function Token ({ credential }: { credential: Credential }) { Credential.on('credential_refreshed', handler); const tagsHandler = ({ id, tags }) => { + console.log('tags updated: ', id, tags); if (id === credential.id) { + console.log('setTags called') setTags(tags); } } Credential.on('tags_updated', tagsHandler); + setToken(credential.token); + setTags(credential.tags); + return () => { Credential.off('credential_refreshed', handler); Credential.off('tags_updated', tagsHandler); }; - }, [setToken]); + }, [credential, setToken, setTags]); - useEffect(() => { - setToken(credential.token); - setTags(credential.tags); - }, [credential]); + // useEffect(() => { + // setToken(credential.token); + // setTags(credential.tags); + // }, [credential]); const remove = async () => { await credential.remove(); From bb7a297e99f6d38cb6a9620babefabfcaf8d9064 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Mon, 31 Aug 2026 12:12:12 -0400 Subject: [PATCH 03/11] linter fix --- packages/auth-foundation/src/utils/EventEmitter.ts | 2 +- packages/spa-platform/src/Credential/CredentialCoordinator.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/auth-foundation/src/utils/EventEmitter.ts b/packages/auth-foundation/src/utils/EventEmitter.ts index f59721af..ff5811e8 100644 --- a/packages/auth-foundation/src/utils/EventEmitter.ts +++ b/packages/auth-foundation/src/utils/EventEmitter.ts @@ -23,7 +23,7 @@ export interface Emitter { */ export class EventEmitter { listeners: { [K in keyof Events]?: Array> } = {}; - signals: WeakMap void }> = new WeakMap(); + signals: WeakMap<(...arg: any[]) => void, { signal: AbortSignal, abortHandler: () => void }> = new WeakMap(); on( eventName: K, diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index 6f5b073a..9aac2b41 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -7,8 +7,7 @@ import type { TokenStorage, JsonPrimitive, TokenStorageEvents, - JsonRecord, - TokenInit, + JsonRecord } from '@okta/auth-foundation/core'; import { Token, From 2779b064edcbaa73550f04b7fcdc9abce147dafc Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Mon, 31 Aug 2026 13:50:03 -0400 Subject: [PATCH 04/11] adds unit test coverage --- .../test/spec/Credential/Credential.spec.ts | 14 +- .../DefaultCredentialDataSource.spec.ts | 7 +- .../test/spec/utils/EventEmitter.spec.ts | 125 ++++++++++++++++-- .../src/Credential/CredentialCoordinator.ts | 11 -- 4 files changed, 133 insertions(+), 24 deletions(-) diff --git a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts index f43fb108..5fa47be8 100644 --- a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts @@ -187,12 +187,14 @@ describe('Credential', () => { it('clear', async () => { expect(Credential.size).toEqual(0); - await Credential.store(makeTestToken()); - await Credential.store(makeTestToken()); - await Credential.store(makeTestToken()); + const c1 = await Credential.store(makeTestToken()); + const c2 = await Credential.store(makeTestToken()); + const c3 = await Credential.store(makeTestToken()); + const disposeSpies = [c1, c2, c3].map(c => jest.spyOn(c, 'dispose')); expect(Credential.size).toEqual(3); await Credential.clear(); expect(Credential.size).toEqual(0); + disposeSpies.forEach(spy => expect(spy).toHaveBeenCalledTimes(1)); }); it('isEqual', async () => { @@ -272,15 +274,21 @@ describe('Credential', () => { it('remove', async () => { const c1 = await Credential.store(makeTestToken()); const c2 = await Credential.store(makeTestToken()); + const c1DisposeSpy = jest.spyOn(c1, 'dispose'); + const c2DisposeSpy = jest.spyOn(c2, 'dispose'); expect(Credential.size).toEqual(2); await c1.remove(); await expect(Credential.with(c1.id)).resolves.toBe(null); expect(Credential.size).toEqual(1); + expect(c1DisposeSpy).toHaveBeenCalledTimes(1); await c1.remove(); // remove c1 again, no ops expect(Credential.size).toEqual(1); + expect(c1DisposeSpy).toHaveBeenCalledTimes(1); // not called again on the no-op + expect(c2DisposeSpy).not.toHaveBeenCalled(); await c2.remove(); await expect(Credential.with(c2.id)).resolves.toBe(null); expect(Credential.size).toEqual(0); + expect(c2DisposeSpy).toHaveBeenCalledTimes(1); }); it('getAuthHeader', async () => { diff --git a/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts b/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts index fe53a559..8b8c6671 100644 --- a/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/DefaultCredentialDataSource.spec.ts @@ -69,20 +69,25 @@ describe('DefaultCredentialDataSource', () => { it('remove', () => { const { c1, dataSrc } = context; + const disposeSpy = jest.spyOn(c1, 'dispose'); expect(dataSrc.size).toEqual(3); expect(dataSrc.hasCredential(c1)).toEqual(true); dataSrc.remove(c1); expect(dataSrc.size).toEqual(2); expect(dataSrc.hasCredential(c1)).toEqual(false); + expect(disposeSpy).toHaveBeenCalledTimes(1); dataSrc.remove(c1); // removing non-existing Credential no-ops expect(dataSrc.size).toEqual(2); + expect(disposeSpy).toHaveBeenCalledTimes(1); // not called again on the no-op }); it('clear', () => { - const { dataSrc } = context; + const { c1, c2, c3, dataSrc } = context; + const disposeSpies = [c1, c2, c3].map(c => jest.spyOn(c, 'dispose')); expect(dataSrc.size).toEqual(3); dataSrc.clear(); expect(dataSrc.size).toEqual(0); + disposeSpies.forEach(spy => expect(spy).toHaveBeenCalledTimes(1)); }); it('size', () => { diff --git a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts index 89af3ed9..501252ab 100644 --- a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts +++ b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts @@ -7,7 +7,6 @@ describe('EventEmitter', () => { const listener2 = jest.fn(); // emit event with no registered handlers at all - // @ts-expect-error `.emit` is protected method emitter.emit('bar', { foo: 'foo '}); // register handlers @@ -15,13 +14,11 @@ describe('EventEmitter', () => { emitter.on('foo', listener2); // emit event with no handlers registered for specific event - // @ts-expect-error `.emit` is protected method emitter.emit('bar', { foo: 'foo '}); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); // emit event with registered handlers - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).toHaveBeenCalledWith({ bar: 'bar' }); expect(listener2).toHaveBeenCalledWith({ bar: 'bar' }); @@ -32,7 +29,6 @@ describe('EventEmitter', () => { listener2.mockClear(); // emit event again (with a single registered handler) - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { baz: 'baz' }); expect(listener1).not.toHaveBeenCalled(); expect(listener2).toHaveBeenCalledWith({ baz: 'baz' }); @@ -41,7 +37,6 @@ describe('EventEmitter', () => { emitter.on('foo', listener1); listener1.mockClear(); listener2.mockClear(); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).toHaveBeenCalledWith({ bar: 'bar' }); expect(listener2).toHaveBeenCalledWith({ bar: 'bar' }); @@ -50,7 +45,6 @@ describe('EventEmitter', () => { listener1.mockClear(); listener2.mockClear(); emitter.off('foo'); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'bar' }); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); @@ -64,11 +58,9 @@ describe('EventEmitter', () => { outer.on('test_event', listener); outer.relay(inner, ['test_event']); - // @ts-expect-error `.emit` is protected method inner.emit('foo', { bar: 'baz' }); expect(listener).not.toHaveBeenCalled(); - // @ts-expect-error `.emit` is protected method inner.emit('test_event', { bar: 'baz' }); expect(listener).toHaveBeenCalledWith({ bar: 'baz' }); }); @@ -90,9 +82,124 @@ describe('EventEmitter', () => { emitter.on('foo', handler1); emitter.on('foo', handler2); - // @ts-expect-error `.emit` is protected method emitter.emit('foo', { bar: 'baz' }); expect(handler2).toHaveBeenCalled(); }); + + describe('AbortSignal support (`{ signal }` option on `.on()`)', () => { + it('removes the listener once the signal is aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).toHaveBeenCalledTimes(1); + + controller.abort(); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).toHaveBeenCalledTimes(1); // not called again after abort + }); + + it('removes every listener sharing the same signal when it is aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener1 = jest.fn(); + const listener2 = jest.fn(); + + emitter.on('foo', listener1, { signal: controller.signal }); + emitter.on('bar', listener2, { signal: controller.signal }); + + controller.abort(); + + emitter.emit('foo', {}); + emitter.emit('bar', {}); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).not.toHaveBeenCalled(); + }); + + it('does not register a listener if the signal is already aborted', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + controller.abort(); + const listener = jest.fn(); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.emit('foo', { bar: 'baz' }); + expect(listener).not.toHaveBeenCalled(); + }); + + it('tears down the abort listener when `.off()` is called directly, not just on abort', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.off('foo', listener); + expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function)); + + // aborting afterward should not throw, nor re-invoke the already-removed listener + expect(() => controller.abort()).not.toThrow(); + emitter.emit('foo', {}); + expect(listener).not.toHaveBeenCalled(); + }); + + it('tears down signal registrations for every handler when `.off(event)` is called with no handler', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener1 = jest.fn(); + const listener2 = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener1, { signal: controller.signal }); + emitter.on('foo', listener2, { signal: controller.signal }); + + emitter.off('foo'); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(2); + + expect(() => controller.abort()).not.toThrow(); + emitter.emit('foo', {}); + expect(listener1).not.toHaveBeenCalled(); + expect(listener2).not.toHaveBeenCalled(); + }); + + it('does not require a `signal` option', () => { + const emitter = new EventEmitter(); + const listener = jest.fn(); + expect(() => emitter.on('foo', listener)).not.toThrow(); + emitter.emit('foo', {}); + expect(listener).toHaveBeenCalledTimes(1); + }); + }); + + describe('clear', () => { + it('removes every listener for every event', () => { + const emitter = new EventEmitter(); + const foo = jest.fn(); + const bar = jest.fn(); + emitter.on('foo', foo); + emitter.on('bar', bar); + + emitter.clear(); + + emitter.emit('foo', {}); + emitter.emit('bar', {}); + expect(foo).not.toHaveBeenCalled(); + expect(bar).not.toHaveBeenCalled(); + }); + + it('tears down any signal registrations for the listeners it clears', () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + const listener = jest.fn(); + const removeEventListenerSpy = jest.spyOn(controller.signal, 'removeEventListener'); + + emitter.on('foo', listener, { signal: controller.signal }); + emitter.clear(); + + expect(removeEventListenerSpy).toHaveBeenCalledWith('abort', expect.any(Function)); + }); + }); }); diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index 9aac2b41..4ddf1b6b 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -137,17 +137,6 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme else if (eventName === 'credential_removed') { log('removal'); this.credentialDataSource.remove(id); - - // // removal messages never carried a token body; `id` is all `hasCredential` actually reads - // const token = new Token({ id } as TokenInit); - // if (this.credentialDataSource.hasCredential(token)) { - // this.credentialDataSource.remove(id); - // } - // else { - // // TODO: is this needed? - // // ensures removal event is broadcast, regardless of the DataSource knowledge of the Credential - // this.emitter.emit('credential_removed', { dataSource: this.credentialDataSource, id }); - // } } else { const token = await this.tokenStorage.get(id); From 8ca2341da596e61c6a2a87df7acb1383962141da Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Tue, 1 Sep 2026 12:19:42 -0400 Subject: [PATCH 05/11] feedback --- .../src/Credential/Credential.ts | 10 +- .../src/Credential/CredentialDataSource.ts | 6 +- .../auth-foundation/src/utils/EventEmitter.ts | 9 +- .../test/spec/utils/EventEmitter.spec.ts | 20 +- .../src/Credential/CredentialCoordinator.ts | 48 ++-- .../src/Credential/TokenStorage.ts | 12 + .../CredentialCoordinatorImpl.spec.ts | 226 ++++++++++++++++++ 7 files changed, 293 insertions(+), 38 deletions(-) create mode 100644 packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts diff --git a/packages/auth-foundation/src/Credential/Credential.ts b/packages/auth-foundation/src/Credential/Credential.ts index 5814559d..3c9c847b 100644 --- a/packages/auth-foundation/src/Credential/Credential.ts +++ b/packages/auth-foundation/src/Credential/Credential.ts @@ -20,7 +20,7 @@ import { CredentialError, OAuth2Error } from '../errors/index.ts'; type CredentialEvents = { - 'credential_added': { credential: Credential }; + 'credential_added': { id: string }; 'credential_removed': { id: string }; 'tags_updated': { id: string, tags: string[] }; } & Omit; @@ -62,7 +62,7 @@ export class Credential implements RequestAuthorizer, JSONSerializable { this.emitter.relay(this.coordinator.emitter, ['cleared', 'default_changed', 'credential_refreshed']); this.coordinator.emitter.on('credential_added', ({ credential }) => { - this.emitter.emit('credential_added', { credential }); + this.emitter.emit('credential_added', { id: credential.id }); }); this.coordinator.emitter.on('credential_removed', ({ id }) => { @@ -334,7 +334,11 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /////// public instances methods /////// /** - * Cleans up resourece associated with the Credential instance to prevent leaks. + * Cleans up resources associated with the Credential instance, so that it may be gargabe collected. + * + * @remarks + * This method is meant to be used in conjunction with {@link CredentialDataSource.remove}. Calling this + * method on an active {@link Credential} may have sigificant consequences */ public dispose () { this.oauth2.dispose(); diff --git a/packages/auth-foundation/src/Credential/CredentialDataSource.ts b/packages/auth-foundation/src/Credential/CredentialDataSource.ts index 665f40fb..36b6ea2f 100644 --- a/packages/auth-foundation/src/Credential/CredentialDataSource.ts +++ b/packages/auth-foundation/src/Credential/CredentialDataSource.ts @@ -33,7 +33,7 @@ export interface CredentialDataSource { * Checks {@link CredentialDataSource} for an existing {@link Credential} instance which * represents the provided {@link Token.Token | Token}. */ - hasCredential (token: Token): boolean; + hasCredential (id: string): boolean; /** * Checks {@link CredentialDataSource} for an existing {@link Credential} instance which * represents the provided {@link Token.Token | Token}. If one does not exist, a new {@link Credential} @@ -80,8 +80,8 @@ export class DefaultCredentialDataSource implements CredentialDataSource { return new this.CredentialConstructor(token, client, metadata); } - public hasCredential (token: Token): boolean { - return this.credentials.has(token.id); + public hasCredential (id: string): boolean { + return this.credentials.has(id); } public credentialFor (token: Token, metadata?: Token.Metadata): Credential { diff --git a/packages/auth-foundation/src/utils/EventEmitter.ts b/packages/auth-foundation/src/utils/EventEmitter.ts index ff5811e8..0fcbf785 100644 --- a/packages/auth-foundation/src/utils/EventEmitter.ts +++ b/packages/auth-foundation/src/utils/EventEmitter.ts @@ -32,12 +32,17 @@ export class EventEmitter { ): this { const { signal } = options; + if (signal?.aborted) { + // if the provided `AbortSignal` has already been aborted, do not bind listener + return this; + } + if (!this.listeners[eventName]) { this.listeners[eventName] = []; } this.listeners[eventName]!.push(handler); - if (signal && !signal?.aborted) { + if (signal) { const abortHandler = () => { this.off(eventName, handler); }; @@ -115,4 +120,4 @@ export class EventEmitter { (Object.keys(this.listeners) as (keyof Events)[]).forEach(eventName => this.off(eventName)); return this; } -} \ No newline at end of file +} diff --git a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts index 501252ab..f070774d 100644 --- a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts +++ b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts @@ -94,11 +94,11 @@ describe('EventEmitter', () => { const listener = jest.fn(); emitter.on('foo', listener, { signal: controller.signal }); - emitter.emit('foo', { bar: 'baz' }); + emitter.emit('foo', { bar: 'baz' }); expect(listener).toHaveBeenCalledTimes(1); controller.abort(); - emitter.emit('foo', { bar: 'baz' }); + emitter.emit('foo', { bar: 'baz' }); expect(listener).toHaveBeenCalledTimes(1); // not called again after abort }); @@ -113,8 +113,8 @@ describe('EventEmitter', () => { controller.abort(); - emitter.emit('foo', {}); - emitter.emit('bar', {}); + emitter.emit('foo', {}); + emitter.emit('bar', {}); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); }); @@ -126,7 +126,7 @@ describe('EventEmitter', () => { const listener = jest.fn(); emitter.on('foo', listener, { signal: controller.signal }); - emitter.emit('foo', { bar: 'baz' }); + emitter.emit('foo', { bar: 'baz' }); expect(listener).not.toHaveBeenCalled(); }); @@ -142,7 +142,7 @@ describe('EventEmitter', () => { // aborting afterward should not throw, nor re-invoke the already-removed listener expect(() => controller.abort()).not.toThrow(); - emitter.emit('foo', {}); + emitter.emit('foo', {}); expect(listener).not.toHaveBeenCalled(); }); @@ -160,7 +160,7 @@ describe('EventEmitter', () => { expect(removeEventListenerSpy).toHaveBeenCalledTimes(2); expect(() => controller.abort()).not.toThrow(); - emitter.emit('foo', {}); + emitter.emit('foo', {}); expect(listener1).not.toHaveBeenCalled(); expect(listener2).not.toHaveBeenCalled(); }); @@ -169,7 +169,7 @@ describe('EventEmitter', () => { const emitter = new EventEmitter(); const listener = jest.fn(); expect(() => emitter.on('foo', listener)).not.toThrow(); - emitter.emit('foo', {}); + emitter.emit('foo', {}); expect(listener).toHaveBeenCalledTimes(1); }); }); @@ -184,8 +184,8 @@ describe('EventEmitter', () => { emitter.clear(); - emitter.emit('foo', {}); - emitter.emit('bar', {}); + emitter.emit('foo', {}); + emitter.emit('bar', {}); expect(foo).not.toHaveBeenCalled(); expect(bar).not.toHaveBeenCalled(); }); diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index 4ddf1b6b..83c16ef6 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -41,8 +41,12 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme private readonly id: string = shortID(); private readonly channel: BroadcastChannel = new BroadcastChannel('CredentialCoordinatorImpl'); + #CredentialEmitter; + constructor (CredentialConstructor: (ConstructorParameters)[0]) { super(CredentialConstructor); + // @ts-expect-error - emitter is protected + this.#CredentialEmitter = CredentialConstructor.emitter; this.tokenStorage = new BrowserTokenStorage(); this.credentialDataSource = new DefaultCredentialDataSource(CredentialConstructor); @@ -134,37 +138,41 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme this.emitter.emit('metadata_updated', { storage: this.tokenStorage, id, metadata }); } } + else if (eventName === 'credential_added') { + log('added'); + this.#CredentialEmitter.emit('credential_added', { id }); + } else if (eventName === 'credential_removed') { log('removal'); this.credentialDataSource.remove(id); } - else { + else if (eventName === 'credential_refreshed') { + log('refresh'); + + // if the tab receiving this event does not "know" (have a corresponding `Credential` instance) + // for the token which refresh, skip processing this event + if (!this.credentialDataSource.hasCredential(id)) { + log('token not known to tab'); + return; + } + const token = await this.tokenStorage.get(id); if (!token) { return; } - const credential = this.credentialDataSource.credentialFor(token); - if (eventName === 'credential_added') { - log('added'); - // credentialDataSource.credentialFor() call above handles updating credDataSrc - } - else if (eventName === 'credential_refreshed') { - log('refresh'); - - // when a Credential is updated in a separate tab, the Token read from storage - // may differ from cred.token via DataSource, so the update should continue. - // If the tokens are equal, this means this DataSource has already updated the token to the new value - if (Token.isEqual(token, credential.token)) { - log('token has already been updated'); - return; - } - - // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API - credential.token = token; - this.emitter.emit('credential_refreshed', { credential }); + // when a Credential is updated in a separate tab, the Token read from storage + // may differ from cred.token via DataSource, so the update should continue. + // If the tokens are equal, this means this DataSource has already updated the token to the new value + if (Token.isEqual(token, credential.token)) { + log('token has already been updated'); + return; } + + // @ts-expect-error - Credential `set token()` is a private setter to avoid exposing this to the public API + credential.token = token; + this.emitter.emit('credential_refreshed', { credential }); } log('allIDs: ', this.allIDs(), 'size: ', this.credentialDataSource.size); diff --git a/packages/spa-platform/src/Credential/TokenStorage.ts b/packages/spa-platform/src/Credential/TokenStorage.ts index 04206eb3..a47d4c9c 100644 --- a/packages/spa-platform/src/Credential/TokenStorage.ts +++ b/packages/spa-platform/src/Credential/TokenStorage.ts @@ -30,6 +30,7 @@ export class BrowserTokenStorage implements TokenStorage { // encryption encryptionKeyStore = new IndexedDBStore('StorageKeys'); encryptionKeyName = 'EncryptionKey'; + #encryptionKey: CryptoKey | null = null; // caches reference to crypto key locally, to reduce frequent DB reads // configurations public includeClaims: boolean = true; // when true, idToken claims are stored in token metadata to use token selection @@ -237,6 +238,7 @@ export class BrowserTokenStorage implements TokenStorage { // if (this.encryptAtRest) { // if no tokens exist in storage, remove the encryption key therefore // it will be rotated once a new token is added + this.#encryptionKey = null; await this.encryptionKeyStore.remove(this.encryptionKeyName); } } @@ -337,6 +339,7 @@ export class BrowserTokenStorage implements TokenStorage { protected async handleReadError (error: unknown, id: string) { // remove token if json structure is malformed localStorage.removeItem(this.idToStoreKey(id)); + this.emitter.emit('token_removed', { storage: this, id }); return null; } @@ -347,10 +350,16 @@ export class BrowserTokenStorage implements TokenStorage { // if token cannot be decrypted, remove it from storage localStorage.removeItem(this.idToStoreKey(id)); await this.removeEncryptionKeyIfEmpty(); + this.emitter.emit('token_removed', { storage: this, id }); return null; } protected async getEncryptionKey (): Promise { + // if local key reference exists, use it + if (this.#encryptionKey) { + return this.#encryptionKey; + } + const encryptionKey = await this.encryptionKeyStore.get(this.encryptionKeyName); if (!encryptionKey) { const newKey = await this.generateEncryptionKey(); @@ -358,6 +367,8 @@ export class BrowserTokenStorage implements TokenStorage { return newKey; } + // cache key reference locally + this.#encryptionKey = encryptionKey; return encryptionKey; } @@ -380,6 +391,7 @@ export class BrowserTokenStorage implements TokenStorage { if ((await this.allIDs()).length === 0) { // if no tokens exist in storage, remove the encryption key therefore // it will be rotated once a new token is added + this.#encryptionKey = null; await this.encryptionKeyStore.remove(this.encryptionKeyName); } } diff --git a/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts new file mode 100644 index 00000000..38c8333a --- /dev/null +++ b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts @@ -0,0 +1,226 @@ +import { CredentialCoordinatorImpl } from 'src/Credential/CredentialCoordinator'; +import { Credential } from 'src/Credential'; +import { BrowserTokenStorage } from 'src/Credential/TokenStorage'; +import { makeTestToken } from '../../helpers/makeTestResource'; + + +// NOTE: this file only covers the platform-specific additions this class layers on top of +// `@okta/auth-foundation`'s base `CredentialCoordinatorImpl` (cross-tab BroadcastChannel sync). +// Behavior inherited unchanged from the base class (store/with/find/remove/clear/getDefault/ +// setDefault/allIDs/expire timeouts) is already covered by auth-foundation's own +// CredentialCoordinatorImpl.spec.ts and isn't re-tested here. +describe('CredentialCoordinatorImpl (spa-platform)', () => { + let cc: CredentialCoordinatorImpl; + let channel: any; + + // simulates an incoming cross-tab BroadcastChannel message, since the mocked BroadcastChannel + // (tooling/jest-helpers/browser/jest.setup.ts) never actually delivers `postMessage` calls anywhere + function receive (eventName: string, data: Record = {}, source = 'other-tab') { + return channel.onmessage({ data: { eventName, source, ...data } }); + } + + beforeEach(() => { + // required to prevent open handles: `store()` creates an expiration timer (inherited from the base class) + jest.useFakeTimers(); + cc = new CredentialCoordinatorImpl(Credential); + channel = (cc as any).channel; + }); + + afterEach(() => { + jest.clearAllTimers(); + }); + + describe('instantiate', () => { + it('should construct', () => { + expect(cc).toBeInstanceOf(CredentialCoordinatorImpl); + }); + }); + + describe('broadcasting', () => { + it('broadcasts only the id (not the token body) when a token is added', async () => { + const cred = await cc.store(makeTestToken()); + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_added', + source: expect.any(String), + id: cred.id + }); + }); + + it('broadcasts only the id when a credential is refreshed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + jest.spyOn(cred.oauth2, 'refresh').mockResolvedValue(makeTestToken(cred.id)); + await cred.refresh(); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_refreshed', + source: expect.any(String), + id: cred.id + }); + }); + + it('broadcasts credential_removed when a credential is removed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + await cc.remove(cred); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_removed', + source: expect.any(String), + id: cred.id + }); + }); + + it('broadcasts default_changed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + await cc.setDefault(cred); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'default_changed', + source: expect.any(String), + id: cred.id + }); + }); + + it('broadcasts metadata_updated', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + await cred.setTags(['foo']); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'metadata_updated', + source: expect.any(String), + id: cred.id + }); + }); + + it('broadcasts cleared when clear() is called without localOnly', async () => { + await cc.clear(); + expect(channel.postMessage).toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); + }); + + it('does not broadcast cleared when clear(true) (localOnly) is called', async () => { + await cc.clear(true); + expect(channel.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ eventName: 'cleared' })); + }); + + it('detaches broadcast listeners from a replaced tokenStorage', () => { + const oldStorage = cc.tokenStorage; + cc.tokenStorage = new BrowserTokenStorage(); + channel.postMessage.mockClear(); + + oldStorage.emitter.emit('token_added', { storage: oldStorage, id: 'foo', token: makeTestToken() }); + + expect(channel.postMessage).not.toHaveBeenCalled(); + }); + }); + + describe('receiving cross-tab messages', () => { + it('ignores messages broadcast by itself', async () => { + const ownId = (cc as any).id; + const addedSpy = jest.fn(); + cc.emitter.on('credential_added', addedSpy); + + await receive('credential_added', { id: 'foo' }, ownId); + + expect(addedSpy).not.toHaveBeenCalled(); + }); + + it('constructs a credential from storage when credential_added is received, not from the message', async () => { + const token = makeTestToken(); + await cc.tokenStorage.add(token); // simulates another tab having already written to shared storage + expect(cc.credentialDataSource.hasCredential(token)).toBe(false); + + await receive('credential_added', { id: token.id }); + + expect(cc.credentialDataSource.hasCredential(token)).toBe(true); + }); + + it('does nothing if the token no longer exists in storage when credential_added is received', async () => { + const credentialForSpy = jest.spyOn(cc.credentialDataSource, 'credentialFor'); + await receive('credential_added', { id: 'does-not-exist' }); + expect(credentialForSpy).not.toHaveBeenCalled(); + }); + + it('applies the freshly stored token and emits credential_refreshed when tokens differ', async () => { + const cred = await cc.store(makeTestToken()); + const refreshed = makeTestToken(cred.id); + await cc.tokenStorage.replace(cred.id, refreshed); // simulates another tab's refresh landing in shared storage + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); + + await receive('credential_refreshed', { id: cred.id }); + + expect(cred.token).toEqual(refreshed); + expect(refreshedSpy).toHaveBeenCalledWith({ credential: cred }); + }); + + it('does not re-emit credential_refreshed if the stored token already matches', async () => { + const cred = await cc.store(makeTestToken()); + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); + + await receive('credential_refreshed', { id: cred.id }); // storage already matches cred.token + + expect(refreshedSpy).not.toHaveBeenCalled(); + }); + + it('removes the local credential when credential_removed is received', async () => { + const cred = await cc.store(makeTestToken()); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); + + await receive('credential_removed', { id: cred.id }); + + expect(cc.credentialDataSource.hasCredential(cred)).toBe(false); + }); + + it('is a no-op removing an id this tab never cached', async () => { + await expect(receive('credential_removed', { id: 'never-seen' })).resolves.not.toThrow(); + }); + + it('applies default_changed from another tab', async () => { + const cred = await cc.store(makeTestToken()); + const defaultChangedSpy = jest.fn(); + cc.emitter.on('default_changed', defaultChangedSpy); + + await receive('default_changed', { id: cred.id }); + + expect(defaultChangedSpy).toHaveBeenCalledWith({ storage: cc.tokenStorage, id: cred.id }); + }); + + it('applies metadata_updated from another tab by reading fresh metadata from storage', async () => { + const cred = await cc.store(makeTestToken(), ['foo']); + const metadataSpy = jest.fn(); + cc.emitter.on('metadata_updated', metadataSpy); + + await receive('metadata_updated', { id: cred.id }); + + expect(metadataSpy).toHaveBeenCalledWith(expect.objectContaining({ id: cred.id })); + }); + + it('applies cleared from another tab without re-broadcasting', async () => { + await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + const clearedSpy = jest.fn(); + cc.emitter.on('cleared', clearedSpy); + + await receive('cleared'); + + expect(cc.size).toEqual(0); + expect(clearedSpy).toHaveBeenCalled(); + expect(channel.postMessage).not.toHaveBeenCalled(); + }); + }); + + describe('close', () => { + it('closes the underlying BroadcastChannel', () => { + cc.close(); + expect(channel.close).toHaveBeenCalledTimes(1); + }); + }); +}); \ No newline at end of file From 1fe4e413f7a59af856d1a0dbf27e269b29613556 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Tue, 1 Sep 2026 15:37:25 -0400 Subject: [PATCH 06/11] event rework --- e2e/apps/redirect-model/src/component/Token.tsx | 7 ------- e2e/apps/redirect-model/src/main.tsx | 8 ++++++++ .../auth-foundation/src/Credential/Credential.ts | 16 ++++------------ .../src/Credential/CredentialCoordinator.ts | 10 ++++++---- .../src/Credential/CredentialCoordinator.ts | 16 ++++++++++------ 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/e2e/apps/redirect-model/src/component/Token.tsx b/e2e/apps/redirect-model/src/component/Token.tsx index 09f7c638..8092c6ef 100644 --- a/e2e/apps/redirect-model/src/component/Token.tsx +++ b/e2e/apps/redirect-model/src/component/Token.tsx @@ -12,9 +12,7 @@ export function Token ({ credential }: { credential: Credential }) { Credential.on('credential_refreshed', handler); const tagsHandler = ({ id, tags }) => { - console.log('tags updated: ', id, tags); if (id === credential.id) { - console.log('setTags called') setTags(tags); } } @@ -29,11 +27,6 @@ export function Token ({ credential }: { credential: Credential }) { }; }, [credential, setToken, setTags]); - // useEffect(() => { - // setToken(credential.token); - // setTags(credential.tags); - // }, [credential]); - const remove = async () => { await credential.remove(); }; diff --git a/e2e/apps/redirect-model/src/main.tsx b/e2e/apps/redirect-model/src/main.tsx index ba51b062..1e8e30a0 100644 --- a/e2e/apps/redirect-model/src/main.tsx +++ b/e2e/apps/redirect-model/src/main.tsx @@ -22,6 +22,14 @@ if (!rootElement) { // @ts-expect-error - This is added for e2e purposes only, not recommended for production apps window.Credential = Credential; +// @ts-ignore +window.leakTest = () => { + Credential.allIDs() + .then(ids => { + console.log('allIds.length: ', ids.length); + console.log('Coordinator Emitter Listeners', Credential.coordinator.emitter.listeners?.metadata_updated?.length); + }); +} const root = createRoot(rootElement); root.render( diff --git a/packages/auth-foundation/src/Credential/Credential.ts b/packages/auth-foundation/src/Credential/Credential.ts index 3c9c847b..de53018b 100644 --- a/packages/auth-foundation/src/Credential/Credential.ts +++ b/packages/auth-foundation/src/Credential/Credential.ts @@ -20,10 +20,8 @@ import { CredentialError, OAuth2Error } from '../errors/index.ts'; type CredentialEvents = { - 'credential_added': { id: string }; - 'credential_removed': { id: string }; 'tags_updated': { id: string, tags: string[] }; -} & Omit; +} & CredentialCoordinatorEvents; /** * Wrapper around a {@link Token.Token | Token}, providing methods to interact with Tokens without the hassle of managing them @@ -59,15 +57,9 @@ export class Credential implements RequestAuthorizer, JSONSerializable { ).forEach((evt) => previousCoordinator.emitter.off(evt)); // binds listeners (and event relays) from coordinator to Credential.emitter - this.emitter.relay(this.coordinator.emitter, ['cleared', 'default_changed', 'credential_refreshed']); - - this.coordinator.emitter.on('credential_added', ({ credential }) => { - this.emitter.emit('credential_added', { id: credential.id }); - }); - - this.coordinator.emitter.on('credential_removed', ({ id }) => { - this.emitter.emit('credential_removed', { id }); - }); + this.emitter.relay(this.coordinator.emitter, [ + 'credential_added', 'credential_removed', 'cleared', 'default_changed', 'credential_refreshed' + ]); this.coordinator.emitter.on('metadata_updated', async ({ id, metadata }) => { this.emitter.emit('tags_updated', { id, tags: metadata?.tags ?? [] }); diff --git a/packages/auth-foundation/src/Credential/CredentialCoordinator.ts b/packages/auth-foundation/src/Credential/CredentialCoordinator.ts index d5c82be2..cd0cb787 100644 --- a/packages/auth-foundation/src/Credential/CredentialCoordinator.ts +++ b/packages/auth-foundation/src/Credential/CredentialCoordinator.ts @@ -24,12 +24,13 @@ function log (...args: any[]) {} export type CredentialCoordinatorEvents = { + 'credential_added': { credential?: Credential, id: string }; + 'credential_removed': { id: string } 'credential_expired': { credential: Credential }; 'credential_refreshed': { credential: Credential }; 'cleared': void; } -& Pick -& CredentialDataSourceEvents; +& Pick; /** * @public @interface @@ -138,13 +139,14 @@ export class CredentialCoordinatorImpl implements CredentialCoordinator { console.error('Failed to replace token after refresh'); } }); + + this.emitter.emit('credential_added', { credential, id: credential.id }); }); this.credentialDataSource.emitter.on('credential_removed', ({ id }) => { this.clearExpireEventTimeout(id); + this.emitter.emit('credential_removed', { id }); }); - - this.emitter.relay(this.credentialDataSource.emitter, ['credential_added', 'credential_removed']); } public get tokenStorage (): TokenStorage { diff --git a/packages/spa-platform/src/Credential/CredentialCoordinator.ts b/packages/spa-platform/src/Credential/CredentialCoordinator.ts index 83c16ef6..cb3d8d9a 100644 --- a/packages/spa-platform/src/Credential/CredentialCoordinator.ts +++ b/packages/spa-platform/src/Credential/CredentialCoordinator.ts @@ -41,12 +41,8 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme private readonly id: string = shortID(); private readonly channel: BroadcastChannel = new BroadcastChannel('CredentialCoordinatorImpl'); - #CredentialEmitter; - constructor (CredentialConstructor: (ConstructorParameters)[0]) { super(CredentialConstructor); - // @ts-expect-error - emitter is protected - this.#CredentialEmitter = CredentialConstructor.emitter; this.tokenStorage = new BrowserTokenStorage(); this.credentialDataSource = new DefaultCredentialDataSource(CredentialConstructor); @@ -140,11 +136,19 @@ export class CredentialCoordinatorImpl extends CredentialCoordinatorBase impleme } else if (eventName === 'credential_added') { log('added'); - this.#CredentialEmitter.emit('credential_added', { id }); + this.emitter.emit('credential_added', { id }); + // NOTE: cross-tab 'credential_added' no longer defaults to adding a `Credential` instance to `dataSource` } else if (eventName === 'credential_removed') { log('removal'); - this.credentialDataSource.remove(id); + if (this.credentialDataSource.hasCredential(id)) { + // if a `Credential` exists for the given token, a event will be relayed via `dataSource.emitter` + this.credentialDataSource.remove(id); + } + else { + // No event will be relayed if a `Credential` exists does not exist, emit one directly + this.emitter.emit('credential_removed', { id }); + } } else if (eventName === 'credential_refreshed') { log('refresh'); From 86562c93fc4f0c41654f2bf31b7c38ed99a0e75d Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Wed, 2 Sep 2026 09:26:36 -0400 Subject: [PATCH 07/11] fixes EventEmitter feedback --- .../auth-foundation/src/utils/EventEmitter.ts | 22 +++++++++----- .../test/spec/utils/EventEmitter.spec.ts | 29 +++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/packages/auth-foundation/src/utils/EventEmitter.ts b/packages/auth-foundation/src/utils/EventEmitter.ts index 0fcbf785..d5130766 100644 --- a/packages/auth-foundation/src/utils/EventEmitter.ts +++ b/packages/auth-foundation/src/utils/EventEmitter.ts @@ -23,7 +23,9 @@ export interface Emitter { */ export class EventEmitter { listeners: { [K in keyof Events]?: Array> } = {}; - signals: WeakMap<(...arg: any[]) => void, { signal: AbortSignal, abortHandler: () => void }> = new WeakMap(); + // scoped per-event, since the same `handler` function reference may be registered against + // multiple events (or reused across `on()` calls) with different signals attached + signals: Map void, { signal: AbortSignal, abortHandler: () => void }>> = new Map(); on( eventName: K, @@ -47,7 +49,11 @@ export class EventEmitter { this.off(eventName, handler); }; signal.addEventListener('abort', abortHandler, { once: true }); - this.signals.set(handler, { signal, abortHandler }); + + if (!this.signals.has(eventName)) { + this.signals.set(eventName, new WeakMap()); + } + this.signals.get(eventName)!.set(handler, { signal, abortHandler }); } return this; @@ -59,22 +65,22 @@ export class EventEmitter { } if (!handler) { - this.listeners[eventName]?.forEach(h => this.detachSignal(h)); + this.listeners[eventName]?.forEach(h => this.detachSignal(eventName, h)); delete this.listeners[eventName]; return this; } - this.detachSignal(handler); + this.detachSignal(eventName, handler); this.listeners[eventName] = this.listeners[eventName]?.filter(l => l !== handler); return this; } - /** @internal removes the `AbortSignal` registration (if any) associated with `handler` */ - protected detachSignal (handler: EventListener): void { - const entry = this.signals.get(handler); + /** @internal removes the `AbortSignal` registration (if any) associated with `handler` for `eventName` */ + protected detachSignal (eventName: K, handler: EventListener): void { + const entry = this.signals.get(eventName)?.get(handler); if (entry) { entry.signal.removeEventListener('abort', entry.abortHandler); - this.signals.delete(handler); + this.signals.get(eventName)!.delete(handler); } } diff --git a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts index f070774d..0ef4d251 100644 --- a/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts +++ b/packages/auth-foundation/test/spec/utils/EventEmitter.spec.ts @@ -172,6 +172,35 @@ describe('EventEmitter', () => { emitter.emit('foo', {}); expect(listener).toHaveBeenCalledTimes(1); }); + + it('does not cross-contaminate signal cleanup when the same handler is shared across different events', () => { + const emitter = new EventEmitter(); + const controllerA = new AbortController(); + const controllerB = new AbortController(); + const sharedHandler = jest.fn(); + const removeEventListenerSpyA = jest.spyOn(controllerA.signal, 'removeEventListener'); + const removeEventListenerSpyB = jest.spyOn(controllerB.signal, 'removeEventListener'); + + emitter.on('foo', sharedHandler, { signal: controllerA.signal }); + emitter.on('bar', sharedHandler, { signal: controllerB.signal }); + + // explicitly detach only the 'foo' registration + emitter.off('foo', sharedHandler); + expect(removeEventListenerSpyA).toHaveBeenCalledWith('abort', expect.any(Function)); + expect(removeEventListenerSpyB).not.toHaveBeenCalled(); // 'bar's own signal registration must be untouched + + // 'bar' should still be live... + emitter.emit('bar', {}); + expect(sharedHandler).toHaveBeenCalledTimes(1); + + // ...and still correctly cleaned up when ITS OWN signal aborts + controllerB.abort(); + emitter.emit('bar', {}); + expect(sharedHandler).toHaveBeenCalledTimes(1); // not called again + + // aborting the already-detached controllerA afterward should not throw or double-invoke anything + expect(() => controllerA.abort()).not.toThrow(); + }); }); describe('clear', () => { From 160c0fa3c6b1ac676fb645868c04e8dc506a99c2 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Wed, 2 Sep 2026 16:18:36 -0400 Subject: [PATCH 08/11] clean up + adds tests --- CHANGELOG.md | 17 ++ package.json | 2 +- packages/auth-foundation/package.json | 2 +- .../src/Credential/CredentialDataSource.ts | 5 +- .../test/spec/Credential/Credential.spec.ts | 63 +++-- .../CredentialCoordinatorImpl.spec.ts | 6 +- packages/oauth2-flows/package.json | 2 +- packages/spa-platform/package.json | 2 +- .../src/Credential/TokenStorage.ts | 10 - .../test/spec/BrowserTokenStorage.spec.ts | 17 +- .../CredentialCoordinatorImpl.spec.ts | 267 +++++++++--------- 11 files changed, 205 insertions(+), 188 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27a9cbb7..218ff79e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ All notable changes to this project will be documented in this file. +## [0.8.0] - 2026-09-02 + +### `@okta/auth-foundation` + +#### Added +- Added `dispose()` to `Credential`, `OAuth2Client`, and `APIClient` to release listeners and cached resources when a credential is removed ([#39](https://github.com/okta/okta-client-javascript/pull/39)) +- Added an optional `{ signal: AbortSignal }` option to `EventEmitter.on()`, and a `clear()` method, for automatic listener cleanup ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + +#### Fixed +- Fixed a memory leak where every constructed `Credential` added a listener to the shared `CredentialCoordinator` emitter that was never removed, retaining every `Credential` (and its `OAuth2Client`) for the lifetime of the page ([#39](https://github.com/okta/okta-client-javascript/pull/39)) +- `DefaultCredentialDataSource.remove()`/`.clear()` now dispose removed credentials instead of only removing them from the internal cache ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + +### `@okta/spa-platform` + +#### Fixed +- Cross-tab credential sync no longer broadcasts full token payloads over `BroadcastChannel`; tabs now read the current value from storage, and only when they already reference the credential in question, reducing memory pressure across many open tabs ([#39](https://github.com/okta/okta-client-javascript/pull/39)) + ## [0.7.2] - 2026-04-09 ### `@okta/spa-platform` diff --git a/package.json b/package.json index 375a76e4..f3996508 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@okta/okta-client-js", - "version": "0.7.2", + "version": "0.8.0", "private": true, "packageManager": "yarn@1.22.19", "engines": { diff --git a/packages/auth-foundation/package.json b/packages/auth-foundation/package.json index 0735a817..66b1daee 100644 --- a/packages/auth-foundation/package.json +++ b/packages/auth-foundation/package.json @@ -1,6 +1,6 @@ { "name": "@okta/auth-foundation", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/auth-foundation/src/Credential/CredentialDataSource.ts b/packages/auth-foundation/src/Credential/CredentialDataSource.ts index 36b6ea2f..d3e94b4b 100644 --- a/packages/auth-foundation/src/Credential/CredentialDataSource.ts +++ b/packages/auth-foundation/src/Credential/CredentialDataSource.ts @@ -33,6 +33,7 @@ export interface CredentialDataSource { * Checks {@link CredentialDataSource} for an existing {@link Credential} instance which * represents the provided {@link Token.Token | Token}. */ + hasCredential (token: Token): boolean; hasCredential (id: string): boolean; /** * Checks {@link CredentialDataSource} for an existing {@link Credential} instance which @@ -80,8 +81,8 @@ export class DefaultCredentialDataSource implements CredentialDataSource { return new this.CredentialConstructor(token, client, metadata); } - public hasCredential (id: string): boolean { - return this.credentials.has(id); + public hasCredential (key: string | Token): boolean { + return this.credentials.has(typeof key === 'string' ? key : key.id); } public credentialFor (token: Token, metadata?: Token.Metadata): Credential { diff --git a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts index 5fa47be8..ea7e03b9 100644 --- a/packages/auth-foundation/test/spec/Credential/Credential.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/Credential.spec.ts @@ -38,8 +38,8 @@ describe('Credential', () => { const token = makeTestToken(); let cred = await Credential.store(token); - expect(onAdded1).toHaveBeenNthCalledWith(1, { credential: cred }); - expect(onAdded2).toHaveBeenNthCalledWith(1, { credential: cred }); + expect(onAdded1).toHaveBeenNthCalledWith(1, { credential: cred, id: cred.id }); + expect(onAdded2).toHaveBeenNthCalledWith(1, { credential: cred, id: cred.id }); expect(onRemoved1).toHaveBeenCalledTimes(0); expect(onRemoved2).toHaveBeenCalledTimes(0); await cred.remove(); @@ -58,37 +58,40 @@ describe('Credential', () => { expect(onRemoved2).toHaveBeenNthCalledWith(2, { id: cred.id }); }); - // regression test for the leaked `observeToken()` -> `coordinator.emitter.on('metadata_updated', ...)` - // subscription: every constructed Credential added one more listener to the page-lifetime coordinator - // emitter, and nothing ever removed it, even after `.remove()` - it('should not accumulate metadata_updated listeners on the coordinator emitter across store/remove cycles', async () => { - const emitter = (Credential as any).coordinator.emitter; - const baseline = emitter.listeners['metadata_updated']?.length ?? 0; + // OKTA-1262864 + describe('Memory leak regression', () => { + // leak: every constructed Credential added one more listener to the page-lifetime coordinator emitter, and nothing ever removed it. + // fix/test: Now, only a single listener should be bound to `metadata_updated`, independent of the number of constructed Credentials - for (let i = 0; i < 25; i++) { - const cred = await Credential.store(makeTestToken()); - await cred.remove(); - } + // confirms `metadata_updated` listeners does not grow as Credentials are constructed + it('should not accumulate `metadata_updated` listeners on the coordinator emitter across store/remove cycles', async () => { + const emitter = (Credential as any).coordinator.emitter; + const baseline = emitter.listeners['metadata_updated']?.length ?? 0; - expect(emitter.listeners['metadata_updated']?.length ?? 0).toBe(baseline); - }); + for (let i = 0; i < 25; i++) { + const cred = await Credential.store(makeTestToken()); + await cred.remove(); + } + + expect(emitter.listeners['metadata_updated']?.length ?? 0).toBe(baseline); + }); - // target behavior for the planned fix: `metadata_updated` should be bound exactly once, from the - // static `coordinator` setter, and cleanly rebound (not duplicated, not left dangling) on reassignment - it('binds exactly one metadata_updated listener via the coordinator setter, and unbinds the previous one on reassignment', () => { - const previousCoordinator = (Credential as any).coordinator; - const newCoordinator = new previousCoordinator.constructor(Credential); - - (Credential as any).coordinator = newCoordinator; - - try { - expect(previousCoordinator.emitter.listeners['metadata_updated']).toBeFalsy(); - expect(newCoordinator.emitter.listeners['metadata_updated']?.length).toBe(1); - } - finally { - // restore original coordinator so later tests aren't affected - (Credential as any).coordinator = previousCoordinator; - } + // confirms the single `metadata_updated` listener is maintained when updating `coordinator` instance (coordinator setter) + it('binds exactly one metadata_updated listener via the coordinator setter during reassignment', () => { + const previousCoordinator = (Credential as any).coordinator; + const newCoordinator = new previousCoordinator.constructor(Credential); + + (Credential as any).coordinator = newCoordinator; + + try { + expect(previousCoordinator.emitter.listeners['metadata_updated']).toBeFalsy(); + expect(newCoordinator.emitter.listeners['metadata_updated']?.length).toBe(1); + } + finally { + // restore original coordinator so later tests aren't affected + (Credential as any).coordinator = previousCoordinator; + } + }); }); }); diff --git a/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts b/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts index 676f1946..88b58497 100644 --- a/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts +++ b/packages/auth-foundation/test/spec/Credential/CredentialCoordinatorImpl.spec.ts @@ -119,7 +119,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.tokenStorage.defaultTokenId).toEqual(cred.id); expect(cred.tags).toEqual(['test']); expect(onAdded).toHaveBeenCalledTimes(1); - expect(onAdded).toHaveBeenCalledWith({ credential: cred, dataSource: cc.credentialDataSource }); + expect(onAdded).toHaveBeenCalledWith({ credential: cred, id: cred.id }); }); it('with', async () => { @@ -173,7 +173,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.credentialDataSource.hasCredential(c1)).toEqual(false); expect(cc.size).toEqual(3); expect(clearExpireTimeoutSpy).toHaveBeenNthCalledWith(1, c1.id); - expect(onRemove).toHaveBeenNthCalledWith(1, { id: c1.id, dataSource: cc.credentialDataSource }); + expect(onRemove).toHaveBeenNthCalledWith(1, { id: c1.id }); expect(onDefaultChanged).toHaveBeenCalledTimes(0); await cc.remove(c2); // remove default credenital @@ -181,7 +181,7 @@ describe('CredentialCoordinatorImpl', () => { expect(cc.credentialDataSource.hasCredential(c2)).toEqual(false); expect(cc.size).toEqual(2); expect(clearExpireTimeoutSpy).toHaveBeenNthCalledWith(2, c2.id); - expect(onRemove).toHaveBeenNthCalledWith(2, { id: c2.id, dataSource: cc.credentialDataSource }); + expect(onRemove).toHaveBeenNthCalledWith(2, { id: c2.id }); expect(onDefaultChanged).toHaveBeenNthCalledWith(1, { id: null, storage: cc.tokenStorage }); }); diff --git a/packages/oauth2-flows/package.json b/packages/oauth2-flows/package.json index 72b47f3a..5f113670 100644 --- a/packages/oauth2-flows/package.json +++ b/packages/oauth2-flows/package.json @@ -1,6 +1,6 @@ { "name": "@okta/oauth2-flows", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/spa-platform/package.json b/packages/spa-platform/package.json index 1af964c1..d0d9c980 100644 --- a/packages/spa-platform/package.json +++ b/packages/spa-platform/package.json @@ -1,6 +1,6 @@ { "name": "@okta/spa-platform", - "version": "0.7.2", + "version": "0.8.0", "type": "module", "main": "dist/esm/index.js", "module": "dist/esm/index.js", diff --git a/packages/spa-platform/src/Credential/TokenStorage.ts b/packages/spa-platform/src/Credential/TokenStorage.ts index a47d4c9c..8192acf1 100644 --- a/packages/spa-platform/src/Credential/TokenStorage.ts +++ b/packages/spa-platform/src/Credential/TokenStorage.ts @@ -30,7 +30,6 @@ export class BrowserTokenStorage implements TokenStorage { // encryption encryptionKeyStore = new IndexedDBStore('StorageKeys'); encryptionKeyName = 'EncryptionKey'; - #encryptionKey: CryptoKey | null = null; // caches reference to crypto key locally, to reduce frequent DB reads // configurations public includeClaims: boolean = true; // when true, idToken claims are stored in token metadata to use token selection @@ -238,7 +237,6 @@ export class BrowserTokenStorage implements TokenStorage { // if (this.encryptAtRest) { // if no tokens exist in storage, remove the encryption key therefore // it will be rotated once a new token is added - this.#encryptionKey = null; await this.encryptionKeyStore.remove(this.encryptionKeyName); } } @@ -355,11 +353,6 @@ export class BrowserTokenStorage implements TokenStorage { } protected async getEncryptionKey (): Promise { - // if local key reference exists, use it - if (this.#encryptionKey) { - return this.#encryptionKey; - } - const encryptionKey = await this.encryptionKeyStore.get(this.encryptionKeyName); if (!encryptionKey) { const newKey = await this.generateEncryptionKey(); @@ -367,8 +360,6 @@ export class BrowserTokenStorage implements TokenStorage { return newKey; } - // cache key reference locally - this.#encryptionKey = encryptionKey; return encryptionKey; } @@ -391,7 +382,6 @@ export class BrowserTokenStorage implements TokenStorage { if ((await this.allIDs()).length === 0) { // if no tokens exist in storage, remove the encryption key therefore // it will be rotated once a new token is added - this.#encryptionKey = null; await this.encryptionKeyStore.remove(this.encryptionKeyName); } } diff --git a/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts b/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts index d97f667e..17de4ca0 100644 --- a/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts +++ b/packages/spa-platform/test/spec/BrowserTokenStorage.spec.ts @@ -1,4 +1,4 @@ -import { Token, CredentialError } from '@okta/auth-foundation'; +import { Token, CredentialError, randomBytes } from '@okta/auth-foundation'; import { BrowserTokenStorage } from 'src/Credential/TokenStorage'; import { makeTestToken, MockIndexedDBStore } from '../helpers/makeTestResource'; @@ -210,6 +210,9 @@ describe('BrowserTokenStorage', () => { }); // NOTE: potentially flaky test + // generating test tokens via `makeTestToken(randomBytes())` seem to help with the flakiness. + // `token.id` is used as the "iv" in `.encrypt({ name: 'AES-GCM', iv: buf(iv) }, ...)` calls + // it seems 'AES-GCM' "typically expects a IV of exactly 12 bytes", the default `shortId()` was not it('encrypts and decrypts tokens in/out of storage', async () => { const expectedKey = { type: 'secret', @@ -220,13 +223,13 @@ describe('BrowserTokenStorage', () => { await expect(storage.encryptionKeyStore.get(storage.encryptionKeyName)).resolves.toBe(null); - const t1 = makeTestToken(); + const t1 = makeTestToken(randomBytes()); await storage.add(t1); // cannot assert .instanceOf(CryptoKey) - Jest throws 'CryptoKey' not defined await expect(storage.encryptionKeyStore.get(storage.encryptionKeyName)).resolves.toMatchObject(expectedKey); - const t2 = makeTestToken(); + const t2 = makeTestToken(randomBytes()); await storage.add(t2); const t1Stored = JSON.parse(localStorage.getItem((storage as any).idToStoreKey(t1.id))!).token; @@ -244,14 +247,14 @@ describe('BrowserTokenStorage', () => { }); it('can gracefully handle `encryptedAtRest` flag being toggled', async () => { - const encryptedToken = makeTestToken(); + const encryptedToken = makeTestToken(randomBytes()); await storage.add(encryptedToken); await expect(storage.get(encryptedToken.id)).resolves.toEqual(encryptedToken); storage.encryptAtRest = false; await expect(storage.get(encryptedToken.id)).resolves.toEqual(encryptedToken); - const unencryptedToken = makeTestToken(); + const unencryptedToken = makeTestToken(randomBytes()); await storage.add(unencryptedToken); await expect(storage.get(unencryptedToken.id)).resolves.toEqual(unencryptedToken); @@ -267,8 +270,8 @@ describe('BrowserTokenStorage', () => { await expect(storage.get(unencryptedToken.id)).resolves.toEqual(unencryptedToken); }); - it('removes token from storage when decryption fails, is found', async () => { - const token = makeTestToken(); + it('removes token from storage when decryption fails', async () => { + const token = makeTestToken(randomBytes()); await storage.add(token); await expect(storage.allIDs()).resolves.toEqual([token.id]); diff --git a/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts index 38c8333a..5dc22346 100644 --- a/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts +++ b/packages/spa-platform/test/spec/Credential/CredentialCoordinatorImpl.spec.ts @@ -4,17 +4,11 @@ import { BrowserTokenStorage } from 'src/Credential/TokenStorage'; import { makeTestToken } from '../../helpers/makeTestResource'; -// NOTE: this file only covers the platform-specific additions this class layers on top of -// `@okta/auth-foundation`'s base `CredentialCoordinatorImpl` (cross-tab BroadcastChannel sync). -// Behavior inherited unchanged from the base class (store/with/find/remove/clear/getDefault/ -// setDefault/allIDs/expire timeouts) is already covered by auth-foundation's own -// CredentialCoordinatorImpl.spec.ts and isn't re-tested here. -describe('CredentialCoordinatorImpl (spa-platform)', () => { +describe('CredentialCoordinatorImpl', () => { let cc: CredentialCoordinatorImpl; let channel: any; - // simulates an incoming cross-tab BroadcastChannel message, since the mocked BroadcastChannel - // (tooling/jest-helpers/browser/jest.setup.ts) never actually delivers `postMessage` calls anywhere + // simulates an incoming cross-tab BroadcastChannel message, since BroadcastChannel is mocked function receive (eventName: string, data: Record = {}, source = 'other-tab') { return channel.onmessage({ data: { eventName, source, ...data } }); } @@ -23,90 +17,96 @@ describe('CredentialCoordinatorImpl (spa-platform)', () => { // required to prevent open handles: `store()` creates an expiration timer (inherited from the base class) jest.useFakeTimers(); cc = new CredentialCoordinatorImpl(Credential); + Credential.coordinator = cc; channel = (cc as any).channel; + (cc.tokenStorage as BrowserTokenStorage).encryptAtRest = false; // disables crypto/indexedDB requirements }); afterEach(() => { jest.clearAllTimers(); }); - describe('instantiate', () => { + describe('Instantiate', () => { it('should construct', () => { expect(cc).toBeInstanceOf(CredentialCoordinatorImpl); }); }); - describe('broadcasting', () => { - it('broadcasts only the id (not the token body) when a token is added', async () => { - const cred = await cc.store(makeTestToken()); - expect(channel.postMessage).toHaveBeenCalledWith({ - eventName: 'credential_added', - source: expect.any(String), - id: cred.id + describe('Broadcast local Credential* events cross-tab', () => { + describe('Events', () => { + test('credential_added', async () => { + const cred = await cc.store(makeTestToken()); + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_added', + source: expect.any(String), + id: cred.id + }); }); - }); - - it('broadcasts only the id when a credential is refreshed', async () => { - const cred = await cc.store(makeTestToken()); - channel.postMessage.mockClear(); - jest.spyOn(cred.oauth2, 'refresh').mockResolvedValue(makeTestToken(cred.id)); - await cred.refresh(); - - expect(channel.postMessage).toHaveBeenCalledWith({ - eventName: 'credential_refreshed', - source: expect.any(String), - id: cred.id + test('credential_refreshed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + + const newToken = makeTestToken(cred.id); + jest.spyOn(cred.oauth2, 'refresh').mockResolvedValue(newToken); + // mocking `oauth2.refresh` means the `token_did_refresh` is not fired, emitting manually for test + cred.oauth2.emitter.emit('token_did_refresh', { token: newToken }); + await cred.refresh(); + + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_refreshed', + source: expect.any(String), + id: cred.id + }); }); - }); - it('broadcasts credential_removed when a credential is removed', async () => { - const cred = await cc.store(makeTestToken()); - channel.postMessage.mockClear(); + test('credential_removed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); - await cc.remove(cred); + await cc.remove(cred); - expect(channel.postMessage).toHaveBeenCalledWith({ - eventName: 'credential_removed', - source: expect.any(String), - id: cred.id + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'credential_removed', + source: expect.any(String), + id: cred.id + }); }); - }); - it('broadcasts default_changed', async () => { - const cred = await cc.store(makeTestToken()); - channel.postMessage.mockClear(); + test('default_changed', async () => { + const cred = await cc.store(makeTestToken()); + channel.postMessage.mockClear(); - await cc.setDefault(cred); + await cc.setDefault(cred); - expect(channel.postMessage).toHaveBeenCalledWith({ - eventName: 'default_changed', - source: expect.any(String), - id: cred.id + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'default_changed', + source: expect.any(String), + id: cred.id + }); }); - }); - it('broadcasts metadata_updated', async () => { - const cred = await cc.store(makeTestToken()); - channel.postMessage.mockClear(); + test('metadata_updated', async () => { + const cred = await cc.store(makeTestToken()); - await cred.setTags(['foo']); + await cred.setTags(['foo']); - expect(channel.postMessage).toHaveBeenCalledWith({ - eventName: 'metadata_updated', - source: expect.any(String), - id: cred.id + expect(channel.postMessage).toHaveBeenCalledWith({ + eventName: 'metadata_updated', + source: expect.any(String), + id: cred.id + }); }); - }); - it('broadcasts cleared when clear() is called without localOnly', async () => { - await cc.clear(); - expect(channel.postMessage).toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); - }); + test('cleared', async () => { + // does not broadcast when `localOnly` = true + await cc.clear(true); + expect(channel.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ eventName: 'cleared' })); - it('does not broadcast cleared when clear(true) (localOnly) is called', async () => { - await cc.clear(true); - expect(channel.postMessage).not.toHaveBeenCalledWith(expect.objectContaining({ eventName: 'cleared' })); + // broadcasts by default (when `localOnly` = false) + await cc.clear(); + expect(channel.postMessage).toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); + }); }); it('detaches broadcast listeners from a replaced tokenStorage', () => { @@ -120,100 +120,103 @@ describe('CredentialCoordinatorImpl (spa-platform)', () => { }); }); - describe('receiving cross-tab messages', () => { - it('ignores messages broadcast by itself', async () => { - const ownId = (cc as any).id; - const addedSpy = jest.fn(); - cc.emitter.on('credential_added', addedSpy); - - await receive('credential_added', { id: 'foo' }, ownId); + describe('Receiving cross-tab messages', () => { + describe('Events', () => { + test('credential_added', async () => { + const addedSpy = jest.fn(); + cc.emitter.on('credential_added', addedSpy); - expect(addedSpy).not.toHaveBeenCalled(); - }); + await receive('credential_added', { id: 'foo' }); - it('constructs a credential from storage when credential_added is received, not from the message', async () => { - const token = makeTestToken(); - await cc.tokenStorage.add(token); // simulates another tab having already written to shared storage - expect(cc.credentialDataSource.hasCredential(token)).toBe(false); + expect(addedSpy).toHaveBeenCalledWith({ id: 'foo' }); + expect(cc.credentialDataSource.hasCredential('foo')).toBe(false); + }); - await receive('credential_added', { id: token.id }); + describe('credential_refreshed', () => { + it('applies the freshly stored token and emits credential_refreshed when tokens differ', async () => { + const cred = await cc.store(makeTestToken()); + const refreshed = makeTestToken(cred.id); + await cc.tokenStorage.replace(cred.id, refreshed); // simulates another tab's refresh landing in shared storage + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); - expect(cc.credentialDataSource.hasCredential(token)).toBe(true); - }); + await receive('credential_refreshed', { id: cred.id }); - it('does nothing if the token no longer exists in storage when credential_added is received', async () => { - const credentialForSpy = jest.spyOn(cc.credentialDataSource, 'credentialFor'); - await receive('credential_added', { id: 'does-not-exist' }); - expect(credentialForSpy).not.toHaveBeenCalled(); - }); + expect(cred.token).toEqual(refreshed); + expect(refreshedSpy).toHaveBeenCalledWith({ credential: cred }); + }); - it('applies the freshly stored token and emits credential_refreshed when tokens differ', async () => { - const cred = await cc.store(makeTestToken()); - const refreshed = makeTestToken(cred.id); - await cc.tokenStorage.replace(cred.id, refreshed); // simulates another tab's refresh landing in shared storage - const refreshedSpy = jest.fn(); - cc.emitter.on('credential_refreshed', refreshedSpy); + it('does not re-emit credential_refreshed if the stored token already matches', async () => { + const cred = await cc.store(makeTestToken()); + const refreshedSpy = jest.fn(); + cc.emitter.on('credential_refreshed', refreshedSpy); - await receive('credential_refreshed', { id: cred.id }); + await receive('credential_refreshed', { id: cred.id }); // storage already matches cred.token - expect(cred.token).toEqual(refreshed); - expect(refreshedSpy).toHaveBeenCalledWith({ credential: cred }); - }); + expect(refreshedSpy).not.toHaveBeenCalled(); + }); + }); - it('does not re-emit credential_refreshed if the stored token already matches', async () => { - const cred = await cc.store(makeTestToken()); - const refreshedSpy = jest.fn(); - cc.emitter.on('credential_refreshed', refreshedSpy); + test('credential_removed', async () => { + const removedSpy = jest.fn(); + cc.emitter.on('credential_removed', removedSpy); - await receive('credential_refreshed', { id: cred.id }); // storage already matches cred.token + const cred = await cc.store(makeTestToken()); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); - expect(refreshedSpy).not.toHaveBeenCalled(); - }); + // simulates removing Credential **not** present in `DataSource` + receive('credential_removed', { id: 'never-seen' }); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); + expect(removedSpy).toHaveBeenLastCalledWith({ id: 'never-seen' }); // `credential_removed` is still emitted - it('removes the local credential when credential_removed is received', async () => { - const cred = await cc.store(makeTestToken()); - expect(cc.credentialDataSource.hasCredential(cred)).toBe(true); + // simulates removing Credential which **is** present in `DataSource` + await receive('credential_removed', { id: cred.id }); + expect(cc.credentialDataSource.hasCredential(cred)).toBe(false); + expect(removedSpy).toHaveBeenLastCalledWith({ id: cred.id }); + }); - await receive('credential_removed', { id: cred.id }); + test('default_changed', async () => { + const cred = await cc.store(makeTestToken()); + const defaultChangedSpy = jest.fn(); + cc.emitter.on('default_changed', defaultChangedSpy); - expect(cc.credentialDataSource.hasCredential(cred)).toBe(false); - }); + await receive('default_changed', { id: cred.id }); - it('is a no-op removing an id this tab never cached', async () => { - await expect(receive('credential_removed', { id: 'never-seen' })).resolves.not.toThrow(); - }); + expect(defaultChangedSpy).toHaveBeenCalledWith({ storage: cc.tokenStorage, id: cred.id }); + }); - it('applies default_changed from another tab', async () => { - const cred = await cc.store(makeTestToken()); - const defaultChangedSpy = jest.fn(); - cc.emitter.on('default_changed', defaultChangedSpy); + test('metadata_updated', async () => { + const cred = await cc.store(makeTestToken(), ['foo']); + const metadataSpy = jest.fn(); + cc.emitter.on('metadata_updated', metadataSpy); - await receive('default_changed', { id: cred.id }); + await receive('metadata_updated', { id: cred.id }); - expect(defaultChangedSpy).toHaveBeenCalledWith({ storage: cc.tokenStorage, id: cred.id }); - }); + expect(metadataSpy).toHaveBeenCalledWith(expect.objectContaining({ id: cred.id })); + }); - it('applies metadata_updated from another tab by reading fresh metadata from storage', async () => { - const cred = await cc.store(makeTestToken(), ['foo']); - const metadataSpy = jest.fn(); - cc.emitter.on('metadata_updated', metadataSpy); + test('cleared', async () => { + await cc.store(makeTestToken()); + channel.postMessage.mockClear(); + const clearedSpy = jest.fn(); + cc.emitter.on('cleared', clearedSpy); - await receive('metadata_updated', { id: cred.id }); + await receive('cleared'); - expect(metadataSpy).toHaveBeenCalledWith(expect.objectContaining({ id: cred.id })); + expect(cc.size).toEqual(0); + expect(clearedSpy).toHaveBeenCalled(); + expect(channel.postMessage).not.toHaveBeenCalledWith({ eventName: 'cleared', source: expect.any(String) }); + }); }); - it('applies cleared from another tab without re-broadcasting', async () => { - await cc.store(makeTestToken()); - channel.postMessage.mockClear(); - const clearedSpy = jest.fn(); - cc.emitter.on('cleared', clearedSpy); + it('will not process messages it broadcasts', async () => { + const ownId = (cc as any).id; + const addedSpy = jest.fn(); + cc.emitter.on('credential_added', addedSpy); - await receive('cleared'); + await receive('credential_added', { id: 'foo' }, ownId); - expect(cc.size).toEqual(0); - expect(clearedSpy).toHaveBeenCalled(); - expect(channel.postMessage).not.toHaveBeenCalled(); + expect(addedSpy).not.toHaveBeenCalled(); }); }); From b7c28503558afe791ea41188460d6676f7f54901 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Wed, 2 Sep 2026 16:19:42 -0400 Subject: [PATCH 09/11] fix --- e2e/apps/redirect-model/src/main.tsx | 8 -------- 1 file changed, 8 deletions(-) diff --git a/e2e/apps/redirect-model/src/main.tsx b/e2e/apps/redirect-model/src/main.tsx index 1e8e30a0..ba51b062 100644 --- a/e2e/apps/redirect-model/src/main.tsx +++ b/e2e/apps/redirect-model/src/main.tsx @@ -22,14 +22,6 @@ if (!rootElement) { // @ts-expect-error - This is added for e2e purposes only, not recommended for production apps window.Credential = Credential; -// @ts-ignore -window.leakTest = () => { - Credential.allIDs() - .then(ids => { - console.log('allIds.length: ', ids.length); - console.log('Coordinator Emitter Listeners', Credential.coordinator.emitter.listeners?.metadata_updated?.length); - }); -} const root = createRoot(rootElement); root.render( From b2f6359f1aa2f4aca642c8404b95014073b0c434 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Thu, 3 Sep 2026 10:39:26 -0400 Subject: [PATCH 10/11] fixes nit comments --- packages/auth-foundation/src/Credential/Credential.ts | 6 ++++-- packages/auth-foundation/src/http/APIClient.ts | 7 ++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/auth-foundation/src/Credential/Credential.ts b/packages/auth-foundation/src/Credential/Credential.ts index de53018b..88d98809 100644 --- a/packages/auth-foundation/src/Credential/Credential.ts +++ b/packages/auth-foundation/src/Credential/Credential.ts @@ -326,11 +326,13 @@ export class Credential implements RequestAuthorizer, JSONSerializable { /////// public instances methods /////// /** - * Cleans up resources associated with the Credential instance, so that it may be gargabe collected. + * Cleans up resources associated with the Credential instance, so that it may be garbage collected. * * @remarks * This method is meant to be used in conjunction with {@link CredentialDataSource.remove}. Calling this - * method on an active {@link Credential} may have sigificant consequences + * method on an active {@link Credential} may have significant consequences + * + * @internal */ public dispose () { this.oauth2.dispose(); diff --git a/packages/auth-foundation/src/http/APIClient.ts b/packages/auth-foundation/src/http/APIClient.ts index c0ddb7d7..bf8a75fc 100644 --- a/packages/auth-foundation/src/http/APIClient.ts +++ b/packages/auth-foundation/src/http/APIClient.ts @@ -57,7 +57,12 @@ export abstract class APIClient { } /** - * Cleans up resourece associated with the client instance to prevent leaks. + * Cleans up resources associated with the client instance, so that it may be garbage collected. + * + * > [!Warning] + * > **DO NOT** use this method on active clients. + * + * @internal */ public dispose () { this.emitter.clear(); From 7975c757953d2c2f30d584cc2b44222e1c2d0595 Mon Sep 17 00:00:00 2001 From: Jared Perreault Date: Thu, 3 Sep 2026 13:27:04 -0400 Subject: [PATCH 11/11] fixes cci pipeline --- .circleci/config.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .circleci/config.yml diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 00000000..699863d5 --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,17 @@ +version: 2.1 + +# Placeholder pipeline: this branch predates the real CircleCI config being added on another, +# not-yet-merged branch. This just gives CircleCI a valid config to parse so the pipeline +# succeeds instead of failing on a missing/empty config.yml. Replace once merged with the +# branch that introduces the real pipeline definition. +jobs: + noop: + docker: + - image: cimg/base:current + steps: + - run: echo "No-op CI config - real pipeline will be introduced when merged from its source branch." + +workflows: + noop-workflow: + jobs: + - noop \ No newline at end of file