Skip to content

Commit 1531fd2

Browse files
committed
fix(auth): Omit null sender field from device commands
Because: * When invoking device commands without a deviceId (null/undefined), the sender field was being stored as null, which failed Joi validation on retrieval since the schema expects sender to be optional (missing) or a valid device ID string. * This fixes the validation error "messages[0].data.sender must be a string" in Sentry * Logging will help identify the root cause of the recent spike in unknown device errors This commit: * Omit sender field entirely from stored data when deviceId is falsy * Add diagnostic logging (device.command.senderDeviceIdMissing) to track when and why deviceId is missing, including client info, token type, and device/browser metadata * Normalize sender to null in metrics logging for consistency Closes #FXA-12956
1 parent 3bae779 commit 1531fd2

2 files changed

Lines changed: 95 additions & 3 deletions

File tree

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

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ module.exports = (
275275
uid,
276276
target: deviceId,
277277
index: msg.index,
278-
sender: data.sender,
278+
sender: data.sender || null,
279279
command: data.command,
280280
});
281281
}
@@ -325,6 +325,27 @@ module.exports = (
325325
const email = credentials.email;
326326
const sender = credentials.deviceId;
327327

328+
// Log diagnostic info when sender deviceId is missing to help identify the cause
329+
if (!sender) {
330+
log.warn('device.command.senderDeviceIdMissing', {
331+
uid,
332+
command,
333+
target,
334+
hasSessionToken: !!credentials.tokenId,
335+
hasRefreshToken: !!credentials.refreshTokenId,
336+
clientId: credentials.client?.id
337+
? hex(credentials.client.id)
338+
: null,
339+
clientName: credentials.client?.name || null,
340+
deviceType: credentials.deviceType || null,
341+
uaBrowser: credentials.uaBrowser || null,
342+
uaBrowserVersion: credentials.uaBrowserVersion || null,
343+
uaOS: credentials.uaOS || null,
344+
uaOSVersion: credentials.uaOSVersion || null,
345+
uaDeviceType: credentials.uaDeviceType || null,
346+
});
347+
}
348+
328349
if (
329350
config.oauth.deviceCommandsEnabled === false &&
330351
credentials.refreshTokenId
@@ -351,7 +372,12 @@ module.exports = (
351372
ttl = DEFAULT_COMMAND_TTL.get(command);
352373
}
353374

354-
const data = { command, payload, sender };
375+
// Only include sender if it exists, since the validation schema expects
376+
// it to be optional (missing) or a valid device ID string, not null
377+
const data = { command, payload };
378+
if (sender) {
379+
data.sender = sender;
380+
}
355381
const { index } = await pushbox.store(uid, targetDevice.id, data, ttl);
356382

357383
// To measure command delivery, we emit an initial "invoked" event for each invoked
@@ -360,7 +386,7 @@ module.exports = (
360386
uid,
361387
target,
362388
index,
363-
sender,
389+
sender: sender || null,
364390
command,
365391
targetOS: targetDevice.uaOS,
366392
targetType: targetDevice.type,

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1381,6 +1381,72 @@ describe('/account/devices/invoke_command', () => {
13811381
});
13821382
});
13831383

1384+
it('omits sender field when deviceId is null/undefined', () => {
1385+
const mockPushbox = mocks.mockPushbox({
1386+
store: sinon.spy(async () => ({ index: 15 })),
1387+
});
1388+
const target = 'bogusid1';
1389+
const payload = { bogus: 'payload' };
1390+
mockRequest.payload = {
1391+
target,
1392+
command,
1393+
payload,
1394+
};
1395+
// Set deviceId to null to simulate the missing deviceId scenario
1396+
mockRequest.auth.credentials.deviceId = null;
1397+
mockRequest.auth.credentials.client = {
1398+
id: 'testclient',
1399+
name: 'Test Client',
1400+
};
1401+
mockRequest.auth.credentials.uaBrowser = 'Firefox';
1402+
mockRequest.auth.credentials.uaOS = 'Linux';
1403+
1404+
const route = getRoute(
1405+
makeRoutes({
1406+
customs: mockCustoms,
1407+
log: mockLog,
1408+
push: mockPush,
1409+
pushbox: mockPushbox,
1410+
db: mockDB,
1411+
}),
1412+
'/account/devices/invoke_command'
1413+
);
1414+
1415+
return runTest(route, mockRequest).then(() => {
1416+
// Verify diagnostic warning was logged
1417+
assert.equal(mockLog.warn.callCount, 1, 'warning was logged');
1418+
assert.calledWith(mockLog.warn, 'device.command.senderDeviceIdMissing');
1419+
const warningArgs = mockLog.warn.getCall(0).args[1];
1420+
assert.equal(warningArgs.uid, uid);
1421+
assert.equal(warningArgs.command, command);
1422+
assert.equal(warningArgs.clientName, 'Test Client');
1423+
assert.equal(warningArgs.uaBrowser, 'Firefox');
1424+
1425+
// Verify pushbox.store was called WITHOUT sender field
1426+
assert.equal(mockPushbox.store.callCount, 1, 'pushbox was called');
1427+
assert.calledWithExactly(
1428+
mockPushbox.store,
1429+
uid,
1430+
target,
1431+
{
1432+
command,
1433+
payload,
1434+
// Note: sender field should be completely absent, not null
1435+
},
1436+
undefined
1437+
);
1438+
1439+
// Verify metrics logging has sender as null
1440+
assert.callCount(mockLog.info, 2);
1441+
const invokedMetrics = mockLog.info.getCall(0).args[1];
1442+
assert.equal(
1443+
invokedMetrics.sender,
1444+
null,
1445+
'sender should be null in metrics'
1446+
);
1447+
});
1448+
});
1449+
13841450
it('supports feature-flag for oauth devices', async () => {
13851451
const mockPushbox = mocks.mockPushbox();
13861452
const route = getRoute(

0 commit comments

Comments
 (0)