fix(auth): grant org namespace only to org Owners, not all members#1383
Conversation
GitHub access-token exchange granted `io.github.<org>/*` publish
permission for *every* organization a user belonged to, derived from
`GET /users/{username}/orgs` — which returns only public memberships and
carries no role. Any member of an org (or anyone whose public membership
listed it) could publish, and silently overwrite, servers under the
org's namespace.
Switch to `GET /user/memberships/orgs`, which returns the caller's role
per org (and includes private memberships), and grant the org namespace
only when the role is `admin` (org Owner). Personal namespaces are
unaffected.
The memberships endpoint requires the `read:org` scope. A minimal token
without it authenticates fine and still publishes to the personal
namespace; the 403 from the memberships call is treated as "no admin
orgs" so we don't force users to over-scope a token. The token needs no
repository scopes — the registry never touches code.
Refs #982.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The token that authenticates to an org namespace can publish and overwrite any server under io.github.<org>/*, and by default it is reachable by every repository writer (not just org Owners) via branch or tag pushes. Document how to contain that blast radius: - store the token as an Environment secret (update the PAT example to use `environment:`) - restrict the environment to the default branch / release tags and protect that branch with required reviews - optionally require an environment reviewer for per-publish approval - avoid pull_request_target-with-checkout and self-hosted runners on public repos Also clarify that the PAT's read:org scope is what authorizes org publishing and that no repository scopes are needed. Refs #982. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two added subtests pushed TestGitHubHandler_ExchangeToken over the cyclop max (complexity 23 > 20). Move the member-role and missing-read:org cases into TestGitHubHandler_ExchangeToken_OrgRoles. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
From an implementation point of view PR looks good but few points that I want to raise as small concerns here:
|
This is a good point - maybe we should get rid of the OIDC path altogether? It feels like basically the same problem as the member-only check. What do you think @BobDickinson @rdimitrov ?
Valid concerns, but I think as long as we are transparent about these limitations and our guidance (i.e. use those protections, don't grant Owner just share a PAT), I think it's better than the status quo while we work out the bigger change of creating a delegation table system. |
|
I think I'm good with landing this as a short-term fix for the PAT path (gating org publishing to Owners is clearly better than the current state) 👍 Regarding the other items in the discussion:
Overall I think we can land this for PAT, keep OIDC and if you agree track per-server ownership via the OIDC |
Address review feedback on the org-Owner-only namespace fix: - Disambiguate the 403 from GET /user/memberships/orgs. A 403 with X-RateLimit-Remaining: 0 (or Retry-After present) is a throttle, not a missing read:org scope, so fail closed rather than silently degrading a legitimate Owner to personal-only permissions. - Correct the githubOrgRoleAdmin godoc: GitHub returns admin/member/ billing_manager; only admin carries publish authority. - Add tests: admin org on a later page is still granted (pagination), 5xx memberships error fails closed, and rate-limit 403 fails closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review findings on the org-namespace-admin-only change: - Don't leak internal error detail in the github-at 401. The handler passed the wrapped err (which can include the raw upstream GitHub response body from readErrorBody) into huma.Error401Unauthorized, and huma serializes extra err args into the response body (CWE-209) — the same leak fixed for GET /v0/servers in #1338. Log it server-side and return only a generic message. Regression test added (github_at_internal_test.go), mirroring #1338. - Fail closed on a SAML/SSO-enforcement 403 (X-GitHub-SSO header) instead of treating it as a missing-scope 403 and silently degrading a legitimate Owner to personal-only with no signal. - Re-check membership state=active client-side, not only via the query param, so a dropped/changed param can't grant a pending (not-yet-accepted) owner invitation. - Cap the org-memberships pagination loop (previously unbounded) and fail closed if the cap is hit with full pages, rather than returning a possibly-truncated grant. - In buildPermissions, skip an org whose name fails validation instead of returning nil for the whole set, so one unexpected org name can't strip the caller's personal namespace or their other valid org grants. - Correct the role comment: GET /user/memberships/orgs returns "admin"/"member", not "billing_manager". Adds tests for pending-state exclusion, billing_manager exclusion, and Retry-After / SSO 403 fail-closed handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- The workflow declares environment: mcp-registry-publish but Step 2 told users to add MCP_GITHUB_TOKEN as a repository secret, implying protection that isn't configured: an auto-created environment has no protection rules and a job with environment: still receives repo secrets. Clarify that the environment: key alone protects nothing and that org tokens should be stored as an Environment secret with the environment configured. - Drop the read:user scope from the PAT requirement; only read:org is needed (aligns with authentication.mdx and the actual handler behavior). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rdimitrov
left a comment
There was a problem hiding this comment.
Thanks for the fix! Merging! 🙏
…+ pagination-cap test) (#1435) Follow-up to #1383, surfaced while validating that change end-to-end against staging. ## Why #1383 hardened org-namespace publishing to require Owner (`admin`) role, read via `GET /user/memberships/orgs`. Validating on staging revealed a gap: the docs only mention the classic `read:org` scope, but a **fine-grained PAT** (GitHub's recommended token type) needs a *different* permission to expose org membership. Without it, GitHub returns no org memberships for the token, so the exchange silently grants only the personal namespace — with no hint why org publishing "doesn't work." This is **not** a code bug (the behavior is fail-closed and correct given GitHub's response); it's a documentation/UX gap. ## What - **Docs** (`authentication.mdx`, `github-actions.mdx`): spell out both token types — - Classic PAT → `read:org` scope - Fine-grained PAT → **Organization permissions → Members → Read-only** (the equivalent), noting a fine-grained PAT is bound to a single resource owner. - Also clarified `read:user` is not required. - **Test** (`github_at_test.go`): cover the `maxOrgMembershipPages` pagination cap — when every page is full, the exchange **fails closed** (no token) rather than returning a possibly-truncated org grant. This path was previously untested. - **Comment** (`github_at.go`): qualify the `X-GitHub-SSO` 403 check as best-effort — we have not confirmed the memberships *list* endpoint emits that header on SSO enforcement (it may just omit the org). The check is safe either way, but the comment no longer overstates it as a guarantee. ## Verification (live, against staging) Confirmed by exchanging real tokens for `rdimitrov` on `staging.registry.modelcontextprotocol.io`: - **Classic `read:org` PAT** across 16 real org memberships → the JWT granted exactly the 8 **admin** orgs + personal namespace, and **zero** of the 8 **member** orgs (incl. `stacklok`, `modelcontextprotocol`) — the core #1383 behavior, working. - **Fine-grained PAT**: without Members:read → personal namespace only (fail-closed); after adding Members:read → the org (Owner) namespace is granted. This is what the new docs describe. No runtime behavior change here beyond the added test; the docs and comment are the substance. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
The bug (scoped to
github-at)When you exchange a GitHub OAuth/PAT token for a registry JWT, the handler granted
io.github.<org>/*publish permission for every organization you belong to. It derived that list fromGET /users/{username}/orgs, which:So membership was silently treated as publishing authority. Anyone who is (or whose public membership lists them as) a member of
acmecould publish — and, because publish is a pure namespace-glob with no per-server ownership check, overwrite — any server underio.github.acme/*.The fix
Switch to
GET /user/memberships/orgs, which returns the authenticated caller's role per org (and includes private memberships), and grant the org namespace only when the role isadmin(org Owner).io.github.<your-username>/*.admin(Owner) ormember. We grant only onadmin.read:orgis the only scope needed. A minimal token without it still authenticates and still publishes to your personal namespace; a genuine missing-scope403from the memberships call is treated as "no admin orgs" rather than a hard failure, so we don't push anyone to over-scope a token. A403/429that is actually a rate limit (X-RateLimit-Remaining: 0/Retry-After) fails closed instead, so a throttle can't silently strip a real Owner's org grant. Critically, the token needs no repository scopes — the registry never reads or writes code, so an org Owner can hand CI a near-powerlessread:orgtoken.The authorization decision stays stateless and delegated to GitHub — no new storage, no new endpoints, no schema changes — consistent with design principle #2 (minimal operational burden).
Containing the CI token
The registry-side change makes the org Owner the only one who can mint org-namespace authority. But if that Owner drops the resulting PAT into a plain repo secret, every repository writer can reach it (via branch/tag pushes that run repo-controlled build/test code in the privileged job) — quietly re-widening publish access beyond Owners. External fork contributors still cannot reach it by default (no secrets on fork PRs, no branch push), barring misconfigurations like
pull_request_target-with-checkout or self-hosted runners on public repos.docs/modelcontextprotocol-io/github-actions.mdxdocuments how to contain that blast radius:environment:),pull_request_targetcheckout, self-hosted runners on public repos).This shrinks who holds the org credential; it does not change the fact that an authorized publish can still overwrite a different server in the same namespace until per-server publisher ownership exists (the follow-up noted below).
Why this shape, given the alternatives
Pree's writeup (Notion analysis doc) is the clearest framing of the problem and the option space, laying out three broad solutions:
The two-property test (does the registry control the decision? is it granted through an act requiring real authority?) is the right lens. This change satisfies it without new state: the registry makes the decision (it filters on role), and the authority is real (live GitHub Owner role, re-checked on every short-lived token exchange).
What this deliberately gives up, so reviewers can weigh it honestly:
github-oidcpath still grants the wholeio.github.<owner>/*namespace to any workflow withid-token: write, i.e. effectively anyone with write access to any repo in the org, and it excludes non-Actions CI (Jenkins/GitLab can't mint GitHub Actions OIDC). The admin-PAT path here is the higher-assurance option for org publishing; OIDC stays as a lower-assurance Actions convenience to be tightened separately.Revocation
The registry JWT is 5-minute, no-refresh (per #264/#273), so loss of Owner status propagates within minutes on the next token exchange, with no revocation list to maintain.
Tests
go test ./internal/api/handlers/v0/auth/...passes, including:403(noread:org) still yields the personal namespace and no error5xxfrom the memberships call fails closed (no token issued) rather than degrading to personal-only403(X-RateLimit-Remaining: 0/Retry-After) fails closed — not mistaken for a missing-scope403gofmt/go vet/golangci-lint(CI-pinned v2.11.4) clean; CI green on all checks.Live verification
Confirmed end-to-end against real
api.github.com(not just mocks) by exercising this branch'sExchangeTokenand decoding thepermissionsclaim of the returned JWT.A dedicated org, Tadasant Test Org (login
Tadasant-Test-Org), is Owned by one account and has a second account joined as a plain member. Both memberships areactive, verified via the same endpoint the fix uses (GET /user/memberships/orgs/{org}):io.github.Tadasant-Test-Org/*granted?admin)io.github.<owner>/*io.github.<member>/*+ the orgs that account actually OwnsThe member case is the meaningful one: that membership is active and returned by
GET /user/memberships/orgs, and is excluded purely because its role ismember, notadmin. Under the previousGET /users/{username}/orgsbehavior (membership = authority, no role), the same active membership would have granted the org namespace. The role filter is also selective rather than blanket-deny — the member account still receives the namespaces of the orgs where it genuinely holds Owner.