Skip to content

Commit 402cb56

Browse files
authored
Merge pull request #585 from koic/refuse_listen_requests_that_cannot_be_encoded
Refuse `subscriptions/listen` requests that cannot be encoded
2 parents 73d6da3 + 739dd55 commit 402cb56

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)