From 390448bcec0ba952268d3824a7a64350e692fff2 Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 21:12:40 +0800 Subject: [PATCH 1/6] fix(transport): refuse cross-origin redirects in default HTTP clients The default http.Client in NewStreamableHTTP, NewSSE and NewOAuthHandler had no CheckRedirect policy. A server answering with a 3xx could send the client to any host, and net/http copied the configured custom headers (and the Authorization header for subdomains) onto the redirected request, along with the body for 307/308. That allowed SSRF and leaked API keys and OAuth bearer tokens (GO-2026-6911, OWASP A10:2021 SSRF / A07:2021 credential exposure). The default clients now follow a redirect only to the same scheme, host and port, up to 10 hops, and otherwise fail with ErrCrossOriginRedirect before the new location is contacted. Clients supplied through WithHTTPBasicClient, WithHTTPClient or OAuthConfig.HTTPClient keep their own policy. --- client/transport/oauth.go | 5 +- client/transport/redirect.go | 58 +++++++++++ client/transport/redirect_test.go | 154 ++++++++++++++++++++++++++++ client/transport/sse.go | 6 +- client/transport/streamable_http.go | 6 +- 5 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 client/transport/redirect.go create mode 100644 client/transport/redirect_test.go diff --git a/client/transport/oauth.go b/client/transport/oauth.go index 52b5d231a..a2011cccc 100644 --- a/client/transport/oauth.go +++ b/client/transport/oauth.go @@ -72,7 +72,8 @@ type OAuthConfig struct { // a mismatched issuer. SkipIssuerMetadataValidation bool // HTTPClient is an optional HTTP client to use for requests. - // If nil, a default HTTP client with a 30 second timeout will be used. + // If nil, a default HTTP client with a 30 second timeout will be used, + // which follows a redirect only to the same scheme, host and port. HTTPClient *http.Client } @@ -296,7 +297,7 @@ func NewOAuthHandler(config OAuthConfig) *OAuthHandler { config.TokenStore = NewMemoryTokenStore() } if config.HTTPClient == nil { - config.HTTPClient = &http.Client{Timeout: 30 * time.Second} + config.HTTPClient = &http.Client{Timeout: 30 * time.Second, CheckRedirect: sameOriginRedirectPolicy} } return &OAuthHandler{ diff --git a/client/transport/redirect.go b/client/transport/redirect.go new file mode 100644 index 000000000..94e6eb71f --- /dev/null +++ b/client/transport/redirect.go @@ -0,0 +1,58 @@ +package transport + +import ( + "errors" + "fmt" + "net/http" + "strings" +) + +// maxRedirects matches the limit net/http applies when CheckRedirect is nil. +const maxRedirects = 10 + +// ErrCrossOriginRedirect is returned when a server redirects a request to a +// different scheme, host or port. The transports' default HTTP clients refuse +// such redirects, because the redirected request would carry the configured +// headers, the OAuth bearer token and, for 307 and 308, the request body to a +// destination the server chose. +var ErrCrossOriginRedirect = errors.New("refusing cross-origin redirect") + +// sameOriginRedirectPolicy is the CheckRedirect policy of the HTTP clients the +// transports create when the caller does not supply one. It follows a +// redirect only when the new URL has the scheme, host and port of the request +// that started the chain, and at most maxRedirects times. +func sameOriginRedirectPolicy(req *http.Request, via []*http.Request) error { + if len(via) == 0 { + return nil + } + if len(via) >= maxRedirects { + return fmt.Errorf("stopped after %d redirects", maxRedirects) + } + origin := via[0].URL + if !strings.EqualFold(req.URL.Scheme, origin.Scheme) || + !strings.EqualFold(req.URL.Hostname(), origin.Hostname()) || + effectivePort(req.URL.Scheme, req.URL.Port()) != effectivePort(origin.Scheme, origin.Port()) { + return fmt.Errorf("%w: %s://%s to %s://%s", ErrCrossOriginRedirect, + origin.Scheme, origin.Host, req.URL.Scheme, req.URL.Host) + } + return nil +} + +func effectivePort(scheme, port string) string { + if port != "" { + return port + } + switch strings.ToLower(scheme) { + case "http": + return "80" + case "https": + return "443" + } + return "" +} + +// newDefaultHTTPClient returns the HTTP client a transport uses when the +// caller does not supply one. +func newDefaultHTTPClient() *http.Client { + return &http.Client{CheckRedirect: sameOriginRedirectPolicy} +} diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go new file mode 100644 index 000000000..f035c2a94 --- /dev/null +++ b/client/transport/redirect_test.go @@ -0,0 +1,154 @@ +package transport + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/mark3labs/mcp-go/mcp" +) + +func TestSameOriginRedirectPolicy(t *testing.T) { + mustReq := func(raw string) *http.Request { + u, err := url.Parse(raw) + require.NoError(t, err) + return &http.Request{URL: u} + } + + tests := []struct { + name string + origin string + target string + wantErr bool + }{ + {name: "same origin path change", origin: "https://mcp.example.com/mcp", target: "https://mcp.example.com/mcp/"}, + {name: "explicit default port", origin: "https://mcp.example.com/mcp", target: "https://mcp.example.com:443/v2"}, + {name: "host case differs", origin: "https://mcp.example.com/mcp", target: "https://MCP.example.com/v2"}, + {name: "other host", origin: "https://mcp.example.com/mcp", target: "https://evil.example.net/", wantErr: true}, + {name: "subdomain", origin: "https://example.com/mcp", target: "https://internal.example.com/", wantErr: true}, + {name: "internal address", origin: "https://mcp.example.com/mcp", target: "http://169.254.169.254/latest/meta-data/", wantErr: true}, + {name: "scheme downgrade", origin: "https://mcp.example.com/mcp", target: "http://mcp.example.com/mcp", wantErr: true}, + {name: "other port", origin: "http://localhost:8080/mcp", target: "http://localhost:9090/mcp", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := sameOriginRedirectPolicy(mustReq(tt.target), []*http.Request{mustReq(tt.origin)}) + if tt.wantErr { + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + } else { + assert.NoError(t, err) + } + }) + } + + t.Run("redirect limit", func(t *testing.T) { + via := make([]*http.Request, maxRedirects) + for i := range via { + via[i] = mustReq("https://mcp.example.com/mcp") + } + assert.Error(t, sameOriginRedirectPolicy(mustReq("https://mcp.example.com/mcp"), via)) + }) +} + +// redirectTarget counts the requests that reach it. +type redirectTarget struct { + srv *httptest.Server + hits atomic.Int32 +} + +func newRedirectTarget(t *testing.T) *redirectTarget { + rt := &redirectTarget{} + rt.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rt.hits.Add(1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(rt.srv.Close) + return rt +} + +func newRedirectingServer(t *testing.T, location string) *httptest.Server { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, location, http.StatusTemporaryRedirect) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestStreamableHTTP_RefusesCrossOriginRedirect(t *testing.T) { + target := newRedirectTarget(t) + redirector := newRedirectingServer(t, target.srv.URL+"/steal") + + trans, err := NewStreamableHTTP(redirector.URL, + WithHTTPHeaders(map[string]string{ + "X-Api-Key": "secret-key", + "Authorization": "Bearer secret-token", + })) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + _, err = trans.SendRequest(ctx, JSONRPCRequest{ + JSONRPC: "2.0", + ID: mcp.NewRequestId(int64(1)), + Method: "initialize", + }) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, target.hits.Load(), "redirect target must not be contacted") +} + +func TestSSE_RefusesCrossOriginRedirect(t *testing.T) { + target := newRedirectTarget(t) + redirector := newRedirectingServer(t, target.srv.URL+"/steal") + + trans, err := NewSSE(redirector.URL, + WithHeaders(map[string]string{"X-Api-Key": "secret-key"})) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + err = trans.Start(ctx) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, target.hits.Load(), "redirect target must not be contacted") +} + +func TestStreamableHTTP_FollowsSameOriginRedirect(t *testing.T) { + var gotKey atomic.Value + mux := http.NewServeMux() + mux.HandleFunc("/old", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/mcp", http.StatusPermanentRedirect) + }) + mux.HandleFunc("/mcp", func(w http.ResponseWriter, r *http.Request) { + gotKey.Store(r.Header.Get("X-Api-Key")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{}}`)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trans, err := NewStreamableHTTP(srv.URL+"/old", + WithHTTPHeaders(map[string]string{"X-Api-Key": "secret-key"})) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + resp, err := trans.SendRequest(ctx, JSONRPCRequest{ + JSONRPC: "2.0", + ID: mcp.NewRequestId(int64(1)), + Method: "initialize", + }) + require.NoError(t, err) + require.NotNil(t, resp) + assert.Equal(t, "secret-key", gotKey.Load()) +} diff --git a/client/transport/sse.go b/client/transport/sse.go index c02faa694..4af221b85 100644 --- a/client/transport/sse.go +++ b/client/transport/sse.go @@ -103,6 +103,10 @@ func WithHeaderFunc(headerFunc HTTPHeaderFunc) ClientOption { } // WithHTTPClient sets a custom HTTP client for the SSE transport. +// +// The default client follows a redirect only to the same scheme, host and +// port, so configured headers and OAuth tokens are not sent elsewhere. A +// custom client's CheckRedirect policy is used as it is. func WithHTTPClient(httpClient *http.Client) ClientOption { return func(sc *SSE) { sc.httpClient = httpClient @@ -157,7 +161,7 @@ func NewSSE(baseURL string, options ...ClientOption) (*SSE, error) { smc := &SSE{ baseURL: parsedURL, - httpClient: &http.Client{}, + httpClient: newDefaultHTTPClient(), responses: make(map[string]chan *JSONRPCResponse), headers: make(map[string]string), logger: slog.Default(), diff --git a/client/transport/streamable_http.go b/client/transport/streamable_http.go index e62a05901..66ae74f98 100644 --- a/client/transport/streamable_http.go +++ b/client/transport/streamable_http.go @@ -43,6 +43,10 @@ func WithContinuousListening() StreamableHTTPCOption { } // WithHTTPBasicClient sets a custom HTTP client on the StreamableHTTP transport. +// +// The default client follows a redirect only to the same scheme, host and +// port, so configured headers and OAuth tokens are not sent elsewhere. A +// custom client's CheckRedirect policy is used as it is. func WithHTTPBasicClient(client *http.Client) StreamableHTTPCOption { return func(sc *StreamableHTTP) { sc.httpClient = client @@ -193,7 +197,7 @@ func NewStreamableHTTP(serverURL string, options ...StreamableHTTPCOption) (*Str smc := &StreamableHTTP{ serverURL: parsedURL, - httpClient: &http.Client{}, + httpClient: newDefaultHTTPClient(), headers: make(map[string]string), closed: make(chan struct{}), logger: slog.Default(), From 8ce69dbc4bd89b5d699e51ec8ce343fce22fea1f Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 21:33:05 +0800 Subject: [PATCH 2/6] fix(transport): tighten origin checks and cover OAuth credential requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Compare origins with ASCII-only lowercasing. strings.EqualFold matched "K" (Kelvin sign) with "k" and "İ" with "i", which net/http dials as different hosts. - Normalize numeric ports, so ":0443" is the same origin as ":443". - Return ErrTooManyRedirects when a chain reaches 10 hops instead of an unwrapped error. - Check the SSE endpoint event against the full origin. It compared only the host, so an https connection could be told to post headers, the OAuth token and bodies to http on the same host. - Give the OAuth handler's default client its own policy: token, refresh and registration requests (which carry client_secret and codes in the body, resent on 307/308) only follow same-origin redirects; metadata discovery may still be redirected across origins but never from https to http (ErrInsecureRedirect). --- client/transport/oauth.go | 8 +- client/transport/redirect.go | 111 ++++++-- client/transport/redirect_test.go | 416 +++++++++++++++++++++++------- client/transport/sse.go | 4 +- 4 files changed, 421 insertions(+), 118 deletions(-) diff --git a/client/transport/oauth.go b/client/transport/oauth.go index a2011cccc..b2d541cc4 100644 --- a/client/transport/oauth.go +++ b/client/transport/oauth.go @@ -72,8 +72,10 @@ type OAuthConfig struct { // a mismatched issuer. SkipIssuerMetadataValidation bool // HTTPClient is an optional HTTP client to use for requests. - // If nil, a default HTTP client with a 30 second timeout will be used, - // which follows a redirect only to the same scheme, host and port. + // If nil, a default HTTP client with a 30 second timeout is used. It + // follows token, refresh and registration requests only to the same + // scheme, host and port, and never redirects metadata discovery from + // https to http. HTTPClient *http.Client } @@ -297,7 +299,7 @@ func NewOAuthHandler(config OAuthConfig) *OAuthHandler { config.TokenStore = NewMemoryTokenStore() } if config.HTTPClient == nil { - config.HTTPClient = &http.Client{Timeout: 30 * time.Second, CheckRedirect: sameOriginRedirectPolicy} + config.HTTPClient = &http.Client{Timeout: 30 * time.Second, CheckRedirect: oauthRedirectPolicy} } return &OAuthHandler{ diff --git a/client/transport/redirect.go b/client/transport/redirect.go index 94e6eb71f..459d6e043 100644 --- a/client/transport/redirect.go +++ b/client/transport/redirect.go @@ -4,45 +4,110 @@ import ( "errors" "fmt" "net/http" + "net/url" + "strconv" "strings" ) -// maxRedirects matches the limit net/http applies when CheckRedirect is nil. -const maxRedirects = 10 - // ErrCrossOriginRedirect is returned when a server redirects a request to a -// different scheme, host or port. The transports' default HTTP clients refuse -// such redirects, because the redirected request would carry the configured +// different origin (scheme, host or port) and the redirect policy does not +// allow it. The redirected request would otherwise carry the configured // headers, the OAuth bearer token and, for 307 and 308, the request body to a // destination the server chose. var ErrCrossOriginRedirect = errors.New("refusing cross-origin redirect") -// sameOriginRedirectPolicy is the CheckRedirect policy of the HTTP clients the -// transports create when the caller does not supply one. It follows a -// redirect only when the new URL has the scheme, host and port of the request -// that started the chain, and at most maxRedirects times. +// ErrInsecureRedirect is returned when a server redirects an https request to +// a plain http URL. +var ErrInsecureRedirect = errors.New("refusing redirect from https to http") + +// ErrTooManyRedirects is returned when a redirect chain reaches maxRedirects. +var ErrTooManyRedirects = errors.New("too many redirects") + +// maxRedirects matches the limit net/http applies when CheckRedirect is nil. +const maxRedirects = 10 + +// newDefaultHTTPClient returns the HTTP client a transport uses when the +// caller does not supply one. It only follows same-origin redirects. +func newDefaultHTTPClient() *http.Client { + return &http.Client{CheckRedirect: sameOriginRedirectPolicy} +} + +// sameOriginRedirectPolicy is an http.Client CheckRedirect function that +// follows a redirect only when the new URL has the origin of the request that +// started the chain, and at most maxRedirects times. func sameOriginRedirectPolicy(req *http.Request, via []*http.Request) error { + if len(via) >= maxRedirects { + return fmt.Errorf("%w: stopped after %d redirects", ErrTooManyRedirects, maxRedirects) + } if len(via) == 0 { return nil } + from := via[0].URL + if !sameOrigin(from, req.URL) { + return fmt.Errorf("%w: %s://%s to %s://%s", + ErrCrossOriginRedirect, from.Scheme, from.Host, req.URL.Scheme, req.URL.Host) + } + return nil +} + +// oauthRedirectPolicy is the CheckRedirect function of the OAuth handler's +// default client. Requests that carry credentials (token, refresh and client +// registration requests, which are not GET or HEAD) follow only same-origin +// redirects. Metadata discovery requests, which carry no credentials, may be +// redirected to another origin but never from https to http. +func oauthRedirectPolicy(req *http.Request, via []*http.Request) error { + if len(via) == 0 { + return nil + } + first := via[0] + if first.Method != http.MethodGet && first.Method != http.MethodHead { + return sameOriginRedirectPolicy(req, via) + } if len(via) >= maxRedirects { - return fmt.Errorf("stopped after %d redirects", maxRedirects) + return fmt.Errorf("%w: stopped after %d redirects", ErrTooManyRedirects, maxRedirects) } - origin := via[0].URL - if !strings.EqualFold(req.URL.Scheme, origin.Scheme) || - !strings.EqualFold(req.URL.Hostname(), origin.Hostname()) || - effectivePort(req.URL.Scheme, req.URL.Port()) != effectivePort(origin.Scheme, origin.Port()) { - return fmt.Errorf("%w: %s://%s to %s://%s", ErrCrossOriginRedirect, - origin.Scheme, origin.Host, req.URL.Scheme, req.URL.Host) + prev := via[len(via)-1].URL + if asciiLower(prev.Scheme) == "https" && asciiLower(req.URL.Scheme) != "https" { + return fmt.Errorf("%w: %s to %s", ErrInsecureRedirect, prev.Redacted(), req.URL.Redacted()) } return nil } -func effectivePort(scheme, port string) string { - if port != "" { - return port +// sameOrigin reports whether a and b have the same scheme, host and port. +func sameOrigin(a, b *url.URL) bool { + return origin(a) == origin(b) +} + +// origin returns a normalized scheme://host:port string. The scheme and host +// are lowercased (ASCII only), a missing port becomes the scheme's default, +// and a numeric port loses leading zeros, so ":0443" and ":443" compare equal +// as net/http dials them. +func origin(u *url.URL) string { + scheme := asciiLower(u.Scheme) + port := u.Port() + if n, err := strconv.ParseUint(port, 10, 16); err == nil { + port = strconv.FormatUint(n, 10) } - switch strings.ToLower(scheme) { + if port == "" { + port = defaultPort(scheme) + } + return scheme + "://[" + asciiLower(u.Hostname()) + "]:" + port +} + +// asciiLower lowercases only ASCII letters. Unicode case mapping must not be +// used on hosts: strings.ToLower maps "İ" to "i" and strings.EqualFold matches +// "K" (Kelvin sign) with "k", while net/http dials those as different hosts. +func asciiLower(s string) string { + return strings.Map(func(r rune) rune { + if 'A' <= r && r <= 'Z' { + return r + ('a' - 'A') + } + return r + }, s) +} + +func defaultPort(scheme string) string { + switch scheme { case "http": return "80" case "https": @@ -50,9 +115,3 @@ func effectivePort(scheme, port string) string { } return "" } - -// newDefaultHTTPClient returns the HTTP client a transport uses when the -// caller does not supply one. -func newDefaultHTTPClient() *http.Client { - return &http.Client{CheckRedirect: sameOriginRedirectPolicy} -} diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go index f035c2a94..9099888f7 100644 --- a/client/transport/redirect_test.go +++ b/client/transport/redirect_test.go @@ -2,9 +2,11 @@ package transport import ( "context" + "io" "net/http" "net/http/httptest" "net/url" + "strings" "sync/atomic" "testing" "time" @@ -15,140 +17,378 @@ import ( "github.com/mark3labs/mcp-go/mcp" ) -func TestSameOriginRedirectPolicy(t *testing.T) { - mustReq := func(raw string) *http.Request { - u, err := url.Parse(raw) - require.NoError(t, err) - return &http.Request{URL: u} +const redirectTestSecret = "secret-key" + +// newRedirectAttacker returns a server that counts the requests it receives, +// and a URL for it whose host differs from the 127.0.0.1 httptest uses. +func newRedirectAttacker(t *testing.T) (*atomic.Int64, string) { + t.Helper() + var hits atomic.Int64 + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(attacker.Close) + return &hits, strings.Replace(attacker.URL, "127.0.0.1", "localhost", 1) +} + +func newRedirectingServer(t *testing.T, location string, status int) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, location, status) + })) + t.Cleanup(srv.Close) + return srv +} + +func redirectTestRequest() JSONRPCRequest { + return JSONRPCRequest{ + JSONRPC: mcp.JSONRPC_VERSION, + ID: mcp.NewRequestId(int64(1)), + Method: "tools/call", + Params: map[string]any{"name": "x"}, } +} + +var redirectStatuses = []struct { + name string + status int +}{ + {name: "301 Moved Permanently", status: http.StatusMovedPermanently}, + {name: "302 Found", status: http.StatusFound}, + {name: "303 See Other", status: http.StatusSeeOther}, + {name: "307 Temporary Redirect", status: http.StatusTemporaryRedirect}, + {name: "308 Permanent Redirect", status: http.StatusPermanentRedirect}, +} +func TestSameOriginRedirectPolicy(t *testing.T) { tests := []struct { name string - origin string - target string - wantErr bool + from string + to string + via int + wantErr error }{ - {name: "same origin path change", origin: "https://mcp.example.com/mcp", target: "https://mcp.example.com/mcp/"}, - {name: "explicit default port", origin: "https://mcp.example.com/mcp", target: "https://mcp.example.com:443/v2"}, - {name: "host case differs", origin: "https://mcp.example.com/mcp", target: "https://MCP.example.com/v2"}, - {name: "other host", origin: "https://mcp.example.com/mcp", target: "https://evil.example.net/", wantErr: true}, - {name: "subdomain", origin: "https://example.com/mcp", target: "https://internal.example.com/", wantErr: true}, - {name: "internal address", origin: "https://mcp.example.com/mcp", target: "http://169.254.169.254/latest/meta-data/", wantErr: true}, - {name: "scheme downgrade", origin: "https://mcp.example.com/mcp", target: "http://mcp.example.com/mcp", wantErr: true}, - {name: "other port", origin: "http://localhost:8080/mcp", target: "http://localhost:9090/mcp", wantErr: true}, + {name: "same origin, other path", from: "https://a.example/mcp", to: "https://a.example/v2/mcp"}, + {name: "explicit default https port", from: "https://a.example/mcp", to: "https://a.example:443/mcp"}, + {name: "explicit default http port", from: "http://a.example:80/mcp", to: "http://a.example/mcp"}, + {name: "empty port is the default port", from: "https://a.example/mcp", to: "https://a.example:/mcp"}, + {name: "leading zeros in port", from: "https://a.example/mcp", to: "https://a.example:0443/mcp"}, + {name: "leading zeros in non-default port", from: "http://a.example:8080/mcp", to: "http://a.example:08080/mcp"}, + {name: "host case differs", from: "https://A.example/mcp", to: "https://a.EXAMPLE/mcp"}, + {name: "scheme case differs", from: "HTTPS://a.example/mcp", to: "https://a.example/mcp"}, + {name: "IPv6 same origin", from: "http://[::1]:8080/mcp", to: "http://[::1]:8080/v2"}, + {name: "IPv6 other port", from: "http://[::1]:8080/mcp", to: "http://[::1]:9090/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "dotless i is not folded onto i", from: "https://i.example/mcp", to: "https://İ.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "i is not folded onto dotless i", from: "https://İ.example/mcp", to: "https://i.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "Kelvin sign is not folded onto k", from: "https://k.example/mcp", to: "https://K.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "other host", from: "https://a.example/mcp", to: "https://b.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "subdomain", from: "https://a.example/mcp", to: "https://x.a.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "parent domain", from: "https://x.a.example/mcp", to: "https://a.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "other port", from: "http://a.example:8080/mcp", to: "http://a.example:9090/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "scheme downgrade", from: "https://a.example/mcp", to: "http://a.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "scheme upgrade", from: "http://a.example/mcp", to: "https://a.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "cloud metadata address", from: "https://a.example/mcp", to: "http://169.254.169.254/latest/meta-data/", wantErr: ErrCrossOriginRedirect}, + {name: "loopback", from: "https://a.example/mcp", to: "http://127.0.0.1:6379/", wantErr: ErrCrossOriginRedirect}, + {name: "userinfo trick", from: "https://a.example/mcp", to: "https://a.example@b.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "too many redirects", from: "https://a.example/mcp", to: "https://a.example/mcp", via: maxRedirects, wantErr: ErrTooManyRedirects}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := sameOriginRedirectPolicy(mustReq(tt.target), []*http.Request{mustReq(tt.origin)}) - if tt.wantErr { - assert.ErrorIs(t, err, ErrCrossOriginRedirect) - } else { + err := sameOriginRedirectPolicy(redirectHop(t, http.MethodPost, tt.from, tt.to, tt.via)) + if tt.wantErr == nil { assert.NoError(t, err) + return + } + assert.ErrorIs(t, err, tt.wantErr) + if tt.wantErr == ErrTooManyRedirects { + assert.NotErrorIs(t, err, ErrCrossOriginRedirect) } }) } - - t.Run("redirect limit", func(t *testing.T) { - via := make([]*http.Request, maxRedirects) - for i := range via { - via[i] = mustReq("https://mcp.example.com/mcp") - } - assert.Error(t, sameOriginRedirectPolicy(mustReq("https://mcp.example.com/mcp"), via)) - }) } -// redirectTarget counts the requests that reach it. -type redirectTarget struct { - srv *httptest.Server - hits atomic.Int32 -} - -func newRedirectTarget(t *testing.T) *redirectTarget { - rt := &redirectTarget{} - rt.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - rt.hits.Add(1) - w.WriteHeader(http.StatusOK) - })) - t.Cleanup(rt.srv.Close) - return rt +func TestOAuthRedirectPolicy(t *testing.T) { + tests := []struct { + name string + method string + from string + to string + via int + wantErr error + }{ + {name: "discovery to another origin", method: http.MethodGet, from: "https://mcp.example/.well-known/oauth-protected-resource", to: "https://auth.example/.well-known/oauth-protected-resource"}, + {name: "discovery same origin", method: http.MethodGet, from: "https://auth.example/.well-known/openid-configuration", to: "https://auth.example/realms/x/.well-known/openid-configuration"}, + {name: "discovery over plain http", method: http.MethodGet, from: "http://localhost:8080/.well-known/oauth-authorization-server", to: "http://localhost:9090/.well-known/oauth-authorization-server"}, + {name: "discovery downgrade to http", method: http.MethodGet, from: "https://auth.example/.well-known/oauth-authorization-server", to: "http://auth.example/.well-known/oauth-authorization-server", wantErr: ErrInsecureRedirect}, + {name: "discovery too many redirects", method: http.MethodGet, from: "https://auth.example/a", to: "https://auth.example/b", via: maxRedirects, wantErr: ErrTooManyRedirects}, + {name: "token request same origin", method: http.MethodPost, from: "https://auth.example/token", to: "https://auth.example/oauth/token"}, + {name: "token request to another origin", method: http.MethodPost, from: "https://auth.example/token", to: "https://evil.example/token", wantErr: ErrCrossOriginRedirect}, + {name: "token request to subdomain", method: http.MethodPost, from: "https://auth.example/token", to: "https://x.auth.example/token", wantErr: ErrCrossOriginRedirect}, + {name: "token request downgrade", method: http.MethodPost, from: "https://auth.example/token", to: "http://auth.example/token", wantErr: ErrCrossOriginRedirect}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := oauthRedirectPolicy(redirectHop(t, tt.method, tt.from, tt.to, tt.via)) + if tt.wantErr == nil { + assert.NoError(t, err) + return + } + assert.ErrorIs(t, err, tt.wantErr) + }) + } } -func newRedirectingServer(t *testing.T, location string) *httptest.Server { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Redirect(w, r, location, http.StatusTemporaryRedirect) - })) - t.Cleanup(srv.Close) - return srv +// redirectHop builds the arguments net/http passes to CheckRedirect for a +// redirect from "from" to "to", with "hops" earlier requests (at least one). +func redirectHop(t *testing.T, method, from, to string, hops int) (*http.Request, []*http.Request) { + t.Helper() + first, err := http.NewRequest(method, from, nil) + require.NoError(t, err) + // net/http builds the next request from the Location with URL.Parse, + // without NewRequest's empty-port cleanup. + nextURL, err := url.Parse(to) + require.NoError(t, err) + next := &http.Request{Method: method, URL: nextURL} + via := []*http.Request{first} + for len(via) < hops { + via = append(via, first) + } + return next, via } func TestStreamableHTTP_RefusesCrossOriginRedirect(t *testing.T) { - target := newRedirectTarget(t) - redirector := newRedirectingServer(t, target.srv.URL+"/steal") - - trans, err := NewStreamableHTTP(redirector.URL, - WithHTTPHeaders(map[string]string{ - "X-Api-Key": "secret-key", - "Authorization": "Bearer secret-token", - })) - require.NoError(t, err) + for _, tt := range redirectStatuses { + t.Run(tt.name, func(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/steal", tt.status) - ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) - defer cancel() + tr, err := NewStreamableHTTP(srv.URL, + WithHTTPHeaders(map[string]string{"X-Api-Key": redirectTestSecret}), + WithHTTPHeaderFunc(func(_ context.Context) map[string]string { + return map[string]string{"Authorization": "Bearer " + redirectTestSecret} + })) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) - _, err = trans.SendRequest(ctx, JSONRPCRequest{ - JSONRPC: "2.0", - ID: mcp.NewRequestId(int64(1)), - Method: "initialize", - }) - require.Error(t, err) - assert.ErrorIs(t, err, ErrCrossOriginRedirect) - assert.Zero(t, target.hits.Load(), "redirect target must not be contacted") + _, err = tr.SendRequest(t.Context(), redirectTestRequest()) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, hits.Load(), "the redirect target must not be contacted") + }) + } } -func TestSSE_RefusesCrossOriginRedirect(t *testing.T) { - target := newRedirectTarget(t) - redirector := newRedirectingServer(t, target.srv.URL+"/steal") +func TestStreamableHTTP_SendNotificationRefusesCrossOriginRedirect(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/steal", http.StatusTemporaryRedirect) - trans, err := NewSSE(redirector.URL, - WithHeaders(map[string]string{"X-Api-Key": "secret-key"})) + tr, err := NewStreamableHTTP(srv.URL, + WithHTTPHeaders(map[string]string{"X-Api-Key": redirectTestSecret})) require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) - ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) - defer cancel() - - err = trans.Start(ctx) + err = tr.SendNotification(t.Context(), mcp.JSONRPCNotification{ + JSONRPC: mcp.JSONRPC_VERSION, + Notification: mcp.Notification{Method: "notifications/initialized"}, + }) require.Error(t, err) assert.ErrorIs(t, err, ErrCrossOriginRedirect) - assert.Zero(t, target.hits.Load(), "redirect target must not be contacted") + assert.Zero(t, hits.Load(), "the redirect target must not be contacted") } func TestStreamableHTTP_FollowsSameOriginRedirect(t *testing.T) { var gotKey atomic.Value + var gotBody atomic.Bool mux := http.NewServeMux() mux.HandleFunc("/old", func(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/mcp", http.StatusPermanentRedirect) }) mux.HandleFunc("/mcp", func(w http.ResponseWriter, r *http.Request) { gotKey.Store(r.Header.Get("X-Api-Key")) + var buf strings.Builder + _, _ = io.Copy(&buf, r.Body) + gotBody.Store(strings.Contains(buf.String(), `"tools/call"`)) w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{}}`)) }) srv := httptest.NewServer(mux) t.Cleanup(srv.Close) - trans, err := NewStreamableHTTP(srv.URL+"/old", - WithHTTPHeaders(map[string]string{"X-Api-Key": "secret-key"})) + tr, err := NewStreamableHTTP(srv.URL+"/old", + WithHTTPHeaders(map[string]string{"X-Api-Key": redirectTestSecret})) require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) - ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) - defer cancel() - - resp, err := trans.SendRequest(ctx, JSONRPCRequest{ - JSONRPC: "2.0", - ID: mcp.NewRequestId(int64(1)), - Method: "initialize", - }) + resp, err := tr.SendRequest(t.Context(), redirectTestRequest()) require.NoError(t, err) require.NotNil(t, resp) - assert.Equal(t, "secret-key", gotKey.Load()) + assert.Equal(t, redirectTestSecret, gotKey.Load()) + assert.True(t, gotBody.Load(), "a same-origin 308 must resend the request body") +} + +func TestStreamableHTTP_CustomClientKeepsItsRedirectPolicy(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/elsewhere", http.StatusFound) + + tr, err := NewStreamableHTTP(srv.URL, WithHTTPBasicClient(&http.Client{})) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + _, _ = tr.SendRequest(t.Context(), redirectTestRequest()) + assert.Equal(t, int64(1), hits.Load(), "a caller-supplied client must keep its own redirect policy") +} + +func TestStreamableHTTP_TimeoutOptionKeepsRedirectPolicy(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/steal", http.StatusTemporaryRedirect) + + tr, err := NewStreamableHTTP(srv.URL, WithHTTPTimeout(5*time.Second)) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + _, err = tr.SendRequest(t.Context(), redirectTestRequest()) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, hits.Load()) +} + +func TestSSE_RefusesCrossOriginRedirect(t *testing.T) { + for _, tt := range redirectStatuses { + t.Run(tt.name, func(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/steal", tt.status) + + tr, err := NewSSE(srv.URL, WithHeaders(map[string]string{"X-Api-Key": redirectTestSecret})) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + err = tr.Start(t.Context()) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, hits.Load(), "the redirect target must not be contacted") + }) + } +} + +func TestSSE_RejectsEndpointOnOtherOrigin(t *testing.T) { + tests := []struct { + name string + endpoint func(host string) string + }{ + {name: "scheme downgrade", endpoint: func(host string) string { return "http://" + host + "/message" }}, + {name: "other host", endpoint: func(string) string { return "https://localhost:1/message" }}, + {name: "other port", endpoint: func(host string) string { + h, _, _ := strings.Cut(host, ":") + return "https://" + h + ":1/message" + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var posts atomic.Int64 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + posts.Add(1) + w.WriteHeader(http.StatusAccepted) + return + } + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("event: endpoint\ndata: " + tt.endpoint(r.Host) + "\n\n")) + w.(http.Flusher).Flush() + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + tr, err := NewSSE(srv.URL, WithHTTPClient(srv.Client()), WithEndpointTimeout(200*time.Millisecond)) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + err = tr.Start(t.Context()) + require.Error(t, err, "an endpoint on another origin must not be accepted") + assert.Contains(t, err.Error(), "timeout waiting for endpoint") + assert.Zero(t, posts.Load()) + }) + } +} + +func TestSSE_AcceptsEndpointOnSameOrigin(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("event: endpoint\ndata: /message\n\n")) + w.(http.Flusher).Flush() + <-r.Context().Done() + })) + t.Cleanup(srv.Close) + + tr, err := NewSSE(srv.URL, WithHTTPClient(srv.Client()), WithEndpointTimeout(2*time.Second)) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + require.NoError(t, tr.Start(t.Context())) +} + +func TestSSE_CustomClientKeepsItsRedirectPolicy(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/elsewhere", http.StatusFound) + + tr, err := NewSSE(srv.URL, WithHTTPClient(&http.Client{})) + require.NoError(t, err) + t.Cleanup(func() { _ = tr.Close() }) + + _ = tr.Start(t.Context()) + assert.Equal(t, int64(1), hits.Load(), "a caller-supplied client must keep its own redirect policy") +} + +func TestOAuthHandler_DefaultClientRefusesCrossOriginTokenRedirect(t *testing.T) { + for _, tt := range []struct { + name string + status int + }{ + {name: "307 Temporary Redirect", status: http.StatusTemporaryRedirect}, + {name: "308 Permanent Redirect", status: http.StatusPermanentRedirect}, + } { + t.Run(tt.name, func(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + srv := newRedirectingServer(t, attackerURL+"/token", tt.status) + + h := NewOAuthHandler(OAuthConfig{}) + form := url.Values{"grant_type": {"refresh_token"}, "client_secret": {redirectTestSecret}} + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, srv.URL+"/token", + strings.NewReader(form.Encode())) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp, err := h.httpClient.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, hits.Load(), "the token request body must not reach another origin") + }) + } +} + +func TestOAuthHandler_DefaultClientFollowsDiscoveryRedirect(t *testing.T) { + var hits atomic.Int64 + metadata := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(metadata.Close) + otherOrigin := strings.Replace(metadata.URL, "127.0.0.1", "localhost", 1) + srv := newRedirectingServer(t, otherOrigin+"/.well-known/oauth-authorization-server", http.StatusFound) + + h := NewOAuthHandler(OAuthConfig{}) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, + srv.URL+"/.well-known/oauth-authorization-server", nil) + require.NoError(t, err) + + resp, err := h.httpClient.Do(req) + require.NoError(t, err) + _ = resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, int64(1), hits.Load()) } diff --git a/client/transport/sse.go b/client/transport/sse.go index 4af221b85..be682e674 100644 --- a/client/transport/sse.go +++ b/client/transport/sse.go @@ -483,7 +483,9 @@ func (c *SSE) handleSSEEvent(event, data string, endpointChan chan struct{}, end c.logger.Error("Error parsing endpoint URL", "err", err) return } - if endpoint.Host != c.baseURL.Host { + // Scheme and port must match too: an https connection must not + // post headers and bodies to an http endpoint on the same host. + if !sameOrigin(endpoint, c.baseURL) { c.logger.Error("Endpoint origin does not match connection origin") return } From 41a7375f7b553a75985bcb1dcb92ab755477a01e Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 21:33:46 +0800 Subject: [PATCH 3/6] test(transport): fix names of the dotted capital I cases --- client/transport/redirect_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go index 9099888f7..91b0733ca 100644 --- a/client/transport/redirect_test.go +++ b/client/transport/redirect_test.go @@ -79,8 +79,8 @@ func TestSameOriginRedirectPolicy(t *testing.T) { {name: "scheme case differs", from: "HTTPS://a.example/mcp", to: "https://a.example/mcp"}, {name: "IPv6 same origin", from: "http://[::1]:8080/mcp", to: "http://[::1]:8080/v2"}, {name: "IPv6 other port", from: "http://[::1]:8080/mcp", to: "http://[::1]:9090/mcp", wantErr: ErrCrossOriginRedirect}, - {name: "dotless i is not folded onto i", from: "https://i.example/mcp", to: "https://İ.example/mcp", wantErr: ErrCrossOriginRedirect}, - {name: "i is not folded onto dotless i", from: "https://İ.example/mcp", to: "https://i.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "dotted capital I is not folded onto i", from: "https://i.example/mcp", to: "https://İ.example/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "i is not folded onto dotted capital I", from: "https://İ.example/mcp", to: "https://i.example/mcp", wantErr: ErrCrossOriginRedirect}, {name: "Kelvin sign is not folded onto k", from: "https://k.example/mcp", to: "https://K.example/mcp", wantErr: ErrCrossOriginRedirect}, {name: "other host", from: "https://a.example/mcp", to: "https://b.example/mcp", wantErr: ErrCrossOriginRedirect}, {name: "subdomain", from: "https://a.example/mcp", to: "https://x.a.example/mcp", wantErr: ErrCrossOriginRedirect}, From 8d778b58ade6b386bbd6a0b5980a5fc3de532205 Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 21:43:05 +0800 Subject: [PATCH 4/6] fix(transport): compare IP literals in canonical form [0:0::1] and [::1] are the same address but compared as different origins. Write IP literals with netip before comparing; an IPv4-mapped IPv6 address stays distinct from the IPv4 address. Also add godoc to defaultPort and the redirect tests. --- client/transport/redirect.go | 16 ++++++++++++---- client/transport/redirect_test.go | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/client/transport/redirect.go b/client/transport/redirect.go index 459d6e043..d320fb7f6 100644 --- a/client/transport/redirect.go +++ b/client/transport/redirect.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/http" + "net/netip" "net/url" "strconv" "strings" @@ -79,11 +80,16 @@ func sameOrigin(a, b *url.URL) bool { } // origin returns a normalized scheme://host:port string. The scheme and host -// are lowercased (ASCII only), a missing port becomes the scheme's default, -// and a numeric port loses leading zeros, so ":0443" and ":443" compare equal -// as net/http dials them. +// are lowercased (ASCII only), an IP literal is written in its canonical form +// (so "[0:0::1]" and "[::1]" compare equal), a missing port becomes the +// scheme's default, and a numeric port loses leading zeros, so ":0443" and +// ":443" compare equal as net/http dials them. func origin(u *url.URL) string { scheme := asciiLower(u.Scheme) + host := asciiLower(u.Hostname()) + if addr, err := netip.ParseAddr(host); err == nil { + host = addr.String() + } port := u.Port() if n, err := strconv.ParseUint(port, 10, 16); err == nil { port = strconv.FormatUint(n, 10) @@ -91,7 +97,7 @@ func origin(u *url.URL) string { if port == "" { port = defaultPort(scheme) } - return scheme + "://[" + asciiLower(u.Hostname()) + "]:" + port + return scheme + "://[" + host + "]:" + port } // asciiLower lowercases only ASCII letters. Unicode case mapping must not be @@ -106,6 +112,8 @@ func asciiLower(s string) string { }, s) } +// defaultPort returns the port net/http dials for scheme when a URL has none, +// or "" for a scheme it does not know. func defaultPort(scheme string) string { switch scheme { case "http": diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go index 91b0733ca..2e3065e13 100644 --- a/client/transport/redirect_test.go +++ b/client/transport/redirect_test.go @@ -32,6 +32,8 @@ func newRedirectAttacker(t *testing.T) (*atomic.Int64, string) { return &hits, strings.Replace(attacker.URL, "127.0.0.1", "localhost", 1) } +// newRedirectingServer returns a server that answers every request with a +// redirect to location using the given status. func newRedirectingServer(t *testing.T, location string, status int) *httptest.Server { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -41,6 +43,7 @@ func newRedirectingServer(t *testing.T, location string, status int) *httptest.S return srv } +// redirectTestRequest returns the JSON-RPC request the redirect tests send. func redirectTestRequest() JSONRPCRequest { return JSONRPCRequest{ JSONRPC: mcp.JSONRPC_VERSION, @@ -50,6 +53,7 @@ func redirectTestRequest() JSONRPCRequest { } } +// redirectStatuses lists the redirect statuses net/http follows. var redirectStatuses = []struct { name string status int @@ -61,6 +65,7 @@ var redirectStatuses = []struct { {name: "308 Permanent Redirect", status: http.StatusPermanentRedirect}, } +// TestSameOriginRedirectPolicy checks which redirects the default transport clients follow. func TestSameOriginRedirectPolicy(t *testing.T) { tests := []struct { name string @@ -78,6 +83,11 @@ func TestSameOriginRedirectPolicy(t *testing.T) { {name: "host case differs", from: "https://A.example/mcp", to: "https://a.EXAMPLE/mcp"}, {name: "scheme case differs", from: "HTTPS://a.example/mcp", to: "https://a.example/mcp"}, {name: "IPv6 same origin", from: "http://[::1]:8080/mcp", to: "http://[::1]:8080/v2"}, + {name: "IPv6 equivalent literals", from: "http://[::1]:8080/mcp", to: "http://[0:0::1]:8080/v2"}, + {name: "IPv6 full and compressed form", from: "http://[2001:db8::1]/mcp", to: "http://[2001:0db8:0000:0000:0000:0000:0000:0001]/v2"}, + {name: "IPv6 hex case", from: "http://[2001:DB8::A]/mcp", to: "http://[2001:db8::a]/v2"}, + {name: "IPv4-mapped IPv6 is not IPv4", from: "http://127.0.0.1/mcp", to: "http://[::ffff:127.0.0.1]/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "IPv6 other address", from: "http://[::1]/mcp", to: "http://[::2]/mcp", wantErr: ErrCrossOriginRedirect}, {name: "IPv6 other port", from: "http://[::1]:8080/mcp", to: "http://[::1]:9090/mcp", wantErr: ErrCrossOriginRedirect}, {name: "dotted capital I is not folded onto i", from: "https://i.example/mcp", to: "https://İ.example/mcp", wantErr: ErrCrossOriginRedirect}, {name: "i is not folded onto dotted capital I", from: "https://İ.example/mcp", to: "https://i.example/mcp", wantErr: ErrCrossOriginRedirect}, @@ -108,6 +118,7 @@ func TestSameOriginRedirectPolicy(t *testing.T) { } } +// TestOAuthRedirectPolicy checks which redirects the OAuth handler's default client follows. func TestOAuthRedirectPolicy(t *testing.T) { tests := []struct { name string @@ -157,6 +168,7 @@ func redirectHop(t *testing.T, method, from, to string, hops int) (*http.Request return next, via } +// TestStreamableHTTP_RefusesCrossOriginRedirect checks that a request is not sent to another origin, whatever the redirect status. func TestStreamableHTTP_RefusesCrossOriginRedirect(t *testing.T) { for _, tt := range redirectStatuses { t.Run(tt.name, func(t *testing.T) { @@ -179,6 +191,7 @@ func TestStreamableHTTP_RefusesCrossOriginRedirect(t *testing.T) { } } +// TestStreamableHTTP_SendNotificationRefusesCrossOriginRedirect checks that notifications get the same redirect policy as requests. func TestStreamableHTTP_SendNotificationRefusesCrossOriginRedirect(t *testing.T) { hits, attackerURL := newRedirectAttacker(t) srv := newRedirectingServer(t, attackerURL+"/steal", http.StatusTemporaryRedirect) @@ -197,6 +210,7 @@ func TestStreamableHTTP_SendNotificationRefusesCrossOriginRedirect(t *testing.T) assert.Zero(t, hits.Load(), "the redirect target must not be contacted") } +// TestStreamableHTTP_FollowsSameOriginRedirect checks that a same-origin 308 is followed with its headers and body. func TestStreamableHTTP_FollowsSameOriginRedirect(t *testing.T) { var gotKey atomic.Value var gotBody atomic.Bool @@ -227,6 +241,7 @@ func TestStreamableHTTP_FollowsSameOriginRedirect(t *testing.T) { assert.True(t, gotBody.Load(), "a same-origin 308 must resend the request body") } +// TestStreamableHTTP_CustomClientKeepsItsRedirectPolicy checks that a client from WithHTTPBasicClient is used unchanged. func TestStreamableHTTP_CustomClientKeepsItsRedirectPolicy(t *testing.T) { hits, attackerURL := newRedirectAttacker(t) srv := newRedirectingServer(t, attackerURL+"/elsewhere", http.StatusFound) @@ -239,6 +254,7 @@ func TestStreamableHTTP_CustomClientKeepsItsRedirectPolicy(t *testing.T) { assert.Equal(t, int64(1), hits.Load(), "a caller-supplied client must keep its own redirect policy") } +// TestStreamableHTTP_TimeoutOptionKeepsRedirectPolicy checks that WithHTTPTimeout does not replace the default client's redirect policy. func TestStreamableHTTP_TimeoutOptionKeepsRedirectPolicy(t *testing.T) { hits, attackerURL := newRedirectAttacker(t) srv := newRedirectingServer(t, attackerURL+"/steal", http.StatusTemporaryRedirect) @@ -252,6 +268,7 @@ func TestStreamableHTTP_TimeoutOptionKeepsRedirectPolicy(t *testing.T) { assert.Zero(t, hits.Load()) } +// TestSSE_RefusesCrossOriginRedirect checks that the SSE stream request is not sent to another origin. func TestSSE_RefusesCrossOriginRedirect(t *testing.T) { for _, tt := range redirectStatuses { t.Run(tt.name, func(t *testing.T) { @@ -270,6 +287,7 @@ func TestSSE_RefusesCrossOriginRedirect(t *testing.T) { } } +// TestSSE_RejectsEndpointOnOtherOrigin checks that an endpoint event naming another scheme, host or port is ignored. func TestSSE_RejectsEndpointOnOtherOrigin(t *testing.T) { tests := []struct { name string @@ -311,6 +329,7 @@ func TestSSE_RejectsEndpointOnOtherOrigin(t *testing.T) { } } +// TestSSE_AcceptsEndpointOnSameOrigin checks that a relative endpoint on the connection's origin is accepted. func TestSSE_AcceptsEndpointOnSameOrigin(t *testing.T) { srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "text/event-stream") @@ -328,6 +347,7 @@ func TestSSE_AcceptsEndpointOnSameOrigin(t *testing.T) { require.NoError(t, tr.Start(t.Context())) } +// TestSSE_CustomClientKeepsItsRedirectPolicy checks that a client from WithHTTPClient is used unchanged. func TestSSE_CustomClientKeepsItsRedirectPolicy(t *testing.T) { hits, attackerURL := newRedirectAttacker(t) srv := newRedirectingServer(t, attackerURL+"/elsewhere", http.StatusFound) @@ -340,6 +360,7 @@ func TestSSE_CustomClientKeepsItsRedirectPolicy(t *testing.T) { assert.Equal(t, int64(1), hits.Load(), "a caller-supplied client must keep its own redirect policy") } +// TestOAuthHandler_DefaultClientRefusesCrossOriginTokenRedirect checks that a token request body never reaches another origin. func TestOAuthHandler_DefaultClientRefusesCrossOriginTokenRedirect(t *testing.T) { for _, tt := range []struct { name string @@ -370,6 +391,7 @@ func TestOAuthHandler_DefaultClientRefusesCrossOriginTokenRedirect(t *testing.T) } } +// TestOAuthHandler_DefaultClientFollowsDiscoveryRedirect checks that metadata discovery may still be redirected to another origin. func TestOAuthHandler_DefaultClientFollowsDiscoveryRedirect(t *testing.T) { var hits atomic.Int64 metadata := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 4c8da10edfaf01b394fea459899b71e4ad235646 Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 21:54:01 +0800 Subject: [PATCH 5/6] fix(transport): keep IPv6 zone case when comparing origins Zones are interface names, which can differ only in case, so fe80::1%eth0 and fe80::1%ETH0 may be different peers. Parse IP literals before lowercasing and lowercase only host names. --- client/transport/redirect.go | 13 +++++++++---- client/transport/redirect_test.go | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/client/transport/redirect.go b/client/transport/redirect.go index d320fb7f6..9441a033a 100644 --- a/client/transport/redirect.go +++ b/client/transport/redirect.go @@ -79,16 +79,21 @@ func sameOrigin(a, b *url.URL) bool { return origin(a) == origin(b) } -// origin returns a normalized scheme://host:port string. The scheme and host -// are lowercased (ASCII only), an IP literal is written in its canonical form -// (so "[0:0::1]" and "[::1]" compare equal), a missing port becomes the +// origin returns a normalized scheme://host:port string. The scheme and a +// host name are lowercased (ASCII only), an IP literal is written in its +// canonical form (so "[0:0::1]" and "[::1]" compare equal) with its zone +// unchanged, a missing port becomes the // scheme's default, and a numeric port loses leading zeros, so ":0443" and // ":443" compare equal as net/http dials them. func origin(u *url.URL) string { scheme := asciiLower(u.Scheme) - host := asciiLower(u.Hostname()) + host := u.Hostname() if addr, err := netip.ParseAddr(host); err == nil { + // netip writes hex digits in lowercase and keeps the zone as is: + // zone names are interface names, which can differ only in case. host = addr.String() + } else { + host = asciiLower(host) } port := u.Port() if n, err := strconv.ParseUint(port, 10, 16); err == nil { diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go index 2e3065e13..b4ebb3810 100644 --- a/client/transport/redirect_test.go +++ b/client/transport/redirect_test.go @@ -87,6 +87,10 @@ func TestSameOriginRedirectPolicy(t *testing.T) { {name: "IPv6 full and compressed form", from: "http://[2001:db8::1]/mcp", to: "http://[2001:0db8:0000:0000:0000:0000:0000:0001]/v2"}, {name: "IPv6 hex case", from: "http://[2001:DB8::A]/mcp", to: "http://[2001:db8::a]/v2"}, {name: "IPv4-mapped IPv6 is not IPv4", from: "http://127.0.0.1/mcp", to: "http://[::ffff:127.0.0.1]/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "IPv6 same zone", from: "http://[fe80::1%25eth0]:8080/mcp", to: "http://[FE80:0::1%25eth0]:8080/v2"}, + {name: "IPv6 zone case differs", from: "http://[fe80::1%25eth0]:8080/mcp", to: "http://[fe80::1%25ETH0]:8080/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "IPv6 other zone", from: "http://[fe80::1%25eth0]:8080/mcp", to: "http://[fe80::1%25eth1]:8080/mcp", wantErr: ErrCrossOriginRedirect}, + {name: "IPv6 zone added", from: "http://[fe80::1]:8080/mcp", to: "http://[fe80::1%25eth0]:8080/mcp", wantErr: ErrCrossOriginRedirect}, {name: "IPv6 other address", from: "http://[::1]/mcp", to: "http://[::2]/mcp", wantErr: ErrCrossOriginRedirect}, {name: "IPv6 other port", from: "http://[::1]:8080/mcp", to: "http://[::1]:9090/mcp", wantErr: ErrCrossOriginRedirect}, {name: "dotted capital I is not folded onto i", from: "https://i.example/mcp", to: "https://İ.example/mcp", wantErr: ErrCrossOriginRedirect}, From c7131c97cf5b3de956c4a30a8c84083281753cbe Mon Sep 17 00:00:00 2001 From: Muhammad Hamizi Jaminan Date: Sat, 7 Nov 2026 22:03:29 +0800 Subject: [PATCH 6/6] test(transport): cover RefreshToken against a cross-origin token redirect Exercise the redirect through OAuthHandler.RefreshToken instead of the client alone, so a handler path that bypasses the default client would be caught. Without the policy the refresh token and client secret reach the redirect target. --- client/transport/redirect_test.go | 50 +++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/client/transport/redirect_test.go b/client/transport/redirect_test.go index b4ebb3810..a88d52461 100644 --- a/client/transport/redirect_test.go +++ b/client/transport/redirect_test.go @@ -2,6 +2,7 @@ package transport import ( "context" + "encoding/json" "io" "net/http" "net/http/httptest" @@ -395,6 +396,55 @@ func TestOAuthHandler_DefaultClientRefusesCrossOriginTokenRedirect(t *testing.T) } } +// TestOAuthHandler_RefreshTokenRefusesCrossOriginRedirect checks, through +// RefreshToken rather than the client alone, that a refresh request answered +// with a cross-origin 307 or 308 never reaches the redirect target. +func TestOAuthHandler_RefreshTokenRefusesCrossOriginRedirect(t *testing.T) { + for _, tt := range []struct { + name string + status int + }{ + {name: "307 Temporary Redirect", status: http.StatusTemporaryRedirect}, + {name: "308 Permanent Redirect", status: http.StatusPermanentRedirect}, + } { + t.Run(tt.name, func(t *testing.T) { + hits, attackerURL := newRedirectAttacker(t) + + var issuer string + authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/.well-known/oauth-authorization-server": + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "issuer": issuer, + "authorization_endpoint": issuer + "/authorize", + "token_endpoint": issuer + "/token", + }) + case "/token": + http.Redirect(w, r, attackerURL+"/token", tt.status) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(authServer.Close) + issuer = authServer.URL + + h := NewOAuthHandler(OAuthConfig{ + ClientID: "test-client", + ClientSecret: redirectTestSecret, + RedirectURI: "http://localhost/callback", + CredentialsIssuer: issuer, + AuthServerMetadataURL: issuer + "/.well-known/oauth-authorization-server", + }) + + _, err := h.RefreshToken(t.Context(), "refresh-"+redirectTestSecret) + require.Error(t, err) + assert.ErrorIs(t, err, ErrCrossOriginRedirect) + assert.Zero(t, hits.Load(), "the refresh token and client secret must not reach another origin") + }) + } +} + // TestOAuthHandler_DefaultClientFollowsDiscoveryRedirect checks that metadata discovery may still be redirected to another origin. func TestOAuthHandler_DefaultClientFollowsDiscoveryRedirect(t *testing.T) { var hits atomic.Int64