fix(clerk-js,react,shared,ui): resume an OAuth transfer after a verification challenge

.changeset/resume-oauth-transfer-after-protect-check.md

@@ -0,0 +1,14 @@
+---
+'@clerk/clerk-js': minor
+'@clerk/react': patch
+'@clerk/shared': minor
+'@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.

packages/clerk-js/src/core/__tests__/clerk.test.ts

@@ -1850,6 +1850,220 @@ 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<string, unknown> = {}) =>
+        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 activates the created session', 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 divert to the sign-up card when a stale gate is on the sign-up resource', async () => {
+        // The sign-in variant below covers the first short-circuit; `resuming` skips a second one
+        // keyed on the SIGN-UP resource, and that is the arm that sends the user to a different
+        // card entirely rather than back to this one.
+        loadEnvironment();
+        mockClientFetch.mockReturnValue(
+          Promise.resolve({
+            signedInSessions: [],
+            signIn: gatedTransferableSignIn(),
+            signUp: new SignUp({
+              protect_check: { status: 'pending', token: 'stale-signup-token', sdk_url: 'https://example.com/sdk.js' },
+            } as any),
+          }),
+        );
+
+        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.mock.calls.some(([to]) => typeof to === 'string' && to.includes('protect-check'))).toBe(
+          false,
+        );
+      });
+
+      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.mock.calls.some(([to]) => typeof to === 'string' && to.includes('protect-check'))).toBe(
+          false,
+        );
+      });
+
+      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({

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<unknown>;
+      /**
+       * 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<unknown> => {
     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<unknown>,
+  ): Promise<unknown> => {
+    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<unknown>,