Merge pull request #20026 from mozilla/FXA-12956

fix(auth): Omit null sender field from device commands

Author: vpomerleau

packages/fxa-auth-server/lib/routes/devices-and-sessions.js

@@ -275,7 +275,7 @@ module.exports = (
             uid,
             target: deviceId,
             index: msg.index,
-            sender: data.sender,
+            sender: data.sender || null,
             command: data.command,
           });
         }
@@ -325,6 +325,27 @@ module.exports = (
         const email = credentials.email;
         const sender = credentials.deviceId;
 
+        // Log diagnostic info when sender deviceId is missing to help identify the cause
+        if (!sender) {
+          log.warn('device.command.senderDeviceIdMissing', {
+            uid,
+            command,
+            target,
+            hasSessionToken: !!credentials.tokenId,
+            hasRefreshToken: !!credentials.refreshTokenId,
+            clientId: credentials.client?.id
+              ? hex(credentials.client.id)
+              : null,
+            clientName: credentials.client?.name || null,
+            deviceType: credentials.deviceType || null,
+            uaBrowser: credentials.uaBrowser || null,
+            uaBrowserVersion: credentials.uaBrowserVersion || null,
+            uaOS: credentials.uaOS || null,
+            uaOSVersion: credentials.uaOSVersion || null,
+            uaDeviceType: credentials.uaDeviceType || null,
+          });
+        }
+
         if (
           config.oauth.deviceCommandsEnabled === false &&
           credentials.refreshTokenId
@@ -351,7 +372,12 @@ module.exports = (
           ttl = DEFAULT_COMMAND_TTL.get(command);
         }
 
-        const data = { command, payload, sender };
+        // Only include sender if it exists, since the validation schema expects
+        // it to be optional (missing) or a valid device ID string, not null
+        const data = { command, payload };
+        if (sender) {
+          data.sender = sender;
+        }
         const { index } = await pushbox.store(uid, targetDevice.id, data, ttl);
 
         // To measure command delivery, we emit an initial "invoked" event for each invoked
@@ -360,7 +386,7 @@ module.exports = (
           uid,
           target,
           index,
-          sender,
+          sender: sender || null,
           command,
           targetOS: targetDevice.uaOS,
           targetType: targetDevice.type,

packages/fxa-auth-server/test/local/routes/devices-and-sessions.js

@@ -1381,6 +1381,72 @@ describe('/account/devices/invoke_command', () => {
     });
   });
 
+  it('omits sender field when deviceId is null/undefined', () => {
+    const mockPushbox = mocks.mockPushbox({
+      store: sinon.spy(async () => ({ index: 15 })),
+    });
+    const target = 'bogusid1';
+    const payload = { bogus: 'payload' };
+    mockRequest.payload = {
+      target,
+      command,
+      payload,
+    };
+    // Set deviceId to null to simulate the missing deviceId scenario
+    mockRequest.auth.credentials.deviceId = null;
+    mockRequest.auth.credentials.client = {
+      id: 'testclient',
+      name: 'Test Client',
+    };
+    mockRequest.auth.credentials.uaBrowser = 'Firefox';
+    mockRequest.auth.credentials.uaOS = 'Linux';
+
+    const route = getRoute(
+      makeRoutes({
+        customs: mockCustoms,
+        log: mockLog,
+        push: mockPush,
+        pushbox: mockPushbox,
+        db: mockDB,
+      }),
+      '/account/devices/invoke_command'
+    );
+
+    return runTest(route, mockRequest).then(() => {
+      // Verify diagnostic warning was logged
+      assert.equal(mockLog.warn.callCount, 1, 'warning was logged');
+      assert.calledWith(mockLog.warn, 'device.command.senderDeviceIdMissing');
+      const warningArgs = mockLog.warn.getCall(0).args[1];
+      assert.equal(warningArgs.uid, uid);
+      assert.equal(warningArgs.command, command);
+      assert.equal(warningArgs.clientName, 'Test Client');
+      assert.equal(warningArgs.uaBrowser, 'Firefox');
+
+      // Verify pushbox.store was called WITHOUT sender field
+      assert.equal(mockPushbox.store.callCount, 1, 'pushbox was called');
+      assert.calledWithExactly(
+        mockPushbox.store,
+        uid,
+        target,
+        {
+          command,
+          payload,
+          // Note: sender field should be completely absent, not null
+        },
+        undefined
+      );
+
+      // Verify metrics logging has sender as null
+      assert.callCount(mockLog.info, 2);
+      const invokedMetrics = mockLog.info.getCall(0).args[1];
+      assert.equal(
+        invokedMetrics.sender,
+        null,
+        'sender should be null in metrics'
+      );
+    });
+  });
+
   it('supports feature-flag for oauth devices', async () => {
     const mockPushbox = mocks.mockPushbox();
     const route = getRoute(