bug(settings): Abandoning sync sign flow can cause down stream problems by dschom · Pull Request #20098 · mozilla/fxa

dschom · 2026-02-24T00:06:26Z

Because

When a sync flow was abandoned at the 'confirm code' step, we would see problems on subsequent logins.

This pull request

Ensures the 'unverified session' errors are handled gracefully during oauth flow.
Ensures that we don't overwrite the local cache state with Firefox's user session state if it isn't actually verified. (This contributed to sign in loops and random failures).
Ensures that we actually set the sessionVerified state correctly on the account object cached in local storage. This had a similar effect as the previous issue.
Makes sure that discarding the session token also discards associated flags
Makes sure that discarding the session token is reactive.
Adds a little polish for setting the smart window to be enabled in nightly.

Issue that this pull request solves

Closes: FXA-13160

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

packages/fxa-settings/src/lib/cache.ts

LZoog · 2026-02-24T22:03:10Z

@dschom I just tested it out and this works for the RP redirect case! \o/ But doesn't seem to fix it for the Smart Window case, I'll send a video in Slack.

packages/fxa-settings/src/lib/cache.ts

    const account = currentAccount();
    if (account) {
      account.sessionToken = undefined;
+      account.verified = undefined;


packages/fxa-settings/src/lib/cache.ts

    if (account) {
      account.sessionToken = undefined;
+      account.verified = undefined;
+      account.sessionVerified = undefined;


packages/fxa-settings/src/lib/cache.ts

      account.sessionToken = undefined;
+      account.verified = undefined;
+      account.sessionVerified = undefined;
+      account.hasPassword = undefined;


LZoog · 2026-02-25T17:35:41Z

packages/functional-tests/tests/oauth/syncSignIn.spec.ts

+
+      // Start sign-into-sync flow
+      await page.goto(
+        `${target.contentServerUrl}?context=fx_desktop_v3&service=sync&action=email&`


Just noting, this tests the Sync flow from over a year ago / non-oauth. I think we have multiples of these around so maybe we file a ticket to adjust our tests?

LZoog · 2026-02-25T17:37:06Z

packages/fxa-dev-launcher/profile.mjs

  };
 }

+const nightlyProfile = isNightly


Nice, in about a month we can have this on Release too (it's supposed to be true by default then as well) but it makes it easier in the meantime.

Yeah, when testing I had to restart nightly alot. On successful sign in with smart window, smart window is then 'signed in', and I actually am not sure if there's a way to 'sign out' other than restarting.

I think you currently have to switch to classic and then sign out from there. It seems to be ever-evolving hence my "I think" lol.

packages/fxa-settings/src/components/App/index.tsx

LZoog · 2026-02-25T17:52:59Z

packages/fxa-settings/src/lib/hooks/useOAuthFlowRecovery/index.tsx

      }

+      // Discard the current session token. We need to do this, otherwise, the cached session will be used
+      // and a new password will not be requested. This can lead to an infinite sign in loop.


nit: add the word entry or input like, "and a new password entry will not be requested" just to disambiguate between a fully new password?

packages/fxa-settings/src/lib/cache.ts

LZoog · 2026-02-25T17:54:49Z

packages/fxa-settings/src/models/Session.ts

+      const resp = await this.authClient.sessionStatus(token);
+      const isVerified = resp.state === 'verified';
+      setSessionVerified(isVerified);
+      return isVerified;


Aha, this totally makes sense given what we did at the GQL level with this.

This was bug was driving me crazy... I couldn't wrap my head around why the session would magically become verified 😅. I wouldn't doubt this will fix several other issues... since it was very common to get a false positive on session verified state.

Ah man, I just mentioned this in Slack, but I just realized now that session.isValid does the same thing that session.isSessionVerified does, so I think we probably want to leave this as-is and use session.isSessionVerified instead...

LZoog · 2026-02-25T17:59:43Z

packages/fxa-settings/src/pages/Signin/utils.ts

+          locationState.verificationMethod !== VerificationMethods.TOTP_2FA &&
+          error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno
+        ) {
+          return `/signin_token_code${navigationOptions.queryParams || ''}`;


This file just gets hairer and hairier 😅 (Does this send the email also?)

Yes, we should rethink how we do this... Using errors like this for flow control isn't ideal.

dschom · 2026-02-25T02:43:32Z

packages/fxa-settings/src/components/App/index.tsx

+        // don't assume it's valid.
+        if (
+          userFromBrowser?.sessionToken &&
+          userFromBrowser.verified === true


verified will be false when if the sync sign wasn't finalized. The sync menu will say 'finish setting up your account'. This intermediate state causes problems because the session is essentially unconfirmed, Overwriting the account state cached in local storage with a partially verified session, violated several down stream assumptions in the code, and gave the perception that the local storage cache wasn't responding to changes, when in fact it was.

dschom · 2026-02-25T02:44:53Z

packages/fxa-settings/src/models/Session.ts

-      await this.authClient.sessionStatus(token);
-      setSessionVerified(true);
-      return true;
+      const resp = await this.authClient.sessionStatus(token);


This was a big problem. It was updating the cache such that the session was always. 'verified' when in fact it very well might not have been. When I finally fixed this, everything else fell into place.

dschom · 2026-02-25T02:45:48Z

packages/fxa-settings/src/pages/Signin/utils.ts

    );
  if (error) {
    if (
+      error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno ||


This state is definitely possible, and like the other two state indicates we need a confirmation code step. It happens when the session is in 'mustVerify' mode.

dschom · 2026-02-25T18:09:13Z

packages/fxa-settings/src/models/Session.ts

+      const resp = await this.authClient.sessionStatus(token);
+      const isVerified = resp.state === 'verified';
+      setSessionVerified(isVerified);
+      return isVerified;


This was bug was driving me crazy... I couldn't wrap my head around why the session would magically become verified 😅. I wouldn't doubt this will fix several other issues... since it was very common to get a false positive on session verified state.

dschom · 2026-02-25T18:10:03Z

packages/fxa-settings/src/pages/Signin/utils.ts

+          locationState.verificationMethod !== VerificationMethods.TOTP_2FA &&
+          error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno
+        ) {
+          return `/signin_token_code${navigationOptions.queryParams || ''}`;


Yes, we should rethink how we do this... Using errors like this for flow control isn't ideal.

LZoog

LGTM. Let's of course leave some notes for QA.

Sync unverified session RP redirect -> prompted for code
Sync unverified session Smart Window sign-in -> not shown cached sign-in, session token is discarded so we have the login restart and user is prompted for password
non-Sync non-2FA unverified session RP redirect -> works as expected going to the RP
non-Sync non-2FA unverified session Smart Window login -> works as expected allowing login

Poked around for other flows too. Note I didn't make sure that the signin token email is sent every time, but we already have some issues filed around signin emails.

With the change for, "only use the browser session token if it's fully verified", I think we should file a ticket for:

a bug I noticed for can_link_account: if you're in a partially verified state (sign in or sign up) and then click "Finish account set up" I'm taken to email-first. If I try to enter an email that is different than the one I'm partially signed in as, for an account that does NOT exist, we show the old merge warning dialog. The user shouldn't be able to continue in this state. Note I'm not sure off the top of my head what the fix is, because with the "can_link_account_uid capability, we send the UID up and for a new account it doesn't exist, but we don't want to show the old merge warning.
To help avoid the above scenario and to help the user sign in more easily, we can at least use the email from that isSignedInUser request, and prefill the email-first page
Just noting, in #20093, we removed the "Use another account" link if the user is currently signed into Firefox desktop. I think we want isSignedIntoFirefoxDesktop set, even if the session is not fully verified, because the entire point of removing it is to prevent users from hitting the "merge stop" (e.g. not being able to continue with the login, since if they start a login on a profile, they're going to have to use a different profile if they want to sign into a different account).

Thank you for the work on this ticket! \o/

…r login Because: - If a user only partially loged in from a FF sign in (sync, smartwindow, etc..), then subsquent logins would break. This Commit: - Configures nightly with smart window on - Recreates issue with abadoning sync as functional test - Fixes bogus cached account state - Discard old session token in oauth recovery flow - Handle unverified session scenarios during oauth flow - Make sure functional test testing credentials include verified flag

dschom force-pushed the worktree-FXA-13160 branch from 1d86eb7 to 35e3af4 Compare February 24, 2026 16:23

github-advanced-security bot found potential problems Feb 24, 2026

View reviewed changes

packages/fxa-settings/src/lib/cache.ts Fixed Show fixed Hide fixed

packages/fxa-settings/src/lib/cache.ts Fixed Show fixed Hide fixed

dschom force-pushed the worktree-FXA-13160 branch from 35e3af4 to ae22753 Compare February 24, 2026 16:57

dschom force-pushed the worktree-FXA-13160 branch from ae22753 to 685933c Compare February 25, 2026 02:33

dschom marked this pull request as ready for review February 25, 2026 02:35

dschom requested a review from a team as a code owner February 25, 2026 02:35

github-advanced-security bot found potential problems Feb 25, 2026

View reviewed changes

dschom force-pushed the worktree-FXA-13160 branch from 685933c to cf0ea01 Compare February 25, 2026 17:08

LZoog reviewed Feb 25, 2026

View reviewed changes

dschom commented Feb 25, 2026

View reviewed changes

dschom force-pushed the worktree-FXA-13160 branch from cf0ea01 to e7b7864 Compare February 25, 2026 18:37

LZoog approved these changes Feb 25, 2026

View reviewed changes

dschom force-pushed the worktree-FXA-13160 branch from e7b7864 to 62a7ce2 Compare February 25, 2026 18:49

dschom merged commit 2b04bd0 into main Feb 25, 2026
22 checks passed

dschom deleted the worktree-FXA-13160 branch February 25, 2026 19:19

Conversation

dschom commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Uh oh!

Uh oh!

Uh oh!

LZoog commented Feb 24, 2026

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Check warning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dschom Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LZoog left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dschom commented Feb 24, 2026 •

edited

Loading

dschom Feb 25, 2026 •

edited

Loading

LZoog left a comment •

edited

Loading