Skip to content

Commit ca31ab4

Browse files
SamMorrowDrumsCopilot
SamMorrowDrums
and
Copilot
committed
fix(auth): scope tokens across GitHub clients
Use exact configured host authorities for every REST, GraphQL, and raw client so redirects cannot reattach credentials to foreign hosts or ports. Add adversarial redirect and lookalike coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 32b1c06 commit ca31ab4

6 files changed

Lines changed: 411 additions & 42 deletions

File tree

‎internal/ghmcp/oauth_test.go‎

Lines changed: 104 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@ import (
77
"log/slog"
88
"net/http"
99
"net/http/httptest"
10+
"net/url"
1011
"testing"
1112

1213
"github.com/github/github-mcp-server/internal/oauth"
1314
"github.com/github/github-mcp-server/pkg/github"
1415
"github.com/github/github-mcp-server/pkg/http/headers"
15-
"github.com/github/github-mcp-server/pkg/utils"
1616
"github.com/google/jsonschema-go/jsonschema"
1717
"github.com/modelcontextprotocol/go-sdk/mcp"
1818
"github.com/stretchr/testify/assert"
@@ -23,6 +23,108 @@ func discardLogger() *slog.Logger {
2323
return slog.New(slog.NewTextHandler(io.Discard, nil))
2424
}
2525

26+
func TestCreateGitHubClientsScopesRESTAndRawTokens(t *testing.T) {
27+
t.Parallel()
28+
29+
var foreignAuth string
30+
foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
31+
foreignAuth = r.Header.Get(headers.AuthorizationHeader)
32+
w.WriteHeader(http.StatusOK)
33+
}))
34+
defer foreign.Close()
35+
36+
var sourceAuth string
37+
source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
38+
sourceAuth = r.Header.Get(headers.AuthorizationHeader)
39+
http.Redirect(w, r, foreign.URL, http.StatusFound)
40+
}))
41+
defer source.Close()
42+
43+
tests := []struct {
44+
name string
45+
cfg github.MCPServerConfig
46+
}{
47+
{
48+
name: "static token",
49+
cfg: github.MCPServerConfig{
50+
Version: "test",
51+
Token: "static-token",
52+
},
53+
},
54+
{
55+
name: "token provider",
56+
cfg: github.MCPServerConfig{
57+
Version: "test",
58+
TokenProvider: func() string { return "provider-token" },
59+
},
60+
},
61+
}
62+
63+
for _, tt := range tests {
64+
t.Run(tt.name, func(t *testing.T) {
65+
apiHost := newStaticAPIHostResolver(t, source.URL)
66+
clients, err := createGitHubClients(tt.cfg, apiHost)
67+
require.NoError(t, err)
68+
69+
sourceAuth = ""
70+
foreignAuth = ""
71+
resp, err := clients.rest.Client().Get(source.URL + "/rest")
72+
require.NoError(t, err)
73+
resp.Body.Close()
74+
assert.NotEmpty(t, sourceAuth, "REST request must authenticate to the configured host")
75+
assert.Empty(t, foreignAuth, "REST redirect must not authenticate to a foreign host")
76+
77+
sourceAuth = ""
78+
foreignAuth = ""
79+
resp, err = clients.raw.GetRawContent(context.Background(), "owner", "repo", "file", nil)
80+
require.NoError(t, err)
81+
resp.Body.Close()
82+
assert.NotEmpty(t, sourceAuth, "raw request must authenticate to the configured host")
83+
assert.Empty(t, foreignAuth, "raw redirect must not authenticate to a foreign host")
84+
})
85+
}
86+
}
87+
88+
type staticAPIHostResolver struct {
89+
restURL *url.URL
90+
graphQLURL *url.URL
91+
uploadURL *url.URL
92+
rawURL *url.URL
93+
}
94+
95+
func newStaticAPIHostResolver(t *testing.T, endpoint string) staticAPIHostResolver {
96+
t.Helper()
97+
98+
u, err := url.Parse(endpoint)
99+
require.NoError(t, err)
100+
return staticAPIHostResolver{
101+
restURL: u,
102+
graphQLURL: u,
103+
uploadURL: u,
104+
rawURL: u,
105+
}
106+
}
107+
108+
func (r staticAPIHostResolver) BaseRESTURL(context.Context) (*url.URL, error) {
109+
return r.restURL, nil
110+
}
111+
112+
func (r staticAPIHostResolver) GraphqlURL(context.Context) (*url.URL, error) {
113+
return r.graphQLURL, nil
114+
}
115+
116+
func (r staticAPIHostResolver) UploadURL(context.Context) (*url.URL, error) {
117+
return r.uploadURL, nil
118+
}
119+
120+
func (r staticAPIHostResolver) RawURL(context.Context) (*url.URL, error) {
121+
return r.rawURL, nil
122+
}
123+
124+
func (r staticAPIHostResolver) AuthorizationServerURL(context.Context) (*url.URL, error) {
125+
return r.restURL, nil
126+
}
127+
26128
// probeToolName is the name of the throwaway tool the harness registers; its
27129
// handler runs a probe closure against a sessionPrompter so the adapter can be
28130
// exercised against a real, fully-negotiated server session from the client side.
@@ -583,8 +685,7 @@ func TestCreateGitHubClientsTokenProvider(t *testing.T) {
583685
defer server.Close()
584686

585687
current := ""
586-
apiHost, err := utils.NewAPIHost(server.URL)
587-
require.NoError(t, err)
688+
apiHost := newStaticAPIHostResolver(t, server.URL)
588689

589690
clients, err := createGitHubClients(github.MCPServerConfig{
590691
Version: "test",

‎internal/ghmcp/server.go‎

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -67,37 +67,28 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv
6767
// response that redirects off them does not carry the token to the redirect
6868
// target. See transport.BearerAuthTransport.
6969
allowedHosts := []string{
70-
restURL.Hostname(),
71-
uploadURL.Hostname(),
72-
graphQLURL.Hostname(),
73-
rawURL.Hostname(),
70+
restURL.Host,
71+
uploadURL.Host,
72+
graphQLURL.Host,
73+
rawURL.Host,
7474
}
7575

76-
// Construct REST client. When a TokenProvider is configured, we
77-
// authenticate via BearerAuthTransport and skip go-github's WithAuthToken:
78-
// the latter installs its own round tripper that would pin the static token
79-
// and shadow the dynamic one.
76+
// Construct REST client. BearerAuthTransport handles both static and
77+
// provider-backed tokens so every authentication mode uses the same host
78+
// restrictions.
8079
restUATransport := &transport.UserAgentTransport{
8180
Transport: http.DefaultTransport,
8281
Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version),
8382
}
84-
var restClient *gogithub.Client
85-
if cfg.TokenProvider != nil {
86-
restClient, err = gogithub.NewClient(
87-
gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{
88-
Transport: restUATransport,
89-
TokenProvider: cfg.TokenProvider,
90-
AllowedHosts: allowedHosts,
91-
}}),
92-
gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()),
93-
)
94-
} else {
95-
restClient, err = gogithub.NewClient(
96-
gogithub.WithHTTPClient(&http.Client{Transport: restUATransport}),
97-
gogithub.WithAuthToken(cfg.Token),
98-
gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()),
99-
)
100-
}
83+
restClient, err := gogithub.NewClient(
84+
gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{
85+
Transport: restUATransport,
86+
Token: cfg.Token,
87+
TokenProvider: cfg.TokenProvider,
88+
AllowedHosts: allowedHosts,
89+
}}),
90+
gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()),
91+
)
10192
if err != nil {
10293
return nil, fmt.Errorf("failed to create REST client: %w", err)
10394
}

