block / block/proto-fleet

SetRackSlotPosition accepts out-of-bounds slots (no dimension read, no rack lock, no membership check)

Open
#861 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.