Merge pull request #6190 from mozilla/feat/issue-6185

feat(event-broker): prevent invalid client ids

packages/fxa-event-broker/src/lib/notificationProcessor.ts

@@ -147,7 +147,8 @@ class ServiceNotificationProcessor {
       return;
     }
     this.metrics.increment('message.type', { eventType: 'login' });
-    await this.db.storeLogin(message.uid, message.clientId);
+    // In case a SQS source capitalized the client id, lower-case it for consistency.
+    await this.db.storeLogin(message.uid, message.clientId.toLowerCase());
   }
 
   /**

packages/fxa-event-broker/src/lib/serviceNotifications.ts

@@ -16,6 +16,8 @@ export const PROFILE_CHANGE_EVENT = 'profileDataChange';
 export const SUBSCRIPTION_UPDATE_EVENT = 'subscription:update';
 
 // Message schemas
+const CLIENT_ID = joi.string().regex(/[a-z0-9]{16}/);
+
 const BASE_MESSAGE_SCHEMA = joi
   .object()
   .keys({
@@ -27,7 +29,7 @@ const BASE_MESSAGE_SCHEMA = joi
 const LOGIN_SCHEMA = joi
   .object()
   .keys({
-    clientId: joi.string().optional(),
+    clientId: CLIENT_ID.optional(),
     deviceCount: joi.number().integer().required(),
     email: joi.string().required(),
     event: joi.string().valid(LOGIN_EVENT).required(),
@@ -100,7 +102,9 @@ export type deleteSchema = joi.Literal<typeof DELETE_SCHEMA>;
 export type loginSchema = joi.Literal<typeof LOGIN_SCHEMA>;
 export type passwordSchema = joi.Literal<typeof PASSWORD_CHANGE_SCHEMA>;
 export type profileSchema = joi.Literal<typeof PROFILE_CHANGE_SCHEMA>;
-export type subscriptionUpdateSchema = joi.Literal<typeof SUBSCRIPTION_UPDATE_SCHEMA>;
+export type subscriptionUpdateSchema = joi.Literal<
+  typeof SUBSCRIPTION_UPDATE_SCHEMA
+>;
 
 export type ServiceNotification =
   | deleteSchema

packages/fxa-event-broker/src/test/lib/notificationProcessor.spec.ts

@@ -34,7 +34,7 @@ const baseMessage = {
 
 const baseLoginMessage = {
   ...baseMessage,
-  clientId: '123client',
+  clientId: '444c5d137fc34d82ae65441d7f26a504',
   deviceCount: 2,
   email: 'test@testuser.com',
   service: '123-client',
@@ -157,6 +157,17 @@ describe('ServiceNotificationProcessor', () => {
     assert.calledWith(db.storeLogin as SinonSpy, baseLoginMessage.uid, baseLoginMessage.clientId);
   });
 
+  it('normalizes the client id', async () => {
+    const message = Object.assign({}, baseLoginMessage);
+    message.clientId = message.clientId.toUpperCase();
+    updateStubMessage(baseLoginMessage);
+    consumer.start();
+    await pEvent(consumer.app, 'message_processed');
+    consumer.stop();
+    assert.calledOnce(db.storeLogin as SinonSpy);
+    assert.calledWith(db.storeLogin as SinonSpy, baseLoginMessage.uid, baseLoginMessage.clientId);
+  });
+
   const fetchOnValidMessage = {
     'delete message': baseDeleteMessage,
     'legacy subscription message': baseSubscriptionUpdateLegacyMessage,
@@ -180,7 +191,7 @@ describe('ServiceNotificationProcessor', () => {
   }
 
   const invalidMessages = {
-    login: { ...baseLoginMessage, ts: false },
+    login: { ...baseLoginMessage, clientId: 'test1234' },
     'password change': { ...basePasswordChangeMessage, ts: false },
     'password reset': { ...basePasswordResetMessage, ts: false },
     'primary email change': { ...basePrimaryEmailMessage, ts: false },