fix: prevent SAML re-fire infinite loop during short-lived-token redeem#96124
fix: prevent SAML re-fire infinite loop during short-lived-token redeem#96124fabioh8010 wants to merge 4 commits into
Conversation
SignInPage.getRenderOptions re-initiated SAML on the fresh mount that follows the /transition redeem whenever account.isLoading was still optimistically true. hasInitiatedSAMLLogin is component-local useState, so it resets on that mount; on a slow-redeem / fast-IdP combination SAML re-fired (window.location .replace) before the redeem landed, aborting it — an infinite loop for SAML-required users (reproduced end-to-end; seen in production across a whole SAML-required domain). Gate shouldInitiateSAMLLogin on the RAM-only IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN flag (already set for the exact duration of the redeem in getShortLivedLoginParams, but previously unread) so SAML cannot re-fire while a redeem is in flight. This removes the isLoading timing dependency and fixes the infinite case regardless of device/IdP/network. Note: this covers the single-tab loop. A multi-tab variant (a second tab re-firing on the shared account.isLoading) is a known follow-up; it needs a non-persisted cross-tab signal (the persisted session flag regresses sign-in). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d6e5dd563
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // with hasInitiatedSAMLLogin reset. If account.isLoading is still optimistically true (slow IdP/network) SAML would | ||
| // re-fire before the redeem lands -> infinite loop. isAuthenticatingWithShortLivedToken (RAM-only, set for the exact | ||
| // duration of the redeem) gates that off, independent of the isLoading timing race. | ||
| const shouldInitiateSAMLLogin = hasAccount && hasLogin && isSAMLRequired && !hasInitiatedSAMLLogin && !!account.isLoading && !isSupportalSession && !isAuthenticatingWithShortLivedToken; |
There was a problem hiding this comment.
Set the token guard before awaiting device info
When a SAML /transition callback is handled, LogInWithShortLivedAuthTokenPage calls signInWithShortLivedAuthToken and immediately schedules Navigation.navigate(ROUTES.HOME) for SAML callbacks, but signInWithShortLivedAuthToken does not apply getShortLivedLoginParams (including RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN) until Device.getDeviceInfoWithID() resolves. If navigation wins that race on a slower device/browser, this predicate still sees isAuthenticatingWithShortLivedToken === false while the old SAML-required/loading account state is present, so the page can re-fire SAML before the guard is ever set and the loop remains. Set the RAM-only guard synchronously before leaving /transition or before awaiting device info.
Useful? React with 👍 / 👎.
Explanation of Change
Fixes a SAML web sign-in loop where an SSO (SAML-required) user bounces between their identity provider and the app and the session never sticks.
On the
/transitionredirect-back,LogInWithShortLivedAuthTokenPagestarts the short-lived token exchange (SignInWithShortLivedAuthToken, which optimistically setsaccount.isLoading) and navigates to HOME while still unauthenticated.SignInPagethen remounts withhasInitiatedSAMLLoginreset (it's component-localuseState); becauseaccount.isLoadingis stilltrueand SAML is required, it re-initiates SAML — a full-pagewindow.location.replaceto the IdP that tears the page down before the token exchange can finish. The token is never consumed, a fresh one is minted on the way back, and it loops. The root problem is thataccount.isLoadingis set for two different reasons (about to start SAML vs. a short-lived token exchange already in flight) andSignInPagecould not tell them apart.This gates
shouldInitiateSAMLLoginon the existingRAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKENflag, which is set only while a short-lived token exchange is in flight (ingetShortLivedLoginParams) but was previously never read.SignInPageno longer re-initiates SAML during the exchange, so the exchange completes and the session sticks. Because the flag is set for the exact duration of the exchange, the fix is independent of theaccount.isLoadingtiming race and holds regardless of IdP/device/network speed. The legitimate first SAML initiation (no exchange in flight) is unchanged.Fixed Issues
$ #95810
PROPOSAL:
Tests
Prerequisites
First, edit
src/libs/actions/Session/index.tsto reproduce the SAML-loop bug conditions:Now, you need to setup a local PROD web server so you are able to perform the entire SAML flow:
new.expensify.comnpm run setup-https, plus one host, written to the cert path. The mkcert CA is already installed, so the new cert is trusted automatically, anddev.new.expensify.comkeeps working.www.expensify.comalone — it must keep resolving to real prod Auth..env.productionis already correct out of the box —EXPENSIFY_URL=https://www.expensify.com/,NEW_EXPENSIFY_URL=https://new.expensify.com/, andUSE_WEB_PROXY=false(talks straight to prod). No edits needed.npm run buildsetsENV=production→ reads.env.production→ outputs todist/(requires bun).distashttps://new.expensify.com(SPA-aware, on :443)To clean up the PROD setup, run these:
Test steps
Offline tests
N/A
QA Steps
It's only possible to test on PROD environment.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-07-15.at.12.18.21.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-07-15.at.11.03.00.mov