SetRackSlotPosition accepts out-of-bounds slots (no dimension read, no rack lock, no membership check)
- Dominant language
- Go
- Stars
- 55
- Forks
- 16
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 87
Description
## Summary
`DeviceCollectionService.SetRackSlotPosition` (and its `DeviceSetService` facade) writes a slot position without ever reading the rack's dimensions, so it accepts positions outside the grid. It also takes no rack row lock and does not verify membership.
Found while reviewing #855, which fixed the equivalent gaps in the *batch* path (`AssignDevicesToRack`). This is the pre-existing single-slot path and is not touched by that PR.
## Example
Rack `R1` is **2×2**, so the only valid cells are (0,0), (0,1), (1,0), (1,1). Miner `M` is a member of `R1`.
| Call | Result |
|---|---|
| `AssignDevicesToRack{target_rack_id: R1, slot_assignments: [{M, row: 5, col: 5}]}` | rejected — `slot row 5 is out of bounds (rack has 2 rows)` |
| `SetRackSlotPosition{collection_id: R1, device_identifier: M, position: {row: 5, col: 5}}` | **200 OK**, row persisted |
`M` now has a `rack_slot` row at a cell the 2×2 grid can never render. It reads as placed in the data but is invisible in the grid UI, so an operator cannot drag it off — recovering needs a `ClearRackSlotPosition` API call.
No race is required for this; the bounds check is simply absent. `buf.validate` on `RackSlotPosition` only constrains `row >= 0` / `column >= 0` (`proto/collection/v1/collection.proto`), with no upper bound.
## Two related gaps in the same method
**No rack lock.** A bounds check alone would still be racy. Resize goes through `resolveAndApplyRackPlacement`, which takes `LockRackPlacementForWrite`; the slot RPC takes none, so:
1. B places `M` at (11,11) in a 12×12 rack — reads dims, 11 < 12, passes
2. A resizes `R1` to 2×2 — commits
3. B writes (11,11) — same orphaned slot
**Silent success for a non-member.** The store query is `INSERT INTO rack_slot ... SELECT ... FROM device_set_membership WHERE ...`. If the device is not a member, zero rows insert, no error is raised, and the RPC returns 200 for a placement that never happened. This is the same bug Codex flagged in `applyRackSlotDelta` on #855, which was fixed there but not here.
## Proposed fix
Mirror what `AssignDevicesToRack` already does, inside the existing transaction in `Service.SetRackSlotPosition` (`server/internal/domain/collection/service.go`). Both the `collection` and `deviceset` handlers delegate to this one service method, so there is a single place to change:
1. `LockRackPlacementForWrite(ctx, req.CollectionId, orgID)` before the reads, so the checks hold for the transaction (this also gives the canonical rack-first lock order).
2. `GetRackInfo(...)`; treat `nil` as a broken invariant and fail — a RACK always has a `device_set_rack` row.
3. Reject `row >= rackInfo.Rows` or `column >= rackInfo.Columns` with InvalidArgument, matching the batch path's wording.
4. Confirm the device is actually a member before reporting success, rather than relying on the query's silent no-op.
Roughly 15 lines plus tests: out-of-bounds rejected, non-member rejected, in-bounds still succeeds.
Worth considering alongside: `ClearRackSlotPosition` has the same no-lock, no-membership-check shape, though with no bounds concern since it only deletes.
## Severity
Low. Triggering it needs `PermRackManage` plus a direct API or CLI call — the web UI only offers rendered cells, so it will not happen by accident. Nothing crashes; the symptom is a miner stuck in an unrenderable slot.
## Notes
An occupied-cell collision on this RPC used to surface as a 500. #855 maps `uk_rack_slot_position` to InvalidArgument in the store, which covers this RPC too, so that part is already handled once #855 lands.
Contributor guide
Research direction
Start in server/internal/domain/collection/service.go at Service.SetRackSlotPosition and compare it with the AssignDevicesToRack path, including LockRackPlacementForWrite and GetRackInfo. Add coverage for out-of-bounds and non-member placements while preserving successful in-bounds placement, then run the relevant service tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100