diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 7a486bb367..88d4a51d67 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -313,6 +313,238 @@ class TestActiveVenvMarkerStripping: assert "CONDA_PREFIX" in _ACTIVE_VENV_MARKER_VARS +class TestPythonpathSelectiveStrip: + """PYTHONPATH site-packages stripping for cross-version ABI safety (#74817). + + The Desktop Electron app injects the Hermes venv's site-packages + (Python 3.11) into PYTHONPATH. When this leaks into subprocesses + running a different Python (e.g. 3.13), 3.11 C extensions appear on + sys.path and crash with ImportError. ``_strip_mismatched_site_packages`` + surgically removes only the dangerous entries, preserving user paths. + """ + + def test_hermes_venv_site_packages_stripped(self): + """A site-packages entry under the Hermes venv is removed.""" + from tools.environments.local import _strip_mismatched_site_packages + import sys + + # Construct a path that looks like the Hermes venv site-packages. + # Use the running interpreter's version so it hits the "under Hermes + # venv" check (check 2), not the cross-version check (check 1). + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + env = { + "PYTHONPATH": os.pathsep.join([venv_sp, "/home/user/my-lib"]), + } + _strip_mismatched_site_packages(env) + assert "PYTHONPATH" in env + entries = env["PYTHONPATH"].split(os.pathsep) + assert venv_sp not in entries + assert "/home/user/my-lib" in entries + + def test_user_pythonpath_preserved(self): + """User PYTHONPATH entries pass through untouched.""" + from tools.environments.local import _strip_mismatched_site_packages + user_pp = os.pathsep.join(["/opt/my-lib", "/another/path"]) + env = {"PYTHONPATH": user_pp} + _strip_mismatched_site_packages(env) + assert env.get("PYTHONPATH") == user_pp + + def test_cross_version_site_packages_stripped(self): + """A python3.12/site-packages entry is stripped even if NOT under the + Hermes venv path - simulates a leak from systemd or another source.""" + from tools.environments.local import _strip_mismatched_site_packages + import sys + + # Use a version different from the running interpreter. + running_major = sys.version_info[0] + running_minor = sys.version_info[1] + # Pick a guaranteed-different version. + other_minor = running_minor + 1 if running_minor < 20 else running_minor - 1 + other_ver = f"python{running_major}.{other_minor}" + + mismatched_sp = f"/opt/other-venv/lib/{other_ver}/site-packages" + env = { + "PYTHONPATH": os.pathsep.join([mismatched_sp, "/home/user/my-lib"]), + } + _strip_mismatched_site_packages(env) + assert "PYTHONPATH" in env + entries = env["PYTHONPATH"].split(os.pathsep) + assert mismatched_sp not in entries + assert "/home/user/my-lib" in entries + + def test_cross_major_version_stripped(self): + """A python2.7/site-packages entry is always stripped.""" + from tools.environments.local import _strip_mismatched_site_packages + env = { + "PYTHONPATH": "/old/lib/python2.7/site-packages:/home/user/lib", + } + _strip_mismatched_site_packages(env) + entries = env["PYTHONPATH"].split(os.pathsep) + assert "/old/lib/python2.7/site-packages" not in entries + assert "/home/user/lib" in entries + + def test_windows_backslash_paths(self): + """Windows-style backslash paths with site-packages are handled. + + On Windows, os.pathsep is ';'. We mock it so the test runs + correctly on POSIX CI.""" + from tools.environments.local import _strip_mismatched_site_packages + import sys + + # Construct a Windows-style path with a different Python version. + running_major = sys.version_info[0] + running_minor = sys.version_info[1] + other_minor = running_minor + 1 if running_minor < 20 else running_minor - 1 + other_ver = f"python{running_major}.{other_minor}" + + mismatched_win = f"C:\\venv\\lib\\{other_ver}\\site-packages" + user_win = "D:\\user\\lib" + env = { + "PYTHONPATH": ";".join([mismatched_win, user_win]), + } + # Mock os.pathsep to ';' (Windows) just for the strip call. + with patch("os.pathsep", ";"): + _strip_mismatched_site_packages(env) + assert "PYTHONPATH" in env + entries = env["PYTHONPATH"].split(";") + assert mismatched_win not in entries + assert user_win in entries + + def test_empty_pythonpath_unchanged(self): + """An empty PYTHONPATH is a no-op (falsy -> early return).""" + from tools.environments.local import _strip_mismatched_site_packages + env = {"PYTHONPATH": ""} + _strip_mismatched_site_packages(env) + # Empty string is falsy, so the function returns early without + # modifying the dict. The key stays as-is (empty string). + assert env.get("PYTHONPATH") == "" + + def test_no_pythonpath_key(self): + """Missing PYTHONPATH key is a no-op.""" + from tools.environments.local import _strip_mismatched_site_packages + env = {"PATH": "/usr/bin"} + _strip_mismatched_site_packages(env) + assert "PYTHONPATH" not in env + + def test_all_entries_stripped_removes_key(self): + """If all entries are stripped, PYTHONPATH key is removed entirely.""" + from tools.environments.local import _strip_mismatched_site_packages + import sys + + running_major = sys.version_info[0] + running_minor = sys.version_info[1] + other_minor = running_minor + 1 if running_minor < 20 else running_minor - 1 + other_ver = f"python{running_major}.{other_minor}" + + env = {"PYTHONPATH": f"/a/lib/{other_ver}/site-packages"} + _strip_mismatched_site_packages(env) + assert "PYTHONPATH" not in env + + def test_make_run_env_strips_hermes_venv_pythonpath(self): + """_make_run_env strips Hermes venv site-packages from PYTHONPATH.""" + from tools.environments.local import _make_run_env + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + with patch.dict(os.environ, { + "PATH": "/usr/bin:/bin", + "PYTHONPATH": os.pathsep.join([venv_sp, "/home/user/my-lib"]), + }, clear=True): + run_env = _make_run_env({}) + pp = run_env.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert venv_sp not in entries + assert "/home/user/my-lib" in entries + + def test_sanitize_subprocess_env_strips_hermes_venv_pythonpath(self): + """_sanitize_subprocess_env strips Hermes venv site-packages.""" + from tools.environments.local import _sanitize_subprocess_env + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + base = { + "PATH": "/usr/bin", + "HOME": "/home/user", + "PYTHONPATH": os.pathsep.join([venv_sp, "/home/user/my-lib"]), + } + result = _sanitize_subprocess_env(base) + pp = result.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert venv_sp not in entries + assert "/home/user/my-lib" in entries + + def test_hermes_subprocess_env_strips_hermes_venv_pythonpath(self): + """hermes_subprocess_env strips Hermes venv site-packages.""" + from tools.environments.local import hermes_subprocess_env + import sys + + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + venv_sp = str( + __import__("pathlib").Path(sys.prefix) / "lib" / pyver / "site-packages" + ) + with patch.dict(os.environ, { + "PATH": "/usr/bin:/bin", + "HOME": "/home/user", + "PYTHONPATH": os.pathsep.join([venv_sp, "/home/user/my-lib"]), + }, clear=True): + result = hermes_subprocess_env() + pp = result.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert venv_sp not in entries + assert "/home/user/my-lib" in entries + + def test_scrub_child_env_strips_mismatched_pythonpath(self): + """execute_code's _scrub_child_env path: after scrubbing, mismatched + site-packages entries should be stripped when _strip_mismatched_site_packages + is applied (as the spawn path does).""" + from tools.code_execution_tool import _scrub_child_env + from tools.environments.local import _strip_mismatched_site_packages + import sys + + running_major = sys.version_info[0] + running_minor = sys.version_info[1] + other_minor = running_minor + 1 if running_minor < 20 else running_minor - 1 + other_ver = f"python{running_major}.{other_minor}" + + mismatched_sp = f"/opt/other-venv/lib/{other_ver}/site-packages" + source = { + "PATH": "/usr/bin", + "HOME": "/home/user", + "PYTHONPATH": os.pathsep.join([mismatched_sp, "/home/user/my-lib"]), + } + scrubbed = _scrub_child_env(source) + # The scrubber passes PYTHONPATH through (it's in _SAFE_ENV_PREFIXES). + assert "PYTHONPATH" in scrubbed + # Now apply the selective strip (as the spawn path does). + _strip_mismatched_site_packages(scrubbed) + pp = scrubbed.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert mismatched_sp not in entries + assert "/home/user/my-lib" in entries + + def test_repo_root_stripped(self): + """The Hermes repo root entry is stripped from PYTHONPATH.""" + from tools.environments.local import _strip_mismatched_site_packages, _hermes_repo_root + repo_root = str(_hermes_repo_root) + env = { + "PYTHONPATH": os.pathsep.join([repo_root, "/home/user/my-lib"]), + } + _strip_mismatched_site_packages(env) + pp = env.get("PYTHONPATH", "") + entries = pp.split(os.pathsep) if pp else [] + assert repo_root not in entries + assert "/home/user/my-lib" in entries + + class TestProfileScopedPassthrough: def test_make_run_env_uses_active_profile_for_passthrough(self, monkeypatch): """Allowlisted values must come from the routed profile, not os.environ.""" diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 3836b203d9..c5749c8c92 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1483,6 +1483,16 @@ def execute_code( # external venv; exposing Hermes's site-packages to that interpreter # can mix incompatible compiled extensions (for example, Python 3.12 # NumPy with a Python 3.9 project interpreter). + # Before re-injecting PYTHONPATH, strip any mismatched site-packages + # entries that leaked through _scrub_child_env (PYTHONPATH is in + # _SAFE_ENV_PREFIXES so it passes the scrub). Cross-version entries + # (e.g. python3.11 site-packages injected by systemd/Electron) would + # poison the sandbox's sys.path with ABI-incompatible C extensions + # (#74817); Hermes venv/repo-root entries are redundant because the + # correct ones are re-added below, gated on the child interpreter + # actually being the Hermes environment. + from tools.environments.local import _strip_mismatched_site_packages + _strip_mismatched_site_packages(child_env) _hermes_root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _existing_pp = child_env.get("PYTHONPATH", "") _pp_parts = [tmpdir] diff --git a/tools/environments/local.py b/tools/environments/local.py index de2a6e0346..d7378ea55a 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -346,6 +346,10 @@ _HERMES_PROVIDER_ENV_BLOCKLIST = _build_provider_env_blocklist() # to a different Python version overwrites it and breaks the gateway). The # Hermes venv stays reachable via PATH (its bin dir is first), so stripping # these markers is safe and only prevents the cross-project clobber (#23473). +# +# PYTHONPATH is NOT included here — it's handled by +# _strip_mismatched_site_packages() which surgically removes only site-packages +# paths that don't match the current Python ABI, preserving user-set entries. _ACTIVE_VENV_MARKER_VARS = ("VIRTUAL_ENV", "CONDA_PREFIX") @@ -506,6 +510,8 @@ def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = Non for _marker in _ACTIVE_VENV_MARKER_VARS: sanitized.pop(_marker, None) + _strip_mismatched_site_packages(sanitized) + _apply_windows_msys_bash_env_defaults(sanitized) sanitized = _scrub_delegated_child_kanban_env(sanitized) @@ -635,6 +641,8 @@ def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str for _marker in _ACTIVE_VENV_MARKER_VARS: env.pop(_marker, None) + _strip_mismatched_site_packages(env) + _apply_windows_msys_bash_env_defaults(env) # Cross-session leak guard, same as the terminal spawn paths: this helper @@ -1321,6 +1329,8 @@ def _make_run_env(env: dict) -> dict: for _marker in _ACTIVE_VENV_MARKER_VARS: run_env.pop(_marker, None) + _strip_mismatched_site_packages(run_env) + _apply_windows_msys_bash_env_defaults(run_env) run_env = _scrub_delegated_child_kanban_env(run_env) @@ -1328,6 +1338,222 @@ def _make_run_env(env: dict) -> dict: return run_env +def _is_path_under(child: Path, parent: Path) -> bool: + """Return True if *child* is the same as or under *parent*. + + Uses ``os.path.normcase`` so the comparison is case-insensitive on + Windows (NTFS is case-insensitive by default) and case-sensitive on + POSIX, matching filesystem semantics. Paths are NOT resolved against + disk (no ``.resolve()``) so non-existent paths - common in test mocks + and in PYTHONPATH entries pointing at yet-to-be-created dirs - work + correctly. ``Path.resolve(strict=False)`` would also touch the + filesystem to resolve symlinks, which we deliberately avoid. + """ + c_parts = [os.path.normcase(p) for p in child.parts] + p_parts = [os.path.normcase(p) for p in parent.parts] + if len(c_parts) < len(p_parts): + return False + return c_parts[: len(p_parts)] == p_parts + + +# --- Hermes venv / repo-root detection (module-level, computed once) --- + +#: The running interpreter's own venv root. On a venv Python ``sys.prefix`` +#: points at the venv root (e.g. ``.../venv``); on a system Python it points +#: at ``/usr`` or similar. We only strip site-packages under this path when +#: the interpreter is actually inside a venv (``sys.prefix != sys.base_prefix``). +_hermes_venv_root: Path = Path(sys.prefix) + +#: The Hermes repository root - two levels up from this file +#: (``tools/environments/local.py`` -> ``tools/`` -> repo root). This is the +#: directory the Electron app prepends to PYTHONPATH so the backend can do +#: ``import tools``, ``import hermes_cli``, etc. Subprocesses that are NOT +#: the Hermes backend don't need it and it can shadow local packages. +_hermes_repo_root: Path = Path(__file__).resolve().parents[1] + +#: Whether the current interpreter is running inside a venv. On Python 3.3+ +#: ``sys.base_prefix != sys.prefix`` indicates a venv (or virtualenv). +#: ``sys.real_prefix`` is the old virtualenv (<20) marker. +_in_venv: bool = ( + getattr(sys, "base_prefix", sys.prefix) != sys.prefix + or hasattr(sys, "real_prefix") +) + +#: Cached set of site-packages directories that belong to the running +#: interpreter's own venv. Computed lazily (once) because ``site`` import +#: and path construction are not free and this function is called on every +#: subprocess spawn. +_hermes_site_packages: list[Path] | None = None + + +def _get_hermes_site_packages() -> list[Path]: + """Return the site-packages dirs of the running interpreter's venv. + + Uses ``site.getsitepackages()`` when available for robustness (it respects + ``.pth`` rewrites and platform conventions), with a manual fallback that + constructs the canonical path from ``sys.prefix`` for POSIX and Windows. + """ + global _hermes_site_packages + if _hermes_site_packages is not None: + return _hermes_site_packages + + result: list[Path] = [] + try: + import site + for sp in site.getsitepackages(): + result.append(Path(sp)) + except Exception: + pass + + # Fallback: construct manually. On POSIX: + # sys.prefix / lib / python{X.Y} / site-packages + # On Windows: + # sys.prefix / Lib / site-packages + if not result: + if _IS_WINDOWS: + result.append(Path(sys.prefix) / "Lib" / "site-packages") + else: + pyver = f"python{sys.version_info[0]}.{sys.version_info[1]}" + result.append(Path(sys.prefix) / "lib" / pyver / "site-packages") + + _hermes_site_packages = result + return result + + +# Regex to extract a Python version marker (e.g. ``python3.11``) from a path. +# Matches ``python3.11``, ``python3.13``, etc. as a path component - i.e. +# preceded by a path separator (``/`` or ``\``) or string start, and followed +# by a separator or string end. This is cross-platform: it works with both +# POSIX forward-slash paths and Windows backslash paths regardless of the +# host OS, so a POSIX host correctly detects version markers in Windows-style +# paths (important for testing and for edge cases like WSL). +_PYVER_IN_PATH_RE = re.compile(r"(?:^|[\\/])python(\d+)\.(\d+)(?:[\\/]|$)") + +# Regex to detect ``site-packages`` as a path component (not a substring of +# a longer directory name). Same cross-platform separator handling. +_SITE_PACKAGES_RE = re.compile(r"(?:^|[\\/])site-packages(?:[\\/]|$)") + + +def _strip_mismatched_site_packages(env: dict) -> None: + """Remove mismatched site-packages paths from PYTHONPATH. + + The Desktop Electron process (and systemd units, gateway VBS launchers, + etc.) inject the Hermes venv's site-packages path (e.g. + ``.../venv/lib/python3.11/site-packages``) into ``PYTHONPATH`` so the + Hermes backend can import its packages. When this ``PYTHONPATH`` leaks + into subprocesses running a **different** Python version (e.g. 3.13), + the 3.11 C extensions appear on ``sys.path`` ahead of the correct 3.13 + versions and crash with ``ImportError`` (``PIL._imaging``, + ``cryptography``, etc.). + + Rather than stripping ``PYTHONPATH`` entirely - which would discard + legitimate user entries (Nix uses ``PYTHONPATH`` for plugin discovery, + users set it for custom library paths) - this function surgically + removes only the dangerous entries: + + 1. **Cross-version site-packages** - any entry whose path contains a + ``python{X.Y}/site-packages`` component where ``{X.Y}`` differs from + the running interpreter's version. This catches ALL leak sources + (Electron, systemd, gateway scripts) with a single version check, + regardless of the venv path. + + 2. **Hermes venv site-packages** (no version marker or same-version) - + entries that live under the running interpreter's own venv + site-packages directory. These are redundant for subprocesses: the + Hermes backend discovers its packages via ``sys.path``, not via an + inherited env var. Only checked when running inside a venv. + + 3. **Hermes repo root** - the Electron app prepends the repo root + (parent of ``tools/``) to ``PYTHONPATH``. Subprocesses don't need + it and it can shadow local packages. + + User ``PYTHONPATH`` entries (``/opt/my-lib``, Nix plugin paths, etc.) + are always preserved. + """ + pp = env.get("PYTHONPATH") + if not pp: + return + + hermes_site_packages = _get_hermes_site_packages() if _in_venv else [] + running_major = sys.version_info[0] + running_minor = sys.version_info[1] + + kept: list[str] = [] + stripped: list[str] = [] + + for entry in pp.split(os.pathsep): + entry = entry.strip() + if not entry: + continue + + entry_path = Path(entry) + should_strip = False + + # --- Check 1: cross-version site-packages --- + # Look for a ``python{X.Y}`` path component and compare its version + # against the running interpreter. If they differ, the entry's + # C extensions are ABI-incompatible - strip unconditionally. + # We search the full entry string (not ``entry_path.parts``) because + # ``Path.parts`` only splits on the host OS separator, so a Windows + # backslash path on a POSIX host would be a single un-split part. + m = _PYVER_IN_PATH_RE.search(entry) + if m: + entry_major = int(m.group(1)) + entry_minor = int(m.group(2)) + if (entry_major, entry_minor) != (running_major, running_minor): + should_strip = True + if should_strip: + stripped.append(entry) + continue + + # --- Check 2: under Hermes venv site-packages --- + # The entry lives under the running interpreter's own venv + # site-packages. Redundant for subprocesses (they get their packages + # via sys.path, not PYTHONPATH) and a common leak vector. + # Use the regex (not ``entry_path.parts``) for cross-platform detection + # so Windows backslash paths are caught on a POSIX host. + if not should_strip and _SITE_PACKAGES_RE.search(entry): + for sp in hermes_site_packages: + if _is_path_under(entry_path, sp): + should_strip = True + break + if should_strip: + stripped.append(entry) + continue + + # --- Check 3: Hermes repo root --- + # The Electron app prepends the repo root so ``import tools`` works + # in the backend. Subprocesses don't need it and it can shadow + # local packages of the same name. + if not should_strip and _is_path_under(entry_path, _hermes_repo_root): + # Only strip if the entry IS the repo root or a direct package + # dir under it (e.g. ``.../hermes-agent/tools``). Don't strip + # arbitrary user paths that happen to be nested deeper. + rel = entry_path.resolve() if entry_path.exists() else entry_path + try: + depth = len(rel.relative_to(_hermes_repo_root).parts) + except (ValueError, OSError): + depth = -1 + if depth <= 1: + should_strip = True + + if should_strip: + stripped.append(entry) + else: + kept.append(entry) + + if kept: + env["PYTHONPATH"] = os.pathsep.join(kept) + else: + env.pop("PYTHONPATH", None) + + if stripped: + logger.debug( + "Stripped mismatched/Hermes-venv site-packages from PYTHONPATH: %s", + stripped, + ) + + def _read_terminal_shell_init_config() -> tuple[list[str], bool]: """Return (shell_init_files, auto_source_bashrc) from config.yaml.