diff --git a/auth/authorization_code.go b/auth/authorization_code.go index 5b60438bc..baf8a6d0f 100644 --- a/auth/authorization_code.go +++ b/auth/authorization_code.go @@ -146,6 +146,9 @@ type AuthorizationCodeHandlerConfig struct { // such as SSRF protections, see // https://modelcontextprotocol.io/docs/tutorials/security/security_best_practices#server-side-request-forgery-ssrf // If not provided, http.DefaultClient will be used. + // Unless Client sets CheckRedirect, the registration and token requests + // follow a redirect only within the origin of the request and only if it + // keeps the method. Client *http.Client // NewTokenSource is an optional function that can be set to construct the @@ -736,7 +739,8 @@ func (h *AuthorizationCodeHandler) exchangeAuthorizationCode(ctx context.Context oauth2.VerifierOption(authResult.usedCodeVerifier), oauth2.SetAuthURLParam("resource", resourceURL), } - clientCtx := context.WithValue(ctx, oauth2.HTTPClient, h.config.Client) + client := util.RestrictRedirects(h.config.Client) + clientCtx := context.WithValue(ctx, oauth2.HTTPClient, client) token, err := cfg.Exchange(clientCtx, authResult.Code, opts...) if err != nil { return fmt.Errorf("token exchange failed: %w", err) @@ -750,7 +754,7 @@ func (h *AuthorizationCodeHandler) exchangeAuthorizationCode(ctx context.Context // once that request (or the connect operation that triggered authorization) // completes. Use a background context that still carries the configured HTTP // client so refreshes keep working for the life of the token source. - refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, h.config.Client) + refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, client) var ts oauth2.TokenSource if h.config.NewTokenSource == nil { if token.RefreshToken == "" { diff --git a/auth/authorization_code_test.go b/auth/authorization_code_test.go index 97002aa7e..b29afda79 100644 --- a/auth/authorization_code_test.go +++ b/auth/authorization_code_test.go @@ -13,6 +13,7 @@ import ( "net/url" "slices" "strings" + "sync/atomic" "testing" "time" @@ -1814,6 +1815,61 @@ func TestNewTokenSource(t *testing.T) { } } +func TestExchangeAuthorizationCode_CrossOriginRedirect(t *testing.T) { + var targetHits atomic.Int32 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + })) + defer target.Close() + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + t.Errorf("ParseForm: %v", err) + } + if r.Form.Get("grant_type") == "refresh_token" || r.Form.Get("code") == "redirect" { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + return + } + // expires_in is below oauth2's expiry delta, so the first Token call refreshes. + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"access_token": "a", "token_type": "bearer", "refresh_token": "r", "expires_in": 1}`)) + })) + defer tokenServer.Close() + + handler, err := NewAuthorizationCodeHandler(validConfig()) + if err != nil { + t.Fatal(err) + } + cfg := &oauth2.Config{ + ClientID: "test_client", + Endpoint: oauth2.Endpoint{TokenURL: tokenServer.URL, AuthStyle: oauth2.AuthStyleInParams}, + } + exchange := func(code string) error { + return handler.exchangeAuthorizationCode(t.Context(), cfg, &authResult{ + AuthorizationResult: &AuthorizationResult{Code: code}, + usedCodeVerifier: "verifier", + }, "https://example.com/resource") + } + + if err := exchange("redirect"); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("code exchange error = %v, want redirect not followed", err) + } + + if err := exchange("ok"); err != nil { + t.Fatalf("code exchange failed: %v", err) + } + ts, err := handler.TokenSource(t.Context()) + if err != nil { + t.Fatal(err) + } + if _, err := ts.Token(); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("token refresh error = %v, want redirect not followed", err) + } + + if n := targetHits.Load(); n != 0 { + t.Errorf("redirect target got %d requests, want 0", n) + } +} + func TestInitialTokenSource(t *testing.T) { handler, err := NewAuthorizationCodeHandler(&AuthorizationCodeHandlerConfig{ RedirectURL: "http://localhost:12345/callback", diff --git a/auth/extauth/client_credentials.go b/auth/extauth/client_credentials.go index 42351ad6b..29df44268 100644 --- a/auth/extauth/client_credentials.go +++ b/auth/extauth/client_credentials.go @@ -16,6 +16,7 @@ import ( "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/internal/authutil" + "github.com/modelcontextprotocol/go-sdk/internal/util" "github.com/modelcontextprotocol/go-sdk/oauthex" "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" @@ -30,6 +31,8 @@ type ClientCredentialsHandlerConfig struct { // HTTPClient is an optional HTTP client for customization. // If nil, http.DefaultClient is used. + // Unless HTTPClient sets CheckRedirect, token requests follow a redirect + // only within the origin of the request and only if it keeps the method. // OPTIONAL. HTTPClient *http.Client } @@ -163,7 +166,8 @@ func (h *ClientCredentialsHandler) Authorize(ctx context.Context, req *http.Requ AuthStyle: selectTokenAuthMethod(asm.TokenEndpointAuthMethodsSupported), } - ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, httpClient) + client := util.RestrictRedirects(httpClient) + ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, client) tok, err := cfg.Token(ctxWithClient) if err != nil { h.mu.Lock() @@ -177,7 +181,7 @@ func (h *ClientCredentialsHandler) Authorize(ctx context.Context, req *http.Requ // ctx would make every request after the first token expires fail with // "context canceled". Use a background context that still carries the HTTP // client, as AuthorizationCodeHandler does. - refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, httpClient) + refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, client) h.mu.Lock() defer h.mu.Unlock() h.tokenSource = oauth2.ReuseTokenSource(tok, cfg.TokenSource(refreshCtx)) diff --git a/auth/extauth/client_credentials_test.go b/auth/extauth/client_credentials_test.go index 248db0036..a278f219e 100644 --- a/auth/extauth/client_credentials_test.go +++ b/auth/extauth/client_credentials_test.go @@ -6,10 +6,12 @@ package extauth import ( "context" + "fmt" "net/http" "net/http/httptest" "strings" "sync" + "sync/atomic" "testing" "github.com/google/go-cmp/cmp" @@ -500,6 +502,45 @@ func TestClientCredentialsHandler_ConcurrentAuthorize(t *testing.T) { wg.Wait() } +func TestClientCredentialsHandler_TokenRedirectToOtherOrigin(t *testing.T) { + var targetHits atomic.Int32 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + })) + t.Cleanup(target.Close) + + // Without protected resource metadata, the resource server is also its + // own authorization server. + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, r *http.Request) { + issuer := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"issuer": %q, "token_endpoint": %q, "code_challenge_methods_supported": ["S256"]}`, issuer, issuer+"/token") + }) + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + handler, err := NewClientCredentialsHandler(validClientCredentialsConfig()) + if err != nil { + t.Fatal(err) + } + resp := &http.Response{ + StatusCode: http.StatusUnauthorized, + Header: http.Header{}, + Body: http.NoBody, + } + req := httptest.NewRequest("GET", server.URL+"/resource", nil) + if err := handler.Authorize(t.Context(), req, resp); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("Authorize() error = %v, want redirect not followed", err) + } + if n := targetHits.Load(); n != 0 { + t.Errorf("redirect target got %d requests, want 0", n) + } +} + func TestSelectTokenAuthMethod(t *testing.T) { tests := []struct { name string diff --git a/auth/extauth/enterprise_handler.go b/auth/extauth/enterprise_handler.go index c5d4f12b6..a62c9c992 100644 --- a/auth/extauth/enterprise_handler.go +++ b/auth/extauth/enterprise_handler.go @@ -16,6 +16,7 @@ import ( "sync" "github.com/modelcontextprotocol/go-sdk/auth" + "github.com/modelcontextprotocol/go-sdk/internal/util" "github.com/modelcontextprotocol/go-sdk/oauthex" "golang.org/x/oauth2" ) @@ -75,6 +76,8 @@ type EnterpriseHandlerConfig struct { // HTTPClient is an optional HTTP client for customization. // If nil, http.DefaultClient is used. + // Unless HTTPClient sets CheckRedirect, token requests follow a redirect + // only within the origin of the request and only if it keeps the method. // OPTIONAL. HTTPClient *http.Client } @@ -243,10 +246,7 @@ func exchangeJWTBearer( cfg.ClientSecret = clientCreds.ClientSecretAuth.ClientSecret } - if httpClient == nil { - httpClient = http.DefaultClient - } - ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, httpClient) + ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, util.RestrictRedirects(httpClient)) token, err := cfg.Exchange( ctxWithClient, diff --git a/auth/extauth/oidc_login.go b/auth/extauth/oidc_login.go index d9d259bdf..cf5d92721 100644 --- a/auth/extauth/oidc_login.go +++ b/auth/extauth/oidc_login.go @@ -18,6 +18,7 @@ import ( "strings" "github.com/modelcontextprotocol/go-sdk/auth" + "github.com/modelcontextprotocol/go-sdk/internal/util" "github.com/modelcontextprotocol/go-sdk/oauthex" "golang.org/x/oauth2" ) @@ -49,6 +50,8 @@ type OIDCLoginConfig struct { LoginHint string // HTTPClient is the HTTP client for making requests. // If nil, http.DefaultClient is used. + // Unless HTTPClient sets CheckRedirect, token requests follow a redirect + // only within the origin of the request and only if it keeps the method. // OPTIONAL. HTTPClient *http.Client } @@ -196,11 +199,7 @@ func completeOIDCLogin( return nil, fmt.Errorf("codeVerifier is required") } - httpClient := config.HTTPClient - if httpClient == nil { - httpClient = http.DefaultClient - } - ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, httpClient) + ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, util.RestrictRedirects(config.HTTPClient)) oauth2Token, err := oauth2Config.Exchange( ctxWithClient, diff --git a/docs/mcpgodebug.md b/docs/mcpgodebug.md index 9e39a9e99..06184ce6c 100644 --- a/docs/mcpgodebug.md +++ b/docs/mcpgodebug.md @@ -61,6 +61,15 @@ Options listed below were added and will be removed in the 1.11.0 version of the fails, restoring the previous behavior. By default, the failure is returned only to the affected call. +- `followallredirects` added. If set to `1`, the requests of + `StreamableClientTransport` and `SSEClientTransport`, and the OAuth token and + dynamic client registration requests, follow redirects as before: to any + origin, and changing the method as `net/http` does. The default behavior was + changed so that, unless the `http.Client` in use sets `CheckRedirect`, a + redirect is followed only if it stays within the origin of the request (an + upgrade from `http` to `https` on the same host with default ports is + allowed) and keeps the method. + ### 1.8.0 Options listed below were added and will be removed in the 1.9.0 version of the SDK. diff --git a/internal/docs/mcpgodebug.src.md b/internal/docs/mcpgodebug.src.md index c99528274..e8f89c75d 100644 --- a/internal/docs/mcpgodebug.src.md +++ b/internal/docs/mcpgodebug.src.md @@ -60,6 +60,15 @@ Options listed below were added and will be removed in the 1.11.0 version of the fails, restoring the previous behavior. By default, the failure is returned only to the affected call. +- `followallredirects` added. If set to `1`, the requests of + `StreamableClientTransport` and `SSEClientTransport`, and the OAuth token and + dynamic client registration requests, follow redirects as before: to any + origin, and changing the method as `net/http` does. The default behavior was + changed so that, unless the `http.Client` in use sets `CheckRedirect`, a + redirect is followed only if it stays within the origin of the request (an + upgrade from `http` to `https` on the same host with default ports is + allowed) and keeps the method. + ### 1.8.0 Options listed below were added and will be removed in the 1.9.0 version of the SDK. diff --git a/internal/util/redirect.go b/internal/util/redirect.go new file mode 100644 index 000000000..22c5e5ac8 --- /dev/null +++ b/internal/util/redirect.go @@ -0,0 +1,93 @@ +// Copyright 2026 The Go MCP SDK Authors. All rights reserved. +// Use of this source code is governed by the license +// that can be found in the LICENSE file. + +package util + +import ( + "fmt" + "net/http" + "net/url" + "strings" + + "github.com/modelcontextprotocol/go-sdk/internal/mcpgodebug" +) + +// followallredirects is a compatibility parameter that restores the previous +// behavior of following every redirect that net/http follows by default. +// See the documentation for the mcpgodebug package for instructions how to enable it. +// The option will be removed in the 1.11.0 version of the SDK. +// +// Mutable for testing. +var followallredirects = mcpgodebug.Value("followallredirects") + +// maxRedirects is the limit that net/http applies when CheckRedirect is nil. +const maxRedirects = 10 + +// RestrictRedirects returns a copy of c, or of [http.DefaultClient] if c is +// nil, that stops after 10 requests, as net/http does by default, and follows +// only redirects that stay within the origin of the request they redirect and +// keep the method of the original request. Otherwise a server could reroute a request, +// with its headers and body, to another origin, or turn a POST into a +// bodyless GET. An upgrade from http to https on the same host, with default +// ports, stays within the origin. +// +// It returns c (or [http.DefaultClient]) unchanged if c sets its own +// CheckRedirect, which then decides alone, or if the followallredirects +// MCPGODEBUG option is set. +func RestrictRedirects(c *http.Client) *http.Client { + if c == nil { + c = http.DefaultClient + } + if c.CheckRedirect != nil || followallredirects == "1" { + return c + } + restricted := *c + restricted.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) >= maxRedirects { + return fmt.Errorf("stopped after %d redirects", maxRedirects) + } + return checkRedirectWithinOrigin(req, via) + } + return &restricted +} + +func checkRedirectWithinOrigin(req *http.Request, via []*http.Request) error { + if len(via) == 0 { + return nil + } + if prev := via[len(via)-1].URL; !withinOrigin(prev, req.URL) { + return fmt.Errorf("redirect not followed: target is outside the origin %s://%s of the request", prev.Scheme, prev.Host) + } + if req.Method != via[0].Method { + return fmt.Errorf("redirect not followed: it changes the method from %s to %s", via[0].Method, req.Method) + } + if req.URL.User != nil && req.URL.User.String() != via[len(via)-1].URL.User.String() { + return fmt.Errorf("redirect not followed: target adds or changes userinfo") + } + return nil +} + +func withinOrigin(from, to *url.URL) bool { + if !strings.EqualFold(from.Hostname(), to.Hostname()) { + return false + } + fromPort, toPort := portOrDefault(from), portOrDefault(to) + if from.Scheme == to.Scheme && fromPort == toPort { + return true + } + return from.Scheme == "http" && fromPort == "80" && to.Scheme == "https" && toPort == "443" +} + +func portOrDefault(u *url.URL) string { + if p := u.Port(); p != "" { + return p + } + switch u.Scheme { + case "http": + return "80" + case "https": + return "443" + } + return "" +} diff --git a/internal/util/redirect_test.go b/internal/util/redirect_test.go new file mode 100644 index 000000000..74fe14939 --- /dev/null +++ b/internal/util/redirect_test.go @@ -0,0 +1,169 @@ +// Copyright 2026 The Go MCP SDK Authors. All rights reserved. +// Use of this source code is governed by the license +// that can be found in the LICENSE file. + +package util + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +func TestCheckRedirectWithinOrigin(t *testing.T) { + newRequest := func(method, u string) *http.Request { + t.Helper() + r, err := http.NewRequest(method, u, nil) + if err != nil { + t.Fatal(err) + } + return r + } + get := func(u string) *http.Request { return newRequest(http.MethodGet, u) } + post := func(u string) *http.Request { return newRequest(http.MethodPost, u) } + + tests := []struct { + name string + via []*http.Request + req *http.Request + wantErr bool + }{ + {"same origin", []*http.Request{post("https://example.com/mcp")}, post("https://example.com/other"), false}, + {"host case", []*http.Request{post("https://example.com/mcp")}, post("https://EXAMPLE.com/mcp"), false}, + {"explicit default port", []*http.Request{post("http://example.com:80/mcp")}, post("http://example.com/mcp"), false}, + {"upgrade to https", []*http.Request{post("http://example.com/mcp")}, post("https://example.com/mcp"), false}, + {"other host", []*http.Request{post("https://example.com/mcp")}, post("https://evil.example/mcp"), true}, + {"subdomain", []*http.Request{post("https://example.com/mcp")}, post("https://www.example.com/mcp"), true}, + {"other port", []*http.Request{post("https://example.com/mcp")}, post("https://example.com:8443/mcp"), true}, + {"downgrade to http", []*http.Request{post("https://example.com/mcp")}, post("http://example.com/mcp"), true}, + {"upgrade from non-default port", []*http.Request{post("http://example.com:8080/mcp")}, post("https://example.com/mcp"), true}, + {"method change", []*http.Request{post("https://example.com/mcp")}, get("https://example.com/mcp"), true}, + {"get stays get", []*http.Request{get("https://example.com/mcp")}, get("https://example.com/other"), false}, + { + // Compared with the request it redirects, not the first one. + name: "downgrade after upgrade", + via: []*http.Request{get("http://example.com/a"), get("https://example.com/b")}, + req: get("http://example.com/c"), + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkRedirectWithinOrigin(tt.req, tt.via) + if (err != nil) != tt.wantErr { + t.Errorf("checkRedirectWithinOrigin(%s %s) = %v, want error: %t", tt.req.Method, tt.req.URL, err, tt.wantErr) + } + }) + } +} + +func TestRestrictRedirects(t *testing.T) { + t.Run("nil", func(t *testing.T) { + c := RestrictRedirects(nil) + if c == http.DefaultClient || c.CheckRedirect == nil { + t.Fatalf("RestrictRedirects(nil) = %+v, want a copy of http.DefaultClient with CheckRedirect", c) + } + if http.DefaultClient.CheckRedirect != nil { + t.Fatal("RestrictRedirects(nil) modified http.DefaultClient") + } + }) + t.Run("copy", func(t *testing.T) { + orig := &http.Client{Timeout: time.Minute} + c := RestrictRedirects(orig) + if c == orig || c.CheckRedirect == nil || c.Timeout != time.Minute { + t.Fatalf("RestrictRedirects() = %+v, want a copy with CheckRedirect", c) + } + if orig.CheckRedirect != nil { + t.Fatal("RestrictRedirects() modified its argument") + } + }) + t.Run("custom CheckRedirect", func(t *testing.T) { + orig := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return nil }} + if c := RestrictRedirects(orig); c != orig { + t.Fatal("RestrictRedirects() replaced a client that sets CheckRedirect") + } + }) + t.Run("followallredirects", func(t *testing.T) { + old := followallredirects + followallredirects = "1" + t.Cleanup(func() { followallredirects = old }) + + orig := &http.Client{} + if c := RestrictRedirects(orig); c != orig { + t.Fatal("RestrictRedirects() replaced the client with followallredirects=1") + } + if c := RestrictRedirects(nil); c != http.DefaultClient { + t.Fatal("RestrictRedirects(nil) did not return http.DefaultClient with followallredirects=1") + } + }) +} + +func TestRestrictRedirectsRequests(t *testing.T) { + var targetHits atomic.Int32 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + })) + t.Cleanup(target.Close) + + var originHits atomic.Int32 + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + originHits.Add(1) + switch r.URL.Path { + case "/same": + http.Redirect(w, r, "/final", http.StatusTemporaryRedirect) + case "/found": + http.Redirect(w, r, "/final", http.StatusFound) + case "/cross": + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + case "/loop": + http.Redirect(w, r, "/loop", http.StatusTemporaryRedirect) + case "/final": + io.Copy(w, r.Body) + } + })) + t.Cleanup(origin.Close) + + client := RestrictRedirects(origin.Client()) + do := func(method, path, body string) (string, error) { + t.Helper() + req, err := http.NewRequest(method, origin.URL+path, strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + resp, err := client.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + got, err := io.ReadAll(resp.Body) + return string(got), err + } + + if got, err := do(http.MethodPost, "/same", "hello"); err != nil || got != "hello" { + t.Errorf("POST after a same-origin 307 = %q, %v, want %q", got, err, "hello") + } + if _, err := do(http.MethodGet, "/found", ""); err != nil { + t.Errorf("GET after a same-origin 302: %v", err) + } + if _, err := do(http.MethodPost, "/found", "hello"); err == nil || !strings.Contains(err.Error(), "changes the method") { + t.Errorf("POST after a 302 = %v, want method change error", err) + } + if _, err := do(http.MethodPost, "/cross", "hello"); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("POST after a cross-origin 307 = %v, want origin error", err) + } + if n := targetHits.Load(); n != 0 { + t.Errorf("cross-origin redirect target got %d requests, want 0", n) + } + + originHits.Store(0) + if _, err := do(http.MethodGet, "/loop", ""); err == nil || !strings.Contains(err.Error(), "stopped after") { + t.Errorf("GET of a redirect loop = %v, want redirect limit error", err) + } + if got, want := originHits.Load(), int32(maxRedirects); got != want { + t.Errorf("redirect loop sent %d requests, want %d", got, want) + } +} diff --git a/mcp/sse.go b/mcp/sse.go index 5ed6dda01..927fd1204 100644 --- a/mcp/sse.go +++ b/mcp/sse.go @@ -412,6 +412,11 @@ type SSEClientTransport struct { // HTTPClient is the client to use for making HTTP requests. If nil, // http.DefaultClient is used. + // + // Unless HTTPClient sets CheckRedirect, a redirect is followed only if it + // stays within the origin of the request (an upgrade from http to https on + // the same host is allowed) and keeps the request method. Set + // CheckRedirect to apply another policy. HTTPClient *http.Client // MaxEventSize bounds the number of bytes buffered while reading a single @@ -440,10 +445,7 @@ func (c *SSEClientTransport) Connect(ctx context.Context) (Connection, error) { if err != nil { return nil, err } - httpClient := c.HTTPClient - if httpClient == nil { - httpClient = http.DefaultClient - } + httpClient := util.RestrictRedirects(c.HTTPClient) req.Header.Set("Accept", "text/event-stream") resp, err := httpClient.Do(req) if err != nil { diff --git a/mcp/sse_test.go b/mcp/sse_test.go index 29dc67e4f..86ed8f4d9 100644 --- a/mcp/sse_test.go +++ b/mcp/sse_test.go @@ -600,3 +600,48 @@ func TestSSEClientConnectContextCanceled(t *testing.T) { t.Errorf("Ping after canceling Connect context failed: %v", err) } } + +// TestSSEClientTransportCrossOriginRedirect verifies that a POST redirected to +// another origin is not followed. See #1429. +func TestSSEClientTransportCrossOriginRedirect(t *testing.T) { + ctx := context.Background() + + var targetHits atomic.Int32 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + http.Error(w, "redirect target", http.StatusInternalServerError) + })) + defer target.Close() + + server := NewServer(testImpl, nil) + sseHandler := NewSSEHandler(func(*http.Request) *Server { return server }, nil) + httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + if bytes.Contains(body, []byte(`"method":"ping"`)) { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + return + } + r.Body = io.NopCloser(bytes.NewReader(body)) + } + sseHandler.ServeHTTP(w, r) + })) + defer httpServer.Close() + + session, err := NewClient(testImpl, nil).Connect(ctx, &SSEClientTransport{Endpoint: httpServer.URL}, nil) + if err != nil { + t.Fatal(err) + } + defer session.Close() + + if err := session.Ping(ctx, nil); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("Ping() error = %v, want redirect not followed", err) + } + if n := targetHits.Load(); n != 0 { + t.Errorf("redirect target got %d requests, want 0", n) + } +} diff --git a/mcp/streamable.go b/mcp/streamable.go index 819bc08f4..e4fe035bc 100644 --- a/mcp/streamable.go +++ b/mcp/streamable.go @@ -2193,7 +2193,14 @@ func (c *streamableServerConn) Close() error { // endpoint serving the streamable HTTP transport defined by the 2025-03-26 // version of the spec. type StreamableClientTransport struct { - Endpoint string + Endpoint string + // HTTPClient is the client to use for making HTTP requests. If nil, + // http.DefaultClient is used. + // + // Unless HTTPClient sets CheckRedirect, a redirect is followed only if it + // stays within the origin of the request (an upgrade from http to https on + // the same host is allowed) and keeps the request method. Set + // CheckRedirect to apply another policy. HTTPClient *http.Client // MaxRetries is the maximum number of times to attempt a reconnect before giving up. // It defaults to 5. To disable retries, use a negative number. @@ -2274,10 +2281,7 @@ func init() { // When closed, the connection issues a DELETE request to terminate the // session, unless no session was established. func (t *StreamableClientTransport) Connect(ctx context.Context) (Connection, error) { - client := t.HTTPClient - if client == nil { - client = http.DefaultClient - } + client := util.RestrictRedirects(t.HTTPClient) maxRetries := t.MaxRetries if maxRetries == 0 { maxRetries = 5 diff --git a/mcp/streamable_client_test.go b/mcp/streamable_client_test.go index 928b65edc..b93a8f5db 100644 --- a/mcp/streamable_client_test.go +++ b/mcp/streamable_client_test.go @@ -2250,3 +2250,54 @@ func TestStreamableClientConnect_ColonlessSSELine(t *testing.T) { }) } } + +// TestStreamableClientRedirects verifies that the client follows a redirect +// within the origin of the endpoint, and that a redirect to another origin is +// not followed and fails only the redirected request. See #1429. +func TestStreamableClientRedirects(t *testing.T) { + ctx := context.Background() + + var targetHits atomic.Int32 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + http.Error(w, "redirect target", http.StatusInternalServerError) + })) + defer target.Close() + + server := NewServer(testImpl, nil) + handler := NewStreamableHTTPHandler(func(*http.Request) *Server { return server }, nil) + httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/old" { + http.Redirect(w, r, "/mcp", http.StatusPermanentRedirect) + return + } + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + if strings.Contains(string(body), `"method":"ping"`) { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + return + } + r.Body = io.NopCloser(strings.NewReader(string(body))) + handler.ServeHTTP(w, r) + })) + defer httpServer.Close() + + session, err := NewClient(testImpl, nil).Connect(ctx, &StreamableClientTransport{Endpoint: httpServer.URL + "/old"}, nil) + if err != nil { + t.Fatalf("Connect through a same-origin redirect: %v", err) + } + defer session.Close() + + if err := session.Ping(ctx, nil); err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("Ping() error = %v, want redirect not followed", err) + } + if n := targetHits.Load(); n != 0 { + t.Errorf("redirect target got %d requests, want 0", n) + } + if _, err := session.ListTools(ctx, nil); err != nil { + t.Errorf("ListTools() after the unfollowed redirect: %v", err) + } +} diff --git a/oauthex/dcr.go b/oauthex/dcr.go index 85f038ac8..7f905dcec 100644 --- a/oauthex/dcr.go +++ b/oauthex/dcr.go @@ -17,6 +17,7 @@ import ( "time" internaljson "github.com/modelcontextprotocol/go-sdk/internal/json" + "github.com/modelcontextprotocol/go-sdk/internal/util" ) // ClientRegistrationMetadata represents the client metadata fields for the DCR POST request (RFC 7591). @@ -259,14 +260,16 @@ func (e *ClientRegistrationError) Error() string { const maxRegistrationResponseBytes = 1 << 20 // 1 MiB // RegisterClient performs Dynamic Client Registration according to RFC 7591. +// +// If c is nil, http.DefaultClient is used. Unless c sets CheckRedirect, the +// request follows a redirect only within the origin of the request and only if +// it keeps the method. func RegisterClient(ctx context.Context, registrationEndpoint string, clientMeta *ClientRegistrationMetadata, c *http.Client) (*ClientRegistrationResponse, error) { if registrationEndpoint == "" { return nil, fmt.Errorf("registration_endpoint is required") } - if c == nil { - c = http.DefaultClient - } + c = util.RestrictRedirects(c) payload, err := json.Marshal(clientMeta) if err != nil { diff --git a/oauthex/dcr_test.go b/oauthex/dcr_test.go index f22115ce4..4b66b817a 100644 --- a/oauthex/dcr_test.go +++ b/oauthex/dcr_test.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -261,6 +262,29 @@ func TestRegisterClient(t *testing.T) { }) } + t.Run("Cross-Origin Redirect", func(t *testing.T) { + var registered atomic.Bool + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + registered.Store(true) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{"client_id":"test-client-id"}`)) + })) + defer target.Close() + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + })) + defer redirector.Close() + + _, err := RegisterClient(context.Background(), redirector.URL, &ClientRegistrationMetadata{RedirectURIs: []string{"http://localhost/cb"}}, nil) + if err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("RegisterClient() error = %v, want redirect not followed", err) + } + if registered.Load() { + t.Error("registration request was sent to the redirect target") + } + }) + t.Run("No Endpoint", func(t *testing.T) { _, err := RegisterClient(context.Background(), "", &ClientRegistrationMetadata{}, nil) if err == nil { diff --git a/oauthex/token_exchange.go b/oauthex/token_exchange.go index 0c7b24eb4..2ad063a36 100644 --- a/oauthex/token_exchange.go +++ b/oauthex/token_exchange.go @@ -13,6 +13,7 @@ import ( "net/http" "strings" + "github.com/modelcontextprotocol/go-sdk/internal/util" "golang.org/x/oauth2" ) @@ -72,6 +73,10 @@ type TokenExchangeRequest struct { // The tokenEndpoint parameter should be the IdP's token endpoint (typically // obtained from the IdP's authorization server metadata). // +// If httpClient is nil, http.DefaultClient is used. Unless httpClient sets +// CheckRedirect, the request follows a redirect only within the origin of the +// request and only if it keeps the method. +// // Returns an oauth2.Token where: // - Extra("issued_token_type") contains the type of the issued token (e.g., TokenTypeIDJAG) // - AccessToken contains the ID-JAG JWT (despite the name, this is not an OAuth access token) @@ -142,11 +147,7 @@ func ExchangeToken( cfg.ClientSecret = clientCreds.ClientSecretAuth.ClientSecret } - // Use custom HTTP client if provided. - if httpClient == nil { - httpClient = http.DefaultClient - } - ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, httpClient) + ctxWithClient := context.WithValue(ctx, oauth2.HTTPClient, util.RestrictRedirects(httpClient)) // Build token exchange parameters per RFC 8693. opts := []oauth2.AuthCodeOption{ diff --git a/oauthex/token_exchange_test.go b/oauthex/token_exchange_test.go index eb026ff2e..43d1915fb 100644 --- a/oauthex/token_exchange_test.go +++ b/oauthex/token_exchange_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -218,6 +219,34 @@ func TestExchangeToken(t *testing.T) { t.Error("expected error for invalid audience URL scheme, got nil") } }) + + t.Run("cross-origin redirect not followed", func(t *testing.T) { + redirector := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, server.URL, http.StatusTemporaryRedirect) + })) + defer redirector.Close() + + req := &TokenExchangeRequest{ + RequestedTokenType: TokenTypeIDJAG, + Audience: "https://auth.mcpserver.example/", + Resource: "https://mcp.mcpserver.example/", + SubjectToken: "fake-id-token", + SubjectTokenType: TokenTypeIDToken, + } + _, err := ExchangeToken( + context.Background(), + redirector.URL, + req, + &ClientCredentials{ + ClientID: "test-client-id", + ClientSecretAuth: &ClientSecretAuth{ClientSecret: "test-client-secret"}, + }, + redirector.Client(), + ) + if err == nil || !strings.Contains(err.Error(), "outside the origin") { + t.Errorf("ExchangeToken() error = %v, want redirect not followed", err) + } + }) } // writeErrorResponse writes an OAuth 2.0 error response per RFC 6749 Section 5.2.