From 32b1c064c40eb52ef44c55f7aa600e9874cb93bc Mon Sep 17 00:00:00 2001 From: Syed Anas Mohiuddin <91664161+SyedAnas01@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:32:03 -0500 Subject: [PATCH 1/2] 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. --- internal/ghmcp/server.go | 12 +++++ pkg/github/dependencies.go | 35 +++++++++++--- pkg/http/transport/bearer.go | 32 ++++++++++++- pkg/http/transport/bearer_test.go | 79 +++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 7 deletions(-) diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index b407f30057..9c4be3f5e2 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -63,6 +63,16 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv return nil, fmt.Errorf("failed to get Raw URL: %w", err) } + // allowedHosts scopes the bearer token to the configured GitHub hosts, so a + // response that redirects off them does not carry the token to the redirect + // target. See transport.BearerAuthTransport. + allowedHosts := []string{ + restURL.Hostname(), + uploadURL.Hostname(), + graphQLURL.Hostname(), + rawURL.Hostname(), + } + // Construct REST client. When a TokenProvider is configured, we // authenticate via BearerAuthTransport and skip go-github's WithAuthToken: // 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 gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ Transport: restUATransport, TokenProvider: cfg.TokenProvider, + AllowedHosts: allowedHosts, }}), gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), ) @@ -100,6 +111,7 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv }, Token: cfg.Token, TokenProvider: cfg.TokenProvider, + AllowedHosts: allowedHosts, }, } diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 4f3a9446cc..1eb665eae3 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -355,6 +355,33 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error } token := tokenInfo.Token + baseRestURL, err := d.apiHosts.BaseRESTURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get base REST URL: %w", err) + } + uploadURL, err := d.apiHosts.UploadURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get upload URL: %w", err) + } + graphqlURL, err := d.apiHosts.GraphqlURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GraphQL URL: %w", err) + } + rawURL, err := d.apiHosts.RawURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get Raw URL: %w", err) + } + + // allowedHosts scopes the bearer token to the configured GitHub hosts, so a + // response that redirects off them does not carry the token to the redirect + // target. See transport.BearerAuthTransport. + allowedHosts := []string{ + baseRestURL.Hostname(), + uploadURL.Hostname(), + graphqlURL.Hostname(), + rawURL.Hostname(), + } + // Construct GraphQL client // We use NewEnterpriseClient unconditionally since we already parsed the API host // Wrap transport with GraphQLFeaturesTransport to inject feature flags from context, @@ -364,15 +391,11 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error Transport: &transport.GraphQLFeaturesTransport{ Transport: http.DefaultTransport, }, - Token: token, + Token: token, + AllowedHosts: allowedHosts, }, } - graphqlURL, err := d.apiHosts.GraphqlURL(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get GraphQL URL: %w", err) - } - gqlClient := githubv4.NewEnterpriseClient(graphqlURL.String(), gqlHTTPClient) return gqlClient, nil } diff --git a/pkg/http/transport/bearer.go b/pkg/http/transport/bearer.go index 6f2ae7fc98..210bfba06a 100644 --- a/pkg/http/transport/bearer.go +++ b/pkg/http/transport/bearer.go @@ -15,6 +15,22 @@ type BearerAuthTransport struct { // TokenProvider, when non-nil, supplies the bearer token for each request // and takes precedence over Token. TokenProvider func() string + + // AllowedHosts, when non-empty, restricts the hosts the Authorization + // header is attached to. The token is set only when the request host + // matches one of these entries (case-insensitive, host only, port + // ignored). This scopes the credential to the configured GitHub hosts, so + // that if a response redirects off them the token is not carried to the + // redirect target. + // + // net/http strips a cross-host Authorization header when it follows a + // redirect, but only for headers set on the initial request. This + // transport re-adds the header on every hop, so that protection does not + // otherwise apply here. + // + // When empty, the token is attached to every request, preserving the + // prior behavior. + AllowedHosts []string } func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -23,7 +39,7 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro if t.TokenProvider != nil { token = t.TokenProvider() } - if token != "" { + if token != "" && t.hostAllowed(req.URL.Hostname()) { req.Header.Set(headers.AuthorizationHeader, "Bearer "+token) } @@ -34,3 +50,17 @@ func (t *BearerAuthTransport) RoundTrip(req *http.Request) (*http.Response, erro return t.Transport.RoundTrip(req) } + +// hostAllowed reports whether the token may be attached to a request bound for +// host. An empty AllowedHosts allows all hosts, preserving prior behavior. +func (t *BearerAuthTransport) hostAllowed(host string) bool { + if len(t.AllowedHosts) == 0 { + return true + } + for _, h := range t.AllowedHosts { + if strings.EqualFold(h, host) { + return true + } + } + return false +} diff --git a/pkg/http/transport/bearer_test.go b/pkg/http/transport/bearer_test.go index 76ef8686cd..eac98b1cec 100644 --- a/pkg/http/transport/bearer_test.go +++ b/pkg/http/transport/bearer_test.go @@ -162,3 +162,82 @@ func TestBearerAuthTransport_DoesNotMutateOriginalRequest(t *testing.T) { assert.Empty(t, req.Header.Get(headers.AuthorizationHeader), "original request must not be mutated") } + +// hostRecordingTransport records the Authorization header seen for each request +// host, so a test can assert what the token would be attached to without a live +// network. It stands in for the real transport at the bottom of the chain. +type hostRecordingTransport struct { + authByHost map[string]string +} + +func (h *hostRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + h.authByHost[req.URL.Hostname()] = req.Header.Get(headers.AuthorizationHeader) + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + Header: make(http.Header), + Request: req, + }, nil +} + +// TestBearerAuthTransport_HostScoping verifies that when AllowedHosts is set, +// the token is attached to a request on an allowed host but withheld from a +// request to any other host. A redirect off the configured GitHub hosts arrives +// here as a RoundTrip to a different host, so this is the property that keeps +// the token from following such a redirect. net/http's own cross-host stripping +// does not cover it, because this transport re-adds the header on every hop. +// +// The hosts are distinct hostnames (matching the real case: api.github.com +// versus objects.githubusercontent.com) rather than two loopback servers on +// different ports, because AllowedHosts matches on hostname and ignores port. +func TestBearerAuthTransport_HostScoping(t *testing.T) { + t.Parallel() + + rec := &hostRecordingTransport{authByHost: map[string]string{}} + rt := &BearerAuthTransport{ + Transport: rec, + Token: "secret-token", + AllowedHosts: []string{"api.github.com", "raw.githubusercontent.com"}, + } + + for _, target := range []string{ + "https://api.github.com/repos/o/r", + "https://raw.githubusercontent.com/o/r/main/f", // allowed, different host + "https://objects.githubusercontent.com/evil", // redirect target, not allowed + "https://attacker.example.com/steal", // arbitrary host, not allowed + } { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, target, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + resp.Body.Close() + } + + assert.Equal(t, "Bearer secret-token", rec.authByHost["api.github.com"], + "token must be sent to an allowed host") + assert.Equal(t, "Bearer secret-token", rec.authByHost["raw.githubusercontent.com"], + "token must be sent to every allowed host") + assert.Empty(t, rec.authByHost["objects.githubusercontent.com"], + "token must not be sent to a non-allowed host (a redirect target)") + assert.Empty(t, rec.authByHost["attacker.example.com"], + "token must not be sent to an arbitrary non-allowed host") +} + +// TestBearerAuthTransport_EmptyAllowedHostsPreservesBehavior verifies the +// backward-compatible default: with no AllowedHosts, the token is attached to +// every host, exactly as before this change. +func TestBearerAuthTransport_EmptyAllowedHostsPreservesBehavior(t *testing.T) { + t.Parallel() + + rec := &hostRecordingTransport{authByHost: map[string]string{}} + rt := &BearerAuthTransport{Transport: rec, Token: "secret-token"} + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://anywhere.example.com/x", nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + resp.Body.Close() + + assert.Equal(t, "Bearer secret-token", rec.authByHost["anywhere.example.com"], + "with no AllowedHosts, token attaches to every host as before") +} From ca31ab4e4ca2e3c6067d6992867f8c53dfaca0de Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 15:41:17 +0200 Subject: [PATCH 2/2] 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> --- internal/ghmcp/oauth_test.go | 107 +++++++++++++++++++- internal/ghmcp/server.go | 41 +++----- pkg/github/dependencies.go | 29 +++++- pkg/github/dependencies_test.go | 104 +++++++++++++++++++ pkg/http/transport/bearer.go | 12 ++- pkg/http/transport/bearer_test.go | 160 +++++++++++++++++++++++++++++- 6 files changed, 411 insertions(+), 42 deletions(-) diff --git a/internal/ghmcp/oauth_test.go b/internal/ghmcp/oauth_test.go index b358876232..e5a29d6b27 100644 --- a/internal/ghmcp/oauth_test.go +++ b/internal/ghmcp/oauth_test.go @@ -7,12 +7,12 @@ import ( "log/slog" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/github/github-mcp-server/internal/oauth" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/headers" - "github.com/github/github-mcp-server/pkg/utils" "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" @@ -23,6 +23,108 @@ func discardLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } +func TestCreateGitHubClientsScopesRESTAndRawTokens(t *testing.T) { + t.Parallel() + + var foreignAuth string + foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + foreignAuth = r.Header.Get(headers.AuthorizationHeader) + w.WriteHeader(http.StatusOK) + })) + defer foreign.Close() + + var sourceAuth string + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sourceAuth = r.Header.Get(headers.AuthorizationHeader) + http.Redirect(w, r, foreign.URL, http.StatusFound) + })) + defer source.Close() + + tests := []struct { + name string + cfg github.MCPServerConfig + }{ + { + name: "static token", + cfg: github.MCPServerConfig{ + Version: "test", + Token: "static-token", + }, + }, + { + name: "token provider", + cfg: github.MCPServerConfig{ + Version: "test", + TokenProvider: func() string { return "provider-token" }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + apiHost := newStaticAPIHostResolver(t, source.URL) + clients, err := createGitHubClients(tt.cfg, apiHost) + require.NoError(t, err) + + sourceAuth = "" + foreignAuth = "" + resp, err := clients.rest.Client().Get(source.URL + "/rest") + require.NoError(t, err) + resp.Body.Close() + assert.NotEmpty(t, sourceAuth, "REST request must authenticate to the configured host") + assert.Empty(t, foreignAuth, "REST redirect must not authenticate to a foreign host") + + sourceAuth = "" + foreignAuth = "" + resp, err = clients.raw.GetRawContent(context.Background(), "owner", "repo", "file", nil) + require.NoError(t, err) + resp.Body.Close() + assert.NotEmpty(t, sourceAuth, "raw request must authenticate to the configured host") + assert.Empty(t, foreignAuth, "raw redirect must not authenticate to a foreign host") + }) + } +} + +type staticAPIHostResolver struct { + restURL *url.URL + graphQLURL *url.URL + uploadURL *url.URL + rawURL *url.URL +} + +func newStaticAPIHostResolver(t *testing.T, endpoint string) staticAPIHostResolver { + t.Helper() + + u, err := url.Parse(endpoint) + require.NoError(t, err) + return staticAPIHostResolver{ + restURL: u, + graphQLURL: u, + uploadURL: u, + rawURL: u, + } +} + +func (r staticAPIHostResolver) BaseRESTURL(context.Context) (*url.URL, error) { + return r.restURL, nil +} + +func (r staticAPIHostResolver) GraphqlURL(context.Context) (*url.URL, error) { + return r.graphQLURL, nil +} + +func (r staticAPIHostResolver) UploadURL(context.Context) (*url.URL, error) { + return r.uploadURL, nil +} + +func (r staticAPIHostResolver) RawURL(context.Context) (*url.URL, error) { + return r.rawURL, nil +} + +func (r staticAPIHostResolver) AuthorizationServerURL(context.Context) (*url.URL, error) { + return r.restURL, nil +} + // probeToolName is the name of the throwaway tool the harness registers; its // handler runs a probe closure against a sessionPrompter so the adapter can be // exercised against a real, fully-negotiated server session from the client side. @@ -583,8 +685,7 @@ func TestCreateGitHubClientsTokenProvider(t *testing.T) { defer server.Close() current := "" - apiHost, err := utils.NewAPIHost(server.URL) - require.NoError(t, err) + apiHost := newStaticAPIHostResolver(t, server.URL) clients, err := createGitHubClients(github.MCPServerConfig{ Version: "test", diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 9c4be3f5e2..e9dbbb9674 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -67,37 +67,28 @@ func createGitHubClients(cfg github.MCPServerConfig, apiHost utils.APIHostResolv // response that redirects off them does not carry the token to the redirect // target. See transport.BearerAuthTransport. allowedHosts := []string{ - restURL.Hostname(), - uploadURL.Hostname(), - graphQLURL.Hostname(), - rawURL.Hostname(), + restURL.Host, + uploadURL.Host, + graphQLURL.Host, + rawURL.Host, } - // Construct REST client. When a TokenProvider is configured, we - // authenticate via BearerAuthTransport and skip go-github's WithAuthToken: - // the latter installs its own round tripper that would pin the static token - // and shadow the dynamic one. + // Construct REST client. BearerAuthTransport handles both static and + // provider-backed tokens so every authentication mode uses the same host + // restrictions. restUATransport := &transport.UserAgentTransport{ Transport: http.DefaultTransport, Agent: fmt.Sprintf("github-mcp-server/%s", cfg.Version), } - var restClient *gogithub.Client - if cfg.TokenProvider != nil { - restClient, err = gogithub.NewClient( - gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ - Transport: restUATransport, - TokenProvider: cfg.TokenProvider, - AllowedHosts: allowedHosts, - }}), - gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), - ) - } else { - restClient, err = gogithub.NewClient( - gogithub.WithHTTPClient(&http.Client{Transport: restUATransport}), - gogithub.WithAuthToken(cfg.Token), - gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), - ) - } + restClient, err := gogithub.NewClient( + gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ + Transport: restUATransport, + Token: cfg.Token, + TokenProvider: cfg.TokenProvider, + AllowedHosts: allowedHosts, + }}), + gogithub.WithEnterpriseURLs(restURL.String(), uploadURL.String()), + ) if err != nil { return nil, fmt.Errorf("failed to create REST client: %w", err) } diff --git a/pkg/github/dependencies.go b/pkg/github/dependencies.go index 1eb665eae3..c13f248c56 100644 --- a/pkg/github/dependencies.go +++ b/pkg/github/dependencies.go @@ -330,10 +330,29 @@ func (d *RequestDeps) GetClient(ctx context.Context) (*gogithub.Client, error) { if err != nil { return nil, fmt.Errorf("failed to get upload URL: %w", err) } + graphqlURL, err := d.apiHosts.GraphqlURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GraphQL URL: %w", err) + } + rawURL, err := d.apiHosts.RawURL(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get Raw URL: %w", err) + } + + allowedHosts := []string{ + baseRestURL.Host, + uploadURL.Host, + graphqlURL.Host, + rawURL.Host, + } // Construct REST client restClient, err := gogithub.NewClient( - gogithub.WithAuthToken(token), + gogithub.WithHTTPClient(&http.Client{Transport: &transport.BearerAuthTransport{ + Transport: http.DefaultTransport, + Token: token, + AllowedHosts: allowedHosts, + }}), gogithub.WithUserAgent(fmt.Sprintf("github-mcp-server/%s", d.version)), gogithub.WithEnterpriseURLs(baseRestURL.String(), uploadURL.String()), ) @@ -376,10 +395,10 @@ func (d *RequestDeps) GetGQLClient(ctx context.Context) (*githubv4.Client, error // response that redirects off them does not carry the token to the redirect // target. See transport.BearerAuthTransport. allowedHosts := []string{ - baseRestURL.Hostname(), - uploadURL.Hostname(), - graphqlURL.Hostname(), - rawURL.Hostname(), + baseRestURL.Host, + uploadURL.Host, + graphqlURL.Host, + rawURL.Host, } // Construct GraphQL client diff --git a/pkg/github/dependencies_test.go b/pkg/github/dependencies_test.go index 1d747cae47..0ff3f3520a 100644 --- a/pkg/github/dependencies_test.go +++ b/pkg/github/dependencies_test.go @@ -4,13 +4,20 @@ import ( "context" "errors" "log/slog" + "net/http" + "net/http/httptest" + "net/url" "testing" + ghcontext "github.com/github/github-mcp-server/pkg/context" "github.com/github/github-mcp-server/pkg/github" + "github.com/github/github-mcp-server/pkg/http/headers" "github.com/github/github-mcp-server/pkg/observability" "github.com/github/github-mcp-server/pkg/observability/metrics" "github.com/github/github-mcp-server/pkg/translations" + "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func testExporters() observability.Exporters { @@ -18,6 +25,103 @@ func testExporters() observability.Exporters { return obs } +type requestDepsAPIHostResolver struct { + endpoint *url.URL +} + +func newRequestDepsAPIHostResolver(t *testing.T, endpoint string) requestDepsAPIHostResolver { + t.Helper() + + u, err := url.Parse(endpoint) + require.NoError(t, err) + return requestDepsAPIHostResolver{endpoint: u} +} + +func (r requestDepsAPIHostResolver) BaseRESTURL(context.Context) (*url.URL, error) { + return r.endpoint, nil +} + +func (r requestDepsAPIHostResolver) GraphqlURL(context.Context) (*url.URL, error) { + return r.endpoint, nil +} + +func (r requestDepsAPIHostResolver) UploadURL(context.Context) (*url.URL, error) { + return r.endpoint, nil +} + +func (r requestDepsAPIHostResolver) RawURL(context.Context) (*url.URL, error) { + return r.endpoint, nil +} + +func (r requestDepsAPIHostResolver) AuthorizationServerURL(context.Context) (*url.URL, error) { + return r.endpoint, nil +} + +func TestRequestDepsScopesTokensToConfiguredHosts(t *testing.T) { + t.Parallel() + + var foreignAuth string + foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + foreignAuth = r.Header.Get(headers.AuthorizationHeader) + w.Header().Set(headers.ContentTypeHeader, headers.ContentTypeJSON) + _, _ = w.Write([]byte(`{"data":{"viewer":{"login":"octocat"}}}`)) + })) + defer foreign.Close() + + var sourceAuth string + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sourceAuth = r.Header.Get(headers.AuthorizationHeader) + http.Redirect(w, r, foreign.URL, http.StatusFound) + })) + defer source.Close() + + deps := github.NewRequestDeps( + newRequestDepsAPIHostResolver(t, source.URL), + "test", + false, + nil, + translations.NullTranslationHelper, + 0, + nil, + testExporters(), + ) + ctx := ghcontext.WithTokenInfo(context.Background(), &ghcontext.TokenInfo{Token: "request-token"}) + + sourceAuth = "" + foreignAuth = "" + restClient, err := deps.GetClient(ctx) + require.NoError(t, err) + resp, err := restClient.Client().Get(source.URL + "/rest") + require.NoError(t, err) + resp.Body.Close() + assert.NotEmpty(t, sourceAuth, "REST request must authenticate to the configured host") + assert.Empty(t, foreignAuth, "REST redirect must not authenticate to a foreign host") + + sourceAuth = "" + foreignAuth = "" + rawClient, err := deps.GetRawClient(ctx) + require.NoError(t, err) + resp, err = rawClient.GetRawContent(ctx, "owner", "repo", "file", nil) + require.NoError(t, err) + resp.Body.Close() + assert.NotEmpty(t, sourceAuth, "raw request must authenticate to the configured host") + assert.Empty(t, foreignAuth, "raw redirect must not authenticate to a foreign host") + + sourceAuth = "" + foreignAuth = "" + gqlClient, err := deps.GetGQLClient(ctx) + require.NoError(t, err) + var query struct { + Viewer struct { + Login githubv4.String + } + } + err = gqlClient.Query(ctx, &query, nil) + require.NoError(t, err) + assert.NotEmpty(t, sourceAuth, "GraphQL request must authenticate to the configured host") + assert.Empty(t, foreignAuth, "GraphQL redirect must not authenticate to a foreign host") +} + func TestIsFeatureEnabled_WithEnabledFlag(t *testing.T) { t.Parallel() diff --git a/pkg/http/transport/bearer.go b/pkg/http/transport/bearer.go index 210bfba06a..522f4c6753 100644 --- a/pkg/http/transport/bearer.go +++ b/pkg/http/transport/bearer.go @@ -18,10 +18,10 @@ type BearerAuthTransport struct { // AllowedHosts, when non-empty, restricts the hosts the Authorization // header is attached to. The token is set only when the request host - // matches one of these entries (case-insensitive, host only, port - // ignored). This scopes the credential to the configured GitHub hosts, so - // that if a response redirects off them the token is not carried to the - // redirect target. + // and port exactly match one of these entries (case-insensitive). This + // scopes the credential to the configured GitHub hosts, so that if a + // response redirects off them the token is not carried to the redirect + // target. // // net/http strips a cross-host Authorization header when it follows a // 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 if t.TokenProvider != nil { token = t.TokenProvider() } - if token != "" && t.hostAllowed(req.URL.Hostname()) { + if !t.hostAllowed(req.URL.Host) { + req.Header.Del(headers.AuthorizationHeader) + } else if token != "" { req.Header.Set(headers.AuthorizationHeader, "Bearer "+token) } diff --git a/pkg/http/transport/bearer_test.go b/pkg/http/transport/bearer_test.go index eac98b1cec..0bf3549fc5 100644 --- a/pkg/http/transport/bearer_test.go +++ b/pkg/http/transport/bearer_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "net/url" "testing" ghcontext "github.com/github/github-mcp-server/pkg/context" @@ -171,7 +172,7 @@ type hostRecordingTransport struct { } func (h *hostRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) { - h.authByHost[req.URL.Hostname()] = req.Header.Get(headers.AuthorizationHeader) + h.authByHost[req.URL.Host] = req.Header.Get(headers.AuthorizationHeader) return &http.Response{ StatusCode: http.StatusOK, Body: http.NoBody, @@ -187,9 +188,8 @@ func (h *hostRecordingTransport) RoundTrip(req *http.Request) (*http.Response, e // the token from following such a redirect. net/http's own cross-host stripping // does not cover it, because this transport re-adds the header on every hop. // -// The hosts are distinct hostnames (matching the real case: api.github.com -// versus objects.githubusercontent.com) rather than two loopback servers on -// different ports, because AllowedHosts matches on hostname and ignores port. +// The hosts are distinct hostnames, matching the real case of api.github.com +// versus objects.githubusercontent.com. func TestBearerAuthTransport_HostScoping(t *testing.T) { t.Parallel() @@ -241,3 +241,155 @@ func TestBearerAuthTransport_EmptyAllowedHostsPreservesBehavior(t *testing.T) { assert.Equal(t, "Bearer secret-token", rec.authByHost["anywhere.example.com"], "with no AllowedHosts, token attaches to every host as before") } + +func TestBearerAuthTransport_ExactHostScoping(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + allowedHosts []string + target string + wantAuth bool + }{ + { + name: "host comparison is case insensitive", + allowedHosts: []string{"API.GITHUB.COM"}, + target: "https://api.github.com/repos/o/r", + wantAuth: true, + }, + { + name: "configured port", + allowedHosts: []string{"github.example.com:8443"}, + target: "https://GITHUB.EXAMPLE.COM:8443/api/v3/repos/o/r", + wantAuth: true, + }, + { + name: "different port", + allowedHosts: []string{"github.example.com:8443"}, + target: "https://github.example.com:9443/api/v3/repos/o/r", + }, + { + name: "explicit default port is not implicitly configured", + allowedHosts: []string{"api.github.com"}, + target: "https://api.github.com:443/repos/o/r", + }, + { + name: "subdomain lookalike", + allowedHosts: []string{"api.github.com"}, + target: "https://evil.api.github.com/steal", + }, + { + name: "suffix lookalike", + allowedHosts: []string{"api.github.com"}, + target: "https://api.github.com.attacker.example/steal", + }, + { + name: "userinfo lookalike", + allowedHosts: []string{"api.github.com"}, + target: "https://api.github.com@attacker.example/steal", + }, + { + name: "trailing dot is not an exact host", + allowedHosts: []string{"api.github.com"}, + target: "https://api.github.com./steal", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := &hostRecordingTransport{authByHost: map[string]string{}} + rt := &BearerAuthTransport{ + Transport: rec, + Token: "secret-token", + AllowedHosts: tt.allowedHosts, + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, tt.target, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + resp.Body.Close() + + if tt.wantAuth { + assert.NotEmpty(t, rec.authByHost[req.URL.Host]) + } else { + assert.Empty(t, rec.authByHost[req.URL.Host]) + } + }) + } +} + +func TestBearerAuthTransport_RedirectHostScoping(t *testing.T) { + t.Parallel() + + var allowedAuth string + allowedTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + allowedAuth = r.Header.Get(headers.AuthorizationHeader) + w.WriteHeader(http.StatusOK) + })) + defer allowedTarget.Close() + + var foreignAuth string + foreignTarget := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + foreignAuth = r.Header.Get(headers.AuthorizationHeader) + w.WriteHeader(http.StatusOK) + })) + defer foreignTarget.Close() + + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/allowed": + http.Redirect(w, r, allowedTarget.URL, http.StatusFound) + case "/foreign": + http.Redirect(w, r, foreignTarget.URL, http.StatusFound) + default: + http.NotFound(w, r) + } + })) + defer source.Close() + + sourceURL, err := url.Parse(source.URL) + require.NoError(t, err) + allowedTargetURL, err := url.Parse(allowedTarget.URL) + require.NoError(t, err) + + client := &http.Client{Transport: &BearerAuthTransport{ + Transport: http.DefaultTransport, + Token: "secret-token", + AllowedHosts: []string{sourceURL.Host, allowedTargetURL.Host}, + }} + + resp, err := client.Get(source.URL + "/allowed") + require.NoError(t, err) + resp.Body.Close() + assert.NotEmpty(t, allowedAuth, "token must be sent to a configured redirect host") + + resp, err = client.Get(source.URL + "/foreign") + require.NoError(t, err) + resp.Body.Close() + assert.Empty(t, foreignAuth, "token must not be sent to an unconfigured redirect host") +} + +func TestBearerAuthTransport_RemovesAuthorizationFromDisallowedHost(t *testing.T) { + t.Parallel() + + rec := &hostRecordingTransport{authByHost: map[string]string{}} + rt := &BearerAuthTransport{ + Transport: rec, + Token: "secret-token", + AllowedHosts: []string{"api.github.com"}, + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://attacker.example/steal", nil) + require.NoError(t, err) + req.Header.Set(headers.AuthorizationHeader, "caller-supplied-token") + + resp, err := rt.RoundTrip(req) + require.NoError(t, err) + resp.Body.Close() + + assert.Empty(t, rec.authByHost[req.URL.Host]) + assert.NotEmpty(t, req.Header.Get(headers.AuthorizationHeader), "original request must not be mutated") +}