diff --git a/.claude/rules/go-style.md b/.claude/rules/go-style.md index 886aa651c2..8b86ac20fa 100644 --- a/.claude/rules/go-style.md +++ b/.claude/rules/go-style.md @@ -200,3 +200,18 @@ Before implementing any non-trivial functionality from scratch: 3. **Look for existing Go packages** — search for well-maintained OSS libraries that solve the problem before writing custom implementations. Implementing from scratch should be a last resort, justified by a specific gap no existing solution fills. + +## JSON-RPC Envelopes + +Never `json.Marshal` (or `json.NewEncoder().Encode`) a `jsonrpc2` type. +`jsonrpc2.Response` carries no json tags and `jsonrpc2.ID`'s only field is +unexported, so reflection emits Go-capitalized keys, drops the mandatory +`"jsonrpc":"2.0"` tag, and renders every id — valid or not — as `{}` (#5950). +Serialize through `jsonrpc2.EncodeMessage`; for HTTP error responses use +`pkg/mcp.EncodeJSONRPCError` / `pkg/mcp.WriteJSONRPCError`, which add +encode-before-header ordering and a conformant fallback body. + +When an envelope must be hand-built as a map (only where no `jsonrpc2.ID` is +in hand), omit the `"id"` key when the id is absent — never emit `"id":null`. +MCP's `RequestId` is `string | number`, so null is not representable, and the +reference TypeScript SDK client throws on it inside its transport (#6038). diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index 3d9ccb4d55..2f20e0c021 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -158,21 +158,10 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) { Error: jsonrpc2.NewError(mcp.JSONRPCCodeDenied, "Unauthorized"), } - // 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":"Unauthorized"}}`, mcp.JSONRPCCodeDenied) - } - - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusForbidden) - //nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error - _, _ = w.Write(body) + // The helper encodes 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). Nothing to do on a write error for a denial body. + _ = mcp.WriteJSONRPCError(w, http.StatusForbidden, errorResponse) } // Middleware creates an HTTP middleware that authorizes MCP requests. diff --git a/pkg/mcp/jsonrpc_write.go b/pkg/mcp/jsonrpc_write.go new file mode 100644 index 0000000000..fce87708a3 --- /dev/null +++ b/pkg/mcp/jsonrpc_write.go @@ -0,0 +1,53 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "log/slog" + "net/http" + + "golang.org/x/exp/jsonrpc2" +) + +// EncodeJSONRPCError returns the wire encoding of a jsonrpc2 error response. +// +// This is the single sanctioned way to serialize a jsonrpc2 envelope in this +// codebase. NEVER json.Marshal (or json.NewEncoder().Encode) a jsonrpc2 type: +// jsonrpc2.Response carries no json tags and jsonrpc2.ID's only field is +// unexported, so reflection produces Go-capitalized keys, drops the mandatory +// "jsonrpc":"2.0" tag, and renders every id — valid or not — as "{}" (#5950). +// jsonrpc2.EncodeMessage marshals through the library's tagged wire struct: +// lowercase keys, the version tag stamped unconditionally, and an id that is +// omitted when absent (never null, which MCP cannot express and the reference +// TypeScript SDK client rejects, #6038) while a legitimate id of 0 survives. +// +// EncodeMessage cannot fail for a Response built from an ID and a +// jsonrpc2.NewError; if it somehow does, this logs and falls back to a +// hardcoded, conformant Internal Error body rather than returning nothing — +// the original code and id are not trustworthy at that point. +func EncodeJSONRPCError(resp *jsonrpc2.Response) []byte { + body, err := jsonrpc2.EncodeMessage(resp) + if err != nil { + slog.Error("failed to encode JSON-RPC error response", "error", err) + return []byte(`{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"}}`) + } + return body +} + +// WriteJSONRPCError writes resp as an HTTP response: the JSON-RPC error body +// under Content-Type application/json with the given HTTP status. The body is +// encoded before any header is written, so an encode failure never leaves a +// half-written response (see EncodeJSONRPCError for the fallback). Returns +// the body write error; callers with no recovery path may discard it. +// +// Do not use this to write into an SSE stream — a bare JSON object in a +// text/event-stream parses as an unrecognized field and is silently +// discarded (#6037); SSE paths must frame the body as an event instead. +func WriteJSONRPCError(w http.ResponseWriter, httpStatus int, resp *jsonrpc2.Response) error { + body := EncodeJSONRPCError(resp) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(httpStatus) + _, err := w.Write(body) + return err +} diff --git a/pkg/mcp/jsonrpc_write_test.go b/pkg/mcp/jsonrpc_write_test.go new file mode 100644 index 0000000000..338cb24de0 --- /dev/null +++ b/pkg/mcp/jsonrpc_write_test.go @@ -0,0 +1,88 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package mcp + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/jsonrpc2" +) + +// TestEncodeJSONRPCError pins the wire properties the helper exists to +// guarantee: lowercase keys with the version tag stamped (the #5950 bug was +// reflection over the untagged jsonrpc2.Response), an absent id omitted +// rather than rendered null (#6038), and a legitimate id of 0 surviving — +// the omitempty on the library's wire struct tests IsNil, not zero-ness. +func TestEncodeJSONRPCError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + id jsonrpc2.ID + want string + }{ + { + name: "int64 id", + id: jsonrpc2.Int64ID(42), + want: `{"jsonrpc":"2.0","id":42,"error":{"code":403,"message":"denied"}}`, + }, + { + name: "string id", + id: jsonrpc2.StringID("abc"), + want: `{"jsonrpc":"2.0","id":"abc","error":{"code":403,"message":"denied"}}`, + }, + { + name: "zero id survives", + id: jsonrpc2.Int64ID(0), + want: `{"jsonrpc":"2.0","id":0,"error":{"code":403,"message":"denied"}}`, + }, + { + name: "empty id omits the key", + id: jsonrpc2.ID{}, + want: `{"jsonrpc":"2.0","error":{"code":403,"message":"denied"}}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + body := EncodeJSONRPCError(&jsonrpc2.Response{ + ID: tt.id, + Error: jsonrpc2.NewError(JSONRPCCodeDenied, "denied"), + }) + + // assert.JSONEq is key-case-sensitive, so this single assertion + // catches "Error"/"ID" substituted for "error"/"id" and a missing + // "jsonrpc" tag; key presence is asserted separately because a + // decoded nil is indistinguishable from a decoded null. + assert.JSONEq(t, tt.want, string(body)) + + var decoded map[string]json.RawMessage + require.NoError(t, json.Unmarshal(body, &decoded)) + _, hasID := decoded["id"] + assert.Equal(t, tt.id != jsonrpc2.ID{}, hasID, "id key presence mismatch") + }) + } +} + +func TestWriteJSONRPCError(t *testing.T) { + t.Parallel() + + rr := httptest.NewRecorder() + err := WriteJSONRPCError(rr, http.StatusForbidden, &jsonrpc2.Response{ + ID: jsonrpc2.Int64ID(7), + Error: jsonrpc2.NewError(JSONRPCCodeDenied, "denied"), + }) + require.NoError(t, err) + + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.Equal(t, "application/json", rr.Header().Get("Content-Type")) + assert.JSONEq(t, `{"jsonrpc":"2.0","id":7,"error":{"code":403,"message":"denied"}}`, rr.Body.String()) +} diff --git a/pkg/webhook/mutating/middleware.go b/pkg/webhook/mutating/middleware.go index ed6dfa183d..05293728af 100644 --- a/pkg/webhook/mutating/middleware.go +++ b/pkg/webhook/mutating/middleware.go @@ -328,19 +328,8 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms Error: jsonrpc2.NewError(code, message), } - // 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"}}`, code) - } - - 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) + // The helper encodes before writing any header, so an encode failure never + // leaves a half-written response (e.g. a status header followed by a + // second write). Nothing to do on a write error for a denial body. + _ = mcp.WriteJSONRPCError(w, statusCode, errResp) } diff --git a/pkg/webhook/validating/middleware.go b/pkg/webhook/validating/middleware.go index 9083655d08..e236943a07 100644 --- a/pkg/webhook/validating/middleware.go +++ b/pkg/webhook/validating/middleware.go @@ -194,19 +194,8 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms Error: jsonrpc2.NewError(code, message), } - // 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"}}`, code) - } - - 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) + // The helper encodes before writing any header, so an encode failure never + // leaves a half-written response (e.g. a status header followed by a + // second write). Nothing to do on a write error for a denial body. + _ = mcp.WriteJSONRPCError(w, statusCode, errResp) }