Repository navigation
fix(transport): refuse cross-origin redirects in default HTTP clients (GO-2026-6911) #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
390448b
fix(transport): refuse cross-origin redirects in default HTTP clients
hymns 8ce69db
fix(transport): tighten origin checks and cover OAuth credential requ…
hymns 41a7375
test(transport): fix names of the dotted capital I cases
hymns 8d778b5
fix(transport): compare IP literals in canonical form
hymns 4c8da10
fix(transport): keep IPv6 zone case when comparing origins
hymns c7131c9
test(transport): cover RefreshToken against a cross-origin token redi…
hymns File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
|
|
||
| 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} | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.