Visitar URL original
fix(auth): harden github-at token exchange after review · modelcontextprotocol/registry@b9844b5 · GitHub
Skip to content

Commit b9844b5

Browse files
rdimitrovclaude
rdimitrov
and
claude
committed
fix(auth): harden github-at token exchange after review
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>
1 parent a10d041 commit b9844b5

3 files changed

Lines changed: 239 additions & 21 deletions

File tree

‎internal/api/handlers/v0/auth/github_at.go‎

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"log"
78
"net/http"
89
"regexp"
910
"strings"
@@ -57,7 +58,7 @@ func RegisterGitHubATEndpoint(api huma.API, pathPrefix string, cfg *config.Confi
5758
}, func(ctx context.Context, input *GitHubTokenExchangeInput) (*v0.Response[auth.TokenResponse], error) {
5859
response, err := handler.ExchangeToken(ctx, input.Body.GitHubToken)
5960
if err != nil {
60-
return nil, huma.Error401Unauthorized("Token exchange failed", err)
61+
return nil, tokenExchangeError(err)
6162
}
6263

6364
return &v0.Response[auth.TokenResponse]{
@@ -66,6 +67,17 @@ func RegisterGitHubATEndpoint(api huma.API, pathPrefix string, cfg *config.Confi
6667
})
6768
}
6869