‎pkg/github/dependencies.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,10 +330,29 @@ func (d *RequestDeps) GetClient(ctx context.Context) (*gogithub.Client, error) {
330330
if err != nil {
331331
return nil, fmt.Errorf("failed to get upload URL: %w", err)
332332
}
333+
graphqlURL, err := d.apiHosts.GraphqlURL(ctx)
334+
if err != nil {
335+
return nil, fmt.Errorf("failed to get GraphQL URL: %w", err)
336+
}
337+
rawURL, err := d.apiHosts.RawURL(ctx)
338+
if err != nil {
339+
return nil, fmt.Errorf("failed to get Raw URL: %w", err)
340+
}
341+
342+
allowedHosts := []string{
343+
baseRestURL.Host,
344+
uploadURL.Host,
345+
graphqlURL.Host,
346+
rawURL.Host,
347+
}
333348

334349
// Construct REST client
335350
restClient, err := gogithub.NewClient(
336-
gogithub.WithAuthToken(token),
351+
gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{
352+
Transport: http.DefaultTransport,
353+
Token: token,
354+
AllowedHosts: allowedHosts,
355+
}}),
337356
gogithub.WithUserAgent(fmt.Sprintf("github-mcp-server/%s", d.version)),
338357
gogithub.WithEnterpriseURLs(baseRestURL.String(), uploadURL.String()),
339358
)
@@ -376,10 +395,10 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error
376395
// response that redirects off them does not carry the token to the redirect
377396
// target. See transport.BearerAuthTransport.
378397
allowedHosts := []string{
379-
baseRestURL.Hostname(),
380-
uploadURL.Hostname(),
381-
graphqlURL.Hostname(),
382-
rawURL.Hostname(),
398+
baseRestURL.Host,
399+
uploadURL.Host,
400+
graphqlURL.Host,
401+
rawURL.Host,
383402
}
384403

