fix(passkeys): extend non-decoupled keys-required sign-in to passkey, 2FA, and recovery flows by vpomerleau · Pull Request #20752 · mozilla/fxa

vpomerleau · 2026-06-17T06:33:33Z

Because

PR fix(mobile): Redirect passwordless users to set_password in OAuthNative flows when Sync is not decoupled #20750 (FXA-13947 / FXA-13946) fixed keyless logins for passwordless OTP and third-party-auth sign-ins to non-Sync Firefox services that request Sync-scoped keys (e.g. Android VPN) when the browser has not decoupled Sync — but it did not cover passkey sign-in, nor the remaining 2FA/recovery completion paths.
Spotted during engineering review of fix(mobile): Redirect passwordless users to set_password in OAuthNative flows when Sync is not decoupled #20750 (not a QA report): a passwordless account signing in with a passkey (or completing TOTP / recovery-code / recovery-phone) to such a service on a non-decoupled browser still receives a keyless login (Sync scope granted, but keys_jwe is None).

This pull request

Passkey redirect — drives the password-step decision from integration.requiresPasswordForLogin(supportsKeysOptionalLogin) (a decoupling-aware, client-side check) instead of the server's service === 'sync' signal. Routes a passwordless account to /post_verify/set_password and an existing-password account to /signin_passkey_fallback, deferring the fxaLogin / fxaOAuthLogin web-channel messages until keys are available.
2FA / recovery parity — extends the same keys-required redirect to the other passwordless completion paths: TOTP, recovery code, and recovery phone.
Server cleanup — tidies the passkey auth response and feature-flag helper types; removes the now-unused requiresPasswordForSync response field.
Notification timing — for keys-required passkey sign-ins, defers the account.login metric / flow signal / security event (the new-device email is still sent, with generic framing) so the metric fires once keys are actually obtained at reauth / set-password. The client passes the decoupling-aware requiresPasswordForLogin to the server, and the passkey fallback reauth now passes reason: 'signin' + the flow metricsContext so the deferred metric lands correlated. keysRequired is a required field on /passkey/authentication/finish (the service === 'sync' fallback was removed).
Unit tests cover all of the above; remote/functional passkey specs updated for the response change.

Issue that this pull request solves

Closes: FXA-13979

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).
I have manually reviewed all AI generated code.

How to review (Optional)

fix(mobile): Redirect passwordless users to set_password in OAuthNative flows when Sync is not decoupled #20750 is already merged to main; this branch is now a single squashed commit on top of main — review the diff directly.
Suggested reading order: (1) passkey redirect via requiresPasswordForLogin, (2) 2FA/recovery parity, (3) server response/type tidy + the now-required keysRequired contract, (4) keys-required notification deferral.
Risky / focus areas: the requiresPasswordForLogin decoupling logic and supportsKeysOptionalLogin fail-closed default; the notification deferral and the fallback-reauth metricsContext correlation.

Screenshots (Optional)

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

Other information (Optional)

The fix sits behind the passkey feature flags, so this is a latent gap closed ahead of passkey GA, not a live production regression.
Found during engineering review of fix(mobile): Redirect passwordless users to set_password in OAuthNative flows when Sync is not decoupled #20750 (not reported by QA) — an extension of the same underlying bug.

Copilot

Pull request overview

This PR closes a remaining “keyless login” gap for passkey sign-ins and 2FA/recovery completion flows when scoped keys are required (e.g., Sync not decoupled / Firefox non-Sync services that still need Sync-scoped keys). It shifts the redirect decision to the client-side integration.requiresPasswordForLogin(supportsKeysOptionalLogin) check, updates the passkey auth API contract accordingly, and defers login notifications/metrics until keys are actually obtained.

Changes:

