Skip to content

Commit 11f062c

Browse files
committed
Attach GitHub token only to configured GitHub hosts
BearerAuthTransport re-adds the Authorization header on every hop, which defeats net/http's cross-host redirect stripping. Scope the credential to the configured hosts so a redirect off them travels without the token. An empty AllowedHosts preserves prior behavior; the three production construction sites populate it from the configured REST, upload, GraphQL and raw hosts.
1 parent 2198e85 commit 11f062c

4 files changed

Lines changed: 151 additions & 7 deletions

File tree

internal/ghmcp/server.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,16 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
6262
return nil, fmt.Errorf("failed to get Raw URL: %w", err)
6363
}
6464

65+
// allowedHosts scopes the bearer token to the configured GitHub hosts, so a
66+
// response that redirects off them does not carry the token to the redirect
67+
// target. See transport.BearerAuthTransport.
68+
allowedHosts := []string{
69+
restURL.Hostname(),
70+
uploadURL.Hostname(),
71+
graphQLURL.Hostname(),
72+
rawURL.Hostname(),
73+
}
74+
6575
// Construct REST client. When a TokenProvider is configured, we
6676
// authenticate via BearerAuthTransport and skip go-github's WithAuthToken:
6777
// the latter installs its own round tripper that would pin the static token
@@ -76,6 +86,7 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
7686
gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{
7787
Transport: restUATransport,
7888
TokenProvider: cfg.TokenProvider,
89+
AllowedHosts: allowedHosts,
7990
}}),
8091
gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()),
8192
)
@@ -99,6 +110,7 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
99110
},
100111
Token: cfg.Token,
101112
TokenProvider: cfg.TokenProvider,
113+
AllowedHosts: allowedHosts,
102114
},
103115
}
104116

pkg/github/dependencies.go

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,33 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error
343343
}
344344
token := tokenInfo.Token
345345

346+
baseRestURL, err := d.apiHosts.BaseRESTURL(ctx)
347+
if err != nil {
348+
return nil, fmt.Errorf("failed to get base REST URL: %w", err)
349+
}
350+
uploadURL, err := d.apiHosts.UploadURL(ctx)
351+
if err != nil {
352+
return nil, fmt.Errorf("failed to get upload URL: %w", err)
353+
}
354+
graphqlURL, err := d.apiHosts.GraphqlURL(ctx)
355+
if err != nil {
356+
return nil, fmt.Errorf("failed to get GraphQL URL: %w", err)
357+
}
358+
rawURL, err := d.apiHosts.RawURL(ctx)
359+
if err != nil {
360+
return nil, fmt.Errorf("failed to get Raw URL: %w", err)
361+
}
362+
363+
// allowedHosts scopes the bearer token to the configured GitHub hosts, so a
364+
// response that redirects off them does not carry the token to the redirect
365+
// target. See transport.BearerAuthTransport.
366+
allowedHosts := []string{
367+
baseRestURL.Hostname(),
368+
uploadURL.Hostname(),
369+
graphqlURL.Hostname(),
370+
rawURL.Hostname(),
371+
}
372+
346373
// Construct GraphQL client
347374
// We use NewEnterpriseClient unconditionally since we already parsed the API host
348375
// Wrap transport with GraphQLFeaturesTransport to inject feature flags from context,
@@ -352,15 +379,11 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error
352379
Transport: &transport.GraphQLFeaturesTransport{
353380
Transport: http.DefaultTransport,
354381
},
355-
Token: token,
382+
Token: token,
383+
AllowedHosts: allowedHosts,
356384
},
357385
}
358386

359-
graphqlURL, err := d.apiHosts.GraphqlURL(ctx)
360-
if err != nil {
361-
return nil, fmt.Errorf("failed to get GraphQL URL: %w", err)
362-
}
363-
364387
gqlClient := githubv4.NewEnterpriseClient(graphqlURL.String(), gqlHTTPClient)
365388
return gqlClient, nil
366389
}

pkg/http/transport/bearer.go

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,22 @@ type BearerAuthTransport struct {
1515
// TokenProvider, when non-nil, supplies the bearer token for each request
1616
// and takes precedence over Token.
1717
TokenProvider func() string
18+
19+
// AllowedHosts, when non-empty, restricts the hosts the Authorization
20+
// header is attached to. The token is set only when the request host
21+
// matches one of these entries (case-insensitive, host only, port
22+
// ignored). This scopes the credential to the configured GitHub hosts, so
23+
// that if a response redirects off them the token is not carried to the
24+
// redirect target.
25+
//
26+
// net/http strips a cross-host Authorization header when it follows a
27+
// redirect, but only for headers set on the initial request. This
28+
// transport re-adds the header on every hop, so that protection does not
29+
// otherwise apply here.
30+
//
31+
// When empty, the token is attached to every request, preserving the
32+
// prior behavior.
33+
AllowedHosts []string
1834
}
1935

