Merge branch 'modelcontextprotocol:main' into issue/552

Author: IAmSurajBobade

auth/client.go

@@ -8,26 +8,19 @@ package auth
 
 import (
 	"bytes"
-	"context"
 	"errors"
 	"io"
 	"net/http"
 	"sync"
 
-	"github.com/modelcontextprotocol/go-sdk/oauthex"
 	"golang.org/x/oauth2"
 )
 
 // An OAuthHandler conducts an OAuth flow and returns a [oauth2.TokenSource] if the authorization
 // is approved, or an error if not.
-type OAuthHandler func(context.Context, OAuthHandlerArgs) (oauth2.TokenSource, error)
-
-// OAuthHandlerArgs are arguments to an [OAuthHandler].
-type OAuthHandlerArgs struct {
-	// The URL to fetch protected resource metadata, extracted from the WWW-Authenticate header.
-	// Empty if not present or there was an error obtaining it.
-	ResourceMetadataURL string
-}
+// The handler receives the HTTP request and response that triggered the authentication flow.
+// To obtain the protected resource metadata, call [oauthex.GetProtectedResourceMetadataFromHeader].
+type OAuthHandler func(req *http.Request, res *http.Response) (oauth2.TokenSource, error)
 
 // HTTPTransport is an [http.RoundTripper] that follows the MCP
 // OAuth protocol when it encounters a 401 Unauthorized response.
