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..776ed15dca 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" 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() + + // 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() 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") + }) + } } 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"]) }) 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..886599fde4 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 @@ -381,13 +382,21 @@ type wireResponse struct { // 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 } + // 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 e0402ecfe7..9f33682f6c 100644 --- a/test/e2e/mcp_raw_client_test.go +++ b/test/e2e/mcp_raw_client_test.go @@ -226,6 +226,40 @@ 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("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) {