fix: act on the Copilot review (role scope widening, host id normalisation) - #96
Merged
Conversation
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
Acts on the Copilot review across today's merged PRs (#90–#93). Nine comments;
two were real defects, the rest consistency and clarity.
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 changesThe two that mattered
A role scoped to only-invalid hosts became unscoped instead of being refused.
hostIds: [0]was sanitised down to an empty list — which means every host. So arequest to narrow a role quietly produced an unrestricted one, and the audit
line described a scope that wasn't there. Now a 400, matching the rule this codebase
already applies to MCP token section scopes: an explicit narrowing that filters to
nothing is an error, never a fall-through to "everything".
A non-positive
?host=is the local daemon everywhere else.docker.Manager.Clienthas always resolved
hostID <= 0to the local daemon, but the new host-scope checktook it literally:
?host=-1was served locally while being authorised and auditedas host −1, so a scoped user was refused something they are in fact allowed.
Normalised at all three entry points — REST, the WebSocket subscribe frame, and MCP
tool arguments.
Both are mutation-tested: restoring either behaviour fails its own test.
The rest
DeleteRole's LDAP-fallback guard was skipped whenGetLDAPerrored ratherthan failing closed — the one situation the guard exists for.
ExistingRoleIDsloaded each role's sections just to test existence, making anLDAP login N+1. Now an existence probe.
hostIdsrather than the stored ones.and "failed" were the same state. It now says what went wrong and offers a retry.
loading, rather than baking in a count that will drift.
toggleHostderived from the render's state instead of the previous one.RoleIDsserialised asnullfor configs written before roles existed —omitemptykeeps them genuinely absent.One I disagreed with, and what I did instead
Copilot flagged that the
TOTPPendingcomment promises secrets are "discardedotherwise" while nothing clears them on cancel. The comment was wrong, not the
code. A stale pending secret is read by exactly one function, which only ever
promotes it after validating a code from the new device, so it grants nothing;
clearing it eagerly would add a cancel path that can itself fail, on a flow whose
entire point is that failure changes nothing. The comment now says that, and warns
against reading its presence as "a pairing is in progress".