HydraIssues

Any node token can read and mutate any head; requireAdminOrNodeToken never scopes to the path ID
open bug Project: hydracluster Reporter: cederik 6 Aug 2026 16:46

Description

Any valid node token in the fleet can read and mutate any head. The middleware that guards the head endpoints authenticates the token but never checks it against the {id} being acted on, and none of the handlers check either.

Found while scoping self-service WireGuard enrollment (#449), where the obvious implementation would have widened this. Filing separately because it is a live authorization gap independent of that work.

The gap

requireAdminOrNodeToken (pkg/api/handlers_body.go:77-107) resolves the bearer token to a node and puts it in the request context:

node, err := st.GetByToken(token)
...
ctx := context.WithValue(r.Context(), nodeContextKey, &nodeContext{node: node, store: st})
next(w, r.WithContext(ctx))

Any token belonging to any enrolled node passes. The middleware never compares that node to the {id} in the path, and it cannot — it does not know the route shape.

The handlers do not compare either. All six head handlers on {id} routes read the path value and act on it directly, and none consult the node the middleware authenticated:

handleGetHead, handleUpdateHead, handleHeadCommands, handleHeadStreamStop,
handleHeadScreenshotUpload, handleGetHeadExperiences
-> all read r.PathValue("id"); none reference nodeCtxFromRequest

So the effective rule is "any enrolled node may act on any head", not "a head may act on itself".

The trust boundary is the whole fleet: every enrolled node holds a token, including bodies and hydraskin nodes, not just heads.

Affected routes

pkg/api/server.go:

  • 325 GET /api/v1/bodies/eligible
  • 340 POST /api/v1/nodes/{id}/sunshine-pin
  • 389 GET /api/v1/heads
  • 390 GET /api/v1/heads/{id}
  • 391 PUT /api/v1/heads/{id}
  • 392 GET /api/v1/heads/{id}/experiences
  • 394 POST /api/v1/heads/{id}/screenshot
  • 395 GET /api/v1/heads/{id}/commands
  • 398 DELETE /api/v1/heads/{id}/stream

Impact, as verified on production

GET /api/v1/heads/{id} returns sunshine_username and sunshine_password, both populated. Any node token can therefore read another head's Sunshine credentials. Full field list returned:

diagnostics, district, experience_library_url, id, name, status, stream,
sunshine_password, sunshine_username, type, venue

PUT /api/v1/heads/{id} accepts status, diagnostics and body_id (handlers_head.go:106-117), so any node can rewrite another head's reported status or its live body assignment.

DELETE /api/v1/heads/{id}/stream lets any node stop any other head's stream.

POST /api/v1/heads/{id}/screenshot lets any node upload a screenshot attributed to another head, so operator-facing evidence can be spoofed.

Bounding it: the list endpoint GET /api/v1/heads does NOT expose token or wireguard_config — it returns only diagnostics, district, id, last_seen, name, node_status, owner, status, stream, type, venue. So node tokens and WireGuard private keys are not readable this way. The exposure is Sunshine credentials plus cross-head state read/write.

Suggested fix

Handlers on {id} routes should assert that the authenticated node is the node being acted on, while still allowing admin callers through. Something like: if the request carries a node context and that node's ID is not the path ID, reject with 403.

Implementation trap worth noting: nodeCtxFromRequest (handlers_body.go:114-116) does an unchecked type assertion:

return r.Context().Value(nodeContextKey).(*nodeContext)

Admin callers pass through requireAdminOrNodeToken without a context value being set (see the comment at handlers_body.go:71), so calling nodeCtxFromRequest in any handler reachable by an admin will panic on a nil value. The fix needs the comma-ok form, or a helper returning (nc, ok).

Worth deciding as part of this whether the same self-scoping rule should apply to POST /api/v1/nodes/{id}/sunshine-pin, which has the same shape on the body side.

The testbook already asserts the correct behaviour, and it does not hold

Found while adding testbook coverage for other work. docs/testbooks/testbook.md states the intended invariant explicitly:

  1. PUT /api/v1/heads/{id} remains admin-only — verify it returns 401 when
    called with a node token.

That step cannot pass. The route is registered with requireAdminOrNodeToken (pkg/api/server.go:391) and handleUpdateHead never compares the authenticated node to {id}, so a node token is accepted and the call returns 200.

Step 13 immediately before it is deliberate and correct — the read endpoints (GET /api/v1/heads, GET /api/v1/heads/{id}, GET /api/v1/heads/{id}/experiences) are documented as accepting node tokens because bodies poll them every tick. So the design intent is clear: reads shared, writes admin-only. The implementation does not draw that line anywhere.

The same gap breaks step 16, which says of DELETE /api/v1/heads/{id}/stream:

  • Flatscreen variant requires admin token — verify 401 with a node token.
  • iPad variant accepts both admin token and node token — verify 200 [...]

handleHeadStreamStop branches on head type (isIPad := headType(*head) == "hydraheadipad"), not on caller identity. There is no auth branch in the handler at all, so the flatscreen variant accepts a node token exactly like the iPad one.

This raises the severity: it is not an unclear design boundary that was never decided. The boundary was decided, written down, and is unenforced in three places. Note also that step 13's "expecting 401" for an unknown token still passes, so a test run against the read endpoints looks healthy while the write endpoints are open.

Both steps have been marked known-failing in the testbook rather than deleted, since the testbook is right and the code is wrong.