Conversation
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
left a comment
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 } = {} |
There was a problem hiding this comment.
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.
Because
GET /accountis gated only by a baresessionToken, with notokenVerifiedor assurance-level check. A caller holding nothing but the account password received the unmasked E.164 number inrecoveryPhone.phoneNumber— the credential needed to mount a SIM swap and defeat TOTP.GET /recovery_phonechose whether to mask fromemailVerified/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
RecoveryPhoneService.hasConfirmedMasked()inrecovery-phone.service.tsand points both read endpoints at it. Neither mode returns a dialable number; thekeepFormattingoption only chooses how much of the shape survives, so the session check no longer decides whether the number leaks.maskNationalFormat()to mask in place while keeping Twilio's separators, so(415) 555-1234becomes(•••) •••-1234.phoneNumberStripparameter fromhasConfirmed(). 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
Other information
No UI changes. Entirely server-side —
fxa-settingsis 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-1234and now shows(•••) •••-1234. That page was rendering the leaked value. It needs no code change to pick up the masked one.