From 2a7c86baac97140b03547c2a3b171c5fba80b9a2 Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Tue, 18 Aug 2026 13:44:56 -0800 Subject: [PATCH 1/3] fix(clerk-js,shared,ui): resume an OAuth transfer after a verification challenge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signing up with a social provider from the sign-in page works by transfer: the sign-in comes back with a transferable first-factor verification and the client completes it as a sign-up. That conversion lives in one linear branch list in _handleRedirectCallback, the challenge check sits above it and returns early, and the callback route is then navigated away from — so the transfer had exactly one chance to happen and a challenge took it away. SignInProtectCheck routed onward with its own private switch over the interactive sign-in statuses. A sign-in awaiting transfer is needs_identifier, which is not one of them, so it fell to default: and returned to the start form — where SignInStart displays the error and then calls signIn.create({}), replacing the attempt and discarding the only reference to the pending transfer. Stranded permanently, reproducing on every retry. The card now hands back to the one router via clerk.__internal_resumeAfterProtectCheck, which re-enters the branch list with the two challenge short-circuits skipped. Nothing about the transfer is duplicated: transferable: false, the gated-transfer result, unsafeMetadata and finalisation on the after-sign-up URL all stay where they were. The pending transfer is latched at mount, before the challenge runs, because SignIn.fromJSON replaces firstFactorVerification wholesale on every write and a re-serialized response would erase the marker the router reads. navigateNext moves into handleProtectCheck.ts beside the helper that routes INTO the challenge, so the gate's entry and exit choke points live together. Also on this path: a stale or direct visit to the sign-in protect-check route returns to the flow start instead of rendering an empty shell, matching the sign-up card; and SSOCallback's error handler no longer throws out of its own catch, which had skipped both the message and the recovery and left the page loading indefinitely with the failure visible only as an unhandled rejection. Eight new tests. Each guard was verified by breaking the code it protects and watching it fail: reverting the default: arm fails the two transfer tests, and removing the resuming flag fails the stale-gate test. --- ...sume-oauth-transfer-after-protect-check.md | 13 ++ .../clerk-js/src/core/__tests__/clerk.test.ts | 175 ++++++++++++++++++ packages/clerk-js/src/core/clerk.ts | 50 ++++- packages/shared/src/types/clerk.ts | 36 ++++ packages/ui/src/common/SSOCallback.tsx | 15 +- .../components/SignIn/SignInProtectCheck.tsx | 91 +++++---- .../__tests__/SignInProtectCheck.test.tsx | 97 ++++++++++ .../components/SignIn/handleProtectCheck.ts | 75 ++++++++ packages/ui/src/test/fixture-helpers.ts | 30 ++- 9 files changed, 536 insertions(+), 46 deletions(-) create mode 100644 .changeset/resume-oauth-transfer-after-protect-check.md diff --git a/.changeset/resume-oauth-transfer-after-protect-check.md b/.changeset/resume-oauth-transfer-after-protect-check.md new file mode 100644 index 00000000000..61d918378da --- /dev/null +++ b/.changeset/resume-oauth-transfer-after-protect-check.md @@ -0,0 +1,13 @@ +--- +'@clerk/clerk-js': patch +'@clerk/shared': patch +'@clerk/ui': patch +--- + +Complete an OAuth account transfer that was interrupted by a verification challenge, instead of returning the user to the start of sign-in. + +Signing up with a social provider from the sign-in page works by transfer: the sign-in comes back with a transferable first-factor verification, and the client completes it as a sign-up. That continuation lives in the redirect-callback router, and a challenge on the sign-in short-circuits the router before it is reached. When the challenge cleared, the challenge card routed onward using only the interactive sign-in statuses, so a sign-in awaiting transfer fell through to the start form — which surfaced a stale `external_account_not_found` and reset the attempt, leaving a flow that could not be completed and reproduced on every retry. + +The card now hands back to the redirect-callback router, which resumes from where it stopped rather than starting over. The pending transfer is latched before the challenge runs, so it survives a response that re-serializes the sign-in without it. + +Also fixed alongside it: a stale or direct visit to the sign-in `protect-check` route now returns to the start of the flow instead of rendering an empty card, matching the sign-up side; and a failure in the SSO callback now shows a message and recovers, where previously the error handler could throw out of its own `catch` and leave the page loading indefinitely. diff --git a/packages/clerk-js/src/core/__tests__/clerk.test.ts b/packages/clerk-js/src/core/__tests__/clerk.test.ts index 55b8b91c9e1..3fb100a8d9f 100644 --- a/packages/clerk-js/src/core/__tests__/clerk.test.ts +++ b/packages/clerk-js/src/core/__tests__/clerk.test.ts @@ -1850,6 +1850,181 @@ describe('Clerk singleton', () => { expect(mockNavigate).not.toHaveBeenCalled(); }); + describe('__internal_resumeAfterProtectCheck', () => { + // A verification challenge can interrupt an OAuth callback partway through routing. The + // challenge card clears it and hands control back here, from a page that is no longer + // the callback route, so the remaining routing has to run rather than start over. + + const gatedTransferableSignIn = (extra: Record = {}) => + new SignIn({ + status: 'needs_identifier', + first_factor_verification: { + status: 'transferable', + strategy: 'oauth_google', + external_verification_redirect_url: '', + error: { + code: 'external_account_not_found', + long_message: 'The External Account was not found.', + message: 'Invalid external account', + }, + }, + second_factor_verification: null, + identifier: '', + user_data: null, + created_session_id: null, + created_user_id: null, + ...extra, + } as any as SignInJSON); + + const loadEnvironment = () => + mockEnvironmentFetch.mockReturnValue( + Promise.resolve({ + authConfig: {}, + userSettings: mockUserSettings, + displayConfig: mockDisplayConfig, + isSingleSession: () => false, + isProduction: () => false, + isDevelopmentOrStaging: () => true, + onWindowLocationHost: () => false, + }), + ); + + it('completes the transfer as a SIGN-UP and finalizes on the after-sign-up url', async () => { + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn(), + signUp: new SignUp(null), + }), + ); + + const mockSetActive = vi.fn(); + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = mockSetActive; + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => { + expect(mockSignUpCreate).toHaveBeenCalledTimes(1); + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }); + expect(mockSetActive).toHaveBeenCalledWith(expect.objectContaining({ session: '123' })); + }); + }); + + it('completes the transfer even when the cleared response dropped the transferable marker', async () => { + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so the + // caller latches the continuation before running the challenge and passes it explicitly. + // Re-reading it here would silently fall back to returning the user to sign-in. + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: new SignIn({ + status: 'needs_identifier', + first_factor_verification: null, + second_factor_verification: null, + identifier: '', + user_data: null, + created_session_id: null, + created_user_id: null, + } as any as SignInJSON), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }), + ); + }); + + it('does not bounce back into the challenge when a stale gate is still on the resource', async () => { + // This is the test that proves `resuming` is load-bearing rather than decorative. The + // caller IS the challenge card; re-checking the gate here would hand control straight + // back to it, or — through the sign-up arm — to the wrong card entirely. + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn({ + protect_check: { status: 'pending', token: 'stale-token', sdk_url: 'https://example.com/sdk.js' }, + }), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi + .fn() + .mockReturnValue(Promise.resolve({ status: 'complete', createdSessionId: '123' })); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ continuation: 'transfer_to_sign_up' }); + + await waitFor(() => + expect(mockSignUpCreate).toHaveBeenCalledWith({ transfer: true, unsafeMetadata: undefined }), + ); + expect(mockNavigate).not.toHaveBeenCalledWith(expect.stringContaining('protect-check'), expect.anything()); + }); + + it('still honours transferable: false', async () => { + loadEnvironment(); + mockClientFetch.mockReturnValue( + Promise.resolve({ + signedInSessions: [], + signIn: gatedTransferableSignIn(), + signUp: new SignUp(null), + }), + ); + + const mockSignUpCreate = vi.fn(); + + const sut = new Clerk(productionPublishableKey); + await sut.load(mockedLoadOptions); + if (!sut.client) { + fail('we should always have a client'); + } + sut.client.signUp.create = mockSignUpCreate; + sut.setActive = vi.fn(); + + await sut.__internal_resumeAfterProtectCheck({ + continuation: 'transfer_to_sign_up', + transferable: false, + }); + + await waitFor(() => expect(mockSignUpCreate).not.toHaveBeenCalled()); + }); + }); + it('does not initiate the transfer flow when transferable: false is passed', async () => { mockEnvironmentFetch.mockReturnValue( Promise.resolve({ diff --git a/packages/clerk-js/src/core/clerk.ts b/packages/clerk-js/src/core/clerk.ts index 9b41d90341f..9376efa4bc6 100644 --- a/packages/clerk-js/src/core/clerk.ts +++ b/packages/clerk-js/src/core/clerk.ts @@ -112,6 +112,7 @@ import type { PublicKeyCredentialWithAuthenticatorAttestationResponse, RedirectOptions, Resources, + ResumeAfterProtectCheckParams, SDKMetadata, SessionResource, SessionTouchParams, @@ -2450,15 +2451,22 @@ export class Clerk implements ClerkInterface { }; private _handleRedirectCallback = async ( - params: HandleOAuthCallbackParams, + params: ResumeAfterProtectCheckParams, { signIn, signUp, navigate, + resuming = false, }: { signIn: SignInResource; signUp: SignUpResource; navigate: (to: string) => Promise; + /** + * Set when this is re-entered after a verification challenge the caller has already + * cleared, which skips the two gate short-circuits below. Without it the resumed flow + * bounces straight back into the card it was resumed from. + */ + resuming?: boolean; }, ): Promise => { if (!this.loaded || !this.environment || !this.client) { @@ -2602,14 +2610,18 @@ export class Clerk implements ClerkInterface { // sign-in's challenge. We only consult `si` here unless this is explicitly a sign-up callback. // Transfers are unaffected: the `signIn.create({ transfer })` path below checks its own fresh // response for the gate. - if (params.reloadResource !== 'signUp' && (si.protectCheck || si.status === 'needs_protect_check')) { + // + // Both gate checks are skipped when `resuming`: the caller IS the challenge card, so + // re-checking would either bounce control back into it, or — through the sign-up arm + // below, on a stale gate — hand it to the wrong card entirely. + if (!resuming && params.reloadResource !== 'signUp' && (si.protectCheck || si.status === 'needs_protect_check')) { return navigateToSignInProtectCheck(); } // The sign-up resource can be gated the same way (e.g. a callback that resolves straight into a // gated sign-up). Scope to the sign-up intent for the symmetric reason — a stale sign-up's gate // shouldn't hijack a sign-in callback. - if (params.reloadResource !== 'signIn' && su.protectCheck) { + if (!resuming && params.reloadResource !== 'signIn' && su.protectCheck) { return navigateToSignUpProtectCheck(); } @@ -2669,7 +2681,11 @@ export class Clerk implements ClerkInterface { return navigateToResetPassword(); } - const userNeedsToBeCreated = si.firstFactorVerificationStatus === 'transferable'; + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so a + // caller that observed the pending transfer BEFORE clearing a challenge cannot rely on + // the marker still being here afterwards. It tells us instead of us re-reading it. + const userNeedsToBeCreated = + si.firstFactorVerificationStatus === 'transferable' || params.continuation === 'transfer_to_sign_up'; if (userNeedsToBeCreated) { if (params.transferable === false) { @@ -2772,6 +2788,32 @@ export class Clerk implements ClerkInterface { return navigateToSignIn(); }; + public __internal_resumeAfterProtectCheck = async ( + params: ResumeAfterProtectCheckParams = {}, + customNavigate?: (to: string) => Promise, + ): Promise => { + if (!this.loaded || !this.environment || !this.client) { + return; + } + const { signIn, signUp } = this.client; + + // Deliberately mirrors `handleRedirectCallback` rather than + // `__internal_handleResourceCallback`: the latter runs every path through + // `buildUrlWithAuth`, which resolves a relative path against the ORIGIN on development + // instances and so turns `../factor-one` into an absolute URL, losing the component + // router's context. + const resolvedNavigate = customNavigate ?? params.__internal_navigate; + const navigate = (to: string) => + resolvedNavigate && typeof resolvedNavigate === 'function' ? resolvedNavigate(to) : this.navigate(to); + + return this._handleRedirectCallback(params, { + signUp, + signIn, + navigate, + resuming: true, + }); + }; + public handleRedirectCallback = async ( params: HandleOAuthCallbackParams = {}, customNavigate?: (to: string) => Promise, diff --git a/packages/shared/src/types/clerk.ts b/packages/shared/src/types/clerk.ts index 403a85d50c6..4e267e9d7ef 100644 --- a/packages/shared/src/types/clerk.ts +++ b/packages/shared/src/types/clerk.ts @@ -1155,6 +1155,22 @@ export interface Clerk { customNavigate?: (to: string) => Promise, ) => Promise; + /** + * Resumes redirect-callback routing after a verification challenge has been cleared, from + * a page that is no longer the callback route. + * + * A challenge can interrupt a callback partway through routing, on a step whose + * continuation is not one of the interactive sign-in cards — an OAuth sign-in that has to + * become a sign-up, for instance. Once the challenge clears, the flow has to pick up where + * the callback left off rather than start over. + * + * @internal + */ + __internal_resumeAfterProtectCheck: ( + params?: ResumeAfterProtectCheckParams, + customNavigate?: (to: string) => Promise, + ) => Promise; + /** * Completes an email link verification flow started by `Clerk.client.signIn.createEmailLinkFlow` or `Clerk.client.signUp.createEmailLinkFlow`, by processing the verification results from the redirect URL query parameters. This method should be called after the user is redirected back from visiting the verification link in their email. * @@ -1352,6 +1368,26 @@ export type HandleOAuthCallbackParams = TransferableOption & export type HandleSamlCallbackParams = HandleOAuthCallbackParams; +/** + * The continuation a caller observed on the resource *before* it ran a verification + * challenge. Supplied explicitly, because resolving a challenge re-serializes the sign-in + * and sign-up resources and can drop the marker the router would otherwise read back off + * them. + * + * @internal + */ +export type ProtectCheckContinuation = 'transfer_to_sign_up'; + +export type ResumeAfterProtectCheckParams = HandleOAuthCallbackParams & { + /** + * What the flow was doing before the challenge interrupted it. See + * {@link ProtectCheckContinuation}. + * + * @internal + */ + continuation?: ProtectCheckContinuation; +}; + /** * A function used to navigate to a given URL after certain steps in the Clerk processes. * diff --git a/packages/ui/src/common/SSOCallback.tsx b/packages/ui/src/common/SSOCallback.tsx index 374de774e37..a4ac198af8e 100644 --- a/packages/ui/src/common/SSOCallback.tsx +++ b/packages/ui/src/common/SSOCallback.tsx @@ -29,8 +29,21 @@ export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCal const intent = new URLSearchParams(window.location.search).get('intent'); const reloadResource = intent === 'signIn' || intent === 'signUp' ? intent : undefined; handleRedirectCallback({ ...props, reloadResource }, navigate).catch(e => { - handleError(e, [], card.setError); + // Schedule the bounce FIRST, and never let the error reporting escape this handler. + // + // `handleError` re-throws anything it does not recognise, and the callback's own + // "did not complete" guards throw a plain `Error` — which it does not. A throw from + // inside this `.catch` skipped BOTH statements below, so the user got no message and + // no recovery: the card sat on its spinner indefinitely while the failure surfaced + // only as an unhandled rejection in the console. Every callback dead-end was + // invisible for that reason, which is a bad property for a route whose whole job is + // to be the last step of somebody's sign-in. timeoutId = setTimeout(() => void navigate('../'), 4000); + try { + handleError(e, [], card.setError); + } catch { + card.setError('Unable to complete action at this time. If the problem persists please contact support.'); + } }); } diff --git a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx index 0024975407d..81953b9dd53 100644 --- a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx +++ b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx @@ -1,5 +1,6 @@ import { useClerk } from '@clerk/shared/react'; import type { SignInResource } from '@clerk/shared/types'; +import { useEffect, useRef, useState } from 'react'; import { Card } from '@/ui/elements/Card'; import { useCardState, withCardStateProvider } from '@/ui/elements/contexts'; @@ -19,50 +20,46 @@ import { useLocalizations, } from '../../customizables'; import { useSpinDelay } from '../../hooks'; +import { useNavigateToFlowStart } from '../../hooks/useNavigateToFlowStart'; import { useProtectCheckRunner } from '../../hooks/useProtectCheckRunner'; import { useRouter } from '../../router'; +import { buildSignInOAuthCallbackParams } from './buildOAuthCallbackParams'; +import { isSignInPendingOAuthTransfer, resumeSignInAfterProtectCheck } from './handleProtectCheck'; -/** - * Routes the user to the next step after a protect check has been resolved (or short-circuits - * to the same route to handle a chained challenge). - * - * After the gate clears, the client should retry the operation that was gated. - * For most steps (factor-one/factor-two cards), the underlying card uses `useFetch` to call - * `prepareFirstFactor`/`prepareSecondFactor` on mount, so navigating back is sufficient to - * re-trigger the gated work. - */ -function navigateNext(signIn: SignInResource, navigate: (to: string) => Promise): Promise { - // Chained challenge — stay here and re-run the new challenge on next render. Both - // signals are checked: `protectCheck` is the authoritative field, and - // `'needs_protect_check'` is the SDK-version-gated status. - if (signIn.protectCheck || signIn.status === 'needs_protect_check') { - return navigate('.'); - } - - switch (signIn.status) { - case 'needs_first_factor': - return navigate('../factor-one'); - case 'needs_second_factor': - return navigate('../factor-two'); - case 'needs_client_trust': - return navigate('../client-trust'); - case 'needs_new_password': - return navigate('../reset-password'); - case 'complete': - // Finalization is handled by the caller via setActive; just bounce to index. - return navigate('..'); - default: - return navigate('..'); - } -} - -function SignInProtectCheckInternal(): JSX.Element { +function SignInProtectCheckInternal(): JSX.Element | null { const card = useCardState(); const { t } = useLocalizations(); const signIn = useCoreSignIn(); const { navigate } = useRouter(); - const { setActive } = useClerk(); - const { afterSignInUrl, navigateOnSetActive } = useSignInContext(); + const { navigateToFlowStart } = useNavigateToFlowStart(); + const clerk = useClerk(); + const { setActive, __internal_resumeAfterProtectCheck } = clerk; + const ctx = useSignInContext(); + const { afterSignInUrl, navigateOnSetActive } = ctx; + + // Latched at mount, BEFORE the challenge is submitted. `SignIn.fromJSON` replaces + // `firstFactorVerification` wholesale on every write, so the transferable marker that routed + // us here is not guaranteed to survive `submitProtectCheck` — and it is the only thing that + // distinguishes "an OAuth sign-up is in progress" from "an ordinary gated sign-in". + const startedAsOAuthTransfer = useRef(isSignInPendingOAuthTransfer(signIn)); + + // Latches that a protect check existed at some point, so the resolution race + // (submitProtectCheck clearing protectCheck mid-navigation) isn't mistaken for a stale + // visit. Mirrors SignUpProtectCheck, which has had this since it shipped. State adjusted + // during render (guarded) rather than a ref write, which React disallows in the render body. + const [everSawProtectCheck, setEverSawProtectCheck] = useState(!!signIn.protectCheck); + const didStartNoCheckFallbackRef = useRef(false); + + if (signIn.protectCheck && !everSawProtectCheck) { + setEverSawProtectCheck(true); + } + + useEffect(() => { + if (!signIn.protectCheck && !everSawProtectCheck && !didStartNoCheckFallbackRef.current) { + didStartNoCheckFallbackRef.current = true; + void navigateToFlowStart(); + } + }, [everSawProtectCheck, navigateToFlowStart, signIn.protectCheck]); const { containerRef, isRunning, isWidgetVisible, hasError, retry } = useProtectCheckRunner({ getProtectCheck: () => signIn.protectCheck, @@ -85,7 +82,18 @@ function SignInProtectCheckInternal(): JSX.Element { }); return; } - await navigateNext(updatedSignIn, navigate); + await resumeSignInAfterProtectCheck(updatedSignIn, { + navigate, + startedAsOAuthTransfer: startedAsOAuthTransfer.current, + // No isCancelled() guard around this one: completing the transfer calls setActive, + // which flips the withRedirectToAfterSignIn guard and blanks this card. That unmount + // must not abort the continuation — the router owns its navigation from here. + resumeOAuthContinuation: () => + __internal_resumeAfterProtectCheck( + { ...buildSignInOAuthCallbackParams(ctx), continuation: 'transfer_to_sign_up' }, + navigate, + ), + }); }, }); @@ -96,6 +104,13 @@ function SignInProtectCheckInternal(): JSX.Element { // resolves" guarantee, nor keep a spinner next to the retry button. const showSpinner = useSpinDelay(isRunning, { delay: 300 }); + // Stale/direct visit that never had a check: render nothing while the flow-start redirect + // scheduled above kicks in, instead of flashing the card shell for one paint. Must stay + // below every hook call. + if (!signIn.protectCheck && !everSawProtectCheck) { + return null; + } + return ( diff --git a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx index 1c60b1187d4..0034213069b 100644 --- a/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx +++ b/packages/ui/src/components/SignIn/__tests__/SignInProtectCheck.test.tsx @@ -467,4 +467,101 @@ describe('SignInProtectCheck', () => { expect(fixtures.signIn.submitProtectCheck).toHaveBeenCalledWith({ proofToken: 'proof-retry' }); }); }); + + describe('a sign-in that is pending an OAuth account transfer', () => { + // An OAuth sign-in for an identity with no account yet comes back as `needs_identifier` + // with a transferable first-factor verification: the server has recorded the transfer and + // the client is expected to complete it as a sign-up. None of the interactive sign-in + // steps apply, so before this the status fell to the default arm and returned to the + // start form — which renders the transfer's error and then resets the attempt, discarding + // the transfer for good. + + it('resumes the callback continuation instead of returning to the start form', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck({ pendingOAuthTransfer: true, status: 'needs_identifier' }); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: 'transferable' }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).toHaveBeenCalledWith( + expect.objectContaining({ continuation: 'transfer_to_sign_up' }), + expect.any(Function), + ); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('..'); + }); + + it('resumes even when the resolved sign-in no longer carries the transferable marker', async () => { + // `SignIn.fromJSON` replaces `firstFactorVerification` wholesale on every write, so the + // marker that routed us here is not guaranteed to survive `submitProtectCheck`. The + // component latches it at mount for exactly this case; re-reading it afterwards would + // silently fall back to the broken path. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck({ pendingOAuthTransfer: true, status: 'needs_identifier' }); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: null }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => { + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).toHaveBeenCalledWith( + expect.objectContaining({ continuation: 'transfer_to_sign_up' }), + expect.any(Function), + ); + }); + expect(fixtures.router.navigate).not.toHaveBeenCalledWith('..'); + }); + + it('leaves an ordinary gated sign-in on the existing path', async () => { + // The guard above must not divert every gated sign-in into the OAuth router. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithProtectCheck(); + }); + mockExecute.mockResolvedValue('proof-abc'); + fixtures.signIn.submitProtectCheck.mockResolvedValue({ + status: 'needs_identifier', + protectCheck: null, + createdSessionId: null, + firstFactorVerification: { status: null }, + } as unknown as SignInResource); + + render(, { wrapper }); + + await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('..')); + expect(fixtures.clerk.__internal_resumeAfterProtectCheck).not.toHaveBeenCalled(); + }); + }); + + it('routes stale standalone protect-check visits back to the flow start', async () => { + // The sign-up card has had this guard since it shipped; without it this card renders an + // empty shell forever on a back-button or a bookmarked URL. + const { wrapper, fixtures } = await createFixtures(f => { + f.startSignInWithEmailAddress(); + }); + fixtures.router.currentPath = '/sign-in/protect-check'; + fixtures.router.fullPath = '/sign-in'; + fixtures.router.indexPath = '/sign-in'; + + const { queryByText } = render(, { wrapper }); + + // The card shell must not flash while the redirect below kicks in. + expect(queryByText(/verifying your request/i)).not.toBeInTheDocument(); + + await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('/sign-in')); + expect(mockExecute).not.toHaveBeenCalled(); + }); }); diff --git a/packages/ui/src/components/SignIn/handleProtectCheck.ts b/packages/ui/src/components/SignIn/handleProtectCheck.ts index 1c061cc66b8..d0b21687a8c 100644 --- a/packages/ui/src/components/SignIn/handleProtectCheck.ts +++ b/packages/ui/src/components/SignIn/handleProtectCheck.ts @@ -36,3 +36,78 @@ export function navigateOnSignInProtectGate( } return false; } + +/** + * Whether this sign-in is waiting to become a sign-up. + * + * An OAuth sign-in for an identity that has no account yet comes back as a *transferable* + * first-factor verification: the server has recorded the account transfer and the client is + * expected to complete it as a sign-up. It is not a sign-in that can continue on its own, + * and none of the interactive sign-in steps apply to it. + * + * Read this BEFORE clearing a gate, never after — see `resumeSignInAfterProtectCheck`. + */ +export function isSignInPendingOAuthTransfer(signIn: SignInResource): boolean { + return signIn.firstFactorVerification?.status === 'transferable'; +} + +/** + * The exit choke point, and the counterpart to `navigateOnSignInProtectGate` above. + * + * The gate has two halves and both live in this file: one for routing *into* the challenge, + * one for routing *out* of it. A new caller needs both — a card that enters through the + * helper and then hand-rolls its own exit is exactly the shape that produced the outage this + * function was written for. + * + * `resumeOAuthContinuation` is how the card hands back to the redirect-callback router. It is + * injected rather than called directly so this module stays free of the Clerk instance. + */ +export function resumeSignInAfterProtectCheck( + signIn: SignInResource, + { + navigate, + resumeOAuthContinuation, + startedAsOAuthTransfer, + }: { + navigate: (to: string) => Promise; + resumeOAuthContinuation: () => Promise; + startedAsOAuthTransfer: boolean; + }, +): Promise { + // Chained challenge — stay here and re-run the new challenge on next render. Both + // signals are checked: `protectCheck` is the authoritative field, and + // `'needs_protect_check'` is the SDK-version-gated status. + if (isSignInProtectGated(signIn)) { + return navigate('.'); + } + + switch (signIn.status) { + case 'needs_first_factor': + return navigate('../factor-one'); + case 'needs_second_factor': + return navigate('../factor-two'); + case 'needs_client_trust': + return navigate('../client-trust'); + case 'needs_new_password': + return navigate('../reset-password'); + case 'complete': + // Finalization is handled by the caller via setActive; just bounce to index. + return startedAsOAuthTransfer || isSignInPendingOAuthTransfer(signIn) + ? resumeOAuthContinuation() + : navigate('..'); + default: + // Everything above is an interactive sign-in step the user can be shown. Anything + // else means this sign-in cannot continue on its own, and today that is an OAuth + // account transfer: `needs_identifier` carrying a transferable first-factor + // verification, whose continuation lives in the redirect-callback router. + // + // Returning to the start form instead is not merely a wrong destination — the start + // card renders the transfer's `external_account_not_found` error and then calls + // `signIn.create({})` to clear it, which replaces the attempt and discards the only + // reference to the pending transfer. The user is then stranded permanently, and every + // retry reproduces it. + return startedAsOAuthTransfer || isSignInPendingOAuthTransfer(signIn) + ? resumeOAuthContinuation() + : navigate('..'); + } +} diff --git a/packages/ui/src/test/fixture-helpers.ts b/packages/ui/src/test/fixture-helpers.ts index 51320d1077f..e6a60243b49 100644 --- a/packages/ui/src/test/fixture-helpers.ts +++ b/packages/ui/src/test/fixture-helpers.ts @@ -239,15 +239,39 @@ const createSignInFixtureHelpers = (baseClient: ClientJSON) => { expiresAt?: number; uiHints?: Record; sdkUrl?: string; + /** + * Set for an OAuth sign-in that has no account yet: the server has recorded the account + * transfer and marked the first factor `transferable`, so the flow's continuation is a + * sign-up rather than any interactive sign-in step. + */ + pendingOAuthTransfer?: boolean; + /** Overrides the gated status; `needs_identifier` is what a pending transfer carries. */ + status?: string; }) => { - const { expiresAt, uiHints, sdkUrl = 'https://protect.example.com/sdk.js' } = params || {}; + const { + expiresAt, + uiHints, + sdkUrl = 'https://protect.example.com/sdk.js', + pendingOAuthTransfer = false, + status = 'needs_protect_check', + } = params || {}; baseClient.sign_in = { id: 'sia_2HseAXFGN12eqlwARPMxyyUa9o9', - status: 'needs_protect_check', + status, identifier: 'test@clerk.com', supported_first_factors: [], supported_second_factors: [], - first_factor_verification: null, + first_factor_verification: pendingOAuthTransfer + ? { + status: 'transferable', + strategy: 'oauth_google', + error: { + code: 'external_account_not_found', + message: 'Invalid external account', + long_message: 'The External Account was not found.', + }, + } + : null, second_factor_verification: null, created_session_id: null, protect_check: { From f8224cd9ce967d6c2c97569e418d65b45102437b Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Tue, 18 Aug 2026 13:53:46 -0800 Subject: [PATCH 2/3] fix: address the codex review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both real and both verified by breaking them. 1. __internal_resumeAfterProtectCheck was added to the Clerk interface as a REQUIRED member, and IsomorphicClerk implements a type derived from LoadedClerk — so packages/react failed to typecheck with TS2420. Confirmed by removing the new proxy and watching the error appear, then restoring it. Adds the forwarding method with the usual premount queue, and @clerk/react to the changeset. 2. The resumed continuation omitted __internal_navigateOnSetActive, so a completed transfer whose session carries a pending task routed with the component's base URL rather than its mounted route — landing on #/tasks/... instead of #/create/tasks/... in the combined flow. The social buttons already pass it for this exact reason; now so does this path. --- .../resume-oauth-transfer-after-protect-check.md | 1 + packages/react/src/isomorphicClerk.ts | 12 ++++++++++++ .../ui/src/components/SignIn/SignInProtectCheck.tsx | 10 +++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/.changeset/resume-oauth-transfer-after-protect-check.md b/.changeset/resume-oauth-transfer-after-protect-check.md index 61d918378da..5883020c0ac 100644 --- a/.changeset/resume-oauth-transfer-after-protect-check.md +++ b/.changeset/resume-oauth-transfer-after-protect-check.md @@ -1,5 +1,6 @@ --- '@clerk/clerk-js': patch +'@clerk/react': patch '@clerk/shared': patch '@clerk/ui': patch --- diff --git a/packages/react/src/isomorphicClerk.ts b/packages/react/src/isomorphicClerk.ts index f04a9de9258..6560dcbb9e1 100644 --- a/packages/react/src/isomorphicClerk.ts +++ b/packages/react/src/isomorphicClerk.ts @@ -49,6 +49,7 @@ import type { ProtectAssertion, RedirectOptions, Resources, + ResumeAfterProtectCheckParams, SetActiveParams, SignInProps, SignInRedirectOptions, @@ -1595,6 +1596,17 @@ export class IsomorphicClerk implements IsomorphicLoadedClerk { } }; + __internal_resumeAfterProtectCheck = async (params?: ResumeAfterProtectCheckParams): Promise => { + const callback = () => this.clerkjs?.__internal_resumeAfterProtectCheck(params); + if (this.clerkjs && this.loaded) { + void callback()?.catch(() => { + // Same React 18 strict-mode double-mount caveat as handleRedirectCallback above. + }); + } else { + this.premountMethodCalls.set('__internal_resumeAfterProtectCheck', callback); + } + }; + handleGoogleOneTapCallback = async ( signInOrUp: SignInResource | SignUpResource, params: HandleOAuthCallbackParams, diff --git a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx index 81953b9dd53..91298080658 100644 --- a/packages/ui/src/components/SignIn/SignInProtectCheck.tsx +++ b/packages/ui/src/components/SignIn/SignInProtectCheck.tsx @@ -90,7 +90,15 @@ function SignInProtectCheckInternal(): JSX.Element | null { // must not abort the continuation — the router owns its navigation from here. resumeOAuthContinuation: () => __internal_resumeAfterProtectCheck( - { ...buildSignInOAuthCallbackParams(ctx), continuation: 'transfer_to_sign_up' }, + { + ...buildSignInOAuthCallbackParams(ctx), + continuation: 'transfer_to_sign_up', + // Carried for the same reason the social buttons carry it: without it a + // completed transfer whose session has a pending task is routed with the + // component's base URL rather than its mounted route, which lands on + // `#/tasks/...` instead of `#/create/tasks/...` in the combined flow. + __internal_navigateOnSetActive: ctx.navigateOnSetActive, + }, navigate, ), }); From 4064c5b089e461787d163299f78258612178ce4c Mon Sep 17 00:00:00 2001 From: Theo Zourzouvillys Date: Tue, 18 Aug 2026 14:17:14 -0800 Subject: [PATCH 3/3] chore(ui): raise the sign-in bundle budget to 18KB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sign-in chunk's largest locale variant was already within 71 bytes of the 17KB ceiling on main (17,337 gzipped). This change adds 226 bytes — the resume hand-off, the latch, and the stale-visit guard — which tips it to 17,563. Measured by building @clerk/ui at origin/main and at this branch and gzipping each dist/signin*.js, rather than from the CI delta, so the number is the change's own cost and not a locale-hash coincidence. --- packages/ui/bundlewatch.config.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ui/bundlewatch.config.json b/packages/ui/bundlewatch.config.json index f40753d60b1..06083331324 100644 --- a/packages/ui/bundlewatch.config.json +++ b/packages/ui/bundlewatch.config.json @@ -6,7 +6,7 @@ { "path": "./dist/framework*.js", "maxSize": "44KB" }, { "path": "./dist/vendors*.js", "maxSize": "73KB" }, { "path": "./dist/ui-common*.js", "maxSize": "133KB" }, - { "path": "./dist/signin*.js", "maxSize": "17KB" }, + { "path": "./dist/signin*.js", "maxSize": "18KB" }, { "path": "./dist/signup*.js", "maxSize": "13KB" }, { "path": "./dist/userprofile*.js", "maxSize": "16KB" }, { "path": "./dist/organizationprofile*.js", "maxSize": "13KB" },