-
Notifications
You must be signed in to change notification settings - Fork 372
fix(clerk-js): Correct signout behavior for multisession #6515
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
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 | ||
--- | ||
|
||
Fixing redirect behavior when signing out from a multisession app with multple singed in accounts |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -809,6 +809,42 @@ describe('Clerk singleton', () => { | |
expect(sut.navigate).toHaveBeenCalledWith('/after-sign-out'); | ||
}); | ||
}); | ||
|
||
it('properly restores auth state from remaining sessions after multisession sign-out', async () => { | ||
const mockClient = { | ||
signedInSessions: [mockSession1, mockSession2], | ||
sessions: [mockSession1, mockSession2], | ||
destroy: mockClientDestroy, | ||
lastActiveSessionId: '1', | ||
}; | ||
|
||
mockSession1.remove = jest.fn().mockImplementation(() => { | ||
mockClient.signedInSessions = mockClient.signedInSessions.filter(s => s.id !== '1'); | ||
mockClient.sessions = mockClient.sessions.filter(s => s.id !== '1'); | ||
return Promise.resolve(mockSession1); | ||
}); | ||
|
||
mockClientFetch.mockReturnValue(Promise.resolve(mockClient)); | ||
|
||
const sut = new Clerk(productionPublishableKey); | ||
sut.navigate = jest.fn(); | ||
await sut.load(); | ||
|
||
expect(sut.session).toBe(mockSession1); | ||
expect(sut.isSignedIn).toBe(true); | ||
|
||
await sut.signOut({ sessionId: '1' }); | ||
|
||
await waitFor(() => { | ||
expect(mockSession1.remove).toHaveBeenCalled(); | ||
expect(mockClientDestroy).not.toHaveBeenCalled(); | ||
expect(sut.navigate).toHaveBeenCalledWith('/'); | ||
}); | ||
|
||
expect(mockClient.lastActiveSessionId).toBe('2'); | ||
expect(sut.session).toBe(mockSession2); | ||
expect(sut.isSignedIn).toBe(true); | ||
}); | ||
Comment on lines
+813
to
+847
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. 🛠️ Refactor suggestion Add assertions for cookie/token sync and event emissions to prevent regressions To ensure reload consistency, assert that the next session’s token is propagated or that a TokenUpdate was emitted. Also validate that UserSignOut is not emitted when removing a non-current session. Examples:
I can draft these test additions if helpful. 🤖 Prompt for AI Agents
|
||
}); | ||
|
||
describe('.navigate(to)', () => { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -531,8 +531,33 @@ export class Clerk implements ClerkInterface { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await session?.remove(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (this.client && this.client.signedInSessions.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const nextSession = this.client.signedInSessions[0]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (nextSession) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.client.lastActiveSessionId = nextSession.id; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.#setAccessors(nextSession); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
this.#emit(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+534
to
+541
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. 🛠️ Refactor suggestion Do not change the active session when a non-current session is removed. This block always switches to the first remaining session, even when the removed session is not the current one. That will unexpectedly flip the active session for the user. Restrict the “pick next session” logic to only when the removed session was the current session. Optionally prefer an “active” session over “pending”. - if (this.client && this.client.signedInSessions.length > 0) {
- const nextSession = this.client.signedInSessions[0];
- if (nextSession) {
- this.client.lastActiveSessionId = nextSession.id;
- this.#setAccessors(nextSession);
- this.#emit();
- }
- }
+ if (shouldSignOutCurrent && this.client && this.client.signedInSessions.length > 0) {
+ // Prefer an active session if available; otherwise fall back to the first remaining session
+ const nextSession =
+ this.client.signedInSessions.find(s => s.status === 'active') ?? this.client.signedInSessions[0];
+ this.client.lastActiveSessionId = nextSession.id;
+ this.#setAccessors(nextSession);
+ this.#emit();
+ }
🤖 Prompt for AI Agents
Comment on lines
+534
to
+541
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. 🛠️ Refactor suggestion Only switch active session if you signed out the current session; prefer an active next session and sync cookies Currently this always selects the first remaining session and updates state, even when removing a non-active session. That can inadvertently switch the active session. Also, the chosen next session might be pending, and cookies are not synced to the new session. Consider:
- if (this.client && this.client.signedInSessions.length > 0) {
- const nextSession = this.client.signedInSessions[0];
- if (nextSession) {
- this.client.lastActiveSessionId = nextSession.id;
- this.#setAccessors(nextSession);
- this.#emit();
- }
- }
+ if (shouldSignOutCurrent && this.client && this.client.signedInSessions.length > 0) {
+ // Prefer an active session; fall back to the first remaining session
+ const nextSession =
+ (this.client.signedInSessions.find(s => s.status === 'active') as SignedInSessionResource | undefined) ??
+ this.client.signedInSessions[0];
+ if (nextSession) {
+ this.client.lastActiveSessionId = nextSession.id;
+ // Sync cookies (__session, __client_uat) to the new session before emitting
+ try {
+ // Touch when in active tab or for non-standard browsers, similar to setActive
+ if (inActiveBrowserTab() || !this.#options.standardBrowser) {
+ await this.#touchCurrentSession(nextSession);
+ }
+ await nextSession.getToken();
+ } catch {
+ // If token retrieval fails, proceed with best-effort state update
+ }
+ this.#setAccessors(nextSession);
+ this.#emit();
+ }
+ } 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+534
to
+541
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. 🛠️ Refactor suggestion Only switch active session when signing out the current session; also sync auth token for the new session Currently the next-session selection runs even when a non-current session is removed, which can inadvertently change the active session if the array ordering differs from the current. Also, after switching, cookies/tokens are not refreshed, risking an inconsistent state on reload.
- if (this.client && this.client.signedInSessions.length > 0) {
- const nextSession = this.client.signedInSessions[0];
- if (nextSession) {
- this.client.lastActiveSessionId = nextSession.id;
- this.#setAccessors(nextSession);
- this.#emit();
- }
- }
+ if (shouldSignOutCurrent && this.client && this.client.signedInSessions.length > 0) {
+ // Prefer selecting based on lastActiveSessionId if present; otherwise first signed-in session.
+ const nextSession =
+ this.client.signedInSessions.find(s => s.id === this.client.lastActiveSessionId) ??
+ this.client.signedInSessions[0];
+
+ if (nextSession) {
+ this.client.lastActiveSessionId = nextSession.id;
+ this.#setAccessors(nextSession);
+ // Ensure downstream consumers receive up-to-date auth. Option A (no network):
+ eventBus.emit(events.TokenUpdate, { token: nextSession.lastActiveToken });
+ // Option B (safer but async): await nextSession.getToken().catch(() => eventBus.emit(events.TokenUpdate, { token: null }));
+ this.#emit();
+ }
+ } 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (shouldSignOutCurrent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await executeSignOut(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
eventBus.emit(events.UserSignOut, null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await tracker.track(async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (signOutCallback) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await signOutCallback(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await this.navigate(redirectUrl); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (tracker.isUnloading()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await onAfterSetActive(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+544
to
561
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. 💡 Verification agent 🧩 Analysis chainVerify side effects of UserSignOut during multi-session switch; ensure cookies aren’t cleared after selecting the next session In the multi-session/current-session path, you emit UserSignOut after switching to the next session. If any listener clears __session cookies on this event, the tab may end up signed-out on reload despite the in-memory switch. Either:
To confirm event side effects and avoid regressions, search for the UserSignOut handlers: If a cookie-clearing listener exists, consider moving TokenUpdate re-sync after the tracker when tracker.isUnloading() is false, or introduce a specialized event for multi-session single sign-out that does not clear cookies globally. 🏁 Script executed: #!/bin/bash
# Find all listeners reacting to UserSignOut and inspect for cookie-clearing.
rg -n "UserSignOut" -A 8 -B 4 Length of output: 8105 🏁 Script executed: #!/bin/bash
# Inspect AuthCookieService.handleSignOut implementation to see if it clears cookies unconditionally
rg -n "handleSignOut" -A 10 -B 2 packages/clerk-js/src/core/auth/AuthCookieService.ts Length of output: 1027 🏁 Script executed: #!/bin/bash
# Find TokenUpdate handler in AuthCookieService to see when cookies are set/updated
rg -n "eventBus.on" -A 10 -B 0 packages/clerk-js/src/core/auth/AuthCookieService.ts Length of output: 728 Prevent clearing cookies on multi‐session switch We confirmed that To fix this, pick one of two approaches: • Introduce a new “session switch” event that only signals other tabs (via broadcast) and doesn’t trigger cookie clearance: • Or, re-sync cookies immediately after the in-memory switch when not unloading:
Locations to update:
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
🛠️ Refactor suggestion
Great addition; add guard-rail tests for non-current removal and active session selection
This verifies the main regression. Please add:
Example additions you can append near this block:
🤖 Prompt for AI Agents