diff --git a/coworker/mcp/tools.py b/coworker/mcp/tools.py index ecd8a1406..696a229b2 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 437c6e267..e8916062b 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"))