Skip to content

refactor(server): make every route state the authority it demands - #7037

Open
otavio wants to merge 2 commits into
refactor/list-query-contractfrom
refactor/route-authority-claims
Open

refactor(server): make every route state the authority it demands#7037
otavio wants to merge 2 commits into
refactor/list-query-contractfrom
refactor/route-authority-claims

Conversation

@otavio

@otavio otavio commented Sep 3, 2026

Copy link
Copy Markdown
Member

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 Unbounded and Anonymous each take a required reason.

It also fixes a reachability bug the audit turned up: POST /api/auth/device and POST /api/auth/user answered 401 to the clients they exist for.

Why

CONTEXT.md already 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. RequiresAny declares and installs its guard in one act, so the claim stays evidence rather than description; it exists because DeleteSSHIdentity's rule is two permissions, which Requires cannot spell. PermissionInHandler(reason) and NoPermission(reason) install nothing and record why.
  • Declaration: one enumerated Authority plus a Permissions slice, replacing Permission/RequiresPermission. The zero value is a route that stated nothing — the case the invariant exists to see — and one field cannot drift against another.
  • route table: unstatedAuthority refuses a route that claims nothing, names an empty permission set, or leaves a reason blank. anonymityDisagreements refuses 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.Manage still 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:

  1. The anonymity check is not the one the issue proposed. "A route whose authority claim says it expects no credential" is not derivable — NoPermission does not mean that (GET /api/devices demands 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.
  2. Two classifications the issue drafted were wrong. 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. NoPermission would have been a false claim, which is the failure this change exists to prevent.
  3. Authority enum 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 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.

Each new predicate was verified against the real route table by deleting a claim, not only against its known-bad fixture — unstatedAuthority listed all 34 routes on first run, and anonymityDisagreements fired on POST /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 NoPermission and PermissionInHandler reason is a claim about what the handler and service actually do, and a wrong one is worse than none. And cloud/'s 32 /admin/api routes 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-lint and go mod tidy all clean.

@otavio
otavio requested a review from a team as a code owner September 3, 2026 22:58
@otavio
otavio requested a review from a team as a code owner September 3, 2026 23:55
@otavio otavio added this to the 0.27.1 milestone Sep 4, 2026
@otavio
otavio requested a review from a team as a code owner September 5, 2026 19:14
@otavio
otavio force-pushed the refactor/route-authority-claims branch from 783595c to c41560d Compare September 5, 2026 19:14
@otavio
otavio force-pushed the refactor/route-authority-claims branch from c41560d to 783595c Compare September 5, 2026 19:16
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
otavio force-pushed the refactor/route-authority-claims branch from 783595c to 20efc72 Compare September 5, 2026 19:21
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.

refactor(server): every route states the authority it demands, or why it demands none

1 participant