From ff08c9cd236dbcf79f537bc6606c8d751390b1ec Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 15:03:32 +0200 Subject: [PATCH 1/2] Generalize reserved _meta strip to the namespace StripReservedModernMeta removed four named io.modelcontextprotocol/* keys, which was enough for its only job at the time: keeping a downstream Modern caller's per-request protocol metadata off a Legacy backend hop. The response-path counterpart needs the same guard against keys this list does not name -- serverInfo, subscriptionId, and whatever a future revision adds -- so match the reserved namespace instead of enumerating it, and drop the "Modern" from the name now that it guards both revisions in both directions. Matching a namespace over-reaches in one direction the old list could not: the reserved prefix holds two kinds of key. Per-hop control keys are a gateway's to terminate, but end-to-end semantic keys are merely carried, and stripping those breaks the feature that depends on them. passthroughMetaKeys exempts the two the 2025-11-25 task facility defines -- related-task is a MUST on task-related responses, the direction the response-path change filters. vMCP does not route tasks/* today, so these are inert; the point is that this strip is what would silently break them later. Delete ReservedModernMetaKeys. It has no production consumers once the predicate is a prefix, and its three test readers get stronger looping on the namespace: they now catch any reserved key, not four named ones. Co-Authored-By: Claude Opus 5 --- pkg/mcp/revision.go | 133 ++++++++++++------ pkg/mcp/revision_test.go | 65 +++++++-- pkg/vmcp/client/client.go | 9 +- pkg/vmcp/client/modern.go | 6 +- pkg/vmcp/client/revision_realbackend_test.go | 9 +- .../session/internal/backend/mcp_session.go | 5 +- .../mcp_session_meta_propagation_test.go | 9 +- 7 files changed, 168 insertions(+), 68 deletions(-) diff --git a/pkg/mcp/revision.go b/pkg/mcp/revision.go index 7be57f44ba..2a63f5bf8d 100644 --- a/pkg/mcp/revision.go +++ b/pkg/mcp/revision.go @@ -62,30 +62,54 @@ const metaKeyClientCapabilities = "io.modelcontextprotocol/clientCapabilities" // hop, but — unlike the other reserved keys — its mere presence is NOT a claim // of the Modern revision: go-sdk's validateRequestMeta gates Modern-ness purely // on protocolVersion, and SEP-2577 already deprecates logLevel. It therefore -// belongs in the egress strip set (ReservedModernMetaKeys) but not the ingress -// signal set (modernSignalMetaKeys). +// belongs in the strip set (ReservedMetaPrefix, minus passthroughMetaKeys) but +// not the ingress signal set (modernSignalMetaKeys). const metaKeyLogLevel = "io.modelcontextprotocol/logLevel" -// ReservedModernMetaKeys is the EGRESS/strip set: every reserved per-hop -// io.modelcontextprotocol/* control key that vMCP must remove before forwarding -// a caller-supplied _meta onto a Legacy (session-based, stateful) backend hop, -// where these keys are invalid — see StripReservedModernMeta. +// ReservedMetaPrefix is the _meta key namespace the MCP spec reserves for the +// protocol's own use. StripReservedMeta removes every key carrying it except +// the passthroughMetaKeys below. // -// Exported so a Modern client (the mirror of the classifier that reads these -// keys) can strip a caller's copies before overlaying its own authoritative -// values — see ModernRequestMeta / mergeModernMeta. +// The trailing slash is load-bearing. The registry's +// "io.modelcontextprotocol.registry/publisher-provided" (docs/registry/schema.md) +// is ALSO spec-reserved — per the 2025-11-25 _meta key-name rules any prefix +// whose second label is "modelcontextprotocol" or "mcp" is reserved — but it is +// server-descriptor provenance, not per-hop control, and never rides a +// tools/call result. The slash is what keeps this predicate off it. // -// This is deliberately NOT the Modern-detection set: that is -// modernSignalMetaKeys, a strict subset. "Strip this key on the way out" and -// "this key means the request is Modern" are separate decisions, because a -// reserved key can require the former without the latter (logLevel). Conflating -// them would make a request rejectable merely for carrying a -// strippable-but-not-signalling key. -var ReservedModernMetaKeys = []string{ - metaKeyProtocolVersion, - metaKeyClientInfo, - metaKeyClientCapabilities, - metaKeyLogLevel, +// Matching one literal prefix deliberately does not implement that full +// second-label rule: "dev.mcp/", "org.modelcontextprotocol.api/" and friends are +// equally reserved and sail through. Nothing in this repo emits them and no +// threat model calls for parsing the general rule. +const ReservedMetaPrefix = "io.modelcontextprotocol/" + +// passthroughMetaKeys are the reserved keys StripReservedMeta must NOT remove. +// +// The reserved namespace holds two different kinds of key, and only one of them +// is a gateway's to terminate: +// +// - per-hop protocol control (protocolVersion, clientInfo, clientCapabilities, +// logLevel, serverInfo, subscriptionId) — scoped to a single MCP connection. +// vMCP is its client's peer and its backends' peer on two different hops, so +// it must strip these and mint its own. +// - end-to-end semantic payload — meaningful to the ORIGINAL endpoints and +// merely carried by intermediaries. Stripping these breaks the feature. +// +// Both entries below are the second kind, from the 2025-11-25 task facility: +// "related-task" is a MUST on every task-related request AND response +// (tasks/result in particular carries the task id nowhere else), and +// "model-immediate-response" rides CreateTaskResult._meta. vMCP does not route +// tasks/* today, so these are inert — but this strip is what would silently +// break them later, and silent broken correlation is expensive to debug. +// +// Extend this by category, not by string: ask whether the key names something +// about THIS hop (strip) or about the two endpoints (pass through). +// A set, not a lookup table: struct{} values make membership the only question +// this map can answer. A map[string]bool would let a future "key": false entry +// read as "in the set" while silently stripping. +var passthroughMetaKeys = map[string]struct{}{ + "io.modelcontextprotocol/related-task": {}, + "io.modelcontextprotocol/model-immediate-response": {}, } // modernSignalMetaKeys is the INGRESS/detection set consumed by hasModernSignal: @@ -95,41 +119,66 @@ var ReservedModernMetaKeys = []string{ // protocolVersion alongside one of them turns into a rejection, never a // downgrade. // -// It is a strict subset of ReservedModernMetaKeys: logLevel is intentionally +// It is deliberately narrower than what StripReservedMeta removes: logLevel is // excluded so a request carrying only logLevel — which go-sdk's // validateRequestMeta accepts (gating purely on protocolVersion) and which // SEP-2577 deprecates — is classified Legacy rather than misdetected as Modern -// and then rejected. +// and then rejected. "Strip this key on the way out" and "this key means the +// request is Modern" are separate decisions; conflating them would make a +// request rejectable merely for carrying a strippable-but-not-signalling key. var modernSignalMetaKeys = []string{ metaKeyProtocolVersion, metaKeyClientInfo, metaKeyClientCapabilities, } -// StripReservedModernMeta returns a copy of meta with every ReservedModernMetaKeys -// entry removed, leaving all other caller-supplied keys (including trace-context -// keys) untouched. The input is never mutated (maps.Clone). +// StripReservedMeta returns a copy of meta with every ReservedMetaPrefix key +// removed except the passthroughMetaKeys, leaving all other caller-supplied keys +// (progressToken, trace context, backend-custom fields) untouched. The input is +// never mutated (maps.Clone). +// +// Use it on BOTH hops, in both directions, wherever a _meta map crosses vMCP: // -// Use this at every Legacy backend egress that forwards a caller-supplied _meta -// map: a downstream Modern request's reserved io.modelcontextprotocol/* _meta -// claims a per-request protocol version that is only valid on a stateless -// Modern hop. If it leaks onto a Legacy (session-based, stateful) backend call, -// go-sdk v1.7 rejects the request outright (HTTP 400: "protocol version ... -// is only supported on stateless HTTP servers") because ANY _meta.protocolVersion -// on a stateful streamable-HTTP server is invalid, regardless of its value. vMCP -// is the backend's actual MCP peer on this hop, not the downstream caller, so -// these reserved keys must never cross it. +// - Legacy backend egress (request): a downstream Modern caller's +// _meta.protocolVersion is only valid on a stateless Modern hop. go-sdk v1.7 +// rejects the request outright (HTTP 400: "protocol version ... is only +// supported on stateless HTTP servers") for ANY _meta.protocolVersion on a +// stateful streamable-HTTP server, regardless of its value. +// - Modern backend egress (request): vMCP overlays its own authoritative +// values afterwards — see ModernRequestMeta / mergeModernMeta. +// - client egress (response, and server->client requests): a backend must not +// speak for vMCP. Only serverInfo is even schema-legal on a result +// (ResultMetaObject); the request-only keys arriving on one are a backend +// fabricating the client's own identity. The spec says nothing about what a +// gateway should forward, so this is derived rather than quoted: serverInfo +// identifies "the server software producing the response", vMCP is that +// software, therefore vMCP's value is the correct one. // -// nil or empty input returns nil (matching mergeModernMeta's caller-tolerant -// convention); a non-empty map with none of the reserved keys present is -// returned as-is (via maps.Clone, so callers still get a copy, not the original). -func StripReservedModernMeta(meta map[string]any) map[string]any { +// TRIPWIRE: if vMCP ever relays resource subscriptions it must MINT its own +// io.modelcontextprotocol/subscriptionId from its own downstream listen request +// — never forward a backend's, which names a request id on the vMCP<->backend +// connection and collides across backends. That belongs in the notification +// relay (pkg/vmcp/client/forwarding.go), not here; this helper only sees +// requests and results. See pkg/transport/proxy/streamable/dispatcher_streams.go +// for the same concern in the streamable proxy. +// +// Returns nil whenever the result would be empty -- for nil or empty input, and +// also for a map whose keys were ALL reserved. Callers can therefore treat nil +// as "no _meta to send" without a second length check (mergeModernMeta's +// caller-tolerant convention, and what lets conversion.ToMCPMeta omit _meta +// entirely rather than emit an empty object). A map with keys left over is +// returned as a copy (maps.Clone), never the original. +func StripReservedMeta(meta map[string]any) map[string]any { if len(meta) == 0 { return nil } stripped := maps.Clone(meta) - for _, k := range ReservedModernMetaKeys { - delete(stripped, k) + maps.DeleteFunc(stripped, func(k string, _ any) bool { + _, passthrough := passthroughMetaKeys[k] + return strings.HasPrefix(k, ReservedMetaPrefix) && !passthrough + }) + if len(stripped) == 0 { + return nil } return stripped } @@ -515,8 +564,8 @@ func metaFromParamsMap(paramsMap map[string]any) map[string]any { // hasModernSignal reports whether the request signals the Modern revision: // either the header exactly names MCPVersionModern, or _meta carries any of the // modernSignalMetaKeys (regardless of whether their values are well-formed). -// It reads modernSignalMetaKeys, NOT ReservedModernMetaKeys — a key can be -// reserved-for-stripping without signalling Modern (logLevel). +// It reads modernSignalMetaKeys, NOT everything StripReservedMeta removes — a +// key can be reserved-for-stripping without signalling Modern (logLevel). func hasModernSignal(meta map[string]any, protoHeader string) bool { if protoHeader == MCPVersionModern { return true diff --git a/pkg/mcp/revision_test.go b/pkg/mcp/revision_test.go index 9ba1c665be..eafc1e10ac 100644 --- a/pkg/mcp/revision_test.go +++ b/pkg/mcp/revision_test.go @@ -6,6 +6,7 @@ package mcp import ( "encoding/base64" "encoding/json" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -215,7 +216,7 @@ func TestClassifyRevision(t *testing.T) { // logLevel is a reserved key that must be STRIPPED on egress but is // NOT a Modern signal (go-sdk's validateRequestMeta gates purely on // protocolVersion; SEP-2577 deprecates logLevel). Guards the split - // between ReservedModernMetaKeys and modernSignalMetaKeys: if + // between what StripReservedMeta removes and modernSignalMetaKeys: if // hasModernSignal ever iterated the strip set again, this request // would be misdetected Modern and rejected instead of classified // Legacy. @@ -673,46 +674,80 @@ func TestEncodeSentinelName(t *testing.T) { } } -// TestStripReservedModernMeta pins the copy-before-mutate contract: it removes -// exactly the reserved io.modelcontextprotocol/* keys, never mutates the -// caller's map, and returns nil for empty input. -func TestStripReservedModernMeta(t *testing.T) { +// TestStripReservedMeta pins the copy-before-mutate contract and the two halves +// of the strip predicate: every ReservedMetaPrefix key goes EXCEPT the +// passthroughMetaKeys, and nothing outside the prefix is touched. +func TestStripReservedMeta(t *testing.T) { t.Parallel() t.Run("nil input returns nil", func(t *testing.T) { t.Parallel() - assert.Nil(t, StripReservedModernMeta(nil)) + assert.Nil(t, StripReservedMeta(nil)) }) t.Run("empty input returns nil", func(t *testing.T) { t.Parallel() - assert.Nil(t, StripReservedModernMeta(map[string]any{})) + assert.Nil(t, StripReservedMeta(map[string]any{})) }) t.Run("removes reserved keys, preserves the rest", func(t *testing.T) { t.Parallel() in := map[string]any{ + // Request-side reserved keys. metaKeyProtocolVersion: MCPVersionModern, metaKeyClientInfo: map[string]any{"name": "x"}, metaKeyClientCapabilities: map[string]any{}, metaKeyLogLevel: "debug", - "progressToken": "tok-1", - "traceparent": "00-abc-def-01", - "custom": 42, + // Response/notification-side reserved keys: a backend must not be able + // to speak for vMCP on the way back to the client (#5986). + "io.modelcontextprotocol/serverInfo": map[string]any{"name": "attacker"}, + "io.modelcontextprotocol/subscriptionId": "sub-1", + // An unknown future reserved key must go too -- that is the whole point + // of matching a namespace rather than a fixed list. + "io.modelcontextprotocol/futureThing": "whatever", + // Not reserved: ordinary caller/backend metadata. + "progressToken": "tok-1", + "traceparent": "00-abc-def-01", + "custom": 42, + // Reserved-adjacent, must NOT match: the second label differs, so the + // trailing slash in ReservedMetaPrefix is what saves it. This is + // registry provenance metadata, not per-hop control. + "io.modelcontextprotocol.registry/publisher-provided": map[string]any{"x": 1}, } - got := StripReservedModernMeta(in) - for _, k := range ReservedModernMetaKeys { - assert.NotContains(t, got, k, "reserved key %q must be stripped", k) + got := StripReservedMeta(in) + + for k := range in { + if strings.HasPrefix(k, ReservedMetaPrefix) { + assert.NotContains(t, got, k, "reserved key %q must be stripped", k) + } } assert.Equal(t, "tok-1", got["progressToken"]) assert.Equal(t, "00-abc-def-01", got["traceparent"]) assert.Equal(t, 42, got["custom"]) + assert.Contains(t, got, "io.modelcontextprotocol.registry/publisher-provided", + "the registry namespace is not this predicate's to strip") + }) + + t.Run("passthrough keys survive despite the reserved prefix", func(t *testing.T) { + t.Parallel() + // related-task is a 2025-11-25 MUST on task-related requests AND responses + // (tasks/result carries the task id nowhere else), so the strip must not + // eat it even though it sits under the reserved prefix. + in := map[string]any{ + metaKeyProtocolVersion: MCPVersionModern, + "io.modelcontextprotocol/related-task": map[string]any{"taskId": "t-1"}, + "io.modelcontextprotocol/model-immediate-response": true, + } + got := StripReservedMeta(in) + assert.NotContains(t, got, metaKeyProtocolVersion) + assert.Equal(t, map[string]any{"taskId": "t-1"}, got["io.modelcontextprotocol/related-task"]) + assert.Equal(t, true, got["io.modelcontextprotocol/model-immediate-response"]) }) t.Run("does not mutate the caller's map", func(t *testing.T) { t.Parallel() in := map[string]any{metaKeyProtocolVersion: MCPVersionModern, "custom": 1} - _ = StripReservedModernMeta(in) + _ = StripReservedMeta(in) assert.Contains(t, in, metaKeyProtocolVersion, "caller's map must be untouched") assert.Len(t, in, 2) }) @@ -720,7 +755,7 @@ func TestStripReservedModernMeta(t *testing.T) { t.Run("no reserved keys returns a copy, not the original", func(t *testing.T) { t.Parallel() in := map[string]any{"custom": 1} - got := StripReservedModernMeta(in) + got := StripReservedMeta(in) require.Equal(t, in, got) got["custom"] = 2 assert.Equal(t, 1, in["custom"], "returned value must be a copy") diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 3cdfbb32b9..3be88a23f6 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -1706,8 +1706,13 @@ func (h *httpBackendClient) legacyCallTool( // Legacy (session-based, stateful) backend: a downstream Modern caller's // _meta.protocolVersion is only valid on a stateless Modern hop, and go-sdk // v1.7 hard-rejects ANY _meta.protocolVersion on a stateful server (HTTP 400) - // regardless of its value — see mcpparser.StripReservedModernMeta. - meta = mcpparser.StripReservedModernMeta(meta) + // regardless of its value — see mcpparser.StripReservedMeta. + // + // conversion.ToMCPMeta below now strips too, so this is belt-and-suspenders; + // it stays because the HTTP-400 hazard is specific to this hop and deserves a + // guard at the site that knows why. (modernCallTool, which builds its _meta + // directly and never reaches ToMCPMeta, needs the helper outright.) + meta = mcpparser.StripReservedMeta(meta) result, err := c.CallTool(ctx, mcp.CallToolRequest{ Params: mcp.CallToolParams{ Name: backendToolName, diff --git a/pkg/vmcp/client/modern.go b/pkg/vmcp/client/modern.go index b62e645b88..35ca59571e 100644 --- a/pkg/vmcp/client/modern.go +++ b/pkg/vmcp/client/modern.go @@ -270,12 +270,12 @@ func interpretModernResult(result json.RawMessage, rpcErr *modernRPCError, metho // mergeModernMeta strips the reserved io.modelcontextprotocol/* keys from a // caller-supplied _meta (if any) and overlays vMCP's authoritative values last. -// The caller's _meta is never mutated (StripReservedModernMeta clones it). +// The caller's _meta is never mutated (StripReservedMeta clones it). func mergeModernMeta(callerMeta any) map[string]any { m, _ := callerMeta.(map[string]any) - meta := mcpparser.StripReservedModernMeta(m) + meta := mcpparser.StripReservedMeta(m) if meta == nil { - // StripReservedModernMeta returns nil for empty/nil input; this needs a + // StripReservedMeta returns nil for empty/nil input; this needs a // non-nil map to overlay vMCP's authoritative values onto below. meta = map[string]any{} } diff --git a/pkg/vmcp/client/revision_realbackend_test.go b/pkg/vmcp/client/revision_realbackend_test.go index 59dbae6541..f19149aa63 100644 --- a/pkg/vmcp/client/revision_realbackend_test.go +++ b/pkg/vmcp/client/revision_realbackend_test.go @@ -7,6 +7,7 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "sync" "testing" @@ -156,8 +157,12 @@ func TestLegacyCallTool_StripsReservedMeta_RealBackend(t *testing.T) { mu.Lock() defer mu.Unlock() require.True(t, sawCall, "the backend's tool handler must have been invoked") - for _, k := range mcpparser.ReservedModernMetaKeys { - assert.NotContains(t, gotMeta, k, "reserved Modern _meta key %q must be stripped before the Legacy hop", k) + // Assert on the namespace, not a fixed list: this catches any reserved key, + // including ones added to the fixture later. No passthroughMetaKeys entry is + // in play here, so a blanket prefix check is exact. + for k := range gotMeta { + assert.False(t, strings.HasPrefix(k, mcpparser.ReservedMetaPrefix), + "reserved _meta key %q must be stripped before the Legacy hop", k) } assert.Equal(t, "custom-value", gotMeta["custom-caller-key"], "non-reserved caller _meta must survive") } diff --git a/pkg/vmcp/session/internal/backend/mcp_session.go b/pkg/vmcp/session/internal/backend/mcp_session.go index 0b3d0851bf..c8affcdb01 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session.go +++ b/pkg/vmcp/session/internal/backend/mcp_session.go @@ -207,8 +207,9 @@ func (c *mcpSession) CallTool( // Legacy (session-based, stateful) backend: a downstream Modern caller's // _meta.protocolVersion is only valid on a stateless Modern hop, and go-sdk // v1.7 hard-rejects ANY _meta.protocolVersion on a stateful server (HTTP 400) - // regardless of its value — see mcpparser.StripReservedModernMeta. - meta = mcpparser.StripReservedModernMeta(meta) + // regardless of its value — see mcpparser.StripReservedMeta. conversion.ToMCPMeta + // below strips too; this stays as the guard at the site that knows the hazard. + meta = mcpparser.StripReservedMeta(meta) result, err := c.client.CallTool(ctx, mcp.CallToolRequest{ Params: mcp.CallToolParams{ Name: backendName, diff --git a/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go b/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go index 714f2b9303..571b109346 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go +++ b/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "maps" + "strings" "testing" "time" @@ -183,8 +184,12 @@ func TestHTTPSession_CallTool_StripsReservedModernMeta(t *testing.T) { var gotMeta map[string]any require.NoError(t, json.Unmarshal(raw, &gotMeta)) - for _, k := range mcpparser.ReservedModernMetaKeys { - assert.NotContains(t, gotMeta, k, "reserved Modern _meta key %q must be stripped before the Legacy hop", k) + // Assert on the namespace, not a fixed list: this catches any reserved key, + // including ones added to the fixture later. No passthroughMetaKeys entry is + // in play here, so a blanket prefix check is exact. + for k := range gotMeta { + assert.False(t, strings.HasPrefix(k, mcpparser.ReservedMetaPrefix), + "reserved _meta key %q must be stripped before the Legacy hop", k) } assert.Equal(t, "custom-value", gotMeta["custom-caller-key"], "non-reserved caller _meta must survive") assert.Equal(t, "tok", gotMeta["progressToken"], "progressToken must survive") From 72da72ce9f3799b751c8557338eae6d437d54579 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 15:04:37 +0200 Subject: [PATCH 2/2] Own the reserved _meta namespace on responses On the response path both revisions forwarded every backend _meta key untouched. Modern overwrote io.modelcontextprotocol/serverInfo and let the rest through; Legacy filtered nothing. A backend could therefore set protocolVersion, clientInfo, or any other reserved key on a tools/call result and have it reach the client as if vMCP had set it. vMCP, not the backend, is the client's MCP peer, so no backend may speak for it -- and of the reserved keys only serverInfo is even schema-legal on a result, which makes the others a backend fabricating the client's own identity. Route both revisions through the one helper. Legacy strips inside conversion.ToMCPMeta, the funnel every Legacy egress already crosses; Modern strips in newModernResultMeta and re-stamps its own serverInfo last. The asymmetry is deliberate: 2025-11-25 has no serverInfo _meta key, so Legacy correctly stamps nothing. Sharing the helper is the point -- fixing one path alone is how they drifted in the first place. The elicitation adapter had the same leak in the server->client request direction, where a leaked protocolVersion is worse because the client may validate it. It now crosses the same chokepoint, which also drops its hand-rolled clone. Closes #5986 Co-Authored-By: Claude Opus 5 --- docs/arch/10-virtual-mcp-architecture.md | 32 ++++--- pkg/vmcp/conversion/conversion_test.go | 54 +++++++++++ pkg/vmcp/conversion/meta.go | 17 +++- pkg/vmcp/server/modern_envelope.go | 23 +++-- pkg/vmcp/server/modern_envelope_test.go | 90 +++++++++++++++++++ pkg/vmcp/server/sdk_elicitation_adapter.go | 21 +++-- .../server/sdk_elicitation_adapter_test.go | 66 +++++++++++++- 7 files changed, 271 insertions(+), 32 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index c503ea964b..b069170480 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -242,16 +242,28 @@ Reclassification never re-runs a request that may already have executed. An request) corrects the cache for *future* calls but returns the error, because the backend may have performed the side effect. -### Reserved `_meta` must not cross a Legacy hop - -Before any Legacy backend call, vMCP strips the reserved -`io.modelcontextprotocol/*` keys from a caller-supplied `_meta` -(`mcp.StripReservedModernMeta`, applied in `pkg/vmcp/client` and -`pkg/vmcp/session/internal/backend`). This is not cosmetic: go-sdk v1.7 rejects -**any** `_meta.protocolVersion` on a stateful streamable-HTTP server outright -(HTTP 400), regardless of its value. vMCP — not the downstream caller — is the -backend's MCP peer on that hop, so those keys are vMCP's to own. Non-reserved -caller keys (progress tokens, W3C trace context) are preserved. +### vMCP owns the reserved `_meta` namespace on both hops + +vMCP is the client's MCP peer and the backends' MCP peer on two different hops, +so the reserved `io.modelcontextprotocol/*` `_meta` keys are vMCP's to own in +both directions. A single helper, `mcp.StripReservedMeta`, removes every key +under that prefix (except the end-to-end passthrough keys — `related-task`, +`model-immediate-response`) wherever a `_meta` map crosses vMCP: + +- **Request egress** — before any Legacy backend call (`pkg/vmcp/client`, + `pkg/vmcp/session/internal/backend`) and on the Modern request path, which + overlays vMCP's own authoritative values afterwards. This is not cosmetic: + go-sdk v1.7 rejects **any** `_meta.protocolVersion` on a stateful + streamable-HTTP server outright (HTTP 400), regardless of its value. +- **Response/request egress to the client** — Legacy strips inside + `conversion.ToMCPMeta` (the funnel every Legacy egress crosses); Modern strips + in `newModernResultMeta` and re-stamps its own `serverInfo` last; the + elicitation adapter (a server→client request) crosses the same chokepoint. + This stops a backend fabricating the client's own identity (`serverInfo`, + `protocolVersion`, `clientInfo`). + +Non-reserved caller/backend keys (progress tokens, W3C trace context) are +preserved throughout. ### Observability diff --git a/pkg/vmcp/conversion/conversion_test.go b/pkg/vmcp/conversion/conversion_test.go index 3713ae8ce8..0d9a95a07c 100644 --- a/pkg/vmcp/conversion/conversion_test.go +++ b/pkg/vmcp/conversion/conversion_test.go @@ -953,6 +953,60 @@ func TestToMCPMeta(t *testing.T) { }, }, }, + { + // #5986: vMCP, not the backend, is the client's MCP peer, so a backend + // must not be able to set reserved io.modelcontextprotocol/* keys on a + // response. Only serverInfo is even schema-legal on a result; the + // request-only keys arriving on one are a backend fabricating the + // client's own identity. + name: "reserved keys are stripped, non-reserved survive", + input: map[string]any{ + "io.modelcontextprotocol/serverInfo": map[string]any{"name": "attacker"}, + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientCapabilities": map[string]any{}, + "io.modelcontextprotocol/futureThing": "whatever", + "traceId": "trace-keep", + }, + expected: &mcp.Meta{ + AdditionalFields: map[string]any{"traceId": "trace-keep"}, + }, + }, + { + // Collapses to nil rather than an empty _meta object on the wire. + name: "map of only reserved keys returns nil", + input: map[string]any{ + "io.modelcontextprotocol/serverInfo": map[string]any{"name": "attacker"}, + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + }, + expected: nil, + }, + { + // The reserved namespace is not stripped wholesale: end-to-end keys + // (task correlation) ride through while per-hop control keys go. + name: "passthrough reserved key survives alongside a stripped one", + input: map[string]any{ + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/related-task": map[string]any{"taskId": "t-1"}, + }, + expected: &mcp.Meta{ + AdditionalFields: map[string]any{ + "io.modelcontextprotocol/related-task": map[string]any{"taskId": "t-1"}, + }, + }, + }, + { + // Pins the ordering: the strip must run BEFORE the progressToken split, + // or a reserved key would land in AdditionalFields. + name: "reserved keys stripped alongside a progressToken", + input: map[string]any{ + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "progressToken": "token-xyz", + }, + expected: &mcp.Meta{ + ProgressToken: "token-xyz", + AdditionalFields: map[string]any{}, + }, + }, } for _, tt := range tests { diff --git a/pkg/vmcp/conversion/meta.go b/pkg/vmcp/conversion/meta.go index 2264a1c02f..17932aaad8 100644 --- a/pkg/vmcp/conversion/meta.go +++ b/pkg/vmcp/conversion/meta.go @@ -7,6 +7,7 @@ import ( "maps" "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" ) // FromMCPMeta converts MCP SDK meta to map[string]any for vmcp wrapper types. @@ -41,9 +42,23 @@ func FromMCPMeta(meta *mcp.Meta) map[string]any { // ToMCPMeta converts vmcp meta map to MCP SDK meta for forwarding to clients. // This reconstructs the _meta field when sending responses back through the MCP protocol. // +// Reserved io.modelcontextprotocol/* keys are stripped first: vMCP, not the +// backend, is the client's MCP peer, so a backend must not speak for it. This is +// the single chokepoint for every Legacy egress that carries backend _meta +// (serve_handlers, sessionmanager, the elicitation adapter); the Modern path's +// mirror is newModernResultMeta, and both call the same mcpparser helper so the +// two revisions cannot drift. +// +// Note the Legacy/Modern asymmetry: Modern re-stamps its own +// io.modelcontextprotocol/serverInfo after stripping, and Legacy deliberately +// does not — the 2025-11-25 revision has no serverInfo _meta key at all. The +// missing Legacy stamp is correct, not an oversight. +// // Returns nil if meta is nil or empty, following the MCP specification that -// _meta is optional and should be omitted when empty. +// _meta is optional and should be omitted when empty. A map consisting only of +// reserved keys therefore collapses to nil rather than an empty _meta object. func ToMCPMeta(meta map[string]any) *mcp.Meta { + meta = mcpparser.StripReservedMeta(meta) if len(meta) == 0 { return nil } diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 7d9e57a23f..072079aaff 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -7,7 +7,6 @@ import ( "encoding/json" "fmt" "log/slog" - "maps" "net/http" "github.com/stacklok/toolhive-core/mcpcompat/mcp" @@ -64,19 +63,19 @@ func newModernMeta(serverName, serverVersion string) modernMeta { // the serverInfo-only newModernMeta above. The SDK path preserves backend // meta via conversion.ToMCPMeta (serve_handlers.go); dropping it here would // silently discard whatever the backend attached (progress tokens, trace -// ids, ...). backendMeta is cloned before the serverInfo key is added (copy -// before mutating caller input) so the domain result's map is never touched. -// A Modern backend could in principle return that same namespaced key; the -// unconditional overwrite is still correct here because the client's actual -// MCP peer is vMCP, not the backend, so vMCP's own serverInfo must win. +// ids, ...). // -// Every other backendMeta key -- including any other io.modelcontextprotocol/* -// reserved key a backend happens to set -- is forwarded unfiltered, matching -// the Legacy path's conversion.ToMCPMeta. Stripping reserved keys is a -// tracked follow-up; it must land in a helper shared by both paths, not here -// only, or Legacy and Modern would drift. +// Every reserved io.modelcontextprotocol/* key a backend set is stripped first, +// then vMCP's own serverInfo is stamped last so it always wins: the client's +// actual MCP peer is vMCP, not the backend, so no backend may speak for it. +// mcpparser.StripReservedMeta is the SAME helper the Legacy path reaches through +// conversion.ToMCPMeta -- keep it that way, or the two revisions drift (which is +// exactly what this function's previous forward-everything behavior caused). +// It clones, so the domain result's map is never mutated, and returns nil +// whenever nothing survives -- hence the fallback below, since this result +// always carries at least serverInfo. func newModernResultMeta(backendMeta map[string]any, serverName, serverVersion string) map[string]any { - meta := maps.Clone(backendMeta) + meta := mcpparser.StripReservedMeta(backendMeta) if meta == nil { meta = make(map[string]any, 1) } diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index bbcef45416..2a75daed05 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -5,14 +5,17 @@ package server import ( "encoding/json" + "maps" "net/http" "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/require" mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/conversion" ) const ( @@ -223,6 +226,93 @@ func TestModernResultMetaOverwritesSpoofedServerInfo(t *testing.T) { require.Equal(t, testServerVersion, serverInfo["version"]) } +// TestReservedResponseMetaStrippedByBothHelpers is the #5986 regression pin: +// the Legacy and Modern response builders must both route backend _meta through +// the SAME mcpparser.StripReservedMeta, so a backend cannot set a reserved +// io.modelcontextprotocol/* key on a result and have it reach the client as if +// vMCP had set it. +// +// It compares the two HELPERS, not the two end-to-end paths -- the outputs are +// deliberately not equal (Modern always mints its own serverInfo, Legacy never +// does, since the 2025-11-25 revision has no serverInfo _meta key). It also +// cannot speak for Legacy resources/read, which drops backend result _meta +// wholesale today (coreResourceHandler returns bare []mcp.ResourceContents) +// while Modern preserves it -- a pre-existing divergence, out of scope here. +func TestReservedResponseMetaStrippedByBothHelpers(t *testing.T) { + t.Parallel() + + // One fixture, fed to both builders: a hostile backend spoofing vMCP's + // identity and asserting protocol control, alongside metadata it is + // legitimately allowed to attach. + backendMeta := map[string]any{ + "io.modelcontextprotocol/serverInfo": map[string]any{"name": "attacker", "version": "666"}, + "io.modelcontextprotocol/protocolVersion": "1999-01-01", + "io.modelcontextprotocol/clientInfo": map[string]any{"name": "spoofed-client"}, + "io.modelcontextprotocol/clientCapabilities": map[string]any{}, + "io.modelcontextprotocol/logLevel": "debug", + "io.modelcontextprotocol/subscriptionId": "sub-1", + "io.modelcontextprotocol/futureThing": "whatever", + // Reserved but relay-semantic: MUST survive (see passthroughMetaKeys). + "io.modelcontextprotocol/related-task": map[string]any{"taskId": "t-1"}, + // Non-reserved backend metadata: MUST survive. + "progressToken": "tok-1", + "traceparent": "00-abc-def-01", + "custom": "keep-me", + } + snapshot := maps.Clone(backendMeta) + + assertClean := func(t *testing.T, meta map[string]any, wantServerInfo bool) { + t.Helper() + for k := range meta { + if k == mcpparser.ReservedMetaPrefix+"related-task" { + continue + } + if k == modernServerInfoKey && wantServerInfo { + continue + } + require.False(t, strings.HasPrefix(k, mcpparser.ReservedMetaPrefix), + "backend-set reserved key %q must not reach the client", k) + } + require.Equal(t, "tok-1", meta["progressToken"], "non-reserved backend meta must survive") + require.Equal(t, "00-abc-def-01", meta["traceparent"]) + require.Equal(t, "keep-me", meta["custom"]) + require.Equal(t, map[string]any{"taskId": "t-1"}, meta[mcpparser.ReservedMetaPrefix+"related-task"], + "end-to-end task correlation must be relayed, not terminated") + } + + t.Run("modern builder strips and re-stamps its own serverInfo", func(t *testing.T) { + t.Parallel() + + got := newModernResultMeta(backendMeta, testServerName, testServerVersion) + assertClean(t, got, true) + + serverInfo, ok := got[modernServerInfoKey].(modernServerInfo) + require.True(t, ok, "vMCP must stamp its own serverInfo") + require.Equal(t, testServerName, serverInfo.Name, "vMCP's serverInfo must beat the backend's") + require.Equal(t, testServerVersion, serverInfo.Version) + }) + + t.Run("legacy builder strips and stamps nothing", func(t *testing.T) { + t.Parallel() + + got := conversion.ToMCPMeta(backendMeta) + require.NotNil(t, got) + // ToMCPMeta hoists progressToken out of the map, so put it back before + // running the shared assertions. + flat := maps.Clone(got.AdditionalFields) + flat["progressToken"] = got.ProgressToken + assertClean(t, flat, false) + }) + + t.Run("the caller's map is never mutated", func(t *testing.T) { + t.Parallel() + + // Compare against a snapshot, not just the length: a swap-one-add-one + // mutation would keep the count identical. + require.Equal(t, snapshot, backendMeta, "neither builder may mutate the domain result's map") + }) +} + // TestModernEnvelopeEmptyCollections asserts that an empty domain slice // marshals to a JSON array ([]), never null. func TestModernEnvelopeEmptyCollections(t *testing.T) { diff --git a/pkg/vmcp/server/sdk_elicitation_adapter.go b/pkg/vmcp/server/sdk_elicitation_adapter.go index 003aa00ea9..b699aa5ed7 100644 --- a/pkg/vmcp/server/sdk_elicitation_adapter.go +++ b/pkg/vmcp/server/sdk_elicitation_adapter.go @@ -7,11 +7,11 @@ package server import ( "context" - "maps" "github.com/stacklok/toolhive-core/mcpcompat/mcp" "github.com/stacklok/toolhive-core/mcpcompat/server" "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/conversion" ) // sdkElicitationAdapter wraps mcpcompat MCPServer to implement vmcp.ElicitationRequester. @@ -87,12 +87,19 @@ func (a *sdkElicitationAdapter) RequestElicitation( RequestedSchema: req.RequestedSchema, }, } - // Only attach _meta when the caller actually set it. NewMetaFromMap mutates - // its argument (it deletes progressToken), so copy first to avoid mutating - // the caller's map. - if req.Meta != nil { - mcpReq.Params.Meta = mcp.NewMetaFromMap(maps.Clone(req.Meta)) - } + // req.Meta came from the BACKEND's elicitation/create (forwarding.go's + // newElicitationForwarder), so it crosses the same trust boundary as a backend + // result: route it through conversion.ToMCPMeta, which strips the reserved + // io.modelcontextprotocol/* keys before they reach the downstream client. A + // leaked protocolVersion here is worse than on a result -- this is a + // server->client REQUEST, where a go-sdk client may validate it. + // + // ToMCPMeta also hoists progressToken and copies (never mutating the caller's + // map), and returns nil for empty input -- so _meta stays absent when the + // backend set none, or set only reserved keys. Do not swap in + // mcp.NewMetaFromMap: it returns a non-nil *Meta for nil input, which would + // start emitting an empty _meta. + mcpReq.Params.Meta = conversion.ToMCPMeta(req.Meta) // Delegate to the mcpcompat SDK's RequestElicitation method. // The SDK will: diff --git a/pkg/vmcp/server/sdk_elicitation_adapter_test.go b/pkg/vmcp/server/sdk_elicitation_adapter_test.go index 587d989005..e5d73aa768 100644 --- a/pkg/vmcp/server/sdk_elicitation_adapter_test.go +++ b/pkg/vmcp/server/sdk_elicitation_adapter_test.go @@ -142,9 +142,71 @@ func TestSDKElicitationAdapter_NilMetaProducesNoMeta(t *testing.T) { assert.Nil(t, fake.gotRequest.Params.Meta) } +// TestSDKElicitationAdapter_StripsReservedMeta is the #5986 regression pin for +// the server->client REQUEST direction. req.Meta here originates from the +// BACKEND's elicitation/create (newElicitationForwarder in pkg/vmcp/client), so +// it crosses the same trust boundary as a backend result: a reserved +// io.modelcontextprotocol/* key must not reach the downstream client, which may +// validate it (a leaked protocolVersion is worse on a request than on a result). +func TestSDKElicitationAdapter_StripsReservedMeta(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + meta map[string]any + wantNilMeta bool + wantAdditions map[string]any + }{ + { + name: "reserved keys stripped, backend metadata preserved", + meta: map[string]any{ + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/serverInfo": map[string]any{"name": "attacker"}, + "trace": "abc", + }, + wantAdditions: map[string]any{"trace": "abc"}, + }, + { + // Collapses to no _meta at all rather than an empty object, matching + // the nil-Meta behavior above. + name: "only reserved keys produces no _meta", + meta: map[string]any{ + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + }, + wantNilMeta: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fake := &fakeSDKElicitationRequester{ + result: &mcp.ElicitationResult{ + ElicitationResponse: mcp.ElicitationResponse{Action: mcp.ElicitationResponseActionAccept}, + }, + } + adapter := &sdkElicitationAdapter{mcpServer: fake} + + _, err := adapter.RequestElicitation(context.Background(), vmcp.ElicitationRequest{ + Message: "Confirm?", + Meta: tt.meta, + }) + require.NoError(t, err) + + if tt.wantNilMeta { + assert.Nil(t, fake.gotRequest.Params.Meta) + return + } + require.NotNil(t, fake.gotRequest.Params.Meta) + assert.Equal(t, tt.wantAdditions, fake.gotRequest.Params.Meta.AdditionalFields) + }) + } +} + // TestSDKElicitationAdapter_MetaIsCopied verifies that translating a request -// with Meta does not mutate the caller's map (NewMetaFromMap deletes -// progressToken from its argument). +// with Meta does not mutate the caller's map (the SDK's NewMetaFromMap deletes +// progressToken from its argument; conversion.ToMCPMeta copies instead). func TestSDKElicitationAdapter_MetaIsCopied(t *testing.T) { t.Parallel()