fix: stop leaking exception messages to clients via JSON-RPC error data
Tool handler exceptions and anything reaching JsonRpcHandler's blind rescue had their #message echoed into the error data member, exposing internals to untrusted clients (CWE-209). Capability errors keep their client-facing message by re-raising through RequestHandlerError, the one channel whose message is deliberately surfaced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
d51d4a0f91
commit
f379e13b47
5 changed files with 23 additions and 14 deletions
|
|
@ -135,11 +135,12 @@ module JsonRpcHandler
|
||||||
success_response(id: id, result: result)
|
success_response(id: id, result: result)
|
||||||
rescue MCP::Server::RequestHandlerError => e
|
rescue MCP::Server::RequestHandlerError => e
|
||||||
handle_request_error(e, id, id_validation_pattern)
|
handle_request_error(e, id, id_validation_pattern)
|
||||||
rescue StandardError => e
|
rescue StandardError
|
||||||
|
# The exception message is deliberately not echoed into `data`: it can carry
|
||||||
|
# internals that must not reach untrusted clients (CWE-209).
|
||||||
error_response(id: id, id_validation_pattern: id_validation_pattern, error: {
|
error_response(id: id, id_validation_pattern: id_validation_pattern, error: {
|
||||||
code: ErrorCode::INTERNAL_ERROR,
|
code: ErrorCode::INTERNAL_ERROR,
|
||||||
message: "Internal error",
|
message: "Internal error",
|
||||||
data: e.message,
|
|
||||||
})
|
})
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
||||||
|
|
@ -560,7 +560,14 @@ module MCP
|
||||||
return
|
return
|
||||||
end
|
end
|
||||||
|
|
||||||
Methods.ensure_capability!(method, capabilities)
|
begin
|
||||||
|
Methods.ensure_capability!(method, capabilities)
|
||||||
|
rescue Methods::MissingRequiredCapabilityError => e
|
||||||
|
# Re-raise through RequestHandlerError, the one channel whose message is
|
||||||
|
# deliberately surfaced to clients: `JsonRpcHandler`'s blind `rescue StandardError`
|
||||||
|
# no longer echoes exception messages into the error `data` member (CWE-209).
|
||||||
|
raise RequestHandlerError.new(e.message, request, error_type: :internal_error, original_error: e)
|
||||||
|
end
|
||||||
|
|
||||||
# `initialize` MUST NOT be cancelled (MCP spec 2025-11-25, cancellation item 2),
|
# `initialize` MUST NOT be cancelled (MCP spec 2025-11-25, cancellation item 2),
|
||||||
# so do not track it in the in-flight registry.
|
# so do not track it in the in-flight registry.
|
||||||
|
|
@ -864,8 +871,11 @@ module MCP
|
||||||
# `JsonRpcHandler::NO_RESPONSE` per the MCP cancellation spec.
|
# `JsonRpcHandler::NO_RESPONSE` per the MCP cancellation spec.
|
||||||
raise
|
raise
|
||||||
rescue => e
|
rescue => e
|
||||||
|
# `e.message` is deliberately not included: it can carry internals (class, method
|
||||||
|
# and host names) that must not reach untrusted clients (CWE-209). The original
|
||||||
|
# exception still reaches `configuration.exception_reporter` via `original_error`.
|
||||||
raise RequestHandlerError.new(
|
raise RequestHandlerError.new(
|
||||||
"Internal error calling tool #{tool_name}: #{e.message}",
|
"Internal error calling tool #{tool_name}",
|
||||||
request,
|
request,
|
||||||
error_type: :internal_error,
|
error_type: :internal_error,
|
||||||
original_error: e,
|
original_error: e,
|
||||||
|
|
|
||||||
|
|
@ -310,7 +310,6 @@ describe JsonRpcHandler do
|
||||||
assert_rpc_error expected_error: {
|
assert_rpc_error expected_error: {
|
||||||
code: -32603,
|
code: -32603,
|
||||||
message: "Internal error",
|
message: "Internal error",
|
||||||
data: "Something bad happened",
|
|
||||||
}
|
}
|
||||||
assert_equal 1, @response[:id]
|
assert_equal 1, @response[:id]
|
||||||
assert_nil @response[:result]
|
assert_nil @response[:result]
|
||||||
|
|
@ -452,7 +451,6 @@ describe JsonRpcHandler do
|
||||||
assert_rpc_error expected_error: {
|
assert_rpc_error expected_error: {
|
||||||
code: -32603,
|
code: -32603,
|
||||||
message: "Internal error",
|
message: "Internal error",
|
||||||
data: "Something bad happened",
|
|
||||||
}
|
}
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -545,7 +545,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool tool_with_required_context: /, response[:error][:data])
|
assert_equal "Internal error calling tool tool_with_required_context", response[:error][:data]
|
||||||
end
|
end
|
||||||
|
|
||||||
test "call_tool_with_args correctly detects server_context parameter presence" do
|
test "call_tool_with_args correctly detects server_context parameter presence" do
|
||||||
|
|
|
||||||
|
|
@ -884,7 +884,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool tool_that_raises: /, response[:error][:data])
|
assert_equal "Internal error calling tool tool_that_raises", response[:error][:data]
|
||||||
assert_instrumentation_data({ method: "tools/call", tool_name: "tool_that_raises", tool_arguments: { message: "test" }, error: :internal_error })
|
assert_instrumentation_data({ method: "tools/call", tool_name: "tool_that_raises", tool_arguments: { message: "test" }, error: :internal_error })
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
@ -941,7 +941,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool tool_that_raises: /, response[:error][:data])
|
assert_equal "Internal error calling tool tool_that_raises", response[:error][:data]
|
||||||
assert_instrumentation_data({ method: "tools/call", tool_name: "tool_that_raises", tool_arguments: { message: "test" }, error: :internal_error })
|
assert_instrumentation_data({ method: "tools/call", tool_name: "tool_that_raises", tool_arguments: { message: "test" }, error: :internal_error })
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
@ -972,7 +972,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool tool_with_faulty_schema: Unexpected schema error/, response[:error][:data])
|
assert_equal "Internal error calling tool tool_with_faulty_schema", response[:error][:data]
|
||||||
end
|
end
|
||||||
|
|
||||||
test "#handle tools/call returns JSON-RPC error for unknown tool" do
|
test "#handle tools/call returns JSON-RPC error for unknown tool" do
|
||||||
|
|
@ -1727,7 +1727,7 @@ module MCP
|
||||||
|
|
||||||
assert_equal(["tool failure", "around ensure boom"], reported)
|
assert_equal(["tool failure", "around ensure boom"], reported)
|
||||||
assert_equal(JsonRpcHandler::ErrorCode::INTERNAL_ERROR, response[:error][:code])
|
assert_equal(JsonRpcHandler::ErrorCode::INTERNAL_ERROR, response[:error][:code])
|
||||||
assert_equal("around ensure boom", response[:error][:data])
|
assert_nil(response[:error][:data])
|
||||||
end
|
end
|
||||||
|
|
||||||
test "#handle reports the same exception object reused across requests on every call" do
|
test "#handle reports the same exception object reused across requests on every call" do
|
||||||
|
|
@ -1779,7 +1779,7 @@ module MCP
|
||||||
response = server.handle(request)
|
response = server.handle(request)
|
||||||
|
|
||||||
assert_equal([frozen_error], reported)
|
assert_equal([frozen_error], reported)
|
||||||
assert_includes(response[:error][:data], "frozen failure")
|
assert_equal("Internal error calling tool frozen_tool", response[:error][:data])
|
||||||
end
|
end
|
||||||
|
|
||||||
test "#handle still reports via exception_reporter when around_request swallows the tool failure" do
|
test "#handle still reports via exception_reporter when around_request swallows the tool failure" do
|
||||||
|
|
@ -2635,7 +2635,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool invalid_structured_content_tool: Invalid result:/, response[:error][:data])
|
assert_equal "Internal error calling tool invalid_structured_content_tool", response[:error][:data]
|
||||||
end
|
end
|
||||||
|
|
||||||
test "tools/call returns JSON-RPC error when output schema validation is enabled and structuredContent is missing" do
|
test "tools/call returns JSON-RPC error when output schema validation is enabled and structuredContent is missing" do
|
||||||
|
|
@ -2664,7 +2664,7 @@ module MCP
|
||||||
assert_nil response[:result]
|
assert_nil response[:result]
|
||||||
assert_equal(-32603, response[:error][:code])
|
assert_equal(-32603, response[:error][:code])
|
||||||
assert_equal "Internal error", response[:error][:message]
|
assert_equal "Internal error", response[:error][:message]
|
||||||
assert_match(/Internal error calling tool missing_structured_content_tool: Invalid result:/, response[:error][:data])
|
assert_equal "Internal error calling tool missing_structured_content_tool", response[:error][:data]
|
||||||
end
|
end
|
||||||
|
|
||||||
test "tools/call skips output schema validation for error responses" do
|
test "tools/call skips output schema validation for error responses" do
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue