From 5ecbbda31f72c416a733f6caf41154adcf66f73d Mon Sep 17 00:00:00 2001 From: tadasant Date: Sun, 21 Jun 2026 23:05:47 +0000 Subject: [PATCH 1/4] fix(auth): grant org namespace only to org Owners, not all members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub access-token exchange granted `io.github./*` 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 --- .../authentication.mdx | 8 ++ internal/api/handlers/v0/auth/github_at.go | 87 +++++++++++-- .../api/handlers/v0/auth/github_at_test.go | 118 ++++++++++++++++-- 3 files changed, 191 insertions(+), 22 deletions(-) diff --git a/docs/modelcontextprotocol-io/authentication.mdx b/docs/modelcontextprotocol-io/authentication.mdx index 78bf23a1f..98131d5db 100644 --- a/docs/modelcontextprotocol-io/authentication.mdx +++ b/docs/modelcontextprotocol-io/authentication.mdx @@ -47,6 +47,14 @@ Successfully authenticated! ✓ Successfully logged in ``` +### Personal vs. organization namespaces + +GitHub authentication always grants your personal namespace, `io.github./*`. + +To publish under an **organization** namespace (`io.github./*`), you must be an **Owner** of that organization. Ordinary org membership is no longer sufficient: the registry checks your membership role and only grants the org namespace to admins. This prevents anyone who merely belongs to an org from publishing — or overwriting — servers under the org's name. + +If you authenticate with a Personal Access Token (for example in CI), the token must include the `read:org` scope for the registry to see your organization role. A token without `read:org` can still publish to your personal namespace, but org publishing will be silently unavailable. The token needs **no** repository scopes — the registry never reads or writes your code. + ## DNS Authentication DNS authentication is a domain-based authentication method that relies on a DNS TXT record. diff --git a/internal/api/handlers/v0/auth/github_at.go b/internal/api/handlers/v0/auth/github_at.go index af5e7931a..cb17e8964 100644 --- a/internal/api/handlers/v0/auth/github_at.go +++ b/internal/api/handlers/v0/auth/github_at.go @@ -74,8 +74,9 @@ func (h *GitHubHandler) ExchangeToken(ctx context.Context, githubToken string) ( return nil, fmt.Errorf("failed to get GitHub user: %w", err) } - // Get user's organizations - orgs, err := h.getGitHubUserOrgs(ctx, user.Login, githubToken) + // Get the organizations the user administers. Org namespaces are only granted + // to org Owners (membership role "admin"), not to ordinary members. + orgs, err := h.getGitHubAdminOrgs(ctx, githubToken) if err != nil { return nil, fmt.Errorf("failed to get GitHub organizations: %w", err) } @@ -133,8 +134,67 @@ func (h *GitHubHandler) getGitHubUser(ctx context.Context, token string) (*GitHu return &user, nil } -func (h *GitHubHandler) getGitHubUserOrgs(ctx context.Context, username string, token string) ([]GitHubUserOrOrg, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, h.baseURL+"/users/"+username+"/orgs", nil) +// githubOrgRoleAdmin is the membership role GitHub returns for an organization +// Owner. It is the only role we treat as carrying publish authority for the org +// namespace. (GitHub has no org-level "maintainer" role; org membership is either +// "admin" (Owner) or "member".) +const githubOrgRoleAdmin = "admin" + +// orgMembershipsPageSize is the page size we request when listing the user's org +// memberships. 100 is GitHub's maximum. +const orgMembershipsPageSize = 100 + +// githubOrgMembership is one entry from GET /user/memberships/orgs: the +// authenticated user's membership in a single organization, including their role. +type githubOrgMembership struct { + State string `json:"state"` + Role string `json:"role"` + Organization GitHubUserOrOrg `json:"organization"` +} + +// getGitHubAdminOrgs returns the organizations in which the authenticated user is +// an Owner (membership role "admin"). +// +// This deliberately uses GET /user/memberships/orgs rather than +// GET /users/{username}/orgs. The latter only returns *public* memberships and +// carries no role, so it cannot distinguish an Owner from an ordinary member — +// historically every org a user belonged to was granted publish access to its +// namespace regardless of the user's role. The memberships endpoint returns the +// caller's role per org (and includes private memberships), letting us grant the +// org namespace only to people who actually administer the org. +// +// The endpoint requires the read:org scope. A minimal token used only for +// personal-namespace publishing won't have it and will get a 403 here; that is +// treated as "no admin orgs" (see fetchOrgMembershipsPage) so personal publishing +// keeps working without asking users to over-scope their token. +func (h *GitHubHandler) getGitHubAdminOrgs(ctx context.Context, token string) ([]GitHubUserOrOrg, error) { + var adminOrgs []GitHubUserOrOrg + + for page := 1; ; page++ { + memberships, err := h.fetchOrgMembershipsPage(ctx, token, page) + if err != nil { + return nil, err + } + + for _, m := range memberships { + if m.Role == githubOrgRoleAdmin { + adminOrgs = append(adminOrgs, m.Organization) + } + } + + if len(memberships) < orgMembershipsPageSize { + break + } + } + + return adminOrgs, nil +} + +// fetchOrgMembershipsPage fetches a single page of the authenticated user's +// active organization memberships. +func (h *GitHubHandler) fetchOrgMembershipsPage(ctx context.Context, token string, page int) ([]githubOrgMembership, error) { + url := fmt.Sprintf("%s/user/memberships/orgs?state=active&per_page=%d&page=%d", h.baseURL, orgMembershipsPageSize, page) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } @@ -145,20 +205,27 @@ func (h *GitHubHandler) getGitHubUserOrgs(ctx context.Context, username string, resp, err := http.DefaultClient.Do(req) if err != nil { - return nil, fmt.Errorf("failed to get user organizations: %w", err) + return nil, fmt.Errorf("failed to get user organization memberships: %w", err) } defer resp.Body.Close() + // A token without the read:org scope can still authenticate (GET /user works) + // but cannot read org memberships, yielding a 403. Treat that as "no admin + // orgs" so personal-namespace publishing keeps working with a minimal token. + if resp.StatusCode == http.StatusForbidden { + return nil, nil + } + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("GitHub API error (status %d): %s", resp.StatusCode, readErrorBody(resp.Body)) } - var orgs []GitHubUserOrOrg - if err := json.NewDecoder(resp.Body).Decode(&orgs); err != nil { - return nil, fmt.Errorf("failed to decode organizations response: %w", err) + var memberships []githubOrgMembership + if err := json.NewDecoder(resp.Body).Decode(&memberships); err != nil { + return nil, fmt.Errorf("failed to decode organization memberships response: %w", err) } - return orgs, nil + return memberships, nil } // buildPermissions builds permissions based on GitHub user and their organizations @@ -181,7 +248,7 @@ func (h *GitHubHandler) buildPermissions(username string, orgs []GitHubUserOrOrg ResourcePattern: fmt.Sprintf("io.github.%s/*", username), }) - // Add permissions for each organization + // Add permissions for each organization the user administers (Owner role) for _, org := range orgs { permissions = append(permissions, auth.Permission{ Action: auth.PermissionActionPublish, diff --git a/internal/api/handlers/v0/auth/github_at_test.go b/internal/api/handlers/v0/auth/github_at_test.go index c0fa4ac97..1337a0da5 100644 --- a/internal/api/handlers/v0/auth/github_at_test.go +++ b/internal/api/handlers/v0/auth/github_at_test.go @@ -22,9 +22,29 @@ import ( const ( githubUserEndpoint = "/user" - githubOrgsEndpoint = "/users/testuser/orgs" + // githubOrgsEndpoint is the endpoint used to list the authenticated user's org + // memberships (with role). It replaces the old public-only /users/{user}/orgs. + githubOrgsEndpoint = "/user/memberships/orgs" ) +// orgMembership mirrors a single entry of GET /user/memberships/orgs for use in +// mock GitHub API responses. Role is "admin" (Owner) or "member". +type orgMembership struct { + State string `json:"state"` + Role string `json:"role"` + Organization v0auth.GitHubUserOrOrg `json:"organization"` +} + +// adminMemberships wraps orgs as active "admin"-role memberships, the shape the +// memberships endpoint returns for an org Owner. +func adminMemberships(orgs []v0auth.GitHubUserOrOrg) []orgMembership { + memberships := make([]orgMembership, 0, len(orgs)) + for _, org := range orgs { + memberships = append(memberships, orgMembership{State: "active", Role: "admin", Organization: org}) + } + return memberships +} + func TestGitHubHandler_ExchangeToken(t *testing.T) { // Create test handler with mock config testSeed := make([]byte, ed25519.SeedSize) @@ -96,12 +116,12 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(user) //nolint:errcheck case githubOrgsEndpoint: - orgs := []v0auth.GitHubUserOrOrg{ + memberships := adminMemberships([]v0auth.GitHubUserOrOrg{ {Login: "test-org-1", ID: 1}, {Login: "test-org-2", ID: 2}, - } + }) w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(orgs) //nolint:errcheck + json.NewEncoder(w).Encode(memberships) //nolint:errcheck default: w.WriteHeader(http.StatusNotFound) } @@ -225,10 +245,9 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { } w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(user) //nolint:errcheck - case "/users/user with spaces/orgs": - orgs := []v0auth.GitHubUserOrOrg{} + case githubOrgsEndpoint: w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(orgs) //nolint:errcheck + json.NewEncoder(w).Encode([]orgMembership{}) //nolint:errcheck } })) defer mockServer.Close() @@ -264,12 +283,12 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(user) //nolint:errcheck case githubOrgsEndpoint: - orgs := []v0auth.GitHubUserOrOrg{ + memberships := adminMemberships([]v0auth.GitHubUserOrOrg{ {Login: "valid-org", ID: 1}, {Login: "org with spaces", ID: 2}, // Invalid name - } + }) w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(orgs) //nolint:errcheck + json.NewEncoder(w).Encode(memberships) //nolint:errcheck } })) defer mockServer.Close() @@ -293,6 +312,81 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { assert.Empty(t, claims.Permissions) // No permissions because one org has invalid name }) + t.Run("member-role org is not granted, only admin orgs", func(t *testing.T) { + // The user is an Owner (admin) of one org and an ordinary member of another. + // Only the org they administer should yield a publish permission. + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case githubUserEndpoint: + user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(user) //nolint:errcheck + case githubOrgsEndpoint: + memberships := []orgMembership{ + {State: "active", Role: "admin", Organization: v0auth.GitHubUserOrOrg{Login: "admin-org", ID: 1}}, + {State: "active", Role: "member", Organization: v0auth.GitHubUserOrOrg{Login: "member-org", ID: 2}}, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(memberships) //nolint:errcheck + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer mockServer.Close() + + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-github-token") + require.NoError(t, err) + require.NotNil(t, response) + + jwtManager := auth.NewJWTManager(cfg) + claims, err := jwtManager.ValidateToken(ctx, response.RegistryToken) + require.NoError(t, err) + + patterns := make([]string, 0, len(claims.Permissions)) + for _, perm := range claims.Permissions { + patterns = append(patterns, perm.ResourcePattern) + } + assert.ElementsMatch(t, []string{"io.github.testuser/*", "io.github.admin-org/*"}, patterns) + assert.NotContains(t, patterns, "io.github.member-org/*") + }) + + t.Run("missing read:org scope (403) still grants personal namespace", func(t *testing.T) { + // A minimal token without read:org authenticates fine but is forbidden from + // reading org memberships. Personal-namespace publishing must still work. + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case githubUserEndpoint: + user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(user) //nolint:errcheck + case githubOrgsEndpoint: + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message": "Token does not have the required scope"}`)) //nolint:errcheck + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer mockServer.Close() + + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-github-token") + require.NoError(t, err) + require.NotNil(t, response) + + jwtManager := auth.NewJWTManager(cfg) + claims, err := jwtManager.ValidateToken(ctx, response.RegistryToken) + require.NoError(t, err) + assert.Len(t, claims.Permissions, 1) + assert.Equal(t, "io.github.testuser/*", claims.Permissions[0].ResourcePattern) + }) + t.Run("malformed JSON response", func(t *testing.T) { // Create mock GitHub API server that returns invalid JSON mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -537,9 +631,9 @@ func TestValidGitHubNames(t *testing.T) { } w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(user) //nolint:errcheck - case "/users/" + tc.username + "/orgs": + case githubOrgsEndpoint: w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(tc.orgs) //nolint:errcheck + json.NewEncoder(w).Encode(adminMemberships(tc.orgs)) //nolint:errcheck } })) defer mockServer.Close() From 7d5eb072c7c32c9cd012e241a708ed20afd15855 Mon Sep 17 00:00:00 2001 From: tadasant Date: Sun, 21 Jun 2026 23:14:29 +0000 Subject: [PATCH 2/4] docs(github-actions): add CI token-hardening guidance for org publishing The token that authenticates to an org namespace can publish and overwrite any server under io.github./*, 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 --- .../github-actions.mdx | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/modelcontextprotocol-io/github-actions.mdx b/docs/modelcontextprotocol-io/github-actions.mdx index 0da50cdf8..b28827e4b 100644 --- a/docs/modelcontextprotocol-io/github-actions.mdx +++ b/docs/modelcontextprotocol-io/github-actions.mdx @@ -81,6 +81,10 @@ on: jobs: publish: runs-on: ubuntu-latest + # Source MCP_GITHUB_TOKEN from a protected Environment, not a plain repo + # secret, and restrict that environment to your default branch / release + # tags. See "Securing your registry token in CI" below for why this matters. + environment: mcp-registry-publish permissions: contents: read @@ -195,13 +199,30 @@ jobs: You may need to add a secret to the repository depending on which authentication method you choose: - **GitHub OIDC Authentication**: No dedicated secret necessary. -- **GitHub PAT Authentication**: Add a `MCP_GITHUB_TOKEN` secret with a GitHub Personal Access Token (PAT) that has `read:org` and `read:user` scopes. +- **GitHub PAT Authentication**: Add a `MCP_GITHUB_TOKEN` secret with a GitHub Personal Access Token (PAT) that has `read:org` and `read:user` scopes. The `read:org` scope is what lets the registry confirm you are an **Owner** of the organization before granting its namespace; the token needs **no** repository scopes (the registry never reads or writes your code). - **DNS Authentication**: Add a `MCP_PRIVATE_KEY` secret with your Ed25519 private key. You may also need to add secrets for your package registry. For example, the workflow above needs an `NPM_TOKEN` secret with your npm token. For information about how to add secrets to a repository, see [Using secrets in GitHub Actions](https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-secrets). +### Securing your registry token in CI + +Whichever token you use to authenticate, treat it as a high-value credential. When you authenticate to an **organization** namespace, the resulting registry token can publish — and overwrite — **any** server under `io.github./*`, not just the one in this repository. The relevant question for your threat model is therefore: *who can cause code to run in a job where this secret is exposed?* By default that is **every repository writer** (via branch or tag pushes), not only org Owners — so a plain repo secret quietly widens publish access to everyone with write access. + +GitHub gives you the tools to close that gap. We recommend, in increasing order of strength: + +1. **Store the token as an [Environment secret](https://docs.github.com/en/actions/how-tos/deploy/configure-and-manage-deployments/manage-environments), not a repository or organization secret.** A job only receives it when it declares `environment:` (as the PAT example above does). +2. **Restrict that environment to your default branch and/or release tags** with a [deployment branch rule](https://docs.github.com/en/actions/how-tos/deploy/configure-and-manage-deployments/manage-environments#deployment-branches-and-tags), and protect that branch with [required pull request reviews](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches). Now only reviewed, maintainer-approved code can ever reach the token. +3. **(Strongest) Add a [required reviewer](https://docs.github.com/en/actions/how-tos/deploy/configure-and-manage-deployments/manage-environments#required-reviewers) to the environment** so each publish pauses for an explicit human approval. + +Two pitfalls to avoid regardless of the above: + +- **Never publish from a `pull_request_target` workflow that checks out PR-head code.** That trigger runs with your secrets in the base-repo context, so an untrusted fork PR could exfiltrate the token. Environment branch rules do **not** protect you here (the ref is still your base branch) — only required reviewers do. +- **Avoid self-hosted runners for the publish job on public repositories**, where fork PRs may be able to schedule jobs onto them. + +By default, pull requests **from forks** receive no secrets and cannot push branches, so external contributors cannot reach the token without one of the misconfigurations above. + ## Step 3: Tag and Release Create and push a version tag to trigger the workflow: From 7e25513aac436b232698f51eb6b2a1545368897e Mon Sep 17 00:00:00 2001 From: tadasant Date: Mon, 22 Jun 2026 03:37:51 +0000 Subject: [PATCH 3/4] test(auth): split org-role cases into own test to satisfy cyclop 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 --- .../api/handlers/v0/auth/github_at_test.go | 56 +++++++++++-------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/internal/api/handlers/v0/auth/github_at_test.go b/internal/api/handlers/v0/auth/github_at_test.go index 1337a0da5..840cfa80f 100644 --- a/internal/api/handlers/v0/auth/github_at_test.go +++ b/internal/api/handlers/v0/auth/github_at_test.go @@ -312,6 +312,39 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { assert.Empty(t, claims.Permissions) // No permissions because one org has invalid name }) + t.Run("malformed JSON response", func(t *testing.T) { + // Create mock GitHub API server that returns invalid JSON + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == githubUserEndpoint { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{invalid json`)) //nolint:errcheck + } + })) + defer mockServer.Close() + + // Create handler and set mock server URL + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + // Test token exchange + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-token") + + require.Error(t, err) + assert.Nil(t, response) + assert.Contains(t, err.Error(), "failed to decode") + }) +} + +func TestGitHubHandler_ExchangeToken_OrgRoles(t *testing.T) { + testSeed := make([]byte, ed25519.SeedSize) + _, err := rand.Read(testSeed) + require.NoError(t, err) + + cfg := &config.Config{ + JWTPrivateKey: hex.EncodeToString(testSeed), + } + t.Run("member-role org is not granted, only admin orgs", func(t *testing.T) { // The user is an Owner (admin) of one org and an ordinary member of another. // Only the org they administer should yield a publish permission. @@ -386,29 +419,6 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) { assert.Len(t, claims.Permissions, 1) assert.Equal(t, "io.github.testuser/*", claims.Permissions[0].ResourcePattern) }) - - t.Run("malformed JSON response", func(t *testing.T) { - // Create mock GitHub API server that returns invalid JSON - mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == githubUserEndpoint { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{invalid json`)) //nolint:errcheck - } - })) - defer mockServer.Close() - - // Create handler and set mock server URL - handler := v0auth.NewGitHubHandler(cfg) - handler.SetBaseURL(mockServer.URL) - - // Test token exchange - ctx := context.Background() - response, err := handler.ExchangeToken(ctx, "valid-token") - - require.Error(t, err) - assert.Nil(t, response) - assert.Contains(t, err.Error(), "failed to decode") - }) } func TestJWTTokenValidation(t *testing.T) { From a10d041559c01cbc59da04c511490bd7b74052a3 Mon Sep 17 00:00:00 2001 From: tadasant Date: Fri, 26 Jun 2026 21:26:02 +0000 Subject: [PATCH 4/4] test(auth): harden org-membership 403 handling and pagination 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 --- internal/api/handlers/v0/auth/github_at.go | 21 +++- .../api/handlers/v0/auth/github_at_test.go | 107 ++++++++++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/internal/api/handlers/v0/auth/github_at.go b/internal/api/handlers/v0/auth/github_at.go index cb17e8964..4960fb46f 100644 --- a/internal/api/handlers/v0/auth/github_at.go +++ b/internal/api/handlers/v0/auth/github_at.go @@ -136,8 +136,9 @@ func (h *GitHubHandler) getGitHubUser(ctx context.Context, token string) (*GitHu // githubOrgRoleAdmin is the membership role GitHub returns for an organization // Owner. It is the only role we treat as carrying publish authority for the org -// namespace. (GitHub has no org-level "maintainer" role; org membership is either -// "admin" (Owner) or "member".) +// namespace. (GitHub has no org-level "maintainer" role. GET /user/memberships/orgs +// returns role "admin" (Owner), "member", or "billing_manager"; only "admin" +// carries publish authority, so the other two are intentionally not granted.) const githubOrgRoleAdmin = "admin" // orgMembershipsPageSize is the page size we request when listing the user's org @@ -209,10 +210,20 @@ func (h *GitHubHandler) fetchOrgMembershipsPage(ctx context.Context, token strin } defer resp.Body.Close() - // A token without the read:org scope can still authenticate (GET /user works) - // but cannot read org memberships, yielding a 403. Treat that as "no admin - // orgs" so personal-namespace publishing keeps working with a minimal token. + // GitHub returns 403 here for two very different reasons, which we must not + // conflate: + // 1. The token lacks the read:org scope. This is the common, benign case for a + // minimal personal-publishing token: GET /user still works, but org + // memberships are forbidden. We degrade gracefully to "no admin orgs" so + // personal-namespace publishing keeps working without over-scoping. + // 2. Rate limiting. GitHub signals primary/secondary rate limits with 403 (or + // 429), setting X-RateLimit-Remaining: 0 and/or Retry-After. Degrading here + // would silently strip a legitimate Owner's org grant, so we fail closed + // (return an error) rather than mistake a throttle for "not an admin". if resp.StatusCode == http.StatusForbidden { + if resp.Header.Get("X-RateLimit-Remaining") == "0" || resp.Header.Get("Retry-After") != "" { + return nil, fmt.Errorf("GitHub API rate limit exceeded while listing org memberships (status 403): %s", readErrorBody(resp.Body)) + } return nil, nil } diff --git a/internal/api/handlers/v0/auth/github_at_test.go b/internal/api/handlers/v0/auth/github_at_test.go index 840cfa80f..f784b4ee9 100644 --- a/internal/api/handlers/v0/auth/github_at_test.go +++ b/internal/api/handlers/v0/auth/github_at_test.go @@ -419,6 +419,113 @@ func TestGitHubHandler_ExchangeToken_OrgRoles(t *testing.T) { assert.Len(t, claims.Permissions, 1) assert.Equal(t, "io.github.testuser/*", claims.Permissions[0].ResourcePattern) }) + + t.Run("admin org on a later page is still granted (pagination)", func(t *testing.T) { + // Page 1 is a full page (100) of member-role orgs; the only admin org is on + // page 2. The pagination loop must not stop at the first page, or the Owner's + // org grant would be silently dropped. + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case githubUserEndpoint: + user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(user) //nolint:errcheck + case githubOrgsEndpoint: + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "1" { + page1 := make([]orgMembership, 0, 100) + for i := 0; i < 100; i++ { + page1 = append(page1, orgMembership{ + State: "active", Role: "member", + Organization: v0auth.GitHubUserOrOrg{Login: fmt.Sprintf("member-org-%d", i), ID: 1000 + i}, + }) + } + json.NewEncoder(w).Encode(page1) //nolint:errcheck + return + } + // page 2: a single admin org, signalling the last (short) page. + page2 := []orgMembership{ + {State: "active", Role: "admin", Organization: v0auth.GitHubUserOrOrg{Login: "late-admin-org", ID: 2}}, + } + json.NewEncoder(w).Encode(page2) //nolint:errcheck + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer mockServer.Close() + + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-github-token") + require.NoError(t, err) + require.NotNil(t, response) + + jwtManager := auth.NewJWTManager(cfg) + claims, err := jwtManager.ValidateToken(ctx, response.RegistryToken) + require.NoError(t, err) + + patterns := make([]string, 0, len(claims.Permissions)) + for _, perm := range claims.Permissions { + patterns = append(patterns, perm.ResourcePattern) + } + assert.ElementsMatch(t, []string{"io.github.testuser/*", "io.github.late-admin-org/*"}, patterns) + }) + + t.Run("memberships server error fails closed (no token issued)", func(t *testing.T) { + // A 5xx (or any non-200, non-scope-403) from the memberships endpoint must + // abort the exchange rather than silently degrade to personal-only perms. + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case githubUserEndpoint: + user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(user) //nolint:errcheck + case githubOrgsEndpoint: + w.WriteHeader(http.StatusInternalServerError) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer mockServer.Close() + + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-github-token") + require.Error(t, err) + assert.Nil(t, response) + }) + + t.Run("rate-limit 403 fails closed (not treated as missing scope)", func(t *testing.T) { + // A 403 with X-RateLimit-Remaining: 0 is a throttle, not a missing-scope + // signal. Degrading would strip a real Owner's org grant, so it must error. + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case githubUserEndpoint: + user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(user) //nolint:errcheck + case githubOrgsEndpoint: + w.Header().Set("X-RateLimit-Remaining", "0") + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message": "API rate limit exceeded"}`)) //nolint:errcheck + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer mockServer.Close() + + handler := v0auth.NewGitHubHandler(cfg) + handler.SetBaseURL(mockServer.URL) + + ctx := context.Background() + response, err := handler.ExchangeToken(ctx, "valid-github-token") + require.Error(t, err) + assert.Nil(t, response) + }) } func TestJWTTokenValidation(t *testing.T) {