Merge branch 'main' into guglielmoc/SEP-2243_http_standardization

Author: guglielmo-san

docs/rough_edges.md

@@ -59,3 +59,7 @@ v2.
   wrapper) we need to first unmarshal into a `map[string]any` in order to do
   server-side validation of required fields. CallToolParams could have just had
   a map[string]any.
+
+- `StreamableHTTPOptions.CrossOriginProtection` should not have been part of
+  the SDK API. Cross-origin protection is a general HTTP concern, not specific
+  to MCP, and can be applied as standard HTTP middleware.

examples/http/README.md

@@ -15,7 +15,7 @@ The example implements:
 ```bash
 go run . server
 ```
-This starts an MCP server on `http://localhost:8080` (default) that provides a `cityTime` tool.
+This starts an MCP server on `http://localhost:8000` (default) that provides a `cityTime` tool.
 
 To run a client in another terminal:
 

internal/docs/rough_edges.src.md

@@ -58,3 +58,7 @@ v2.
   wrapper) we need to first unmarshal into a `map[string]any` in order to do
   server-side validation of required fields. CallToolParams could have just had
   a map[string]any.
+
+- `StreamableHTTPOptions.CrossOriginProtection` should not have been part of
+  the SDK API. Cross-origin protection is a general HTTP concern, not specific
+  to MCP, and can be applied as standard HTTP middleware.

mcp/sse.go

@@ -10,6 +10,7 @@ import (
 	"crypto/rand"
 	"fmt"
 	"io"
+	"mime"
 	"net"
 	"net/http"
 	"net/url"
@@ -64,14 +65,6 @@ type SSEOptions struct {
 	// Only disable this if you understand the security implications.
 	// See: https://modelcontextprotocol.io/specification/2025-11-25/basic/security_best_practices#local-mcp-server-compromise
 	DisableLocalhostProtection bool
-
-	// CrossOriginProtection allows to customize cross-origin protection.
-	// The deny handler set in the CrossOriginProtection through SetDenyHandler
-	// is ignored.
-	// If nil, default (zero-value) cross-origin protection will be used.
-	// Use `disablecrossoriginprotection` MCPGODEBUG compatibility parameter
-	// to disable the default protection until v1.7.0.
-	CrossOriginProtection *http.CrossOriginProtection
 }
 
 // NewSSEHandler returns a new [SSEHandler] that creates and manages MCP
@@ -97,10 +90,6 @@ func NewSSEHandler(getServer func(request *http.Request) *Server, opts *SSEOptio
 		s.opts = *opts
 	}
 
-	if s.opts.CrossOriginProtection == nil {
-		s.opts.CrossOriginProtection = &http.CrossOriginProtection{}
-	}
-
 	return s
 }
 
@@ -212,20 +201,13 @@ func (h *SSEHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		}
 	}
 
-	if disablecrossoriginprotection != "1" {
-		// Verify the 'Origin' header to protect against CSRF attacks.
-		if err := h.opts.CrossOriginProtection.Check(req); err != nil {
-			http.Error(w, err.Error(), http.StatusForbidden)
+	// Validate 'Content-Type' header.
+	if req.Method == http.MethodPost {
+		mediaType, _, err := mime.ParseMediaType(req.Header.Get("Content-Type"))
+		if err != nil || mediaType != "application/json" {
+			http.Error(w, "Content-Type must be 'application/json'", http.StatusUnsupportedMediaType)
 			return
 		}
-		// Validate 'Content-Type' header.
-		if req.Method == http.MethodPost {
-			contentType := req.Header.Get("Content-Type")
-			if contentType != "application/json" {
-				http.Error(w, "Content-Type must be 'application/json'", http.StatusUnsupportedMediaType)
-				return
-			}
-		}
 	}
 
 	sessionID := req.URL.Query().Get("sessionid")

mcp/sse_test.go

@@ -12,7 +12,6 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"sync/atomic"
 	"testing"
 
@@ -320,77 +319,3 @@ func TestSSELocalhostProtection(t *testing.T) {
 		})
 	}
 }
