textbee/textbee
1
0
Fork 0
mirror of https://github.com/vernu/textbee.git synced 2026-09-03 03:29:58 +03:00
Code Issues Packages Projects Releases Wiki Activity

fix(gateway): drop the conditional OS write, restrict writable device fields

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
isra el
2026-08-08 12:14:15 +03:00
co-authored by Claude Opus 5
parent d60ea6e9bc
commit bfcafe598f
5 changed files with 125 additions and 187 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/src/gateway/gateway.controller.ts
+9 -2
View File
@@ -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 }
}
api/src/gateway/gateway.dto.spec.ts
+60
View File
@@ -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({})
})
})
api/src/gateway/gateway.dto.ts
+37
View File
@@ -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,
api/src/gateway/gateway.service.spec.ts
-128
View File
@@ -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', () => {
api/src/gateway/gateway.service.ts
+19 -57
View File
@@ -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<any> {
@@ -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 {