Skip to content

redirect 2FA in incognito while 2FA is enabled #93656

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.

Sign up for GitHub

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

Merged
blimpich merged 5 commits into from
Jun 19, 2026
Merged

redirect 2FA in incognito while 2FA is enabled #93656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
049d508
redirect 2FA in incognito while 2FA is enabled
mukhrr Jun 16, 2026
703a615
removed !exitTo unnecessary gate
mukhrr Jun 16, 2026
20e3941
fixed spell check fail
mukhrr Jun 16, 2026
b6cc47c
added 2FA enabled account tests
mukhrr Jun 16, 2026
65638bc
fix: scope the 2FA sign-in redirect to the current magic link
mukhrr Jun 19, 2026
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
57 changes: 45 additions & 12 deletions src/pages/ValidateLoginPage/index.web.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import JustSignedInModal from '@components/ValidateCode/JustSignedInModal';
import ValidateCodeModal from '@components/ValidateCode/ValidateCodeModal';
import useOnyx from '@hooks/useOnyx';
import Log from '@libs/Log';
import Navigation from '@libs/Navigation/Navigation';
import Navigation, {navigationRef} from '@libs/Navigation/Navigation';
import {isValidValidateCode} from '@libs/ValidationUtils';
import {handleExitToNavigation, initAutoAuthState, signInWithValidateCode} from '@userActions/Session';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import SCREENS from '@src/SCREENS';
import type {Session as SessionType} from '@src/types/onyx';
import type ValidateLoginPageProps from './types';