Extend the “keys-required → set_password/password-fallback” redirect logic to passkey sign-in plus TOTP/recovery-code/recovery-phone completion flows.
Update passkey auth server/client contracts: add requiresPasswordForLogin request flag, remove requiresPasswordForSync response field, and adjust notification/metric timing.
Propagate supportsKeysOptionalLogin and flow metricsContext through the relevant Settings pages and passkey fallback reauth so deferred metrics correlate correctly.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx	Treat third-party-auth marker as passwordless for keys-required redirect; set reason accordingly.
packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx	Add coverage for third-party-auth + TOTP redirect to set_password.
packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/interfaces.ts	Add `supportsKeysOptionalLogin` prop for recovery phone container.
packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx	Redirect passwordless completion to set_password when keys are required; store `hasPassword:false`.
packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.test.tsx	Add tests for passwordless redirect behavior and non-redirect case.
packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/interfaces.ts	Add `supportsKeysOptionalLogin` prop for recovery code page.
packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.tsx	Redirect passwordless completion to set_password when keys are required; store `hasPassword:false`.
packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.test.tsx	Add tests for passwordless redirect behavior and non-redirect case.
packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/container.tsx	Thread `supportsKeysOptionalLogin` through the container into the page component.
packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx	Pass `supportsKeysOptionalLogin` into passkey sign-in hook.
packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx	Include `reason:'signin'` and flow `metricsContext` in `sessionReauthWithAuthPW` for correlation.
packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.test.tsx	Verify `metricsContext` is forwarded to session reauth and request options include reason.
packages/fxa-settings/src/pages/Signin/interfaces.ts	Extend alternative-auth props to include `supportsKeysOptionalLogin`.
packages/fxa-settings/src/pages/Signin/index.tsx	Pass `supportsKeysOptionalLogin` into passkey sign-in hook.
packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.tsx	Propagate `supportsKeysOptionalLogin` down into Signin component props.
packages/fxa-settings/src/pages/Signin/components/SigninAlternativeAuthOptions/index.tsx	Pass `supportsKeysOptionalLogin` into passkey sign-in hook for this surface.
packages/fxa-settings/src/pages/Index/index.tsx	Pass `supportsKeysOptionalLogin` into passkey sign-in hook for email-first surface.
packages/fxa-settings/src/lib/passkeys/signin-flow.ts	Compute `requiresPasswordForLogin` client-side, forward it to server, and drive redirect off it.
packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx	Update expectations for new request param and add redirect tests for non-Sync keys-required flows.
packages/fxa-settings/src/components/App/index.tsx	Thread `flowQueryParams` into passkey fallback route; pass `supportsKeysOptionalLogin` to recovery routes.
packages/fxa-auth-server/test/remote/passkeys.in.spec.ts	Remove assertion for removed `requiresPasswordForSync` response field.
packages/fxa-auth-server/lib/routes/passkeys.ts	Accept `requiresPasswordForLogin` and use it to defer login notifications/metrics; remove requiresPasswordForSync response.
packages/fxa-auth-server/lib/routes/passkeys.spec.ts	Update route factory typing and add coverage for deferral behavior driven by client flag.
packages/fxa-auth-server/lib/passkey-utils.ts	Narrow helper config typing to only the passkeys flag slice for easier testing.
packages/fxa-auth-server/lib/passkey-utils.spec.ts	Adjust tests for narrowed config types.
packages/fxa-auth-server/docs/swagger/passkeys-api.ts	Update docs to reflect response field removal (needs request field doc update).
packages/fxa-auth-client/lib/client.ts	Remove `requiresPasswordForSync` from types; add `requiresPasswordForLogin` request option; minor formatting fixes.
packages/functional-tests/tests/passkeyAuth/passkeyPasswordFallback.spec.ts	Update test description to reflect client-side redirect decision.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dschom · 2026-06-20T01:39:35Z

