feat(filter): add mcp_tool_resolve filter for MCP tool discovery#331
feat(filter): add mcp_tool_resolve filter for MCP tool discovery#331leseb wants to merge 5 commits into
Conversation
Resolves MCP tool entries in Responses API requests by calling tools/list on upstream MCP servers. Includes SSRF validation, MCPToolFilter object support, OAuth authorization token injection, example config, and integration tests. Closes praxis-proxy#43 Signed-off-by: Sébastien Han <seb@redhat.com>
|
PR too large: 2449 lines added (limit: 750, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. Add |
CI requires MAJOR.MINOR.PATCH format for dependency versions. Signed-off-by: Sébastien Han <seb@redhat.com>
- Add allow_loopback config option to permit connections to 127.0.0.0/8, ::1, and localhost for development environments - Make inject_authorization return explicit InvalidAuthorization error on invalid header characters instead of silently dropping - Add integration tests against mock MCP server: tools/list succeeds, TooManyTools rejected, resolved body echoed through responses_proxy, invalid authorization rejected - Document openai_responses_rehydrate dependency in example config - Add mcp_tool_resolve to full-flow.yaml pipeline - Regenerate filter docs with new allow_loopback field Signed-off-by: Sébastien Han <seb@redhat.com>
praxis-bot
left a comment
There was a problem hiding this comment.
Review: mcp_tool_resolve filter (Filter 18)
Solid implementation with thorough SSRF hardening, DNS pinning, and extensive test coverage across unit, integration, and example-config layers. The security model is well-considered (hop-by-hop stripping, reserved prefix blocking, redirect prohibition, proxy env suppression).
Four issues identified below.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 4 |
Reviewed files: apis/src/mcp_client/mod.rs, apis/src/mcp_client/tests.rs, apis/src/openai/responses/mcp_tool_resolve/mod.rs, apis/src/openai/responses/mcp_tool_resolve/config.rs, apis/src/openai/responses/mcp_tool_resolve/tests.rs, apis/src/openai/responses/state.rs, apis/src/openai/responses/rehydrate/mod.rs, Cargo.toml, apis/Cargo.toml, server/src/lib.rs, tests/integration/tests/suite/mcp_tool_resolve.rs, tests/integration/tests/suite/examples/mcp_tool_resolve.rs, examples/configs/openai/responses/mcp-tool-resolve.yaml, examples/configs/openai/responses/full-flow.yaml
| url: &str, | ||
| ) -> Result<Vec<rmcp::model::Tool>, McpClientError> { | ||
| let mut all_tools = Vec::new(); | ||
| let mut cursor = None; |
There was a problem hiding this comment.
[Medium] paginate_tools has no maximum iteration count. A hostile MCP server can return empty pages (zero tools) with a valid next_cursor on every response, causing this loop to run indefinitely. The max_tools guard only fires when tools accumulate, so empty-page responses bypass it entirely. The per-page timeout bounds each individual request but not the total number of iterations.
Add a max_pages constant (e.g., const MAX_PAGES: usize = 100;) and break with a TooManyTools or new TooManyPages error after that many iterations:
let mut page_count = 0usize;
loop {
page_count += 1;
if page_count > MAX_PAGES {
return Err(McpClientError::TooManyTools {
url: url.to_owned(),
count: all_tools.len(),
max: max_tools,
});
}
// ... existing page fetch ...
}| ctx: &mut HttpFilterContext<'_>, | ||
| body: &[u8], | ||
| ) -> Result<FilterAction, ResolveError> { | ||
| let mcp_entries = extract_mcp_entries(body); |
There was a problem hiding this comment.
[Medium] The request body is deserialized twice: once here in extract_mcp_entries and again in write_tool_map (line 338) via serde_json::from_slice. With max_body_bytes defaulting to 64 MiB, this doubles the parsing cost on large payloads.
Refactor resolve_mcp_tools to parse the body once and thread the parsed serde_json::Value through to both extract_mcp_entries (change it to accept &serde_json::Value) and write_tool_map (pass the already-parsed value instead of raw bytes).
|
|
||
| /// Build tool map from all MCP entries, resolving each | ||
| /// server once and applying per-entry filters. | ||
| async fn build_tool_map( |
There was a problem hiding this comment.
[Medium] build_tool_map resolves MCP servers sequentially. With max_servers: 10 and timeout_ms: 5000, a request referencing 10 slow servers blocks for up to 50 seconds of serial I/O. Since each server resolution is independent, use tokio::JoinSet (or futures::future::join_all) to resolve servers concurrently, bounded by max_servers.
This matters in production where users specify multiple MCP servers and tail latency on any single server currently gates the entire pipeline.
| regex = "1.12.4" | ||
| reqwest = { version = "0.12.28", default-features = false, features = ["rustls-tls"] } | ||
| reqwest = { version = "0.13.4", default-features = false, features = ["rustls"] } | ||
| rmcp = { version = "2.0.0", default-features = false, features = ["client", "transport-streamable-http-client-reqwest", "reqwest"] } |
There was a problem hiding this comment.
[Medium] The workspace dependency specifies version = "2.0.0" but Cargo.lock resolves to 2.2.0. Project convention requires full semver with patch level matching the resolved version (e.g., reqwest on line 38 uses "0.13.4" exactly). Change to version = "2.2.0" to match the lockfile and follow the established pattern.
Summary
Adds Filter 18 (
mcp_tool_resolve) which resolves MCP tool entries in Responses APItoolsarrays into concrete tool definitions by callingtools/liston upstream MCP servers.Key capabilities
apis/src/mcp_client/): Streamable HTTP transport viarmcp, with SSRF validation (loopback, link-local, unspecified, cloud metadata IPs, AWS IMDS IPv6), DNS pinning to prevent rebinding, URL userinfo rejection, hop-by-hop/framing header blocking, and reserved prefix stripping (x-praxis-*,x-mcp-*,x-a2a-*)tools/listwith configurablemax_toolscap (default 128),max_serverslimit (default 10), and per-server timeout (default 5s)(label, url)and cross-request reuse viaprevious_toolsfrom rehydrated responses, with label-only matching for real API shapesMCPToolFilterobject forms ofallowed_tools, includingread_onlyannotation filteringauthorizationfield injected as Bearer token;Authorizationinheadersis strippedconnector_id-only entries,defer_loading: trueentries, and entries withoutserver_labelFiles changed (23)
mcp_clientmodule with full SSRF/DNS/transport hardeningmcp_tool_resolvefilter with config, resolution logic, and 113 unit testsResponsesStatewithmcp_tool_mapandprevious_toolsfieldsrehydrateto persistserver_urlinmcp_list_toolsitemsCloses #43