From a099b2466ece2042e15562c5eb427136ee0d5b5a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 18:41:22 +0200 Subject: [PATCH 1/6] Emit conformant JSON-RPC on authz denials handleUnauthorized encoded a *jsonrpc2.Response with encoding/json directly. That struct carries no json tags and no MarshalJSON, so reflection emitted Go-capitalized keys and dropped the mandatory "jsonrpc":"2.0". Worse, jsonrpc2.ID's only field is unexported, so it rendered as {} unconditionally -- destroying the request id even when valid, which left clients unable to correlate the denial at all. Encode through jsonrpc2.EncodeMessage instead, which marshals via the library's wireCombined struct and gets the lowercase tags, the version tag, and id,omitempty. Omitting an absent id rather than emitting null is deliberate: MCP types the error response as id?: string | number, so null is not representable, and the transport spec says the body "has no id". Encoding now happens before any header is written, which removes a double-write where a 403 header could be followed by an http.Error 500. Renames a local that was assigning the incoming err parameter rather than shadowing it. Refs #5950 Co-Authored-By: Claude Opus 5 --- pkg/authz/middleware.go | 24 +++++++---- pkg/authz/middleware_test.go | 81 ++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 9 deletions(-) diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index 8850ed5ba7..2680837765 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -145,8 +145,8 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) { } // Create a JSON-RPC error response - id, err := mcp.ConvertToJSONRPC2ID(msgID) - if err != nil { + id, convErr := mcp.ConvertToJSONRPC2ID(msgID) + if convErr != nil { id = jsonrpc2.ID{} // Use empty ID if conversion fails } @@ -155,15 +155,21 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) { Error: jsonrpc2.NewError(mcp.JSONRPCCodeDenied, errorMsg), } - // Set the response headers + // Encode before writing any header, so a marshal failure never leaves a + // half-written response (e.g. a 403 header followed by a second 500 write). + body, encErr := jsonrpc2.EncodeMessage(errorResponse) + if encErr != nil { + // Unreachable in practice: errorResponse is always a well-formed + // jsonrpc2.Response built from strings/ints above. Fall back to a + // hardcoded valid JSON-RPC error body rather than writing nothing. + slog.Error("failed to encode JSON-RPC unauthorized response", "error", encErr) + body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, mcp.JSONRPCCodeDenied) + } + w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusForbidden) - - // Write the error response - if err := json.NewEncoder(w).Encode(errorResponse); err != nil { - // If we can't encode the error response, log it and return a simple error - http.Error(w, "Internal server error", http.StatusInternalServerError) - } + //nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error + _, _ = w.Write(body) } // Middleware creates an HTTP middleware that authorizes MCP requests. diff --git a/pkg/authz/middleware_test.go b/pkg/authz/middleware_test.go index 7c3ff11cb0..98fd075bf8 100644 --- a/pkg/authz/middleware_test.go +++ b/pkg/authz/middleware_test.go @@ -1245,6 +1245,87 @@ func TestMiddlewareOptimizerCallToolJSONRoundTrip(t *testing.T) { assert.True(t, handlerCalled, "handler should be called for authorized call_tool") } +// TestHandleUnauthorizedIsConformantJSONRPC drives the real middleware chain (parsing + +// authorization) for a denied request and asserts the 403 body is a spec-conformant +// JSON-RPC error response: lowercase "jsonrpc"/"id"/"error" keys and, for notifications, +// no "id" key at all. assert.JSONEq is case-sensitive, so it alone would catch "ID"/"Error" +// substituted for "id"/"error" — the id-presence check is asserted separately since JSONEq +// on a body that's missing "id" doesn't distinguish "absent" from "any other value". +func TestHandleUnauthorizedIsConformantJSONRPC(t *testing.T) { + t.Parallel() + + // The policy content is irrelevant: tasks/list is unknown to + // MCPMethodToFeatureOperation and denied before any policy is consulted. If a + // future PR adds authorization for tasks/* and this test starts failing, swap + // in any other method still absent from that map rather than reading it as an + // envelope regression. + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ + Policies: []string{`permit(principal, action == Action::"call_tool", resource == Tool::"weather");`}, + EntitiesJSON: `[]`, + }, "") + require.NoError(t, err) + + handler := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Fatal("next handler must not be called for a denied request") + }) + middleware := mcpparser.ParsingMiddleware(Middleware(authorizer, handler, nil)) + + const wantErr = `"error":{"code":403,"message":"unknown MCP method: tasks/list (not configured for authorization)"}` + + testCases := []struct { + name string + reqIDJSON string // raw "id" field in the request; "" omits it (notification) + wantBody string + }{ + { + name: "int64 id", + reqIDJSON: `"id":42,`, + wantBody: `{"jsonrpc":"2.0","id":42,` + wantErr + `}`, + }, + { + name: "string id", + reqIDJSON: `"id":"abc",`, + wantBody: `{"jsonrpc":"2.0","id":"abc",` + wantErr + `}`, + }, + { + name: "nil id (notification) omits id entirely", + reqIDJSON: "", + wantBody: `{"jsonrpc":"2.0",` + wantErr + `}`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + reqBody := `{"jsonrpc":"2.0",` + tc.reqIDJSON + `"method":"tasks/list"}` + req, err := http.NewRequest(http.MethodPost, "/messages", bytes.NewBufferString(reqBody)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{ + Subject: "test-user", + Claims: jwt.MapClaims{"sub": "test-user"}, + }} + req = req.WithContext(auth.WithIdentity(req.Context(), identity)) + + rr := httptest.NewRecorder() + middleware.ServeHTTP(rr, req) + + require.Equal(t, http.StatusForbidden, rr.Code) + assert.Equal(t, "application/json", rr.Header().Get("Content-Type")) + assert.JSONEq(t, tc.wantBody, rr.Body.String()) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded)) + _, hasID := decoded["id"] + assert.Equal(t, tc.reqIDJSON != "", hasID, "id key presence mismatch") + _, hasResult := decoded["result"] + assert.False(t, hasResult, "denied response must not contain a result key") + }) + } +} + // TestConvertToJSONRPC2ID tests the convertToJSONRPC2ID function with various ID types func TestConvertToJSONRPC2ID(t *testing.T) { t.Parallel() From 335d33f641206f205e8e71374237916117450416 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 18:57:22 +0200 Subject: [PATCH 2/6] Restore the request id on filter-failure responses writeErrorResponse marshaled a *jsonrpc2.Response with encoding/json, so like the denial path it emitted capitalized keys, no "jsonrpc":"2.0", and an id that always serialized as {}. Its marshal-failure fallback was not valid JSON-RPC either: a bare {"error": "Internal server error"} with no envelope at all. Encode through jsonrpc2.EncodeMessage, before writing the header, and make the fallback a real JSON-RPC error body. The protocol-violation path also hardcoded an empty id, so fixing the encoder alone would still have left that response uncorrelatable. Recover the real id from the parsed request on the context instead. The typed jsonrpc2.ID parameter is kept deliberately: an any-typed id is exactly how a caller reintroduces the {} bug by passing the struct. Refs #5950 Co-Authored-By: Claude Opus 5 --- pkg/authz/response_filter.go | 36 +++++++++--- pkg/authz/response_filter_test.go | 92 +++++++++++++++++++++++++++---- 2 files changed, 108 insertions(+), 20 deletions(-) diff --git a/pkg/authz/response_filter.go b/pkg/authz/response_filter.go index 46be1731be..1c5eae8ccb 100644 --- a/pkg/authz/response_filter.go +++ b/pkg/authz/response_filter.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/mcp" "github.com/stacklok/toolhive/pkg/authz/authorizers" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp/optimizer" "github.com/stacklok/toolhive/pkg/vmcp/session/optimizerdec" ) @@ -140,7 +141,7 @@ func (rfw *ResponseFilteringWriter) processJSONResponse(rawResponse []byte) erro if carriesResult(rawResponse) { slog.Warn("JSON response carried a result outside a clean Response frame; dropping as a protocol violation", "method", rfw.method) - return rfw.writeErrorResponse(jsonrpc2.ID{}, + return rfw.writeErrorResponse(rfw.requestID(), fmt.Errorf("dropped a frame carrying a result outside a clean Response")) } rfw.ResponseWriter.WriteHeader(rfw.statusCode) @@ -532,19 +533,38 @@ func (rfw *ResponseFilteringWriter) writeErrorResponse(id jsonrpc2.ID, err error Error: jsonrpc2.NewError(500, fmt.Sprintf("Error filtering response: %v", err)), } - errorData, marshalErr := json.Marshal(errorResponse) - if marshalErr != nil { - // If we can't even marshal the error, write a simple error - rfw.ResponseWriter.WriteHeader(http.StatusInternalServerError) - _, writeErr := rfw.ResponseWriter.Write([]byte(`{"error": "Internal server error"}`)) - return writeErr + // Encode before writing any header, so an encode failure never leaves a + // half-written response. + body, encErr := jsonrpc2.EncodeMessage(errorResponse) + if encErr != nil { + // Unreachable in practice: errorResponse is always a well-formed + // jsonrpc2.Response built from a valid ID and message above. Fall back + // to a hardcoded valid JSON-RPC error body rather than writing nothing. + slog.Error("failed to encode JSON-RPC error response", "error", encErr) + body = []byte(`{"jsonrpc":"2.0","error":{"code":500,"message":"Internal server error"}}`) } rfw.ResponseWriter.WriteHeader(http.StatusInternalServerError) - _, writeErr := rfw.ResponseWriter.Write(errorData) + _, writeErr := rfw.ResponseWriter.Write(body) return writeErr } +// requestID recovers the JSON-RPC id of the original request so an error +// response written mid-filtering can still be correlated by the client. +// Falls back to an empty jsonrpc2.ID (which EncodeMessage omits from the +// wire entirely) if the request was never parsed or its id doesn't convert. +func (rfw *ResponseFilteringWriter) requestID() jsonrpc2.ID { + parsed := mcpparser.GetParsedMCPRequest(rfw.request.Context()) + if parsed == nil { + return jsonrpc2.ID{} + } + id, err := mcpparser.ConvertToJSONRPC2ID(parsed.ID) + if err != nil { + return jsonrpc2.ID{} + } + return id +} + // filterFindToolResponse filters the tools list embedded in a find_tool tools/call // response. The response is a CallToolResult whose first text content item contains // a JSON-encoded optimizer.FindToolOutput. Only tools the caller is authorized to diff --git a/pkg/authz/response_filter_test.go b/pkg/authz/response_filter_test.go index a6d1f20f47..b70a3f429a 100644 --- a/pkg/authz/response_filter_test.go +++ b/pkg/authz/response_filter_test.go @@ -1138,6 +1138,11 @@ func TestResponseFilteringWriter_SSE_DisguisedResponseFrame(t *testing.T) { // TestResponseFilteringWriter_JSON_DisguisedResponseFrame is the application/json // counterpart: the disguised-result bypass is transport-independent, so the JSON // path must also fail closed on a smuggled result rather than write it through. +// +// It also pins the writeErrorResponse envelope: the error body written on this +// path must be conformant JSON-RPC (lowercase "jsonrpc"/"id"/"error" keys, no +// "id" key for a notification) and must carry the *original request's* id, not +// an empty one, so the client can still correlate the denial. func TestResponseFilteringWriter_JSON_DisguisedResponseFrame(t *testing.T) { t.Parallel() @@ -1156,20 +1161,83 @@ func TestResponseFilteringWriter_JSON_DisguisedResponseFrame(t *testing.T) { // A batch array smuggling a tools/list result. DecodeMessage rejects the // array, so the pre-fix JSON path wrote it through raw. - smuggled := `[{"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"admin_tool"}]}}]` + const smuggled = `[{"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"admin_tool"}]}}]` - req, err := http.NewRequest(http.MethodPost, "/messages", nil) - require.NoError(t, err) - req = req.WithContext(auth.WithIdentity(req.Context(), identity)) + // writeErrorResponse wraps the carriesResult error message with a fixed + // prefix and reports code 500 — an HTTP status, not a JSON-RPC error code. + // This test pins that current behavior; it does not assert the code is + // correct. + const wantErr = `"error":{"code":500,"message":"Error filtering response: ` + + `dropped a frame carrying a result outside a clean Response"}` - rr := httptest.NewRecorder() - rfw := NewResponseFilteringWriter(rr, authorizer, req, string(mcp.MethodToolsList), nil, nil) - rfw.ResponseWriter.Header().Set("Content-Type", "application/json") + testCases := []struct { + name string + reqIDJSON string // raw "id" field on the *incoming* request; "" omits it (notification) + wantIDKey string // expected "id" fragment in the error envelope; "" if id must be absent + }{ + { + name: "int id is recovered onto the error envelope", + reqIDJSON: `"id":42,`, + wantIDKey: `"id":42,`, + }, + { + name: "string id is recovered onto the error envelope", + reqIDJSON: `"id":"abc",`, + wantIDKey: `"id":"abc",`, + }, + { + name: "notification (no id) omits id entirely", + reqIDJSON: "", + wantIDKey: "", + }, + } - _, err = rfw.Write([]byte(smuggled)) - require.NoError(t, err) - require.NoError(t, rfw.FlushAndFilter()) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + reqBody := `{"jsonrpc":"2.0",` + tc.reqIDJSON + `"method":"tools/list"}` + req, err := http.NewRequest(http.MethodPost, "/messages", strings.NewReader(reqBody)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(auth.WithIdentity(req.Context(), identity)) + + // Run the real MCP parsing middleware so the request context carries a + // ParsedMCPRequest, matching how the request would look at this point + // in the real middleware chain. writeErrorResponse's id-recovery path + // reads the request id back out of that context. + var parsedReq *http.Request + mcpparser.ParsingMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + parsedReq = r + })).ServeHTTP(httptest.NewRecorder(), req) + require.NotNil(t, parsedReq) + + rr := httptest.NewRecorder() + rfw := NewResponseFilteringWriter(rr, authorizer, parsedReq, string(mcp.MethodToolsList), nil, nil) + rfw.ResponseWriter.Header().Set("Content-Type", "application/json") - assert.NotContains(t, rr.Body.String(), "admin_tool", - "smuggled result on the application/json transport leaked past the filter") + _, err = rfw.Write([]byte(smuggled)) + require.NoError(t, err) + require.NoError(t, rfw.FlushAndFilter()) + + assert.Equal(t, http.StatusInternalServerError, rr.Code) + + body := rr.Body.String() + assert.NotContains(t, body, "admin_tool", + "smuggled result on the application/json transport leaked past the filter") + + // assert.JSONEq is key-case-sensitive, so this single assertion catches + // "Error"/"ID" substituted for "error"/"id" (the historical bug), a + // missing "jsonrpc" tag, and a wrong or missing id all at once. + wantBody := `{"jsonrpc":"2.0",` + tc.wantIDKey + wantErr + `}` + assert.JSONEq(t, wantBody, body) + + var decoded map[string]json.RawMessage + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded)) + _, hasID := decoded["id"] + assert.Equal(t, tc.wantIDKey != "", hasID, "id key presence mismatch") + _, hasResult := decoded["result"] + assert.False(t, hasResult, "error response must not contain a result key") + }) + } } From cfa5edd2fa848dfee2d1039fe259362f085dc302 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 19:12:19 +0200 Subject: [PATCH 3/6] Emit conformant JSON-RPC from webhook denials Both webhook middlewares carried the same defect as the authz paths: sendErrorResponse encoded a *jsonrpc2.Response with encoding/json, so the wire body used capitalized keys, omitted "jsonrpc":"2.0", and serialized every request id as {}. Both also wrote the status header before building the body. Encode through jsonrpc2.EncodeMessage, before the header, in both. Five test assertions pinned the broken shape and so kept it in place; two of those were unchecked type assertions that would have panicked rather than failed. One assertion was worse than useless: a require.NotNil on errResp["ID"] under a comment claiming the string id round-tripped. It never did -- the value was {}, and NotNil passes on a non-nil empty map. Each package now has a test pinning the whole envelope instead. Test fixtures built ids as int and float64, types the parser never produces, since ParsedMCPRequest.ID comes from a normalized jsonrpc2.ID. They now use int64. The error code stays as the HTTP status for now; correcting it is client-visible and tracked separately. Refs #5950 Co-Authored-By: Claude Opus 5 --- pkg/webhook/mutating/middleware.go | 20 ++++- pkg/webhook/mutating/middleware_test.go | 101 ++++++++++++++-------- pkg/webhook/validating/middleware.go | 20 ++++- pkg/webhook/validating/middleware_test.go | 94 +++++++++++++++++--- 4 files changed, 178 insertions(+), 57 deletions(-) diff --git a/pkg/webhook/mutating/middleware.go b/pkg/webhook/mutating/middleware.go index d484f6af98..363f72e456 100644 --- a/pkg/webhook/mutating/middleware.go +++ b/pkg/webhook/mutating/middleware.go @@ -316,9 +316,6 @@ func readSourceIP(r *http.Request) string { } func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, msgID interface{}) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(statusCode) - id, err := mcp.ConvertToJSONRPC2ID(msgID) if err != nil { id = jsonrpc2.ID{} // Use empty ID if conversion fails. @@ -329,5 +326,20 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms ID: id, Error: jsonrpc2.NewError(int64(statusCode), message), } - _ = json.NewEncoder(w).Encode(errResp) + + // Encode before writing any header, so an encode failure never leaves a + // half-written response (e.g. a status header followed by a second write). + body, encErr := jsonrpc2.EncodeMessage(errResp) + if encErr != nil { + // Unreachable in practice: errResp is always a well-formed jsonrpc2.Response + // built from strings/ints above. Fall back to a hardcoded valid JSON-RPC + // error body rather than writing nothing. + slog.Error("failed to encode JSON-RPC error response", "error", encErr) + body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, statusCode) + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(statusCode) + //nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error + _, _ = w.Write(body) } diff --git a/pkg/webhook/mutating/middleware_test.go b/pkg/webhook/mutating/middleware_test.go index 12670dd816..ff9c3cd094 100644 --- a/pkg/webhook/mutating/middleware_test.go +++ b/pkg/webhook/mutating/middleware_test.go @@ -50,7 +50,7 @@ func makeMCPRequest(tb testing.TB, body []byte) *http.Request { req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) parsedMCP := &mcp.ParsedMCPRequest{ Method: "tools/call", - ID: float64(1), + ID: int64(1), } ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, parsedMCP) req = req.WithContext(ctx) @@ -405,6 +405,65 @@ func TestMutatingMiddleware_SkipNonMCPRequests(t *testing.T) { assert.Equal(t, http.StatusOK, rr.Code) } +// TestMutatingMiddlewareDenialIsConformantJSONRPC pins the sendErrorResponse envelope on +// the wire: lowercase "jsonrpc"/"id"/"error" keys, no "id" key for a notification, and no +// "result" key. assert.JSONEq is case-sensitive, so a single assertion catches "Error"/"ID" +// substituted for "error"/"id" (the historical bug) alongside a missing "jsonrpc" tag. +func TestMutatingMiddlewareDenialIsConformantJSONRPC(t *testing.T) { + t.Parallel() + + // Deterministic denial: the webhook always disallows, driving sendErrorResponse + // through the real middleware chain rather than constructing the response by hand. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(webhook.MutatingResponse{ + Response: webhook.Response{Version: webhook.APIVersion, UID: "resp-uid", Allowed: false}, + }) + })) + t.Cleanup(server.Close) + + cfg := makeConfig(server.URL, webhook.FailurePolicyFail) + mw := createMutatingHandler(makeExecutors(t, []webhook.Config{cfg}), "srv", "stdio") + + const wantErr = `"error":{"code":403,"message":"Request denied by webhook policy"}` + + testCases := []struct { + name string + id interface{} // ParsedMCPRequest.ID; nil for a notification + wantBody string + }{ + {name: "int64 id", id: int64(42), wantBody: `{"jsonrpc":"2.0","id":42,` + wantErr + `}`}, + {name: "string id", id: "abc", wantBody: `{"jsonrpc":"2.0","id":"abc",` + wantErr + `}`}, + {name: "nil id (notification) omits id entirely", id: nil, wantBody: `{"jsonrpc":"2.0",` + wantErr + `}`}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader([]byte(`{"jsonrpc":"2.0","method":"tools/call"}`))) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: tc.id}) + req = req.WithContext(ctx) + + rr := httptest.NewRecorder() + mw(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Fatal("next handler must not be called for a denied request") + })).ServeHTTP(rr, req) + + require.Equal(t, http.StatusForbidden, rr.Code) + assert.Equal(t, "application/json", rr.Header().Get("Content-Type")) + assert.JSONEq(t, tc.wantBody, rr.Body.String()) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded)) + _, hasID := decoded["id"] + assert.Equal(t, tc.id != nil, hasID, "id key presence mismatch") + _, hasResult := decoded["result"] + assert.False(t, hasResult, "denied response must not contain a result key") + }) + } +} + // makeJSONBodyOfSize builds a syntactically-valid JSON body of exactly `size` // bytes by padding the value of a "data" field with ASCII characters. The // resulting bytes are valid JSON-RPC for use as an MCP request body. @@ -435,7 +494,7 @@ func TestMutatingMiddleware_RequestBodySizeLimit(t *testing.T) { body := makeJSONBodyOfSize(t, webhook.MaxRequestSize+1) req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: 1}) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: int64(1)}) req = req.WithContext(ctx) var nextCalled bool @@ -449,7 +508,7 @@ func TestMutatingMiddleware_RequestBodySizeLimit(t *testing.T) { var errResp map[string]interface{} require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &errResp)) - errObj, ok := errResp["Error"].(map[string]interface{}) + errObj, ok := errResp["error"].(map[string]interface{}) require.True(t, ok) assert.Equal(t, float64(http.StatusRequestEntityTooLarge), errObj["code"]) assert.Equal(t, "Request body exceeds maximum size", errObj["message"]) @@ -466,7 +525,7 @@ func TestMutatingMiddleware_RequestBodySizeLimit(t *testing.T) { body := makeJSONBodyOfSize(t, webhook.MaxRequestSize) req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: 1}) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: int64(1)}) req = req.WithContext(ctx) var nextCalled bool @@ -720,40 +779,6 @@ func TestMutatingMiddleware_MalformedPatchJSON(t *testing.T) { assert.Equal(t, http.StatusInternalServerError, rr.Code) } -//nolint:paralleltest -func TestMutatingMiddleware_StringRequestID(t *testing.T) { - // Tests that the middleware correctly handles a string JSON-RPC ID. - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - resp := webhook.MutatingResponse{ - Response: webhook.Response{Version: webhook.APIVersion, UID: "uid", Allowed: false}, - } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(resp) - })) - defer server.Close() - - cfg := makeConfig(server.URL, webhook.FailurePolicyFail) - mw := createMutatingHandler(makeExecutors(t, []webhook.Config{cfg}), "srv", "stdio") - - reqBody := []byte(`{"jsonrpc":"2.0","method":"tools/call","id":"string-id"}`) - req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(reqBody)) - // Use string ID in parsedMCP. - parsedMCP := &mcp.ParsedMCPRequest{Method: "tools/call", ID: "string-id"} - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, parsedMCP) - req = req.WithContext(ctx) - - rr := httptest.NewRecorder() - mw(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})).ServeHTTP(rr, req) - - assert.Equal(t, http.StatusForbidden, rr.Code) - assert.Contains(t, rr.Body.String(), "Request denied by webhook policy") - - // Confirm JSON-RPC error has the string ID. - var errResp map[string]interface{} - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &errResp)) - require.NotNil(t, errResp["ID"]) -} - //nolint:paralleltest func TestMutatingMiddleware_InvalidPatchOp_FailPolicy(t *testing.T) { // Returns a well-formed JSON array but with an invalid op type, so diff --git a/pkg/webhook/validating/middleware.go b/pkg/webhook/validating/middleware.go index a7783ae0bf..7b872ba497 100644 --- a/pkg/webhook/validating/middleware.go +++ b/pkg/webhook/validating/middleware.go @@ -181,9 +181,6 @@ func readSourceIP(r *http.Request) string { } func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, msgID interface{}) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(statusCode) - id, err := mcp.ConvertToJSONRPC2ID(msgID) if err != nil { id = jsonrpc2.ID{} // Use empty ID if conversion fails @@ -195,5 +192,20 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms ID: id, Error: jsonrpc2.NewError(int64(statusCode), message), } - _ = json.NewEncoder(w).Encode(errResp) + + // Encode before writing any header, so an encode failure never leaves a + // half-written response (e.g. a status header followed by a second write). + body, encErr := jsonrpc2.EncodeMessage(errResp) + if encErr != nil { + // Unreachable in practice: errResp is always a well-formed jsonrpc2.Response + // built from strings/ints above. Fall back to a hardcoded valid JSON-RPC + // error body rather than writing nothing. + slog.Error("failed to encode JSON-RPC error response", "error", encErr) + body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, statusCode) + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(statusCode) + //nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error + _, _ = w.Write(body) } diff --git a/pkg/webhook/validating/middleware_test.go b/pkg/webhook/validating/middleware_test.go index 862b65938c..b3e32e6721 100644 --- a/pkg/webhook/validating/middleware_test.go +++ b/pkg/webhook/validating/middleware_test.go @@ -77,7 +77,7 @@ func TestValidatingMiddleware(t *testing.T) { // Add parsed MCP request and auth identity to context parsedMCP := &mcp.ParsedMCPRequest{ Method: "tools/call", - ID: 1, + ID: int64(1), } ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, parsedMCP) @@ -169,7 +169,7 @@ func TestValidatingMiddleware(t *testing.T) { err := json.Unmarshal(rr.Body.Bytes(), &errResp) require.NoError(t, err) - errObj, ok := errResp["Error"].(map[string]interface{}) + errObj, ok := errResp["error"].(map[string]interface{}) require.True(t, ok) assert.Equal(t, float64(http.StatusForbidden), errObj["code"]) assert.Equal(t, "Request denied by policy", errObj["message"]) @@ -270,6 +270,76 @@ func TestValidatingMiddleware(t *testing.T) { }) } +// TestValidatingMiddlewareDenialIsConformantJSONRPC pins the sendErrorResponse envelope on +// the wire: lowercase "jsonrpc"/"id"/"error" keys, no "id" key for a notification, and no +// "result" key. assert.JSONEq is case-sensitive, so a single assertion catches "Error"/"ID" +// substituted for "error"/"id" (the historical bug) alongside a missing "jsonrpc" tag. +func TestValidatingMiddlewareDenialIsConformantJSONRPC(t *testing.T) { + t.Parallel() + + // Deterministic denial: the webhook always disallows, driving sendErrorResponse + // through the real middleware chain rather than constructing the response by hand. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(webhook.Response{ + Version: webhook.APIVersion, + UID: "resp-uid", + Allowed: false, + Message: "ignored", // sendErrorResponse ignores the webhook's message to avoid leaking info + }) + })) + t.Cleanup(server.Close) + + cfg := webhook.Config{ + Name: "test-webhook", + URL: server.URL, + Timeout: webhook.DefaultTimeout, + FailurePolicy: webhook.FailurePolicyFail, + TLSConfig: &webhook.TLSConfig{InsecureSkipVerify: true}, + } + client, err := webhook.NewClient(cfg, webhook.TypeValidating, nil) + require.NoError(t, err) + mw := createValidatingHandler([]clientExecutor{{client: client, config: cfg}}, "test-server", "stdio") + + const wantErr = `"error":{"code":403,"message":"Request denied by policy"}` + + testCases := []struct { + name string + id interface{} // ParsedMCPRequest.ID; nil for a notification + wantBody string + }{ + {name: "int64 id", id: int64(42), wantBody: `{"jsonrpc":"2.0","id":42,` + wantErr + `}`}, + {name: "string id", id: "abc", wantBody: `{"jsonrpc":"2.0","id":"abc",` + wantErr + `}`}, + {name: "nil id (notification) omits id entirely", id: nil, wantBody: `{"jsonrpc":"2.0",` + wantErr + `}`}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader([]byte(`{"jsonrpc":"2.0","method":"tools/call"}`))) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: tc.id}) + req = req.WithContext(ctx) + + rr := httptest.NewRecorder() + mw(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Fatal("next handler must not be called for a denied request") + })).ServeHTTP(rr, req) + + require.Equal(t, http.StatusForbidden, rr.Code) + assert.Equal(t, "application/json", rr.Header().Get("Content-Type")) + assert.JSONEq(t, tc.wantBody, rr.Body.String()) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &decoded)) + _, hasID := decoded["id"] + assert.Equal(t, tc.id != nil, hasID, "id key presence mismatch") + _, hasResult := decoded["result"] + assert.False(t, hasResult, "denied response must not contain a result key") + }) + } +} + // makeJSONBodyOfSize builds a syntactically-valid JSON body of exactly `size` // bytes by padding the value of a "data" field with ASCII characters. The // resulting bytes are valid JSON-RPC for use as an MCP request body. @@ -312,7 +382,7 @@ func TestValidatingMiddleware_RequestBodySizeLimit(t *testing.T) { body := makeJSONBodyOfSize(t, webhook.MaxRequestSize+1) req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: 1}) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: int64(1)}) req = req.WithContext(ctx) var nextCalled bool @@ -327,7 +397,7 @@ func TestValidatingMiddleware_RequestBodySizeLimit(t *testing.T) { // The error response is a JSON-RPC envelope. var errResp map[string]interface{} require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &errResp)) - errObj, ok := errResp["Error"].(map[string]interface{}) + errObj, ok := errResp["error"].(map[string]interface{}) require.True(t, ok) assert.Equal(t, float64(http.StatusRequestEntityTooLarge), errObj["code"]) assert.Equal(t, "Request body exceeds maximum size", errObj["message"]) @@ -352,7 +422,7 @@ func TestValidatingMiddleware_RequestBodySizeLimit(t *testing.T) { body := makeJSONBodyOfSize(t, webhook.MaxRequestSize) req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body)) - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: 1}) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: int64(1)}) req = req.WithContext(ctx) var nextCalled bool @@ -496,7 +566,7 @@ func TestCreateMiddleware_ResolvesHMACSecret(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader([]byte(`{"jsonrpc":"2.0","method":"tools/call","id":1}`))) req = req.WithContext(context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{ Method: "tools/call", - ID: 1, + ID: int64(1), })) rr := httptest.NewRecorder() @@ -550,7 +620,7 @@ func TestValidatingMiddleware_HTTP422AlwaysDenies(t *testing.T) { reqBody := []byte(`{"jsonrpc":"2.0","method":"tools/call","id":1}`) req := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(reqBody)) - ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: 1}) + ctx := context.WithValue(req.Context(), mcp.MCPRequestContextKey, &mcp.ParsedMCPRequest{Method: "tools/call", ID: int64(1)}) req = req.WithContext(ctx) var nextCalled bool @@ -660,8 +730,9 @@ func TestMultiWebhookChain(t *testing.T) { // Verify error response var errResp map[string]interface{} - _ = json.Unmarshal(rr.Body.Bytes(), &errResp) - errObj := errResp["Error"].(map[string]interface{}) + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &errResp)) + errObj, ok := errResp["error"].(map[string]interface{}) + require.True(t, ok) assert.Equal(t, "Request denied by policy", errObj["message"]) }) @@ -685,8 +756,9 @@ func TestMultiWebhookChain(t *testing.T) { // Verify error response var errResp map[string]interface{} - _ = json.Unmarshal(rr.Body.Bytes(), &errResp) - errObj := errResp["Error"].(map[string]interface{}) + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &errResp)) + errObj, ok := errResp["error"].(map[string]interface{}) + require.True(t, ok) assert.Equal(t, "Request denied by policy", errObj["message"]) }) From 71d114a2c7467f7dce0cc9908103bc6519d37c2a Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 19:12:19 +0200 Subject: [PATCH 4/6] Correct a JSONEq claim in a test comment The comment justified asserting id presence separately by saying JSONEq cannot distinguish an absent id from any other value. It can: JSONEq compares fully decoded values, so it enforces the exact key set. The separate assertion is worth keeping as self-documenting, just not for that reason. Co-Authored-By: Claude Opus 5 --- pkg/authz/middleware_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/authz/middleware_test.go b/pkg/authz/middleware_test.go index 98fd075bf8..776ed15dca 100644 --- a/pkg/authz/middleware_test.go +++ b/pkg/authz/middleware_test.go @@ -1249,8 +1249,8 @@ func TestMiddlewareOptimizerCallToolJSONRoundTrip(t *testing.T) { // authorization) for a denied request and asserts the 403 body is a spec-conformant // JSON-RPC error response: lowercase "jsonrpc"/"id"/"error" keys and, for notifications, // no "id" key at all. assert.JSONEq is case-sensitive, so it alone would catch "ID"/"Error" -// substituted for "id"/"error" — the id-presence check is asserted separately since JSONEq -// on a body that's missing "id" doesn't distinguish "absent" from "any other value". +// substituted for "id"/"error" and would fail on an unexpected "id" key — the id-presence +// check is asserted separately anyway, as a belt-and-braces, self-documenting assertion. func TestHandleUnauthorizedIsConformantJSONRPC(t *testing.T) { t.Parallel() From f7b530d321e3b6924ea87eb04f51a9da1bac59bf Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 19:27:30 +0200 Subject: [PATCH 5/6] Let the e2e harness see the JSON-RPC version tag The raw client's response struct had no jsonrpc field at all, so a missing or wrong version tag was invisible to every e2e test -- the harness could not have asserted the thing #5950 is about even if a test wanted to. Combined with encoding/json matching "Error" to json:"error" case-insensitively, that is how the malformed envelope rode through the suite unnoticed: only the outer envelope was reflected, while the inner error object carries real json tags, so assertions on error.code kept passing. Surface the tag on RawResponse and assert it, along with the request id round-tripping, on the dual-era denial path. A comment there described the non-conformant wire shape as a known open problem and deferred the conformance assertion; it now makes that assertion instead. Because the dual-era suite needs live infrastructure, the new field also gets hermetic coverage in the httptest-based client test, which runs in every shard -- otherwise the detector added to catch this bug would itself be unverified before merge. Refs #5950 Co-Authored-By: Claude Opus 5 --- test/e2e/dual_era_functional_test.go | 17 +++++++++-------- test/e2e/mcp_raw_client.go | 15 +++++++++------ test/e2e/mcp_raw_client_test.go | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/test/e2e/dual_era_functional_test.go b/test/e2e/dual_era_functional_test.go index 36b0362b2b..6132611de3 100644 --- a/test/e2e/dual_era_functional_test.go +++ b/test/e2e/dual_era_functional_test.go @@ -5,6 +5,7 @@ package e2e_test import ( "context" + "encoding/json" "fmt" "os" "os/exec" @@ -423,29 +424,29 @@ var _ = Describe("server/discover Authorization Guard", Label("proxy", "stateles Expect(callResp.Error).To(BeNil()) }) - It("still denies with 403 a method the Cedar policy does not permit", func() { + It("still denies with 403 a method the Cedar policy does not permit, with a conformant JSON-RPC envelope", func() { // server/discover's always-allowed flip doesn't touch the general // default-deny rule (MCPMethodToFeatureOperation's doc comment: // "Methods not in this map are denied by default"). tools/call IS in // the map and routes through Cedar; the policy in this BeforeEach // only permits resource == Tool::"echo", so calling any other tool - // name still gets denied -- keeps the 403/denial path (and its - // non-conformant-envelope wire shape, #5950) covered by this suite. + // name still gets denied -- keeps the 403/denial path covered by this + // suite. req, err := e2e.NewModernRequest("tools/call", map[string]any{ "name": "notpermitted", "arguments": map[string]any{"input": "shouldbedenied"}, }) Expect(err).ToNot(HaveOccurred()) + req.WithID(int64(9001)) resp, err := client.Send(context.Background(), proxyURL, req) Expect(err).ToNot(HaveOccurred()) Expect(resp.StatusCode).To(Equal(403)) Expect(resp.Error).ToNot(BeNil()) - // The denial envelope is currently non-conformant JSON-RPC (capitalized - // Result/Error/ID, no "jsonrpc":"2.0" -- tracked in #5950); this parses - // only because encoding/json's unmarshal is case-insensitive. That's the - // envelope's job to fix, not this guard's -- here we're asserting authz - // *behavior* (denied, code 403), not wire conformance. Expect(resp.Error.Code).To(Equal(int64(403))) + // The denial envelope must be conformant JSON-RPC: correct version tag + // and the request's id echoed back. + Expect(resp.JSONRPC).To(Equal("2.0")) + Expect(resp.ID).To(Equal(json.Number("9001"))) }) }) diff --git a/test/e2e/mcp_raw_client.go b/test/e2e/mcp_raw_client.go index 8dbb96be78..28792b8348 100644 --- a/test/e2e/mcp_raw_client.go +++ b/test/e2e/mcp_raw_client.go @@ -274,11 +274,12 @@ type RawRPCError struct { // RawResponse is the parsed result of sending a single (non-batch) JSON-RPC // request over MCP-over-HTTP. For batch responses (a JSON array), inspect -// Body directly instead of ID/Result/Error. +// Body directly instead of JSONRPC/ID/Result/Error. type RawResponse struct { StatusCode int Headers http.Header Body []byte // raw response body, always populated + JSONRPC string // echoed "jsonrpc" version tag; "" if absent or unparsable ID any // echoed "id"; nil if absent, JSON null, or unparsable Result json.RawMessage Error *RawRPCError @@ -374,20 +375,22 @@ type wireError struct { // wireResponse is the shape of a single JSON-RPC response object. ID is kept // as raw JSON so decodeID can preserve large integers exactly. type wireResponse struct { - ID json.RawMessage `json:"id"` - Result json.RawMessage `json:"result"` - Error *wireError `json:"error"` + JSONRPC string `json:"jsonrpc"` + ID json.RawMessage `json:"id"` + Result json.RawMessage `json:"result"` + Error *wireError `json:"error"` } // populateEnvelope best-effort parses body as a single JSON-RPC response // object into out. A malformed or non-object body (e.g. a batch array, or -// deliberately garbled test input) leaves ID/Result/Error at their zero -// values; out.Body still carries the raw bytes for the caller to inspect. +// deliberately garbled test input) leaves JSONRPC/ID/Result/Error at their +// zero values; out.Body still carries the raw bytes for the caller to inspect. func populateEnvelope(body []byte, out *RawResponse) { var w wireResponse if err := json.Unmarshal(body, &w); err != nil { return } + out.JSONRPC = w.JSONRPC out.ID = decodeID(w.ID) out.Result = w.Result if w.Error != nil { diff --git a/test/e2e/mcp_raw_client_test.go b/test/e2e/mcp_raw_client_test.go index e0402ecfe7..3f5bba9cb7 100644 --- a/test/e2e/mcp_raw_client_test.go +++ b/test/e2e/mcp_raw_client_test.go @@ -226,6 +226,25 @@ func TestRawClientSend(t *testing.T) { require.Equal(t, json.Number("42"), resp.ID) require.JSONEq(t, `{"ok":true}`, string(resp.Result)) require.Nil(t, resp.Error) + require.Equal(t, "2.0", resp.JSONRPC) + }) + + t.Run("pre-fix denial shape (capitalized keys, no jsonrpc tag) leaves JSONRPC empty", func(t *testing.T) { + t.Parallel() + // This is the exact shape a jsonrpc2.Response marshaled with + // encoding/json's default reflection produces (no json tags, unexported + // ID field): pins that the harness reads it as having no version tag, + // rather than defaulting or being fooled into "2.0" by the stdlib's + // case-insensitive key matching. The case above is what actually fails + // if JSONRPC stops being populated. + server, _ := newCapturingServer(t, `{"Result":null,"Error":{"code":403,"message":"x"},"ID":{}}`) + + req, err := NewLegacyRequest("ping", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Empty(t, resp.JSONRPC) }) t.Run("parses error.code, error.data and a large-int64 id without precision loss", func(t *testing.T) { From 0e7fc5892dc7c362bedb047076975cc2ebcd694c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 19:54:00 +0200 Subject: [PATCH 6/6] Read the version tag case-sensitively in the e2e client encoding/json falls back to case-insensitive field matching, so a body carrying "JSONRPC":"2.0" satisfied json:"jsonrpc" and passed the new version-tag assertion. That is a hole in exactly the place the assertion was added to guard, since #5950 was a capitalized-keys bug: the absent tag a reflection-marshaled response produces is caught, but an envelope hand-rolled with an untagged field would not have been, and this tree has five hand-built envelope builders. Read the tag from a raw map instead, which is case-sensitive, so every assertion on it is strict without each test opting in. Refs #5950 Co-Authored-By: Claude Opus 5 --- test/e2e/mcp_raw_client.go | 16 +++++++++++----- test/e2e/mcp_raw_client_test.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/test/e2e/mcp_raw_client.go b/test/e2e/mcp_raw_client.go index 28792b8348..886599fde4 100644 --- a/test/e2e/mcp_raw_client.go +++ b/test/e2e/mcp_raw_client.go @@ -375,10 +375,9 @@ type wireError struct { // wireResponse is the shape of a single JSON-RPC response object. ID is kept // as raw JSON so decodeID can preserve large integers exactly. type wireResponse struct { - JSONRPC string `json:"jsonrpc"` - ID json.RawMessage `json:"id"` - Result json.RawMessage `json:"result"` - Error *wireError `json:"error"` + ID json.RawMessage `json:"id"` + Result json.RawMessage `json:"result"` + Error *wireError `json:"error"` } // populateEnvelope best-effort parses body as a single JSON-RPC response @@ -390,7 +389,14 @@ func populateEnvelope(body []byte, out *RawResponse) { if err := json.Unmarshal(body, &w); err != nil { return } - out.JSONRPC = w.JSONRPC + // encoding/json struct tags match case-insensitively as a fallback, which + // would let a miscased "JSONRPC" or "JsonRpc" key satisfy `json:"jsonrpc"` + // -- exactly the kind of malformed envelope this field exists to catch + // (see #5950). Read it from a raw map instead, which is case-sensitive. + var raw map[string]json.RawMessage + if err := json.Unmarshal(body, &raw); err == nil { + _ = json.Unmarshal(raw["jsonrpc"], &out.JSONRPC) + } out.ID = decodeID(w.ID) out.Result = w.Result if w.Error != nil { diff --git a/test/e2e/mcp_raw_client_test.go b/test/e2e/mcp_raw_client_test.go index 3f5bba9cb7..9f33682f6c 100644 --- a/test/e2e/mcp_raw_client_test.go +++ b/test/e2e/mcp_raw_client_test.go @@ -247,6 +247,21 @@ func TestRawClientSend(t *testing.T) { require.Empty(t, resp.JSONRPC) }) + t.Run("a miscased jsonrpc key does not satisfy the version-tag check", func(t *testing.T) { + t.Parallel() + // encoding/json struct tags match case-insensitively as a fallback, so + // a naive `json:"jsonrpc"` unmarshal would let this non-conformant key + // through. resp.JSONRPC must only ever reflect the exact lowercase key. + server, _ := newCapturingServer(t, `{"JSONRPC":"2.0","id":1,"result":{}}`) + + req, err := NewLegacyRequest("ping", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Empty(t, resp.JSONRPC) + }) + t.Run("parses error.code, error.data and a large-int64 id without precision loss", func(t *testing.T) { t.Parallel() const bigID = "9223372036854775807"