Skip to content

fix(recovery-phone): use reg limit consistently across phone flows#20848

Open
toufali wants to merge 1 commit into
mainfrom
sec-audit-phone-number-cap
Open

fix(recovery-phone): use reg limit consistently across phone flows#20848
toufali wants to merge 1 commit into
mainfrom
sec-audit-phone-number-cap

Conversation

@toufali

@toufali toufali commented Jul 10, 2026

Copy link
Copy Markdown
Member

Because

  • The recovery phone registration limit was applied in some setup paths but not when changing an existing number, leaving the check inconsistent across flows.

This pull request

  • Applies the existing getCountByPhoneNumber() limit check in changePhoneNumber(), matching setupPhoneNumber() and confirmSetupCode().
  • Adds a unit test covering the registration-limit case.

Issue that this pull request solves

Closes: FXA-14069

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.

@toufali toufali marked this pull request as ready for review July 10, 2026 20:11
@toufali toufali requested a review from a team as a code owner July 10, 2026 20:11
Copilot AI review requested due to automatic review settings July 10, 2026 20:11

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

Pull request overview

This PR makes the recovery phone registration-limit enforcement consistent by applying the existing “max registrations per phone number” check to the changePhoneNumber() flow and adds a unit test to cover the limit-reached case.

Changes:

  • Add registration-limit enforcement to RecoveryPhoneService.changePhoneNumber().
  • Add a unit test asserting changePhoneNumber() rejects numbers that exceed the registration limit.

Reviewed changes

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

File Description
libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts Adds the max-registrations-per-number check to the change-number flow (throws RecoveryPhoneRegistrationLimitReached).
libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts Adds a unit test for the registration-limit rejection in the change-number flow.

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

Comment thread libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts
Comment thread libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts Outdated
@toufali toufali force-pushed the sec-audit-phone-number-cap branch from 2cfbba0 to f31f36e Compare July 10, 2026 20:30
@toufali toufali requested a review from Copilot July 10, 2026 20:32

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

Pull request overview

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

Comments suppressed due to low confidence (1)

packages/fxa-auth-server/lib/routes/recovery-phone.ts:675

  • In the unexpected-error path for changePhoneNumber, the backendServiceFailure call reports the operation as confirmCode, which makes logs/telemetry misleading for this route. It should identify the failing operation as changePhoneNumber.
        throw AppError.recoveryPhoneRegistrationLimitReached();
      } else {
        throw AppError.backendServiceFailure(
          'RecoveryPhoneService',
          'confirmCode',

Comment thread libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts
Comment thread packages/fxa-auth-server/lib/routes/recovery-phone.ts
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