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.
This commit is contained in:
Koichi ITO
parent
61233ed43b
commit
8875990909
5 changed files with 134 additions and 1 deletions
|
|
@ -63,12 +63,29 @@ module JsonRpcHandler
|
|||
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])
|
||||
|
|
|
|||
|
|
@ -259,7 +259,9 @@ module MCP
|
|||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -682,6 +682,37 @@ describe JsonRpcHandler do
|
|||
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
|
||||
|
|
|
|||
|
|
@ -146,6 +146,68 @@ module MCP
|
|||
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
|
||||
|
|
|
|||
|
|
@ -133,6 +133,27 @@ module MCP
|
|||
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",
|
||||
|
|
|
|||
Loading…
Reference in a new issue