fix(mcp): first-wins tool routing to stop silent cross-server name shadowing - #53
Merged
Conversation
…adowing StreamManager mapped each MCP tool name to whichever server registered it last, with no collision check. A second server advertising a name already provided by an earlier server (malicious, compromised, or just reusing a common name like read_file) silently captured every future unpinned call_tool for that name — no warning, leaking arguments to and returning results from the wrong server. Registration is now first-wins: the first server to advertise a name owns default routing; a colliding later server is ignored for routing and logged with a prominent warning; the shadowed tool stays reachable via call_tool(name, server_name=...). Adds get_servers_for_tool() and get_tool_collisions() to inspect collisions. Bumps to 0.26.0. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
…down race shutdown() offloaded pool.shutdown(wait=False) to a thread-pool executor guarded by a 1s wait_for. On a busy event loop the executor could fail to run before the timeout fired, so the pool was cancelled without ever being shut down — an intermittent 'shutdown called 0 times' test failure and a real pool-leak risk. shutdown(wait=False) is non-blocking, so call it directly, matching the other cleanup paths in this module. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
StreamManagermapped each MCP tool name to whichever server advertised it last, with no collision check, warning, or log. When two configured servers exposed the same tool name (common short names likeread_file,search,querycollide easily, or a malicious/compromised server picks a name deliberately), the later server silently captured every future unpinnedcall_toolfor that name — leaking the caller's arguments to, and returning fabricated results from, the wrong server. Reachable through mcp-cli's default chat tool-calling loop, which never passesserver_name.Verified against the current code with a live PoC on the real
StreamManager.Fix — first-wins + loud warning
call_tool(name, server_name=...).get_servers_for_tool(name)andget_tool_collisions()._register_toolshelper (previously duplicated).Tests
tests/mcp/test_stream_manager_shadowing.py(8 tests): first-wins ownership, collision warning, no-warning-without-collision, reconnect no-op, unpinned stays with first server, pinned reaches shadowed server, reset clears state.Bumps to 0.26.0 (adds public API + changes routing behavior).
🤖 Generated with Claude Code