refactor(server): make every route state the authority it demands - #7037
Open
otavio wants to merge 2 commits into
Open
refactor(server): make every route state the authority it demands#7037otavio wants to merge 2 commits into
otavio wants to merge 2 commits into
Conversation
otavio
force-pushed
the
refactor/route-authority-claims
branch
from
September 5, 2026 19:14
783595c to
c41560d
Compare
otavio
force-pushed
the
refactor/route-authority-claims
branch
from
September 5, 2026 19:16
c41560d to
783595c
Compare
CONTEXT.md already claimed this rule, and two thirds of it was true: Unbounded and Anonymous each take a required reason, so breadth and anonymity cannot arrive by omission. Authority had no such option, so 34 of the community route table's registrations carried no permission and nothing asked why. A reader could not tell a route that genuinely demands none from one whose check merely moved somewhere the route table cannot see. Three options rather than one, because the 34 do not share a reason. RequiresAny declares and installs in one act, like Requires, so the claim stays evidence rather than description; it exists because DeleteSSHIdentity's rule is two permissions, which Requires cannot spell. The other two install nothing and record why: PermissionInHandler for a check needing request data no middleware can see, NoPermission for a route demanding none. Anonymous discharges the question on its own — a request carrying no actor has no role to check a permission against. Declaration carries one enumerated Authority plus a Permissions slice rather than a boolean per claim. The zero value is a route that stated nothing, which is the case the invariant exists to see, and one field cannot drift against another. POST /api/auth/device and POST /api/auth/user were the bug the audit turned up. The V2 spellings were mounted without their siblings' anonymity claim and added to no allowlist, so the authenticator refused the very clients they exist for. anonymityMismatches could not see it: it fires on a claim without an allowlist entry or the reverse, and a route carrying neither reads as consistent — which is what every authenticated route looks like. The check added for it is not the one the issue proposed. "A route whose authority claim says it expects no credential" is not derivable, because NoPermission does not mean that: GET /api/devices demands a credential and claims it. What distinguishes the bug is the handler, so the new predicate refuses two mounts of one handler that disagree on whether it needs a credential. That is narrower than the issue asked for: a first-of-its-kind anonymous route with a unique handler still reads as consistent. It catches the V2-spelling shape, which is the shape that bit us. Two of the issue's drafted classifications were wrong and are not followed. GET /api/ssh-approvals/:code and GET /api/devices/login-code/:code were assigned NoPermission on the grounds that the credential carries no role; both take a user token and both services resolve the namespace from the code and refuse a non-member, so they claim PermissionInHandler instead. NoPermission would have been a false claim, which is the failure this change exists to prevent. The reason on POST /api/auth/ssh is deliberately not the one the issue drafted either. AuthPublicKey does not check whether an offered key belongs to the namespace: it loads a private key by fingerprint and signs the caller's payload, reading no namespace at all. The handler's doc comment claimed the same wrong thing and is corrected here. No caller observes a permission change. RequiresAny(SSHIdentityAdd, SSHIdentityManage) is exactly the guard deleted from DeleteSSHIdentity's body, since no role holds Manage without Add. One ordering shifts: the 403 now precedes the bind, so an unpermitted caller sending a malformed body gets 403 where it got 400. cloud/'s 32 /admin/api routes are not yet held to this invariant. The vocabulary is built here so that deciding their policy is a separate question; tracked in shellhub-io/team#238. Fixes: #7036
Every billing permission is the owner's, and every billing route demands one — except GET /api/billing/customer, which demanded nothing and so admitted every member of the namespace. RoleAdministrator's own documentation says an administrator holds no billing permission; the customer read was the policy escaping a route rather than a policy choosing to. BillingGetCustomer is granted to the owner alone, beside BillingGetSubscription, which guards the sibling read of the same resource. Nothing in this repository requires it: the route is cloud's, and the constant is here because the role model is. BillingGetPaymentMethod, two lines below, is declared and granted to no role at all and required by no route in either repository. It is left as it is: naming it on the customer read would have refused every caller, and deleting it is not this change. Part of #7032.
otavio
force-pushed
the
refactor/route-authority-claims
branch
from
September 5, 2026 19:21
783595c to
20efc72
Compare
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.
What
Every route registration now states what it demands of the caller's role, and a route-table invariant refuses the omission. This closes the last of the three claims a route makes — breadth and anonymity already could not arrive silently, because
UnboundedandAnonymouseach take a required reason.It also fixes a reachability bug the audit turned up:
POST /api/auth/deviceandPOST /api/auth/useranswered 401 to the clients they exist for.Why
CONTEXT.mdalready asserted the rule. Authority had no option and no check, so 34 of the community route table's registrations carried no permission and nothing asked why — a reader could not tell a route that genuinely demands none from one whose check merely moved somewhere the route table cannot see.Closes #7036
Changes
gateway: three options, because the 34 do not share a reason.RequiresAnydeclares and installs its guard in one act, so the claim stays evidence rather than description; it exists becauseDeleteSSHIdentity's rule is two permissions, whichRequirescannot spell.PermissionInHandler(reason)andNoPermission(reason)install nothing and record why.Declaration: one enumeratedAuthorityplus aPermissionsslice, replacingPermission/RequiresPermission. The zero value is a route that stated nothing — the case the invariant exists to see — and one field cannot drift against another.unstatedAuthorityrefuses a route that claims nothing, names an empty permission set, or leaves a reason blank.anonymityDisagreementsrefuses two mounts of one handler that disagree on whether it needs a credential.DeleteSSHIdentity: the two-permission check left the handler body for the mounting line.req.Managestill derives the ownership half, which is a data decision rather than an admission one.POST /api/auth/ssh: the handler's doc comment claimed it checks whether an offered key belongs to the namespace. It loads a private key by fingerprint and signs the caller's payload, reading no namespace at all. Corrected alongside its route reason.Deviations from the issue
Three, all deliberate:
NoPermissiondoes not mean that (GET /api/devicesdemands a credential and claims it), so that predicate would have fired on ~20 correct routes. The handler-agreement rule is narrower than asked: a first-of-its-kind anonymous route with a unique handler still reads as consistent. It catches the V2-spelling shape, which is the shape that bit us.GET /api/ssh-approvals/:codeandGET /api/devices/login-code/:codewere assignedNoPermissionon the grounds that the credential carries no role. Both take a user token, and both services resolve the namespace from the code and refuse a non-member — so they claimPermissionInHandler.NoPermissionwould have been a false claim, which is the failure this change exists to prevent.Authorityenum over four booleans, which the issue explicitly left as an implementation choice.Testing
No caller observes a permission change.
RequiresAny(SSHIdentityAdd, SSHIdentityManage)is exactly the guard deleted from the handler body, since no role holdsManagewithoutAdd. One ordering shifts: the 403 now precedes the bind, so an unpermitted caller sending a malformed body gets 403 where it got 400.Each new predicate was verified against the real route table by deleting a claim, not only against its known-bad fixture —
unstatedAuthoritylisted all 34 routes on first run, andanonymityDisagreementsfired onPOST /api/auth/device. Worth re-running that mutation if you want the green run to mean something.Two things a reviewer should probe rather than take on trust: every
NoPermissionandPermissionInHandlerreason is a claim about what the handler and service actually do, and a wrong one is worse than none. Andcloud/'s 32/admin/apiroutes are not yet held to this invariant — the vocabulary is built here so their policy is a separate decision (shellhub-io/team#238).Note this is stacked on
refactor/list-query-contract, so CI runs zero checks. Locally: server suite,golangci-lintandgo mod tidyall clean.