Clear SAML short-lived-token guard on the no-JSON early-return path (based on #94082)#95017
Conversation
…owser Co-authored-by: Rory Abraham <roryabraham@users.noreply.github.com>
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
🤖 Fixed the failing check. What was failing: The only failing check was Fix: I restored the standard Still needed from the human co-author (
I intentionally did not check any of those boxes or fill in Tests/QA, since they're human attestations that you actually performed the testing. |
|
🤖 Status update — nothing left for me to fix here. All automated checks are green. The only remaining red check is That one can only be cleared by a human. Before marking this PR ready for review,
I don't check those boxes or fill in Tests/QA myself, since they're attestations that a human actually performed the testing. |
|
@MelvinBot Please replace all |
melvin-bot
Bot
requested review from
JmillsExpensify and
thelullabyy
and removed request for
a team
|
@thelullabyy 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] |
|
🤖 Done, @thelullabyy:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afeead7dff
ℹ️ 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".
| // The browser returned but there's nothing to sign in with, so clear the guard we set before | ||
| // opening it. Otherwise it stays true, and since loginCallback URLs hide the back button and leave | ||
| // account.isLoading true, future reauthenticate() calls would keep aborting and the user gets stuck. | ||
| setIsAuthenticatingWithShortLivedToken(false); |
There was a problem hiding this comment.
When the IdP redirects back to a loginCallback URL without json, the code above has already hidden the back button, but this branch only drops the reauth guard and returns. In that callback-failure scenario the user remains on the SAML loading screen with the sign-in state still loading and no error/navigation unless some later request happens to recover, so this path should follow the other callback-failure cleanup path instead of returning immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in f9660aa.
The no-json case was only clearing the guard and returning, so the user was left on the SAML loading screen (back button hidden, account.isLoading still true) with no error or navigation. I removed the early return and let this case fall through to the shared callback-failure recovery path, so a no-json callback now also runs clearSignInData() + setAccountError() + navigate-to-HOME, exactly like the parse-failure / missing-token cases.
The json-parsing is now guarded by if (jsonParam) { … } else { Log.hmmm('… No JSON parameter …') }, shortLivedAuthToken stays null, and the single recovery block at the bottom handles all three failure modes (no JSON, parse failure, missing token). See SAMLSignInPage/index.native.tsx:47-82.
Note: I couldn't run the local AI checks (lint/typecheck/prettier/tests) in this run — the tooling was unavailable in my environment. The PR's CI will validate them.
|
🤖 No fix to push — the only remaining red check is the PR Reviewer Checklist, which needs a human. Status of the checks:
|
|
Please check and address this comment #95017 (comment) @MelvinBot |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
🤖 Addressed the review comment from What the comment flagged: the no- Fix (f9660aa): removed the early return so the no- |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Nothing left for me to fix here — the real failure is already resolved. Current check status: 33 passing, 4 skipping, 1 failing. The only red check is The remaining red requires the human co-author (
These are human attestations that the testing was actually performed, so I've deliberately left them for you. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9660aaaee
ℹ️ 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".
| // wiping the session before the SAML callback can sign the user back in. Setting this guard up front makes | ||
| // reauthenticate() abort while the SAML sign-in is in progress. signInWithShortLivedAuthToken() resets it on | ||
| // success; the cancel/error/failure paths reset it via handleExitSAMLFlow and handleNavigationStateChange. | ||
| setIsAuthenticatingWithShortLivedToken(true); |
There was a problem hiding this comment.
In the SAML resume race this guard needs to be visible to Reauthentication.ts before the app is backgrounded. setIsAuthenticatingWithShortLivedToken(true) only kicks off an Onyx.set, while openAuthSessionAsync is invoked immediately on the next line; Onyx subscriber updates are asynchronous in this codebase (for example, auth-token updates are manually mirrored because subscribers update later). If the in-app browser backgrounds/resumes the app before the RAM-only key subscriber observes true, the 407 path can still enter reauthenticate() and redirect to sign-in, so the original logout/freeze can still occur for SAML users. Return/await the Onyx write or otherwise update the guard synchronously before launching the browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Codex bot describes the mechanism correctly but the race it predicts can't actually happen here, so no change is needed.
Why it's safe: The dangerous window the bot worries about — browser backgrounds the app → app resumes → reconnectApp() → 407 → reauthenticate() — is driven entirely by the native AppState 'change' listener at App.ts:279, which is a macrotask. The guard write at SAMLSignInPage/index.native.tsx:98 propagates to the Reauthentication.ts subscriber via Onyx, whose broadcast resolves on the microtask queue (Reauthentication.ts:51-56). Microtasks always flush before the next macrotask, so isAuthenticatingWithShortLivedToken is true before the app can even background — let alone resume and fire reconnectApp() seconds/minutes later once the user finishes IdP + MFA. openAuthSessionAsync returns a promise and does not synchronously re-enter JS or fire the resume event.
On the authToken analogy: the "manually mirrored" example the bot cites (NetworkStore.setAuthToken at NetworkStore.ts:81) exists because the very next network request in the same synchronous tick must see the new token — a real same-tick dependency. This guard has no same-tick reader; it's only read after a full background→resume cycle, so the two situations aren't comparable.
Confidence & caveat
High confidence on the JS event-loop ordering. There's no synchronous path from openAuthSessionAsync to reconnectApp(). If you wanted belt-and-suspenders robustness independent of Onyx's broadcast timing, you could mirror the flag synchronously into the Reauthentication.ts module variable the same way setAuthToken does — but that's an optional hardening, not a fix for a live bug.
|
🤖 No code fix needed — the only failing check is PR Reviewer Checklist / checklist, and it's not a code problem. It fails because the reviewer checklist isn't completely filled out ( Everything else is green: To clear it: the assigned reviewer should copy the checklist from REVIEWER_CHECKLIST.md, post it as a comment, and check every box. The check re-runs automatically once a completed checklist is present. |
|
@MelvinBot Please check this comment #95017 (comment) |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
|
🤖 The only failing check is PR Reviewer Checklist / checklist, and it's not a code failure — nothing to push a fix for. That check fails because the reviewer's checklist hasn't been completed yet: the run reports Every other check on this PR is passing (tests, typecheck, lint, prettier, perf, build, etc.), so there's no code change needed here. |
| // Opening the in-app browser backgrounds the app. When it returns, the app resumes and fires | ||
| // reconnectApp() with the expired authToken, which 407s and triggers reauthenticate() -> redirectToSignIn(), | ||
| // wiping the session before the SAML callback can sign the user back in. Setting this guard up front makes | ||
| // reauthenticate() abort while the SAML sign-in is in progress. signInWithShortLivedAuthToken() resets it on | ||
| // success; the cancel/error/failure paths reset it via handleExitSAMLFlow and handleNavigationStateChange. |
There was a problem hiding this comment.
NAB these comments are too long
|
@thelullabyy could you ask melv to merge main |
| // the guard we set before opening it and send the user back to a clean state. Otherwise the guard stays | ||
| // true, and since loginCallback URLs hide the back button and leave account.isLoading true, the user gets | ||
| // stuck on the loading screen with future reauthenticate() calls aborting. | ||
| setIsAuthenticatingWithShortLivedToken(false); |
There was a problem hiding this comment.
when should we use this vs clearShortLivedAuthState? @thelullabyy
There was a problem hiding this comment.
Short answer: setIsAuthenticatingWithShortLivedToken(false) is correct here — it's not interchangeable with clearShortLivedAuthState.
They differ in scope:
setIsAuthenticatingWithShortLivedToken(false)flips only the RAM-only guardRAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN.clearShortLivedAuthState()flips that guard and wipes theSESSIONshort-lived fields (signedInWithShortLivedAuthToken,isSupportAuthTokenUsed), returning a promise.
This failure path only ever set the guard (right before opening the browser) and never reached signInWithShortLivedAuthToken — so the SESSION short-lived fields were never populated by this flow and there's nothing else to clear. The setter is the exact inverse of what we set, matching handleExitSAMLFlow.
clearShortLivedAuthState() is the heavier recovery helper that reauthenticate() uses when it detects a stale guard after a real short-lived sign-in may have populated SESSION — there it needs to wipe the whole state, not just the guard. We don't need that reach here, so I'd keep the minimal setter.
Separately, I've merged main into the branch (the only conflict was the reformatted import block, which I resolved).
|
@thelullabyy would you merge main and fix conflicts and address the comment |
|
@MelvinBot Please merge main and address this comment as well #95017 (comment) |
…oginClearGuard Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com> # Conflicts: # src/pages/signin/SAMLSignInPage/index.native.tsx
|
@grgia We are good to do next review round now |
|
🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@grgia @JmillsExpensify @roryabraham Anything to QA here specifically? |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.4.33-0 🚀
|
|
🤖 Help site review: no docs changes required I reviewed the changes in this PR against the help site articles under Why:
If you'd like a troubleshooting note added anyway (e.g., a mobile "if you're prompted to sign in twice after your SSO session expires" entry), let me know and I'll draft one. |
7 participants
Explanation of Change
This PR is based on #94082 and includes all of its changes, plus an additional fix requested in review.
Background (from #94082). On mobile, when a SAML/Okta SSO user returns to the app after their IdP session has gone idle and completes re-authentication + MFA in the in-app browser, the app briefly freezes, logs them out, and forces a second login. The root cause is a race on app resume: opening the in-app browser backgrounds the app, and when it closes
reconnectApp()fires with the now-expiredauthToken, gets a407, andreauthenticate()takes theisSAMLRequiredbranch and callsredirectToSignIn()— wiping the session before the SAML callback can sign the user in. The fix sets theisAuthenticatingWithShortLivedTokenguard (RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN) totruebefore opening the in-app browser soreauthenticate()aborts for the entire SAML browser session. The guard is reset tofalseon the cancel/error/callback-failure paths, and on successsignInWithShortLivedAuthToken()takes over flag management.Additional fix in this PR (review suggestion).
handleNavigationStateChange()has an early return when the callback URL has nojsonparameter (SAMLSignInPage/index.native.tsx:50). That return happens before the callback-failure cleanup path that clears the guard, so the guard could remaintrue. BecauseloginCallbackURLs hide the back button (shouldShowNavigation(false)) andaccount.isLoadingstaystrue, a stuck guard means futurereauthenticate()calls keep aborting instead of recovering or redirecting — leaving the user unable to recover. This PR clearsisAuthenticatingWithShortLivedTokenon that early-return path as well, so the guard is always cleared whenever the browser returns without a usable token.Changes are native-only (
SAMLSignInPage/index.native.tsx), matching where the bug occurs; desktop uses a different SAML implementation.AI tests run locally by MelvinBot (informational — does not replace human Tests/QA)
npx prettieron changed file — pass (unchanged)./scripts/lint.shon changed files — passnpm run typecheck-tsgo— passnpm test -- tests/actions/SessionTest.ts— pass (32 tests)npm run react-compiler-compliance-check check src/pages/signin/SAMLSignInPage/index.native.tsx— COMPILEDThis change can only be fully validated on a device with a real Okta SAML session, which MelvinBot cannot operate.
Fixed Issues
$ #86705
PROPOSAL: #86705 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
None.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari