diff --git a/src/mcp_manager.py b/src/mcp_manager.py index 6f44e999ab..c4cd0cd14a 100644 --- a/src/mcp_manager.py +++ b/src/mcp_manager.py @@ -184,13 +184,27 @@ async def _connect_stdio(self, server_id: str, name: str, command: str, args: Li """Connect to an MCP server via stdio transport.""" try: from mcp import ClientSession, StdioServerParameters - from mcp.client.stdio import stdio_client + from mcp.client.stdio import stdio_client, get_default_environment from contextlib import AsyncExitStack + # Passing env=None does not mean "inherit the parent environment" — + # the MCP SDK substitutes a minimal allowlist (HOME, PATH, …). The + # built-in NPX browser server passed nothing and so lost + # PLAYWRIGHT_BROWSERS_PATH, then reported `Browser "firefox" is not + # installed` with the browser sitting one directory away. + # + # Only the built-ins get the full environment. User-added servers + # are third-party processes: this one file's environment carries + # ODYSSEUS_INTERNAL_TOKEN (an admin bypass) among other secrets, so + # handing them os.environ would leak credentials to an arbitrary + # command. They keep the SDK's filtered default, plus whatever the + # server's own configuration explicitly sets. + base = os.environ if self.is_builtin(server_id) else get_default_environment() + server_params = StdioServerParameters( command=command, args=args, - env={**os.environ, **env} if env else None, + env={**base, **env}, ) stack = AsyncExitStack() @@ -477,6 +491,14 @@ async def call_tool(self, qualified_name: str, arguments: Dict) -> Dict: tool_name = parts[2] session = self._sessions.get(server_id) + if not session and self.is_builtin(server_id): + # A stdio session can disappear without the process dying — the + # teardown races across asyncio tasks. The recovery below only runs + # when a call raises, which presupposes a session, so a missing one + # was terminal even though reconnecting would have fixed it. + logger.warning(f"No session for builtin {server_id}; attempting reconnect") + if await self._reconnect_builtin(server_id): + session = self._sessions.get(server_id) if not session: return {"error": f"MCP server not connected: {server_id}", "exit_code": 1} @@ -537,7 +559,30 @@ async def _do_call(self, session, tool_name: str, arguments: Dict) -> Dict: async def _reconnect_builtin(self, server_id: str) -> bool: """Tear down and reconnect a crashed builtin MCP server.""" import sys - from src.builtin_mcp import _BUILTIN_SERVERS, builtin_python_env + from src.builtin_mcp import ( + _BUILTIN_SERVERS, _BUILTIN_NPX_SERVERS, _find_npx, builtin_python_env, + ) + + # NPX-backed builtins (the browser) are builtins too — is_builtin() + # says so — but this membership test only knew about the Python ones, + # so the browser could never be reconnected. + if server_id in _BUILTIN_NPX_SERVERS: + cfg = _BUILTIN_NPX_SERVERS[server_id] + await self.disconnect_server(server_id) + try: + ok = await self.connect_server( + server_id=server_id, + name=cfg["name"], + transport="stdio", + command=_find_npx(), + args=cfg["args"], + ) + if ok: + logger.info(f"Reconnected builtin MCP server: {cfg['name']}") + return ok + except Exception as e: + logger.error(f"Failed to reconnect builtin MCP server {cfg['name']}: {e}") + return False if server_id not in _BUILTIN_SERVERS: return False diff --git a/tests/test_mcp_stdio_env_scope.py b/tests/test_mcp_stdio_env_scope.py new file mode 100644 index 0000000000..891ba673e8 --- /dev/null +++ b/tests/test_mcp_stdio_env_scope.py @@ -0,0 +1,90 @@ +"""Only built-in MCP servers may inherit the full process environment. + +`_connect_stdio` has to widen the environment for the built-in NPX browser +server, which otherwise loses PLAYWRIGHT_BROWSERS_PATH and reports +`Browser "firefox" is not installed`. Widening it for *every* stdio server +would bypass the MCP SDK's allowlist and hand the whole environment — which +carries ODYSSEUS_INTERNAL_TOKEN, an admin bypass, among other secrets — to +user-added servers, i.e. to an arbitrary third-party command. + +These pin the split: full inheritance for built-ins, the SDK's filtered +default for everything else, with a server's own explicit env still applied +on top in both cases. +""" + +import asyncio +from unittest.mock import patch + +import pytest + +from src.mcp_manager import McpManager + + +CANARY = "ODYSSEUS_TEST_CANARY_9931" + + +class _Captured(Exception): + """Raised by the stub to stop _connect_stdio once params are captured.""" + + +def _connect_and_capture(server_id, env=None): + """Run _connect_stdio far enough to capture its StdioServerParameters.""" + seen = {} + + def _stub(params): + seen["params"] = params + raise _Captured + + mgr = McpManager() + with patch("mcp.client.stdio.stdio_client", _stub): + try: + asyncio.run( + mgr._connect_stdio( + server_id, "Test Server", "echo", ["hi"], env or {} + ) + ) + except _Captured: + pass # expected: the stub aborts once it has the params + assert "params" in seen, "stdio_client was never reached" + return seen["params"].env + + +@pytest.fixture(autouse=True) +def _secrets(monkeypatch): + monkeypatch.setenv(CANARY, "canary-value") + monkeypatch.setenv("ODYSSEUS_INTERNAL_TOKEN", "admin-bypass-token") + + +def test_builtin_server_inherits_full_environment(): + """Built-ins are our own code — they need the deployment's environment.""" + env = _connect_and_capture("builtin_browser") + + assert env[CANARY] == "canary-value" + assert env["ODYSSEUS_INTERNAL_TOKEN"] == "admin-bypass-token" + + +def test_user_server_does_not_receive_unfiltered_environment(): + """A user-added server is a third-party process: no secrets by default.""" + env = _connect_and_capture("user_added_server") + + assert CANARY not in env + assert "ODYSSEUS_INTERNAL_TOKEN" not in env + + +def test_user_server_still_gets_the_sdk_default_and_its_own_env(): + """Filtering must not starve the server of what it legitimately needs.""" + from mcp.client.stdio import get_default_environment + + env = _connect_and_capture("user_added_server", {"MY_SERVER_KEY": "abc"}) + + for key in get_default_environment(): + assert key in env, f"SDK default {key} was dropped" + assert env["MY_SERVER_KEY"] == "abc" + assert CANARY not in env + + +def test_explicit_env_overrides_inherited_value_for_builtins(): + """An explicit per-server value still wins over the inherited one.""" + env = _connect_and_capture("builtin_browser", {CANARY: "explicit"}) + + assert env[CANARY] == "explicit"