2036
func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) {
@@ -23,7 +39,7 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro
2339
if t.TokenProvider != nil {
2440
token = t.TokenProvider()
2541
}
26-
if token != "" {
42+
if token != "" && t.hostAllowed(req.URL.Hostname()) {
2743
req.Header.Set(headers.AuthorizationHeader, "Bearer "+token)
2844
}
2945

@@ -34,3 +50,17 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro
3450

3551
return t.Transport.RoundTrip(req)
3652
}
53+
54+
// hostAllowed reports whether the token may be attached to a request bound for
55+
// host. An empty AllowedHosts allows all hosts, preserving prior behavior.
56+
func (t *BearerAuthTransport) hostAllowed(host string) bool {
57+
if len(t.AllowedHosts) == 0 {
58+
return true
59+
}
60+
for _, h := range t.AllowedHosts {
61+
if strings.EqualFold(h, host) {
62+
return true
63+
}
64+
}
65+
return false
66+
}

pkg/http/transport/bearer_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,3 +162,82 @@ func TestBearerAuthTransport_DoesNotMutateOriginalRequest(t *testing.T) {
162162

163163
assert.Empty(t, req.Header.Get(headers.AuthorizationHeader), "original request must not be mutated")
164164
}
165+
166+
// hostRecordingTransport records the Authorization header seen for each request
167+
// host, so a test can assert what the token would be attached to without a live
168+
// network. It stands in for the real transport at the bottom of the chain.
169+
type hostRecordingTransport struct {
170+
authByHost map[string]string
171+
}
172+
173+
func (h *hostRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
174+
h.authByHost[req.URL.Hostname()] = req.Header.Get(headers.AuthorizationHeader)
175+
return &http.Response{
176+
StatusCode: http.StatusOK,
177+
Body: http.NoBody,
178+
Header: make(http.Header),
179+
Request: req,
180+
}, nil
181+
}
182+
183+
// TestBearerAuthTransport_HostScoping verifies that when AllowedHosts is set,
184+
// the token is attached to a request on an allowed host but withheld from a
185+
// request to any other host. A redirect off the configured GitHub hosts arrives
186+
// here as a RoundTrip to a different host, so this is the property that keeps
187+
// the token from following such a redirect. net/http's own cross-host stripping
188+
// does not cover it, because this transport re-adds the header on every hop.
189+
//
190+
// The hosts are distinct hostnames (matching the real case: api.github.com
191+
// versus objects.githubusercontent.com) rather than two loopback servers on
192+
// different ports, because AllowedHosts matches on hostname and ignores port.
193+
func TestBearerAuthTransport_HostScoping(t *testing.T) {
194+
t.Parallel()
195+
196+
rec := &hostRecordingTransport{authByHost: map[string]string{}}
197+
rt := &BearerAuthTransport{
198+
Transport: rec,
199+
Token: "secret-token",
200+
AllowedHosts: []string{"api.github.com", "raw.githubusercontent.com"},
201+
}
202+
203+
for _, target := range []string{
204+
"https://api.github.com/repos/o/r",
205+
"https://raw.githubusercontent.com/o/r/main/f", // allowed, different host
206+
"https://objects.githubusercontent.com/evil", // redirect target, not allowed
207+
"https://attacker.example.com/steal", // arbitrary host, not allowed
208+
} {
209+
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, target, nil)
210+
require.NoError(t, err)
211+
resp, err := rt.RoundTrip(req)
212+
require.NoError(t, err)
213+
resp.Body.Close()
214+
}
215+
216+
assert.Equal(t, "Bearer secret-token", rec.authByHost["api.github.com"],
217+
"token must be sent to an allowed host")
218+
assert.Equal(t, "Bearer secret-token", rec.authByHost["raw.githubusercontent.com"],
219+
"token must be sent to every allowed host")
220+
assert.Empty(t, rec.authByHost["objects.githubusercontent.com"],
221+
"token must not be sent to a non-allowed host (a redirect target)")
222+
assert.Empty(t, rec.authByHost["attacker.example.com"],
223+
"token must not be sent to an arbitrary non-allowed host")
224+
}
225+
226+
// TestBearerAuthTransport_EmptyAllowedHostsPreservesBehavior verifies the
227+
// backward-compatible default: with no AllowedHosts, the token is attached to
228+
// every host, exactly as before this change.
229+
func TestBearerAuthTransport_EmptyAllowedHostsPreservesBehavior(t *testing.T) {
230+
t.Parallel()
231+
232+
rec := &hostRecordingTransport{authByHost: map[string]string{}}
233+
rt := &BearerAuthTransport{Transport: rec, Token: "secret-token"}
234+
235+
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://anywhere.example.com/x", nil)
236+
require.NoError(t, err)
237+
resp, err := rt.RoundTrip(req)
238+
require.NoError(t, err)
239+
resp.Body.Close()
240+
241+
assert.Equal(t, "Bearer secret-token", rec.authByHost["anywhere.example.com"],
242+
"with no AllowedHosts, token attaches to every host as before")
243+
}

0 commit comments

Comments
 (0)