Skip to content

Commit 32b1c06

Browse files
SyedAnas01SamMorrowDrums
authored andcommitted
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 8ec6249 commit 32b1c06

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
@@ -63,6 +63,16 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
6363
return nil, fmt.Errorf("failed to get Raw URL: %w", err)
6464
}
6565

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

pkg/github/dependencies.go

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

358+
baseRestURL, err := d.apiHosts.BaseRESTURL(ctx)
359+
if err != nil {
360+
return nil, fmt.Errorf("failed to get base REST URL: %w", err)
361+
}
362+
uploadURL, err := d.apiHosts.UploadURL(ctx)
363+
if err != nil {
364+
return nil, fmt.Errorf("failed to get upload URL: %w", err)
365+
}
366+
graphqlURL, err := d.apiHosts.GraphqlURL(ctx)
367+
if err != nil {
368+
return nil, fmt.Errorf("failed to get GraphQL URL: %w", err)
369+
}
370+
rawURL, err := d.apiHosts.RawURL(ctx)
371+
if err != nil {
372+
return nil, fmt.Errorf("failed to get Raw URL: %w", err)
373+
}
374+
375+
// allowedHosts scopes the bearer token to the configured GitHub hosts, so a
376+
// response that redirects off them does not carry the token to the redirect
377+
// target. See transport.BearerAuthTransport.
378+
allowedHosts := []string{
379+
baseRestURL.Hostname(),
380+
uploadURL.Hostname(),
381+
graphqlURL.Hostname(),
382+
rawURL.Hostname(),
383+
}
384+
358385
// Construct GraphQL client
359386
// We use NewEnterpriseClient unconditionally since we already parsed the API host
360387
// Wrap transport with GraphQLFeaturesTransport to inject feature flags from context,
@@ -364,15 +391,11 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error
364391
Transport: &transport.GraphQLFeaturesTransport{
365392
Transport: http.DefaultTransport,
366393
},
367-
Token: token,
394+
Token: token,
395+
AllowedHosts: allowedHosts,
368396
},
369397
}
370398

371-
graphqlURL, err := d.apiHosts.GraphqlURL(ctx)
372-
if err != nil {
373-
return nil, fmt.Errorf("failed to get GraphQL URL: %w", err)
374-
}
375-
376399
gqlClient := githubv4.NewEnterpriseClient(graphqlURL.String(), gqlHTTPClient)
377400
return gqlClient, nil
378401
}

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)