-
Notifications
You must be signed in to change notification settings - Fork 437
fix(clerk-js): Prevent session cookie removal during offline token refresh #7912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
618871a
be6256a
4c86bb9
2c9a84e
2c09e1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/clerk-js': patch | ||
| --- | ||
|
|
||
| Fix random sign-outs when the browser temporarily loses network connectivity. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,8 +261,6 @@ describe('Session', () => { | |
|
|
||
| describe('with offline browser and network failure', () => { | ||
| beforeEach(() => { | ||
| // Use real timers for offline tests to avoid unhandled rejection issues with retry logic | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π |
||
| vi.useRealTimers(); | ||
| Object.defineProperty(window.navigator, 'onLine', { | ||
| writable: true, | ||
| value: false, | ||
|
|
@@ -274,10 +272,9 @@ describe('Session', () => { | |
| writable: true, | ||
| value: true, | ||
| }); | ||
| vi.useFakeTimers(); | ||
| }); | ||
|
|
||
| it('throws ClerkOfflineError when offline', async () => { | ||
| it('throws ClerkOfflineError after retries when offline', async () => { | ||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
|
|
@@ -292,15 +289,15 @@ describe('Session', () => { | |
| mockNetworkFailedFetch(); | ||
| BaseResource.clerk = { getFapiClient: () => createFapiClient(baseFapiClientOptions) } as any; | ||
|
|
||
| try { | ||
| await session.getToken({ skipCache: true }); | ||
| expect.fail('Expected ClerkOfflineError to be thrown'); | ||
| } catch (error) { | ||
| expect(ClerkOfflineError.is(error)).toBe(true); | ||
| } | ||
| const errorPromise = session.getToken({ skipCache: true }).catch(e => e); | ||
|
|
||
| await vi.advanceTimersByTimeAsync(60_000); | ||
|
|
||
| const error = await errorPromise; | ||
| expect(ClerkOfflineError.is(error)).toBe(true); | ||
| }); | ||
|
|
||
| it('throws ClerkOfflineError after fetch fails while offline', async () => { | ||
| it('retries 3 times before throwing when offline without making network requests', async () => { | ||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
|
|
@@ -315,11 +312,114 @@ describe('Session', () => { | |
| mockNetworkFailedFetch(); | ||
| BaseResource.clerk = { getFapiClient: () => createFapiClient(baseFapiClientOptions) } as any; | ||
|
|
||
| await expect(session.getToken({ skipCache: true })).rejects.toThrow(ClerkOfflineError); | ||
| const getTokenSpy = vi.spyOn(session as any, '_getToken'); | ||
|
|
||
| const errorPromise = session.getToken({ skipCache: true }).catch(e => e); | ||
|
|
||
| await vi.advanceTimersByTimeAsync(60_000); | ||
|
|
||
| // Fetch should have been called at least once | ||
| expect(global.fetch).toHaveBeenCalled(); | ||
| await errorPromise; | ||
|
|
||
| expect(getTokenSpy).toHaveBeenCalledTimes(4); | ||
| expect(global.fetch).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('does not emit token:update with an empty token when offline', async () => { | ||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
| object: 'session', | ||
| user: createUser({}), | ||
| last_active_organization_id: null, | ||
| actor: null, | ||
| created_at: new Date().getTime(), | ||
| updated_at: new Date().getTime(), | ||
| } as SessionJSON); | ||
|
|
||
| mockNetworkFailedFetch(); | ||
| BaseResource.clerk = { getFapiClient: () => createFapiClient(baseFapiClientOptions) } as any; | ||
|
|
||
| const errorPromise = session.getToken({ skipCache: true }).catch(e => e); | ||
| await vi.advanceTimersByTimeAsync(60_000); | ||
| await errorPromise; | ||
|
|
||
| const emptyTokenUpdates = dispatchSpy.mock.calls.filter( | ||
| (call: unknown[]) => | ||
| call[0] === 'token:update' && !(call[1] as { token: { getRawString(): string } })?.token?.getRawString(), | ||
| ); | ||
| expect(emptyTokenUpdates).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('throws error instead of returning null when browser recovers mid-request', async () => { | ||
| // Simulate the race condition: | ||
| // 1. _baseFetch catches a network error while offline β returns null | ||
| // 2. Browser comes back online before _getToken checks isValidBrowserOnline() | ||
| // 3. _getToken sees result=null but browser is online β skips the throw β returns null | ||
| // The caller gets null which looks like "signed out" even though user is authenticated. | ||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
| object: 'session', | ||
| user: createUser({}), | ||
| last_active_organization_id: null, | ||
| actor: null, | ||
| created_at: new Date().getTime(), | ||
| updated_at: new Date().getTime(), | ||
| } as SessionJSON); | ||
|
|
||
| // Browser was offline (set by parent describe's beforeEach) but has now recovered. | ||
| Object.defineProperty(window.navigator, 'onLine', { writable: true, value: true }); | ||
|
|
||
| // Mock _fetch to return null, simulating what _baseFetch does when the offline | ||
| // branch fires. The browser was offline when the catch | ||
| // ran, but has since recovered by the time _getToken checks. | ||
| const fetchSpy = vi.spyOn(BaseResource, '_fetch' as any).mockResolvedValue(null); | ||
|
|
||
| try { | ||
| const promise = session.getToken(); | ||
| // Suppress unhandled rejection from intermediate retry promises during timer advancement. | ||
| // The assertion below still checks the original rejected promise. | ||
| promise.catch(() => {}); | ||
| // Advance timers to allow all retries to complete | ||
| await vi.advanceTimersByTimeAsync(200_000); | ||
| // Should throw β not silently return null | ||
| await expect(promise).rejects.toThrow(); | ||
| } finally { | ||
| fetchSpy.mockRestore(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it('does not emit token:update with an empty token even when online', async () => { | ||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
| object: 'session', | ||
| user: createUser({}), | ||
| last_active_organization_id: null, | ||
| actor: null, | ||
| created_at: new Date().getTime(), | ||
| updated_at: new Date().getTime(), | ||
| } as SessionJSON); | ||
|
|
||
| BaseResource.clerk = { getFapiClient: () => createFapiClient(baseFapiClientOptions) } as any; | ||
|
|
||
| const fetchSpy = vi.spyOn(BaseResource, '_fetch' as any).mockResolvedValue(null); | ||
|
|
||
| try { | ||
| const promise = session.getToken(); | ||
| promise.catch(() => {}); | ||
| await vi.advanceTimersByTimeAsync(200_000); | ||
| await expect(promise).rejects.toThrow(); | ||
|
|
||
| const emptyTokenUpdates = dispatchSpy.mock.calls.filter( | ||
| (call: unknown[]) => | ||
| call[0] === 'token:update' && !(call[1] as { token: { getRawString(): string } })?.token?.getRawString(), | ||
| ); | ||
| expect(emptyTokenUpdates).toHaveLength(0); | ||
| } finally { | ||
| fetchSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| it(`uses the current session's lastActiveOrganizationId by default, not clerk.organization.id`, async () => { | ||
|
|
@@ -588,6 +688,48 @@ describe('Session', () => { | |
| expect(requestSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not emit token:update with an empty token when background refresh fires while offline', async () => { | ||
| BaseResource.clerk = clerkMock(); | ||
| const requestSpy = BaseResource.clerk.getFapiClient().request as Mock<any>; | ||
|
|
||
| const session = new Session({ | ||
| status: 'active', | ||
| id: 'session_1', | ||
| object: 'session', | ||
| user: createUser({}), | ||
| last_active_organization_id: null, | ||
| last_active_token: { object: 'token', jwt: mockJwt }, | ||
| actor: null, | ||
| created_at: new Date().getTime(), | ||
| updated_at: new Date().getTime(), | ||
| } as SessionJSON); | ||
|
|
||
| await Promise.resolve(); | ||
| requestSpy.mockClear(); | ||
| dispatchSpy.mockClear(); | ||
|
|
||
| // Go offline before the refresh timer fires | ||
| Object.defineProperty(window.navigator, 'onLine', { writable: true, value: false }); | ||
| mockNetworkFailedFetch(); | ||
| BaseResource.clerk = { getFapiClient: () => createFapiClient(baseFapiClientOptions) } as any; | ||
|
|
||
| // Advance to trigger the refresh timer (~43s) and let the refresh complete | ||
| await vi.advanceTimersByTimeAsync(44 * 1000); | ||
|
|
||
| const emptyTokenUpdates = dispatchSpy.mock.calls.filter( | ||
| (call: unknown[]) => | ||
| call[0] === 'token:update' && !(call[1] as { token: { getRawString(): string } })?.token?.getRawString(), | ||
| ); | ||
| expect(emptyTokenUpdates).toHaveLength(0); | ||
|
|
||
| // Come back online and restore mock | ||
| Object.defineProperty(window.navigator, 'onLine', { writable: true, value: true }); | ||
| BaseResource.clerk = clerkMock(); | ||
|
|
||
| const token = await session.getToken(); | ||
| expect(token).toEqual(mockJwt); | ||
| }); | ||
|
|
||
| it('does not make API call when token has plenty of time remaining', async () => { | ||
| BaseResource.clerk = clerkMock(); | ||
| const requestSpy = BaseResource.clerk.getFapiClient().request as Mock<any>; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,13 +73,12 @@ export function isBrowserOnline(): boolean { | |
| return false; | ||
| } | ||
|
|
||
| const isNavigatorOnline = navigator?.onLine; | ||
|
|
||
| // Being extra safe with the experimental `connection` property, as it is not defined in all browsers | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/Navigator/connection#browser_compatibility | ||
| // @ts-ignore | ||
| const isExperimentalConnectionOnline = navigator?.connection?.rtt !== 0 && navigator?.connection?.downlink !== 0; | ||
| return isExperimentalConnectionOnline && isNavigatorOnline; | ||
| // navigator.onLine is the standard API and is reliable for detecting | ||
| // complete disconnection (airplane mode, WiFi off, etc.). | ||
| // The experimental navigator.connection API (rtt/downlink) was previously | ||
| // used as a secondary signal, but it reports zero values in headless browsers | ||
| // and CI environments even when connected, causing false offline detection. | ||
| return !!navigator.onLine; | ||
|
Comment on lines
+76
to
+81
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, I'm not sure we want to change this. can we look back in the history? I feel like I recall there being issues with only relying on |
||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this throw if offline? should we avoid fetching if we're offline in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question! So that sent me through a rabbit hole and tl;dr: we can do it and if we throw here when we're offline, we'll trigger the retrying mechanism without firing network requests.
Additionally because of this exploration I figured a race condition that we had where the browser could go back online while mid-request, so I added a test for it and a fix here