Skip to content

fix(auth): grant org namespace only to org Owners, not all members#1383

Open
tadasant wants to merge 4 commits into
mainfrom
fix/org-namespace-admin-only
Open

fix(auth): grant org namespace only to org Owners, not all members#1383
tadasant wants to merge 4 commits into
mainfrom
fix/org-namespace-admin-only

Conversation

@tadasant

@tadasant tadasant commented Jun 21, 2026

Copy link
Copy Markdown
Member

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 from GET /users/{username}/orgs, which:

  1. returns only public org memberships, and
  2. carries no role — an org Owner and a brand-new member look identical.

So membership was silently treated as publishing authority. Anyone who is (or whose public membership lists them as) a member of acme could publish — and, because publish is a pure namespace-glob with no per-server ownership check, overwrite — any server under io.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 is admin (org Owner).

  • Personal namespaces are unchanged — you always get io.github.<your-username>/*.
  • Org namespaces now require Owner. GitHub has no org-level "maintainer" role; org membership is either admin (Owner) or member. We grant only on admin.
  • read:org is the only scope needed. A minimal token without it still authenticates and still publishes to your personal namespace; a genuine missing-scope 403 from 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. A 403/429 that 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-powerless read:org token.

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.mdx documents how to contain that blast radius:

  • store the token as a GitHub Actions Environment secret (the PAT example uses environment:),
  • restrict that environment to the default branch / release tags and require PR reviews on it, so only reviewed, maintainer-approved code can reach the token,
  • optionally add a required environment reviewer for explicit per-publish approval,
  • and avoid the two footguns (pull_request_target checkout, 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:

  • Approach A — GitHub App as the authority layer. Strong model, but its natural unit of authority is a repo, so it needs a repo→server mapping to authorize a namespace. The safe, flexible version of that mapping is registry-managed state — which collapses into Approach B. It's also GitHub-only.
  • Approach B — a registry-managed grants table (OIDC bootstrap, or device-flow admin check). The most capable and provider-agnostic option, and probably where v1 wants to go. But it introduces exactly the "registry manages an authorization database" layer we wanted to avoid taking on right now: revocation, sync, audit, migration.
  • Approach C — scoped bearer tokens (npm-style). Good ergonomics, but again implies issuance/storage the registry would own.

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:

  • Owners are always in the loop. Only org Owners can publish org servers; there's no delegated "this team can publish this server" — that needs Approach B's state. Reduced flexibility in exchange for zero new infrastructure.
  • Blast radius within an org is unchanged. An Owner (or Owner-scoped CI token) can still overwrite any server under the org namespace, because there's still no per-server publisher ownership. That hijack-an-existing-server problem is real but orthogonal to this fix and belongs in its own change (record publisher identity per server; enforce it on update) — a follow-up to take alongside the OIDC hardening below.
  • OIDC is untouched and remains coarser. The github-oidc path still grants the whole io.github.<owner>/* namespace to any workflow with id-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:

  • member-role org is not granted; only admin orgs are
  • a missing-scope 403 (no read:org) still yields the personal namespace and no error
  • an admin org on a later page of memberships is still granted (pagination correctness)
  • a 5xx from the memberships call fails closed (no token issued) rather than degrading to personal-only
  • a rate-limit 403 (X-RateLimit-Remaining: 0 / Retry-After) fails closed — not mistaken for a missing-scope 403
  • existing org/name-validation tests updated to the new endpoint + role shape

gofmt/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's ExchangeToken and decoding the permissions claim 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 are active, verified via the same endpoint the fix uses (GET /user/memberships/orgs/{org}):

Token (role) io.github.Tadasant-Test-Org/* granted? Other patterns granted
Owner (admin) ✅ yes personal io.github.<owner>/*
member ❌ no personal io.github.<member>/* + the orgs that account actually Owns

The member case is the meaningful one: that membership is active and returned by GET /user/memberships/orgs, and is excluded purely because its role is member, not admin. Under the previous GET /users/{username}/orgs behavior (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.

tadasant and others added 2 commits June 21, 2026 23:05
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>
@BobDickinson

Copy link
Copy Markdown
Contributor

I reviewed the issue and @pree-dew's doc and the PR here and I agree with this approach. I'd still like to have @rado review the code.

@pree-dew

Copy link
Copy Markdown
Contributor

From an implementation point of view PR looks good but few points that I want to raise as small concerns here:

  1. It pushes orgs toward over-privileging people because only owner can now publish. Org Owner is a role for billing/settings/member management, not "should publish this package." so if they want to handover this responsibility to some maintainer they have to make them owner which means providing more privilege on the repo.
  2. Since it doesn't change OIDC path, it still grants org-wide publish to anyone with write access to any repo in the org.
  3. We have to make N+1(N => number of orgs user belongs to) call in every validation of JWT.
  4. If an Owner puts their token in a CI secret without the recommended branch/environment protections, every repo collaborator can effectively use it — recreating the "any member can publish" problem.

@tadasant

tadasant commented Jun 23, 2026

Copy link
Copy Markdown
Member Author

Since it doesn't change OIDC path, it still grants org-wide publish to anyone with write access to any repo in the org.

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 ?

It pushes orgs toward over-privileging people because only owner can now publish.
If an Owner puts their token in a CI secret without the recommended branch/environment protections, every repo collaborator can effectively use it — recreating the "any member can publish" problem.

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.

@rdimitrov

Copy link
Copy Markdown
Member

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:

  • I think we shouldn't drop OIDC as it would break a lot of users. OIDC is the recommended CI flow and what most people actually publish with so removing it would break the majority of publishers. Asking users to go via PAT path isn't an easy/better replacement. Also it's usually preferable to use OIDC rather than a PAT (can leak, expire, etc) so in general I'd rather improve on OIDC than delete it.
  • An option I think we haven't captured (sorry if we did) but the OIDC token is signed by GitHub and already carries a bunch of metadata including the specific repository that's publishing (not just the org). Today we throw that away and grant the whole io.github.<org>/*. Instead we could record which repo first published a server and require later updates to that server to come from the same repo (leveraging the immutable repository_id so renames don't break it). That turns "any repo in the org can publish/overwrite anything" into "only the owning repo can update its own server." Monorepos still work (one repo can own many servers).

Overall I think we can land this for PAT, keep OIDC and if you agree track per-server ownership via the OIDC repository_id as the real follow-up 👍

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>
@tadasant tadasant marked this pull request as ready for review June 26, 2026 22:07
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.

4 participants