From dfd552368ead4733a9acfa41da1f441758495f9a Mon Sep 17 00:00:00 2001 From: cristhianzl Date: Thu, 7 May 2026 11:50:56 -0300 Subject: [PATCH] add windows const paths --- .../langflow/api/utils/mcp/default_servers.py | 39 ++-- .../mcp/test_mcp_stdio_env_passthrough.py | 198 ++++++++++++++++++ src/lfx/src/lfx/base/mcp/util.py | 69 +++++- 3 files changed, 288 insertions(+), 18 deletions(-) create mode 100644 src/backend/tests/unit/base/mcp/test_mcp_stdio_env_passthrough.py diff --git a/src/backend/base/langflow/api/utils/mcp/default_servers.py b/src/backend/base/langflow/api/utils/mcp/default_servers.py index 0111938567..2cb0b73446 100644 --- a/src/backend/base/langflow/api/utils/mcp/default_servers.py +++ b/src/backend/base/langflow/api/utils/mcp/default_servers.py @@ -105,6 +105,7 @@ async def auto_configure_default_mcp_servers(session: AsyncSession) -> None: "mcpServers", {} ) persisted = existing_servers.get(server_name) + is_reconcile = False if persisted is not None: persisted_meta = persisted.get("metadata") or {} if not persisted_meta.get("auto_configured"): @@ -124,18 +125,10 @@ async def auto_configure_default_mcp_servers(session: AsyncSession) -> None: reason="already_in_sync", ) continue - # Spec drifted (e.g. user upgraded across a commit that changed the - # canonical payload). Reconcile by overwriting with the new spec — - # `auto_configured: True` was our marker that the entry is ours. - # Platform context lets ops correlate reconciliations against the - # reported OS when the prior version's payload is too short for - # that OS (notably first-run npx timeouts on Windows). - await logger.ainfo( - "default_mcp_server_reconciled", - user_id=str(user.id), - server_name=server_name, - platform=platform.system(), - ) + # Spec drifted (e.g. user upgraded across a commit that changed + # the canonical payload). `auto_configured: True` was our marker + # that the entry is ours, so overwriting is safe. + is_reconcile = True await update_server( server_name=server_name, server_config=payload, @@ -144,11 +137,23 @@ async def auto_configure_default_mcp_servers(session: AsyncSession) -> None: storage_service=storage_service, settings_service=settings_service, ) - await logger.ainfo( - "default_mcp_server_added", - user_id=str(user.id), - server_name=server_name, - ) + # Distinct event names so Sentry/Datadog filtering can separate + # fresh installs from upgrade-driven reconciliations. Platform is + # included on reconcile because the bug that motivated this path + # (first-run `npx -y` timeouts on Windows) is OS-correlated. + if is_reconcile: + await logger.ainfo( + "default_mcp_server_reconciled", + user_id=str(user.id), + server_name=server_name, + platform=platform.system(), + ) + else: + await logger.ainfo( + "default_mcp_server_added", + user_id=str(user.id), + server_name=server_name, + ) except ( HTTPException, sqlalchemy_exc.SQLAlchemyError, diff --git a/src/backend/tests/unit/base/mcp/test_mcp_stdio_env_passthrough.py b/src/backend/tests/unit/base/mcp/test_mcp_stdio_env_passthrough.py new file mode 100644 index 0000000000..7590be5247 --- /dev/null +++ b/src/backend/tests/unit/base/mcp/test_mcp_stdio_env_passthrough.py @@ -0,0 +1,198 @@ +"""Tests for OS-essential env var passthrough in MCP stdio connections. + +Why this exists: child MCP servers (notably `@wonderwhy-er/desktop-commander`) +call Node's `os.homedir()` at module load. On Windows, `os.homedir()` reads +`USERPROFILE` (or `HOMEDRIVE`+`HOMEPATH`); on Unix it reads `HOME` first and +only then falls back to a `getpwuid()` lookup. + +Before this fix, `MCPStdioClient._connect_to_server` passed only `{DEBUG, PATH}` +into the subprocess env. On Windows that left `os.homedir()` returning `null`, +which then crashed any server that built paths from `path.join(USER_HOME, ...)`. +The MCP SDK then surfaced the crash as an opaque "Connection closed" / +TaskGroup sub-exception — same error users saw for the shell-execution server. + +This file pins the curated passthrough so the next person who tightens the env +list doesn't accidentally re-strip `USERPROFILE` / `HOME` and resurrect the +Windows bug. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest +from lfx.base.mcp.util import MCPStdioClient + + +@pytest.fixture +def stdio_client(): + return MCPStdioClient() + + +async def _connect_capturing_env(client: MCPStdioClient, *, command: str = "echo hi") -> dict[str, str]: + """Capture the env dict the StdioServerParameters was built with. + + Drives `_connect_to_server` far enough that StdioServerParameters is built, + then returns the env dict — without launching a subprocess. + """ + with patch.object( + client, + "_get_or_create_session", + new=AsyncMock(return_value=SimpleNamespace(list_tools=AsyncMock(return_value=SimpleNamespace(tools=[])))), + ): + await client._connect_to_server(command) + return dict(client._connection_params.env) + + +class TestWindowsEnvPassthrough: + """When the host is Windows, `USERPROFILE` etc must reach the child.""" + + async def test_should_pass_userprofile_when_present_on_windows(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Windows"), + patch.dict( + "os.environ", + {"PATH": "C:/Windows", "USERPROFILE": r"C:\Users\test"}, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("USERPROFILE") == r"C:\Users\test" + + async def test_should_pass_homedrive_and_homepath_fallbacks_on_windows(self, stdio_client): + """`os.homedir()` falls back to HOMEDRIVE+HOMEPATH when USERPROFILE is absent.""" + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Windows"), + patch.dict( + "os.environ", + {"PATH": "C:/Windows", "HOMEDRIVE": "C:", "HOMEPATH": r"\Users\test"}, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("HOMEDRIVE") == "C:" + assert env.get("HOMEPATH") == r"\Users\test" + + async def test_should_pass_appdata_and_temp_on_windows(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Windows"), + patch.dict( + "os.environ", + { + "PATH": "C:/Windows", + "APPDATA": r"C:\Users\test\AppData\Roaming", + "LOCALAPPDATA": r"C:\Users\test\AppData\Local", + "TEMP": r"C:\Users\test\AppData\Local\Temp", + "TMP": r"C:\Users\test\AppData\Local\Temp", + }, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("APPDATA", "").endswith("Roaming") + assert env.get("LOCALAPPDATA", "").endswith("Local") + assert env.get("TEMP", "").endswith("Temp") + assert env.get("TMP", "").endswith("Temp") + + async def test_should_pass_pathext_so_cmd_resolves_npx_cmd_on_windows(self, stdio_client): + """Without PATHEXT, `cmd /c npx ...` can't find `npx.cmd` in PATH.""" + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Windows"), + patch.dict( + "os.environ", + {"PATH": "C:/Windows", "PATHEXT": ".COM;.EXE;.BAT;.CMD"}, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert ".CMD" in env.get("PATHEXT", "") + + async def test_should_omit_windows_vars_when_absent_from_host_environ(self, stdio_client): + """Only forward what's actually present. + + If the host doesn't have a Windows var (e.g. running tests on Unix), + we don't fabricate one. + """ + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Windows"), + patch.dict("os.environ", {"PATH": "C:/Windows"}, clear=True), + ): + env = await _connect_capturing_env(stdio_client) + assert "USERPROFILE" not in env + assert "APPDATA" not in env + + +class TestUnixEnvPassthrough: + """Forward Unix-essential env vars to the subprocess. + + On Unix, `HOME` falls back to `getpwuid()` but many tools still read it + from env first. Forwarding it avoids per-tool quirks. + """ + + async def test_should_pass_home_when_present_on_unix(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Linux"), + patch.dict("os.environ", {"PATH": "/usr/bin", "HOME": "/home/test"}, clear=True), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("HOME") == "/home/test" + + async def test_should_pass_lang_for_locale_aware_tools_on_unix(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Darwin"), + patch.dict( + "os.environ", + {"PATH": "/usr/bin", "HOME": "/Users/test", "LANG": "en_US.UTF-8"}, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("LANG") == "en_US.UTF-8" + + async def test_should_not_pass_windows_vars_when_running_on_unix(self, stdio_client): + """Windows-only vars must not bleed through on Unix hosts.""" + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Linux"), + patch.dict( + "os.environ", + {"PATH": "/usr/bin", "HOME": "/h", "USERPROFILE": "should-not-leak"}, + clear=True, + ), + ): + env = await _connect_capturing_env(stdio_client) + assert "USERPROFILE" not in env + + +class TestUserSuppliedEnvWins: + """User-supplied env (from server config) must override host passthrough.""" + + async def test_should_let_user_env_override_passthrough_value(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Linux"), + patch.dict( + "os.environ", + {"PATH": "/usr/bin", "HOME": "/host/home"}, + clear=True, + ), + patch.object( + stdio_client, + "_get_or_create_session", + new=AsyncMock( + return_value=SimpleNamespace(list_tools=AsyncMock(return_value=SimpleNamespace(tools=[]))) + ), + ), + ): + await stdio_client._connect_to_server("echo hi", env={"HOME": "/user/override"}) + assert dict(stdio_client._connection_params.env).get("HOME") == "/user/override" + + +class TestLegacyEnvKeysPreserved: + """Don't drop keys the previous behavior guaranteed (avoids silent regressions).""" + + async def test_should_still_pass_path_and_debug(self, stdio_client): + with ( + patch("lfx.base.mcp.util.platform.system", return_value="Linux"), + patch.dict("os.environ", {"PATH": "/usr/bin"}, clear=True), + ): + env = await _connect_capturing_env(stdio_client) + assert env.get("PATH") == "/usr/bin" + assert env.get("DEBUG") == "true" diff --git a/src/lfx/src/lfx/base/mcp/util.py b/src/lfx/src/lfx/base/mcp/util.py index 01ce8b7dc5..4fb992316f 100644 --- a/src/lfx/src/lfx/base/mcp/util.py +++ b/src/lfx/src/lfx/base/mcp/util.py @@ -1547,6 +1547,73 @@ class MCPSessionManager: self._context_to_session.pop(context_id, None) +# OS-essential env vars to forward into MCP stdio subprocesses. +# +# Why this list exists: Node-based MCP servers (e.g. `@wonderwhy-er/desktop-commander`) +# call `os.homedir()` at module load. On Windows that reads `USERPROFILE` (or the +# `HOMEDRIVE`+`HOMEPATH` fallback); when the var is missing the call returns +# `null` and any `path.join(homedir, ...)` blows up — surfacing to MCP clients +# as an opaque "Connection closed" / TaskGroup sub-exception. The same is true +# of locale, temp-dir, and shell-related vars for other server families. +# +# Why not pass `os.environ` wholesale: the parent Langflow process may carry +# secrets (AWS_*, GITHUB_TOKEN, OPENAI_API_KEY, ...) that have no business +# being inherited by an arbitrary MCP server subprocess. We curate. +# +# User-supplied env (from server config) still wins via merge order. +_PASSTHROUGH_ENV_VARS_WINDOWS: tuple[str, ...] = ( + # `os.homedir()` lookup chain on Windows. + "USERPROFILE", + "HOMEDRIVE", + "HOMEPATH", + # Per-user app data directories — many MCP servers store config here. + "APPDATA", + "LOCALAPPDATA", + # Temp dirs — required by tools that spawn helper subprocesses. + "TEMP", + "TMP", + # Required for `cmd /c` and child Win32 APIs. + "SYSTEMROOT", + "SYSTEMDRIVE", + "WINDIR", + "COMSPEC", + # Without PATHEXT, `cmd /c npx ...` cannot resolve `npx.cmd` from PATH. + "PATHEXT", +) +_PASSTHROUGH_ENV_VARS_UNIX: tuple[str, ...] = ( + # `os.homedir()` reads HOME first; getpwuid() is only the fallback. + "HOME", + "USER", + "LOGNAME", + # Shell-aware tools. + "SHELL", + "TERM", + "TMPDIR", + # Locale — affects text encoding, sort order, error messages. + "LANG", + "LC_ALL", + "LC_CTYPE", +) + + +def _build_subprocess_env(user_env: dict[str, str] | None) -> dict[str, str]: + """Build the env dict passed to MCP stdio subprocesses. + + Always-set keys: ``DEBUG=true`` (legacy) and ``PATH`` (so tools resolve). + Forwarded if present in the parent env: a curated list of OS-essential + vars (``USERPROFILE`` etc on Windows, ``HOME`` etc on Unix). Anything in + ``user_env`` overrides everything else. + """ + passthrough = _PASSTHROUGH_ENV_VARS_WINDOWS if platform.system() == "Windows" else _PASSTHROUGH_ENV_VARS_UNIX + env: dict[str, str] = {"DEBUG": "true", "PATH": os.environ.get("PATH", "")} + for var in passthrough: + if var in os.environ: + env[var] = os.environ[var] + if user_env: + env.update(user_env) + return env + + class MCPStdioClient: def __init__(self, component_cache=None): self.session: ClientSession | None = None @@ -1576,7 +1643,7 @@ class MCPStdioClient: from mcp import StdioServerParameters command = shlex.split(command_str) - env_data: dict[str, str] = {"DEBUG": "true", "PATH": os.environ["PATH"], **(env or {})} + env_data: dict[str, str] = _build_subprocess_env(env) if platform.system() == "Windows": server_params = StdioServerParameters(