fix(mcp_client): enforce overall timeout for list_tools pagination (#836) - #912
fix(mcp_client): enforce overall timeout for list_tools pagination (#836)#912Artemon-line wants to merge 1 commit into
Conversation
…raxis-proxy#836) Signed-off-by: Artemy <ahladenk@redhat.com>
praxis-bot
left a comment
There was a problem hiding this comment.
MCP Client Cumulative Timeout Review
Purpose: Wraps list_tools and call_tool in a top-level tokio::time::timeout so that cumulative pagination, DNS, and transport time cannot exceed the configured timeout. Previously a malicious MCP server could delay each paginated page by just under the per-page timeout, causing list_tools to run for up to ~100x the configured duration.
Assessment: The outer timeout correctly closes the cumulative-pagination DoS vector described in #836. The new test (list_tools_cumulative_pagination_timeout) exercises the fix with a real mock MCP server. The parse_display_url extraction is a clean way to make a display URL available for the outer timeout error without running resolve_and_validate first.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
| .map_err(|_source| McpClientError::Connection { | ||
| url: display_url.clone(), | ||
| })?; | ||
| let tools = paginate_tools(&client, timeout, max_tools, &display_url).await?; |
There was a problem hiding this comment.
Medium: inner timeouts are now unreachable dead code.
The outer tokio::time::timeout(timeout, Box::pin(work)) at line 256 uses the same timeout duration as every inner timeout: the connection timeout here (line 243), the per-page timeouts inside paginate_tools (line 353 in the unchanged code), and the call_tool timeout in the other function (line 316). Because the outer timer starts strictly before any inner timer using the same duration, the inner timeouts can never fire -- the outer one always expires first and cancels the inner future.
This means ~30 lines of inner timeout/map_err handling across both functions, plus the timeout parameter on paginate_tools, are dead code paths that will never execute.
Suggested change: either remove the inner timeouts (simplest) or, if per-operation limits are desired alongside the overall limit, use a shorter per-operation duration (e.g. timeout / 2 or a separate config field). Keeping both at the same duration is misleading -- a reader would assume both can fire.
Summary
Enforces a top-level
tokio::time::timeoutwrapper across the completelist_toolsandcall_toolclient lifecycle (DNS resolution, transport connection/handshake, and multi-page tools listing pagination).Previously, each individual tools/list page request had its own timeout check inside
paginate_tools. A degraded or malicious MCP server returning paginated tools (up to 100 pages) could delay every page request by slightly under the timeout duration, causinglist_toolsto execute for up to ~100x the configured timeout.Related issue
Closes #836
Validation
cargo test -p praxis-ai-apis -- mcp_client)list_tools_cumulative_pagination_timeoutwith mock slow paginated MCP server)make lintChecklist
Signed-off-bytrailer.Breaking changes
None.