diff --git a/pkg/mcp/revision.go b/pkg/mcp/revision.go index 2f2e7e58f9..22dc9aebe6 100644 --- a/pkg/mcp/revision.go +++ b/pkg/mcp/revision.go @@ -116,11 +116,16 @@ var passthroughMetaKeys = map[string]struct{}{ } // modernSignalMetaKeys is the INGRESS/detection set consumed by hasModernSignal: -// the reserved keys a Legacy client never sets, whose presence — independent of -// whether the value is well-formed — is itself a claim of the Modern revision -// and must not be silently downgraded to Legacy. Only a malformed/absent -// protocolVersion alongside one of them turns into a rejection, never a -// downgrade. +// the reserved keys whose presence — independent of whether the value is +// well-formed — is a claim of the Modern revision when nothing else contradicts +// it, and must not then be silently downgraded to Legacy. +// +// "Nothing else contradicts it" is load-bearing. Real Legacy clients DO set +// these keys (the ChatGPT connector sets them while negotiating 2025-11-25), +// so an explicit non-Modern MCP-Protocol-Version header outranks them — see +// ClassifyRevision. The claim stands where the key is the only signal present, +// and there a malformed/absent protocolVersion alongside one of them turns into +// a rejection, never a downgrade. // // It is deliberately narrower than what StripReservedMeta removes: logLevel is // excluded so a request carrying only logLevel — which go-sdk's @@ -444,6 +449,25 @@ func ClassifyRevision(method string, meta map[string]any, protoHeader string) (R bodyVersion, hasBodyVersion := stringMetaValue(meta, metaKeyProtocolVersion) if !hasBodyVersion { + if protoHeader != "" && protoHeader != MCPVersionModern { + // An explicit non-Modern header is the client's negotiated + // declaration and outranks a stray reserved _meta key that carries + // no version of its own. Without this, merely including e.g. + // clientInfo flipped an otherwise-fine Legacy request from accepted + // to -32020 -- the same request with no reserved key at all is + // classified Legacy by hasModernSignal above, so rejecting this one + // was incoherent rather than strict. It broke every tools/call from + // the ChatGPT connector, which negotiates 2025-11-25 and sets + // reserved keys without a _meta protocolVersion (#6188). + // + // Modern enforcement is untouched: a Modern header with no _meta + // version still falls through to the mismatch below, and a reserved + // key with no header at all still yields MissingModernMetadata -- + // which is where "a reserved key is a claim of Modern that must not + // be silently downgraded" (see modernSignalMetaKeys) actually + // applies, because there the key is the only signal present. + return RevisionLegacy, nil + } if protoHeader != "" { return RevisionModern, &HeaderMismatchError{Header: protoHeader, Body: ""} } diff --git a/pkg/mcp/revision_test.go b/pkg/mcp/revision_test.go index a70c50839b..8d4b793f14 100644 --- a/pkg/mcp/revision_test.go +++ b/pkg/mcp/revision_test.go @@ -290,12 +290,34 @@ func TestClassifyRevision(t *testing.T) { }, }, { - name: "modern signal via reserved key with non-modern header is a header mismatch", + // #6188: the ChatGPT connector negotiates 2025-11-25 and sets + // reserved keys without a _meta protocolVersion. The explicit + // non-Modern header is its negotiated declaration and outranks the + // stray key; rejecting this broke every tools/call it sent. Note + // the same request WITHOUT the reserved key is classified Legacy by + // hasModernSignal, so rejecting it was incoherent, not strict. + name: "legacy: reserved key with an explicit non-modern header", method: "tools/call", meta: map[string]any{ metaKeyClientCapabilities: map[string]any{}, }, protoHeader: "2025-11-25", + expectedRev: RevisionLegacy, + checkErr: func(t *testing.T, err error) { + t.Helper() + require.NoError(t, err) + }, + }, + { + // Modern enforcement is untouched: a Modern header with no _meta + // protocolVersion is still the malformed shape go-sdk sends for a + // removed RPC, and still -32020. + name: "modern header with reserved key but no body version is still a mismatch", + method: "tools/call", + meta: map[string]any{ + metaKeyClientCapabilities: map[string]any{}, + }, + protoHeader: MCPVersionModern, expectedRev: RevisionModern, checkErr: func(t *testing.T, err error) { t.Helper() @@ -303,10 +325,26 @@ func TestClassifyRevision(t *testing.T) { var mismatchErr *HeaderMismatchError require.ErrorAs(t, err, &mismatchErr) assert.Equal(t, CodeHeaderMismatch, mismatchErr.Code()) - assert.Equal(t, "2025-11-25", mismatchErr.Header) + assert.Equal(t, MCPVersionModern, mismatchErr.Header) assert.Empty(t, mismatchErr.Body) }, }, + { + // An unrecognised header is still non-Modern, so it is classified + // Legacy for the same reason. Version validity is the transport's + // job (isSupportedMCPVersion), not the classifier's. + name: "legacy: reserved key with an unrecognised header", + method: "tools/call", + meta: map[string]any{ + metaKeyClientInfo: map[string]any{"name": "openai-mcp"}, + }, + protoHeader: "1999-01-01", + expectedRev: RevisionLegacy, + checkErr: func(t *testing.T, err error) { + t.Helper() + require.NoError(t, err) + }, + }, { name: "legacy: initialize with nil meta", method: "initialize", diff --git a/pkg/vmcp/server/session_management_realbackend_integration_test.go b/pkg/vmcp/server/session_management_realbackend_integration_test.go index bcd265aa74..3521aaad29 100644 --- a/pkg/vmcp/server/session_management_realbackend_integration_test.go +++ b/pkg/vmcp/server/session_management_realbackend_integration_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/aggregator" vmcpauth "github.com/stacklok/toolhive/pkg/vmcp/auth" @@ -259,11 +260,15 @@ func TestIntegration_RealBackend_ModernRequestRejectedByClassification(t *testin "id": 1, "method": "tools/call", "params": map[string]any{ - // Presence of a reserved io.modelcontextprotocol/* _meta key is - // itself a Modern signal, regardless of its value (see - // pkg/mcp/revision.go). No protocolVersion is present, so the - // non-empty header below cannot be validated against the body - // and the request is a hard rejection. + // A reserved io.modelcontextprotocol/* _meta key with no + // protocolVersion cannot be validated against the Modern header + // below, so the request is a hard rejection (see + // pkg/mcp/revision.go). + // + // The header must be the MODERN version: an explicit non-Modern + // header is the client's negotiated declaration and now outranks a + // stray reserved key, classifying the request Legacy instead + // (#6188). This case is about the still-rejected Modern shape. "_meta": map[string]any{"io.modelcontextprotocol/clientInfo": map[string]any{"name": "test"}}, "name": "echo", "arguments": map[string]any{}, @@ -276,7 +281,7 @@ func TestIntegration_RealBackend_ModernRequestRejectedByClassification(t *testin context.Background(), http.MethodPost, ts.URL+"/mcp", bytes.NewReader(payload)) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") - req.Header.Set("MCP-Protocol-Version", "2025-11-25") + req.Header.Set("MCP-Protocol-Version", mcpparser.MCPVersionModern) resp, err := ts.Client().Do(req) require.NoError(t, err) diff --git a/pkg/vmcp/server/telemetry_integration_test.go b/pkg/vmcp/server/telemetry_integration_test.go index 0a23b05e43..e0e1e634b8 100644 --- a/pkg/vmcp/server/telemetry_integration_test.go +++ b/pkg/vmcp/server/telemetry_integration_test.go @@ -18,6 +18,7 @@ import ( "go.uber.org/mock/gomock" "github.com/stacklok/toolhive/pkg/auth" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/telemetry" transportsession "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/vmcp" @@ -467,9 +468,11 @@ func TestIntegration_TelemetryRunsBeforeClassificationRejection(t *testing.T) { // Same malformed-Modern rejection payload as // TestIntegration_RealBackend_ModernRequestRejectedByClassification: a - // reserved _meta key signals Modern, but no valid protocolVersion is - // present and the header names a different (Legacy) version, so - // classifyingHandler rejects with -32020 before dispatch. + // A reserved _meta key with no valid protocolVersion cannot be validated + // against the Modern header, so classifyingHandler rejects with -32020 + // before dispatch. The header must be the MODERN version: an explicit + // non-Modern header now outranks a stray reserved key and classifies the + // request Legacy instead (#6188). body := map[string]any{ "jsonrpc": "2.0", "id": 1, @@ -486,7 +489,7 @@ func TestIntegration_TelemetryRunsBeforeClassificationRejection(t *testing.T) { req, err := http.NewRequestWithContext(ctx, http.MethodPost, baseURL+"/mcp", bytes.NewReader(payload)) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") - req.Header.Set("MCP-Protocol-Version", "2025-11-25") + req.Header.Set("MCP-Protocol-Version", mcpparser.MCPVersionModern) resp, err := http.DefaultClient.Do(req) require.NoError(t, err)