Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions packages/functional-tests/lib/targets/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export type Credentials = Awaited<ReturnType<AuthClient['signUp']>> & {
password: string;
secret?: string;
sessionToken?: string;
verified?: boolean;
};

export abstract class BaseTarget {
Expand Down
1 change: 1 addition & 0 deletions packages/functional-tests/lib/targets/local.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export class LocalTarget extends BaseTarget {
return {
email,
password,
verified: options.preVerified === 'true',
...result,
} as Credentials;
}
Expand Down
1 change: 1 addition & 0 deletions packages/functional-tests/lib/targets/remote.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export abstract class RemoteTarget extends BaseTarget {
return {
email,
password,
verified: preVerified === 'true',
...creds,
};
}
Expand Down
37 changes: 37 additions & 0 deletions packages/functional-tests/tests/oauth/syncSignIn.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,43 @@ test.describe('severity-1 #smoke', () => {
await expect(signin.cachedSigninHeading).toBeVisible();
await expect(page.getByText(email)).toBeVisible();
});

test('can sign in to OAuth after abandoning sync confirmation code', async ({
target,
syncBrowserPages: { page, signin, signinTokenCode, relier },
testAccountTracker,
}) => {
const syncCredentials = await testAccountTracker.signUpSync();

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

Choose a reason for hiding this comment

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

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?

);
await signin.fillOutEmailFirstForm(syncCredentials.email);
await signin.fillOutPasswordForm(syncCredentials.password);

// Confirm we reached the token code page, but intentionally skip entering the code
await expect(page).toHaveURL(/signin_token_code/);

// Navigate to 123done without completing sync verification
await relier.goto();
await relier.clickEmailFirst();

// FxA sees the existing session and shows cached account
await expect(signin.cachedSigninHeading).toBeVisible();
await signin.signInButton.click();

// We get a signin code, because we are using a restmail address, and forces
// verification. ie Must verify will always be set on this client.
await expect(page).toHaveURL(/signin_token_code/);
const signinCode = await target.emailClient.getVerifyLoginCode(
syncCredentials.email
);
await signinTokenCode.fillOutCodeForm(signinCode);

// OAuth sign-in should succeed even though the sync session was not verified
expect(await relier.isLoggedIn()).toBe(true);
});
});

test.describe('signin to Sync after OAuth', () => {
Expand Down
8 changes: 8 additions & 0 deletions packages/fxa-dev-launcher/profile.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const CONFIGS = {
},
};

const isNightly = /Nightly/gi.test(process.env.FIREFOX_BIN || '');
const env = process.env.FXA_ENV || 'local';
const FXA_DESKTOP_CONTEXT =
process.env.FXA_DESKTOP_CONTEXT || 'oauth_webchannel_v1';
Expand All @@ -65,7 +66,14 @@ if (!fxaEnv) {
};
}

const nightlyProfile = isNightly
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@dschom dschom Feb 25, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

? {
'browser.smartwindow.enabled': true,
}
: {};

const fxaProfile = {
...nightlyProfile,
// enable debugger and toolbox
'devtools.chrome.enabled': true,
'devtools.debugger.remote-enabled': true,
Expand Down
8 changes: 7 additions & 1 deletion packages/fxa-settings/src/components/App/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,13 @@ export const App = ({
integration.data?.service || ''
);

if (userFromBrowser?.sessionToken) {
// Don't intialize session state from partially succesful firefox logins (ie sync, relay, smartwindow).
// This reprensents an abandonded login. Basically the user hasn't actually confirmed the session yet, so
// don't assume it's valid.
if (
userFromBrowser?.sessionToken &&
userFromBrowser.verified === true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

) {
const isValidSession = await session.isValid(
userFromBrowser.sessionToken
);
Expand Down
8 changes: 8 additions & 0 deletions packages/fxa-settings/src/lib/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,15 @@
const account = currentAccount();
if (account) {
account.sessionToken = undefined;
account.verified = undefined;

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.
account.sessionVerified = undefined;

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.
account.hasPassword = undefined;

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
user controlled input
.

currentAccount(account);

// If there was a state change, dispatch an event
dispatchStorageEvent('accounts');
dispatchStorageEvent('currentAccountUid');
}
} catch (e) {
// noop
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from '../../../models';
import firefox from '../../channels/firefox';
import { hardNavigate } from 'fxa-react/lib/utils';
import { discardSessionToken } from '../../cache';

export type OAuthFlowRecoveryResult = {
isRecovering: boolean;
Expand Down Expand Up @@ -67,9 +68,16 @@ export function useOAuthFlowRecovery(
currentParams.set('code_challenge', params.code_challenge);
}
if (params.code_challenge_method) {
currentParams.set('code_challenge_method', params.code_challenge_method);
currentParams.set(
'code_challenge_method',
params.code_challenge_method
);
}

// Discard the current session token. We need to do this, otherwise the cached session will be used
// and the UI will show an option to enter a password. This can lead to an infinite sign in loop.
discardSessionToken();

// Redirect to /signin - user will re-enter password
hardNavigate(`/signin?${currentParams.toString()}`);

Expand Down
4 changes: 4 additions & 0 deletions packages/fxa-settings/src/lib/oauth/hooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,11 @@ export function useFinishOAuthFlowHandler(
} catch (error) {
// We only care about these errors, else just tell the user to try again.
if (
// Signals that session requires verification, ie a mustVerify state
error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno ||
// Signals that session requires totp verification
error.errno === AuthUiErrors.TOTP_REQUIRED.errno ||
// Signals that we are dealing with an RP that requires a second factor
error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno
) {
return { error };
Expand Down
7 changes: 4 additions & 3 deletions packages/fxa-settings/src/models/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,10 @@ export class Session implements SessionData {

async isValid(token: string): Promise<boolean> {
try {
await this.authClient.sessionStatus(token);
setSessionVerified(true);
return true;
const resp = await this.authClient.sessionStatus(token);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const isVerified = resp.state === 'verified';
setSessionVerified(isVerified);
return isVerified;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

} catch (e) {
setSessionVerified(false);
return false;
Expand Down
28 changes: 22 additions & 6 deletions packages/fxa-settings/src/pages/Signin/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -513,17 +513,33 @@ const getOAuthNavigationTarget = async (
);
if (error) {
if (
error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

error.errno === AuthUiErrors.TOTP_REQUIRED.errno ||
error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno
) {
GleanMetrics.login.error({ event: { reason: error.message } });
// If user already has TOTP enabled, send them to enter their code instead of setup
const hasTotp =
locationState.verificationMethod === VerificationMethods.TOTP_2FA;

const to = (() => {
// If the user doesn't have totp, and encountered an unverified_session error, send them
// to signin_token_code, this is a 'mustVerify' case
if (
locationState.verificationMethod !== VerificationMethods.TOTP_2FA &&
error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno
) {
return `/signin_token_code${navigationOptions.queryParams || ''}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// If user already has TOTP enabled, send them to enter their code instead of setup
if (locationState.verificationMethod === VerificationMethods.TOTP_2FA) {
return `/signin_totp_code${navigationOptions.queryParams || ''}`;
}

// Otherwise, we are dealing with an RP that requires totp setup. Send them there.
return `/inline_totp_setup${navigationOptions.queryParams || ''}`;
})();

return {
to: hasTotp
? `/signin_totp_code${navigationOptions.queryParams || ''}`
: `/inline_totp_setup${navigationOptions.queryParams || ''}`,
to,
locationState,
};
}
Expand Down
Loading