-
-func TestSSEOriginProtection(t *testing.T) {
-	server := NewServer(testImpl, nil)
-
-	tests := []struct {
-		name           string
-		protection     *http.CrossOriginProtection
-		requestOrigin  string
-		wantStatusCode int
-	}{
-		{
-			name:           "default protection with Origin header",
-			protection:     nil,
-			requestOrigin:  "https://example.com",
-			wantStatusCode: http.StatusForbidden,
-		},
-		{
-			name: "custom protection with trusted origin and same Origin",
-			protection: func() *http.CrossOriginProtection {
-				p := http.NewCrossOriginProtection()
-				if err := p.AddTrustedOrigin("https://example.com"); err != nil {
-					t.Fatal(err)
-				}
-				return p
-			}(),
-			requestOrigin:  "https://example.com",
-			wantStatusCode: http.StatusNotFound, // origin accepted; session not found
-		},
-		{
-			name: "custom protection with trusted origin and different Origin",
-			protection: func() *http.CrossOriginProtection {
-				p := http.NewCrossOriginProtection()
-				if err := p.AddTrustedOrigin("https://example.com"); err != nil {
-					t.Fatal(err)
-				}
-				return p
-			}(),
-			requestOrigin:  "https://malicious.com",
-			wantStatusCode: http.StatusForbidden,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			opts := &SSEOptions{
-				CrossOriginProtection: tt.protection,
-			}
-			handler := NewSSEHandler(func(req *http.Request) *Server { return server }, opts)
-			httpServer := httptest.NewServer(handler)
-			defer httpServer.Close()
-
-			// Use POST with a valid session-like URL to test origin protection
-			// without creating a hanging GET connection.
-			reqReader := strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"ping"}`)
-			req, err := http.NewRequest(http.MethodPost, httpServer.URL+"?sessionid=nonexistent", reqReader)
-			if err != nil {
-				t.Fatal(err)
-			}
-			req.Header.Set("Content-Type", "application/json")
-			req.Header.Set("Origin", tt.requestOrigin)
-
-			resp, err := http.DefaultClient.Do(req)
-			if err != nil {
-				t.Fatal(err)
-			}
-			defer resp.Body.Close()
-
-			if got := resp.StatusCode; got != tt.wantStatusCode {
-				body, _ := io.ReadAll(resp.Body)
-				t.Errorf("Status code: got %d, want %d (body: %s)", got, tt.wantStatusCode, body)
-			}
-		})
-	}
-}

mcp/streamable.go

@@ -174,9 +174,15 @@ type StreamableHTTPOptions struct {
 	// CrossOriginProtection allows to customize cross-origin protection.
 	// The deny handler set in the CrossOriginProtection through SetDenyHandler
 	// is ignored.
-	// If nil, default (zero-value) cross-origin protection will be used.
-	// Use `disablecrossoriginprotection` MCPGODEBUG compatibility parameter
-	// to disable the default protection until v1.7.0.
+	// If nil, no cross-origin protection is applied. Use the `enableoriginverification`
+	// MCPGODEBUG compatibility parameter to enable the default protection until v1.8.0.
+	//
+	// Deprecated: wrap the handler with cross-origin protection middleware
+	// instead. For example:
+	//
+	//   handler := mcp.NewStreamableHTTPHandler(...)
+	//   protection := http.NewCrossOriginProtection()
+	//   protectedHandler := protection.Handler(handler)
 	CrossOriginProtection *http.CrossOriginProtection
 }
 
@@ -196,7 +202,7 @@ func NewStreamableHTTPHandler(getServer func(*http.Request) *Server, opts *Strea
 
 	h.opts.Logger = ensureLogger(h.opts.Logger)
 
-	if h.opts.CrossOriginProtection == nil {
+	if h.opts.CrossOriginProtection == nil && enableoriginverification == "1" {
 		h.opts.CrossOriginProtection = &http.CrossOriginProtection{}
 	}
 
@@ -229,15 +235,16 @@ func (h *StreamableHTTPHandler) closeAll() {
 // disablelocalhostprotection is a compatibility parameter that allows to disable
 // DNS rebinding protection, which was added in the 1.4.0 version of the SDK.
 // See the documentation for the mcpgodebug package for instructions how to enable it.
-// The option will be removed in the 1.7.0 version of the SDK.
+// The option will be removed in the 1.6.0 version of the SDK.
 var disablelocalhostprotection = mcpgodebug.Value("disablelocalhostprotection")
 
-// disablecrossoriginprotection is a compatibility parameter that allows to disable
-// the verification of the 'Origin' and 'Content-Type' headers, which was added in
-// the 1.4.1 version of the SDK. See the documentation for the mcpgodebug package
-// for instructions how to enable it.
-// The option will be removed in the 1.7.0 version of the SDK.
-var disablecrossoriginprotection = mcpgodebug.Value("disablecrossoriginprotection")
+// enableoriginverification is a compatibility parameter that restores the
+// default cross-origin protection behavior from v1.4.1-v1.5.0. When set to
+// "1", a zero-value CrossOriginProtection will be applied if none is
+// explicitly provided in StreamableHTTPOptions.
+// See the documentation for the mcpgodebug package for instructions how to enable it.
+// The option will be removed in the 1.8.0 version of the SDK.
+var enableoriginverification = mcpgodebug.Value("enableoriginverification")
 
 func (h *StreamableHTTPHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	// DNS rebinding protection: auto-enabled for localhost servers.
@@ -251,17 +258,18 @@ func (h *StreamableHTTPHandler) ServeHTTP(w http.ResponseWriter, req *http.Reque
 		}
 	}
 
-	if disablecrossoriginprotection != "1" {
+	if h.opts.CrossOriginProtection != nil {
 		// Verify the 'Origin' header to protect against CSRF attacks.
 		if err := h.opts.CrossOriginProtection.Check(req); err != nil {
 			http.Error(w, err.Error(), http.StatusForbidden)
 			return
 		}
-		// Validate 'Content-Type' header.
-		if req.Method == http.MethodPost && baseMediaType(req.Header.Get("Content-Type")) != "application/json" {
-			http.Error(w, "Content-Type must be 'application/json'", http.StatusUnsupportedMediaType)
-			return
-		}
+	}
+
+	// Validate 'Content-Type' header.
+	if req.Method == http.MethodPost && baseMediaType(req.Header.Get("Content-Type")) != "application/json" {
+		http.Error(w, "Content-Type must be 'application/json'", http.StatusUnsupportedMediaType)
+		return
 	}
 
 	// Allow multiple 'Accept' headers.
@@ -1799,14 +1807,14 @@ func (c *streamableClientConn) Write(ctx context.Context, msg jsonrpc.Message) e
 			// Failure to set headers means that the request was not sent.
 			// Wrap with ErrRejected so the jsonrpc2 connection doesn't set writeErr
 			// and permanently break the connection.
-			return nil, nil, fmt.Errorf("%s: %w: %v", requestSummary, jsonrpc2.ErrRejected, err)
+			return nil, nil, fmt.Errorf("%s: %w: %w", requestSummary, jsonrpc2.ErrRejected, err)
 		}
 		resp, err := c.client.Do(req)
 		if err != nil {
 			// Any error from client.Do means the request didn't reach the server.
 			// Wrap with ErrRejected so the jsonrpc2 connection doesn't set writeErr
 			// and permanently break the connection.
-			err = fmt.Errorf("%s: %w: %v", requestSummary, jsonrpc2.ErrRejected, err)
+			err = fmt.Errorf("%s: %w: %w", requestSummary, jsonrpc2.ErrRejected, err)
 		}
 		return req, resp, err
 	}
@@ -1818,6 +1826,22 @@ func (c *streamableClientConn) Write(ctx context.Context, msg jsonrpc.Message) e
 
 	if (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) && c.oauthHandler != nil {
 		if err := c.oauthHandler.Authorize(ctx, req, resp); err != nil {
+			// If the caller's context was cancelled while we were running the
+			// authorization flow, treat the connection as failed so subsequent
+			// operations on it (e.g. the cancellation notify the call layer
+			// sends in response to ctx cancellation) short-circuit instead of
+			// re-invoking the OAuth handler. Otherwise the user gets prompted
+			// to authorize a request they have already abandoned. See #882.
+			//
+			// We check ctx.Err() rather than the error returned by Authorize,
+			// because the handler is user-implemented and may return an error
+			// that does not wrap context.Canceled (e.g. a custom sentinel or
+			// a fmt.Errorf with %v). The context itself is the authoritative
+			// source for whether the caller abandoned the request.
+			ctxErr := ctx.Err()
+			if errors.Is(ctxErr, context.Canceled) || errors.Is(ctxErr, context.DeadlineExceeded) {
+				c.fail(fmt.Errorf("%s: authorization cancelled: %w", requestSummary, err))
+			}
 			// Wrap with ErrRejected so the jsonrpc2 connection doesn't set writeErr
 			// and permanently break the connection.
 			// Wrap the authorization error as well for client inspection.

mcp/streamable_client_test.go

@@ -1017,6 +1017,94 @@ func TestStreamableClientOAuth_401(t *testing.T) {
 	}
 }
 
+// blockingCountingOAuthHandler is an OAuthHandler that blocks inside
+// Authorize until the caller's context is cancelled, then returns a custom
+// error that does NOT wrap context.Canceled. This mirrors real-world OAuth
+// handlers that catch the cancellation internally and surface their own
+// error type. The fix for #882 checks ctx.Err() directly rather than
+// relying on the error from Authorize, so this must still trigger c.fail().
+// It records how many times Authorize is invoked.
+type blockingCountingOAuthHandler struct {
+	mu        sync.Mutex
+	callCount int
+}
+
+func (h *blockingCountingOAuthHandler) TokenSource(ctx context.Context) (oauth2.TokenSource, error) {
+	return nil, nil
+}
+
+func (h *blockingCountingOAuthHandler) Authorize(ctx context.Context, req *http.Request, resp *http.Response) error {
+	h.mu.Lock()
+	h.callCount++
+	h.mu.Unlock()
+	// Block until the caller's context is cancelled, mirroring an
+	// interactive OAuth flow that the user has abandoned.
+	<-ctx.Done()
+	// Return a custom error that does not wrap context.Canceled, as a
+	// real-world handler might. The code under test must check ctx.Err()
+	// to detect the cancellation, not this error.
+	return fmt.Errorf("oauth flow interrupted")
+}
+
+func (h *blockingCountingOAuthHandler) Calls() int {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	return h.callCount
+}
+
+// TestStreamableClientOAuth_CancelledAuthorize_NoReprompt is a regression
+// test for #882. When OAuthHandler.Authorize returns a context-cancelled
+// error, the connection must enter a failed state so that the cancellation
+// notification the call layer sends in response to ctx cancellation does
+// not flow back through the same broken auth path and re-invoke Authorize.
+func TestStreamableClientOAuth_CancelledAuthorize_NoReprompt(t *testing.T) {
+	handler := &blockingCountingOAuthHandler{}
+
+	fake := &fakeStreamableServer{
+		t: t,
+		responses: fakeResponses{
+			{"POST", "", methodInitialize, ""}: {
+				header: header{
+					"Content-Type":  "application/json",
+					sessionIDHeader: "123",
+				},
+				body: jsonBody(t, initResp),
+			},
+		},
+	}
+	verifier := func(ctx context.Context, token string, req *http.Request) (*auth.TokenInfo, error) {
+		return &auth.TokenInfo{Expiration: time.Now().Add(time.Hour)}, nil
+	}
+	httpServer := httptest.NewServer(auth.RequireBearerToken(verifier, nil)(fake))
+	t.Cleanup(httpServer.Close)
+
+	transport := &StreamableClientTransport{
+		Endpoint:     httpServer.URL,
+		OAuthHandler: handler,
+	}
+	client := NewClient(testImpl, nil)
+
+	// Use a context with a tight deadline so the cancellation path runs
+	// while the auth flow is in progress.
+	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
+	defer cancel()
+
+	_, err := client.Connect(ctx, transport, nil)
+	if err == nil {
+		t.Fatal("expected client.Connect to fail")
+	}
+
+	// Give the cancellation Notify path a moment to (try to) run.
+	time.Sleep(50 * time.Millisecond)
+
+	// Authorize should be invoked exactly once. The bug in #882 caused
+	// it to be invoked a second time when the call layer sent the
+	// cancellation notification through the same auth-broken connection.
+	if got := handler.Calls(); got != 1 {
+		t.Errorf("expected Authorize to be called exactly 1 time, got %d", got)
+	}
+}
+
 func TestTokenInfo(t *testing.T) {
 	ctx := context.Background()
 

mcp/streamable_test.go

@@ -2839,10 +2839,10 @@ func TestStreamableOriginProtection(t *testing.T) {
 		wantStatusCode int
 	}{
 		{
-			name:           "default protection with Origin header",
+			name:           "no protection with Origin header",
 			protection:     nil,
 			requestOrigin:  "https://example.com",
-			wantStatusCode: http.StatusForbidden,
+			wantStatusCode: http.StatusOK,
 		},
 		{
 			name: "custom protection with trusted origin and same Origin",