70+
// tokenExchangeError maps an internal ExchangeToken failure to a client-facing
71+
// 401 without leaking internal detail. huma serializes extra error args into the
72+
// response body, and the wrapped err here can include the raw upstream GitHub
73+
// response body captured by readErrorBody — so passing it to huma would echo that
74+
// detail back to the caller (CWE-209). We log it server-side (like the sibling
75+
// handlers in servers.go) and return only a generic message.
76+
func tokenExchangeError(err error) error {
77+
log.Printf("github-at token exchange failed: %v", err)
78+
return huma.Error401Unauthorized("Token exchange failed")
79+
}
80+
6981
// ExchangeToken exchanges a GitHub OAuth token for a Registry JWT token
7082
func (h *GitHubHandler) ExchangeToken(ctx context.Context, githubToken string) (*auth.TokenResponse, error) {
7183
// Get GitHub user information
@@ -136,15 +148,26 @@ func (h *GitHubHandler) getGitHubUser(ctx context.Context, token string) (*GitHu
136148

137149
// githubOrgRoleAdmin is the membership role GitHub returns for an organization
138150
// Owner. It is the only role we treat as carrying publish authority for the org
139-
// namespace. (GitHub has no org-level "maintainer" role. GET /user/memberships/orgs
140-
// returns role "admin" (Owner), "member", or "billing_manager"; only "admin"
141-
// carries publish authority, so the other two are intentionally not granted.)
151+
// namespace. GET /user/memberships/orgs reports the caller's role as either
152+
// "admin" (Owner) or "member" (GitHub has no org-level "maintainer" role); only
153+
// "admin" carries publish authority, so a "member" is intentionally not granted.
142154
const githubOrgRoleAdmin = "admin"
143155

156+
// githubMembershipStateActive is the membership state for an accepted (as opposed
157+
// to merely invited/pending) org membership. We only honor active memberships so a
158+
// not-yet-accepted owner invitation never grants the org namespace.
159+
const githubMembershipStateActive = "active"
160+
144161
// orgMembershipsPageSize is the page size we request when listing the user's org
145162
// memberships. 100 is GitHub's maximum.
146163
const orgMembershipsPageSize = 100
147164

165+
// maxOrgMembershipPages bounds the pagination loop. At orgMembershipsPageSize=100
166+
// this covers 10,000 org memberships — far beyond any real user — and exists only
167+
// as a backstop so a misbehaving or redirected upstream cannot drive an unbounded
168+
// number of requests.
169+
const maxOrgMembershipPages = 100
170+
148171
// githubOrgMembership is one entry from GET /user/memberships/orgs: the
149172
// authenticated user's membership in a single organization, including their role.
150173
type githubOrgMembership struct {
@@ -171,24 +194,32 @@ type githubOrgMembership struct {
171194
func (h *GitHubHandler) getGitHubAdminOrgs(ctx context.Context, token string) ([]GitHubUserOrOrg, error) {
172195
var adminOrgs []GitHubUserOrOrg
173196

174-
for page := 1; ; page++ {
197+
for page := 1; page <= maxOrgMembershipPages; page++ {
175198
memberships, err := h.fetchOrgMembershipsPage(ctx, token, page)
176199
if err != nil {
177200
return nil, err
178201
}
179202

180203
for _, m := range memberships {
181-
if m.Role == githubOrgRoleAdmin {
204+
// state=active is already requested in the query string, but re-check it
205+
// here as defense in depth: if that param is ever dropped or changed, this
206+
// keeps a pending (not-yet-accepted) owner invitation from being granted
207+
// the org namespace.
208+
if m.State == githubMembershipStateActive && m.Role == githubOrgRoleAdmin {
182209
adminOrgs = append(adminOrgs, m.Organization)
183210
}
184211
}
185212

213+
// A short page is the last page: return the complete result.
186214
if len(memberships) < orgMembershipsPageSize {
187-
break
215+
return adminOrgs, nil
188216
}
189217
}
190218

191-
return adminOrgs, nil
219+
// Every page up to the cap was full. A real user never has this many org
220+
// memberships, so rather than return a possibly-truncated set (which would
221+
// silently strip a legitimate Owner's grant), fail closed.
222+
return nil, fmt.Errorf("org memberships exceeded %d pages; refusing to issue a possibly-truncated grant", maxOrgMembershipPages)
192223
}
193224

194225
// fetchOrgMembershipsPage fetches a single page of the authenticated user's
@@ -224,6 +255,13 @@ func (h *GitHubHandler) fetchOrgMembershipsPage(ctx context.Context, token strin
224255
if resp.Header.Get("X-RateLimit-Remaining") == "0" || resp.Header.Get("Retry-After") != "" {
225256
return nil, fmt.Errorf("GitHub API rate limit exceeded while listing org memberships (status 403): %s", readErrorBody(resp.Body))
226257
}
258+
// A SAML/SSO-enforced org returns 403 with an X-GitHub-SSO header when the
259+
// token has not been SSO-authorized. That is an Owner being blocked, not a
260+
// missing scope, so fail closed rather than silently dropping the org grant
261+
// (which would degrade a legitimate Owner to personal-only with no signal).
262+
if resp.Header.Get("X-GitHub-SSO") != "" {
263+
return nil, fmt.Errorf("GitHub org memberships require SSO authorization for this token (status 403): %s", readErrorBody(resp.Body))
264+
}
227265
return nil, nil
228266
}
229267

@@ -241,26 +279,27 @@ func (h *GitHubHandler) fetchOrgMembershipsPage(ctx context.Context, token strin
241279

242280
// buildPermissions builds permissions based on GitHub user and their organizations
243281
func (h *GitHubHandler) buildPermissions(username string, orgs []GitHubUserOrOrg) []auth.Permission {
244-
permissions := []auth.Permission{}
245-
246-
// Assert user and org names match expected regex, to harden against people doing weird things in names
282+
// Assert the username matches the expected regex, to harden against people doing
283+
// weird things in names. The username is the caller's own identity, so if it is
284+
// invalid we grant nothing.
247285
if !isValidGitHubName(username) {
248286
return nil
249287
}
250-
for _, org := range orgs {
251-
if !isValidGitHubName(org.Login) {
252-
return nil
253-
}
254-
}
255288

256289
// Add permission for user's own namespace
257-
permissions = append(permissions, auth.Permission{
290+
permissions := []auth.Permission{{
258291
Action: auth.PermissionActionPublish,
259292
ResourcePattern: fmt.Sprintf("io.github.%s/*", username),
260-
})
293+
}}
261294

262-
// Add permissions for each organization the user administers (Owner role)
295+
// Add permissions for each organization the user administers (Owner role). Skip
296+
// any org whose name fails validation rather than rejecting the whole set, so one
297+
// unexpected org name can't strip the caller's personal namespace or their other
298+
// (valid) org grants.
263299
for _, org := range orgs {
300+
if !isValidGitHubName(org.Login) {
301+
continue
302+
}
264303
permissions = append(permissions, auth.Permission{
265304
Action: auth.PermissionActionPublish,
266305
ResourcePattern: fmt.Sprintf("io.github.%s/*", org.Login),

‎internal/api/handlers/v0/auth/github_at_internal_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package auth
2+
3+
import (
4+
"encoding/json"
5+
"errors"
6+
"testing"
7+
8+
"github.com/danielgtaylor/huma/v2"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// The 401 returned to the client must not echo the internal error, which can
14+
// include the raw upstream GitHub response body captured by readErrorBody
15+
// (CWE-209). huma serializes extra error args into the response body, so the fix
16+
// is to log server-side and return only a generic message. This mirrors the
17+
// regression test added for the GET /v0/servers 500 leak (#1338).
18+
func TestTokenExchangeError_doesNotLeakDetail(t *testing.T) {
19+
const internal = `failed to get GitHub organizations: GitHub API error (status 500): {"message":"internal-host db detail"}`
20+
21+
err := tokenExchangeError(errors.New(internal))
22+
require.Error(t, err)
23+
24+
var se huma.StatusError
25+
require.True(t, errors.As(err, &se))
26+
assert.Equal(t, 401, se.GetStatus())
27+
28+
body, marshalErr := json.Marshal(err)
29+
require.NoError(t, marshalErr)
30+
assert.NotContains(t, string(body), "internal-host db detail")
31+
assert.NotContains(t, string(body), "status 500")
32+
assert.Contains(t, string(body), "Token exchange failed")
33+
}

‎internal/api/handlers/v0/auth/github_at_test.go‎

Lines changed: 148 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,59 @@ func adminMemberships(orgs []v0auth.GitHubUserOrOrg) []orgMembership {
4545
return memberships
4646
}
4747

48+
// newMockGitHubServer returns a mock GitHub API server that serves a fixed
49+
// "testuser" on /user and delegates the org-memberships endpoint to orgsHandler.
50+
// It keeps individual test cases small (only the memberships behavior varies),
51+
// which also keeps their cyclomatic complexity down.
52+
func newMockGitHubServer(orgsHandler http.HandlerFunc) *httptest.Server {
53+
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
54+
switch r.URL.Path {
55+
case githubUserEndpoint:
56+
user := v0auth.GitHubUserOrOrg{Login: "testuser", ID: 12345}
57+
w.Header().Set("Content-Type", "application/json")
58+
json.NewEncoder(w).Encode(user) //nolint:errcheck
59+
case githubOrgsEndpoint:
60+
orgsHandler(w, r)
61+
default:
62+
w.WriteHeader(http.StatusNotFound)
63+
}
64+
}))
65+
}
66+
67+
// grantedPatterns exchanges the token and returns the resource patterns granted
68+
// in the resulting JWT.
69+
func grantedPatterns(t *testing.T, cfg *config.Config, server *httptest.Server) []string {
70+
t.Helper()
71+
handler := v0auth.NewGitHubHandler(cfg)
72+
handler.SetBaseURL(server.URL)
73+
74+
ctx := context.Background()
75+
response, err := handler.ExchangeToken(ctx, "valid-github-token")
76+
require.NoError(t, err)
77+
require.NotNil(t, response)
78+
79+
claims, err := auth.NewJWTManager(cfg).ValidateToken(ctx, response.RegistryToken)
80+
require.NoError(t, err)
81+
82+
patterns := make([]string, 0, len(claims.Permissions))
83+
for _, perm := range claims.Permissions {
84+
patterns = append(patterns, perm.ResourcePattern)
85+
}
86+
return patterns
87+
}
88+
89+
// assertExchangeFailsClosed asserts that a token exchange against server returns
90+
// an error and no token (used for the fail-closed 403 variants).
91+
func assertExchangeFailsClosed(t *testing.T, cfg *config.Config, server *httptest.Server) {
92+
t.Helper()
93+
handler := v0auth.NewGitHubHandler(cfg)
94+
handler.SetBaseURL(server.URL)
95+
96+
response, err := handler.ExchangeToken(context.Background(), "valid-github-token")
97+
require.Error(t, err)
98+
assert.Nil(t, response)
99+
}
100+
48101
func TestGitHubHandler_ExchangeToken(t *testing.T) {
49102
// Create test handler with mock config
50103
testSeed := make([]byte, ed25519.SeedSize)
@@ -309,7 +362,15 @@ func TestGitHubHandler_ExchangeToken(t *testing.T) {
309362
claims, err := jwtManager.ValidateToken(ctx, response.RegistryToken)
310363
require.NoError(t, err)
311364
assert.Equal(t, "testuser", claims.AuthMethodSubject)
312-
assert.Empty(t, claims.Permissions) // No permissions because one org has invalid name
365+
366+
// The invalid org is skipped, but the personal namespace and the valid org
367+
// are still granted — one weird org name must not strip everything else.
368+
patterns := make([]string, 0, len(claims.Permissions))
369+
for _, perm := range claims.Permissions {
370+
patterns = append(patterns, perm.ResourcePattern)
371+
}
372+
assert.ElementsMatch(t, []string{"io.github.testuser/*", "io.github.valid-org/*"}, patterns)
373+
assert.NotContains(t, patterns, "io.github.org with spaces/*")
313374
})
314375

315376
t.Run("malformed JSON response", func(t *testing.T) {
@@ -528,6 +589,82 @@ func TestGitHubHandler_ExchangeToken_OrgRoles(t *testing.T) {
528589
})
529590
}
530591

592+
func TestGitHubHandler_ExchangeToken_MembershipFiltering(t *testing.T) {
593+
testSeed := make([]byte, ed25519.SeedSize)
594+
_, err := rand.Read(testSeed)
595+
require.NoError(t, err)
596+
597+
cfg := &config.Config{JWTPrivateKey: hex.EncodeToString(testSeed)}
598+
599+
t.Run("pending admin membership is not granted (state filter)", func(t *testing.T) {
600+
// A user invited as an Owner but who has not accepted has role "admin" with
601+
// state "pending". They are not an Owner yet, so the org must not be granted
602+
// even if the state=active query filter is somehow bypassed.
603+
server := newMockGitHubServer(func(w http.ResponseWriter, _ *http.Request) {
604+
memberships := []orgMembership{
605+
{State: "pending", Role: "admin", Organization: v0auth.GitHubUserOrOrg{Login: "pending-org", ID: 1}},
606+
{State: "active", Role: "admin", Organization: v0auth.GitHubUserOrOrg{Login: "active-org", ID: 2}},
607+
}
608+
w.Header().Set("Content-Type", "application/json")
609+
json.NewEncoder(w).Encode(memberships) //nolint:errcheck
610+
})
611+
defer server.Close()
612+
613+
patterns := grantedPatterns(t, cfg, server)
614+
assert.ElementsMatch(t, []string{"io.github.testuser/*", "io.github.active-org/*"}, patterns)
615+
assert.NotContains(t, patterns, "io.github.pending-org/*")
616+
})
617+
618+
t.Run("billing_manager role is not granted", func(t *testing.T) {
619+
server := newMockGitHubServer(func(w http.ResponseWriter, _ *http.Request) {
620+
memberships := []orgMembership{
621+
{State: "active", Role: "billing_manager", Organization: v0auth.GitHubUserOrOrg{Login: "billing-org", ID: 1}},
622+
}
623+
w.Header().Set("Content-Type", "application/json")
624+
json.NewEncoder(w).Encode(memberships) //nolint:errcheck
625+
})
626+
defer server.Close()
627+
628+
patterns := grantedPatterns(t, cfg, server)
629+
assert.Equal(t, []string{"io.github.testuser/*"}, patterns)
630+
})
631+
}
632+
633+
func TestGitHubHandler_ExchangeToken_403FailClosed(t *testing.T) {
634+
testSeed := make([]byte, ed25519.SeedSize)
635+
_, err := rand.Read(testSeed)
636+
require.NoError(t, err)
637+
638+
cfg := &config.Config{JWTPrivateKey: hex.EncodeToString(testSeed)}
639+
640+
t.Run("Retry-After 403 fails closed (secondary rate limit)", func(t *testing.T) {
641+
// A 403 carrying only Retry-After (no X-RateLimit-Remaining: 0) is a secondary
642+
// rate limit, not a missing scope. Degrading would strip a real Owner's grant.
643+
server := newMockGitHubServer(func(w http.ResponseWriter, _ *http.Request) {
644+
w.Header().Set("Retry-After", "60")
645+
w.WriteHeader(http.StatusForbidden)
646+
w.Write([]byte(`{"message": "You have exceeded a secondary rate limit"}`)) //nolint:errcheck
647+
})
648+
defer server.Close()
649+
650+
assertExchangeFailsClosed(t, cfg, server)
651+
})
652+
653+
t.Run("SSO-enforced 403 fails closed (not treated as missing scope)", func(t *testing.T) {
654+
// A SAML/SSO-enforced org returns 403 with X-GitHub-SSO when the token is not
655+
// SSO-authorized. That is an Owner being blocked, so it must fail closed rather
656+
// than silently degrade to personal-only.
657+
server := newMockGitHubServer(func(w http.ResponseWriter, _ *http.Request) {
658+
w.Header().Set("X-GitHub-SSO", "required; two_factor_authentication=false")
659+
w.WriteHeader(http.StatusForbidden)
660+
w.Write([]byte(`{"message": "Resource protected by organization SAML enforcement."}`)) //nolint:errcheck
661+
})
662+
defer server.Close()
663+
664+
assertExchangeFailsClosed(t, cfg, server)
665+
})
666+
}
667+
531668
func TestJWTTokenValidation(t *testing.T) {
532669
testSeed := make([]byte, ed25519.SeedSize)
533670
_, err := rand.Read(testSeed)
@@ -732,7 +869,16 @@ func TestValidGitHubNames(t *testing.T) {
732869
orgs: []v0auth.GitHubUserOrOrg{
733870
{Login: "invalid org", ID: 1},
734871
},
735-
wantPerms: 0, // Should return nil if any name is invalid
872+
wantPerms: 1, // Personal namespace kept; the invalid org is skipped
873+
},
874+
{
875+
name: "valid username with one valid and one invalid org",
876+
username: "valid-user",
877+
orgs: []v0auth.GitHubUserOrOrg{
878+
{Login: "valid-org", ID: 1},
879+
{Login: "invalid org", ID: 2},
880+
},
881+
wantPerms: 2, // Personal + valid org; the invalid org is skipped
736882
},
737883
}
738884

0 commit comments

Comments
 (0)