task(recovery-phone): Add support for masking phone number server side

Author: dschom

libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts

@@ -133,6 +133,19 @@ describe('RecoveryPhoneService', () => {
       ).toHaveBeenCalledWith(uid);
     });
 
+    it('can return masked phone number', async () => {
+      mockRecoveryPhoneManager.getConfirmedPhoneNumber.mockReturnValueOnce({
+        phoneNumber,
+      });
+
+      const result = await service.hasConfirmed(uid, 4);
+
+      expect(result.phoneNumber).toEqual('+•••••••1234');
+      expect(
+        mockRecoveryPhoneManager.getConfirmedPhoneNumber
+      ).toHaveBeenCalledWith(uid);
+    });
+
     it('can determine confirmed phone number does not exist', async () => {
       const mockError = new RecoveryNumberNotExistsError(uid);
       mockRecoveryPhoneManager.getConfirmedPhoneNumber.mockRejectedValueOnce(
@@ -354,4 +367,15 @@ describe('RecoveryPhoneService', () => {
       expect(result).toBe(false);
     });
   });
+
+  describe('mask phone number', () => {
+    it('can mask number', () => {
+      const phoneNumber = '+123456789';
+      expect(service.maskPhoneNumber(phoneNumber, -1)).toEqual('+•••••••••');
+      expect(service.maskPhoneNumber(phoneNumber, 0)).toEqual('+•••••••••');
+      expect(service.maskPhoneNumber(phoneNumber, 4)).toEqual('+•••••6789');
+      expect(service.maskPhoneNumber(phoneNumber, 9)).toEqual('+123456789');
+      expect(service.maskPhoneNumber(phoneNumber, 12)).toEqual('+123456789');
+    });
+  });
 });

libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts

