From 81a811dace0ead4bad5963bfb552a59047c13ad0 Mon Sep 17 00:00:00 2001 From: Totopo27 <35828647+Totopo27@users.noreply.github.com> Date: Wed, 14 Oct 2026 08:05:23 -0600 Subject: [PATCH] fix(mcp): return JSON-RPC error response with id null on rejected request IDs --- internal/jsonrpc2/messages.go | 8 +- mcp/rejected_request_id_test.go | 144 ++++++++++++++++++++++++++++++++ mcp/streamable.go | 7 ++ mcp/transport.go | 96 +++++++++++---------- 4 files changed, 211 insertions(+), 44 deletions(-) create mode 100644 mcp/rejected_request_id_test.go diff --git a/internal/jsonrpc2/messages.go b/internal/jsonrpc2/messages.go index 8b9677061..26a70dbd3 100644 --- a/internal/jsonrpc2/messages.go +++ b/internal/jsonrpc2/messages.go @@ -36,7 +36,7 @@ func MakeID(v any) (ID, error) { case string: return StringID(v), nil } - return ID{}, fmt.Errorf("%w: invalid ID type %T", ErrParse, v) + return ID{}, fmt.Errorf("%w: invalid ID type %T", ErrInvalidRequest, v) } // Message is the interface to all jsonrpc2 message types. @@ -121,7 +121,11 @@ func NewResponse(id ID, result any, rerr error) (*Response, error) { } func (msg *Response) marshal(to *wireCombined) { - to.ID = msg.ID.value + if msg.ID.value == nil { + to.ID = json.RawMessage("null") + } else { + to.ID = msg.ID.value + } to.Error = toWireError(msg.Error) to.Result = msg.Result } diff --git a/mcp/rejected_request_id_test.go b/mcp/rejected_request_id_test.go new file mode 100644 index 000000000..afbf77cb3 --- /dev/null +++ b/mcp/rejected_request_id_test.go @@ -0,0 +1,144 @@ +// Copyright 2026 The Go MCP SDK Authors. All rights reserved. +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file. + +package mcp + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2" + "github.com/modelcontextprotocol/go-sdk/jsonrpc" +) + +func TestRejectedRequestID_StreamableHTTP(t *testing.T) { + for _, invalidID := range []string{"true", "false", "{}", "[]"} { + t.Run("id_"+invalidID, func(t *testing.T) { + server := NewServer(&Implementation{Name: "test-server", Version: "1.0.0"}, nil) + handler := NewStreamableHTTPHandler(func(*http.Request) *Server { return server }, nil) + + body := strings.NewReader(`{"jsonrpc":"2.0","id":` + invalidID + `,"method":"ping"}`) + req := httptest.NewRequest(http.MethodPost, "/", body) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected status %d, got %d (body: %s)", http.StatusBadRequest, rec.Code, rec.Body.String()) + } + contentType := rec.Header().Get("Content-Type") + if !strings.HasPrefix(contentType, "application/json") { + t.Fatalf("expected application/json Content-Type, got %q (body: %s)", contentType, rec.Body.String()) + } + + var resp struct { + JSONRPC string `json:"jsonrpc"` + ID any `json:"id"` + Error *struct { + Code int64 `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("failed to decode JSON response: %v; body = %s", err, rec.Body.String()) + } + + if resp.JSONRPC != "2.0" { + t.Errorf("expected jsonrpc 2.0, got %q", resp.JSONRPC) + } + if resp.ID != nil { + t.Errorf("expected id null, got %v", resp.ID) + } + if resp.Error == nil || resp.Error.Code != jsonrpc.CodeInvalidRequest { + t.Errorf("expected error code %d, got %v", jsonrpc.CodeInvalidRequest, resp.Error) + } + }) + } +} + +func TestRejectedRequestID_IOTransport(t *testing.T) { + rClient, wServer := io.Pipe() + rServer, wClient := io.Pipe() + + serverConn := newIOConn(rwc{rc: rClient, wc: wClient}) + defer serverConn.Close() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + const invalidCount = 50 + go func() { + defer wServer.Close() + for i := 0; i < invalidCount; i++ { + var payload string + switch i % 3 { + case 0: + payload = "{\"jsonrpc\":\"2.0\",\"id\":true,\"method\":\"ping\"}\n" + case 1: + payload = "{\"jsonrpc\":\"2.0\",\"id\":{},\"method\":\"ping\"}\n" + case 2: + payload = "{\"jsonrpc\":\"2.0\",\"id\":[],\"method\":\"ping\"}\n" + } + _, _ = wServer.Write([]byte(payload)) + } + _, _ = wServer.Write([]byte("{\"jsonrpc\":\"2.0\",\"id\":7,\"method\":\"ping\"}\n")) + }() + + decoder := json.NewDecoder(rServer) + + readMsgCh := make(chan jsonrpc.Message, 1) + readErrCh := make(chan error, 1) + go func() { + msg, err := serverConn.Read(ctx) + if err != nil { + readErrCh <- err + return + } + readMsgCh <- msg + }() + + for i := 0; i < invalidCount; i++ { + var errResp struct { + JSONRPC string `json:"jsonrpc"` + ID any `json:"id"` + Error *struct { + Code int64 `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + if err := decoder.Decode(&errResp); err != nil { + t.Fatalf("[%d] failed to decode error response: %v", i, err) + } + if errResp.ID != nil { + t.Errorf("[%d] expected id null, got %v", i, errResp.ID) + } + if errResp.Error == nil || errResp.Error.Code != jsonrpc.CodeInvalidRequest { + t.Errorf("[%d] expected error code %d, got %v", i, jsonrpc.CodeInvalidRequest, errResp.Error) + } + } + + // Verify that serverConn.Read continued and returned the subsequent valid request + select { + case err := <-readErrCh: + t.Fatalf("serverConn.Read failed: %v", err) + case msg := <-readMsgCh: + req, ok := msg.(*jsonrpc.Request) + if !ok { + t.Fatalf("expected *jsonrpc.Request, got %T", msg) + } + if req.Method != "ping" { + t.Errorf("expected method ping, got %q", req.Method) + } + if req.ID != jsonrpc2.Int64ID(7) { + t.Errorf("expected id 7, got %v", req.ID) + } + } +} diff --git a/mcp/streamable.go b/mcp/streamable.go index a7ab22756..4d8618407 100644 --- a/mcp/streamable.go +++ b/mcp/streamable.go @@ -1619,6 +1619,13 @@ func (c *streamableServerConn) servePOST(w http.ResponseWriter, req *http.Reques // logic. incoming, isBatch, err := readBatch(body) if err != nil { + if !isBatch && errors.Is(err, jsonrpc2.ErrInvalidRequest) { + writeJSONRPCError(w, http.StatusBadRequest, jsonrpc.ID{}, &jsonrpc.Error{ + Code: jsonrpc.CodeInvalidRequest, + Message: err.Error(), + }) + return + } http.Error(w, fmt.Sprintf("malformed payload: %v", err), http.StatusBadRequest) return } diff --git a/mcp/transport.go b/mcp/transport.go index d2423d9dd..464e81f4d 100644 --- a/mcp/transport.go +++ b/mcp/transport.go @@ -692,59 +692,71 @@ func (t *ioConn) Read(ctx context.Context) (jsonrpc.Message, error) { return next, nil } - var raw json.RawMessage - select { - case <-ctx.Done(): - return nil, ctx.Err() + for { + var raw json.RawMessage + select { + case <-ctx.Done(): + return nil, ctx.Err() + + case v := <-t.incoming: + if v.err != nil { + return nil, v.err + } + raw = v.msg - case v := <-t.incoming: - if v.err != nil { - return nil, v.err + case <-t.closed: + return nil, io.EOF } - raw = v.msg - - case <-t.closed: - return nil, io.EOF - } - msgs, batch, err := readBatch(raw) - if err != nil { - return nil, err - } - var protocolVersion string - t.sessionMu.Lock() - protocolVersion = t.protocolVersion - t.sessionMu.Unlock() - if batch && protocolVersion >= protocolVersion20250618 { - return nil, fmt.Errorf("JSON-RPC batching is not supported in %s and later (request version: %s)", protocolVersion20250618, protocolVersion) - } + msgs, batch, err := readBatch(raw) + if err != nil { + if !batch && errors.Is(err, jsonrpc2.ErrInvalidRequest) { + errResp := &jsonrpc.Response{ + ID: jsonrpc.ID{}, + Error: jsonrpc2.ErrInvalidRequest, + } + if writeErr := t.Write(ctx, errResp); writeErr != nil { + return nil, writeErr + } + continue + } + return nil, err + } + var protocolVersion string + t.sessionMu.Lock() + protocolVersion = t.protocolVersion + t.sessionMu.Unlock() + if batch && protocolVersion >= protocolVersion20250618 { + return nil, fmt.Errorf("JSON-RPC batching is not supported in %s and later (request version: %s)", protocolVersion20250618, protocolVersion) + } - t.queue = msgs[1:] + t.queue = msgs[1:] - if batch { - var respBatch *msgBatch // track incoming requests in the batch - for _, msg := range msgs { - if req, ok := msg.(*jsonrpc.Request); ok && req.IsCall() { - if respBatch == nil { - respBatch = &msgBatch{ - unresolved: make(map[jsonrpc2.ID]int), + if batch { + var respBatch *msgBatch // track incoming requests in the batch + for _, msg := range msgs { + if req, ok := msg.(*jsonrpc.Request); ok && req.IsCall() { + if respBatch == nil { + respBatch = &msgBatch{ + unresolved: make(map[jsonrpc2.ID]int), + } } + if _, ok := respBatch.unresolved[req.ID]; ok { + return nil, fmt.Errorf("duplicate message ID %q", req.ID) + } + respBatch.unresolved[req.ID] = len(respBatch.responses) + respBatch.responses = append(respBatch.responses, nil) } - if _, ok := respBatch.unresolved[req.ID]; ok { - return nil, fmt.Errorf("duplicate message ID %q", req.ID) - } - respBatch.unresolved[req.ID] = len(respBatch.responses) - respBatch.responses = append(respBatch.responses, nil) } - } - if respBatch != nil { - // The batch contains one or more incoming requests to track. - if err := t.addBatch(respBatch); err != nil { - return nil, err + if respBatch != nil { + // The batch contains one or more incoming requests to track. + if err := t.addBatch(respBatch); err != nil { + return nil, err + } } } + return msgs[0], nil } - return msgs[0], err } // readBatch reads batch data, which may be either a single JSON-RPC message,