Merge pull request #20269 from mozilla/otp-statsd-metrics

feat(auth): add more StatsD metrics for passwordless OTP and account status

Author: vbudhram

.claude/skills/fxa-review/SKILL.md

@@ -51,6 +51,7 @@ Tell this agent it is a senior security engineer. It should:
 - Check CORS configuration — no `*` on credentialed endpoints
 - Verify OTP/TOTP handling: constant-time comparison, immediate invalidation, rate limiting
 - Check that secrets are accessed via Convict config, not hardcoded or read from env directly
+- Check StatsD metric tags for unbounded cardinality: user-controlled values (clientId, email, service) used as metric tags must be validated against a known allowlist (e.g. `getRegisteredClientIds()` or `getClientServiceTags(request)`). Free-form strings as tags allow attackers to blow up Prometheus storage.
 
 Output JSON array with fields: severity, category ("Security"), subcategory, file, line, issue, recommendation.
 

packages/fxa-auth-server/lib/routes/account.ts

@@ -1615,6 +1615,8 @@ export class AccountHandler {
 
     await this.customs.check(request, email, 'accountStatusCheck');
 
+    const statusTags = getClientServiceTags(request);
+
     // Block creation if email is reserved for secondary email registration
     const normalizedEmail = normalizeEmail(email);
     const existingSecondaryEmailRecord = await getExistingSecondaryEmailRecord(
@@ -1683,6 +1685,13 @@ export class AccountHandler {
         result.invalidDomain = invalidDomain;
       }
 
+      this.statsd.increment('account.statusCheck.result', {
+        exists: String(result.exists),
+        passwordlessSupported: String(!!result.passwordlessSupported),
+        hasPassword: String(!!result.hasPassword),
+        ...statusTags,
+      });
+
       return result;
     } catch (err) {
       if (err.errno === error.ERRNO.ACCOUNT_UNKNOWN) {
@@ -1706,6 +1715,13 @@ export class AccountHandler {
               service
             );
         }
+        this.statsd.increment('account.statusCheck.result', {
+          exists: 'false',
+          passwordlessSupported: String(!!result.passwordlessSupported),
+          hasPassword: 'false',
+          ...statusTags,
+        });
+
         if (this.customs.v2Enabled()) {
           await this.customs.check(request, email, 'accountStatusCheckFailed');
         }

packages/fxa-auth-server/lib/routes/passwordless.spec.ts

@@ -1152,12 +1152,17 @@ describe('passwordless statsd metrics', () => {
       payload: {
         email: TEST_EMAIL,
         clientId: 'test-client-id',
+        service: 'test-service',
         metricsContext: {
           deviceId: 'device123',
           flowId: 'flow123',
           flowBeginTime: Date.now(),
         },
       },
+      app: {
+        clientIdTag: 'test-client-id',
+        serviceTag: 'test-service',
+      },
     });
   });
 
@@ -1247,8 +1252,58 @@ describe('passwordless statsd metrics', () => {
         .map((c: any) => c.args[0]);
       expect(incrementCalls).toContain('passwordless.registration.success');
       expect(incrementCalls).toContain('passwordless.confirmCode.success');
+
+      // confirmCode.success should include isNewAccount tag
+      const confirmCall = mockStatsd.increment
+        .getCalls()
+        .find((c: any) => c.args[0] === 'passwordless.confirmCode.success');
+      expect(confirmCall).toBeDefined();
+      expect(confirmCall.args[1]).toEqual(
+        expect.objectContaining({ isNewAccount: 'true' })
+      );
     });
   });
+
+  it('should increment passwordless.blocked when client is not allowed', () => {
+    mockDB.accountRecord = sinon.spy(() =>
+      Promise.reject(error.unknownAccount())
+    );
+
+    routes = makeRoutes({
+      log: mockLog,
+      db: mockDB,
+      customs: mockCustoms,
+      statsd: mockStatsd,
+      config: {
+        passwordlessOtp: {
+          enabled: true,
+          ttl: 300,
+          digits: 6,
+          allowedClientServices: {},
+        },
+      },
+    });
+    route = getRoute(routes, '/account/passwordless/send_code', 'POST');
+
+    return runTest(route, mockRequest).then(
+      () => {
+        throw new Error('should have failed');
+      },
+      () => {
+        const blockedCall = mockStatsd.increment
+          .getCalls()
+          .find((c: any) => c.args[0] === 'passwordless.blocked');
+        expect(blockedCall).toBeDefined();
+        expect(blockedCall.args[1]).toEqual(
+          expect.objectContaining({
+            reason: 'clientNotAllowed',
+            clientId: 'test-client-id',
+            service: 'test-service',
+          })
+        );
+      }
+    );
+  });
 });
 
 describe('/account/passwordless/resend_code', () => {

packages/fxa-auth-server/lib/routes/passwordless.ts

@@ -119,7 +119,8 @@ class PasswordlessHandler {
     email: string,
     clientId: string | undefined,
     service: string | undefined,
-    logTag: string
+    logTag: string,
+    request: AuthRequest
   ) {
     const hasLinkedAccount =
       account && (account.linkedAccounts?.length || 0) > 0;
@@ -137,6 +138,10 @@ class PasswordlessHandler {
           clientId,
           service,
         });
+        this.statsd.increment('passwordless.blocked', {
+          reason: 'clientNotAllowed',
+          ...getClientServiceTags(request),
+        });
         throw error.featureNotEnabled();
       }
     }
@@ -147,6 +152,10 @@ class PasswordlessHandler {
         this.config.passwordlessOtp.enabled
       )
     ) {
+      this.statsd.increment('passwordless.blocked', {
+        reason: 'notEligible',
+        ...getClientServiceTags(request),
+      });
       throw error.cannotCreatePassword();
     }
   }
@@ -168,7 +177,8 @@ class PasswordlessHandler {
       email,
       clientId,
       service,
-      'sendCode'
+      'sendCode',
+      request
     );
 
     return this.generateAndSendOtp(request, email, account, isNewAccount);
@@ -197,7 +207,8 @@ class PasswordlessHandler {
       email,
       clientId,
       service,
-      'confirmCode'
+      'confirmCode',
+      request
     );
 
     // Verify OTP
@@ -229,6 +240,10 @@ class PasswordlessHandler {
         this.log.info('passwordless.confirmCode.totpRequired', {
           uid: account.uid,
         });
+        this.statsd.increment(
+          'passwordless.confirmCode.totpRequired',
+          getClientServiceTags(request)
+        );
       }
     }
 
@@ -267,10 +282,10 @@ class PasswordlessHandler {
       tokenVerificationId
     );
 
-    this.statsd.increment(
-      'passwordless.confirmCode.success',
-      getClientServiceTags(request)
-    );
+    this.statsd.increment('passwordless.confirmCode.success', {
+      ...getClientServiceTags(request),
+      isNewAccount: String(isNewAccount),
+    });
 
     if (!isNewAccount) {
       this.glean.login.complete(request, {
@@ -326,7 +341,8 @@ class PasswordlessHandler {
       email,
       clientId,
       service,
-      'resendCode'
+      'resendCode',
+      request
     );
 
     // Delete existing code before sending a new one