From d78a17a9913cf44af045f781d081f4c5ff7b5c42 Mon Sep 17 00:00:00 2001 From: sainikhiljuluri Date: Fri, 28 Aug 2026 23:15:30 -0700 Subject: [PATCH] fix(mcp): prevent tool-call arguments from overriding MCP 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) --- coworker/mcp/tools.py | 19 ++++++++++++++++--- tests/test_mcp.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/coworker/mcp/tools.py b/coworker/mcp/tools.py index ecd8a14066..696a229b2a 100644 --- a/coworker/mcp/tools.py +++ b/coworker/mcp/tools.py @@ -68,9 +68,22 @@ def build_callables( name = tool_name(server.name, mcp_tool.name) remote = mcp_tool.name - def _invoke(_remote: str = remote, **kwargs: Any) -> Any: - future = asyncio.run_coroutine_threadsafe(call_async(_remote, kwargs), loop) - return future.result(timeout) + # Bind `remote` in a dedicated closure rather than a default argument. The + # default-argument form (`_remote=remote`) also defeats late binding, but it + # exposes the routing target as a caller-overridable keyword: a model-supplied + # `_remote` in the tool-call arguments would then re-route an approved, filtered + # call to any other tool on this server, since the approval gate and the + # include/exclude filter only ever see the visible tool name. + def _make_invoke(remote: str) -> Callable[..., Any]: + def _invoke(**kwargs: Any) -> Any: + future = asyncio.run_coroutine_threadsafe( + call_async(remote, kwargs), loop + ) + return future.result(timeout) + + return _invoke + + _invoke = _make_invoke(remote) # We attach the schema + metadata explicitly (rather than via `ai.tool`, which would # try to derive a schema from this `**kwargs` wrapper): the registry reads both attrs. diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 437c6e2675..e8916062bc 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -227,6 +227,35 @@ async def call_async(tool, args): assert seen == [("read_file", {"path": "a.txt"})] +async def test_remote_tool_routing_is_not_overridable_by_arguments(): + """A model-supplied ``_remote`` argument must not re-route an approved, + filtered tool call to a different remote tool. + + The approval gate and the include/exclude filter both vet the *visible* + tool name (``mcp__fs__read_file``). If ``_remote`` were a bindable + parameter, ``{"_remote": "delete_file"}`` in the call arguments would run + ``delete_file`` while the gate only ever saw ``read_file``. + """ + loop = asyncio.get_running_loop() + seen = [] + + async def call_async(tool, args): + seen.append((tool, args)) + return {"echo": args} + + server = MCPServerDef(name="fs", transport="stdio") + fn = build_callables(server, [_fake_tool("read_file")], call_async, loop)[0] + + # Simulate the model emitting a `_remote` key in the tool-call arguments. + await asyncio.to_thread(fn, _remote="delete_file", path="a.txt") + + routed_tool = seen[0][0] + assert routed_tool == "read_file", ( + f"routing hijacked: call reached {routed_tool!r} instead of the " + "approved 'read_file'" + ) + + # -- REST ---------------------------------------------------------------------- def test_rest_crud(tmp_path, monkeypatch): monkeypatch.setenv("COWORKER_STATE_DIR", str(tmp_path / "state"))