Expand Down Expand Up @@ -40,7 +41,20 @@ function ValidateLoginPage({
const effectiveAutoAuthState = hasInitialized ? autoAuthState : undefined;
const autoAuthStateWithDefault = effectiveAutoAuthState ?? CONST.AUTO_AUTH_STATE.NOT_STARTED;
const is2FARequired = !!account?.requiresTwoFactorAuth;
const cachedAccountID = credentials?.accountID;
// A magic-link sign-in that needs 2FA completes on the sign-in page: it reuses the stored
// `credentials.validateCode`, and SignInPage renders the authenticator-code stage once
// `requiresTwoFactorAuth` + that code are present. Send the user there to enter their code instead
// of the informational "2FA required" modal, which is a dead end. Require the cached credentials to
// match THIS link — `signInWithValidateCode` only caches the code on a successful 2FA-required
// response, so a stale code left over from an earlier attempt can't redirect a later failed/expired
// link. `exitTo` deep links route here too; the effect keeps the deferred `exitTo` navigation
// pending so the user reaches their destination after 2FA completes.
const canCompleteTwoFactorOnSignIn =
is2FARequired &&
!isSignedIn &&
credentials?.validateCode === validateCode &&
credentials?.accountID === Number(accountID) &&
(autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN || autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.FAILED);
const isUserClickedSignIn = !login && isSignedIn && (autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.SIGNING_IN || autoAuthStateWithDefault === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN);
const shouldStartSignInWithValidateCode = !isUserClickedSignIn && !isSignedIn && (!!login || !!exitTo) && isValidValidateCode(validateCode);
const isNavigatingToExitTo = isSignedIn && !!exitTo;
Expand Down Expand Up @@ -94,18 +108,37 @@ function ValidateLoginPage({
);

useEffect(() => {
if (!!login || !cachedAccountID || !is2FARequired) {
if (exitTo) {
handleExitToNavigation(exitTo);
}
return;
let ignore = false;
if (canCompleteTwoFactorOnSignIn) {
// Show the sign-in page so its ValidateCodeForm renders the authenticator-code stage.
// ROUTES.HOME ('home') is nested under the authenticated TAB_NAVIGATOR, so navigate/goBack
// to it no-op from the public /v/ route; reset the stack to SCREENS.HOME instead — the same
// mechanism logout uses to surface the public SignInPage. The "2FA required" modal stays
// rendered as the fallback so a failed hand-off shows it, not a blank or an endless loader.
Comment thread
mukhrr marked this conversation as resolved.
Navigation.isNavigationReady().then(() => {
// Bail if the effect re-ran (e.g. `isSignedIn` flipped true) before this resolved, so a
// stale callback can't reset the stack out from under the new state.
if (ignore) {
return;
}
Comment on lines +119 to +123

@mkhutornyi mkhutornyi Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ignore will be reset to false when this effect re-runs. Am I wrong?

@mukhrr mukhrr Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each effect run closes over its own ignore. On re-run, React runs the previous run's cleanup (ignore = true on the binding the pending .then captured) before the new body runs with a fresh ignore = false and a new .then — so the stale callback bails and the new one proceeds. It's the standard React "ignore" cleanup pattern; the stale-callback guard test exercises exactly this.

navigationRef.reset({index: 0, routes: [{name: SCREENS.HOME}]});
});
}

// Register the deferred `exitTo` destination navigation while still unauthenticated.
// `handleExitToNavigation` waits for the authToken, so for a 2FA account it fires only after the
// user enters their code on the sign-in page above (the token lands and resolves the shared
// sign-in promise) — landing them on their destination instead of Home. For a non-2FA account
// it's the normal post-sign-in handoff. The `!isSignedIn` guard registers it once and never
// re-fires after sign-in, so the effect re-running can't queue a duplicate navigation.
if (exitTo && !isSignedIn) {
handleExitToNavigation(exitTo);

@mkhutornyi mkhutornyi Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now also called when canCompleteTwoFactorOnSignIn = true. Is it safe?

@mukhrr mukhrr Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question — it's safe (the reset unmounts this page before 2FA completes, and the only double-call is at mount where both share the single authPromiseResolver, so one navigate fires). But I'll make it explicit by guarding the registration with !isSignedIn so it can never re-register post-sign-in — no reliance on timing.

}

// The user clicked the option to sign in the current tab
Navigation.isNavigationReady().then(() => {
Navigation.goBack();
});
}, [login, cachedAccountID, is2FARequired, exitTo]);
return () => {
ignore = true;
};
}, [canCompleteTwoFactorOnSignIn, exitTo, isSignedIn]);

// waitForProtectedRoutes()/authToken can hang (lazy AuthScreens chunk fails, token
// never lands). We can't recover the consumed code here, but surface a stuck sign-in to
Expand Down
154 changes: 149 additions & 5 deletions tests/ui/ValidateLoginPageTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Navigation from '@libs/Navigation/Navigation';
import createPlatformStackNavigator from '@libs/Navigation/PlatformStackNavigation/createPlatformStackNavigator';
import type {PublicScreensParamList} from '@libs/Navigation/types';
import ValidateLoginPage from '@pages/ValidateLoginPage/index.web';
import {handleExitToNavigation} from '@userActions/Session';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand All @@ -16,10 +17,22 @@ import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'
// protected routes are available) — not just the final args.
const mockWaitForProtectedRoutes = {resolve: () => {}};

// Controllable deferred for isNavigationReady() so tests can resolve it on demand — and, for the
// stale-callback guard, resolve it *after* the page unmounts to prove the reset is skipped.
const mockIsNavigationReady = {resolve: () => {}};

// Standalone fn so assertions don't access `navigationRef.reset` unbound (unbound-method lint rule).
const mockNavigationReset = jest.fn();

jest.mock('@libs/Navigation/Navigation', () => ({
navigate: jest.fn(),
goBack: jest.fn(),
isNavigationReady: jest.fn(() => Promise.resolve()),
isNavigationReady: jest.fn(
() =>
new Promise<void>((resolve) => {
mockIsNavigationReady.resolve = resolve;
}),
),
waitForProtectedRoutes: jest.fn(
() =>
new Promise<void>((resolve) => {
Expand All @@ -29,6 +42,22 @@ jest.mock('@libs/Navigation/Navigation', () => ({
getActiveRoute: jest.fn(() => ''),
getActiveRouteWithoutParams: jest.fn(() => ''),
isActiveRoute: jest.fn(() => false),
// Dereference inside the closure (not at factory time) — the factory runs before the const below
// is initialized, so capturing `mockNavigationReset` directly would freeze `undefined`.
navigationRef: {
reset: (...args: unknown[]) => {
mockNavigationReset(...args);
},
isReady: () => true,
},
}));

// Mock the session actions the page calls so `signInWithValidateCode` doesn't hit the API and the
// deferred `handleExitToNavigation` handoff can be asserted.
jest.mock('@userActions/Session', () => ({
initAutoAuthState: jest.fn(),
signInWithValidateCode: jest.fn(),
handleExitToNavigation: jest.fn(),
}));

const RootStack = createPlatformStackNavigator<PublicScreensParamList>();
Expand Down Expand Up @@ -57,6 +86,7 @@ describe('ValidateLoginPage', () => {
beforeEach(async () => {
jest.clearAllMocks();
mockWaitForProtectedRoutes.resolve = () => {};
mockIsNavigationReady.resolve = () => {};
await act(async () => {
await Onyx.clear();
});
Expand Down Expand Up @@ -164,10 +194,10 @@ describe('ValidateLoginPage', () => {
expect(Navigation.navigate).not.toHaveBeenCalledWith(ROUTES.HOME, {forceReplace: true});
});

it('Should show the 2FA-required prompt (not an infinite loader) for a separate-session sign-in needing 2FA', async () => {
// Separate session: no cached `login`, signed in via the link but 2FA is required so authToken
// never lands. Without the fix isCompletingDirectSignIn keeps the loader up forever; instead we
// must surface the "2FA required" prompt and never redirect Home.
it('Should show the 2FA-required prompt (not an infinite loader) when 2FA is needed and no validate code is cached', async () => {
// Genuinely-stuck fallback: 2FA is required but there's no cached `credentials.validateCode`, so
// the sign-in page can't render the authenticator stage and there's nowhere to send the user.
// Surface the informational "2FA required" prompt and never redirect Home or loop on a loader.
await act(async () => {
await Onyx.set(ONYXKEYS.ACCOUNT, {requiresTwoFactorAuth: true});
await Onyx.set(ONYXKEYS.SESSION, {
Expand All @@ -179,6 +209,120 @@ describe('ValidateLoginPage', () => {
await waitForBatchedUpdatesWithAct();

expect(screen.queryByTestId('validate-login-loading')).toBeNull();
expect(mockNavigationReset).not.toHaveBeenCalled();
expect(Navigation.navigate).not.toHaveBeenCalledWith(ROUTES.HOME, {forceReplace: true});
});

it('Should redirect to the sign-in page to enter the 2FA code when a validate code is cached', async () => {
// Initiating browser: the magic-link attempt stored `credentials.validateCode` and 2FA is
// required. SignInPage reuses that code to render the authenticator-code stage, so we replace
// the consumed /v/ route with the sign-in page instead of the dead-end "2FA required" modal.
await act(async () => {
await Onyx.set(ONYXKEYS.ACCOUNT, {requiresTwoFactorAuth: true});
await Onyx.set(ONYXKEYS.SESSION, {
autoAuthState: CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN,
});
await Onyx.set(ONYXKEYS.CREDENTIALS, {
accountID: 1,
validateCode: '123456',
});
});

renderPage({accountID: '1', validateCode: '123456'});
await waitForBatchedUpdatesWithAct();

// Resolve the navigation-ready gate, then the effect resets the public stack to the SignInPage.
await act(async () => {
mockIsNavigationReady.resolve();
await Promise.resolve();
});
await waitForBatchedUpdatesWithAct();

expect(mockNavigationReset).toHaveBeenCalledWith({index: 0, routes: [{name: SCREENS.HOME}]});
});

it('Should route an exitTo 2FA magic link to the sign-in page AND keep the deferred exitTo navigation', async () => {
// exitTo deep link on a 2FA account: the user must still reach the authenticator form (not the
// dead-end "2FA required" modal), so we reset to the sign-in page. We also register
// handleExitToNavigation so that once 2FA completes and the authToken lands, the user is taken
// to their deep-link destination instead of being dropped on Home.
await act(async () => {
await Onyx.set(ONYXKEYS.ACCOUNT, {requiresTwoFactorAuth: true});
await Onyx.set(ONYXKEYS.SESSION, {
autoAuthState: CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN,
});
await Onyx.set(ONYXKEYS.CREDENTIALS, {
accountID: 1,
validateCode: '123456',
});
});

renderPage({accountID: '1', validateCode: '123456', exitTo: 'concierge'});
await waitForBatchedUpdatesWithAct();

await act(async () => {
mockIsNavigationReady.resolve();
await Promise.resolve();
});
await waitForBatchedUpdatesWithAct();

expect(mockNavigationReset).toHaveBeenCalledWith({index: 0, routes: [{name: SCREENS.HOME}]});
expect(handleExitToNavigation).toHaveBeenCalledWith('concierge');
});

it('Should NOT redirect to the sign-in page when the cached validate code is for a different link (stale code on a failed attempt)', async () => {
// A 2FA account can keep an old `credentials.validateCode` from an earlier attempt. A later /v/
// link that fails leaves that stale code in Onyx (failureData doesn't touch it) while
// `requiresTwoFactorAuth` lingers — so the redirect must NOT fire for a code that isn't this
// link's, otherwise the user lands on the 2FA stage for the stale code instead of the failed UI.
await act(async () => {
await Onyx.set(ONYXKEYS.ACCOUNT, {requiresTwoFactorAuth: true});
await Onyx.set(ONYXKEYS.SESSION, {
autoAuthState: CONST.AUTO_AUTH_STATE.FAILED,
});
await Onyx.set(ONYXKEYS.CREDENTIALS, {
accountID: 1,
validateCode: '111111', // stale code cached by an earlier attempt
});
});

// The link the user just opened carries a different code.
renderPage({accountID: '1', validateCode: '222222'});
await waitForBatchedUpdatesWithAct();
await waitForBatchedUpdatesWithAct();

expect(mockNavigationReset).not.toHaveBeenCalled();
});

it('Should not reset to the sign-in page when the page unmounts before navigation is ready (stale-callback guard)', async () => {
// PERF-15 cleanup: if the effect tears down (deps change / unmount) before isNavigationReady()
// resolves, the pending callback must bail instead of resetting the stack out from under the
// screen that replaced it.
await act(async () => {
await Onyx.set(ONYXKEYS.ACCOUNT, {requiresTwoFactorAuth: true});
await Onyx.set(ONYXKEYS.SESSION, {
autoAuthState: CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN,
});
await Onyx.set(ONYXKEYS.CREDENTIALS, {
accountID: 1,
validateCode: '123456',
});
});

const {unmount} = renderPage({accountID: '1', validateCode: '123456'});
await waitForBatchedUpdatesWithAct();

// isNavigationReady() is still pending. Unmounting runs the effect cleanup (sets `ignore = true`).
await act(async () => {
unmount();
});

// Resolving now fires the stale callback, which must skip the reset.
await act(async () => {
mockIsNavigationReady.resolve();
await Promise.resolve();
});

expect(mockNavigationReset).not.toHaveBeenCalled();
});
});
Loading