@@ -153,17 +153,26 @@ export class RecoveryPhoneService {
   /**
    * Checks if the given uid has confirmed a phone number.
    * @param uid Account id
+   * @param phoneNumberMask When provided will mask the number so the full value is not shown.
    * @returns If the account has confirmed, returns {exists:true, phoneNumber }. If not returns {exists:false}
+   *
+   * @remarks The value provided for phoneNumberMask will preserve the last N digits of of the phone number.
+   * e.g. If the phone number was +15005551234 and phoneNumberMask was 4, the result would be +*******1234.
    */
   public async hasConfirmed(
-    uid: string
-  ): Promise<{ exists: boolean; phoneNumber?: string }> {
+    uid: string,
+    phoneNumberMask?: number
+  ): Promise<{
+    exists: boolean;
+    phoneNumber?: string;
+  }> {
     try {
       const { phoneNumber } =
         await this.recoveryPhoneManager.getConfirmedPhoneNumber(uid);
+
       return {
         exists: true,
-        phoneNumber,
+        phoneNumber: this.maskPhoneNumber(phoneNumber, phoneNumberMask),
       };
     } catch (err) {
       if (err instanceof RecoveryNumberNotExistsError) {
@@ -178,6 +187,41 @@ export class RecoveryPhoneService {
     }
   }
 
+  /**
+   * Masks a phone number so as to not divulge the entire value.
+   *
+   * @param phoneNumber The actual phone number
+   * @param lastN The last N number of digits to show
+   * @returns A masked number
+   *
+   * @remarks This will not mask a + symbol, since this technically isn't part of the
+   * number. e.g. +15005551234 would be masked as +*******1234 if lastN was 4.
+   */
+  public maskPhoneNumber(phoneNumber: string, lastN?: number) {
+    // The + notation can be confusing in a masked number. Don't count it
+    // as a digit.
+    let prefix = '';
+    if (phoneNumber.startsWith('+')) {
+      prefix = '+';
+      phoneNumber = phoneNumber.substring(1);
+    }
+
+    // Clamp lastN between 0 and phoneNumber.length
+    if (lastN === undefined) {
+      lastN = phoneNumber.length;
+    } else if (lastN > phoneNumber.length) {
+      lastN = phoneNumber.length;
+    } else if (lastN < 0) {
+      lastN = 0;
+    }
+
+    // Create mask
+    const maskedPhoneNumber = phoneNumber
+      .substring(phoneNumber.length - lastN)
+      .padStart(phoneNumber.length, '•');
+    return `${prefix}${maskedPhoneNumber}`;
+  }
+
   /**
    * Sends an totp code to a user
    * @param uid Account id

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

@@ -243,17 +243,21 @@ class RecoveryPhoneHandler {
   async exists(request: AuthRequest) {
     const { uid, emailVerified, mustVerify, tokenVerified } = request.auth
       .credentials as SessionTokenAuthCredential;
-
-    // Short circuit if the account / session still needs verification.
-    // Note this is typically due to totp being required, but there are
-    // other states that could also result in an unverified session, such
-    // as a forced password change.
-    if (!emailVerified || (mustVerify && !tokenVerified)) {
-      return {};
+    const payload = request.payload as unknown as { phoneNumberMask: number };
+    let phoneNumberMask = payload?.phoneNumberMask;
+
+    // To ensure no data is leaked, we will never expose the full phone number, if
+    // the session is not verified. e.g. The user has entered the correct password,
+    // but failed to provide 2FA.
+    if (
+      phoneNumberMask === undefined &&
+      (!emailVerified || (mustVerify && !tokenVerified))
+    ) {
+      phoneNumberMask = 4;
     }
 
     try {
-      return await this.recoveryPhoneService.hasConfirmed(uid);
+      return await this.recoveryPhoneService.hasConfirmed(uid, phoneNumberMask);
     } catch (error) {
       throw AppError.backendServiceFailure(
         'RecoveryPhoneService',

packages/fxa-auth-server/test/local/routes/recovery-phone.js

@@ -419,7 +419,7 @@ describe('/recovery_phone', () => {
       );
     });
 
-    it('indicates service', async () => {
+    it('indicates  error', async () => {
       mockRecoveryPhoneService.hasConfirmed = sinon.fake.returns(
         Promise.reject(new Error('BOOM'))
       );
@@ -433,7 +433,7 @@ describe('/recovery_phone', () => {
       assert.equal(mockGlean.twoStepAuthPhoneRemove.success.callCount, 0);
     });
 
-    it('returns empty response for unverified session', async () => {
+    it('returns masked phone number for unverified session', async () => {
       mockRecoveryPhoneService.hasConfirmed = sinon.fake.returns({
         exists: true,
         phoneNumber,
@@ -444,8 +444,36 @@ describe('/recovery_phone', () => {
         credentials: { uid, mustVerify: true },
       });
       assert.isDefined(resp);
-      assert.isEmpty(resp);
-      assert.equal(mockRecoveryPhoneService.hasConfirmed.callCount, 0);
+      assert.isDefined(resp.exists);
+      assert.isDefined(resp.phoneNumber);
+      assert.equal(mockRecoveryPhoneService.hasConfirmed.callCount, 1);
+      assert.equal(
+        mockRecoveryPhoneService.hasConfirmed.getCall(0).args[0],
+        uid
+      );
+      assert.equal(mockRecoveryPhoneService.hasConfirmed.getCall(0).args[1], 4);
+    });
+
+    it('returns masked phone number format requested', async () => {
+      mockRecoveryPhoneService.hasConfirmed = sinon.fake.returns({
+        exists: true,
+        phoneNumber,
+      });
+      const resp = await makeRequest({
+        method: 'GET',
+        path: '/recovery_phone',
+        credentials: { uid, mustVerify: true },
+        payload: { phoneNumberMask: 2 },
+      });
+      assert.isDefined(resp);
+      assert.isDefined(resp.exists);
+      assert.isDefined(resp.phoneNumber);
+      assert.equal(mockRecoveryPhoneService.hasConfirmed.callCount, 1);
+      assert.equal(
+        mockRecoveryPhoneService.hasConfirmed.getCall(0).args[0],
+        uid
+      );
+      assert.equal(mockRecoveryPhoneService.hasConfirmed.getCall(0).args[1], 2);
     });
   });
 });