Skip to content

Commit ea81396

Browse files
wobsorianosteve-cooks
wobsoriano
and
steve-cooks
committed
fix(electron,ui): ignore stale OAuth callbacks and re-enable social buttons on return
Each transport redirect URL now carries a per-attempt id, so a callback from a replaced attempt no longer resolves the newer flow. A replaced flow is dropped without rejecting, so its card does not show an error over the new attempt. Social buttons go idle when the app window regains focus while a transport flow is pending. Co-authored-by: steve-ai-vibe <[email protected]>
1 parent 8227c72 commit ea81396

6 files changed

Lines changed: 104 additions & 15 deletions

File tree

‎.changeset/electron-replace-stale-oauth-flow.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
'@clerk/electron': patch
33
---
44

5-
Fix OAuth sign-in staying blocked after a user abandons the browser tab. Starting a new OAuth sign-in now replaces the pending one instead of failing with `Clerk: an OAuth flow is already pending.` until the 3-minute callback timeout. An unsupported OAuth URL no longer cancels a pending flow, a replaced flow's browser launch failure no longer rejects the new flow, and replacing a flow while its browser is still launching no longer causes an unhandled promise rejection in the main process.
5+
Fix OAuth sign-in staying blocked after a user abandons the browser tab. Starting a new OAuth sign-in now replaces the pending one instead of failing with `Clerk: an OAuth flow is already pending.` until the 3-minute callback timeout. If the user later finishes the abandoned browser tab, its callback is ignored and no longer completes the newer sign-in.

‎.changeset/electron-social-buttons-idle-on-focus.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@clerk/ui': patch
3+
---
4+
5+
In Electron apps, the social sign-in and sign-up buttons become clickable again when the user returns to the app without finishing OAuth in the browser. Before, they stayed disabled for up to 3 minutes.

‎packages/electron/src/main/__tests__/create-clerk-bridge.test.ts‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -558,7 +558,9 @@ describe('createClerkBridge', () => {
558558
return channel === OAUTH_TRANSPORT_CHANNELS.getRedirectUrl;
559559
})?.[1];
560560

561-
expect(getRedirectUrlHandler?.(mainFrameEvent)).toBe('my-app://renderer/');
561+
expect(getRedirectUrlHandler?.(mainFrameEvent)).toMatch(
562+
/^my-app:\/\/renderer\/\?__clerk_oauth_attempt=[0-9a-f-]{36}$/,
563+
);
562564
});
563565

564566
it('rejects OAuth transport requests that do not originate from the main frame', async () => {
@@ -706,7 +708,7 @@ describe('createClerkBridge', () => {
706708
expect(rendererWindow.focus).not.toHaveBeenCalled();
707709
});
708710

709-
it('replaces a pending OAuth flow when a new one starts', async () => {
711+
it('replaces a pending OAuth flow when a new one starts without settling the replaced one', async () => {
710712
vi.mocked(shell.openExternal).mockResolvedValue(undefined);
711713
createClerkBridge({
712714
storage,
@@ -719,13 +721,45 @@ describe('createClerkBridge', () => {
719721
const openHandler = vi.mocked(ipcMain.handle).mock.calls.find(([channel]) => {
720722
return channel === OAUTH_TRANSPORT_CHANNELS.open;
721723
})?.[1];
722-
const firstPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1');
724+
const firstSettled = vi.fn();
725+
openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1').then(firstSettled, firstSettled);
723726
const secondPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=2');
724727
const openUrlListener = getAppListener<(event: Electron.Event, url: string) => void>('open-url');
725728

726-
await expect(firstPromise).rejects.toThrow('replaced by a new one');
727729
openUrlListener?.({ preventDefault: vi.fn() } as unknown as Electron.Event, 'my-app://renderer/?code=123');
728730
await expect(secondPromise).resolves.toEqual({ callbackUrl: 'my-app://renderer/?code=123' });
731+
expect(firstSettled).not.toHaveBeenCalled();
732+
});
733+
734+
it('ignores a callback from a replaced attempt', async () => {
735+
vi.mocked(shell.openExternal).mockResolvedValue(undefined);
736+
createClerkBridge({
737+
storage,
738+
renderer: {
739+
host: 'renderer',
740+
scheme: 'my-app',
741+
},
742+
});
743+
744+
const findHandler = (channel: string) =>
745+
vi.mocked(ipcMain.handle).mock.calls.find(([registered]) => registered === channel)?.[1];
746+
const getRedirectUrl = () => String(findHandler(OAUTH_TRANSPORT_CHANNELS.getRedirectUrl)?.(mainFrameEvent));
747+
const open = (url: string) => findHandler(OAUTH_TRANSPORT_CHANNELS.open)?.(mainFrameEvent, url);
748+
749+
const firstRedirectUrl = getRedirectUrl();
750+
void open('https://accounts.example.com/oauth?attempt=1');
751+
const secondRedirectUrl = getRedirectUrl();
752+
const secondPromise = open('https://accounts.example.com/oauth?attempt=2');
753+
const openUrlListener = getAppListener<(event: Electron.Event, url: string) => void>('open-url');
754+
755+
const secondSettled = vi.fn();
756+
secondPromise?.then(secondSettled, secondSettled);
757+
openUrlListener?.({ preventDefault: vi.fn() } as unknown as Electron.Event, `${firstRedirectUrl}&code=first`);
758+
await new Promise(resolve => setTimeout(resolve, 0));
759+
expect(secondSettled).not.toHaveBeenCalled();
760+
761+
openUrlListener?.({ preventDefault: vi.fn() } as unknown as Electron.Event, `${secondRedirectUrl}&code=second`);
762+
await expect(secondPromise).resolves.toEqual({ callbackUrl: `${secondRedirectUrl}&code=second` });
729763
});
730764

731765
it('keeps a pending OAuth flow when a new request has an unsupported URL', async () => {
@@ -771,11 +805,10 @@ describe('createClerkBridge', () => {
771805
const openHandler = vi.mocked(ipcMain.handle).mock.calls.find(([channel]) => {
772806
return channel === OAUTH_TRANSPORT_CHANNELS.open;
773807
})?.[1];
774-
const firstPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1');
808+
void openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1');
775809
const secondPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=2');
776810

777811
rejectFirstLaunch(new Error('launch failed'));
778-
await expect(firstPromise).rejects.toThrow('replaced by a new one');
779812

780813
const openUrlListener = getAppListener<(event: Electron.Event, url: string) => void>('open-url');
781814
openUrlListener?.({ preventDefault: vi.fn() } as unknown as Electron.Event, 'my-app://renderer/?code=123');
@@ -803,12 +836,11 @@ describe('createClerkBridge', () => {
803836
const openHandler = vi.mocked(ipcMain.handle).mock.calls.find(([channel]) => {
804837
return channel === OAUTH_TRANSPORT_CHANNELS.open;
805838
})?.[1];
806-
const firstPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1');
839+
void openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=1');
807840
const secondPromise = openHandler?.(mainFrameEvent, 'https://accounts.example.com/oauth?attempt=2');
808841
await new Promise(resolve => setTimeout(resolve, 0));
809842

810843
resolveFirstLaunch();
811-
await expect(firstPromise).rejects.toThrow('replaced by a new one');
812844

813845
const openUrlListener = getAppListener<(event: Electron.Event, url: string) => void>('open-url');
814846
openUrlListener?.({ preventDefault: vi.fn() } as unknown as Electron.Event, 'my-app://renderer/?code=123');

‎packages/electron/src/main/oauth-transport.ts‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
1+
import { randomUUID } from 'node:crypto';
2+
13
import { app, BrowserWindow, ipcMain, shell } from 'electron';
24

35
import { OAUTH_TRANSPORT_CHANNELS } from '../shared/ipc';
46
import type { RendererSchemeOptions } from '../shared/types';
57
import { isMainFrameEvent } from './validate-sender';
68

79
const CALLBACK_TIMEOUT_MS = 3 * 60 * 1000;
10+
const ATTEMPT_PARAM = '__clerk_oauth_attempt';
811

912
type PendingOAuthFlow = {
1013
resolve: (value: { callbackUrl: string }) => void;
1114
reject: (reason: Error) => void;
1215
timeout: NodeJS.Timeout;
16+
attemptId: string | undefined;
1317
window: Electron.BrowserWindow | null;
1418
};
1519

@@ -36,6 +40,11 @@ function isMatchingCallbackUrl(url: string, redirectUrl: string): boolean {
3640
}
3741
}
3842

43+
function isCallbackForAttempt(url: string, attemptId: string | undefined): boolean {
44+
const callbackAttemptId = new URL(url).searchParams.get(ATTEMPT_PARAM);
45+
return callbackAttemptId === null || callbackAttemptId === attemptId;
46+
}
47+
3948
function findMatchingCallbackUrl(argv: string[], redirectUrl: string): string | undefined {
4049
return argv.find(url => isMatchingCallbackUrl(url, redirectUrl));
4150
}
@@ -64,6 +73,7 @@ function assertExternalOAuthUrl(url: string): void {
6473
export function setupOAuthTransportIpcHandlers(options: OAuthTransportOptions): () => void {
6574
const redirectUrl = buildRedirectUrl(options);
6675
let pendingOAuthFlow: PendingOAuthFlow | null = null;
76+
const attemptIds = new WeakMap<Electron.WebContents, string>();
6777

6878
const disposePendingOAuthFlow = (reason?: Error): void => {
6979
if (!pendingOAuthFlow) {
@@ -80,7 +90,11 @@ export function setupOAuthTransportIpcHandlers(options: OAuthTransportOptions):
8090
};
8191

8292
const handleCallbackUrl = (url: string): void => {
83-
if (!pendingOAuthFlow || !isMatchingCallbackUrl(url, redirectUrl)) {
93+
if (
94+
!pendingOAuthFlow ||
95+
!isMatchingCallbackUrl(url, redirectUrl) ||
96+
!isCallbackForAttempt(url, pendingOAuthFlow.attemptId)
97+
) {
8498
return;
8599
}
86100

@@ -115,7 +129,12 @@ export function setupOAuthTransportIpcHandlers(options: OAuthTransportOptions):
115129
if (!isMainFrameEvent(event)) {
116130
throw new Error("Clerk: OAuth request did not originate from a window's main frame.");
117131
}
118-
return redirectUrl;
132+
133+
const attemptId = randomUUID();
134+
attemptIds.set(event.sender, attemptId);
135+
const attemptRedirectUrl = new URL(redirectUrl);
136+
attemptRedirectUrl.searchParams.set(ATTEMPT_PARAM, attemptId);
137+
return attemptRedirectUrl.href;
119138
});
120139

121140
ipcMain.handle(OAUTH_TRANSPORT_CHANNELS.open, async (event, url: string) => {
@@ -124,9 +143,10 @@ export function setupOAuthTransportIpcHandlers(options: OAuthTransportOptions):
124143
}
125144

126145
assertExternalOAuthUrl(url);
127-
disposePendingOAuthFlow(new Error('Clerk: OAuth flow was replaced by a new one.'));
146+
disposePendingOAuthFlow();
128147

129148
const window = BrowserWindow.fromWebContents(event.sender);
149+
const attemptId = attemptIds.get(event.sender);
130150

131151
let flow: PendingOAuthFlow | null = null;
132152
const callbackPromise = new Promise<{ callbackUrl: string }>((resolve, reject) => {
@@ -135,10 +155,9 @@ export function setupOAuthTransportIpcHandlers(options: OAuthTransportOptions):
135155
reject(new Error('Clerk: OAuth callback timed out.'));
136156
}, CALLBACK_TIMEOUT_MS);
137157

138-
flow = { resolve, reject, timeout, window };
158+
flow = { resolve, reject, timeout, attemptId, window };
139159
pendingOAuthFlow = flow;
140160
});
141-
callbackPromise.catch(() => {});
142161

143162
try {
144163
await shell.openExternal(url);

‎packages/ui/src/elements/SocialButtons.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,10 @@ export const SocialButtons = React.memo((props: SocialButtonsRootProps) => {
116116

117117
const startOauth = async (strategy: OAuthStrategy | Web3Strategy) => {
118118
card.setLoading(strategy);
119+
const setIdleOnReturn = () => card.setIdle();
120+
if (clerk.__internal_hasOAuthTransport && !isWeb3Strategy(strategy)) {
121+
window.addEventListener('focus', setIdleOnReturn, { once: true });
122+
}
119123
try {
120124
if (isWeb3Strategy(strategy)) {
121125
await web3Callback(strategy);
@@ -125,6 +129,8 @@ export const SocialButtons = React.memo((props: SocialButtonsRootProps) => {
125129
} catch {
126130
await sleep(1000);
127131
card.setIdle();
132+
} finally {
133+
window.removeEventListener('focus', setIdleOnReturn);
128134
}
129135
if (idleAfterDelay) {
130136
await sleep(5000);

‎packages/ui/src/elements/__tests__/SocialButtons.test.tsx‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// packages/clerk-js/src/ui/elements/__tests__/SocialButtons.test.tsx
2-
import { render, renderHook, screen } from '@testing-library/react';
2+
import { act, fireEvent, render, renderHook, screen } from '@testing-library/react';
33
import { describe, expect, it, vi } from 'vitest';
44

55
import { bindCreateFixtures } from '@/test/utils';
@@ -357,4 +357,31 @@ describe('SocialButtons', () => {
357357
expect(screen.queryByText('Last used')).not.toBeInTheDocument();
358358
});
359359
});
360+
361+
describe('With an OAuth transport', () => {
362+
it('re-enables the buttons when the window regains focus while the browser flow is pending', async () => {
363+
const { wrapper, fixtures } = await createFixtures(f => {
364+
f.withSocialProvider({ provider: 'google' });
365+
f.withSocialProvider({ provider: 'github' });
366+
});
367+
Object.defineProperty(fixtures.clerk, '__internal_hasOAuthTransport', { value: true });
368+
mockOAuthCallback.mockReturnValue(new Promise(() => {}));
369+
370+
render(
371+
<CardStateProvider>
372+
<SocialButtons {...defaultProps} />
373+
</CardStateProvider>,
374+
{ wrapper },
375+
);
376+
377+
fireEvent.click(screen.getByRole('button', { name: /google/i }));
378+
expect(await screen.findByRole('button', { name: /github/i })).toBeDisabled();
379+
380+
act(() => {
381+
window.dispatchEvent(new Event('focus'));
382+
});
383+
384+
expect(screen.getByRole('button', { name: /github/i })).toBeEnabled();
385+
});
386+
});
360387
});

0 commit comments

Comments
 (0)