@@ -112,10 +105,7 @@ func (t *HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	// TODO: We hold the lock for the entire OAuth flow. This could be a long
 	// time. Is there a better way?
 	if _, ok := t.opts.Base.(*oauth2.Transport); !ok {
-		authHeaders := resp.Header[http.CanonicalHeaderKey("WWW-Authenticate")]
-		ts, err := t.handler(req.Context(), OAuthHandlerArgs{
-			ResourceMetadataURL: extractResourceMetadataURL(authHeaders),
-		})
+		ts, err := t.handler(req, resp)
 		if err != nil {
 			return nil, err
 		}
@@ -131,11 +121,3 @@ func (t *HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 
 	return t.opts.Base.RoundTrip(req)
 }
-
-func extractResourceMetadataURL(authHeaders []string) string {
-	cs, err := oauthex.ParseWWWAuthenticate(authHeaders)
-	if err != nil {
-		return ""
-	}
-	return oauthex.ResourceMetadataURL(cs)
-}

auth/client_test.go

@@ -7,7 +7,6 @@
 package auth
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -65,10 +64,11 @@ func TestHTTPTransport(t *testing.T) {
 
 	t.Run("successful auth flow", func(t *testing.T) {
 		var handlerCalls int
-		handler := func(ctx context.Context, args OAuthHandlerArgs) (oauth2.TokenSource, error) {
+		handler := func(req *http.Request, res *http.Response) (oauth2.TokenSource, error) {
 			handlerCalls++
-			if args.ResourceMetadataURL != "http://metadata.example.com" {
-				t.Errorf("handler got metadata URL %q, want %q", args.ResourceMetadataURL, "http://metadata.example.com")
+			// Verify that the response has the expected WWW-Authenticate header
+			if res.Header.Get("WWW-Authenticate") != `Bearer resource_metadata="http://metadata.example.com"` {
+				t.Errorf("handler got WWW-Authenticate header %q", res.Header.Get("WWW-Authenticate"))
 			}
 			return fakeTokenSource, nil
 		}
@@ -108,9 +108,10 @@ func TestHTTPTransport(t *testing.T) {
 	})
 
 	t.Run("request body is cloned", func(t *testing.T) {
-		handler := func(ctx context.Context, args OAuthHandlerArgs) (oauth2.TokenSource, error) {
-			if args.ResourceMetadataURL != "http://metadata.example.com" {
-				t.Errorf("handler got metadata URL %q, want %q", args.ResourceMetadataURL, "http://metadata.example.com")
+		handler := func(req *http.Request, res *http.Response) (oauth2.TokenSource, error) {
+			// Verify that the response has the expected WWW-Authenticate header
+			if res.Header.Get("WWW-Authenticate") != `Bearer resource_metadata="http://metadata.example.com"` {
+				t.Errorf("handler got WWW-Authenticate header %q", res.Header.Get("WWW-Authenticate"))
 			}
 			return fakeTokenSource, nil
 		}
@@ -134,7 +135,7 @@ func TestHTTPTransport(t *testing.T) {
 
 	t.Run("handler returns error", func(t *testing.T) {
 		handlerErr := errors.New("user rejected auth")
-		handler := func(ctx context.Context, args OAuthHandlerArgs) (oauth2.TokenSource, error) {
+		handler := func(req *http.Request, res *http.Response) (oauth2.TokenSource, error) {
 			return nil, handlerErr
 		}
 

docs/README.md

@@ -38,3 +38,9 @@ protocol.
 # TroubleShooting
 
 See [troubleshooting.md](troubleshooting.md) for a troubleshooting guide.
+
+# Rough edges
+
+See [rough_edges.md](rough_edges.md) for a list of rough edges or API
+oversights that can't be addressed due to our compatibility promise. We'll
+revisit these if/when we move to a v2 of the SDK.

docs/rough_edges.md

@@ -0,0 +1,12 @@
+<!-- Autogenerated by weave; DO NOT EDIT -->
+# Rough edges: API decisions to reconsider for v2
+
+This file collects a list of API oversights or rough edges that we've uncovered
+post v1.0.0, along with their current workarounds. These issues can't be
+addressed without breaking backward compatibility, but we'll revisit them for
+v2.
+
+- `EventStore.Open` is unnecessary. This was an artifact of an earlier version
+  of the SDK where event persistence and delivery were combined.
+  
+  **Workaround**: `Open` may be implemented as a no-op.

internal/docs/README.src.md

@@ -37,3 +37,9 @@ protocol.
 # TroubleShooting
 
 See [troubleshooting.md](troubleshooting.md) for a troubleshooting guide.
+
+# Rough edges
+
+See [rough_edges.md](rough_edges.md) for a list of rough edges or API
+oversights that can't be addressed due to our compatibility promise. We'll
+revisit these if/when we move to a v2 of the SDK.

internal/docs/doc.go

@@ -8,6 +8,7 @@
 //go:generate weave -o ../../docs/client.md ./client.src.md
 //go:generate weave -o ../../docs/server.md ./server.src.md
 //go:generate weave -o ../../docs/troubleshooting.md ./troubleshooting.src.md
+//go:generate weave -o ../../docs/rough_edges.md ./rough_edges.src.md
 
 // The doc package generates the documentation at /doc, via go:generate.
 //

internal/docs/rough_edges.src.md

@@ -0,0 +1,11 @@
+# Rough edges: API decisions to reconsider for v2
+
+This file collects a list of API oversights or rough edges that we've uncovered
+post v1.0.0, along with their current workarounds. These issues can't be
+addressed without breaking backward compatibility, but we'll revisit them for
+v2.
+
+- `EventStore.Open` is unnecessary. This was an artifact of an earlier version
+  of the SDK where event persistence and delivery were combined.
+  
+  **Workaround**: `Open` may be implemented as a no-op.

mcp/streamable.go

@@ -1377,13 +1377,14 @@ func (c *streamableClientConn) connectStandaloneSSE() {
 		resp.Body.Close()
 		return
 	}
-	if resp.StatusCode == http.StatusNotFound && !c.strict {
-		// modelcontextprotocol/gosdk#393: some servers return NotFound instead
-		// of MethodNotAllowed for the standalone SSE stream.
+	if resp.StatusCode >= 400 && resp.StatusCode < 500 && !c.strict {
+		// modelcontextprotocol/go-sdk#393,#610: some servers return NotFound or
+		// other status codes instead of MethodNotAllowed for the standalone SSE
+		// stream.
 		//
 		// Treat this like MethodNotAllowed in non-strict mode.
 		if c.logger != nil {
-			c.logger.Warn("got 404 instead of 405 for standalone SSE stream")
+			c.logger.Warn(fmt.Sprintf("got %d instead of 405 for standalone SSE stream", resp.StatusCode))
 		}
 		resp.Body.Close()
 		return

mcp/streamable_client_test.go

@@ -239,7 +239,11 @@ func TestStreamableClientGETHandling(t *testing.T) {
 	}{
 		{http.StatusOK, ""},
 		{http.StatusMethodNotAllowed, ""},
-		{http.StatusBadRequest, "standalone SSE"},
+		// The client error status code is not treated as an error in non-strict
+		// mode.
+		{http.StatusNotFound, ""},
+		{http.StatusBadRequest, ""},
+		{http.StatusInternalServerError, "standalone SSE"},
 	}
 
 	for _, test := range tests {
@@ -305,7 +309,11 @@ func TestStreamableClientStrictness(t *testing.T) {
 		{"strict initialized", true, http.StatusOK, http.StatusMethodNotAllowed, true},
 		{"unstrict initialized", false, http.StatusOK, http.StatusMethodNotAllowed, false},
 		{"strict GET", true, http.StatusAccepted, http.StatusNotFound, true},
-		{"unstrict GET", false, http.StatusOK, http.StatusNotFound, false},
+		// The client error status code is not treated as an error in non-strict
+		// mode.
+		{"unstrict GET on StatusNotFound", false, http.StatusOK, http.StatusNotFound, false},
+		{"unstrict GET on StatusBadRequest", false, http.StatusOK, http.StatusBadRequest, false},
+		{"GET on InternlServerError", false, http.StatusOK, http.StatusInternalServerError, true},
 	}
 	for _, test := range tests {
 		t.Run(test.label, func(t *testing.T) {