Skip to content

fix(mcp): prevent tool-call arguments from overriding MCP routing - #589

Open
sainikhiljuluri wants to merge 1 commit into
andrewyng:mainfrom
sainikhiljuluri:fix/mcp-remote-tool-routing-override
Open

fix(mcp): prevent tool-call arguments from overriding MCP routing#589
sainikhiljuluri wants to merge 1 commit into
andrewyng:mainfrom
sainikhiljuluri:fix/mcp-remote-tool-routing-override

Conversation

@sainikhiljuluri

Copy link
Copy Markdown

Summary

The MCP tool wrapper bound its remote routing target through a default argument, which made that target overridable by the model-supplied tool-call arguments. A tool call whose arguments contain a _remote key would run a different tool on the same server than the one the approval gate and the include/exclude filter actually vetted. This hardens the wrapper so the routing target is fixed at wrap time and cannot be redirected by call arguments.

Details

build_callables wraps each MCP tool as:

def _invoke(_remote: str = remote, **kwargs):
    future = asyncio.run_coroutine_threadsafe(call_async(_remote, kwargs), loop)
    return future.result(timeout)

The default-argument form is used to defeat closure late-binding, but it also exposes _remote as a normal, caller-overridable keyword. The registry forwards the model's raw tool-call arguments to the wrapper unchanged (spec.func(**(arguments or {}))), and _remote is not part of the model-facing schema — so if the arguments dict happens to carry a _remote key, Python binds it and call_async is invoked with a caller-chosen tool name.

That matters because the approval/reviewer decision (PermissionEngine.evaluate), the per-tool read/write classification for connector-backed servers, and the include_tools/exclude_tools filter all key off the visible tool name (mcp__<server>__<tool>). The gate vets one tool; a _remote override runs another on the same server — including a tool the include/exclude filter was meant to hide. This is reachable without a compromised machine: a prompt-injection payload in content the agent is processing, or a malicious/compromised MCP server steering the model, is enough to get a _remote key into the arguments.

Fix

Bind remote in a dedicated closure (_make_invoke) instead of a default argument. This still defeats late-binding, but _remote is no longer a bindable parameter, so a stray _remote argument is simply forwarded to the remote tool as an ordinary argument and cannot change the routing target. One function, no signature or schema change for callers.

Test plan

Added tests/test_mcp.py::test_remote_tool_routing_is_not_overridable_by_arguments: it wraps a single read_file tool and invokes it with _remote="delete_file" in the arguments, asserting the bridge still routes to read_file. Verified it fails on main (routes to delete_file) and passes with this change.

pytest tests/test_mcp.py -q            # 21 passed
pytest tests -q                        # 1897 passed, 1 skipped

Risk / backward compatibility

  • No change to the tool schema shown to the model, to tool names, or to the wrapper's observable behavior for well-formed calls. Late-binding across a server's tools is preserved (verified: each wrapper routes to its own tool).
  • The only behavioral change is that a _remote key in call arguments no longer redirects routing.

The MCP tool wrapper bound its remote routing target via a default
argument (`_invoke(_remote=remote, **kwargs)`). That defeats closure
late-binding, but it also makes `_remote` a caller-overridable keyword.
Because the registry forwards the model's raw tool-call arguments to the
wrapper unchanged, a tool call whose arguments include a `_remote` key
re-routes to a different tool on the same server than the one the
approval gate and the include/exclude filter vetted (both key off the
visible tool name).

Bind `remote` in a dedicated closure instead, so it is no longer a
bindable parameter; a stray `_remote` argument is now forwarded to the
remote tool as an ordinary argument and cannot change the routing target.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant