From bfcafe598fe6e04f0e2a0af070b444127e8b0b76 Mon Sep 17 00:00:00 2001 From: isra el Date: Sat, 8 Aug 2026 12:14:15 +0300 Subject: [PATCH] fix(gateway): drop the conditional OS write, restrict writable device fields Reverts the conditional findOneAndUpdate filter added for the OS provenance race. When the filter did not match it discarded the whole update rather than just the OS fields, so a raced call silently dropped the fcm token, the enabled flag and, on the heartbeat path, lastHeartbeat itself, leaving the device looking offline with no error and no retry. The race it guarded against only produced a temporarily wrong osVersion that the next heartbeat corrects, so the guard cost more than it saved. It also only engaged when normalizeOsFields had already approved the write, and it broke the existing updateDevice tests. The stored-version check in normalizeOsFields is kept as-is. Also restricts what a client may write on a device. There is no global ValidationPipe, so the request body reached $set unfiltered and any device field was settable on your own device, including user and the SMS counters. The allowed set is applied at the controller, which is where input stops being trusted; the service still receives internally built payloads that legitimately carry other fields. Co-Authored-By: Claude Opus 5 (1M context) --- api/src/gateway/gateway.controller.ts | 11 +- api/src/gateway/gateway.dto.spec.ts | 60 +++++++++++ api/src/gateway/gateway.dto.ts | 37 +++++++ api/src/gateway/gateway.service.spec.ts | 128 ------------------------ api/src/gateway/gateway.service.ts | 76 ++++---------- 5 files changed, 125 insertions(+), 187 deletions(-) create mode 100644 api/src/gateway/gateway.dto.spec.ts diff --git a/api/src/gateway/gateway.controller.ts b/api/src/gateway/gateway.controller.ts index b88704c..3474a65 100644 --- a/api/src/gateway/gateway.controller.ts +++ b/api/src/gateway/gateway.controller.ts @@ -23,6 +23,7 @@ import { AuthGuard } from '../auth/guards/auth.guard' import { ReceivedSMSDTO, RegisterDeviceInputDTO, + pickDeviceWritableFields, RetrieveSMSResponseDTO, SendBulkSMSInputDTO, SendBulkSMSRequestDTO, @@ -67,7 +68,10 @@ export class GatewayController { @ApiOperation({ summary: 'Register device' }) @Post('/devices') async registerDevice(@Body() input: RegisterDeviceInputDTO, @Request() req) { - const data = await this.gatewayService.registerDevice(input, req.user) + const data = await this.gatewayService.registerDevice( + pickDeviceWritableFields(input), + req.user, + ) return { data } } @@ -98,7 +102,10 @@ export class GatewayController { @Param('id') deviceId: string, @Body() input: RegisterDeviceInputDTO, ) { - const data = await this.gatewayService.updateDevice(deviceId, input) + const data = await this.gatewayService.updateDevice( + deviceId, + pickDeviceWritableFields(input), + ) return { data } } diff --git a/api/src/gateway/gateway.dto.spec.ts b/api/src/gateway/gateway.dto.spec.ts new file mode 100644 index 0000000..0190e05 --- /dev/null +++ b/api/src/gateway/gateway.dto.spec.ts @@ -0,0 +1,60 @@ +import { pickDeviceWritableFields } from './gateway.dto' + +describe('pickDeviceWritableFields', () => { + it('keeps every field a device client legitimately sends', () => { + const input = { + enabled: true, + fcmToken: 'token', + brand: 'samsung', + manufacturer: 'samsung', + model: 'SM-S928B', + name: 'samsung SM-S928B', + serial: 'abc', + buildId: 'BP2A.250605.031.A3', + os: 'Android', + osVersion: '16', + osApiLevel: 36, + osBuildFingerprint: 'samsung/e3qxxx/e3q:16/BP2A.250605.031.A3/x:user/release-keys', + appVersionName: '2.8.0', + appVersionCode: 18, + simInfo: { lastUpdated: 1, sims: [] }, + } as any + + expect(pickDeviceWritableFields(input)).toEqual(input) + }) + + // There is no global ValidationPipe, so the body reaches `$set` as-is. These + // are device fields a client must never be able to write on its own device. + it.each([ + ['user', '507f1f77bcf86cd799439011'], + ['sentSMSCount', 0], + ['receivedSMSCount', 999], + ['isDefault', true], + ['osVersionSource', 'reported'], + ['heartbeatEnabled', false], + ['smsSendDelaySeconds', 0], + ['lastHeartbeat', new Date()], + ['_id', 'deadbeefdeadbeefdeadbeef'], + ])('drops a client-sent %s', (field, value) => { + const picked = pickDeviceWritableFields({ + model: 'Pixel 6', + [field]: value, + } as any) + + expect(picked).not.toHaveProperty(field) + expect(picked.model).toBe('Pixel 6') + }) + + it('omits absent fields rather than setting them undefined', () => { + // An explicit undefined would still be a key, and Mongoose only drops + // undefined from $set by convention, so keep the object minimal. + expect(pickDeviceWritableFields({ model: 'Pixel 6' } as any)).toEqual({ + model: 'Pixel 6', + }) + }) + + it('tolerates an empty or missing body', () => { + expect(pickDeviceWritableFields({} as any)).toEqual({}) + expect(pickDeviceWritableFields(undefined as any)).toEqual({}) + }) +}) diff --git a/api/src/gateway/gateway.dto.ts b/api/src/gateway/gateway.dto.ts index 4689013..0307564 100644 --- a/api/src/gateway/gateway.dto.ts +++ b/api/src/gateway/gateway.dto.ts @@ -87,6 +87,43 @@ export class RegisterDeviceInputDTO { simInfo?: SimInfoCollectionDTO } +/** + * Fields a client may write on a device. There is no global ValidationPipe, so + * without this the whole request body reaches `$set` and any device field is + * settable, `user` and the SMS counters included. + * + * Applied at the controller, which is where input stops being trusted. The + * service also receives internally built payloads (registerDevice hands its own + * data to updateDevice) that legitimately carry fields absent from this list. + */ +const DEVICE_WRITABLE_FIELDS = [ + 'enabled', + 'fcmToken', + 'brand', + 'manufacturer', + 'model', + 'name', + 'serial', + 'buildId', + 'os', + 'osVersion', + 'osApiLevel', + 'osBuildFingerprint', + 'appVersionName', + 'appVersionCode', + 'simInfo', +] as const + +export function pickDeviceWritableFields( + input: RegisterDeviceInputDTO, +): RegisterDeviceInputDTO { + const picked: any = {} + for (const field of DEVICE_WRITABLE_FIELDS) { + if (input?.[field] !== undefined) picked[field] = input[field] + } + return picked +} + export class SMSData { @ApiProperty({ type: String, diff --git a/api/src/gateway/gateway.service.spec.ts b/api/src/gateway/gateway.service.spec.ts index 1c6ab49..42c7fe2 100644 --- a/api/src/gateway/gateway.service.spec.ts +++ b/api/src/gateway/gateway.service.spec.ts @@ -44,7 +44,6 @@ describe('GatewayService', () => { find: jest.fn(), findById: jest.fn(), findByIdAndUpdate: jest.fn(), - findOneAndUpdate: jest.fn(), findByIdAndDelete: jest.fn(), updateMany: jest.fn(), create: jest.fn(), @@ -1326,133 +1325,6 @@ describe('GatewayService', () => { }) }) - // The device guard only sees the :id param, so the second identifier on - // these routes has to be bound to the device by the query itself. - describe('concurrent OS metadata update race protection (Finding 1 regression tests)', () => { - const mockDeviceId = 'device123' - const mockDevice = { - _id: mockDeviceId, - user: 'user123', - enabled: true, - osVersion: '14', - osVersionSource: 'fingerprint', - } - - beforeEach(() => { - mockBillingService.getUserLimits.mockResolvedValue({ deviceLimit: -1 }) - }) - - describe('updateDevice atomic OS update', () => { - it('uses a conditional filter to prevent weaker source from overwriting stronger', async () => { - mockDeviceModel.findById.mockResolvedValue(mockDevice) - mockDeviceModel.findOneAndUpdate.mockResolvedValue({ - ...mockDevice, - osVersion: '14', - osVersionSource: 'fingerprint', - }) - - // Attempt to update with a weaker buildId-derived value - await service.updateDevice(mockDeviceId, { - model: 'Pixel 6', - buildId: 'TP1A.220624.014', // derives Android 13, buildId source - } as RegisterDeviceInputDTO) - - // Verify the filter includes source rank condition - const [filter, update] = mockDeviceModel.findOneAndUpdate.mock.calls[0] - expect(filter._id).toBe(mockDeviceId) - expect(filter.$or).toBeDefined() - // buildId rank=1 should only update if stored source is also buildId or missing - expect(filter.$or).toEqual([ - { osVersionSource: { $exists: false } }, - { osVersionSource: null }, - { osVersionSource: 'buildId' }, - ]) - }) - - it('allows stronger source to update when filter includes it', async () => { - mockDeviceModel.findById.mockResolvedValue({ - ...mockDevice, - osVersionSource: 'buildId', - }) - mockDeviceModel.findOneAndUpdate.mockResolvedValue({ - ...mockDevice, - osVersion: '16', - osVersionSource: 'reported', - }) - - // Attempt to update with a stronger reported value - await service.updateDevice(mockDeviceId, { - model: 'Pixel 6', - buildId: 'build123', - os: 'Android', - osVersion: '16', - } as RegisterDeviceInputDTO) - - const [filter] = mockDeviceModel.findOneAndUpdate.mock.calls[0] - // reported rank=3 should update buildId(1) and fingerprint(2) - expect(filter.$or).toContainEqual({ osVersionSource: 'buildId' }) - expect(filter.$or).toContainEqual({ osVersionSource: 'fingerprint' }) - expect(filter.$or).toContainEqual({ osVersionSource: 'reported' }) - }) - - it('refetches device when conditional filter rejects the update', async () => { - mockDeviceModel.findById.mockResolvedValue(mockDevice) - // findOneAndUpdate returns null when filter doesn't match (stronger source in DB) - mockDeviceModel.findOneAndUpdate.mockResolvedValueOnce(null) - mockDeviceModel.findById.mockResolvedValueOnce(mockDevice) - - const result = await service.updateDevice(mockDeviceId, { - model: 'Pixel 6', - buildId: 'TP1A.220624.014', - } as RegisterDeviceInputDTO) - - // Should have called findById twice: once at start, once after rejected update - expect(mockDeviceModel.findById).toHaveBeenCalledTimes(2) - expect(result).toEqual(mockDevice) - }) - }) - - describe('heartbeat atomic OS update', () => { - it('uses a conditional filter when OS metadata changes', async () => { - mockDeviceModel.findById.mockResolvedValue(mockDevice) - mockDeviceModel.findOneAndUpdate.mockResolvedValue({ - ...mockDevice, - lastHeartbeat: expect.any(Date), - }) - - await service.heartbeat(mockDeviceId, { - fcmToken: 'token', - os: 'Android', - osVersion: '16', - } as any) - - const [filter] = mockDeviceModel.findOneAndUpdate.mock.calls[0] - expect(filter._id).toBe(mockDeviceId) - // reported rank=3 should be able to update any source - if (filter.$or) { - expect(filter.$or).toContainEqual({ osVersionSource: 'buildId' }) - expect(filter.$or).toContainEqual({ osVersionSource: 'fingerprint' }) - expect(filter.$or).toContainEqual({ osVersionSource: 'reported' }) - } - }) - - it('does not add filter when OS fields are unchanged', async () => { - mockDeviceModel.findById.mockResolvedValue(mockDevice) - mockDeviceModel.findOneAndUpdate.mockResolvedValue(mockDevice) - - // Heartbeat with no OS changes - await service.heartbeat(mockDeviceId, { - fcmToken: 'token', - batteryPercentage: 85, - } as any) - - const [filter] = mockDeviceModel.findOneAndUpdate.mock.calls[0] - // No $or filter when OS fields don't change - expect(filter._id).toBe(mockDeviceId) - }) - }) - }) - // The device guard only sees the :id param, so the second identifier on // these routes has to be bound to the device by the query itself. describe('SMS lookups are scoped to the requesting device', () => { diff --git a/api/src/gateway/gateway.service.ts b/api/src/gateway/gateway.service.ts index 854c6f3..0670114 100644 --- a/api/src/gateway/gateway.service.ts +++ b/api/src/gateway/gateway.service.ts @@ -114,7 +114,10 @@ export class GatewayService { delete deviceData.osApiLevel delete deviceData.osBuildFingerprint delete deviceData.osVersionSource - const osFields = normalizeOsFields(input, device?.osVersionSource, device?.osVersion) + Object.assign( + deviceData, + normalizeOsFields(input, device?.osVersionSource, device?.osVersion), + ) // Set default name to "brand model" if not provided if (!deviceData.name && input.brand && input.model) { @@ -136,10 +139,10 @@ export class GatewayService { } if (device && device.appVersionCode <= 11) { - // re-enable path: use updateDevice which now handles atomic OS updates + // re-enable path: updateDevice enforces the device limit on the + // disabled -> enabled transition return await this.updateDevice(device._id.toString(), { ...deviceData, - ...osFields, enabled: true, }) } else { @@ -153,8 +156,6 @@ export class GatewayService { deviceData.isDefault = true } - // New device creation: no race possible, directly assign OS fields - Object.assign(deviceData, osFields) return await this.deviceModel.create(deviceData) } } @@ -303,7 +304,10 @@ export class GatewayService { delete updateData.osApiLevel delete updateData.osBuildFingerprint delete updateData.osVersionSource - const osFields = normalizeOsFields(input, device.osVersionSource, device.osVersion) + Object.assign( + updateData, + normalizeOsFields(input, device.osVersionSource, device.osVersion), + ) // Handle simInfo if provided if (input.simInfo) { @@ -319,36 +323,11 @@ export class GatewayService { updateData.fcmTokenInvalidReason = undefined } - // Atomic OS metadata update: build a filter that only allows the update - // when the current osVersionSource in the DB is not stronger than what - // we're writing. This prevents concurrent weaker writes from clobbering - // stronger ones. - const filter: any = { _id: deviceId } - if (osFields.osVersionSource) { - const newSourceRank = { buildId: 1, fingerprint: 2, reported: 3 }[osFields.osVersionSource] - // Only update if: no source exists, OR existing source rank <= new rank - filter.$or = [ - { osVersionSource: { $exists: false } }, - { osVersionSource: null }, - { osVersionSource: 'buildId' }, // rank 1 - ...(newSourceRank >= 2 ? [{ osVersionSource: 'fingerprint' }] : []), - ...(newSourceRank >= 3 ? [{ osVersionSource: 'reported' }] : []), - ] - } - - const updated = await this.deviceModel.findOneAndUpdate( - filter, - { $set: { ...updateData, ...osFields } }, + return await this.deviceModel.findByIdAndUpdate( + deviceId, + { $set: updateData }, { new: true }, ) - - // If the filter rejected the update (stronger source already in DB), refetch - // to return current state with the stronger OS metadata - if (!updated) { - return await this.deviceModel.findById(deviceId) - } - - return updated } async deleteDevice(deviceId: string): Promise { @@ -1467,8 +1446,9 @@ const updatedSms = await this.smsModel.findByIdAndUpdate( // Update OS info if provided. These change at most once per OS upgrade, // so skip keys already matching the stored value to keep the write a no-op. - const osFields = normalizeOsFields(input, device.osVersionSource, device.osVersion) - for (const [key, value] of Object.entries(osFields)) { + for (const [key, value] of Object.entries( + normalizeOsFields(input, device.osVersionSource, device.osVersion), + )) { if (device[key] !== value) updateData[key] = value } @@ -1497,30 +1477,12 @@ const updatedSms = await this.smsModel.findByIdAndUpdate( } } - // Atomic OS metadata update: build a filter that only allows the update - // when the current osVersionSource in the DB is not stronger than what - // we're writing. This prevents concurrent weaker writes from clobbering - // stronger ones. - const filter: any = { _id: deviceId } - if (osFields.osVersionSource && updateData.osVersionSource) { - const newSourceRank = { buildId: 1, fingerprint: 2, reported: 3 }[osFields.osVersionSource] - // Only update if: no source exists, OR existing source rank <= new rank - filter.$or = [ - { osVersionSource: { $exists: false } }, - { osVersionSource: null }, - { osVersionSource: 'buildId' }, // rank 1 - ...(newSourceRank >= 2 ? [{ osVersionSource: 'fingerprint' }] : []), - ...(newSourceRank >= 3 ? [{ osVersionSource: 'reported' }] : []), - ] - } - - // Update device with all changes (filter only applies if OS fields changed) - await this.deviceModel.findOneAndUpdate(filter, { + // Update device with all changes + await this.deviceModel.findByIdAndUpdate(deviceId, { $set: updateData, }) - // Fetch updated device to get current name and OS metadata (which may not - // have been updated if a stronger source won the race) + // Fetch updated device to get current name const updatedDevice = await this.deviceModel.findById(deviceId) return {