Skip to content

Commit 739dd55

Browse files
committed
Refuse subscriptions/listen requests that cannot be encoded
## Motivation and Context A `subscriptions/listen` request whose id, or one of whose honored resource URIs, is a string holding bytes that are not valid UTF-8 parses, since the JSON parser accepts such bytes, but cannot be written back as JSON. The transport answered 200, registered the stream, and then failed to encode the acknowledgement that echoes both; `JSON::GeneratorError` is not among the write errors the stream handles, so the registered entry was never removed and kept one of the `max_listen_subscriptions` slots until the transport closed. The acknowledgement is now encoded before the stream is committed to. An id that cannot be encoded is refused with HTTP 400 and JSON-RPC `-32600`, the error carrying a null id since the id itself cannot be written; an honored filter that cannot be encoded is refused with HTTP 400 and `-32602`. Neither registers a stream. ## How Has This Been Tested? New tests in `test/mcp/server/transports/streamable_http_transport_test.rb` send a listen request whose id and one whose resource URI hold an invalid byte, and check the 400 and that no stream is registered. Against the previous library both answer 200. Two more send the invalid URI to a server without the `subscribe` capability, which acknowledges without it, and to a transport at its stream cap, which answers 503 first. ## Breaking Changes None.
1 parent 14dbb20 commit 739dd55

2 files changed

Lines changed: 120 additions & 11 deletions

File tree

‎lib/mcp/server/transports/streamable_http_transport.rb‎

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -904,6 +904,14 @@ def handle_subscriptions_listen(body)
904904
return invalid_request_response("Invalid Request: subscriptions/listen requires an id")
905905
end
906906

907+
# The id is echoed in every message of the stream, so one that cannot be written back as JSON
908+
# (a String holding bytes that are not valid UTF-8 parses, but does not generate) is refused here,
909+
# while the refusal can still answer with a null id, rather than surfacing as a failed write after
910+
# the stream has been registered.
911+
unless json_encodable?(request_id)
912+
return invalid_request_response("Invalid Request: subscriptions/listen id must be valid UTF-8")
913+
end
914+
907915
begin
908916
if RequestEnvelope.modern?(params)
909917
RequestEnvelope.parse!(params, request: params)
@@ -946,7 +954,43 @@ def handle_subscriptions_listen(body)
946954
return too_many_listen_subscriptions_response(request_id)
947955
end
948956

949-
[200, SSE_HEADERS.dup, listen_sse_body(request_id, honored_filter(filter))]
957+
honored = honored_filter(filter)
958+
959+
# The acknowledgement echoes the honored filter, whose resource URIs are client-supplied strings,
960+
# so it is encoded before the stream is committed to: a URI that cannot be written as JSON is
961+
# refused with a 400 instead of failing the first write after the stream was registered.
962+
unless (acknowledgement = listen_acknowledgement_json(request_id, honored))
963+
return json_rpc_error_response(
964+
status: 400,
965+
code: JsonRpcHandler::ErrorCode::INVALID_PARAMS,
966+
message: "Invalid params: subscriptions/listen `notifications` must be valid UTF-8",
967+
id: request_id,
968+
)
969+
end
970+
971+
[200, SSE_HEADERS.dup, listen_sse_body(request_id, honored, acknowledgement)]
972+
end
973+
974+
# The `notifications/subscriptions/acknowledged` frame for a listen stream as JSON, or `nil` when
975+
# the request id or the honored filter holds a string JSON cannot encode.
976+
def listen_acknowledgement_json(request_id, honored)
977+
{
978+
jsonrpc: "2.0",
979+
method: Methods::NOTIFICATIONS_SUBSCRIPTIONS_ACKNOWLEDGED,
980+
params: {
981+
notifications: honored,
982+
_meta: { RequestEnvelope::SUBSCRIPTION_ID_META_KEY.to_sym => request_id },
983+
},
984+
}.to_json
985+
rescue JSON::GeneratorError
986+
nil
987+
end
988+
989+
def json_encodable?(value)
990+
value.to_json
991+
true
992+
rescue JSON::GeneratorError
993+
false
950994
end
951995

952996
def listen_subscriptions_full?
@@ -1005,7 +1049,7 @@ def first
10051049
# The entry is keyed by an identifier minted here, not by the request id: that id is unique only among
10061050
# the requesting client's own in-flight requests, and two clients that pick the same one must each get
10071051
# their stream, stamped with the id they sent.
1008-
def listen_sse_body(request_id, honored)
1052+
def listen_sse_body(request_id, honored, acknowledgement)
10091053
ListenStreamBody.new do |stream|
10101054
subscription_key = SecureRandom.uuid
10111055
subscription = nil
@@ -1029,15 +1073,6 @@ def listen_sse_body(request_id, honored)
10291073
if subscription.nil?
10301074
close_stream_safely(stream)
10311075
else
1032-
acknowledgement = {
1033-
jsonrpc: "2.0",
1034-
method: Methods::NOTIFICATIONS_SUBSCRIPTIONS_ACKNOWLEDGED,
1035-
params: {
1036-
notifications: honored,
1037-
_meta: { RequestEnvelope::SUBSCRIPTION_ID_META_KEY.to_sym => request_id },
1038-
},
1039-
}
1040-
10411076
begin
10421077
acknowledged = subscription[:write_mutex].synchronize do
10431078
next false if subscription[:closed]

