From 90c3a5cb052ca0f9065956b07388045f235a570c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 22:27:29 +0200 Subject: [PATCH 1/4] Omit absent JSON-RPC id in session-not-found MCP narrows base JSON-RPC 2.0 for error responses: schema/2025-11-25 types them as `id?: RequestId` where RequestId = string | number, so null is not representable and an absent id must be encoded by omitting the key. Base JSON-RPC 2.0 section 5 says the id "MUST be Null" when it cannot be determined; MCP deliberately overrides that. This is enforced, not advisory. The reference TypeScript SDK gates every inbound message through JSONRPCErrorResponseSchema, a .strict() object with `id: RequestIdSchema.optional()`, using the throwing parse(). An "id":null body therefore makes a conformant client throw inside its transport before the application sees the error. For session-not-found that is especially costly: clients use code -32001 to trigger automatic session recovery, so they lose recovery, not merely an error message. Add session.HasJSONRPCID as the single definition of an absent id. A plain nil check is insufficient because the transparent proxy threads the incoming id through as raw bytes: json.RawMessage(nil) and a RawMessage holding literal null are both non-nil interfaces that marshal to null. A zero id still round-trips - the predicate tests nil-ness, not zero-ness. Part of #6038. Co-Authored-By: Claude Opus 5 --- .../streamable/streamable_proxy_spec_test.go | 8 +- .../proxy/transparent/backend_routing_test.go | 49 ++++--- .../proxy/transparent/transparent_proxy.go | 5 +- pkg/transport/session/jsonrpc_errors.go | 43 +++++- pkg/transport/session/jsonrpc_errors_test.go | 124 ++++++++++++++++-- 5 files changed, 192 insertions(+), 37 deletions(-) diff --git a/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go b/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go index ad523d5878..df8f78ee91 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go +++ b/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go @@ -282,7 +282,13 @@ func TestDeleteUnknownSessionReturnsJSONRPCError(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), `"code":-32001`) - assert.Contains(t, string(body), `"id":null`) + + // A DELETE carries no JSON-RPC id, and MCP encodes an absent id by + // omitting the "id" key entirely (schema/2025-11-25), never as null. + var parsed map[string]any + require.NoError(t, json.Unmarshal(body, &parsed)) + _, ok := parsed["id"] + assert.False(t, ok, `"id" key should be absent`) } // TestSingleRequestWithStaleSessionIncludesRequestID verifies that a single diff --git a/pkg/transport/proxy/transparent/backend_routing_test.go b/pkg/transport/proxy/transparent/backend_routing_test.go index 6d2b22adee..c9e2e3082d 100644 --- a/pkg/transport/proxy/transparent/backend_routing_test.go +++ b/pkg/transport/proxy/transparent/backend_routing_test.go @@ -204,36 +204,42 @@ func TestRoundTripReturns404ForUnknownSession(t *testing.T) { // same RoundTrip already echoed it. // // Note TestRoundTripReturns404ForUnknownSession above cannot catch this: its body -// carries no "id", so it renders a null id either way. +// carries no "id" key either way, so it can't tell a present-but-omitted id apart +// from this test's cases. func TestRoundTrip404EchoesRequestID(t *testing.T) { t.Parallel() tests := []struct { - name string - body string - wantID string + name string + body string + wantIDKey bool + wantID string // only checked when wantIDKey }{ { - name: "numeric id is echoed verbatim", - body: `{"jsonrpc":"2.0","id":7,"method":"tools/list"}`, - wantID: `"id":7`, + name: "numeric id is echoed verbatim", + body: `{"jsonrpc":"2.0","id":7,"method":"tools/list"}`, + wantIDKey: true, + wantID: `"id":7`, }, { - name: "string id is echoed verbatim", - body: `{"jsonrpc":"2.0","id":"abc-123","method":"tools/list"}`, - wantID: `"id":"abc-123"`, + name: "string id is echoed verbatim", + body: `{"jsonrpc":"2.0","id":"abc-123","method":"tools/list"}`, + wantIDKey: true, + wantID: `"id":"abc-123"`, }, { - // A notification has no id by definition, so JSON-RPC requires null. - name: "notification renders a null id", - body: `{"jsonrpc":"2.0","method":"notifications/initialized"}`, - wantID: `"id":null`, + // A notification has no id by definition. MCP encodes that by + // omitting the "id" key entirely, not by emitting null. + name: "notification omits the id key", + body: `{"jsonrpc":"2.0","method":"notifications/initialized"}`, + wantIDKey: false, }, { - // An explicit null id is not a correlatable id either. - name: "explicit null id stays null", - body: `{"jsonrpc":"2.0","id":null,"method":"tools/list"}`, - wantID: `"id":null`, + // An explicit null id is not a correlatable id either, so it is + // also encoded by omitting the key. + name: "explicit null id omits the id key", + body: `{"jsonrpc":"2.0","id":null,"method":"tools/list"}`, + wantIDKey: false, }, } @@ -269,13 +275,18 @@ func TestRoundTrip404EchoesRequestID(t *testing.T) { _ = resp.Body.Close() assert.Contains(t, string(body), `"code":-32001`) - assert.Contains(t, string(body), tt.wantID) // The body must remain a single valid JSON-RPC error object -- echoing // a raw id must not corrupt the envelope. var decoded map[string]any require.NoError(t, json.Unmarshal(body, &decoded)) assert.Equal(t, "2.0", decoded["jsonrpc"]) + + _, ok := decoded["id"] + assert.Equal(t, tt.wantIDKey, ok, `"id" key presence`) + if tt.wantIDKey { + assert.Contains(t, string(body), tt.wantID) + } }) } } diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index 65db351525..e1c43b5d0c 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -602,7 +602,10 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) // can echo it, letting a client correlate the error with its request. It is set // only when parseRPCRequest reports a single request, which by construction // means the id is present and non-null; it stays nil for a notification, a - // batch, or a bodiless GET/DELETE, where JSON-RPC requires a null id. + // batch, or a bodiless GET/DELETE. session.NotFoundResponse (via NotFoundBody) + // omits the "id" key entirely for a nil requestID rather than emitting a null + // id — MCP narrows base JSON-RPC to make that omission the correct encoding + // of "no id" (see session.HasJSONRPCID). var requestID any if len(reqBody) > 0 && diff --git a/pkg/transport/session/jsonrpc_errors.go b/pkg/transport/session/jsonrpc_errors.go index 841e79a7c3..6cc4ab9faf 100644 --- a/pkg/transport/session/jsonrpc_errors.go +++ b/pkg/transport/session/jsonrpc_errors.go @@ -27,10 +27,17 @@ const ( // JSON-RPC request, echoed so a client correlating by id can match this error // to the request that caused it. // -// Pass nil only when the request genuinely carries no id, in which case -// JSON-RPC requires a null id: a bodiless GET (standalone SSE) or DELETE, a -// notification, or a batch. Do NOT pass nil merely because threading the id to -// the call site is inconvenient — that was the asymmetry fixed in #5945. +// MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the error +// response as `id?: RequestId` where RequestId = string | number, so an +// absent id is encoded by omitting the "id" key entirely, never as null. The +// reference TypeScript SDK enforces this with a .strict() schema and a +// throwing parse(), so an "id":null response crashes a conformant client's +// transport. See HasJSONRPCID. +// +// Pass nil only when the request genuinely carries no id: a bodiless GET +// (standalone SSE) or DELETE, a notification, or a batch. Do NOT pass nil +// merely because threading the id to the call site is inconvenient — that +// was the asymmetry fixed in #5945. func NotFoundBody(requestID any) []byte { resp := map[string]any{ "jsonrpc": "2.0", @@ -38,17 +45,41 @@ func NotFoundBody(requestID any) []byte { "code": CodeSessionNotFound, "message": MessageSessionNotFound, }, - "id": requestID, + } + if HasJSONRPCID(requestID) { + resp["id"] = requestID } data, err := json.Marshal(resp) if err != nil { // This should never happen with simple map types, but return a // hand-crafted fallback to guarantee a valid JSON-RPC error. - return []byte(`{"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"},"id":null}`) + return []byte(`{"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"}}`) } return data } +// HasJSONRPCID reports whether requestID is a usable JSON-RPC id, i.e. whether +// callers should emit an "id" key at all. +// +// MCP narrows base JSON-RPC here: schema/2025-11-25 types the error response as +// `id?: RequestId` where RequestId = string | number, so an absent id is encoded +// by omitting the key, never as null. The reference TypeScript SDK enforces this +// with a .strict() schema and a throwing parse(), so "id":null crashes a +// conformant client's transport. +// +// The transparent proxy threads the incoming id through as raw bytes, so a +// json.RawMessage holding literal null -- or nothing -- is an absent id even +// though the interface value is non-nil. +func HasJSONRPCID(requestID any) bool { + if requestID == nil { + return false + } + if raw, ok := requestID.(json.RawMessage); ok { + return len(raw) > 0 && string(raw) != "null" + } + return true +} + // WriteNotFound writes an HTTP 404 response with a JSON-RPC error body // for session-not-found. Use this with http.ResponseWriter in the streamable // HTTP and SSE proxies. diff --git a/pkg/transport/session/jsonrpc_errors_test.go b/pkg/transport/session/jsonrpc_errors_test.go index 7cfce0721b..594294a468 100644 --- a/pkg/transport/session/jsonrpc_errors_test.go +++ b/pkg/transport/session/jsonrpc_errors_test.go @@ -20,26 +20,46 @@ func TestNotFoundBody(t *testing.T) { tests := []struct { name string requestID any - expectedID any // expected value after JSON round-trip + wantIDKey bool + expectedID any // expected value after JSON round-trip, only checked when wantIDKey }{ { - name: "nil request ID", - requestID: nil, - expectedID: nil, + name: "nil request ID omits id key", + requestID: nil, + wantIDKey: false, + }, + { + name: "raw null request ID omits id key", + requestID: json.RawMessage(`null`), + wantIDKey: false, + }, + { + name: "nil RawMessage omits id key", + requestID: json.RawMessage(nil), + wantIDKey: false, }, { name: "integer request ID", requestID: 42, + wantIDKey: true, expectedID: float64(42), // JSON numbers decode as float64 }, + { + name: "zero request ID is still present", + requestID: 0, + wantIDKey: true, + expectedID: float64(0), + }, { name: "string request ID", requestID: "abc-123", + wantIDKey: true, expectedID: "abc-123", }, { name: "float64 request ID", requestID: float64(7), + wantIDKey: true, expectedID: float64(7), }, } @@ -56,7 +76,12 @@ func TestNotFoundBody(t *testing.T) { // Check JSON-RPC fields assert.Equal(t, "2.0", parsed["jsonrpc"]) - assert.Equal(t, tt.expectedID, parsed["id"]) + + id, ok := parsed["id"] + assert.Equal(t, tt.wantIDKey, ok, `"id" key presence`) + if tt.wantIDKey { + assert.Equal(t, tt.expectedID, id) + } errObj, ok := parsed["error"].(map[string]any) require.True(t, ok, "error field should be an object") @@ -69,6 +94,54 @@ func TestNotFoundBody(t *testing.T) { } } +func TestNotFoundBodyMarshalFallback(t *testing.T) { + t.Parallel() + + // A channel is not JSON-marshalable, forcing json.Marshal to fail so the + // hand-crafted fallback literal is exercised. Since requestID is non-nil + // and not a json.RawMessage, HasJSONRPCID reports it as present, so the + // fallback must still omit "id" per the marshal-failure branch, not echo it. + body := NotFoundBody(make(chan int)) + + var parsed map[string]any + require.NoError(t, json.Unmarshal(body, &parsed)) + assert.Equal(t, "2.0", parsed["jsonrpc"]) + _, ok := parsed["id"] + assert.False(t, ok, `fallback body must omit "id" key`) + + errObj, ok := parsed["error"].(map[string]any) + require.True(t, ok, "error field should be an object") + assert.Equal(t, float64(CodeSessionNotFound), errObj["code"]) + assert.Equal(t, MessageSessionNotFound, errObj["message"]) +} + +func TestHasJSONRPCID(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requestID any + want bool + }{ + {name: "nil", requestID: nil, want: false}, + {name: "raw null", requestID: json.RawMessage(`null`), want: false}, + {name: "raw nil slice", requestID: json.RawMessage(nil), want: false}, + {name: "raw empty slice", requestID: json.RawMessage(""), want: false}, + {name: "raw numeric", requestID: json.RawMessage(`42`), want: true}, + {name: "raw string", requestID: json.RawMessage(`"abc"`), want: true}, + {name: "string", requestID: "abc-123", want: true}, + {name: "integer zero", requestID: 0, want: true}, + {name: "integer", requestID: 42, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, HasJSONRPCID(tt.requestID)) + }) + } +} + func TestWriteNotFound(t *testing.T) { t.Parallel() @@ -87,31 +160,54 @@ func TestNotFoundResponse(t *testing.T) { tests := []struct { name string requestID any - wantID string + wantIDKey bool + wantID string // only checked when wantIDKey }{ { // A request with no id of its own: bodiless GET/DELETE, or a - // notification. JSON-RPC requires a null id here. - name: "nil id renders null", + // notification. MCP narrows base JSON-RPC to omit the key here + // rather than emit "id":null (schema/2025-11-25, HasJSONRPCID). + name: "nil id omits the id key", requestID: nil, - wantID: `"id":null`, + wantIDKey: false, + }, + { + // The transparent proxy threads the incoming id through as raw + // bytes; literal null must be treated the same as a missing id. + name: "raw null id omits the id key", + requestID: json.RawMessage(`null`), + wantIDKey: false, + }, + { + name: "nil RawMessage omits the id key", + requestID: json.RawMessage(nil), + wantIDKey: false, }, { name: "string id is echoed", requestID: "req-1", + wantIDKey: true, wantID: `"id":"req-1"`, }, + { + name: "zero id is echoed, not treated as absent", + requestID: 0, + wantIDKey: true, + wantID: `"id":0`, + }, { // The shape the transparent proxy passes: the raw bytes of the // incoming "id", forwarded verbatim so a numeric id stays numeric // rather than being coerced to a float or a string (#5945). name: "raw numeric id is echoed verbatim", requestID: json.RawMessage(`42`), + wantIDKey: true, wantID: `"id":42`, }, { name: "raw string id is echoed verbatim", requestID: json.RawMessage(`"abc"`), + wantIDKey: true, wantID: `"id":"abc"`, }, } @@ -130,7 +226,15 @@ func TestNotFoundResponse(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), `"code":-32001`) - assert.Contains(t, string(body), tt.wantID) + + var parsed map[string]any + require.NoError(t, json.Unmarshal(body, &parsed)) + _, ok := parsed["id"] + assert.Equal(t, tt.wantIDKey, ok, `"id" key presence`) + if tt.wantIDKey { + assert.Contains(t, string(body), tt.wantID) + } + // ContentLength must match the body actually written, or a client // reading exactly that many bytes truncates or blocks. assert.Equal(t, int64(len(body)), resp.ContentLength) From 8b6088f178654049042fba5ccad62618ba5331cc Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 22:48:59 +0200 Subject: [PATCH 2/4] Omit absent JSON-RPC id in pkg/mcp error bodies Apply the rule established in the previous commit to the three hand-built error envelopes in pkg/mcp: classification errors, batch rejections, and filtered tool calls. Each dropped the raw request id straight into the map, so an absent id marshaled to "id":null and made a spec-strict client throw inside its transport. classificationErrorBody is the second site reached with raw bytes -- the transparent proxy threads a json.RawMessage through it -- so it uses session.HasJSONRPCID rather than a nil check. Two tests decoded the id into a tagged struct and asserted Nil, which holds whether the key is absent or null, so they could not observe this bug at all; one of them passed with the bug deliberately reintroduced. Both now decode into a map and assert key absence. Where an id is present, the tests assert verbatim echo against the wire bytes, since a map decode coerces a numeric id to float64 and would drop the guarantee pinned in #5945. Part of #6038. Co-Authored-By: Claude Opus 5 --- pkg/mcp/batch.go | 10 +- pkg/mcp/batch_test.go | 11 +- pkg/mcp/classification_response.go | 13 +- pkg/mcp/classification_response_test.go | 132 ++++++++++++------ pkg/mcp/parser_test.go | 11 +- pkg/mcp/tool_filter.go | 12 +- pkg/mcp/tool_filter_test.go | 26 +++- .../streamable/streamable_proxy_spec_test.go | 11 +- .../proxy/transparent/backend_routing_test.go | 11 +- ...xy_over_streamable_http_mcp_server_test.go | 8 +- 10 files changed, 189 insertions(+), 56 deletions(-) diff --git a/pkg/mcp/batch.go b/pkg/mcp/batch.go index c9142ac0f2..ce9866d1e2 100644 --- a/pkg/mcp/batch.go +++ b/pkg/mcp/batch.go @@ -49,9 +49,11 @@ func IsBatchRequest(body []byte) bool { } // WriteBatchUnsupportedError writes an HTTP 400 response carrying a JSON-RPC -// "Invalid Request" error for a rejected batch. The JSON-RPC id is null: a -// batch has no single request id to echo. Use this where an http.ResponseWriter -// is available (the streamable proxy, ParsingMiddleware). +// "Invalid Request" error for a rejected batch. A batch has no single request +// id to echo, so the "id" key is omitted entirely (schema/2025-11-25 types the +// error response's id as optional, not nullable; see session.HasJSONRPCID). +// Use this where an http.ResponseWriter is available (the streamable proxy, +// ParsingMiddleware). func WriteBatchUnsupportedError(w http.ResponseWriter) { body := classificationErrorBody(nil, batchUnsupported) w.Header().Set("Content-Type", "application/json") @@ -63,7 +65,7 @@ func WriteBatchUnsupportedError(w http.ResponseWriter) { // BatchUnsupportedResponse builds an *http.Response carrying the batch-rejection // JSON-RPC error, for proxies that intercept at the RoundTripper layer (the // transparent proxy) where no http.ResponseWriter is available. Mirrors -// ClassificationErrorResponse; the JSON-RPC id is null. +// ClassificationErrorResponse; the JSON-RPC id is omitted, not null. func BatchUnsupportedResponse(req *http.Request) *http.Response { return jsonRPCErrorResponse(req, nil, batchUnsupported) } diff --git a/pkg/mcp/batch_test.go b/pkg/mcp/batch_test.go index e618818d0d..5ea28ff0ea 100644 --- a/pkg/mcp/batch_test.go +++ b/pkg/mcp/batch_test.go @@ -62,7 +62,6 @@ func TestWriteBatchUnsupportedError(t *testing.T) { var resp struct { JSONRPC string `json:"jsonrpc"` - ID any `json:"id"` Error struct { Code int64 `json:"code"` Message string `json:"message"` @@ -70,7 +69,15 @@ func TestWriteBatchUnsupportedError(t *testing.T) { } require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) assert.Equal(t, "2.0", resp.JSONRPC) - assert.Nil(t, resp.ID) assert.Equal(t, CodeInvalidRequest, resp.Error.Code) assert.NotEmpty(t, resp.Error.Message) + + // A batch has no single request id to echo. A tagged-struct decode + // (above) can't tell an absent "id" key from a present null -- both + // decode to the zero value -- so the omission is checked via a map + // decode instead. + var asMap map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &asMap)) + _, hasID := asMap["id"] + assert.False(t, hasID, `"id" key must be omitted, not present as null`) } diff --git a/pkg/mcp/classification_response.go b/pkg/mcp/classification_response.go index 9c7a4c2447..724ae942ef 100644 --- a/pkg/mcp/classification_response.go +++ b/pkg/mcp/classification_response.go @@ -10,6 +10,8 @@ import ( "fmt" "io" "net/http" + + "github.com/stacklok/toolhive/pkg/transport/session" ) // WriteClassificationError writes an HTTP 400 response with a JSON-RPC error @@ -65,6 +67,11 @@ func jsonRPCErrorResponse(req *http.Request, requestID any, err error) *http.Res // the error implements CodedError, falling back to the standard JSON-RPC // Invalid Params code otherwise -- a fallback that is currently unreachable, // since every error ClassifyRevision returns implements CodedError. +// +// MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the error +// response as `id?: RequestId` where RequestId = string | number, so an +// absent id is encoded by omitting the "id" key, never as null. See +// session.HasJSONRPCID. func classificationErrorBody(requestID any, err error) []byte { code := CodeInvalidParams var coded CodedError @@ -84,14 +91,16 @@ func classificationErrorBody(requestID any, err error) []byte { resp := map[string]any{ "jsonrpc": "2.0", "error": errBody, - "id": requestID, + } + if session.HasJSONRPCID(requestID) { + resp["id"] = requestID } body, marshalErr := json.Marshal(resp) if marshalErr != nil { // This should never happen with simple map types, but return a // hand-crafted fallback to guarantee a valid JSON-RPC error. - return []byte(`{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"},"id":null}`) + return []byte(`{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"}}`) } return body } diff --git a/pkg/mcp/classification_response_test.go b/pkg/mcp/classification_response_test.go index 631af936c5..efc7948681 100644 --- a/pkg/mcp/classification_response_test.go +++ b/pkg/mcp/classification_response_test.go @@ -10,59 +10,106 @@ import ( "net/http" "net/http/httptest" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestClassificationError(t *testing.T) { t.Parallel() tests := []struct { - name string - requestID any - err error - wantCode int64 - wantData bool + name string + requestID any + err error + wantCode int64 + wantData bool + wantIDKey bool + wantIDEcho string // substring the body must contain, verbatim, when wantIDKey }{ { - name: "header mismatch", - requestID: "req-1", - err: &HeaderMismatchError{Header: "2026-07-28", Body: "2025-11-25"}, - wantCode: CodeHeaderMismatch, - wantData: true, + name: "header mismatch", + requestID: "req-1", + err: &HeaderMismatchError{Header: "2026-07-28", Body: "2025-11-25"}, + wantCode: CodeHeaderMismatch, + wantData: true, + wantIDKey: true, + wantIDEcho: `"id":"req-1"`, }, { - name: "unsupported version", - requestID: float64(42), - err: &UnsupportedVersionError{Requested: "1999-01-01", Supported: []string{MCPVersionModern}}, - wantCode: CodeUnsupportedProtocolVersion, - wantData: true, + name: "unsupported version", + requestID: float64(42), + err: &UnsupportedVersionError{Requested: "1999-01-01", Supported: []string{MCPVersionModern}}, + wantCode: CodeUnsupportedProtocolVersion, + wantData: true, + wantIDKey: true, + wantIDEcho: `"id":42`, }, { - name: "missing client capability", - requestID: "req-3", - err: &MissingClientCapabilityError{RequiredCapabilities: map[string]any{}}, - wantCode: CodeMissingClientCapability, - wantData: true, + name: "missing client capability", + requestID: "req-3", + err: &MissingClientCapabilityError{RequiredCapabilities: map[string]any{}}, + wantCode: CodeMissingClientCapability, + wantData: true, + wantIDKey: true, + wantIDEcho: `"id":"req-3"`, }, { - name: "missing modern metadata", - requestID: "req-4", - err: &MissingModernMetadataError{}, - wantCode: CodeInvalidParams, - wantData: false, // Data() returns an empty (non-nil) map, so len(data) == 0 + name: "missing modern metadata", + requestID: "req-4", + err: &MissingModernMetadataError{}, + wantCode: CodeInvalidParams, + wantData: false, // Data() returns an empty (non-nil) map, so len(data) == 0 + wantIDKey: true, + wantIDEcho: `"id":"req-4"`, }, { - name: "nil request id", + // A nil requestID means the caller couldn't determine an id at + // all (e.g. the request failed to parse). MCP narrows base + // JSON-RPC to omit the "id" key here rather than emit + // "id":null: see session.HasJSONRPCID. + name: "nil request id omits the id key", requestID: nil, err: &HeaderMismatchError{Header: "a", Body: "b"}, wantCode: CodeHeaderMismatch, wantData: true, + wantIDKey: false, + }, + { + // The transparent proxy threads the incoming id through as raw + // bytes; literal null must be treated the same as a missing id. + name: "raw null request id omits the id key", + requestID: json.RawMessage(`null`), + err: &HeaderMismatchError{Header: "a", Body: "b"}, + wantCode: CodeHeaderMismatch, + wantData: true, + wantIDKey: false, + }, + { + name: "nil RawMessage omits the id key", + requestID: json.RawMessage(nil), + err: &HeaderMismatchError{Header: "a", Body: "b"}, + wantCode: CodeHeaderMismatch, + wantData: true, + wantIDKey: false, + }, + { + name: "zero request id is still present", + requestID: float64(0), + err: &HeaderMismatchError{Header: "a", Body: "b"}, + wantCode: CodeHeaderMismatch, + wantData: true, + wantIDKey: true, + wantIDEcho: `"id":0`, }, { - name: "plain non-coded error falls back to invalid params", - requestID: "req-6", - err: errors.New("boom"), - wantCode: CodeInvalidParams, - wantData: false, + name: "plain non-coded error falls back to invalid params", + requestID: "req-6", + err: errors.New("boom"), + wantCode: CodeInvalidParams, + wantData: false, + wantIDKey: true, + wantIDEcho: `"id":"req-6"`, }, } @@ -115,7 +162,6 @@ func TestClassificationError(t *testing.T) { Message string `json:"message"` Data map[string]any `json:"data"` } `json:"error"` - ID any `json:"id"` } if err := json.Unmarshal(wireBody, &decoded); err != nil { t.Fatalf("unmarshaling response body: %v", err) @@ -137,13 +183,19 @@ func TestClassificationError(t *testing.T) { t.Errorf("expected no data, got %v", decoded.Error.Data) } - gotID, wantID := decoded.ID, tt.requestID - if wantID == nil { - if gotID != nil { - t.Errorf("id = %v, want nil", gotID) - } - } else if gotID != wantID { - t.Errorf("id = %v (%T), want %v (%T)", gotID, gotID, wantID, wantID) + // Decoding into map[string]any (rather than a tagged struct) is + // the only way to tell an absent "id" key apart from a present + // null: a struct field would read as the zero value either way. + var asMap map[string]any + require.NoError(t, json.Unmarshal(wireBody, &asMap)) + _, hasID := asMap["id"] + assert.Equal(t, tt.wantIDKey, hasID, `"id" key presence`) + if tt.wantIDKey { + require.NotEmpty(t, tt.wantIDEcho, "table entry must set wantIDEcho when wantIDKey is true") + // A map decode would coerce a numeric id to float64, losing + // the verbatim-echo guarantee #5945 pinned -- so check the + // raw wire bytes instead. + assert.Contains(t, string(wireBody), tt.wantIDEcho) } }) } @@ -157,7 +209,7 @@ func TestClassificationError(t *testing.T) { func TestClassificationErrorBodyMarshalFallback(t *testing.T) { t.Parallel() - const want = `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"},"id":null}` + const want = `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"}}` got := classificationErrorBody(make(chan int), errors.New("boom")) if string(got) != want { t.Fatalf("fallback body = %s, want %s", got, want) diff --git a/pkg/mcp/parser_test.go b/pkg/mcp/parser_test.go index 048dae401b..11c4bb6835 100644 --- a/pkg/mcp/parser_test.go +++ b/pkg/mcp/parser_test.go @@ -284,7 +284,6 @@ func TestParsingMiddlewareRejectsBatch(t *testing.T) { var resp struct { JSONRPC string `json:"jsonrpc"` - ID any `json:"id"` Error struct { Code int64 `json:"code"` Message string `json:"message"` @@ -292,9 +291,17 @@ func TestParsingMiddlewareRejectsBatch(t *testing.T) { } require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) assert.Equal(t, "2.0", resp.JSONRPC) - assert.Nil(t, resp.ID, "batch rejection carries a null JSON-RPC id") assert.Equal(t, CodeInvalidRequest, resp.Error.Code) assert.NotEmpty(t, resp.Error.Message) + + // A batch has no single request id to echo. A tagged-struct + // decode (above) can't tell an absent "id" key apart from a + // present null -- both decode to the zero value -- so the + // omission is checked via a map decode instead. + var asMap map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &asMap)) + _, hasID := asMap["id"] + assert.False(t, hasID, `"id" key must be omitted, not present as null`) }) } } diff --git a/pkg/mcp/tool_filter.go b/pkg/mcp/tool_filter.go index a84ead2e61..5b7b30fd70 100644 --- a/pkg/mcp/tool_filter.go +++ b/pkg/mcp/tool_filter.go @@ -13,6 +13,7 @@ import ( "net/http" "strings" + "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/transport/types" ) @@ -388,6 +389,11 @@ func writeFilteredToolCallError(w http.ResponseWriter, id any) { // tool call, modeled on classificationErrorBody: the body is marshaled first // (with a hand-crafted fallback on marshal failure) so the caller only writes // headers/status once a valid body is ready. +// +// MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the error +// response as `id?: RequestId` where RequestId = string | number, so an +// absent id is encoded by omitting the "id" key, never as null. See +// session.HasJSONRPCID. func filteredToolCallErrorBody(id any) []byte { resp := map[string]any{ "jsonrpc": "2.0", @@ -395,14 +401,16 @@ func filteredToolCallErrorBody(id any) []byte { "code": CodeInvalidParams, "message": filteredToolNotFoundMessage, }, - "id": id, + } + if session.HasJSONRPCID(id) { + resp["id"] = id } body, err := json.Marshal(resp) if err != nil { // This should never happen with simple map types, but return a // hand-crafted fallback to guarantee a valid JSON-RPC error. - return []byte(`{"jsonrpc":"2.0","error":{"code":-32602,"message":"tool not found"},"id":null}`) + return []byte(`{"jsonrpc":"2.0","error":{"code":-32602,"message":"tool not found"}}`) } return body } diff --git a/pkg/mcp/tool_filter_test.go b/pkg/mcp/tool_filter_test.go index 0e400bf27a..8d967437f7 100644 --- a/pkg/mcp/tool_filter_test.go +++ b/pkg/mcp/tool_filter_test.go @@ -1945,6 +1945,16 @@ func TestNewToolCallMappingMiddleware_FilteredTool(t *testing.T) { expectStatus: http.StatusOK, expectJSONRPC: true, }, + { + // Zero is a valid JSON-RPC id and must stay present, not be + // mistaken for absent. + name: "Accept: application/json - zero id is still present", + accept: "application/json", + setAccept: true, + id: 0, + expectStatus: http.StatusOK, + expectJSONRPC: true, + }, { name: "Accept: application/json, text/event-stream", accept: "application/json, text/event-stream", @@ -1968,7 +1978,11 @@ func TestNewToolCallMappingMiddleware_FilteredTool(t *testing.T) { expectJSONRPC: true, }, { - name: "null id is echoed as null", + // MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the + // error response id as optional, not nullable, so a request with + // no usable id gets an error body that omits the "id" key + // entirely rather than echoing "id":null (session.HasJSONRPCID). + name: "null id omits the id key", accept: "application/json", setAccept: true, id: nil, @@ -2025,6 +2039,16 @@ func TestNewToolCallMappingMiddleware_FilteredTool(t *testing.T) { assert.NotContains(t, errObj["message"], "filter", "a filtered tool must look the same as a nonexistent one") + // Map decode is the only way to tell an absent "id" key apart + // from a present null -- response["id"] reads back as the same + // nil interface value either way. + _, hasID := response["id"] + if tt.id == nil { + assert.False(t, hasID, `"id" key must be omitted for a nil request id, not present as null`) + return + } + assert.True(t, hasID, `"id" key must be present`) + expectedID, err := json.Marshal(tt.id) require.NoError(t, err) actualID, err := json.Marshal(response["id"]) diff --git a/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go b/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go index df8f78ee91..255955597f 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go +++ b/pkg/transport/proxy/streamable/streamable_proxy_spec_test.go @@ -254,7 +254,16 @@ func TestBatchRequestsRejected(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), `"code":-32600`) - assert.Contains(t, string(body), `"id":null`) + + // A batch has no single request id to echo. MCP encodes that by + // omitting the "id" key entirely (schema/2025-11-25 types the + // error response id as optional, not nullable), never as null -- + // a substring check for "id":null would miss the key being + // present-but-null, so the key's absence is checked directly. + var parsed map[string]any + require.NoError(t, json.Unmarshal(body, &parsed)) + _, hasID := parsed["id"] + assert.False(t, hasID, `"id" key must be omitted, not present as null`) }) } } diff --git a/pkg/transport/proxy/transparent/backend_routing_test.go b/pkg/transport/proxy/transparent/backend_routing_test.go index c9e2e3082d..55bd76d503 100644 --- a/pkg/transport/proxy/transparent/backend_routing_test.go +++ b/pkg/transport/proxy/transparent/backend_routing_test.go @@ -383,7 +383,16 @@ func TestRoundTripRejectsBatch(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), `"code":-32600`) - assert.Contains(t, string(body), `"id":null`) + + // A batch has no single request id to echo. MCP encodes that by + // omitting the "id" key entirely (schema/2025-11-25 types the + // error response id as optional, not nullable), never as null -- + // a substring check for "id":null would miss the key being + // present-but-null, so the key's absence is checked directly. + var parsed map[string]any + require.NoError(t, json.Unmarshal(body, &parsed)) + _, hasID := parsed["id"] + assert.False(t, hasID, `"id" key must be omitted, not present as null`) }) } } diff --git a/test/e2e/stdio_proxy_over_streamable_http_mcp_server_test.go b/test/e2e/stdio_proxy_over_streamable_http_mcp_server_test.go index 3d06cd85ee..88a4a28309 100644 --- a/test/e2e/stdio_proxy_over_streamable_http_mcp_server_test.go +++ b/test/e2e/stdio_proxy_over_streamable_http_mcp_server_test.go @@ -126,7 +126,13 @@ var _ = Describe("TimeStreamableHttpMcpServer", Label("proxy", "streamable-http" body, err := io.ReadAll(resp.Body) Expect(err).ToNot(HaveOccurred()) Expect(string(body)).To(ContainSubstring(`"code":-32600`), "Should carry a JSON-RPC Invalid Request error") - Expect(string(body)).To(ContainSubstring(`"id":null`), "Batch rejection carries a null JSON-RPC id") + + // A batch has no single request id to echo. MCP encodes that by + // omitting the "id" key entirely (schema/2025-11-25 types the + // error response id as optional, not nullable), never as null. + var parsed map[string]any + Expect(json.Unmarshal(body, &parsed)).To(Succeed()) + Expect(parsed).ToNot(HaveKey("id"), "Batch rejection omits the JSON-RPC id key rather than emitting null") }) }) }) From 8a64f8b89ba99d269af57a0bed28abebeb305c04 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 23:30:56 +0200 Subject: [PATCH 3/4] Omit absent JSON-RPC id in rate-limit and vMCP Two sites, with different exposure - worth distinguishing. pkg/ratelimit is a reachable bug. rateLimitHandler gates only on the method being tools/call and has no notification guard, so a well-formed tools/call notification that trips the limiter reaches writeRateLimited with a nil id and emits "id":null at HTTP 429 - precisely the payload that makes a spec-strict client throw inside its transport. A new handler-level test drives the middleware with a notification and pins the key's absence; the pre-existing coverage only exercised the body builder, so it could not observe this. pkg/vmcp/server is defense-in-depth. Tracing every call site of writeModernError and writeModernDenied shows all of them downstream of dispatchModern's `parsed.ID == nil` guard, which answers 202 before any error envelope is built, and vMCP ids arrive from jsonrpc2.ID.Raw() as nil, int64 or string - never raw bytes. So the predicate is always true there today. The writers take `id any`, so guarding them is still right, but no client was receiving a null id from this path. writeModernResult deliberately keeps emitting id unconditionally: MCP requires it on a result response, so omitting would be as wrong as null. A test pins that, since the obvious future refactor is to make it match its siblings. Part of #6038. Co-Authored-By: Claude Opus 5 --- pkg/ratelimit/middleware.go | 15 +++- pkg/ratelimit/middleware_test.go | 92 +++++++++++++++++++++ pkg/vmcp/server/modern_dispatch_test.go | 26 ++++++ pkg/vmcp/server/modern_envelope.go | 36 ++++++-- pkg/vmcp/server/modern_envelope_test.go | 104 ++++++++++++++++++++++++ 5 files changed, 263 insertions(+), 10 deletions(-) diff --git a/pkg/ratelimit/middleware.go b/pkg/ratelimit/middleware.go index c1a842ab4f..45e81afd47 100644 --- a/pkg/ratelimit/middleware.go +++ b/pkg/ratelimit/middleware.go @@ -20,6 +20,7 @@ import ( v1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/pkg/auth" "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/transport/types" ) @@ -160,7 +161,13 @@ func writeRateLimited(w http.ResponseWriter, requestID any, retryAfter time.Dura _, _ = w.Write(rateLimitedBody(requestID, retryAfter)) } -// rateLimitedBody returns the JSON-encoded body for a rate-limited JSON-RPC error. +// rateLimitedBody returns the JSON-encoded body for a rate-limited JSON-RPC +// error. +// +// MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the error +// response as `id?: RequestId` where RequestId = string | number, so an +// absent id is encoded by omitting the "id" key, never as null. See +// session.HasJSONRPCID. func rateLimitedBody(requestID any, retryAfter time.Duration) []byte { retrySeconds := math.Ceil(retryAfter.Seconds()) resp := map[string]any{ @@ -172,12 +179,14 @@ func rateLimitedBody(requestID any, retryAfter time.Duration) []byte { "retryAfterSeconds": retrySeconds, }, }, - "id": requestID, + } + if session.HasJSONRPCID(requestID) { + resp["id"] = requestID } data, err := json.Marshal(resp) if err != nil { return []byte(fmt.Sprintf( - `{"jsonrpc":"2.0","error":{"code":-32029,"message":"Rate limit exceeded","data":{"retryAfterSeconds":%.0f}},"id":null}`, + `{"jsonrpc":"2.0","error":{"code":-32029,"message":"Rate limit exceeded","data":{"retryAfterSeconds":%.0f}}}`, retrySeconds, )) } diff --git a/pkg/ratelimit/middleware_test.go b/pkg/ratelimit/middleware_test.go index 512ff382ec..de52c38dc0 100644 --- a/pkg/ratelimit/middleware_test.go +++ b/pkg/ratelimit/middleware_test.go @@ -115,6 +115,98 @@ func TestRateLimitHandler_ToolCallRejected(t *testing.T) { assert.Equal(t, float64(42), resp["id"]) } +// TestRateLimitHandler_NotificationRejected is the live regression case: +// rateLimitHandler has no notification guard (unlike dispatchModern), so a +// well-formed tools/call notification (no id) that gets rate-limited reaches +// writeRateLimited with a nil requestID. Before the fix this emitted +// "id":null at HTTP 429; it must now omit the key entirely. +func TestRateLimitHandler_NotificationRejected(t *testing.T) { + t.Parallel() + + limiter := &dummyLimiter{decision: &Decision{Allowed: false, RetryAfter: 5 * time.Second}} + handler := rateLimitHandler(limiter)(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Fatal("next handler should not be called when rate limited") + })) + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req = withParsedMCPRequest(req, "tools/call", "echo", nil) // no id: a notification + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + assert.Equal(t, http.StatusTooManyRequests, w.Code) + + body, err := io.ReadAll(w.Body) + require.NoError(t, err) + + var resp map[string]any + require.NoError(t, json.Unmarshal(body, &resp)) + _, hasID := resp["id"] + assert.False(t, hasID, `"id" key must be absent for a notification, never "id":null`) +} + +// TestRateLimitedBody covers the "id" key's presence/absence per +// session.HasJSONRPCID: MCP narrows base JSON-RPC 2.0 so an absent id is +// encoded by omitting the key entirely, never as "id":null (schema/2025-11-25 +// types the error response id as optional string|number). +func TestRateLimitedBody(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + requestID any + wantIDKey bool + wantIDEcho string // substring the body must contain, verbatim, when wantIDKey + }{ + {name: "nil id is omitted", requestID: nil, wantIDKey: false}, + {name: "null RawMessage id is omitted", requestID: json.RawMessage("null"), wantIDKey: false}, + {name: "nil RawMessage id is omitted", requestID: json.RawMessage(nil), wantIDKey: false}, + // int64 matches the real wire type: parsed.ID comes from jsonrpc2.ID.Raw(), + // which yields nil | int64 | string, never float64. + {name: "zero id is still present", requestID: int64(0), wantIDKey: true, wantIDEcho: `"id":0`}, + {name: "present id is present", requestID: int64(42), wantIDKey: true, wantIDEcho: `"id":42`}, + {name: "string id is present", requestID: "req-1", wantIDKey: true, wantIDEcho: `"id":"req-1"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + body := rateLimitedBody(tt.requestID, 5*time.Second) + + // Decoding into map[string]any (rather than a tagged struct) is + // the only way to tell an absent "id" key apart from a present + // null: a struct field would read as the zero value either way. + var asMap map[string]any + require.NoError(t, json.Unmarshal(body, &asMap)) + _, hasID := asMap["id"] + assert.Equal(t, tt.wantIDKey, hasID, `"id" key presence`) + if tt.wantIDKey { + // A map decode would coerce a numeric id to float64, losing + // the verbatim-echo guarantee -- so check the raw wire bytes + // instead. + assert.Contains(t, string(body), tt.wantIDEcho) + } + }) + } +} + +// TestRateLimitedBodyMarshalFallback verifies the defensive fallback: if the +// response fails to marshal (unreachable in production, where requestID is +// always a simple value), rateLimitedBody still returns a valid JSON-RPC +// error with no "id" key (never "id":null) and the retryAfterSeconds value +// correctly interpolated. A channel is not JSON-marshalable and, being +// non-nil, satisfies session.HasJSONRPCID -- forcing the map's marshal to +// fail after "id" was already added. +func TestRateLimitedBodyMarshalFallback(t *testing.T) { + t.Parallel() + + const want = `{"jsonrpc":"2.0","error":{"code":-32029,"message":"Rate limit exceeded","data":{"retryAfterSeconds":5}}}` + got := rateLimitedBody(make(chan int), 5*time.Second) + assert.Equal(t, want, string(got)) + assert.NotContains(t, string(got), `"id"`) +} + func TestRateLimitHandler_RedisErrorFailOpen(t *testing.T) { t.Parallel() diff --git a/pkg/vmcp/server/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index b606a6e4df..03ed9469da 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -180,18 +180,41 @@ func TestDispatchModern_ControlFlow(t *testing.T) { parsed *mcpparser.ParsedMCPRequest wantStatus int wantCode float64 + wantIDKey bool }{ { name: "batch request returns -32600 invalid request", parsed: &mcpparser.ParsedMCPRequest{Method: "tools/call", IsRequest: true, IsBatch: true, ID: "1"}, wantStatus: http.StatusOK, wantCode: -32600, + wantIDKey: true, + }, + { + // A batch with a raw-null id exercises writeModernError's + // absent-id path through the real dispatch route, not just the + // unit-level writer tests in modern_envelope_test.go: + // schema/2025-11-25 types the error id as optional string|number, + // so MCP omits the "id" key entirely here, never "id":null. + // + // The real parser can never actually produce this state: it sets ID + // from jsonrpc2.ID.Raw(), which yields only nil, int64, or string + // (parser.go), and a plain Go nil id can't reach this branch either -- + // dispatchModern's earlier `parsed.ID == nil` notification guard (an + // interface nil check, :54) would short-circuit it to 202 first. + // json.RawMessage("null") is constructed directly here solely to drive + // the writer's defensive absent-id backstop through this route. + name: "batch request with raw-null id omits the id key", + parsed: &mcpparser.ParsedMCPRequest{Method: "tools/call", IsRequest: true, IsBatch: true, ID: json.RawMessage("null")}, + wantStatus: http.StatusOK, + wantCode: -32600, + wantIDKey: false, }, { name: "unknown method returns -32601 method not found", parsed: &mcpparser.ParsedMCPRequest{Method: "roots/list", IsRequest: true, ID: "1"}, wantStatus: http.StatusNotFound, wantCode: -32601, + wantIDKey: true, }, } @@ -205,6 +228,9 @@ func TestDispatchModern_ControlFlow(t *testing.T) { errObj, ok := body["error"].(map[string]any) require.True(t, ok, "expected a JSON-RPC error envelope") assert.Equal(t, tt.wantCode, errObj["code"]) + + _, hasID := body["id"] + assert.Equal(t, tt.wantIDKey, hasID, `"id" key presence`) }) } } diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 072079aaff..53a7e1b693 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -11,6 +11,7 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + transportsession "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/conversion" ) @@ -483,6 +484,13 @@ func modernPromptFromDomain(p vmcp.Prompt) mcp.Prompt { // {"jsonrpc":"2.0","id":,"result":} as a single HTTP 200 // application/json response -- never SSE, no Mcp-Session-Id -- matching the // Modern stateless wire contract. +// +// Unlike writeModernError/writeModernDenied below, id is ALWAYS emitted here, +// never conditionally omitted: MCP's JSONRPCResultResponse schema keeps id +// required (only the error response narrows it to optional), and dispatchModern +// never reaches this builder for a notification -- it returns 202 before +// dispatch when parsed.ID is nil (modern_dispatch.go). Making this conditional +// would be as wrong as emitting "id":null on an error response. func writeModernResult(w http.ResponseWriter, id, result any) { writeModernEnvelope(w, http.StatusOK, map[string]any{ "jsonrpc": "2.0", @@ -504,6 +512,11 @@ func writeModernResult(w http.ResponseWriter, id, result any) { // // This is a protocol-level mapping, unrelated to authorization: only // writeModernDenied changes status for a POLICY reason (403). +// +// MCP narrows base JSON-RPC 2.0 here: schema/2025-11-25 types the error +// response as `id?: RequestId` where RequestId = string | number, so an +// absent id is encoded by omitting the "id" key, never as null. See +// transportsession.HasJSONRPCID. func writeModernError(w http.ResponseWriter, id any, code int, msg string) { status := http.StatusOK switch code { @@ -512,29 +525,38 @@ func writeModernError(w http.ResponseWriter, id any, code int, msg string) { case jsonRPCCodeInvalidParams: status = http.StatusBadRequest } - writeModernEnvelope(w, status, map[string]any{ + envelope := map[string]any{ "jsonrpc": "2.0", - "id": id, "error": map[string]any{ "code": code, "message": msg, }, - }) + } + if transportsession.HasJSONRPCID(id) { + envelope["id"] = id + } + writeModernEnvelope(w, status, envelope) } // writeModernDenied writes a JSON-RPC error envelope at HTTP 403 with // mcpparser.JSONRPCCodeDenied, mirroring the Legacy call gate // (call_gate.go) and pkg/authz.handleUnauthorized: the 403 status is what // makes the audit middleware log the request as denied rather than failed. +// +// id follows writeModernError: absent (via transportsession.HasJSONRPCID) is +// encoded by omitting the "id" key, never as null. func writeModernDenied(w http.ResponseWriter, id any, msg string) { - writeModernEnvelope(w, http.StatusForbidden, map[string]any{ + envelope := map[string]any{ "jsonrpc": "2.0", - "id": id, "error": map[string]any{ "code": mcpparser.JSONRPCCodeDenied, "message": msg, }, - }) + } + if transportsession.HasJSONRPCID(id) { + envelope["id"] = id + } + writeModernEnvelope(w, http.StatusForbidden, envelope) } // writeModernEnvelope marshals envelope before writing headers/status, so a @@ -548,7 +570,7 @@ func writeModernEnvelope(w http.ResponseWriter, status int, envelope map[string] // all JSON-marshalable. Fall back to a valid JSON-RPC error body // rather than writing nothing. slog.Error("failed to marshal Modern JSON-RPC envelope", "error", err) - body = []byte(`{"jsonrpc":"2.0","id":null,"error":{"code":-32603,"message":"Internal error"}}`) + body = []byte(`{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"}}`) } w.Header().Set("Content-Type", "application/json") // Belt-and-suspenders behind the JSON-level cacheScope:"private" hint: diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index 2a75daed05..2dee69a518 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -538,6 +538,62 @@ func TestModernWriters(t *testing.T) { require.Equal(t, true, decoded.Result["ok"]) }) + // writeModernResult must ALWAYS emit "id", unlike writeModernError/ + // writeModernDenied below -- a result response's id is required by the + // MCP schema, not optional. This pins that a future refactor doesn't + // "helpfully" make it conditional to match its error-writing siblings. + t.Run("writeModernResult always emits id, even absent", func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernResult(rec, nil, map[string]any{"ok": true}) + + var asMap map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &asMap)) + _, hasID := asMap["id"] + require.True(t, hasID, `"id" key must be present on a result response even when id is nil`) + }) + + // writeModernError's "id" key follows transportsession.HasJSONRPCID: MCP + // narrows base JSON-RPC 2.0 so an absent id is encoded by omitting the + // key, never as "id":null (schema/2025-11-25 types the error response id + // as optional string|number). A map decode (not a tagged struct) is the + // only way to tell an absent key apart from a present null. + t.Run("writeModernError id presence", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id any + wantIDKey bool + wantIDEcho string // substring the body must contain, verbatim, when wantIDKey + }{ + {name: "nil id is omitted", id: nil, wantIDKey: false}, + {name: "zero id is still present", id: int64(0), wantIDKey: true, wantIDEcho: `"id":0`}, + {name: "present id is present", id: "req-2", wantIDKey: true, wantIDEcho: `"id":"req-2"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernError(rec, tt.id, jsonRPCCodeInternalError, "some message") + + var asMap map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &asMap)) + _, hasID := asMap["id"] + require.Equal(t, tt.wantIDKey, hasID, `"id" key presence`) + if tt.wantIDKey { + // A map decode would coerce a numeric id to float64, + // losing the verbatim-echo guarantee -- check the raw + // wire bytes instead. + require.Contains(t, rec.Body.String(), tt.wantIDEcho) + } + }) + } + }) + t.Run("writeModernError", func(t *testing.T) { t.Parallel() @@ -591,6 +647,54 @@ func TestModernWriters(t *testing.T) { require.Equal(t, int(mcpparser.JSONRPCCodeDenied), decoded.Error.Code) require.Equal(t, "denied by policy", decoded.Error.Message) }) + + // writeModernDenied's "id" key follows the same HasJSONRPCID rule as + // writeModernError above. + t.Run("writeModernDenied id presence", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id any + wantIDKey bool + wantIDEcho string + }{ + {name: "nil id is omitted", id: nil, wantIDKey: false}, + {name: "zero id is still present", id: int64(0), wantIDKey: true, wantIDEcho: `"id":0`}, + {name: "present id is present", id: "req-3", wantIDKey: true, wantIDEcho: `"id":"req-3"`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernDenied(rec, tt.id, "denied by policy") + + var asMap map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &asMap)) + _, hasID := asMap["id"] + require.Equal(t, tt.wantIDKey, hasID, `"id" key presence`) + if tt.wantIDKey { + require.Contains(t, rec.Body.String(), tt.wantIDEcho) + } + }) + } + }) +} + +// TestWriteModernEnvelopeMarshalFallback verifies the defensive fallback +// drops the "id" key (never "id":null) when the envelope fails to marshal. +// A channel is not JSON-marshalable, forcing json.Marshal to fail. +func TestWriteModernEnvelopeMarshalFallback(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernEnvelope(rec, http.StatusOK, map[string]any{"bad": make(chan int)}) + + const want = `{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"}}` + require.Equal(t, want, rec.Body.String()) + require.NotContains(t, rec.Body.String(), `"id"`) } // TestModernDiscover asserts server/discover's capability-flags shape: a From 2aefac42a3e1a2cbdf715e161f53b4655bd8d9da Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 23:51:12 +0200 Subject: [PATCH 4/4] Omit absent JSON-RPC id in origin and SSE errors The last two hand-built envelopes, neither of which needs the shared predicate. The origin middleware's 403 hardcoded "id": nil, so the key is simply deleted: the request is rejected on its Origin header before any JSON-RPC body is parsed, so there is no id to echo. This is the case the MCP transport spec names explicitly when it says the body "has no id" - it calls out the Origin check by name. writeSSEErrorEvent holds a typed jsonrpc2.ID, so it gates on id.IsValid() rather than HasJSONRPCID. That is not merely stylistic: the shared predicate takes `any`, and a jsonrpc2.ID zero value is a non-nil interface holding an empty struct, so it would report the id present and emit "id":{} - neither absent nor a valid RequestId. HasJSONRPCID now documents that. The origin test decoded the id into a tagged struct and asserted Nil, so it could not observe this bug at all; it now decodes into a map and pins the exact body. The new SSE test asserts the data: framing as well as the payload, since a bare JSON object in a text/event-stream is silently discarded by the client (#6037, a neighbouring writer). Part of #6038. Co-Authored-By: Claude Opus 5 --- pkg/transport/middleware/origin/origin.go | 10 ++-- .../middleware/origin/origin_test.go | 40 +++++++++------- .../proxy/streamable/streamable_proxy.go | 9 +++- .../proxy/streamable/streamable_proxy_test.go | 48 +++++++++++++++++++ pkg/transport/session/jsonrpc_errors.go | 4 ++ 5 files changed, 91 insertions(+), 20 deletions(-) diff --git a/pkg/transport/middleware/origin/origin.go b/pkg/transport/middleware/origin/origin.go index 302264751d..556a988f66 100644 --- a/pkg/transport/middleware/origin/origin.go +++ b/pkg/transport/middleware/origin/origin.go @@ -36,7 +36,7 @@ const ( // forbiddenBodyFallback is returned if JSON marshalling of the error body // fails (should never happen with simple map types). - forbiddenBodyFallback = `{"jsonrpc":"2.0","error":{"code":-32600,"message":"Origin not allowed"},"id":null}` + forbiddenBodyFallback = `{"jsonrpc":"2.0","error":{"code":-32600,"message":"Origin not allowed"}}` ) // MiddlewareParams holds the parameters for the origin middleware factory. @@ -256,7 +256,12 @@ func isLoopbackHost(host string) bool { return false } -// writeForbidden emits a 403 response with a JSON-RPC error body (id: null). +// writeForbidden emits a 403 response with a JSON-RPC error body. The "id" +// key is omitted: this rejection happens on the Origin header alone, before +// any JSON-RPC body is parsed, so there is no incoming id to echo. MCP +// narrows base JSON-RPC 2.0 here -- schema/2025-11-25 types the error +// response id as optional, string|number (no null), so an absent id must be +// encoded by omitting the key, never as "id":null. func writeForbidden(w http.ResponseWriter) { body, err := json.Marshal(map[string]any{ "jsonrpc": "2.0", @@ -264,7 +269,6 @@ func writeForbidden(w http.ResponseWriter) { "code": jsonRPCCodeInvalidRequest, "message": "Origin not allowed", }, - "id": nil, }) if err != nil { // Marshal of a static map should never fail; fall back to a literal. diff --git a/pkg/transport/middleware/origin/origin_test.go b/pkg/transport/middleware/origin/origin_test.go index 805ddb1ae7..e3b8683500 100644 --- a/pkg/transport/middleware/origin/origin_test.go +++ b/pkg/transport/middleware/origin/origin_test.go @@ -5,7 +5,6 @@ package origin import ( "encoding/json" - "io" "net/http" "net/http/httptest" "testing" @@ -175,27 +174,36 @@ func TestOriginMiddleware_MultipleOriginHeadersRejected(t *testing.T) { // assertForbiddenJSONRPC validates that rec carries a 403 with a canonical // JSON-RPC error body and that the inner handler was never invoked. +// +// The JSONEq below already catches an extra "id":null (it wouldn't match the +// expected body), so the explicit key-absence check that follows is +// belt-and-suspenders -- kept because it names the failure directly, whereas +// a JSONEq diff would just show an unexpected field. func assertForbiddenJSONRPC(t *testing.T, rec *httptest.ResponseRecorder, nextCalled bool) { t.Helper() assert.False(t, nextCalled, "next handler must NOT be invoked") assert.Equal(t, http.StatusForbidden, rec.Code) assert.Equal(t, "application/json", rec.Header().Get("Content-Type")) + assert.JSONEq(t, `{"jsonrpc":"2.0","error":{"code":-32600,"message":"Origin not allowed"}}`, rec.Body.String()) - body, err := io.ReadAll(rec.Body) - require.NoError(t, err) - var parsed struct { - JSONRPC string `json:"jsonrpc"` - Error struct { - Code int64 `json:"code"` - Message string `json:"message"` - } `json:"error"` - ID any `json:"id"` - } - require.NoError(t, json.Unmarshal(body, &parsed)) - assert.Equal(t, "2.0", parsed.JSONRPC) - assert.Equal(t, jsonRPCCodeInvalidRequest, parsed.Error.Code) - assert.Equal(t, "Origin not allowed", parsed.Error.Message) - assert.Nil(t, parsed.ID) + var parsed map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &parsed)) + _, hasID := parsed["id"] + assert.False(t, hasID, "forbidden body must omit \"id\" entirely, never send it as null") +} + +// TestForbiddenBodyFallback verifies the hand-written fallback literal used +// when json.Marshal fails (unreachable in practice with static map types, so +// this is the only path that exercises it) has the same shape as the +// marshaled body: no "id" key, never "null". +func TestForbiddenBodyFallback(t *testing.T) { + t.Parallel() + assert.JSONEq(t, `{"jsonrpc":"2.0","error":{"code":-32600,"message":"Origin not allowed"}}`, forbiddenBodyFallback) + + var parsed map[string]any + require.NoError(t, json.Unmarshal([]byte(forbiddenBodyFallback), &parsed)) + _, hasID := parsed["id"] + assert.False(t, hasID, "fallback body must omit \"id\" entirely, never send it as null") } func TestNewHandler_RejectsDisallowedOrigin(t *testing.T) { diff --git a/pkg/transport/proxy/streamable/streamable_proxy.go b/pkg/transport/proxy/streamable/streamable_proxy.go index e794736998..6de9059c1a 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy.go +++ b/pkg/transport/proxy/streamable/streamable_proxy.go @@ -769,6 +769,11 @@ func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( // data: frame, for a request whose response headers (200 + text/event-stream) // have already been sent -- an HTTP error status can no longer be set at this // point, so the error must be communicated in-band as the response payload. +// +// The "id" key is included only if id.IsValid(); MCP narrows base JSON-RPC +// 2.0 here (schema/2025-11-25 types the error response id as optional, +// string|number, no null), so an invalid/absent id must be encoded by +// omitting the key, never as "id":null. func writeSSEErrorEvent(w http.ResponseWriter, flusher http.Flusher, id jsonrpc2.ID, err error) { errMsg := "Internal error" code := -32603 @@ -778,12 +783,14 @@ func writeSSEErrorEvent(w http.ResponseWriter, flusher http.Flusher, id jsonrpc2 } errObj := map[string]any{ "jsonrpc": "2.0", - "id": id.Raw(), "error": map[string]any{ "code": code, "message": errMsg, }, } + if id.IsValid() { + errObj["id"] = id.Raw() + } data, mErr := json.Marshal(errObj) if mErr != nil { slog.Error("failed to encode SSE error event", "error", mErr) diff --git a/pkg/transport/proxy/streamable/streamable_proxy_test.go b/pkg/transport/proxy/streamable/streamable_proxy_test.go index 291c03d535..e7f6e78b16 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy_test.go +++ b/pkg/transport/proxy/streamable/streamable_proxy_test.go @@ -6,9 +6,12 @@ package streamable import ( "bytes" "context" + "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -552,3 +555,48 @@ func TestHandleGet_SessionDecisionMatrix(t *testing.T) { }) } } + +// TestWriteSSEErrorEvent verifies the "id" key is present and echoed for a +// valid jsonrpc2.ID, and omitted entirely -- never sent as "id":null -- for +// the zero-value ID (jsonrpc2.ID{}.IsValid() == false), in both cases +// preserving the SSE "data: ...\n\n" framing writeSSEData produces. +// +// The body is decoded to map[string]any rather than a tagged struct: a +// struct field of type `any` cannot distinguish an absent key from a +// present-but-null one, so it would not detect a regression to "id":null. +func TestWriteSSEErrorEvent(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id jsonrpc2.ID + wantID bool + }{ + {name: "valid id is present and echoed", id: jsonrpc2.Int64ID(7), wantID: true}, + {name: "zero-value id is omitted, not sent as null", id: jsonrpc2.ID{}, wantID: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() + + writeSSEErrorEvent(rec, rec, tt.id, errors.New("boom")) + + assert.True(t, rec.Flushed, "writeSSEErrorEvent must flush the SSE frame to the client") + + body := rec.Body.String() + require.True(t, strings.HasPrefix(body, "data: "), "SSE frame must carry the data: prefix, got %q", body) + require.True(t, strings.HasSuffix(body, "\n\n"), "SSE frame must end with a blank line, got %q", body) + payload := strings.TrimSuffix(strings.TrimPrefix(body, "data: "), "\n\n") + + var parsed map[string]any + require.NoError(t, json.Unmarshal([]byte(payload), &parsed)) + _, hasID := parsed["id"] + assert.Equal(t, tt.wantID, hasID) + if tt.wantID { + assert.EqualValues(t, tt.id.Raw(), parsed["id"]) + } + }) + } +} diff --git a/pkg/transport/session/jsonrpc_errors.go b/pkg/transport/session/jsonrpc_errors.go index 6cc4ab9faf..441396336d 100644 --- a/pkg/transport/session/jsonrpc_errors.go +++ b/pkg/transport/session/jsonrpc_errors.go @@ -70,6 +70,10 @@ func NotFoundBody(requestID any) []byte { // The transparent proxy threads the incoming id through as raw bytes, so a // json.RawMessage holding literal null -- or nothing -- is an absent id even // though the interface value is non-nil. +// +// Do not pass a typed jsonrpc2.ID here: its zero value is a non-nil interface +// holding an empty struct, so this predicate would (wrongly) report it as +// present and callers would emit "id":{}. Use id.IsValid() instead. func HasJSONRPCID(requestID any) bool { if requestID == nil { return false