diff --git a/mcpcompat/server/server.go b/mcpcompat/server/server.go index 425aacb..f72dcc2 100644 --- a/mcpcompat/server/server.go +++ b/mcpcompat/server/server.go @@ -766,20 +766,14 @@ func (s *MCPServer) sessionDispatchMiddleware(srv *gosdk.Server, stateless bool) // fireBeforeHooks for the extraction and fallback behavior. s.fireBeforeHooks(ctx, method, req) res, err := next(ctx, method, req) - // TODO(cacheScope): go-sdk's setDefaultCacheableValues stamps - // cacheScope:"public" (Cacheable.CacheScope, an IN-BODY MCP result - // field, NOT an HTTP header) on 2026-07-28 list/discover result - // bodies, but leaves ttlMs=0. Per the spec a zero TTL means - // "immediately stale", so a bare "public" scope with ttlMs==0 is - // inert: nothing may actually cache it yet. The cross-identity - // cache-share hazard for a per-identity-projected result only - // materializes once something sets a positive ttlMs. When that - // happens, the fix is an IN-BODY rewrite of cacheScope to - // "private" on per-identity-projected results — either here - // (this middleware already holds res) or in the vMCP envelope — - // NOT an HTTP Cache-Control header, which cannot neutralize a - // cacheScope an MCP-aware cache reads from the body. Deferred: - // no consumer sets a positive ttlMs yet. Tracked as follow-up. + // Narrow go-sdk's default in-body cacheScope:"public" hint to + // "private" on results this shim serves per-identity — which is + // every serving mode, stateful included. See stripPublicCacheScope + // for the full rationale and why this is a body rewrite rather than + // an HTTP Cache-Control header. + if err == nil { + s.stripPublicCacheScope(res) + } if err != nil && method == string(mcp.MethodToolsCall) { err = translateUnknownToolError(err, req) } @@ -799,6 +793,86 @@ func (s *MCPServer) sessionDispatchMiddleware(srv *gosdk.Server, stateless bool) } } +// The two members of the MCP cacheScope union. go-sdk models CacheScope as a +// bare string and exports no constants for it, so name them here. +const ( + cacheScopePublic = "public" + cacheScopePrivate = "private" +) + +// stripPublicCacheScope rewrites an in-body "public" Cacheable.CacheScope hint +// to "private" on the go-sdk results that carry one. +// +// go-sdk stamps a "public" CacheScope (setDefaultCacheableValues, +// mcp/protocol.go) on every list/discover result AND on resources/read results, +// regardless of whether the body was produced per-identity — which, in this +// shim, it is: +// +// - Every dispatch runs the handler under the caller's bridged identity +// (the valueBridgeContext in sessionDispatchMiddleware is not gated on +// serving mode), so resources/read can return per-identity content. +// - Stateless serving projects per-identity tools via the before-list hooks +// (fireBeforeHooks) over a per-request ephemeral session +// (bindSessionForDispatch). +// - Stateful serving projects MORE, not less: registerAndSync installs the +// SessionWithTools/Resources/ResourceTemplates/Prompts overlays onto that +// session's own go-sdk server (syncSession*), and that path runs only when +// !stateless. ToolHive's vMCP layer is the consumer. +// +// A "public" hint on any of those bodies is a cross-identity cache-leak signal: +// per the 2026-07-28 caching spec it tells any client, shared gateway or +// caching proxy that it MAY serve the response to a different user, and it does +// so regardless of the endpoint being authenticated. So the rewrite applies to +// every serving mode. It must be an in-body rewrite, not an HTTP Cache-Control +// header, which cannot neutralize a scope hint an MCP-aware cache reads out of +// the body. It only ever narrows the SDK's default; it never widens +// cache-sharing. +// +// gosdk.GetPromptResult, CompleteResult and CallToolResult do not embed +// Cacheable and are never stamped, so they need no handling here. A res of an +// unhandled type, or one already scoped "private", is a no-op — as is an +// untyped-nil gosdk.Result, which matches no case arm. A typed-nil pointer of a +// covered type would panic on the field read, but go-sdk cannot hand one back on +// a success path: its own pointer-receiver setDefaultCacheableValues would have +// panicked on it first, and wrapResourceHandler always allocates a non-nil +// result. +func (s *MCPServer) stripPublicCacheScope(res gosdk.Result) { + var c *gosdk.Cacheable + switch r := res.(type) { + case *gosdk.ListToolsResult: + c = &r.Cacheable + case *gosdk.ListResourcesResult: + c = &r.Cacheable + case *gosdk.ListResourceTemplatesResult: + c = &r.Cacheable + case *gosdk.ListPromptsResult: + c = &r.Cacheable + case *gosdk.DiscoverResult: + c = &r.Cacheable + case *gosdk.ReadResourceResult: + c = &r.Cacheable + default: + // Anything that embeds Cacheable satisfies gosdk.CacheableResult for + // free (GetCacheScope/GetTTLMs are promoted from the embedded struct), + // so a result landing here while reporting a "public" scope is one a + // newer go-sdk started stamping and the switch above does not cover — + // the one way a go-sdk bump could silently reintroduce the leak this + // function exists to prevent. Warn rather than panic: a missed cache + // hint must not take down a request on a goroutine go-sdk never + // recovers. Log the type only, never the body. + if cr, ok := res.(gosdk.CacheableResult); ok && cr.GetCacheScope() == cacheScopePublic { + if s.logger != nil { + s.logger.Warn("unhandled cacheable MCP result type: leaving cacheScope public", + "type", fmt.Sprintf("%T", res)) + } + } + return + } + if c.CacheScope == cacheScopePublic { + c.CacheScope = cacheScopePrivate + } +} + // bindSessionForDispatch binds the ClientSession for this dispatch into ctx, // selecting the binding strategy for the session's shape: // - no go-sdk session at all (ss==nil): nothing to bind. diff --git a/mcpcompat/server/server_internal_test.go b/mcpcompat/server/server_internal_test.go index 558dd06..778cd8b 100644 --- a/mcpcompat/server/server_internal_test.go +++ b/mcpcompat/server/server_internal_test.go @@ -4,8 +4,11 @@ package server import ( + "bytes" "context" "encoding/json" + "log/slog" + "strings" "testing" gosdk "github.com/modelcontextprotocol/go-sdk/mcp" @@ -443,3 +446,148 @@ func TestBindSessionForDispatch_StatelessForcesEphemeralRegardlessOfSessionID(t assert.NotSame(t, csA, csB, "each stateless dispatch must get its own ephemeral session, even under a shared client-supplied session id") } + +// TestStripPublicCacheScope covers the mapping directly, so all six go-sdk +// result types that embed Cacheable are exercised without standing up an HTTP +// server per type (the blackbox tests in stateless_test.go cover three of them +// end-to-end and cannot reach this unexported helper at all). The go-sdk +// version is pinned to a PRE-RELEASE, so the completeness of this switch is +// exactly what a version bump can silently break. +func TestStripPublicCacheScope(t *testing.T) { + t.Parallel() + + // Every gosdk result type that embeds Cacheable, i.e. every type + // setDefaultCacheableValues is called on. Keep in sync with the switch. + covered := map[string]gosdk.Result{ + "ListToolsResult": &gosdk.ListToolsResult{}, + "ListResourcesResult": &gosdk.ListResourcesResult{}, + "ListResourceTemplatesResult": &gosdk.ListResourceTemplatesResult{}, + "ListPromptsResult": &gosdk.ListPromptsResult{}, + "DiscoverResult": &gosdk.DiscoverResult{}, + "ReadResourceResult": &gosdk.ReadResourceResult{}, + } + + s := NewMCPServer("strip-cachescope", "1.0.0") + + for name, res := range covered { + t.Run(name+"/public is narrowed", func(t *testing.T) { + t.Parallel() + cr, ok := res.(gosdk.CacheableResult) + require.True(t, ok, "%s must satisfy CacheableResult via its embedded Cacheable", name) + // Stamp it the way go-sdk's setDefaultCacheableValues would. + setCacheScopeForTest(t, res, cacheScopePublic) + s.stripPublicCacheScope(res) + assert.Equal(t, cacheScopePrivate, cr.GetCacheScope(), + "%s is served per-identity and must be narrowed to private", name) + }) + } + + t.Run("already private is a no-op", func(t *testing.T) { + t.Parallel() + res := &gosdk.ListToolsResult{Cacheable: gosdk.Cacheable{CacheScope: cacheScopePrivate, TTLMs: 5}} + s.stripPublicCacheScope(res) + assert.Equal(t, cacheScopePrivate, res.CacheScope) + assert.Equal(t, 5, res.TTLMs, "ttlMs must never be touched") + }) + + t.Run("unset scope is left alone", func(t *testing.T) { + t.Parallel() + // Only the literal "public" is narrowed: an unset scope is not a + // cache-share signal, and inventing one is not this function's job. + res := &gosdk.ListToolsResult{} + s.stripPublicCacheScope(res) + assert.Empty(t, res.CacheScope) + }) + + t.Run("non-cacheable result is a no-op", func(t *testing.T) { + t.Parallel() + // CallToolResult, GetPromptResult and CompleteResult do not embed + // Cacheable, so they have no scope to narrow and must not trip the + // unhandled-type warning either. + for _, res := range []gosdk.Result{ + &gosdk.CallToolResult{}, &gosdk.GetPromptResult{}, &gosdk.CompleteResult{}, + } { + assert.NotImplements(t, (*gosdk.CacheableResult)(nil), res, + "%T must not embed Cacheable, or it belongs in the switch", res) + s.stripPublicCacheScope(res) + } + }) + + t.Run("untyped nil result is a no-op", func(t *testing.T) { + t.Parallel() + assert.NotPanics(t, func() { s.stripPublicCacheScope(nil) }) + }) + + t.Run("unhandled cacheable type is left public and warned about", func(t *testing.T) { + t.Parallel() + // The scenario the default arm exists for: a go-sdk version newer than + // the pinned pre-release embeds Cacheable into a result type the switch + // does not cover. Such a type satisfies gosdk.CacheableResult for free, + // so the arm must notice it, leave the body alone, and say so — this is + // the one backstop against a version bump silently reintroducing the + // cross-identity leak, since the covered map above is hand-maintained. + var buf bytes.Buffer + logged := NewMCPServer("strip-cachescope-warn", "1.0.0", + WithLogger(slog.New(slog.NewTextHandler(&buf, nil)))) + res := &fakeCacheableResult{Cacheable: gosdk.Cacheable{CacheScope: cacheScopePublic}} + require.Implements(t, (*gosdk.CacheableResult)(nil), res, + "the fake must satisfy CacheableResult, or it is not exercising the default arm") + + logged.stripPublicCacheScope(res) + + assert.Equal(t, cacheScopePublic, res.CacheScope, + "an unhandled type cannot be narrowed; the arm only reports it") + assert.Contains(t, buf.String(), "unhandled cacheable MCP result type", + "an unhandled cacheable result must be reported, not silently skipped") + assert.Contains(t, buf.String(), "fakeCacheableResult", + "the warning must name the offending type so the switch can be updated") + assert.NotContains(t, strings.ToLower(buf.String()), "cachescope=public", + "the warning must log the type only, never result body fields") + }) + + t.Run("unhandled cacheable type with no logger does not panic", func(t *testing.T) { + t.Parallel() + // WithLogger is optional, so the warn path must tolerate a nil logger. + unlogged := NewMCPServer("strip-cachescope-nolog", "1.0.0") + require.Nil(t, unlogged.logger, "this subtest is only meaningful without a logger") + assert.NotPanics(t, func() { + unlogged.stripPublicCacheScope(&fakeCacheableResult{ + Cacheable: gosdk.Cacheable{CacheScope: cacheScopePublic}, + }) + }) + }) +} + +// fakeCacheableResult stands in for a result type that a future go-sdk embeds +// Cacheable into without stripPublicCacheScope's switch being updated to match. +// gosdk.ResultBase supplies the unexported marker method that makes it a +// gosdk.Result; the embedded Cacheable promotes GetCacheScope/GetTTLMs, which is +// what makes it a gosdk.CacheableResult too — exactly as a real new SDK type +// would. +type fakeCacheableResult struct { + gosdk.ResultBase + gosdk.Cacheable +} + +// setCacheScopeForTest stamps a CacheScope onto a result the way go-sdk's +// unexported setDefaultCacheableValues does, without duplicating the switch +// under test. +func setCacheScopeForTest(t *testing.T, res gosdk.Result, scope string) { + t.Helper() + switch r := res.(type) { + case *gosdk.ListToolsResult: + r.CacheScope = scope + case *gosdk.ListResourcesResult: + r.CacheScope = scope + case *gosdk.ListResourceTemplatesResult: + r.CacheScope = scope + case *gosdk.ListPromptsResult: + r.CacheScope = scope + case *gosdk.DiscoverResult: + r.CacheScope = scope + case *gosdk.ReadResourceResult: + r.CacheScope = scope + default: + t.Fatalf("setCacheScopeForTest: unsupported result type %T", res) + } +} diff --git a/mcpcompat/server/stateless_test.go b/mcpcompat/server/stateless_test.go index 5a8836e..74655d6 100644 --- a/mcpcompat/server/stateless_test.go +++ b/mcpcompat/server/stateless_test.go @@ -84,6 +84,135 @@ func TestStateless_BasicRequestResponse(t *testing.T) { }) } +// TestStateless_ListAndDiscoverCacheScopePrivate verifies the fix for the +// cacheScope cross-identity hazard: go-sdk's setDefaultCacheableValues stamps +// every list/discover result's IN-BODY Cacheable.CacheScope with "public", +// with no awareness that, under stateless serving, that same result is +// produced by the shim's per-identity before-hook projection +// (sessionDispatchMiddleware). sessionDispatchMiddleware must rewrite that +// hint to "private" before the response is serialized, so an MCP-aware cache +// never treats one identity's projected list/discover result as shareable with +// another identity. See TestStateful_ListToolsCacheScopePrivate for the same +// guarantee on the stateful path. +func TestStateless_ListAndDiscoverCacheScopePrivate(t *testing.T) { + t.Parallel() + + const resourceURI = "file:///r" + mcpSrv := server.NewMCPServer("stateless-cachescope", "1.0.0") + addGreetTool(mcpSrv) + mcpSrv.AddResource( + mcp.Resource{URI: resourceURI, Name: "r", MIMEType: "text/plain"}, + func(context.Context, mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) { + return []mcp.ResourceContents{ + mcp.TextResourceContents{URI: resourceURI, MIMEType: "text/plain", Text: "hi"}, + }, nil + }, + ) + s := server.NewStreamableHTTPServer(mcpSrv, server.WithStateless(true)) + ts := httptest.NewServer(s) + t.Cleanup(ts.Close) + + t.Run("tools/list", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + resp := postRPC(ctx, t, ts.URL, "", `{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}`) + r := readFirstResult(t, resp) + require.Nil(t, r.Error, "tools/list should not error") + var res struct { + CacheScope string `json:"cacheScope"` + } + require.NoError(t, json.Unmarshal(r.Result, &res)) + assert.Equal(t, "private", res.CacheScope, + "a stateless tools/list result is per-identity projected and must never be marked cacheScope:public") + }) + + t.Run("server/discover", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // server/discover is only recognized when the request's _meta carries + // io.modelcontextprotocol/protocolVersion:"2026-07-28" (SEP-2575's + // per-request new-protocol signal; see go-sdk's ServerSession.handle), + // so this is sent via a raw request rather than postRPC: postRPC always + // sets Mcp-Protocol-Version: 2025-06-18, which go-sdk rejects as + // mismatched against a 2026-07-28 _meta on the very same request. + body := `{"jsonrpc":"2.0","id":2,"method":"server/discover","params":{"_meta":{` + + `"io.modelcontextprotocol/protocolVersion":"2026-07-28",` + + `"io.modelcontextprotocol/clientInfo":{"name":"test","version":"1.0"},` + + `"io.modelcontextprotocol/clientCapabilities":{}}}}` + req, err := http.NewRequestWithContext(ctx, http.MethodPost, ts.URL, strings.NewReader(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", bothAcceptMediaTypesForTest) + req.Header.Set("MCP-Protocol-Version", "2026-07-28") + // 2026-07-28's standard-headers requirement (SEP-2575) mandates an + // Mcp-Method header mirroring the body's method for any request once + // Mcp-Protocol-Version >= 2026-07-28. + req.Header.Set("Mcp-Method", "server/discover") + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + r := readFirstResult(t, resp) + require.Nil(t, r.Error, "server/discover should not error") + var res struct { + CacheScope string `json:"cacheScope"` + } + require.NoError(t, json.Unmarshal(r.Result, &res)) + assert.Equal(t, "private", res.CacheScope, + "a stateless server/discover result must never be marked cacheScope:public") + }) + + t.Run("resources/read", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // resources/read runs its handler with the caller's bridged identity and + // can return per-identity content, so its go-sdk-default cacheScope:"public" + // hint is the same cross-identity cache-leak risk as a projected list. + resp := postRPC(ctx, t, ts.URL, "", + `{"jsonrpc":"2.0","id":3,"method":"resources/read","params":{"uri":"file:///r"}}`) + r := readFirstResult(t, resp) + require.Nil(t, r.Error, "resources/read should not error") + var res struct { + CacheScope string `json:"cacheScope"` + } + require.NoError(t, json.Unmarshal(r.Result, &res)) + assert.Equal(t, "private", res.CacheScope, + "a stateless resources/read result is served per-identity and must never be marked cacheScope:public") + }) +} + +// TestStateful_ListToolsCacheScopePrivate pins that the rewrite is NOT +// stateless-only. A stateful server is per-identity projected at least as much +// as a stateless one: registerAndSync installs the SessionWithTools/Resources/ +// ResourceTemplates/Prompts overlays onto that session's own go-sdk server +// (syncSession*), and that path runs only when !stateless — stateless projects +// tools alone. The identity bridge in sessionDispatchMiddleware is likewise not +// gated on serving mode. So go-sdk's default cacheScope:"public" is just as +// wrong here, and must be narrowed to "private" on this path too. +func TestStateful_ListToolsCacheScopePrivate(t *testing.T) { + t.Parallel() + + mcpSrv := server.NewMCPServer("stateful-cachescope", "1.0.0") + addGreetTool(mcpSrv) + s := server.NewStreamableHTTPServer(mcpSrv, server.WithStateless(false)) + ts := httptest.NewServer(s) + t.Cleanup(ts.Close) + + sid := initSession(t, ts.URL) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + resp := postRPC(ctx, t, ts.URL, sid, `{"jsonrpc":"2.0","id":2,"method":"tools/list","params":{}}`) + r := readFirstResult(t, resp) + require.Nil(t, r.Error, "tools/list should not error") + var res struct { + CacheScope string `json:"cacheScope"` + } + require.NoError(t, json.Unmarshal(r.Result, &res)) + assert.Equal(t, "private", res.CacheScope, + "a stateful tools/list result carries this session's tool overlay and must never be marked cacheScope:public") +} + // TestStateless_GETAndDELETENotAllowed verifies that a stateless server has no // stream to open and no session to terminate, so go-sdk answers both GET and // DELETE with 405, carrying the RFC 9110-mandated Allow header.