‎test/mcp/server/transports/streamable_http_transport_test.rb‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6564,6 +6564,80 @@ def string
65646564
refute(events.any? { |event| event["method"] == "notifications/tools/list_changed" })
65656565
end
65666566

6567+
test "subscriptions/listen refuses an id that is not valid UTF-8 without registering a stream" do
6568+
body = modern_listen_body(id: "listen-BAD", params: { notifications: { toolsListChanged: true } }).b.sub("BAD", "\xFF".b)
6569+
6570+
response = @transport.handle_request(modern_rack_request(body))
6571+
6572+
assert_equal 400, response[0]
6573+
6574+
parsed = JSON.parse(response[2][0])
6575+
6576+
# The member is present and null, as JSON-RPC 2.0 requires when the id cannot be determined.
6577+
assert parsed.key?("id")
6578+
assert_nil parsed["id"]
6579+
assert_equal JsonRpcHandler::ErrorCode::INVALID_REQUEST, parsed.dig("error", "code")
6580+
assert_empty @transport.instance_variable_get(:@listen_subscriptions)
6581+
end
6582+
6583+
test "subscriptions/listen refuses a resource URI that is not valid UTF-8 without registering a stream" do
6584+
server = Server.new(name: "listen_test", capabilities: { resources: { subscribe: true } })
6585+
transport = StreamableHTTPTransport.new(server, listen_keepalive_interval: nil)
6586+
body = modern_listen_body(
6587+
id: "listen-1",
6588+
params: { notifications: { resourceSubscriptions: ["file:///BAD"] } }
6589+
).b.sub("BAD", "\xFF".b)
6590+
6591+
response = transport.handle_request(modern_rack_request(body))
6592+
6593+
assert_equal 400, response[0]
6594+
6595+
parsed = JSON.parse(response[2][0])
6596+
6597+
assert_equal "listen-1", parsed["id"]
6598+
assert_equal(-32602, parsed.dig("error", "code"))
6599+
assert_empty transport.instance_variable_get(:@listen_subscriptions)
6600+
ensure
6601+
transport.close
6602+
end
6603+
6604+
test "subscriptions/listen acknowledges without a resource URI that is not valid UTF-8 when subscribe is not declared" do
6605+
# Without the `subscribe` capability the URIs are not honored, so the acknowledgement never echoes
6606+
# the unencodable one and the request is served.
6607+
body = modern_listen_body(
6608+
id: "listen-1",
6609+
params: { notifications: { toolsListChanged: true, resourceSubscriptions: ["file:///BAD"] } },
6610+
).b.sub("BAD", "\xFF".b)
6611+
6612+
response = @transport.handle_request(modern_rack_request(body))
6613+
6614+
assert_equal 200, response[0]
6615+
6616+
io = StringIO.new
6617+
response[2].call(io)
6618+
acknowledgement = sse_events(io).first
6619+
6620+
assert_equal "notifications/subscriptions/acknowledged", acknowledgement["method"]
6621+
refute acknowledgement.dig("params", "notifications").key?("resourceSubscriptions")
6622+
end
6623+
6624+
test "subscriptions/listen refuses a request past the stream cap before checking its resource URIs" do
6625+
server = Server.new(name: "listen_test", capabilities: { resources: { subscribe: true } })
6626+
transport = StreamableHTTPTransport.new(server, max_listen_subscriptions: 1, listen_keepalive_interval: nil)
6627+
open_listen_stream(id: "listen-1", notifications: { toolsListChanged: true }, transport: transport)
6628+
body = modern_listen_body(
6629+
id: "listen-2",
6630+
params: { notifications: { resourceSubscriptions: ["file:///BAD"] } }
6631+
).b.sub("BAD", "\xFF".b)
6632+
6633+
response = transport.handle_request(modern_rack_request(body))
6634+
6635+
assert_equal 503, response[0]
6636+
assert_equal "listen-2", JSON.parse(response[2][0])["id"]
6637+
ensure
6638+
transport.close
6639+
end
6640+
65676641
test "transport close during the acknowledgement write still sends the acknowledgement first" do
65686642
# The stream blocks its first write, the acknowledgement, until released, so the close arrives while
65696643
# that write is in progress and has to queue behind it on the stream's write mutex.

0 commit comments

Comments
 (0)