385404
// Construct GraphQL client

‎pkg/github/dependencies_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,124 @@ import (
44
"context"
55
"errors"
66
"log/slog"
7+
"net/http"
8+
"net/http/httptest"
9+
"net/url"
710
"testing"
811

12+
ghcontext "github.com/github/github-mcp-server/pkg/context"
913
"github.com/github/github-mcp-server/pkg/github"
14+
"github.com/github/github-mcp-server/pkg/http/headers"
1015
"github.com/github/github-mcp-server/pkg/observability"
1116
"github.com/github/github-mcp-server/pkg/observability/metrics"
1217
"github.com/github/github-mcp-server/pkg/translations"
18+
"github.com/shurcooL/githubv4"
1319
"github.com/stretchr/testify/assert"
20+
"github.com/stretchr/testify/require"
1421
)
1522

1623
func testExporters() observability.Exporters {
1724
obs, _ := observability.NewExporters(slog.New(slog.DiscardHandler), metrics.NewNoopMetrics())
1825
return obs
1926
}
2027

28+
type requestDepsAPIHostResolver struct {
29+
endpoint *url.URL
30+
}
31+
32+
func newRequestDepsAPIHostResolver(t *testing.T, endpoint string) requestDepsAPIHostResolver {
33+
t.Helper()
34+
35+
u, err := url.Parse(endpoint)
36+
require.NoError(t, err)
37+
return requestDepsAPIHostResolver{endpoint: u}
38+
}
39+
40+
func (r requestDepsAPIHostResolver) BaseRESTURL(context.Context) (*url.URL, error) {
41+
return r.endpoint, nil
42+
}
43+
44+
func (r requestDepsAPIHostResolver) GraphqlURL(context.Context) (*url.URL, error) {
45+
return r.endpoint, nil
46+
}
47+
48+
func (r requestDepsAPIHostResolver) UploadURL(context.Context) (*url.URL, error) {
49+
return r.endpoint, nil
50+
}
51+
52+
func (r requestDepsAPIHostResolver) RawURL(context.Context) (*url.URL, error) {
53+
return r.endpoint, nil
54+
}
55+
56+
func (r requestDepsAPIHostResolver) AuthorizationServerURL(context.Context) (*url.URL, error) {
57+
return r.endpoint, nil
58+
}
59+
60+
func TestRequestDepsScopesTokensToConfiguredHosts(t *testing.T) {
61+
t.Parallel()
62+
63+
var foreignAuth string
64+
foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
65+
foreignAuth = r.Header.Get(headers.AuthorizationHeader)
66+
w.Header().Set(headers.ContentTypeHeader, headers.ContentTypeJSON)
67+
_, _ = w.Write([]byte(`{"data":{"viewer":{"login":"octocat"}}}`))
68+
}))
69+
defer foreign.Close()
70+
71+
var sourceAuth string
72+
source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
73+
sourceAuth = r.Header.Get(headers.AuthorizationHeader)
74+
http.Redirect(w, r, foreign.URL, http.StatusFound)
75+
}))
76+
defer source.Close()
77+
78+
deps := github.NewRequestDeps(
79+
newRequestDepsAPIHostResolver(t, source.URL),
80+
"test",
81+
false,
82+
nil,
83+
translations.NullTranslationHelper,
84+
0,
85+
nil,
86+
testExporters(),
87+
)
88+
ctx := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "request-token"})
89+
90+
sourceAuth = ""
91+
foreignAuth = ""
92+
restClient, err := deps.GetClient(ctx)
93+
require.NoError(t, err)
94+
resp, err := restClient.Client().Get(source.URL + "/rest")
95+
require.NoError(t, err)
96+
resp.Body.Close()
97+
assert.NotEmpty(t, sourceAuth, "REST request must authenticate to the configured host")
98+
assert.Empty(t, foreignAuth, "REST redirect must not authenticate to a foreign host")
99+
100+
sourceAuth = ""
101+
foreignAuth = ""
102+
rawClient, err := deps.GetRawClient(ctx)
103+
require.NoError(t, err)
104+
resp, err = rawClient.GetRawContent(ctx, "owner", "repo", "file", nil)
105+
require.NoError(t, err)
106+
resp.Body.Close()
107+
assert.NotEmpty(t, sourceAuth, "raw request must authenticate to the configured host")
108+
assert.Empty(t, foreignAuth, "raw redirect must not authenticate to a foreign host")
109+
110+
sourceAuth = ""
111+
foreignAuth = ""
112+
gqlClient, err := deps.GetGQLClient(ctx)
113+
require.NoError(t, err)
114+
var query struct {
115+
Viewer struct {
116+
Login githubv4.String
117+
}
118+
}
119+
err = gqlClient.Query(ctx, &query, nil)
120+
require.NoError(t, err)
121+
assert.NotEmpty(t, sourceAuth, "GraphQL request must authenticate to the configured host")
122+
assert.Empty(t, foreignAuth, "GraphQL redirect must not authenticate to a foreign host")
123+
}
124+
21125
func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) {
22126
t.Parallel()
23127

‎pkg/http/transport/bearer.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,10 @@ type BearerAuthTransport struct {
1818

1919
// AllowedHosts, when non-empty, restricts the hosts the Authorization
2020
// 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.
21+
// and port exactly match one of these entries (case-insensitive). This
22+
// scopes the credential to the configured GitHub hosts, so that if a
23+
// response redirects off them the token is not carried to the redirect
24+
// target.
2525
//
2626
// net/http strips a cross-host Authorization header when it follows a
2727
// redirect, but only for headers set on the initial request. This
@@ -39,7 +39,9 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro
3939
if t.TokenProvider != nil {
4040
token = t.TokenProvider()
4141
}
42-
if token != "" && t.hostAllowed(req.URL.Hostname()) {
42+
if !t.hostAllowed(req.URL.Host) {
43+
req.Header.Del(headers.AuthorizationHeader)
44+
} else if token != "" {
4345
req.Header.Set(headers.AuthorizationHeader, "Bearer "+token)
4446
}
4547

0 commit comments

Comments
 (0)