feat(security): per-host RBAC scoping (phase 2) - #93
Merged
Conversation
A role can now be limited to specific Docker hosts, so "may restart containers" can mean "on staging, not production". Phase 2 of design/rbac-roles-and-host-scoping.md. This is new authorization, not a tightened check. A non-admin holding a section could act on ANY daemon by passing ?host=N, including hosts they couldn't see on the Hosts page, which is gated by the separate hosts section. The enforcement point is the permissions middleware, NOT resolveHostID as the design note sketched. Every host-targeting route already passes through that middleware, so the ~60 call sites are covered without editing them — and, more to the point, so is the route someone adds next year. resolveHostID stays a plain parse and now says so. The two places a host is named outside the URL still need an explicit call: the WebSocket subscribe frame and a managed project's own host_id, both of which are covered. MCP gets the same treatment: every tool passes its host_id to the gate, tokens can be narrowed to a host subset, and deploy_project looks up the project's host rather than assuming the local daemon. The audit log records which host an action happened on, because a scoped action is only meaningful with the where. Nothing changes on upgrade. An empty host list means every host, a section granted directly on an account stays unscoped, and the local daemon is always in scope — making it scopeable would let a single-host install lock itself out. Invariants 1, 2, 3, 5, 6 and 7 of §5 are pentests. Invariant 4 is deliberately NOT asserted: aggregate reads are phase 3, and the leak is documented in docs/users.md and the changelog in the agreed terms rather than implied away. Verified the tests fail on the wrong behaviour: dropping the host check in checkAccess fails six of them, passing 0 instead of the parsed host fails the two middleware tests, removing MCP token host narrowing fails two, and removing the project-host lookup fails one. Also verified end to end over HTTP — a user scoped to staging gets 403 on ?host=prod before any Docker call, 502 on ?host=staging (authorization passed, the fake daemon didn't answer), 200 on local; an admin gets 502 everywhere, never 403.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
A few authorization/audit edge cases (notably consistent normalization/validation of non-positive host IDs and sanitizing role hostIds before persisting/auditing) should be addressed to avoid inconsistent host scoping behavior and misleading audit records.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Implements per-host RBAC scoping (phase 2) so section grants (REST ?host=, WebSocket subscribe frames, MCP tool host_id, and managed-project deploys) are authorized against the specific Docker host targeted, with backwards-compatible defaults (empty host scope = all hosts; local daemon always in scope) and audit logging extended to record the target host.
Changes:
- Add host scoping to the RBAC gate and propagate host context through REST middleware, WebSocket streaming, and MCP authorization/token narrowing.
- Persist role host scopes and token host scopes, and record
host_idin audit log entries. - Update UI to allow selecting host scope in the role editor and display effective “Where” per grant on the profile page; update docs/design/changelog accordingly.
File summaries
| File | Description |
|---|---|
| web/src/pages/Users.tsx | Role editor UI adds host picker and submits hostIds. |
| web/src/pages/Profile.tsx | Profile “Access” table gains “Where” column for host reach. |
| web/src/lib/types.ts | Add Role.hostIds and effective grant host metadata types. |
| web/src/lib/roles.ts | roleSummary flags host-scoped roles in UI summaries. |
| web/src/lib/roles.test.ts | Unit tests for roleSummary host-scope rendering. |
| web/src/lib/api.ts | Roles API payloads now include hostIds. |
| web/dist/index.html | Rebuilt web bundle entrypoint hash update. |
| web/dist/assets/CodeEditor-B6Yen_hm.js | Rebuilt web asset referencing new bundle hash. |
| NEXT.md | Mark RBAC host scoping as shipped and clarify phase status. |
| internal/ws/hub.go | WebSocket hub gate now receives (channel, hostID). |
| internal/ws/hub_test.go | Update hub allow callback signature in tests. |
| internal/store/users.go | Add JSON helpers for storing token host id lists. |
| internal/store/store.go | Add role_hosts table; migrate api_tokens.host_ids and audit_log.host_id. |
| internal/store/roles.go | Persist/load role host scopes; compute effective grants with host scope. |
| internal/store/roles_test.go | Update role CRUD tests for new UpdateRole signature. |
| internal/store/audit.go | Store/read host_id in audit log entries. |
| internal/store/api_tokens.go | Persist/load token host scope in api_tokens.host_ids. |
| internal/mcp/tools_read.go | MCP read tools authorize with host context (host_id). |
| internal/mcp/tools_read_more.go | MCP additional read tools authorize with host context; document unscoped metrics history. |
| internal/mcp/tools_control.go | MCP control tools authorize project deploy/down via project host lookup. |
| internal/mcp/tool_authz_coverage_test.go | Update coverage gate signature for host-aware CheckAccess. |
| internal/mcp/smoke_test.go | Update smoke RBAC check signature for host-aware CheckAccess. |
| internal/mcp/server.go | Add token host narrowing + host-aware CheckAccess plumbing. |
| internal/mcp/server_test.go | Update authorize/narrowed tests for new host-aware signature. |
| internal/mcp/resources.go | Resources authorize via host-aware authorizeExtra. |
| internal/mcp/pentest_test.go | Update pentest fixtures for host-aware CheckAccess signature. |
| internal/mcp/oauth_token_test.go | Update OAuth token tests for host-aware CheckAccess signature. |
| internal/mcp/host_scope_pentest_test.go | New pentests for MCP host scoping + stale-token scenario + project host lookup. |
| internal/api/ws_handler.go | WebSocket allow closure now checks access per host. |
| internal/api/server.go | resolveHostID documented as parse-only; audit now records host id from request. |
| internal/api/role_pentest_test.go | Update role pentests for host-aware checkAccess signature. |
| internal/api/role_handlers.go | Roles API returns/accepts hostIds and audits scope summary. |
| internal/api/rbac_coverage_test.go | Update RBAC coverage tests for host-aware checkAccess signature. |
| internal/api/project_handlers.go | Enforce projects host scope on deploy paths and host-permission checks. |
| internal/api/mcp_token_handlers.go | MCP token create returns/stores host scope; validate/clean host ids. |
| internal/api/host_scope_pentest_test.go | New pentests for REST host scoping, middleware enforcement, WS host check, project host checks, and audit host column. |
| internal/api/auth_handlers.go | /me/access now returns role hostIds and effective grant “where” metadata. |
| internal/api/alert_email_test.go | Fix my-access pentest to avoid false positives after adding hosts key. |
| internal/api/access_middleware.go | Centralize host authorization in permissions middleware; add hostParam helper. |
| docs/users.md | Document role host scoping semantics + known aggregate-read limitation. |
| docs/mcp.md | Document token host narrowing and live permission re-checks. |
| docs/audit.md | Document audit host column and its meaning. |
| design/rbac-roles-and-host-scoping.md | Update design note: phase 2 shipped + enforcement point explanation. |
| CHANGELOG.md | Add user-facing entry describing per-host RBAC scoping and limitations. |
Review details
Comments suppressed due to low confidence (1)
internal/api/role_handlers.go:105
- Same as create: update should clean/validate hostIds before persisting and auditing. Otherwise a payload containing only invalid ids can silently widen the role to "every host" while the audit log suggests it was scoped.
var b roleBody
if err := decodeJSON(r, &b); err != nil {
writeErr(w, http.StatusBadRequest, "invalid body")
return
}
- Files reviewed: 42/45 changed files
- Comments generated: 5
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+377
to
+378
| const toggleHost = (id: number) => | ||
| setHostIds(hostIds.includes(id) ? hostIds.filter((h) => h !== id) : [...hostIds, id]); |
Comment on lines
+219
to
+225
| func hostParam(r *http.Request) (int64, error) { | ||
| q := r.URL.Query().Get("host") | ||
| if q == "" { | ||
| return 0, nil | ||
| } | ||
| return strconv.ParseInt(q, 10, 64) | ||
| } |
Comment on lines
+113
to
116
| // RBAC gate: a user may only stream channels whose section they can access, | ||
| // and only on hosts their grant reaches. | ||
| if c.allow != nil && !c.allow(msg.Channel, msg.HostID) { | ||
| c.write(parent, serverMsg{Type: "error", SubID: msg.SubID, Message: "access to this section is not permitted"}) |
Comment on lines
+260
to
264
| func (h *handler) authorizeExtra(ctx context.Context, re *mcpsdk.RequestExtra, section string, write bool, hostID int64) (*principal, error) { | ||
| p := principalFromExtra(re) | ||
| if p == nil { | ||
| return nil, errors.New("unauthenticated") | ||
| } |
Comment on lines
74
to
78
| func (s *Server) handleCreateRole(w http.ResponseWriter, r *http.Request) { | ||
| var b roleBody | ||
| if err := decodeJSON(r, &b); err != nil { | ||
| writeErr(w, http.StatusBadRequest, "invalid body") | ||
| return |
10 tasks
malickyeu
added a commit
that referenced
this pull request
Jul 30, 2026
Nine comments across #90-#93. Two were real defects, the rest were consistency and clarity. An explicit role host scope that sanitised to nothing became UNSCOPED rather than being refused: hostIds:[0] cleaned to an empty list, which means every host, so a request to narrow a role produced an unrestricted one and the audit line described a scope that wasn't there. Now a 400, matching the rule already applied to MCP token section scopes. A non-positive ?host= is the local daemon to the Docker layer, but the new host-scope check took it literally: ?host=-1 was served locally while being authorised and audited as host -1, refusing a scoped user something they are allowed. Normalised at all three entry points — REST, the WS subscribe frame and MCP tool args. Also: DeleteRole's LDAP-fallback guard was skipped when GetLDAP errored instead of failing closed; ExistingRoleIDs loaded each role's sections just to test existence, making an LDAP login N+1; the role handlers audited the raw hostIds rather than the stored ones; the profile page spun forever when loading permissions failed, because "loading" and "failed" were the same state; the Access badge hard-coded 13 sections; toggleHost derived from the render's state instead of the previous one; RoleIDs serialised as null for pre-roles configs; and the TOTPPending comment claimed a discard that never happens (the comment was wrong, not the code — a stale pending secret is unreadable by anything except the confirm path, and clearing it eagerly would add a cancel path that can itself fail). 2 tests, 1 pentest, both verified to fail on the reported behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A role can be limited to specific Docker hosts, so "may restart containers" can
mean "on staging, not production". This is phase 2 of
design/rbac-roles-and-host-scoping.md.
This is new authorization, not a tightened check. Until now a non-admin holding
a section could act on any daemon by passing
?host=N— including hosts theycouldn't see on the Hosts page, which is gated by the separate
hostssection.Type of change
Checklist
go test -short ./...andgo vet ./...passgofmtgate is clean (gofmt -l $(git ls-files '*.go')after staging)cd web && npx tsc --noEmit)web/distdocs/and added aCHANGELOG.mdentry for user-facing changesNotes for reviewers
The enforcement point moved, and that's the main design decision here. The note
sketched
resolveHostID(60 call sites) as the chokepoint. I put the check in thepermissionsmiddleware instead, which every host-targeting route already passesthrough. That covers all 60 sites without editing them — and, more to the point,
covers the route someone adds next year.
resolveHostIDstays a plain parse and nowcarries a comment saying so, so nobody reintroduces a per-handler check expecting it
to be the gate.
Two places name a host outside the URL, and both needed explicit calls:
hostId. The hub'sallowcallback now receives it, so streaming a container's logs from an out-of-scope
host is refused where no HTTP request exists to block.
host 0.
requireHostAccess(REST) andauthorizeProjectHost(MCP) close that —without them a role scoped to staging could deploy a project pointing at prod.
MCP got the full treatment, per D7: every tool passes its
host_idto the gate,tokens can be narrowed to a host subset, and
deploy_projectlooks up the project'shost. A token minted before its owner's scope shrank still can't outrun the live
user check — there's a pentest named after that case.
Nothing changes on upgrade. An empty host list means every host, a section
granted directly on an account stays unscoped, and the local daemon is always in
scope (making it scopeable would let a single-host install lock itself out).
TestPentestHostScope_UnscopedAccessIsUnchangedpins that in both directions.Tests
Invariants 1, 2, 3, 5, 6, 7 of §5 are pentests. Invariant 4 is deliberately
not asserted — aggregate reads are phase 3, and a passing test would misrepresent
the code. The limitation is written into
docs/users.mdand the changelog in theagreed terms instead:
I checked the tests fail on the wrong behaviour. Dropping the host check in
checkAccessfails six; passing0instead of the parsed host fails the twomiddleware tests; removing MCP token host narrowing fails two; removing the
project-host lookup fails one.
One existing test needed a real fix, not a rubber stamp.
TestPentestMyAccess_OnlyOwnDataasserted viastrings.Contains(body, "hosts").The access payload now legitimately contains a
hostskey, so that would havefailed for the wrong reason. It now checks the leaked role name by substring and
the section structurally — same property, precisely stated.
Verified end to end over HTTP
A user holding one role scoped to
staging:/api/containers?host=prod/api/containers?host=staging/api/containersand?host=0An admin gets 502 on every host and never a 403. The profile page's new Where
column reads
allHosts: false, hosts: [2]for that user.UI
The role editor gets a host picker that states plainly which mode you're in
("applies to EVERY Docker host" vs "only the selected hosts"), role cards flag a
limited role, and the profile access table gained a Where column so it can't
imply reach the account hasn't got.