Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fifty-ads-smell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/ui': patch
---

fix(ui): Avoid races between email link tabs when using sign up if missing
18 changes: 5 additions & 13 deletions packages/ui/src/common/EmailLinkVerify.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,10 @@ export type EmailLinkVerifyProps = {
verifyPhonePath?: string;
continuePath?: string;
texts: Record<EmailLinkUIStatus, { title: LocalizationKey; subtitle: LocalizationKey }>;
/**
* Invoked when the link lands with `__clerk_status=transferable`. Returns whether this tab took
* the flow over; when it returns false the "return to the original tab" card renders instead.
*/
onTransferable?: () => Promise<boolean>;
};

export const EmailLinkVerify = (props: EmailLinkVerifyProps) => {
const { redirectUrl, redirectUrlComplete, verifyEmailPath, verifyPhonePath, continuePath, onTransferable } = props;
const { redirectUrl, redirectUrlComplete, verifyEmailPath, verifyPhonePath, continuePath } = props;
const { handleEmailLinkVerification } = useClerk();
const { navigate } = useRouter();
const signUp = useCoreSignUp();
Expand All @@ -38,14 +33,11 @@ export const EmailLinkVerify = (props: EmailLinkVerifyProps) => {
await sleep(750);
await handleEmailLinkVerification({ redirectUrlComplete, redirectUrl }, navigate);

// `transferable` = the email was verified but no user exists (`signUpIfMissing`), so there
// is no session to complete here. The sign-up transfer is banked on the client that owns
// the sign-in; if that is this one, `onTransferable` carries the flow forward from this tab,
// otherwise the originating tab's poll does and this one only points the user back there.
// `transferable` = the email was verified but no user exists (`signUpIfMissing`).
// The originating tab's poll performs the sign-up transfer, so this tab has no
// session and nothing to complete - it only points the user back there.
if (getClerkQueryParam('__clerk_status') === 'transferable') {
if (!(await onTransferable?.())) {
setVerificationStatus('transferable');
}
setVerificationStatus('transferable');
return;
}

Expand Down
35 changes: 6 additions & 29 deletions packages/ui/src/components/SignIn/SignInEmailLinkVerify.tsx
Original file line number Diff line number Diff line change
@@ -1,45 +1,22 @@
import { useClerk } from '@clerk/shared/react';

import { SignInEmailLinkFlowComplete } from '../../common/EmailLinkCompleteFlowCard';
import { useSignInContext } from '../../contexts';
import { useRouter } from '../../router';
import { handleSignUpIfMissingTransfer } from './handleSignUpIfMissingTransfer';

/**
* The SignIn tree's email-link verify route: the tab the verification link opened in.
*
* Mounted directly under the SignIn root, so the `../create/...` paths
* `handleSignUpIfMissingTransfer` navigates to resolve as they do from `factor-one`.
* A `transferable` verification is consumed by the polling tab, never here. The banked account
* transfer can be consumed exactly once and no signal available to this tab says whether the
* polling tab is about to do it — `verifiedAtClient` does not match on a development instance
* even when both tabs share a client. Two consumers means the loser's rejected create detaches
* the winner's sign-up server-side, so this tab only points the user back to the original.
*/
export const SignInEmailLinkVerify = () => {
const clerk = useClerk();
const { navigate } = useRouter();
const { afterSignInUrl, afterSignUpUrl, signUpIfMissingEnabled, navigateOnSetActive, unsafeMetadata } =
useSignInContext();

const onTransferable = async () => {
// Mirrors `verifiedFromTheSameClient` on the polling card: whichever tab shares the client
// with the sign-in carries the flow forward. Only that client holds the banked account
// transfer, so a link opened on another device has nothing to consume here.
if (!signUpIfMissingEnabled || clerk.client.signIn.firstFactorVerification.status !== 'transferable') {
return false;
}

await handleSignUpIfMissingTransfer({
clerk,
navigate,
afterSignUpUrl,
navigateOnSetActive,
unsafeMetadata,
});
return true;
};
const { afterSignInUrl } = useSignInContext();

return (
<SignInEmailLinkFlowComplete
redirectUrlComplete={afterSignInUrl}
redirectUrl='../factor-two'
onTransferable={onTransferable}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,23 @@ export const SignInFactorOneEmailLinkCard = (props: SignInFactorOneEmailLinkCard
const ver = si.firstFactorVerification;
if (ver.status === 'expired') {
card.setError(t(localizationKeys('formFieldError__verificationLinkExpired')));
} else if (ver.verifiedFromTheSameClient()) {
// The tab that opened the link shares this client, so it carries the flow forward and
// this one points at it. That holds for a `transferable` verification too: the account
// transfer is banked on the client, so either tab could consume it and only one may.
setSwitchTabStatus(
signUpIfMissingEnabled && ver.status === 'transferable' ? 'transferable' : 'verified_switch_tab',
);
} else if (signUpIfMissingEnabled && ver.status === 'transferable') {
// Verified from another client, which has no banked transfer of its own, so this tab owns it.
// This tab is the sole consumer of the banked account transfer, whichever tab the link
// opened in. `verifiedFromTheSameClient()` cannot arbitrate that: it compares
// `verifiedAtClient` against this client, and on a development instance the link click
// reaches FAPI without dev-browser context, so it reports false even when the link opened
// in a tab of this same browser. Letting the opened tab transfer as well makes both fire,
// and the loser's rejected create detaches the winner's sign-up server-side (cleanUpClient
// in the FAPI sign-up create handler), stranding the flow with no sign-up at all.
return handleSignUpIfMissingTransfer({
clerk,
navigate,
afterSignUpUrl,
navigateOnSetActive,
unsafeMetadata: signInContext.unsafeMetadata,
});
} else if (ver.verifiedFromTheSameClient()) {
setSwitchTabStatus('verified_switch_tab');
} else {
await completeSignInFlow(si);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ describe('SignInEmailLinkVerify', () => {
});
});

it('completes the signUpIfMissing transfer when this client owns the sign-in', async () => {
// The banked transfer is consumed exactly once, and this tab cannot tell whether the polling
// tab is about to consume it: `verifiedAtClient` does not match on a development instance even
// when both tabs share a client. A second consumer would have its create rejected, and that
// rejection detaches the winner's sign-up server-side, so this tab never transfers.
it('leaves the signUpIfMissing transfer to the polling tab even when this client owns the sign-in', async () => {
window.history.replaceState({}, '', '/sign-in/verify?__clerk_status=transferable');
const { wrapper, fixtures, props } = await createFixtures(f => {
f.withEmailAddress();
Expand All @@ -32,18 +36,16 @@ describe('SignInEmailLinkVerify', () => {
props.setProps({ withSignUp: true });

fixtures.signIn.firstFactorVerification = { status: 'transferable' } as any;
fixtures.signUp.create.mockResolvedValueOnce({
status: 'missing_requirements',
missingFields: ['first_name'],
unverifiedFields: [],
} as any);

render(<SignInEmailLinkVerify />, { wrapper });

await waitFor(() => expect(fixtures.clerk.handleEmailLinkVerification).toHaveBeenCalled());
await waitFor(() => {
expect(fixtures.signUp.create).toHaveBeenCalledWith(expect.objectContaining({ transfer: true }));
expect(fixtures.router.navigate).toHaveBeenCalledWith('../create/continue');
screen.getByText('Email verified');
screen.getByText(/return to original tab/i);
});
expect(fixtures.signUp.create).not.toHaveBeenCalled();
expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../create/continue');
});

it('points back to the original tab when another client owns the signUpIfMissing transfer', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ describe('SignInFactorOne sign-up-if-missing transfer', () => {
});
});

it('defers to the newly opened tab when a transferable email link was verified from the same client', async () => {
it('performs the transfer itself when a transferable email link was verified from the same client', async () => {
const email = 'test@clerk.com';
const { wrapper, fixtures, props } = await createFixtures(f => {
f.withEmailAddress();
Expand All @@ -178,8 +178,11 @@ describe('SignInFactorOne sign-up-if-missing transfer', () => {
});
props.setProps({ withSignUp: true });

// The account transfer is banked once on the shared client, so the tab that opened the
// link consumes it and this one must not race for it.
// This tab is the sole consumer of the banked transfer regardless of which tab the link
// opened in. `verifiedFromTheSameClient()` cannot arbitrate that - it reports false on a
// development instance even for a tab of this same browser - so handing the transfer to the
// opened tab would let both fire, and the loser's rejected create detaches the winner's
// sign-up server-side.
fixtures.signIn.createEmailLinkFlow.mockReturnValue({
startEmailLinkFlow: vi.fn().mockResolvedValue({
status: 'needs_first_factor',
Expand All @@ -190,17 +193,21 @@ describe('SignInFactorOne sign-up-if-missing transfer', () => {
}),
cancelEmailLinkFlow: vi.fn(),
} as any);
fixtures.signUp.create.mockResolvedValueOnce({
status: 'missing_requirements',
missingFields: ['first_name'],
unverifiedFields: [],
} as any);

const { userEvent } = render(<SignInFactorOne />, { wrapper });

await userEvent.click(await screen.findByText('Use another method'));
await userEvent.click(await screen.findByText(`Email link to ${email}`));

await waitFor(() => {
screen.getByText('Email verified');
screen.getByText(/newly opened tab/i);
expect(fixtures.signUp.create).toHaveBeenCalledWith(expect.objectContaining({ transfer: true }));
expect(fixtures.router.navigate).toHaveBeenCalledWith('../create/continue');
});
expect(fixtures.signUp.create).not.toHaveBeenCalled();
});

it('triggers sign-up transfer when email link verification becomes transferable', async () => {
Expand Down Expand Up @@ -245,6 +252,53 @@ describe('SignInFactorOne sign-up-if-missing transfer', () => {
});
});

// Every other email-link transfer case here resolves to `missing_requirements`, which routes
// with a relative in-component navigate. A `complete` transfer instead has to leave the
// component: it consumes the sign-in, so the SignIn route guard sends `factor-one` back to the
// start path, and an in-component navigate loses that race and lands on a blank sign-in.
it('leaves the component on a completed email-link transfer rather than routing in-component', async () => {
const email = 'test@clerk.com';
const { wrapper, fixtures, props } = await createFixtures(f => {
f.withEmailAddress();
f.withPassword();
f.withPreferredSignInStrategy({ strategy: 'password' });
f.withEnumerationProtection();
f.startSignInWithEmailAddress({ supportEmailLink: true, identifier: email });
});
props.setProps({ withSignUp: true });

fixtures.signIn.createEmailLinkFlow.mockReturnValue({
startEmailLinkFlow: vi.fn().mockResolvedValue({
status: 'needs_first_factor',
firstFactorVerification: {
status: 'transferable',
verifiedFromTheSameClient: () => false,
},
}),
cancelEmailLinkFlow: vi.fn(),
} as any);
fixtures.signUp.create.mockResolvedValueOnce({
status: 'complete',
createdSessionId: 'sess_transfer',
} as any);
fixtures.clerk.setActive.mockImplementation(async (params: any) => {
await params.navigate?.({ session: { currentTask: null }, decorateUrl: (url: string) => url });
});

const { userEvent } = render(<SignInFactorOne />, { wrapper });

await userEvent.click(await screen.findByText('Use another method'));
await userEvent.click(await screen.findByText(`Email link to ${email}`));

await waitFor(() => {
expect(fixtures.signUp.create).toHaveBeenCalledWith(expect.objectContaining({ transfer: true }));
expect(fixtures.clerk.setActive).toHaveBeenCalledWith(expect.objectContaining({ session: 'sess_transfer' }));
});

// The terminal redirect must not go through the component's router.
expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../create/continue');
});

it('surfaces transfer errors instead of leaving the code form loading', async () => {
const { wrapper, fixtures, props } = await createFixtures(f => {
f.withEmailAddress();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const createMockClerk = (signUpCreateResult: unknown = {}) => {
},
navigate: vi.fn(),
setActive: vi.fn(),
__internal_windowNavigate: vi.fn(),
} as unknown as LoadedClerk;
};

Expand Down Expand Up @@ -77,12 +78,14 @@ describe('handleSignUpIfMissingTransfer', () => {
expect(mockNavigate).not.toHaveBeenCalled();
});

it('delegates post-setActive navigation to navigateOnSetActive with afterSignUpUrl', async () => {
it('delegates post-setActive navigation to navigateOnSetActive when the session has a pending task', async () => {
const clerk = createMockClerk({ status: 'complete', createdSessionId: 'sess_123' }) as LoadedClerk & {
setActive: ReturnType<typeof vi.fn>;
};

const session = { currentTask: null } as any;
// A pending task routes into the combined flow's `create/...` task routes, which are mounted
// inside the component, so that navigation has to stay with the in-component router.
const session = { currentTask: { key: 'choose-organization' } } as any;
const decorateUrl = (url: string) => url;

clerk.setActive.mockImplementation(async params => {
Expand All @@ -101,6 +104,34 @@ describe('handleSignUpIfMissingTransfer', () => {
redirectUrl: 'https://test.com',
decorateUrl,
});
expect((clerk as any).__internal_windowNavigate).not.toHaveBeenCalled();
});

// The transfer consumes the sign-in, so the SignIn route guard is about to bounce `factor-one`
// to the component's start path. Routing the terminal redirect in-component loses that race and
// strands the user on a blank sign-in with the session already created.
it('leaves the component for the terminal redirect when the session has no pending task', async () => {
const clerk = createMockClerk({ status: 'complete', createdSessionId: 'sess_123' }) as LoadedClerk & {
setActive: ReturnType<typeof vi.fn>;
};

const session = { currentTask: null } as any;
const decorateUrl = (url: string) => url;

clerk.setActive.mockImplementation(async params => {
await params.navigate({ session, decorateUrl });
});

await handleSignUpIfMissingTransfer({
clerk,
navigate: mockNavigate,
afterSignUpUrl: 'https://test.com',
navigateOnSetActive: mockNavigateOnSetActive,
});

expect((clerk as any).__internal_windowNavigate).toHaveBeenCalledWith('https://test.com', undefined);
expect(mockNavigateOnSetActive).not.toHaveBeenCalled();
expect(mockNavigate).not.toHaveBeenCalled();
});

it('routes to the combined-flow continue page when sign-up has missing fields', async () => {
Expand Down
17 changes: 14 additions & 3 deletions packages/ui/src/components/SignIn/handleSignUpIfMissingTransfer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { LoadedClerk } from '@clerk/shared/types';

import type { SignInContextType } from '../../contexts';
import type { RouteContextValue } from '../../router/RouteContext';
import { clerkWindowNavigate } from '../../utils/windowNavigate';

type HandleSignUpIfMissingTransferProps = {
clerk: LoadedClerk;
Expand Down Expand Up @@ -45,9 +46,19 @@ export async function handleSignUpIfMissingTransfer({
return clerk.setActive({
session: res.createdSessionId,
navigate: async ({ session, decorateUrl }) => {
// navigateOnSetActive routes pending session tasks to the combined
// flow's `create/...` task routes and handles Safari ITP via decorateUrl.
await navigateOnSetActive({ session, redirectUrl: afterSignUpUrl, decorateUrl });
// A pending task routes into the combined flow's `create/...` task routes, which are
// mounted inside this component, so the in-component router is the right one to use.
if (session.currentTask) {
await navigateOnSetActive({ session, redirectUrl: afterSignUpUrl, decorateUrl });
return;
}

// Terminal redirect. The transfer consumed the sign-in, so `client.signIn` is now null
// and the SignIn route guard is about to bounce this route to the component's start
// path. An in-component navigate races that and loses, stranding the user on a blank
// sign-in screen with the session already created. Leave the component outright
// instead. decorateUrl still applies, for the Safari ITP cookie refresh.
clerkWindowNavigate(clerk, decorateUrl(afterSignUpUrl));
},
});
case 'missing_requirements':
Expand Down
Loading