From f0f8bf91109089ddd12f3278a649ad8c6b66db68 Mon Sep 17 00:00:00 2001 From: Rafael Dantas Justo Date: Tue, 28 Jul 2026 15:16:01 -0300 Subject: [PATCH 1/2] Feature: Upgrade MCP SDK to v1.7.0 and adapt to stateless protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-07-28 revision drops session-based architecture. The SDK negotiates the highest mutually supported version, so older clients keep working. Four fixes the bump required: - request.ResponseWriter implements Flush/Unwrap, so the SDK's SSE writes through http.ResponseController actually flush instead of no-opping. Log capture is capped at 64 KiB; SSE streams made it unbounded. - tools/list is marked cacheScope "private" โ€” it is filtered per OAuth scope, and the SDK defaults cacheable results to "public". - cmd/mcp-stdio drops its keepalive goroutine, which duplicated ServerOptions.KeepAlive and wrote id-less errors onto stdout. - server/discover (SEP-2575) bypasses auth, as initialize already does. Also pins MaxRequestBodyBytes to the 10 MB already enforced (the SDK default would tighten it to 4 MiB) and stops advertising the deprecated logging capability, which this server never honoured. Stateless servers no longer echo Mcp-Session-Id and answer DELETE with 405; both are spec-permitted, and MCPGODEBUG=allowsessionsinstateless=1 restores the old behaviour if a client needs it. See cmd/mcp-http/README.md. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 10 ++ cmd/mcp-http/README.md | 40 ++++++ cmd/mcp-http/main.go | 4 + cmd/mcp-stdio/main.go | 21 +-- go.mod | 3 +- go.sum | 6 +- internal/auth/bypass.go | 10 ++ internal/auth/bypass_test.go | 79 +++++++++++ internal/config/cache_test.go | 217 ++++++++++++++++++++++++++++++ internal/config/config.go | 23 +++- internal/request/response.go | 59 +++++++- internal/request/response_test.go | 107 +++++++++++++++ 12 files changed, 553 insertions(+), 26 deletions(-) create mode 100644 internal/auth/bypass_test.go create mode 100644 internal/config/cache_test.go create mode 100644 internal/request/response_test.go diff --git a/AGENTS.md b/AGENTS.md index 8d48bb1e..f21288f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,16 @@ Common variables (subset; see command READMEs for complete lists): - Run `go test ./internal/twprojects` until green. - `docs/tool-reference.md` is generated by `cmd/docs-gen` and guarded by `go test`; ordinary tool add/remove/rename is caught automatically (regenerate with `go run ./cmd/docs-gen`), but two changes need a manual edit to `cmd/docs-gen/main.go`: flipping `allowDelete` to `true` on a shipped server, or adding a new product package (register it in `products()`). +## MCP SDK upgrades and protocol compatibility +- The SDK (`github.com/modelcontextprotocol/go-sdk`) keeps its Go API stable but does change wire behaviour between minor versions. Read `docs/mcpgodebug.md` in the module cache (`$(go env GOMODCACHE)/github.com/modelcontextprotocol/go-sdk@/docs/mcpgodebug.md`) before and after any bump: it lists every behaviour change and the `MCPGODEBUG==1` escape hatch that restores the old behaviour. Escape hatches are removed two minor versions later, so they are stopgaps, never permanent settings. +- Old clients must keep working. When the SDK adds a protocol method that replaces an older one, support both: `server/discover` (SEP-2575) and `initialize` are both in `auth.methodsWhitelist` for exactly this reason. Never remove a legacy entry from that whitelist. +- `internal/request.ResponseWriter` must keep its `Flush()` and `Unwrap()` methods. The SDK writes Server-Sent Events through `http.ResponseController`, which resolves the flusher by unwrapping โ€” every streamable HTTP response on `/` is an SSE stream. Without them flushing silently degrades to a no-op and streaming clients hang. It also caps the body it captures for logging, because SSE streams live as long as the request. +- Cacheable results (SEP-2549: `tools/list`, `prompts/list`, `resources/list`, `resources/templates/list`, `resources/read`, `server/discover`) default to `cacheScope: "public"`. Anything whose content varies per token โ€” `tools/list` is filtered by OAuth scope โ€” must be set to `cacheScopePrivate` in the receiving middleware in `internal/config/config.go`, or a shared proxy can serve one tenant's response to another. +- Do not advertise capabilities the server does not honour. `logging` is deliberately omitted, and the `listChanged` flags are deliberately false: advertising them invites clients to open a long-lived `subscriptions/listen` stream that a stateless, load-balanced deployment would hold open with nothing to send. `TestCapabilitiesOmitLogging` and `TestCapabilitiesOmitListChanged` guard this. +- Do not add a manual keepalive ping loop. `mcp.ServerOptions.KeepAlive` in `config.NewMCPServer` already drives pings. In `cmd/mcp-stdio` a hand-rolled loop is actively harmful: on failure it has no request to reply to, so the error is written onto stdout, which is the protocol stream. +- Pin limits the SDK also enforces. `mcp.StreamableHTTPOptions.MaxRequestBodyBytes` is set explicitly in `cmd/mcp-http/main.go`; left at zero the SDK applies its own default (4 MiB), silently tightening the limit `limitBodyMiddleware` advertises. +- Client-visible protocol changes belong in the "Protocol Compatibility" section of `cmd/mcp-http/README.md`. + ## Security considerations for agents - Never print or commit bearer tokens. Use env vars only. Redact tokens in logs. - Prefer running the STDIO server with `-read-only` by default during development. diff --git a/cmd/mcp-http/README.md b/cmd/mcp-http/README.md index dc2d9573..42b9f9bc 100644 --- a/cmd/mcp-http/README.md +++ b/cmd/mcp-http/README.md @@ -123,6 +123,46 @@ The server can be configured using the following environment variables: | `DD_ENV` | Environment for Datadog APM | _(uses TW_MCP_ENV)_ | `staging`, `production` | | `DD_VERSION` | Version for Datadog APM | _(uses TW_MCP_VERSION)_ | `v1.0.0` | +## ๐Ÿ”„ Protocol Compatibility + +The server negotiates the highest protocol revision the client also supports, so +clients on older spec revisions keep working unchanged. Both handshakes are +served: `server/discover` (SEP-2575) for stateless clients and `initialize` for +everything older. Both bypass authentication, so pre-auth connector setup works +either way. + +Two wire-level behaviours changed with the `2026-07-28` revision, and the server +runs stateless, so they apply here: + +| Behaviour | Before | Now | Spec basis | +|-----------|--------|-----|------------| +| `Mcp-Session-Id` on responses | A session ID was generated and echoed back | Not sent; an incoming one is ignored | Session IDs are optional; a client that never receives one never sends one | +| `DELETE /` (session termination) | `204 No Content` | `405 Method Not Allowed` | Servers MAY refuse session termination | + +If a client turns out to depend on the old behaviour, the SDK ships an escape +hatch that restores it without a code change: + +```bash +MCPGODEBUG=allowsessionsinstateless=1 +``` + +Set it on the deployment and the server reads `Mcp-Session-Id`, echoes it back, +and answers `DELETE` with `204` again, exactly as before. It is a temporary +compatibility parameter: the SDK removes it in **v1.9.0**, so treat it as a +stopgap while the client is fixed, not a permanent setting. `MCPGODEBUG` takes a +comma-separated list, and the full set of parameters for the pinned SDK version +is documented at + (also in-tree at +`docs/mcpgodebug.md` of the `modelcontextprotocol/go-sdk` module). + +Two further notes for clients: + +- The `logging` capability is no longer advertised. It was deprecated by + SEP-2577 and this server never sent a `notifications/message`, so nothing that + worked before stops working โ€” `logging/setLevel` is still answered. +- `tools/list` responses carry `cacheScope: "private"`. The tool list is filtered + per OAuth token scope, so shared intermediaries must not cache it. + ## ๐Ÿงช Testing ### MCP HTTP CLI diff --git a/cmd/mcp-http/main.go b/cmd/mcp-http/main.go index a9b559c0..784ac81a 100644 --- a/cmd/mcp-http/main.go +++ b/cmd/mcp-http/main.go @@ -64,6 +64,10 @@ func main() { }, &mcp.StreamableHTTPOptions{ Stateless: true, DisableLocalhostProtection: resources.Info.Environment == "dev", + // Pin the body limit to the one limitBodyMiddleware already enforces. + // Left at zero the SDK applies its own DefaultMaxRequestBodyBytes (4 MiB), + // which would silently tighten the limit clients have been coded against. + MaxRequestBodyBytes: maxBodySize, }) mcpSSEServer := mcp.NewSSEHandler(func(*http.Request) *mcp.Server { return mcpServer diff --git a/cmd/mcp-stdio/main.go b/cmd/mcp-stdio/main.go index c8027dbe..71b5bb43 100644 --- a/cmd/mcp-stdio/main.go +++ b/cmd/mcp-stdio/main.go @@ -7,7 +7,6 @@ import ( "fmt" "log/slog" "os" - "time" "github.com/modelcontextprotocol/go-sdk/jsonrpc" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -90,22 +89,10 @@ func main() { exit(exitCodeSetupFailure) } - go func() { - ticker := time.NewTicker(30 * time.Second) - defer ticker.Stop() - for { - select { - case <-ticker.C: - pingCtx, cancel := context.WithTimeout(ctx, 10*time.Second) - if err := ss.Ping(pingCtx, nil); err != nil { - mcpError(resources.Logger(), fmt.Errorf("failed to ping: %s", err), jsonRPCErrorCodeInternalError) - } - cancel() - case <-ctx.Done(): - return - } - } - }() + // Keepalive pings are driven by the SDK itself, via + // mcp.ServerOptions.KeepAlive in config.NewMCPServer. Do not add a manual + // ping loop here: on failure it has no request to reply to, so mcpError + // writes an id-less JSON-RPC error onto stdout, which is the protocol stream. if err := ss.Wait(); err != nil { mcpError(resources.Logger(), fmt.Errorf("failed to serve: %s", err), jsonRPCErrorCodeInternalError) diff --git a/go.mod b/go.mod index 1cc5f01b..dce5a5a7 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,7 @@ require ( github.com/google/jsonschema-go v0.4.3 github.com/google/uuid v1.6.0 github.com/localit-io/tiktoken-go v0.2.1 - github.com/modelcontextprotocol/go-sdk v1.6.1 + github.com/modelcontextprotocol/go-sdk v1.7.0 github.com/teamwork/desksdkgo v1.0.1 github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb github.com/teamwork/twapi-go-sdk v1.20.4 @@ -87,6 +87,7 @@ require ( golang.org/x/exp v0.0.0-20260312153236-7ab1446f8b90 // indirect golang.org/x/mod v0.35.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect + golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.37.0 // indirect golang.org/x/time v0.15.0 // indirect diff --git a/go.sum b/go.sum index b39afeba..708cb965 100644 --- a/go.sum +++ b/go.sum @@ -117,8 +117,8 @@ github.com/lufia/plan9stats v0.0.0-20260330125221-c963978e514e h1:Q6MvJtQK/iRcRt github.com/lufia/plan9stats v0.0.0-20260330125221-c963978e514e/go.mod h1:autxFIvghDt3jPTLoqZ9OZ7s9qTGNAWmYCjVFWPX/zg= github.com/minio/simdjson-go v0.4.5 h1:r4IQwjRGmWCQ2VeMc7fGiilu1z5du0gJ/I/FsKwgo5A= github.com/minio/simdjson-go v0.4.5/go.mod h1:eoNz0DcLQRyEDeaPr4Ru6JpjlZPzbA0IodxVJk8lO8E= -github.com/modelcontextprotocol/go-sdk v1.6.1 h1:0zOSupjKUxPKSocPT1Wtago+mUHU2/uZ4xSOY0FGReU= -github.com/modelcontextprotocol/go-sdk v1.6.1/go.mod h1:kzm3kzFL1/+AziGOE0nUs3gvPoNxMCvkxokMkuFapXQ= +github.com/modelcontextprotocol/go-sdk v1.7.0 h1:yqjY2dsbKAC0LSuWZVBMrHgiG8ukXv6NRo0JiALay44= +github.com/modelcontextprotocol/go-sdk v1.7.0/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -286,6 +286,8 @@ golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= +golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/auth/bypass.go b/internal/auth/bypass.go index 67d2e24a..6c2f216f 100644 --- a/internal/auth/bypass.go +++ b/internal/auth/bypass.go @@ -22,6 +22,16 @@ var methodsWhitelist = []string{ "resources/list", "resources/templates/list", "prompts/list", + + // "server/discover" (SEP-2575) is the stateless replacement for the + // "initialize" handshake: clients probe capabilities and supported protocol + // versions with it before they hold a token. It has to bypass authentication + // for the same reason "initialize" does, otherwise the pre-auth connector + // setup flow answers 401. The legacy entries above are kept so clients on + // older spec revisions keep working unchanged. + // + // https://modelcontextprotocol.io/seps/2575-stateless-mcp + "server/discover", } // Bypass checks if the protocol method can bypass authentication. diff --git a/internal/auth/bypass_test.go b/internal/auth/bypass_test.go new file mode 100644 index 00000000..d6c61b5a --- /dev/null +++ b/internal/auth/bypass_test.go @@ -0,0 +1,79 @@ +package auth + +import ( + "testing" +) + +func TestBypassMethod(t *testing.T) { + tests := []struct { + method string + want bool + }{ + // SEP-2575 stateless capability discovery, the replacement for the + // "initialize" handshake. Clients probe it before holding a token. + {method: "server/discover", want: true}, + + // legacy spec revisions must keep working unchanged + {method: "initialize", want: true}, + {method: "notifications/initialized", want: true}, + {method: "logging/setLevel", want: true}, + {method: "tools/list", want: true}, + {method: "resources/list", want: true}, + {method: "resources/templates/list", want: true}, + {method: "prompts/list", want: true}, + + // everything else must stay authenticated + {method: "tools/call", want: false}, + {method: "resources/read", want: false}, + {method: "prompts/get", want: false}, + {method: "server/discovery", want: false}, + {method: "", want: false}, + } + + for _, tt := range tests { + t.Run(tt.method, func(t *testing.T) { + if got := BypassMethod(tt.method); got != tt.want { + t.Errorf("BypassMethod(%q) = %t, want %t", tt.method, got, tt.want) + } + }) + } +} + +func TestBypass(t *testing.T) { + tests := []struct { + name string + data string + want bool + wantErr bool + }{{ + name: "server/discover request", + data: `{"jsonrpc":"2.0","id":1,"method":"server/discover"}`, + want: true, + }, { + name: "initialize request", + data: `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18"}}`, + want: true, + }, { + name: "tools/call request", + data: `{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"twprojects-get_task"}}`, + want: false, + wantErr: true, + }, { + name: "malformed payload", + data: `{not json`, + want: false, + wantErr: true, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Bypass([]byte(tt.data)) + if (err != nil) != tt.wantErr { + t.Errorf("Bypass() error = %v, wantErr %t", err, tt.wantErr) + } + if got != tt.want { + t.Errorf("Bypass() = %t, want %t", got, tt.want) + } + }) + } +} diff --git a/internal/config/cache_test.go b/internal/config/cache_test.go new file mode 100644 index 00000000..404f4558 --- /dev/null +++ b/internal/config/cache_test.go @@ -0,0 +1,217 @@ +package config + +import ( + "context" + "io" + "log/slog" + "testing" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/teamwork/mcp/internal/toolsets" +) + +// TestListToolsCacheScopeIsPrivate guards a cross-tenant leak. The tool list is +// filtered per OAuth token scope, but the SDK marks cacheable results "public" +// by default (SEP-2549), which would let a shared intermediary serve one +// tenant's tool list to another. +func TestListToolsCacheScopeIsPrivate(t *testing.T) { + tests := []struct { + name string + scopes []string + // wantTools is the set of tool names the caller should be able to see. + wantTools []string + }{{ + name: "no scopes, unfiltered list", + scopes: nil, + wantTools: []string{"twdesk-read", "twprojects-read"}, + }, { + name: "projects scope only", + scopes: []string{"projects"}, + wantTools: []string{"twprojects-read"}, + }, { + name: "scope matching no tool", + scopes: []string{"spaces"}, + wantTools: nil, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + if tt.scopes != nil { + ctx = WithScopes(ctx, tt.scopes) + } + + result := listTools(ctx, t) + + if got := result.CacheScope; got != cacheScopePrivate { + t.Errorf("tools/list CacheScope = %q, want %q", got, cacheScopePrivate) + } + + got := make([]string, 0, len(result.Tools)) + for _, tool := range result.Tools { + got = append(got, tool.Name) + } + if len(got) != len(tt.wantTools) { + t.Fatalf("tools = %v, want %v", got, tt.wantTools) + } + for i, want := range tt.wantTools { + if got[i] != want { + t.Errorf("tools = %v, want %v", got, tt.wantTools) + break + } + } + }) + } +} + +// TestCapabilitiesOmitLogging pins the decision to stop advertising the +// deprecated "logging" capability (SEP-2577), which this server never honoured. +// The "logging/setLevel" method itself keeps working, so old clients are +// unaffected. +func TestCapabilitiesOmitLogging(t *testing.T) { + ctx := context.Background() + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := newTestMCPServer(t).Connect(ctx, serverTransport, nil) + if err != nil { + t.Fatalf("failed to connect server: %v", err) + } + defer serverSession.Close() //nolint:errcheck + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "1.0.0"}, nil) + clientSession, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("failed to connect client: %v", err) + } + defer clientSession.Close() //nolint:errcheck + + capabilities := clientSession.InitializeResult().Capabilities + if capabilities.Logging != nil { //nolint:staticcheck // asserting the deprecated capability is absent + t.Error("server advertises the deprecated logging capability, want it omitted") + } + if capabilities.Tools == nil { + t.Error("server does not advertise the tools capability, want it advertised") + } + if _, ok := capabilities.Extensions["io.modelcontextprotocol/ui"]; !ok { + t.Error("server does not advertise the MCP Apps UI extension, want it advertised") + } +} + +// TestCapabilitiesOmitListChanged pins the list-changed capabilities off. This +// server never pushes list-change notifications, and advertising them invites +// clients to open a long-lived "subscriptions/listen" stream (SEP-2575) that a +// stateless, load-balanced deployment would hold open forever with nothing to +// send. The SDK turns these on by default whenever the matching capability +// struct is left nil, so this test guards against that default leaking in. +func TestCapabilitiesOmitListChanged(t *testing.T) { + ctx := context.Background() + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := newTestMCPServer(t).Connect(ctx, serverTransport, nil) + if err != nil { + t.Fatalf("failed to connect server: %v", err) + } + defer serverSession.Close() //nolint:errcheck + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "1.0.0"}, nil) + clientSession, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("failed to connect client: %v", err) + } + defer clientSession.Close() //nolint:errcheck + + capabilities := clientSession.InitializeResult().Capabilities + if capabilities.Tools == nil || capabilities.Tools.ListChanged { + t.Error("tools.listChanged is advertised, want it off") + } + if capabilities.Prompts == nil || capabilities.Prompts.ListChanged { + t.Error("prompts.listChanged is advertised, want it off") + } + if capabilities.Resources == nil || capabilities.Resources.ListChanged { + t.Error("resources.listChanged is advertised, want it off") + } + if capabilities.Resources != nil && capabilities.Resources.Subscribe { + t.Error("resources.subscribe is advertised, want it off") + } +} + +// listTools runs a tools/list round trip against a server built by +// NewMCPServer, so the receiving middleware under test is exercised end to end. +func listTools(ctx context.Context, t *testing.T) *mcp.ListToolsResult { + t.Helper() + + serverTransport, clientTransport := mcp.NewInMemoryTransports() + + serverSession, err := newTestMCPServer(t).Connect(ctx, serverTransport, nil) + if err != nil { + t.Fatalf("failed to connect server: %v", err) + } + defer serverSession.Close() //nolint:errcheck + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "1.0.0"}, nil) + clientSession, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("failed to connect client: %v", err) + } + defer clientSession.Close() //nolint:errcheck + + result, err := clientSession.ListTools(ctx, nil) + if err != nil { + t.Fatalf("failed to list tools: %v", err) + } + return result +} + +// newTestMCPServer builds a server through NewMCPServer with one read tool per +// product prefix, so scope filtering has something to filter, plus a prompt and +// a resource so the prompt and resource capabilities are populated the way the +// real toolset groups populate them. +func newTestMCPServer(t *testing.T) *mcp.Server { + t.Helper() + + toolsets.RegisterToolOrder(nil) + + toolset := toolsets.NewToolset("test", "toolset used by the config tests") + toolset.AddReadTools( + newTestReadTool("twprojects-read"), + newTestReadTool("twdesk-read"), + ) + toolset.AddPrompts(toolsets.NewServerPrompt( + &mcp.Prompt{Name: "twprojects-test_prompt"}, + func(context.Context, *mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + return &mcp.GetPromptResult{}, nil + }, + )) + toolset.AddResources(toolsets.NewServerResource( + &mcp.Resource{Name: "twprojects-test_resource", URI: "ui://teamwork/test"}, + func(context.Context, *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { + return &mcp.ReadResourceResult{}, nil + }, + )) + + group := toolsets.NewToolsetGroup(false) + group.AddToolset(toolset) + if err := group.EnableToolsets(toolsets.MethodAll); err != nil { + t.Fatalf("failed to enable toolsets: %v", err) + } + + var resources Resources + resources.logger = slog.New(slog.NewTextHandler(io.Discard, nil)) + + return NewMCPServer(resources, group) +} + +func newTestReadTool(name string) toolsets.ToolWrapper { + return toolsets.ToolWrapper{ + Tool: &mcp.Tool{ + Name: name, + Description: "tool used by the config tests", + Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, + InputSchema: &jsonschema.Schema{Type: "object"}, + }, + Handler: func(context.Context, *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{}, nil + }, + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 7853ba0d..4ed70018 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -31,6 +31,11 @@ import ( const ( mcpName = "Teamwork.com" sentryFlushTimeout = 2 * time.Second + + // cacheScopePrivate marks a cacheable result as only cacheable by the + // requesting user's client, never by a shared intermediary. See SEP-2549 and + // the mcp.Cacheable type, whose CacheScope field this fills in. + cacheScopePrivate = "private" ) // Load loads the configuration for the MCP service. @@ -186,8 +191,12 @@ func NewMCPServer(resources Resources, groups ...*toolsets.ToolsetGroup) *mcp.Se Logger: resources.logger, HasTools: hasTools, HasPrompts: hasPrompts, + // The "logging" capability is deliberately not advertised. It was + // deprecated by SEP-2577, and this server never sent a + // "notifications/message" anyway, so claiming it only told clients to call + // "logging/setLevel" for no effect. Old clients are unaffected: the SDK + // still answers "logging/setLevel", and auth.Bypass still whitelists it. Capabilities: &mcp.ServerCapabilities{ - Logging: &mcp.LoggingCapabilities{}, Extensions: map[string]any{ // https://github.com/modelcontextprotocol/ext-apps/blob/main/specification/2026-01-26/apps.mdx#extension-identifier "io.modelcontextprotocol/ui": map[string]any{}, @@ -240,10 +249,20 @@ func NewMCPServer(resources Resources, groups ...*toolsets.ToolsetGroup) *mcp.Se } listToolsResult, ok := result.(*mcp.ListToolsResult) - if !ok || listToolsResult == nil || len(listToolsResult.Tools) == 0 { + if !ok || listToolsResult == nil { return result, nil } + // The tool list is filtered per OAuth token scopes below, so it is + // specific to the requesting user. The SDK defaults cacheable results + // to "public" (SEP-2549), which would allow a shared intermediary to + // serve one tenant's tool list to another. Downgrade it to "private". + listToolsResult.CacheScope = cacheScopePrivate + + if len(listToolsResult.Tools) == 0 { + return listToolsResult, nil + } + // filter tools based on scopes if scopes := scopes(ctx); len(scopes) > 0 { projectsScope := slices.Contains(scopes, "projects") diff --git a/internal/request/response.go b/internal/request/response.go index 8b203b54..c2a2555b 100644 --- a/internal/request/response.go +++ b/internal/request/response.go @@ -5,15 +5,37 @@ import ( "net/http" ) +// maxCapturedBodySize caps how much of the response body is retained for +// logging. Streamable HTTP responses are Server-Sent Events streams that can +// stay open for the whole lifetime of a request, so capturing them in full +// would grow without bound. +const maxCapturedBodySize = 64 << 10 // 64 KiB + +// truncatedBodySuffix is appended to the captured body when it was truncated. +const truncatedBodySuffix = "...[truncated]" + // ResponseWriter is a custom http.ResponseWriter that captures the status code // and response body. +// +// It deliberately implements http.Flusher and Unwrap so that +// http.ResponseController can reach the flusher of the wrapped writer. The MCP +// SDK writes Server-Sent Events through http.ResponseController: without these +// methods flushing silently becomes a no-op and clients waiting on the stream +// receive nothing until the handler returns. type ResponseWriter struct { http.ResponseWriter statusCode int body bytes.Buffer + truncated bool } +// Ensure the wrapper keeps satisfying the interfaces the MCP SDK relies on. +var ( + _ http.ResponseWriter = (*ResponseWriter)(nil) + _ http.Flusher = (*ResponseWriter)(nil) +) + // NewResponseWriter creates a new ResponseWriter. func NewResponseWriter(w http.ResponseWriter) *ResponseWriter { return &ResponseWriter{ @@ -28,18 +50,47 @@ func (w *ResponseWriter) WriteHeader(code int) { w.ResponseWriter.WriteHeader(code) } -// Write captures the response body. +// Write captures the response body, up to maxCapturedBodySize, and forwards it +// to the wrapped writer. func (w *ResponseWriter) Write(b []byte) (int, error) { - _, _ = w.body.Write(b) // if error occurs, we ignore it + if remaining := maxCapturedBodySize - w.body.Len(); remaining > 0 { + if len(b) <= remaining { + _, _ = w.body.Write(b) // if error occurs, we ignore it + } else { + _, _ = w.body.Write(b[:remaining]) // if error occurs, we ignore it + w.truncated = true + } + } else if len(b) > 0 { + w.truncated = true + } return w.ResponseWriter.Write(b) } +// Flush forwards the flush to the wrapped writer, when it supports it. This +// keeps Server-Sent Events streams responsive while still being logged. +func (w *ResponseWriter) Flush() { + // http.NewResponseController resolves the flusher through Unwrap, so it + // handles arbitrarily deep wrapper chains. + _ = http.NewResponseController(w.ResponseWriter).Flush() +} + +// Unwrap returns the wrapped http.ResponseWriter, allowing +// http.ResponseController to reach optional interfaces (flushing, hijacking, +// deadlines) implemented further down the chain. +func (w *ResponseWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +} + // StatusCode returns the captured status code. func (w *ResponseWriter) StatusCode() int { return w.statusCode } -// Body returns the captured response body. +// Body returns the captured response body. Bodies larger than +// maxCapturedBodySize are truncated and marked as such. func (w *ResponseWriter) Body() []byte { - return w.body.Bytes() + if !w.truncated { + return w.body.Bytes() + } + return append(bytes.Clone(w.body.Bytes()), truncatedBodySuffix...) } diff --git a/internal/request/response_test.go b/internal/request/response_test.go new file mode 100644 index 00000000..316c51e7 --- /dev/null +++ b/internal/request/response_test.go @@ -0,0 +1,107 @@ +package request + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// flushRecorder is an http.ResponseWriter that records how often it was +// flushed, standing in for the real connection. +type flushRecorder struct { + http.ResponseWriter + flushes int +} + +func (f *flushRecorder) Flush() { f.flushes++ } + +// TestResponseWriterFlushPropagates guards the reason Flush and Unwrap exist: +// the MCP SDK writes Server-Sent Events through http.ResponseController, which +// resolves the flusher by unwrapping. Without it, flushing silently degrades to +// a no-op and streaming clients hang until the handler returns. +func TestResponseWriterFlushPropagates(t *testing.T) { + inner := &flushRecorder{ResponseWriter: httptest.NewRecorder()} + rw := NewResponseWriter(inner) + + // Direct http.Flusher assertion, as used by older middleware. + flusher, ok := any(rw).(http.Flusher) + if !ok { + t.Fatal("ResponseWriter does not implement http.Flusher") + } + flusher.Flush() + if inner.flushes != 1 { + t.Errorf("after Flush(): inner flushes = %d, want 1", inner.flushes) + } + + // http.ResponseController resolution, as used by the MCP SDK. + if err := http.NewResponseController(rw).Flush(); err != nil { + t.Errorf("http.NewResponseController(rw).Flush() = %v, want nil", err) + } + if inner.flushes != 2 { + t.Errorf("after ResponseController flush: inner flushes = %d, want 2", inner.flushes) + } +} + +func TestResponseWriterUnwrap(t *testing.T) { + inner := httptest.NewRecorder() + rw := NewResponseWriter(inner) + + if got := rw.Unwrap(); got != http.ResponseWriter(inner) { + t.Errorf("Unwrap() = %v, want the wrapped writer", got) + } +} + +func TestResponseWriterCapturesStatusAndBody(t *testing.T) { + recorder := httptest.NewRecorder() + rw := NewResponseWriter(recorder) + + rw.WriteHeader(http.StatusAccepted) + if _, err := rw.Write([]byte("hello ")); err != nil { + t.Fatalf("Write() error = %v", err) + } + if _, err := rw.Write([]byte("world")); err != nil { + t.Fatalf("Write() error = %v", err) + } + + if got := rw.StatusCode(); got != http.StatusAccepted { + t.Errorf("StatusCode() = %d, want %d", got, http.StatusAccepted) + } + if got := string(rw.Body()); got != "hello world" { + t.Errorf("Body() = %q, want %q", got, "hello world") + } + // The body must still reach the client untouched. + if got := recorder.Body.String(); got != "hello world" { + t.Errorf("forwarded body = %q, want %q", got, "hello world") + } +} + +// TestResponseWriterTruncatesLargeBody guards against unbounded memory growth +// on long-lived Server-Sent Events streams, which stay open for the lifetime of +// the request. +func TestResponseWriterTruncatesLargeBody(t *testing.T) { + recorder := httptest.NewRecorder() + rw := NewResponseWriter(recorder) + + // Write in chunks, crossing the cap mid-chunk and then well past it. + chunk := bytes.Repeat([]byte("a"), maxCapturedBodySize/2) + for range 5 { + if _, err := rw.Write(chunk); err != nil { + t.Fatalf("Write() error = %v", err) + } + } + + body := rw.Body() + if !strings.HasSuffix(string(body), truncatedBodySuffix) { + t.Errorf("Body() does not end with %q", truncatedBodySuffix) + } + if want := maxCapturedBodySize + len(truncatedBodySuffix); len(body) != want { + t.Errorf("len(Body()) = %d, want %d", len(body), want) + } + + // Truncation must only affect what is logged, never what is sent. + if got, want := recorder.Body.Len(), len(chunk)*5; got != want { + t.Errorf("forwarded body length = %d, want %d", got, want) + } +} From c27642acea48e8a6c1cb761d43720e67bda3f424 Mon Sep 17 00:00:00 2001 From: Rafael Dantas Justo Date: Wed, 29 Jul 2026 09:20:44 -0300 Subject: [PATCH 2/2] Fix comments/docs https://github.com/Teamwork/mcp/pull/411#issuecomment-5117395920 --- cmd/mcp-http/README.md | 6 ++++-- internal/config/cache_test.go | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/mcp-http/README.md b/cmd/mcp-http/README.md index 42b9f9bc..bb2b9aa5 100644 --- a/cmd/mcp-http/README.md +++ b/cmd/mcp-http/README.md @@ -158,8 +158,10 @@ is documented at Two further notes for clients: - The `logging` capability is no longer advertised. It was deprecated by - SEP-2577 and this server never sent a `notifications/message`, so nothing that - worked before stops working โ€” `logging/setLevel` is still answered. + SEP-2577 and this server never sent a `notifications/message`, so no client + loses functionality. Clients on pre-`2026-07-28` revisions can still call + `logging/setLevel`; on `2026-07-28` the SDK rejects it with `-32601`, as the + revision removed the method. - `tools/list` responses carry `cacheScope: "private"`. The tool list is filtered per OAuth token scope, so shared intermediaries must not cache it. diff --git a/internal/config/cache_test.go b/internal/config/cache_test.go index 404f4558..0f5d53b5 100644 --- a/internal/config/cache_test.go +++ b/internal/config/cache_test.go @@ -67,8 +67,9 @@ func TestListToolsCacheScopeIsPrivate(t *testing.T) { // TestCapabilitiesOmitLogging pins the decision to stop advertising the // deprecated "logging" capability (SEP-2577), which this server never honoured. -// The "logging/setLevel" method itself keeps working, so old clients are -// unaffected. +// Old clients are unaffected: on pre-2026-07-28 revisions the SDK still answers +// "logging/setLevel". On 2026-07-28 it rejects the method, but only because the +// revision removed it, not because of this change. func TestCapabilitiesOmitLogging(t *testing.T) { ctx := context.Background()