From 887599090981c5a86238c868c5a0b0e6e9386308 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 8 Jul 2026 13:10:40 +0900 Subject: [PATCH] Handle non-object JSON-RPC messages without raising ## Motivation and Context A JSON-RPC message is an object, but two stdio-facing paths assumed that and raised on a non-object value parsed from an incoming frame: - `JsonRpcHandler.handle` maps each element of a batch array through `process_request`, which reads `request[:id]` assuming a Hash. A non-object batch element (for example `[[]]` or `[5]`) makes `[][:id]` raise `TypeError`. `handle_json` rescued only `JSON::ParserError`, so the error escaped; over stdio it escaped the `StdioTransport#open` read loop. The HTTP transport already rejects array bodies before dispatch, so this surfaced only on stdio. - `MCP::Client::Stdio#read_response` parses each frame from the server and calls `parsed.key?("id")`, which raises `NoMethodError` when the server emits a non-object frame. Only `JSON::ParserError` was rescued, so one malformed line from the server escaped. Both are robustness gaps: a malformed frame should be reported or skipped, not raise an unhandled exception out of the transport. - `process_request` now returns an `INVALID_REQUEST` error response for a non-object request before touching `request[:id]`, so a malformed batch element becomes a normal per-element error. - `handle_json` gains a last-resort `StandardError` rescue that returns an `INTERNAL_ERROR` response, so an unexpected handling error still produces a JSON-RPC reply. - `read_response` skips a non-object frame the same way it skips a frame without an id. ## How Has This Been Tested? New tests in `test/json_rpc_handler_test.rb` cover non-object batch elements (`[[]]`, `[5]`, `["x"]`, `[nil]`, `[true]`), a valid request followed by one non-object element, and `handle_json("[[]]")` returning an error instead of raising. A test in `test/mcp/server/transports/stdio_transport_test.rb` drives `open` with a `[[]]` line and asserts it emits an error response without raising. A test in `test/mcp/client/stdio_test.rb` has the server emit non-object frames before the response and asserts the client skips them and still returns the response. ## Breaking Changes None. A well-formed request, batch, or response is handled exactly as before; only a non-object message now yields a JSON-RPC error on the server or is skipped by the client instead of raising. --- lib/json_rpc_handler.rb | 17 +++++ lib/mcp/client/stdio.rb | 4 +- test/json_rpc_handler_test.rb | 31 ++++++++++ test/mcp/client/stdio_test.rb | 62 +++++++++++++++++++ .../server/transports/stdio_transport_test.rb | 21 +++++++ 5 files changed, 134 insertions(+), 1 deletion(-) diff --git a/lib/json_rpc_handler.rb b/lib/json_rpc_handler.rb index 2c6c836a..9992ae7b 100644 --- a/lib/json_rpc_handler.rb +++ b/lib/json_rpc_handler.rb @@ -63,12 +63,29 @@ def handle_json(request_json, id_validation_pattern: DEFAULT_ALLOWED_ID_CHARACTE message: "Parse error", data: "Invalid JSON", }) + rescue StandardError + # Last-resort guard so an unexpected handling error returns a JSON-RPC error + # instead of escaping to the transport loop. + response = error_response(id: :unknown_id, id_validation_pattern: id_validation_pattern, error: { + code: ErrorCode::INTERNAL_ERROR, + message: "Internal error", + }) end response&.to_json end def process_request(request, id_validation_pattern:, &method_finder) + # A batch element can be any JSON value; a non-object element cannot carry an id or a method, + # so reject it before the `request[:id]` access below. + unless request.is_a?(Hash) + return error_response(id: :unknown_id, id_validation_pattern: id_validation_pattern, error: { + code: ErrorCode::INVALID_REQUEST, + message: "Invalid Request", + data: "Request must be an object", + }) + end + id = request[:id] error = if !valid_version?(request[:jsonrpc]) diff --git a/lib/mcp/client/stdio.rb b/lib/mcp/client/stdio.rb index e8359687..5bcf6517 100644 --- a/lib/mcp/client/stdio.rb +++ b/lib/mcp/client/stdio.rb @@ -259,7 +259,9 @@ def read_response(request) parsed = JSON.parse(line.strip) - next unless parsed.key?("id") + # A JSON-RPC message is an object; skip a non-object frame (array or scalar) + # the same way as a frame without an id. + next unless parsed.is_a?(Hash) && parsed.key?("id") return parsed if parsed["id"] == request_id end diff --git a/test/json_rpc_handler_test.rb b/test/json_rpc_handler_test.rb index 83a18df5..e823e42f 100644 --- a/test/json_rpc_handler_test.rb +++ b/test/json_rpc_handler_test.rb @@ -682,6 +682,37 @@ assert_equal 3, @response.first[:result] assert_equal(-32600, @response.last.dig(:error, :code)) end + + it "returns an invalid request error for a non-object batch element instead of raising" do + [[], [1], ["x"], [nil], [true]].each do |batch| + handle batch + + assert_equal(-32600, @response.dig(:error, :code)) + assert_equal "Invalid Request", @response.dig(:error, :message) + assert_nil @response[:id] + end + end + + it "handles a valid request followed by a non-object element without raising" do + register("add") { |params| params[:a] + params[:b] } + + handle [ + { jsonrpc: "2.0", id: 100, method: "add", params: { a: 1, b: 2 } }, + [], + ] + + assert @response.is_a?(Array) + assert_equal 3, @response.first[:result] + assert_equal(-32600, @response.last.dig(:error, :code)) + assert_nil @response.last[:id] + end + + it "does not raise from handle_json when a batch element is not an object" do + result = handle_json("[[]]") + + assert_equal(-32600, @response.dig(:error, :code)) + refute_nil result + end end # 7 Examples diff --git a/test/mcp/client/stdio_test.rb b/test/mcp/client/stdio_test.rb index 205fae43..1995222b 100644 --- a/test/mcp/client/stdio_test.rb +++ b/test/mcp/client/stdio_test.rb @@ -146,6 +146,68 @@ def test_send_request_skips_notifications stdout_write.close end + def test_send_request_skips_non_object_frames + stdin_read, stdin_write = IO.pipe + stdout_read, stdout_write = IO.pipe + stderr_read, _ = IO.pipe + + Open3.stubs(:popen3).returns([stdin_write, stdout_read, stderr_read, mock_wait_thread]) + + transport = Stdio.new(command: "ruby", args: ["server.rb"]) + + request = { + jsonrpc: "2.0", + id: "test-id", + method: "tools/list", + } + + server_thread = Thread.new do + init_line = stdin_read.gets + init_request = JSON.parse(init_line) + stdout_write.puts(JSON.generate({ + jsonrpc: "2.0", + id: init_request["id"], + result: { + protocolVersion: "2025-11-25", + capabilities: {}, + serverInfo: { name: "test-server", version: "1.0.0" }, + }, + })) + stdout_write.flush + + # Read initialized notification + stdin_read.gets + + # Read tools/list request + stdin_read.gets + + # Non-object frames are skipped like any other non-matching frame. + stdout_write.puts("[[]]") + stdout_write.puts("null") + stdout_write.flush + + # Then send the actual response + stdout_write.puts(JSON.generate({ + jsonrpc: "2.0", + id: "test-id", + result: { tools: [] }, + })) + stdout_write.flush + end + + transport.connect + response = transport.send_request(request: request) + + assert_equal("test-id", response["id"]) + assert_empty(response.dig("result", "tools")) + ensure + server_thread.join + stdin_read.close + stdin_write.close + stdout_read.close + stdout_write.close + end + def test_send_request_raises_error_when_process_exits stdin_read, stdin_write = IO.pipe stdout_read, stdout_write = IO.pipe diff --git a/test/mcp/server/transports/stdio_transport_test.rb b/test/mcp/server/transports/stdio_transport_test.rb index c9cda78f..f8d81a02 100644 --- a/test/mcp/server/transports/stdio_transport_test.rb +++ b/test/mcp/server/transports/stdio_transport_test.rb @@ -133,6 +133,27 @@ class StdioTransportTest < ActiveSupport::TestCase end end + test "open handles a malformed JSON-RPC batch element and emits an error response" do + input = StringIO.new("[[]]\n") + output = StringIO.new + original_stdin = $stdin + original_stdout = $stdout + + begin + $stdin = input + $stdout = output + @transport.open + + response = JSON.parse(output.string, symbolize_names: true) + assert_nil(response[:id]) + assert_equal(-32600, response[:error][:code]) + assert_equal("Invalid Request", response[:error][:message]) + ensure + $stdin = original_stdin + $stdout = original_stdout + end + end + test "rejects duplicate initialize on the same stdio session with -32600" do first = { jsonrpc: "2.0",