Repository navigation
fix(security): re-derive JWT admin role and scope agent tokens per owner - #1183
Merged
Merged
Conversation
… scope agent-token names per tenant Two server-edition auth defects. #1169 — the bearer-JWT path trusted the frozen `role` claim while the session path re-derived the role from `admin_emails` on every request. Removing someone from `admin_emails` therefore did not demote them: they kept every AdminOnly route and CanRevealSecrets() until their JWT expired — and the bug did not even self-heal at expiry, because POST /api/v1/auth/token mints a fresh JWT from ac.Role, so an active holder could renew admin indefinitely. authenticateFromBearer now calls buildAuthContext(user) instead of branching on claims.Role. Identity parity is exact: GetUser is keyed by User.ID (so user.ID == claims.Subject) and both mint sites pass user.ID/user.Email/ user.DisplayName/user.Provider — the same four values buildAuthContext reads, only fresher. Promotion works symmetrically, matching the session path; a demote-only AND of claim and config would strand promoted users and is pinned against by a test. Note this is not hot-reloadable: the middleware captures the config pointer at wiring time, so an admin_emails edit lands at the next process restart on BOTH paths, as it always did. #1168 — (a) agent-token names lived in one global index keyed by the bare name, so two tenants could not both have a token called `ci`, and revoke/delete/ regenerate answered 403 "not another user's token" versus 404 for an absent one, which is a name-existence oracle over other tenants. Storage now resolves by (owner, name) with a scan of the authoritative agent_tokens bucket, which carries both fields; the legacy agent_token_names index is maintained only for ownerless (personal-edition) tokens, so the personal edition is byte-identical on disk and NO MIGRATION is needed. Handlers lost their 403 branches entirely — "not yours" and "not there" are now the same nil and the same 404 — and the create path no longer echoes the storage conflict, which leaked the same fact. UpdateAgentTokenLastUsed on the auth hot path moved to a by-hash form, since a bare name is now ambiguous. (b) AgentToken.AuthContext() dropped UserID, so an agent-token request carried no tenant identity at all despite the constructor's promise that no auth path can silently drop a field. The field is added, and the surfaces that gated only on a non-empty UserID (getUserID, getMe, generateToken) now require the user TIER — without that, a scoped read-only agent token could mint a full user session JWT for its owner. principalFromContext also qualifies the agent principal with the owner, so two tenants' `ci` tokens no longer collapse into one work session in the audit trail. Deliberately NOT done: the one-time index sweep an earlier draft proposed. Its premise was false — every by-name path on current main re-checks ownership after the lookup, so a stale entry can only deny, never mutate the wrong tenant's token — and sweeping would strand pre-upgrade tokens on a rollback.
…wner may see POST /api/v1/user/tokens persisted the requested `allowed_servers` verbatim — no validation of any kind, where the personal-edition twin (internal/httpapi/tokens.go) at least checks the names are real. AllowedServers is the sole input to auth.AuthContext.CanAccessServer, so a regular tenant could mint themselves a token scoped to ["*"], or naming servers they have never been shown, and walk straight through the REST enumeration scope filter into the admin's whole inventory. NewUserHandlers is handed deps.Config.Servers — the entire admin configuration, not just the shared subset — so the leak covers servers no tenant is ever meant to know exist. The scope is now narrowed to the per-user door's own entitlement predicate: the caller's personal servers plus the admin servers flagged `shared`, reusing what listServers renders rather than defining entitlement a second time. - An unentitled name is REJECTED (400), not silently dropped: a token that quietly sees less than asked is worse than a refusal. The message is identical for another tenant's server, the admin's unshared server and a server that exists nowhere — distinguishing them would re-open, on server names, the existence oracle this branch just closed on token names. - "*" is the only wildcard the enforcement layer honours, so there are no globs to expand. It stays literal for an admin, who does administer every server; for a tenant it is materialised into their entitled set at mint time (a snapshot, echoed back in the response) and refused when that set is empty. - An omitted allowed_servers is left empty, which already denies every server at the agent tier. The personal edition's "empty means [*]" default is not copied: there the only caller is the operator. Tests follow the branch's oracle discipline — a positive control minting an entitled scope first on the same router, an independent entitlement proof through GET /api/v1/user/servers, pinned 201/400 statuses rather than parity, and storage-layer confirmation that a rejected request minted nothing.
…kens pooling onto their owner Cross-model review findings on the #1168/#1169 branch. H1 — the allowed_servers entitlement check was bypassable by a name collision. createServer only refused a personal-server name colliding with a server flagged Shared, so a tenant could create a personal server named exactly like one of the admin's PRIVATE upstreams; entitledServerNames then added that name on the strength of the personal server, and the minted token named the admin's server as far as CanAccessServer is concerned — it compares bare strings and has no notion of an owner. The same collision is a routing hazard independent of tokens: Router.GetServerForUser matches the whole configuration before the caller's workspace, so the name resolves to the admin's config and credentials. The refusal now covers ANY server in the admin configuration, with a message identical for a shared and a private collision — distinguishing them would name the admin's private inventory to a tenant who cannot list it, reopening the existence oracle this branch closed on token names, one shape over. A refusal necessarily signals that a name is unavailable; it signals nothing else. entitledServerNames independently drops a collision for a non-admin, so a row written before the refusal existed, or an admin who later adds a colliding name, cannot resurrect the escalation. H2 — regression introduced by this branch. Router.BrokeredConnectionKey resolved the FR-018 pooling key from ac.GetUserID() with no tier check. That was harmless while only user sessions carried a UserID; #1168 made an agent token carry its OWNER's, so an agent-token request keyed identically to the owner's session and pooled onto the owner's brokered connection — carrying the owner's injected IdP credential. Only a user tier now keys as its bare id; every other tier is namespaced, which keeps two owners' agent tokens apart without ever colliding with a session. Sweep of the other GetUserID()/AuthContext.UserID readers, since carrying a new field through a shared constructor changes every reader: - multiuser/activity.go GetUserActivity gated on `UserID != ""`, which now admits an agent token as its owner's session. Hardened to IsUser(); the server-edition middleware already refuses agent tokens on that route, so this is defence in depth, but one middleware branch is not a boundary. EnrichRecord deliberately stays UserID-based — attribution is what the field is for. - router.go GetUserServers/GetServerForUser, api/user_handlers.go getUserID, api/auth_endpoints.go, server/observability_edition_server.go: already gated on IsUser(). No change. - server/workspace.go principalFromContext, mcp.go, preflight.go, mcp_describe_check.go: attribution only, and workspace.go already namespaces the agent tier explicitly. No change. H3 — findAgentTokenHashLocked walks the whole bucket, so returning an error on the first row that failed json.Unmarshal turned create/revoke/delete/regenerate into a 500 for every tenant. Bad rows are now skipped and logged: one corrupt record degrades to "that token is unresolvable". H4 — revoke/delete/regenerate dropped their preflight read. It was a TOCTOU window (a concurrent delete-then-recreate had the in-flight regenerate land on the replacement) and its fall-through 500 interpolated the storage sentinel into the response body. The mutator's own error is classified instead: ErrAgentTokenNotFound -> 404 with the same body as an absent token. H5 — CreateAgentToken blindly Put the legacy owner-blind name index, overwriting whatever the name pointed at, including the pre-upgrade server-edition entry the bucket comment promises to preserve for a rollback. The slot is now claimed only when free or dangling, matching what delete and regenerate already did. H6 — a token's server scope was decided once at mint time and never re-checked, so un-sharing a server does not revoke live grants. Regenerate now re-runs the entitlement predicate and persists the narrowed list (narrowing only, never a refusal: refusing to rotate would leave the over-broad grant in place and working). Documented, including what to do about tokens minted with a literal "*" before this branch. H7 — the token cap answered 409 on the personal surface and 503 here. Now 409 on both: one storage condition, one status, and the cap is a permanent conflict rather than a transient outage worth retrying. H8 — the role-freshness docs put "promotes them on their next request, without re-login" two bullets above "not hot-reloadable", so an operator could read the first and believe a DEMOTION is immediate. The caveat now leads. Every fix carries a test proven to fail without it by permissive neutering, and follows the branch's oracle discipline: positive controls on the same router, pinned statuses, storage-level confirmation of the end state, and never "the body does not contain the name".
…disabled owner's tokens Seven findings from the second cross-model review. Q1 (blocking): UserHandlers snapshotted deps.Config.Servers at construction, so collidesWithAdminConfig and entitledServerNames were blind to any admin server added after process start — a tenant could pre-create a personal server named like an admin server that only appears later and walk through the allowed_servers control exactly as before the fix. NewUserHandlers now takes an AdminServersProvider read on every check, wired through a new Dependencies.ConfigProvider over the runtime's atomic config snapshot. The configuration is read once per call and threaded down, so one decision is never split across two versions of a hot-reloadable file. Q2: a disabled user's agent tokens kept authenticating. storage.Manager gains an owner gate consulted by ValidateAgentToken for every owned token, installed in setup.go over the user store and failing closed; ownerless personal-edition tokens are never gated. disableUser additionally revokes that user's tokens, so re-enabling the account cannot resurrect a stolen credential. Q3: the token-cap 409 used the personal edition's wording, which reads as the caller's own quota. auth.MaxTokens is deployment-wide, so the body now says the limit is shared and points at an administrator (per-owner quota is #1177). Q4: createServer answered a name held by the admin and one held by the caller with different bodies, letting a tenant enumerate a private inventory. Both are now one answer. Q5: ListAgentTokens aborted on the first unparseable record, turning one corrupt row into a 500 for every tenant; it now skips and logs, as the by-name resolver already did. Q6: the narrowScope narrow-only contract was a doc comment. Storage now intersects the hook's return with the token's stored AllowedServers (a stored "*" still materialises into a concrete list). Q7: multiuser.Router has no production caller, so its guards are latent. Comments that justified the collision refusal by a routing hazard now say the escalation is the token-scope one, and the router documents that nothing may rest on it until it is wired.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 33618632999 --repo smart-mcp-proxy/mcpproxy-go
|
…rst, stop rotation resurrecting burned tokens Closes the three MAJORs from the final cross-model review, plus two smaller contradictions with the branch's own claims. N1 — the #1169 fix read a BOOT-TIME config. The middleware held the *config.ServerEditionConfig captured at wiring time, so removing someone from admin_emails took effect only at the next process restart. admin_emails IS hot-reloadable (LoadFromFile unmarshals server_edition whole, it is not restart-pinned, and the watcher republishes the snapshot), so the role is now derived through a ServerEditionConfigProvider over Dependencies.ConfigProvider — the same provider the admin-servers check already uses, not a second mechanism. The docs bullet is rewritten to match, including the two limits it does not cover. N2 — data race on the published config snapshot. handleAddUpstream appended to runtime.Config().Servers in place; this branch installs a per-request reader over that slice. Both append sites now go through one copy-on-write helper. N3 — the agent-token owner gate was installed after four fallible steps, and SetupAll's error is only logged, so the server could come up with a disabled user's tokens ungated. It is now the first thing setupMultiUserOAuth does, so a failure leaves it in force and failing closed. N4 — RegenerateAgentTokenForOwner cleared Revoked, letting an owner un-revoke a token disableUser had deliberately burned. Rotation now refuses a revoked token (ErrAgentTokenRevoked, 409 on both editions' surfaces). N5 — regenerate repointed the legacy name index unconditionally for ownerless tokens, stomping the pre-upgrade entry the no-migration rollback rationale promises to preserve. It now applies create's claim rule.
Deploying mcpproxy-docs with
|
| Latest commit: |
bbfe39d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d5be4f03.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-sec-1168-1169-server.mcpproxy-docs.pages.dev |
Dumbris
added a commit
that referenced
this pull request
Sep 2, 2026
…sory (#1189) The server edition has no meaningful install base, so the section was noise for every reader of the notice. Cutting it keeps the advisory focused on what a personal-edition user has to act on, which is the whole reason for pinning a hand-written notice above the generated notes in the first place. The fixes themselves still ship in this release - #1168 and #1169 are merged in #1183 and remain linked from the changelog and their issues, so a server-edition operator can still find them.
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
2 participants
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.
Closes #1168, #1169.
Server-edition auth. All changes are behind
//go:build server; the personal edition is behaviourally unchanged, and there is no storage migration — existing on-disk tokens resolve as before.#1169 — admin demotion did not take effect until token expiry
The session path re-derived the admin role from
admin_emailson every request; the bearer-JWT path trustedclaims.Roleverbatim. Removing someone fromadmin_emailsleft them with full admin — everyAdminOnly()route andCanRevealSecrets()— until their JWT expired, by default 24 hours. The JWT path now re-derives the role the way the session path does, with field parity proven rather than assumed.#1168 — token names were a global namespace, and
AuthContext()droppedUserIDTwo tenants could not both have a token named
ci, and the revoke path's 403-vs-404 split was a name-probing oracle. Separately,AuthContext()copied every field exceptUserID, so an agent-token request carried no tenant identity — under a constructor whose own doc comment says it exists "so no auth path can silently drop a field".Name resolution is now owner-scoped, and unowned and absent are indistinguishable on revoke, delete and regenerate. The TOCTOU preflight is gone in favour of classifying the mutator's own error, which closes the window and a leaked sentinel string together.
A proposed one-time migration sweep was dropped: its justification — that a rolled-back binary could resolve
cito the wrong tenant — is false, since every user-facing by-name path already re-checks ownership after the lookup and denies. The sweep bought nothing and its rollback behaviour would have stranded existing tokens.Found while fixing these
UserIDintroduced a regression.multiuser.Router.BrokeredConnectionKeykeyed the connection pool offGetUserID()with noIsUser()guard, so an agent token would pool onto its owner's brokered upstream connection — carrying the owner's injected IdP credential. Previously it keyed as anonymous. Guarded, plus a sweep of every otherGetUserID()consumer. (multiuser.Routerhas no production caller yet; the guard is correct for when it is wired, and the comments now say so instead of resting on it.)allowed_serverswas accepted verbatim onPOST /api/v1/user/tokens, with no intersection against the tenant's entitled set — so a user could mint["*"]and walk straight through the REST server enumeration ignores agent-token scope: /api/v1/servers and /api/v1/config list every server regardless of allowed_servers #1166 scope filter. Now intersected, read live from the config rather than a boot-time snapshot, which was itself the first fix's blind spot.Follow-ups filed, deliberately not bundled
auth.MaxTokensis a deployment-wide cap, so one tenant can exhaust it for everyone. The misleading 409 body is fixed here; the per-owner quota is not.Review
Two
opencode(gpt-5.6-sol) rounds, both BLOCKING; 14 genuine findings closed, 1 false positive rejected with evidence. 27 tests. The oracle tests carry a positive control on the same router and pin exact status values, rather than asserting bare parity — a sibling test in this package passes today only because a nil store makes both answers 500.