diff --git a/forge/ee/db/controllers/DeviceGroup.js b/forge/ee/db/controllers/DeviceGroup.js index 11bb899eeb..1278c94af8 100644 --- a/forge/ee/db/controllers/DeviceGroup.js +++ b/forge/ee/db/controllers/DeviceGroup.js @@ -104,6 +104,23 @@ module.exports = { if (!snapshot) { throw new ValidationError('Snapshot does not exist') } + // ensure the snapshot belongs to the same application as the device + // group - never trust a payload-supplied snapshot id to be in-scope. + // Scoped to application (not just team) because granular RBAC can + // authorize a caller for this application without authorizing them + // for another application in the same team. + const application = await deviceGroup.getApplication() + let snapshotApplicationId = null + if (snapshot.ownerType === 'instance') { + const project = await snapshot.getProject() + snapshotApplicationId = project?.ApplicationId + } else if (snapshot.ownerType === 'device') { + const device = await snapshot.getDevice() + snapshotApplicationId = device?.ApplicationId || (device?.ProjectId ? (await device.getProject())?.ApplicationId : null) + } + if (!application || !snapshotApplicationId || snapshotApplicationId !== application.id) { + throw new ValidationError('Snapshot does not belong to the same application') + } snapshotId = snapshot.id } diff --git a/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js b/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js index ed0638f257..832222ee71 100644 --- a/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js +++ b/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js @@ -442,10 +442,13 @@ describe('Application Device Groups API', function () { // first lets verify the devices have the snapshot set as target device1of2.should.have.property('targetSnapshotId', snapshot.id) - device1of2.should.have.property('targetSnapshotId', snapshot.id) + device2of2.should.have.property('targetSnapshotId', snapshot.id) - // create a new snapshot - const newSnapshot = await factory.createSnapshot({ name: generateName('snapshot') }, TestObjects.instance, TestObjects.bob) + // create a new snapshot on an instance in the group's own application - + // TestObjects.instance lives in a different BTeam application and would + // now be rejected by the application-scope check + const ownInstance = await factory.createInstance({ name: generateName('instance') }, application, app.stack, app.template, app.projectType, { start: false }) + const newSnapshot = await factory.createSnapshot({ name: generateName('snapshot') }, ownInstance, TestObjects.bob) // now call the API to update targetSnapshot only const response = await callUpdate(sid, application, deviceGroup, { @@ -529,6 +532,84 @@ describe('Application Device Groups API', function () { app.comms.devices.sendCommand.callCount.should.equal(0) // no devices should have been sent an update }) + // Bob (BTeam owner) should not be able to point his BTeam device group at an ATeam snapshot. + it('Rejects a targetSnapshot that belongs to another team (400)', async function () { + const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare() + + // Create an application, instance and snapshot owned by ATeam (foreign to bob's BTeam group) + const foreignApplication = await factory.createApplication({ name: generateName('foreign-app') }, TestObjects.ATeam) + const foreignInstance = await factory.createInstance({ name: generateName('foreign-instance') }, foreignApplication, app.stack, app.template, app.projectType, { start: false }) + const foreignSnapshot = await factory.createSnapshot({ name: generateName('foreign-snapshot') }, foreignInstance, TestObjects.alice) + + // now call the API to point the group at the foreign snapshot + const response = await callUpdate(sid, application, deviceGroup, { + targetSnapshotId: foreignSnapshot.hashid + }) + + // should fail + response.statusCode.should.equal(400) + response.json().should.have.property('code', 'invalid_input') + + const updatedDeviceGroup = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) + const updatedDevice1 = await app.db.models.Device.byId(device1of2.hashid) + const updatedDevice2 = await app.db.models.Device.byId(device2of2.hashid) + + // should not have updated the group or devices - still the original in-team snapshot + updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice1.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice2.should.have.property('targetSnapshotId', snapshot.id) + + // check no devices got an update command + app.comms.devices.sendCommand.callCount.should.equal(0) + }) + + // Bob (BTeam) should not be able to point his BTeam device group at a same-team + // snapshot that belongs to a different BTeam application. + it('Rejects a targetSnapshot that belongs to a different application in the same team (400)', async function () { + const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare() + // Create a second BTeam application (same team, different application) with its own instance snapshot + const otherApplication = await factory.createApplication({ name: generateName('other-app') }, TestObjects.BTeam) + const otherInstance = await factory.createInstance({ name: generateName('other-instance') }, otherApplication, app.stack, app.template, app.projectType, { start: false }) + const otherInstanceSnapshot = await factory.createSnapshot({ name: generateName('other-instance-snapshot') }, otherInstance, TestObjects.bob) + const response = await callUpdate(sid, application, deviceGroup, { + targetSnapshotId: otherInstanceSnapshot.hashid + }) + response.statusCode.should.equal(400) + response.json().should.have.property('code', 'invalid_input') + const updatedDeviceGroup = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) + const updatedDevice1 = await app.db.models.Device.byId(device1of2.hashid) + const updatedDevice2 = await app.db.models.Device.byId(device2of2.hashid) + updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice1.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice2.should.have.property('targetSnapshotId', snapshot.id) + app.comms.devices.sendCommand.callCount.should.equal(0) + }) + // Same scenario, but the foreign snapshot belongs to an application-owned device + // rather than an instance - exercises the device-owned branch of the fix. + it('Rejects a device-owned targetSnapshot that belongs to a different application in the same team (400)', async function () { + const { sid, application, deviceGroup, device1of2, device2of2, snapshot } = await prepare() + const otherApplication = await factory.createApplication({ name: generateName('other-app') }, TestObjects.BTeam) + const otherDevice = await factory.createDevice({ name: generateName('other-device') }, TestObjects.BTeam, null, otherApplication) + const otherDeviceSnapshot = await app.db.models.ProjectSnapshot.create({ + name: generateName('other-device-snapshot'), + DeviceId: otherDevice.id, + settings: {}, + flows: {} + }) + const response = await callUpdate(sid, application, deviceGroup, { + targetSnapshotId: otherDeviceSnapshot.hashid + }) + response.statusCode.should.equal(400) + response.json().should.have.property('code', 'invalid_input') + const updatedDeviceGroup = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) + const updatedDevice1 = await app.db.models.Device.byId(device1of2.hashid) + const updatedDevice2 = await app.db.models.Device.byId(device2of2.hashid) + updatedDeviceGroup.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice1.should.have.property('targetSnapshotId', snapshot.id) + updatedDevice2.should.have.property('targetSnapshotId', snapshot.id) + app.comms.devices.sendCommand.callCount.should.equal(0) + }) + it('Cannot update a device group with empty name', async function () { const sid = await login('bob', 'bbPassword') const application = await factory.createApplication({ name: generateName('app') }, TestObjects.BTeam)