Skip to content

Clear SAML short-lived-token guard on the no-JSON early-return path (based on #94082) #95017

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
grgia merged 4 commits into from
Jul 10, 2026
+71 −13
Merged

Clear SAML short-lived-token guard on the no-JSON early-return path (based on #94082) #95017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
feb15e0
Fix mobile SAML/Okta double login by guarding reauth during in-app br…
MelvinBot Jun 19, 2026
afeead7
Clear SAML short-lived-token guard on the no-JSON early-return path
MelvinBot Jul 1, 2026
f9660aa
Route the no-JSON SAML callback through the shared failure recovery path
MelvinBot Jul 1, 2026
7e88ae4
Merge remote-tracking branch 'origin/main' into claude-samlSsoDoubleL…
MelvinBot Jul 7, 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
16 changes: 16 additions & 0 deletions src/libs/actions/Session/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,21 @@ function signInWithShortLivedAuthToken(authToken: string, isSAML = false) {
NetworkStore.setLastShortAuthToken(authToken);
}

/**
* Marks (or clears) that a short-lived-token sign-in is in progress.
*
* This should be set to `true` before opening the in-app browser for native SAML sign-in so the
* reauthentication middleware won't race against the SAML callback when the app resumes from the
* browser. On resume, `reconnectApp()` fires with the now-expired authToken and gets a 407; without
* this flag set, `reauthenticate()` takes the `isSAMLRequired` branch and calls `redirectToSignIn()`,
* wiping the session before the SAML callback can sign the user back in. `signInWithShortLivedAuthToken`
* resets this flag automatically once the sign-in succeeds, so the caller only needs to reset it if the
* browser is cancelled or fails.
*/
function setIsAuthenticatingWithShortLivedToken(isAuthenticating: boolean) {
Onyx.set(ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN, isAuthenticating);
}

/**
* Sign the user into the application. This will first authenticate their account
* then it will create a temporary login for them which is used when re-authenticating
Expand Down Expand Up @@ -1696,6 +1711,7 @@ export {
signInWithValidateCodeAndNavigate,
initAutoAuthState,
signInWithShortLivedAuthToken,
setIsAuthenticatingWithShortLivedToken,
cleanupSession,
signOut,
signOutAndRedirectToSignIn,
Expand Down
38 changes: 25 additions & 13 deletions src/pages/signin/SAMLSignInPage/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import Log from '@libs/Log';
import {handleSAMLLoginError, postSAMLLogin} from '@libs/LoginUtils';
import Navigation from '@libs/Navigation/Navigation';

import {clearSignInData, setAccountError, signInWithShortLivedAuthToken} from '@userActions/Session';
import {clearSignInData, setAccountError, setIsAuthenticatingWithShortLivedToken, signInWithShortLivedAuthToken} from '@userActions/Session';

import CONFIG from '@src/CONFIG';
import CONST from '@src/CONST';
Expand All @@ -32,6 +32,8 @@ function SAMLSignInPage() {
const hasOpenedAuthSession = useRef(false);

const handleExitSAMLFlow = useCallback(() => {
// Clear the guard we set before opening the in-app browser so we don't block future reauthentication
setIsAuthenticatingWithShortLivedToken(false);
Navigation.isNavigationReady().then(() => {
Navigation.goBack();
clearSignInData();
Expand All @@ -51,20 +53,19 @@ function SAMLSignInPage() {
const searchParams = new URLSearchParams(new URL(url).search);
const jsonParam = searchParams.get('json');

if (!jsonParam) {
Log.hmmm('SAMLSignInPage - No JSON parameter found in callback URL');
return;
}

let shortLivedAuthToken: string | null = null;
try {
const decodedData = JSON.parse(jsonParam) as Record<string, string | null>;
shortLivedAuthToken = decodedData.shortLivedAuthToken ?? null;
if (decodedData.error) {
Log.hmmm('SAMLSignInPage - SAML login returned error', {error: decodedData.error});
if (jsonParam) {
try {
const decodedData = JSON.parse(jsonParam) as Record<string, string | null>;
shortLivedAuthToken = decodedData.shortLivedAuthToken ?? null;
if (decodedData.error) {
Log.hmmm('SAMLSignInPage - SAML login returned error', {error: decodedData.error});
}
} catch (parseError) {
Log.hmmm('SAMLSignInPage - Failed to parse JSON parameter', {error: parseError});
}
} catch (parseError) {
Log.hmmm('SAMLSignInPage - Failed to parse JSON parameter', {error: parseError});
} else {
Log.hmmm('SAMLSignInPage - No JSON parameter found in callback URL');
}

if (!account?.isLoading && credentials?.login && shortLivedAuthToken) {
Expand All @@ -73,6 +74,11 @@ function SAMLSignInPage() {
return;
}

// The browser returned but we couldn't sign in (no JSON parameter, a parse failure, or no token), so clear
// 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);

@grgia grgia Jul 3, 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.

when should we use this vs clearShortLivedAuthState? @thelullabyy

@grgia grgia Jul 3, 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.

@MelvinBot

@MelvinBot MelvinBot Jul 7, 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.

Short answer: setIsAuthenticatingWithShortLivedToken(false) is correct here — it's not interchangeable with clearShortLivedAuthState.

They differ in scope:

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).

clearSignInData();
setAccountError(translate('common.error.login'));
Navigation.isNavigationReady().then(() => {
Expand All @@ -90,6 +96,12 @@ function SAMLSignInPage() {
return;
}
hasOpenedAuthSession.current = true;
// 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.
Comment on lines +99 to +103

@grgia grgia Jul 3, 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.

NAB these comments are too long

setIsAuthenticatingWithShortLivedToken(true);

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Await the SAML reauth guard before opening the browser

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 👍 / 👎.

@MelvinBot MelvinBot Jul 2, 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.

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.

openAuthSessionAsync(SAMLUrl, CONST.SAML_REDIRECT_URL)
.then((response: WebBrowserAuthSessionResult) => {
if (response.type !== 'success') {
Expand Down
30 changes: 30 additions & 0 deletions tests/actions/SessionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,36 @@ describe('Session', () => {
redirectToSignInSpy.mockRestore();
});

test('setIsAuthenticatingWithShortLivedToken(true) makes reauthenticate abort (blocks the SAML resume race)', async () => {
let isAuthenticatingWithShortLivedToken: OnyxEntry<boolean>;
Onyx.connect({
key: ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
callback: (val) => (isAuthenticatingWithShortLivedToken = val),
});

// Given the SAML sign-in flow set the guard before opening the in-app browser
SessionUtil.setIsAuthenticatingWithShortLivedToken(true);
await waitForBatchedUpdates();
expect(isAuthenticatingWithShortLivedToken).toBe(true);

const redirectToSignInSpy = jest.spyOn(SignInRedirect, 'default').mockImplementation(() => Promise.resolve());

// When the app resumes and reconnectApp's 407 triggers reauthenticate
const result = await reauthenticate('TestCommand');
await waitForBatchedUpdates();

// Then reauthenticate aborts without redirecting to sign in, so the SAML callback can complete
expect(result).toBe(false);
expect(redirectToSignInSpy).not.toHaveBeenCalled();

// When the browser is cancelled/fails, the guard is cleared so future reauthentication isn't blocked
SessionUtil.setIsAuthenticatingWithShortLivedToken(false);
await waitForBatchedUpdates();
expect(isAuthenticatingWithShortLivedToken).toBe(false);

redirectToSignInSpy.mockRestore();
});

test('reauthenticate proceeds even when a legacy session.isAuthenticatingWithShortLivedToken=true is persisted (recovers stuck users)', async () => {
// Given a session in Onyx that still carries the legacy stuck flag from before the RAM-only migration.
// The Session type no longer declares the field, so cast to write the legacy shape.
Expand Down
Loading