Merge pull request #20233 from mozilla/FXA-13313

LZoog · web-flow · commit e51b70a3ea7e · 2026-03-23T11:27:27.000-05:00
feat(logging): Add client_id + service to more logs, expand valid client_ids to log
diff --git a/packages/fxa-auth-server/bin/key_server.js b/packages/fxa-auth-server/bin/key_server.js
@@ -432,7 +432,8 @@ async function run(config) {
     database,
     statsd,
     glean,
-    customs
+    customs,
+    oauthDb
   );
 
   try {
diff --git a/packages/fxa-auth-server/lib/account-events.spec.ts b/packages/fxa-auth-server/lib/account-events.spec.ts
@@ -133,7 +133,7 @@ describe('Account Events', () => {
   describe('security events', () => {
     it('can record security event', async () => {
       const message = {
-        name: 'account.login' as const,
+        name: 'account.login',
         uid: '000',
         ipAddr: '123.123.123.123',
         tokenId: '123',
@@ -144,14 +144,54 @@ describe('Account Events', () => {
 
       sinon.assert.calledOnceWithExactly(
         statsd.increment,
-        'accountEvents.recordSecurityEvent.write.account.login'
+        'accountEvents.recordSecurityEvent.write.account.login',
+        { clientId: 'none', service: 'none' }
+      );
+    });
+
+    it('includes clientId and service tags from additionalInfo', async () => {
+      const message = {
+        name: 'account.login',
+        uid: '000',
+        ipAddr: '123.123.123.123',
+        tokenId: '123',
+        additionalInfo: {
+          client_id: '5882386c6d801776',
+          service: 'sync',
+        },
+      };
+      await accountEventsManager.recordSecurityEvent(mockDb, message);
+
+      sinon.assert.calledOnceWithExactly(
+        statsd.increment,
+        'accountEvents.recordSecurityEvent.write.account.login',
+        { clientId: '5882386c6d801776', service: 'sync' }
+      );
+    });
+
+    it('uses none for missing service when clientId is present', async () => {
+      const message = {
+        name: 'account.login',
+        uid: '000',
+        ipAddr: '123.123.123.123',
+        tokenId: '123',
+        additionalInfo: {
+          client_id: 'deadbeefdeadbeef',
+        },
+      };
+      await accountEventsManager.recordSecurityEvent(mockDb, message);
+
+      sinon.assert.calledOnceWithExactly(
+        statsd.increment,
+        'accountEvents.recordSecurityEvent.write.account.login',
+        { clientId: 'deadbeefdeadbeef', service: 'none' }
       );
     });
 
     it('logs and does not throw on failure', async () => {
       mockDb.securityEvent = sinon.stub().throws();
       const message = {
-        name: 'account.login' as const,
+        name: 'account.login',
         uid: '000',
         ip: '123.123.123.123',
         tokenId: '123',
@@ -160,7 +200,29 @@ describe('Account Events', () => {
       expect(addMock.called).toBe(false);
       sinon.assert.calledOnceWithExactly(
         statsd.increment,
-        'accountEvents.recordSecurityEvent.error.account.login'
+        'accountEvents.recordSecurityEvent.error.account.login',
+        { clientId: 'none', service: 'none' }
+      );
+    });
+
+    it('includes tags on error path', async () => {
+      mockDb.securityEvent = sinon.stub().throws();
+      const message = {
+        name: 'account.login',
+        uid: '000',
+        ipAddr: '123.123.123.123',
+        tokenId: '123',
+        additionalInfo: {
+          client_id: '5882386c6d801776',
+          service: 'sync',
+        },
+      };
+      await accountEventsManager.recordSecurityEvent(mockDb, message as any);
+
+      sinon.assert.calledOnceWithExactly(
+        statsd.increment,
+        'accountEvents.recordSecurityEvent.error.account.login',
+        { clientId: '5882386c6d801776', service: 'sync' }
       );
     });
   });
diff --git a/packages/fxa-auth-server/lib/account-events.ts b/packages/fxa-auth-server/lib/account-events.ts
@@ -6,7 +6,11 @@ import { Container } from 'typedi';
 
 import { AuthFirestore, AppConfig } from '../lib/types';
 import { StatsD } from 'hot-shots';
-import { SecurityEventNames } from 'fxa-shared/db/models/auth/security-event';
+import * as Sentry from '@sentry/node';
+import {
+  EVENT_NAMES,
+  SecurityEventNames,
+} from 'fxa-shared/db/models/auth/security-event';
 
 type BaseEvent = {
   // General metric properties
@@ -163,6 +167,11 @@ export class AccountEventsManager {
   public async recordSecurityEvent(db: AuthDatabase, message: SecurityEvent) {
     const { uid, name, ipAddr, tokenId, additionalInfo } = message;
 
+    if (!EVENT_NAMES[name]) {
+      Sentry.captureMessage(`Unknown security event name: ${name}`);
+      return;
+    }
+
     const eventData: SecurityEvent = {
       name,
       uid,
@@ -175,12 +184,23 @@ export class AccountEventsManager {
       }),
     };
 
+    const metricTags = {
+      clientId: additionalInfo?.client_id || 'none',
+      service: additionalInfo?.service || 'none',
+    };
+
     try {
       await db.securityEvent(eventData);
-      this.statsd.increment(`accountEvents.recordSecurityEvent.write.${name}`);
+      this.statsd.increment(
+        `accountEvents.recordSecurityEvent.write.${name}`,
+        metricTags
+      );
     } catch (err) {
       // Failing to write to events shouldn't break anything
-      this.statsd.increment(`accountEvents.recordSecurityEvent.error.${name}`);
+      this.statsd.increment(
+        `accountEvents.recordSecurityEvent.error.${name}`,
+        metricTags
+      );
     }
   }
 }
diff --git a/packages/fxa-auth-server/lib/metrics/client-tags.ts b/packages/fxa-auth-server/lib/metrics/client-tags.ts
@@ -2,6 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+import * as Sentry from '@sentry/node';
 import { OAuthNativeClients, OAuthNativeServices } from '@fxa/accounts/oauth';
 import { MetricsContext } from '@fxa/shared/metrics/glean';
 
@@ -15,15 +16,23 @@ export type ClientTagsRequest = {
   query: any;
 };
 
-const validClientIds = new Set<string>(Object.values(OAuthNativeClients));
+const nativeClientIds = new Set<string>(Object.values(OAuthNativeClients));
 const validServices = new Set<string>(Object.values(OAuthNativeServices));
 
 /**
  * Resolves and validates clientId and service from the request's metricsContext.
  * Only returns values that are in the known allowlists to prevent infinite
  * cardinality in StatsD metrics.
+ *
+ * clientId is accepted if it exists in configuredClientIds (loaded from the
+ * fxa_oauth.clients table per-request, cached). service is only resolved for
+ * native clients, as those are the only IDs where service applies (e.g.
+ * Firefox Desktop can be sync, relay, vpn, etc.).
  */
-export async function resolveClientTags(request: ClientTagsRequest): Promise<{
+export async function resolveClientTags(
+  request: ClientTagsRequest,
+  configuredClientIds?: Set<string>
+): Promise<{
   clientId: string | undefined;
   service: string | undefined;
 }> {
@@ -32,25 +41,27 @@ export async function resolveClientTags(request: ClientTagsRequest): Promise<{
 
   try {
     const metricsContext = await request.app.metricsContext;
+    const rawClientId = metricsContext?.clientId;
 
-    if (
-      metricsContext?.clientId &&
-      validClientIds.has(metricsContext.clientId)
-    ) {
-      clientId = metricsContext.clientId;
-    }
+    if (rawClientId && configuredClientIds?.has(rawClientId)) {
+      clientId = rawClientId;
 
-    // service can come from payload, query, or stashed metricsContext
-    const rawService =
-      (request.payload as any)?.service ||
-      (request.query as any)?.service ||
-      metricsContext?.service;
+      // service only applies to native clients and can come from payload,
+      // query, or stashed metricsContext
+      if (nativeClientIds.has(rawClientId)) {
+        const rawService =
+          request.payload?.service ||
+          request.query?.service ||
+          metricsContext?.service;
 
-    if (rawService && validServices.has(rawService)) {
-      service = rawService;
+        if (rawService && validServices.has(rawService)) {
+          service = rawService;
+        }
+      }
     }
-  } catch {
-    // If metricsContext resolution fails, just return undefined for both
+  } catch (err) {
+    // Capture in Sentry, return undefined clientId/service.
+    Sentry.captureException(err);
   }
 
   return { clientId, service };
diff --git a/packages/fxa-auth-server/lib/oauth/db/mysql/index.js b/packages/fxa-auth-server/lib/oauth/db/mysql/index.js
@@ -39,6 +39,7 @@ const QUERY_DEVELOPER_OWNS_CLIENT =
 const QUERY_DEVELOPER_INSERT =
   'INSERT INTO developers ' + '(developerId, email) ' + 'VALUES (?, ?);';
 const QUERY_CLIENT_GET = 'SELECT * FROM clients WHERE id=?';
+const QUERY_CLIENT_IDS = 'SELECT id FROM clients';
 const QUERY_CLIENT_LIST =
   'SELECT id, name, redirectUri, imageUri, ' +
   'canGrant, publicClient, trusted ' +
@@ -342,6 +343,9 @@ class MysqlStore extends MysqlOAuthShared {
   getClient(id) {
     return this._readOne(QUERY_CLIENT_GET, [buf(id)]);
   }
+  getAllClientIds() {
+    return this._read(QUERY_CLIENT_IDS);
+  }
   getClients(email) {
     return this._read(QUERY_CLIENT_LIST, [email]);
   }
diff --git a/packages/fxa-auth-server/lib/routes/utils/signin.js b/packages/fxa-auth-server/lib/routes/utils/signin.js
@@ -642,6 +642,8 @@ module.exports = (
       }
 
       async function recordSecurityEvent() {
+        const clientId = request.app.clientIdTag;
+        const service = request.app.serviceTag;
         await accountEventsManager.recordSecurityEvent(db, {
           name: 'account.login',
           uid: accountRecord.uid,
@@ -650,6 +652,8 @@ module.exports = (
           additionalInfo: {
             userAgent: request.headers['user-agent'],
             location: request.app.geo.location,
+            ...(clientId && { client_id: clientId }),
+            ...(service && { service }),
           },
         });
       }
diff --git a/packages/fxa-auth-server/lib/server.js b/packages/fxa-auth-server/lib/server.js
@@ -85,7 +85,17 @@ function translateStripeErrors(response) {
   return response;
 }
 
-async function create(log, error, config, routes, db, statsd, glean, customs) {
+async function create(
+  log,
+  error,
+  config,
+  routes,
+  db,
+  statsd,
+  glean,
+  customs,
+  oauthDb
+) {
   const getGeoData = require('./geodb')(log);
   const metricsContext = require('./metrics/context')(log, config);
   const metricsEvents = require('./metrics/events')(log, config, glean);
@@ -317,11 +327,19 @@ async function create(log, error, config, routes, db, statsd, glean, customs) {
       await customs.checkIpOnly(request, action);
     }
 
-    // Validate clientId/service against known enums (to prevent unbounded
-    // cardinality) and normalize onto request.app for use as StatsD/Sentry tags.
-    // clientId comes from metricsContext (async, may be stashed in Redis);
-    // service comes from query/payload but needs allowlist validation.
-    const tags = await resolveClientTags(request);
+    // Validate clientId/service against registered OAuth clients to prevent
+    // unbounded cardinality and normalize onto request.app for StatsD/Sentry tags.
+    // Client IDs are loaded from the fxa_oauth.clients table on each request;
+    // the table is small and MySQL serves it from cache.
+    let registeredClientIds;
+    try {
+      const clients = await oauthDb.mysql.getAllClientIds();
+      registeredClientIds = new Set(clients.map((c) => c.id.toString('hex')));
+    } catch (err) {
+      Sentry.captureException(err);
+      registeredClientIds = new Set();
+    }
+    const tags = await resolveClientTags(request, registeredClientIds);
     if (tags.clientId) {
       request.app.clientIdTag = tags.clientId;
       Sentry.setTag('clientId', tags.clientId);
@@ -331,7 +349,6 @@ async function create(log, error, config, routes, db, statsd, glean, customs) {
       Sentry.setTag('service', tags.service);
     }
 
-
     return h.continue;
   });
 
diff --git a/packages/fxa-auth-server/test/local/account-events.js b/packages/fxa-auth-server/test/local/account-events.js
@@ -132,7 +132,8 @@ describe('Account Events', () => {
 
       assert.calledOnceWithExactly(
         statsd.increment,
-        'accountEvents.recordSecurityEvent.write.account.login'
+        'accountEvents.recordSecurityEvent.write.account.login',
+        { clientId: 'none', service: 'none' }
       );
     });
 
@@ -148,7 +149,8 @@ describe('Account Events', () => {
       assert.isFalse(addMock.called);
       assert.calledOnceWithExactly(
         statsd.increment,
-        'accountEvents.recordSecurityEvent.error.account.login'
+        'accountEvents.recordSecurityEvent.error.account.login',
+        { clientId: 'none', service: 'none' }
       );
     });
   });
diff --git a/packages/fxa-auth-server/test/local/metrics/client-tags.ts b/packages/fxa-auth-server/test/local/metrics/client-tags.ts