+    routes = passkeyRoutes(
+      customs,
+      db,
+      // Fixture provides only the passkeys flags; cast to the full ConfigType.


Should we just create a better config object?

Also the passkeyRoutes should probaby have config as config: Pick<ConfigType, 'passkeys'>

dschom · 2026-06-22T16:26:04Z

+        isPasswordlessSignin &&
+        integration.requiresPasswordForLogin(supportsKeysOptionalLogin)
+      ) {
+        navigateWithQuery('/post_verify/set_password', {


Do we need to emit a glean metric here. This does imply the recovery succeeded, we are just now asking them to set a password.

I moved it above since it is a success metric, but it does make "success view" (= alert bar) a bit of a misnomer.

Oh interesting. Maybe we should double check with product and follow up if necessary.

Related to FXA-13972

dschom · 2026-06-22T16:29:21Z

      account,
      service,
-      requiresPasswordForSync
+      deferLoginNotifications


This feels misleading. Even if this is true, we send a login email, just not a service specific one. I wonder if could result in 'double mails'?

I see what you mean, the metrics are deferred but not the email so the naming was confusing. Sending the email here is intentional since this login results in a verified session token, even if the browser sign-in may not be complete. Session/reauth, doesn't at the moment result in a second email.

dschom · 2026-06-22T17:04:26Z

    it('should throw featureNotEnabled error when config.passkeys.enabled is undefined', () => {
-      const config = { passkeys: {} };
+      // Simulate a malformed config that omits the required `enabled`.
+      const config = { passkeys: {} } as Parameters<


Nothing wrong with this, but it's kind of a noisy / messy way to cast this.

Better like this?

dschom · 2026-06-22T17:08:41Z

        challengeOptions.challenge,
        {
          ...(serviceForRequest ? { service: serviceForRequest } : {}),
+          // Keys-pending signal for the server; password-step decision for the client.


I feel like this comment could be a little bit more clear.

dschom

Thanks for these updates. Just a few comments. After you review let me know and I'll take for more quick manual test and give an R+.

… 2FA, and recovery flows Because: - PR #20750 fixed keyless logins for passwordless OTP and third-party-auth sign-ins to keys-requiring services on non-decoupled browsers, but missed passkey sign-in and the TOTP/recovery-code/recovery-phone completion paths. This commit: - Drives the passkey password-step decision from integration.requiresPasswordForLogin(), not the server's service === 'sync' signal, and extends the same keys-required redirect to TOTP, recovery code, and recovery phone. - Makes keysRequired a required field on POST /passkey/authentication/finish, dropping the service === 'sync' fallback; removes the unused requiresPasswordForSync response field. - Defers the account.login metric/flow-signal/security-event for keys-required sign-ins until keys are obtained at reauth/set-password (the new-device email is still sent, with generic framing). - Adds unit tests for every path; updates remote/functional passkey specs. Closes: FXA-13979

LZoog mentioned this pull request Jun 17, 2026

fix(mobile): Redirect passwordless users to set_password in OAuthNative flows when Sync is not decoupled #20750

Merged

5 tasks

vpomerleau force-pushed the FXA-13979-passkey-keys-not-decoupled branch from b08d682 to 30a8f32 Compare June 17, 2026 21:57

vpomerleau requested a review from Copilot June 17, 2026 23:09

Copilot started reviewing on behalf of vpomerleau June 17, 2026 23:09 View session

vpomerleau changed the title ~~fix(fxa-settings): extend set_password redirect to passkey sign-in~~ fix(passkeys): extend non-decoupled keys-required sign-in to passkey, 2FA, and recovery flows Jun 17, 2026

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Comment thread packages/fxa-auth-server/docs/swagger/passkeys-api.ts

Comment thread packages/fxa-auth-client/lib/client.ts

vpomerleau force-pushed the FXA-13979-passkey-keys-not-decoupled branch 3 times, most recently from 6eeaa5f to 4907b80 Compare June 19, 2026 22:30

vpomerleau marked this pull request as ready for review June 19, 2026 22:32

vpomerleau requested a review from a team as a code owner June 19, 2026 22:32

dschom reviewed Jun 20, 2026

View reviewed changes

dschom reviewed Jun 22, 2026

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx Outdated

dschom reviewed Jun 22, 2026

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx Outdated

dschom reviewed Jun 22, 2026

View reviewed changes

vpomerleau force-pushed the FXA-13979-passkey-keys-not-decoupled branch from 4907b80 to 139c847 Compare June 23, 2026 00:21

vpomerleau requested a review from dschom June 23, 2026 00:40

Conversation

vpomerleau commented Jun 17, 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

How to review (Optional)

Screenshots (Optional)

Other information (Optional)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

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!

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!

Uh oh!

dschom left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vpomerleau commented Jun 17, 2026 •

edited

Loading