Skip to content

feat(security): filter aggregate reads by host scope (phase 3) - #94

Merged
malickyeu merged 1 commit into
mainfrom
feat/rbac-aggregate-filtering
Jul 30, 2026
Merged

feat(security): filter aggregate reads by host scope (phase 3)#94
malickyeu merged 1 commit into
mainfrom
feat/rbac-aggregate-filtering

Conversation

@malickyeu

Copy link
Copy Markdown
Contributor

Summary

Yes, the leaks were fixable. Phase 2 stopped a scoped user acting on another
host; it didn't stop them reading about it. This closes that — invariant 4 of
the design note, the one phase 2 left open.

Type of change

  • Bug fix
  • New feature
  • Docs only
  • Refactor / chore

Checklist

  • go test -short ./... and go vet ./... pass
  • gofmt gate is clean (gofmt -l $(git ls-files '*.go') after staging)
  • Frontend type-checks (cd web && npx tsc --noEmit)
  • Rebuilt and committed web/dist
  • Added/updated tests for the change
  • Updated docs/ and added a CHANGELOG.md entry for user-facing changes

Notes for reviewers

It was smaller than the design note feared, for an instructive reason: most of
the "aggregates" aren't aggregates.
Topology, the events feed, disk usage, stats
overview and published ports are each per-host reads taking ?host= — so phase
2's middleware check already covered the ones that map to a section. What actually
leaked was narrower and sharper:

  1. Three dashboard routes map to no section/api/stats/overview,
    /api/system/df, /api/stats/ports. The middleware returned before it looked
    at the host, so they served another host's counts, disk usage and published
    ports. Ungated means "no section required", not "any host you like": a named host
    must now be one the caller's grants reach somewhere. There's no single section to
    check against, hence the coarser Store.ReachableHosts.
  2. List endpoints — hosts, projects, the alert feed and the audit log named
    hosts and workloads the caller was scoped away from. All four filter now, through
    one shared predicate. The alert feed's unread count is computed after
    filtering
    ; a badge that still counted hidden events would announce them.
  3. Metrics history was the one real gap. The store keys by container id, so
    knowing an id was enough to read its CPU/memory series from any host.
    ContainerStat already carried HostID and recordHistory was throwing it
    away — the sample keeps it now and the series is authorised against it. An
    unrecorded id is treated as unknown, not local, so ids can't be probed for
    existence.

Running it end to end found a bug phase 2 shipped

The local daemon can be named two ways: ?host=0 ("the default"), or the id of
the seeded kind = 'local' row (usually 1). Only 0 was treated as
always-in-scope. So a scoped user kept ?host=0 but was denied ?host=1, and
the local host vanished from their host list. Both names normalise to the same
daemon now.

The unit tests missed it because they only ever used 0.
TestHostScope_LocalHostIsInScopeUnderEitherID uses both and fails on the old code.

Verification

Every fix was mutation-tested — removing the ungated-route check, the filter
predicate, the post-filter unread count, or the metrics host check each fails its
own test and nothing else's.

End to end, a user scoped to staging:

admin dev (scoped)
host list local, prod, staging local, staging
/api/stats/overview?host=prod 200/502 403
/api/containers?host=0 / ?host=1 ok both 200
/api/containers?host=prod ok 403

What's deliberately left

The alert engine still watches every host — it's background work with no user
context. So a rule that names you as an e-mail recipient can mail you about a host
you can't see in the app. That's a property of how recipients are configured, not
something the app can decide per viewer, and it's documented in docs/users.md
rather than implied away.

Phase 2 stopped a scoped user ACTING on another host. It did not stop them
READING about it, and this closes that — invariant 4 of the design note.

It was smaller than the note feared, for an instructive reason: most of the
"aggregates" aren't aggregates. Topology, the events feed, disk usage, stats
overview and published ports are per-host reads taking ?host=, so phase 2
already covered the ones that map to a section. What actually leaked:

- Three dashboard routes map to NO section, so the middleware returned before
  it looked at the host and served another host's counts, disk usage and
  published ports. Ungated means "no section required", not "any host you
  like"; a named host must now be one the caller's grants reach somewhere.
- The host list, project list, alert feed and audit log named hosts and
  workloads the caller was scoped away from. All four are filtered now. The
  alert feed's unread count is computed AFTER filtering — a badge that still
  counted hidden events would announce them.
- Metrics history was the one real gap. The store keys by container id, so
  knowing an id was enough to read its CPU/memory series from any host.
  ContainerStat already carried HostID and recordHistory was dropping it; the
  sample now keeps it and the series is authorised against it. An unrecorded id
  is unknown, not local, so ids can't be probed for existence.

Running it end to end found a bug phase 2 shipped: the local daemon can be
named as host 0 or as the id of the seeded kind='local' row, and only 0 was
treated as always-in-scope. A scoped user therefore kept ?host=0 but lost
?host=1, and the local host vanished from their host list. Both names now
normalise to the same daemon. The unit tests missed it because they only ever
used 0; there is now a test that uses both, and it fails on the old code.

Left deliberately global: the alert engine, which watches every host as
background work with no user context. Documented rather than implied away.

Verified each fix fails its test when removed, and end to end: a user scoped to
staging sees only local + staging in the host list, gets 403 on the three
dashboard routes for prod, and reaches the local daemon under either id.
Copilot AI review requested due to automatic review settings July 30, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@malickyeu
malickyeu merged commit 697e11a into main Jul 30, 2026
3 checks passed
@malickyeu
malickyeu deleted the feat/rbac-aggregate-filtering branch July 30, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants