Skip to content

fix(auth-server): never return the full recovery phone number - #20975

Open
vbudhram wants to merge 1 commit into
mainfrom
fxa-14028
Open

fix(auth-server): never return the full recovery phone number#20975
vbudhram wants to merge 1 commit into
mainfrom
fxa-14028

Conversation

@vbudhram

@vbudhram vbudhram commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Because

  • GET /account is gated only by a bare sessionToken, with no tokenVerified or assurance-level check. A caller holding nothing but the account password received the unmasked E.164 number in recoveryPhone.phoneNumber — the credential needed to mount a SIM swap and defeat TOTP.
  • GET /recovery_phone chose whether to mask from emailVerified / mustVerify / tokenVerified, and that check decided whether the number leaked at all. A session that cleared it without reaching AAL2 received the full number.

This pull request

  • Adds RecoveryPhoneService.hasConfirmedMasked() in recovery-phone.service.ts and points both read endpoints at it. Neither mode returns a dialable number; the keepFormatting option only chooses how much of the shape survives, so the session check no longer decides whether the number leaks.
  • Adds maskNationalFormat() to mask in place while keeping Twilio's separators, so (415) 555-1234 becomes (•••) •••-1234.
  • Removes the phoneNumberStrip parameter from hasConfirmed(). Defaulting it to "do not mask" is what made the leak possible; the raw accessor is now server-side only, used by the email, setup and change paths that need a dialable number.

Issue that this pull request solves

Closes: https://mozilla-hub.atlassian.net/browse/FXA-14028

Checklist

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

Other information

No UI changes. Entirely server-side — fxa-settings is untouched. Every screen renders exactly what it does today: sessions that previously received the full number now receive (•••) •••-1234, which the client already masks to the identical string, and sessions that previously received the last four still do.

The one unavoidable difference: the remove-recovery-phone page previously printed the full (415) 555-1234 and now shows (•••) •••-1234. That page was rendering the leaked value. It needs no code change to pick up the masked one.

@vbudhram
vbudhram requested a review from a team as a code owner August 6, 2026 11:49
@vbudhram vbudhram self-assigned this Aug 6, 2026
Because:
* GET /account is gated only by a bare sessionToken, with no tokenVerified
  or assurance-level check. A caller holding nothing but the account
  password therefore received the unmasked E.164 number in
  recoveryPhone.phoneNumber, which is the credential needed to mount a SIM
  swap and defeat TOTP.
* GET /recovery_phone chose whether to mask from emailVerified, mustVerify
  and tokenVerified, and that check decided whether the number leaked at
  all. A session clearing it without reaching AAL2 received the full
  number.

This commit:
* Adds RecoveryPhoneService.hasConfirmedMasked() and points both read
  endpoints at it. Neither mode returns a dialable number: the
  keepFormatting option only decides whether the separators survive, so the
  session check no longer governs whether the number leaks, only how it is
  rendered.
* Adds maskNationalFormat(), which masks in place so Twilio's separators
  survive, turning (415) 555-1234 into (•••) •••-1234.
* Drops the phoneNumberStrip parameter from hasConfirmed(). Defaulting it
  to "do not mask" is what made the leak possible. The raw accessor is now
  documented server-side only and is reached by the email, setup and change
  paths, which need a dialable number.
* Extracts the session predicate to requestHelper.hasProvenSession(), used
  by both endpoints and unit tested directly rather than inlined in one
  handler.
* Adds a maskedPhoneNumber validator and applies it to both response
  schemas, so a response carrying more than four digits fails validation
  instead of shipping.

There are no UI changes. Sessions that previously received the full number
now receive (•••) •••-1234, which fxa-settings already masks to the string
it renders today, and sessions that previously received the last four still
do. The one exception is the remove-recovery-phone page, which printed the
full number and now shows it masked; it needed no change to pick that up.

The confirm and change endpoints still return a dialable number. Both are
gated on a verified session or mfa:2fa and echo a number the caller just
supplied and proved control of, so they are not an egress path.

Closes FXA-14028

@vpomerleau vpomerleau left a comment

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.

Core fix looks right — masking is unconditional now, and the maskedPhoneNumber response validator is a solid fail-closed backstop. Two notes inline.

}

/**
* Returns `true` if the request's session has proven more than the password:

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.

Blocker: this doesn't check what it says. emailVerified is an account property copied at token creation (account.ts:1406), and mustVerify is set almost only by wantsKeys (:1366-1377) while needsVerificationId defaults true (:1344) — so a plain web sign-in returns true having done nothing past the password. Passkey and third-party sessions return true with no password at all.

It's the mustVerify gate, identical to oauth/util.js:54. Since it's newly exported, the name will get picked up as an auth gate. Suggest isPastMustVerifyGate and drop the assurance language — same for the 'proven only the password' label in recovery-phone.spec.ts.

Minor: the hasConfirmedMasked JSDoc spells out the SIM-swap scenario — probably better pointed at FXA-14028 than written out in a public repo.

*/
public async hasConfirmedMasked(
uid: string,
{ keepFormatting = false }: { keepFormatting?: boolean } = {}

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.

Suggestion, non-blocking — possible follow-up: keepFormatting (and with it hasProvenSession and maskNationalFormat) may not be earning its keep. We're US/CA-only (config/index.ts:2643, :2683), so every number is (XXX) XXX-XXXX — the format is a constant and discloses nothing beyond the last 4, which means there's nothing for the session check to protect.

Not free though: always sending the masked national format changes the copy on SigninRecoveryChoice and ResetPasswordRecoveryChoice from "Number ending in 1234" to (•••) •••-1234, kills the length === 4 branch in recovery-phone-utils.tsx:58, and orphans the recovery-phone-number-ending-digits string. Wants UX sign-off, so probably its own ticket rather than scope here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants