Merge pull request #672 from koic/refuse_ids_and_method_names_that_cannot_be_encoded

Refuse ids and method names that cannot be encoded

Author: koic

lib/json_rpc_handler.rb

@@ -227,7 +227,7 @@ def valid_id?(id, pattern = nil)
   end
 
   def valid_method_name?(method)
-    method.is_a?(String) && !method.start_with?("rpc.")
+    method.is_a?(String) && method.valid_encoding? && !method.start_with?("rpc.")
   end
 
   def valid_params?(params)

lib/mcp/server/transports/streamable_http_transport.rb

@@ -921,7 +921,7 @@ def handle_modern(request, header_version, body_string: nil)
                 supported: MCP::Configuration::SUPPORTED_MODERN_PROTOCOL_VERSIONS,
                 requested: printable_value(header_version),
               },
-              id: body[:id],
+              id: echoable_id(body[:id]),
             )
           end
 
@@ -930,7 +930,7 @@ def handle_modern(request, header_version, body_string: nil)
               status: 400,
               code: JsonRpcHandler::ErrorCode::INVALID_REQUEST,
               message: "Bad Request: Mcp-Session-Id is not accepted in the modern lifecycle",
-              id: body[:id],
+              id: echoable_id(body[:id]),
             )
           end
 
@@ -1189,6 +1189,14 @@ def json_encodable?(value)
           false
         end
 
+        # The id a refusal built here can echo: an Integer, or a String that is valid UTF-8. A String holding other
+        # bytes would make the body's `to_json` raise, so the refusal carries no id, as for an id it could not read.
+        def echoable_id(id)
+          return id if id.is_a?(Integer) || (id.is_a?(String) && id.valid_encoding?)
+
+          nil
+        end
+
         def listen_subscriptions_full?
           return false unless @max_listen_subscriptions
 
@@ -1639,7 +1647,7 @@ def header_mismatch_response(message, id)
             status: 400,
             code: ErrorCodes::HEADER_MISMATCH,
             message: "Header mismatch: #{message}",
-            id: id,
+            id: echoable_id(id),
           )
         end
 
@@ -1716,7 +1724,7 @@ def handle_post(request, body_string: nil)
             if header_version && MCP::Configuration.modern_protocol_version?(header_version)
               return invalid_request_response(
                 "Invalid Request: the session already negotiated the legacy lifecycle via `initialize`",
-                request_id: body[:id],
+                request_id: echoable_id(body[:id]),
               )
             end
 
@@ -1762,7 +1770,7 @@ def handle_post(request, body_string: nil)
                 session = @mutex.synchronize { @sessions[session_id] }
                 return session_not_found_response unless session && session_principal_matches?(session, request)
 
-                return already_initialized_response(body[:id])
+                return already_initialized_response(echoable_id(body[:id]))
               end
 
               return session_not_found_response
@@ -2029,7 +2037,7 @@ def initialize_too_large_response(request_id, reason)
             status: 413,
             code: JsonRpcHandler::ErrorCode::INVALID_REQUEST,
             message: "Payload too large: initialize request #{reason}",
-            id: request_id,
+            id: echoable_id(request_id),
           )
         end
 

test/json_rpc_handler_test.rb

@@ -73,6 +73,21 @@
       assert_equal 1, @response[:id]
     end
 
+    it "returns an error and does not call the method when the method name is not valid UTF-8" do
+      # Such a name reached dispatch: unknown, its Method not found error could not be written as JSON, and registered, it ran.
+      called = false
+      register("no\xFFpe".dup.force_encoding(Encoding::UTF_8)) { called = true }
+
+      handle_json "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"no\xFFpe\"}".b
+
+      assert_rpc_error expected_error: {
+        code: -32600,
+        message: "Invalid Request",
+        data: 'Method name must be a string and not start with "rpc."',
+      }
+      refute called
+    end
+
     it "returns an error when method begins with 'rpc.'" do
       handle jsonrpc: "2.0", id: 1, method: "rpc.add", params: { a: 1, b: 2 }
 

test/mcp/server/transports/streamable_http_transport_test.rb

@@ -398,6 +398,63 @@ def string
           assert_equal 200, ping_response[0]
         end
 
+        test "a duplicate initialize whose id is not valid UTF-8 is refused with no id" do
+          reported = []
+          original_reporter = MCP.configuration.exception_reporter
+          MCP.configuration.exception_reporter = ->(error, _context) { reported << error }
+          init_request = create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json" },
+            { jsonrpc: "2.0", method: "initialize", id: "first", params: initialize_params }.to_json,
+          )
+          session_id = @transport.handle_request(init_request)[1]["mcp-session-id"]
+          duplicate = { jsonrpc: "2.0", method: "initialize", id: "second", params: initialize_params }.to_json
+            .b.sub('"second"', "\"a\xFF\"".b)
+
+          response = @transport.handle_request(create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json", "HTTP_MCP_SESSION_ID" => session_id },
+            duplicate,
+          ))
+
+          assert_equal 400, response[0]
+          parsed = JSON.parse(response[2][0])
+          refute parsed.key?("id")
+          assert_equal JsonRpcHandler::ErrorCode::INVALID_REQUEST, parsed["error"]["code"]
+          assert_empty reported
+        ensure
+          MCP.configuration.exception_reporter = original_reporter
+        end
+
+        test "a request whose method name is not valid UTF-8 is refused with -32600 on the legacy path" do
+          # Dispatched as sent, the name reached the Method not found error, whose `data` could not be written
+          # as JSON, so the request was answered `-32603` Internal error instead of refused.
+          init_request = create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json" },
+            { jsonrpc: "2.0", method: "initialize", id: "first", params: initialize_params }.to_json,
+          )
+          session_id = @transport.handle_request(init_request)[1]["mcp-session-id"]
+          body = { jsonrpc: "2.0", method: "PLACEHOLDER", id: "m1" }.to_json.b.sub("PLACEHOLDER", "no\xFFpe".b)
+
+          response = @transport.handle_request(create_rack_request(
+            "POST",
+            "/",
+            { "CONTENT_TYPE" => "application/json", "HTTP_MCP_SESSION_ID" => session_id },
+            body,
+          ))
+
+          assert_equal 200, response[0]
+          io = StringIO.new
+          response[2].call(io)
+          parsed = JSON.parse(io.string.match(/^data: (.+)$/)[1])
+          assert_equal "m1", parsed["id"]
+          assert_equal JsonRpcHandler::ErrorCode::INVALID_REQUEST, parsed["error"]["code"]
+        end
+
         test "rejects initialize with stale Mcp-Session-Id with 404" do
           request = create_rack_request(
             "POST",
@@ -6431,6 +6488,49 @@ def string
           assert_equal(-32020, JSON.parse(response[2][0]).dig("error", "code"))
         end
 
+        test "modern POST refusals carry no id when the request id is not valid UTF-8" do
+          # The refusal is built before the id is validated; one whose bytes could not be encoded made `to_json`
+          # raise and the rescue answer 500 with an exception report.
+          reported = []
+          original_reporter = MCP.configuration.exception_reporter
+          MCP.configuration.exception_reporter = ->(error, _context) { reported << error }
+          unsupported = modern_body("ping", {}, version: "2027-01-01").b.sub('"id":1', "\"id\":\"a\xFF\"".b)
+          mismatched = modern_body("ping", {}).b.sub('"id":1', "\"id\":\"a\xFF\"".b)
+
+          by_version = @transport.handle_request(modern_rack_request(unsupported, version: "2027-01-01"))
+          by_header = @transport.handle_request(modern_rack_request(mismatched, headers: { "HTTP_MCP_METHOD" => "tools/call" }))
+
+          [[by_version, -32022], [by_header, -32020]].each do |response, code|
+            assert_equal 400, response[0]
+            parsed = JSON.parse(response[2][0])
+            assert_equal code, parsed.dig("error", "code")
+            refute parsed.key?("id")
+          end
+          assert_empty reported
+        ensure
+          MCP.configuration.exception_reporter = original_reporter
+        end
+
+        test "modern POST refusals keep an integer or string id and drop any other" do
+          kept = ['""', "123456789012345678901234567890"]
+          dropped = ["true", "1.5", "{}"]
+
+          (kept + dropped).each do |id|
+            body = modern_body("ping", {}, version: "2027-01-01").sub('"id":1', "\"id\":#{id}")
+
+            response = @transport.handle_request(modern_rack_request(body, version: "2027-01-01"))
+
+            assert_equal 400, response[0], id
+            parsed = JSON.parse(response[2][0])
+            assert_equal(-32022, parsed.dig("error", "code"), id)
+            if kept.include?(id)
+              assert_equal JSON.parse(id), parsed["id"], id
+            else
+              refute parsed.key?("id"), id
+            end
+          end
+        end
+
         test "modern POST without the Mcp-Method header returns 400 with -32020" do
           # The header is required on every modern POST; a missing required standard header is
           # a validation failure, matching the TypeScript SDK's enforcement.