Repository navigation
fix: sign out user when session expires - #91619
Conversation
|
@marcochavezf 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: be102d76e0
ℹ️ 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".
| // Prevent re-authentication if authentication with shortLiveToken is in progress | ||
| if (isAuthenticatingWithShortLivedToken) { | ||
| Log.hmmm('[Reauthenticate] Authentication with shortLivedToken is in progress. Re-authentication aborted.', { | ||
| if (account?.isLoading) { |
There was a problem hiding this comment.
Avoid clearing short-lived auth state before account is loaded
When isAuthenticatingWithShortLivedToken is true but ONYXKEYS.ACCOUNT has not been hydrated yet, account is undefined so this check falls through to the "stale state" branch and clears the short-lived auth flags. That can happen during startup races (session loaded before account), and then reauthenticate() proceeds with normal credentials that may not exist yet, potentially redirecting the user to sign-in in the middle of a valid short-lived-token login. Previously this path always aborted while short-lived auth was active, so this introduces a real login interruption risk.
Useful? React with 👍 / 👎.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f96b37385
ℹ️ 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".
| }) | ||
| .catch((error) => { | ||
| if (isExpiredSessionError(error)) { | ||
| return handleExpiredSession(request, isFromSequentialQueue, undefined, true).catch((reauthenticationError) => { |
There was a problem hiding this comment.
Respect offline state before retrying HTTP 407 auth flow
The new HTTP-407 catch path forces handleExpiredSession(..., true), which bypasses the offline guard and can trigger full reauthentication retries even when the app is already offline by the time reauth starts. In that case, reauthenticate() keeps retrying Authenticate and eventually redirects to sign-in after max retries, whereas the existing body-407 path explicitly avoids reauth while offline. This creates a regression for flaky connections where users can be signed out solely because connectivity dropped right after receiving the 407 response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
What do you think about this comment @nabi-ebrahimi? I think it makes sense, but I want to know your thoughts. Thank you.
There was a problem hiding this comment.
@brunovjk Thank you for the feedback and for pointing this out. I agree that the HTTP 407 path should respect offline state in the same way as the existing body-407 path. Without this safeguard, unstable network conditions could incorrectly trigger reauthentication retries and potentially sign the user out. I’ve updated the thrown-407 flow to honor the offline check and added regression test coverage for this scenario.
|
Using the code snippet you provided, I can reproduce the bug in the last main, still in dev, normally. However, in our PR here, the error message doesn't appear, but we aren't redirected to the login screen. Screen.Recording.2026-05-26.at.20.18.31.movDid everything go well in your tests? |
@brunovjk, thanks for checking. That matches what I’m seeing as well: on |
|
Thank you for the updates @nabi-ebrahimi. But it still won't "log out" for me: Screen.Recording.2026-05-28.at.12.12.56.movI have to leave now, when I get back I'll investigate further. |
|
@nabi-ebrahimi, did everything go well in your tests? Thank you. |
@brunovjk, thank you for checking. Yes, everything went well in my testing. I verified the relevant scenarios, and the updated flow behaved as expected in all cases I tested. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari91619_web_chrome.mov |
|
It worked now @nabi-ebrahimi, thank you so much for your hard work. |
|
Great! I will check ASAP. |
|
Hi, @brunovjk, I addressed the ESLint failure by isolating the Reauthentication tests into a dedicated test file. Could you please take another look when you have a chance? |
|
@brunovjk can you prioritize this one please? |
|
@nabi-ebrahimi your mock doesn't seem to work anymore, I changed a bit and was able to test it normally: |
|
🚧 @marcochavezf 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.10-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no docs changes required. This PR is an internal auth/session bug fix — it touches only the re-authentication middleware and session-state logic:
Why no help site update is needed:
No draft PR was created since there are no help site files to change. @nabi-ebrahimi, let me know if you'd like a docs note added anyway (for example, to the Login troubleshooting article) and I'll open a draft help site PR. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.12-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review — no docs changes required I reviewed the changes in this PR against the help site articles under Why: This is a purely internal fix to the re-authentication flow. It changes how the app handles an expired short-lived auth token (support/delegate sessions): instead of getting stuck unable to re-authenticate, the app now detects the completed-but-stale auth state, clears the leftover flags, and gracefully redirects the user to the login page.
If you believe a specific help article should mention this behavior, reply with the article and I'll take another look. |
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.4.14-0 🚀
Bundle Size Analysis (Sentry): |
|
We are still able to reproduce the original issue. 2026-06-17.23-11-40.mp4 |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.14-1 🚀
|
Hi @brunovjk, thanks for the feedback. I'm unable to reproduce this on the latest Screencast.From.2026-06-19.06-54-35.mp4 |
|
@brunovjk Gentle bump.thanks |
|
Sorry for the delay, I'll check it out ASAP. |
|
@nabi-ebrahimi—using the mocks we have, the app stays logged in. I wonder if there's a more reliable way to test this? Screen.Recording.2026-06-23.at.12.54.24.movThanks a lot. |
@brunovjk Thank you for testing this and sharing your feedback. I retested it on staging with a cleaner mock that invalidates the session and clears the stored credentials, which makes the expected sign-in redirect deterministic. With the old mock, valid stored credentials could still exist, so the app could reauthenticate successfully instead of logging out, which is expected. With the updated mock, the app redirects to the sign-in page correctly. Tested Demo ON Staging: Screencast.From.2026-06-24.16-44-08.mp4 |
|
Great! Thank you @nabi-ebrahimi |
Explanation of Change
When short-lived auth tokens (used for support/delegate sessions) expire, the app previously got stuck unable to re-authenticate, requiring users to sign out and back in manually. This change detects when the auth process has completed, but stale state flags remain, clears them, and allows normal re-authentication to proceed automatically.
Fixed Issues
$ #90544
PROPOSAL: #90544 (comment)
Tests
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
Not applicable.
Android: mWeb Chrome
Not applicable.
iOS: Native
Not applicable.
iOS: mWeb Safari
Not applicable.
MacOS: Chrome / Safari
Screen.Recording.2026-05-25.at.6.48.11.PM.mov