diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 07d944f77f..69fa685138 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -1721,7 +1721,7 @@ def _run_post_setup(post_setup_key: str): " npx not found - install Chromium manually: npx agent-browser install --with-deps" ) return - install_cmd = [npx_bin, "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps"] + install_cmd = [npx_bin, "--ignore-scripts", "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps"] else: install_cmd = [browser_cmd, "install", "--with-deps"] diff --git a/tests/hermes_cli/test_tools_config.py b/tests/hermes_cli/test_tools_config.py index 17d4c3ad06..1f5328ec58 100644 --- a/tests/hermes_cli/test_tools_config.py +++ b/tests/hermes_cli/test_tools_config.py @@ -384,6 +384,8 @@ class TestAgentBrowserPostSetup: def test_chromium_already_installed_skips_subprocess(self): with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "tools.browser_tool.node_tool_runnable", return_value=True + ), patch( "subprocess.run" ) as run, patch( "tools.browser_tool._chromium_installed", return_value=True @@ -398,6 +400,8 @@ class TestAgentBrowserPostSetup: def test_docker_with_missing_chromium_warns_instead_of_installing(self): with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "tools.browser_tool.node_tool_runnable", return_value=True + ), patch( "subprocess.run" ) as run, patch( "tools.browser_tool._chromium_installed", return_value=False @@ -440,7 +444,11 @@ class TestAgentBrowserPostSetup: string as a single argv element).""" with patch( "shutil.which", - side_effect=lambda name: "/usr/bin/npx" if name == "npx" else None, + # accepts the `path=` kwarg _resolve_npx_bin's extended-path rung + # calls shutil.which with, not just the bare-PATH positional form. + side_effect=lambda name, path=None: "/usr/bin/npx" if name == "npx" else None, + ), patch( + "tools.browser_tool.node_tool_runnable", return_value=True ), patch("subprocess.run") as run, patch( "tools.browser_tool._chromium_installed", return_value=False ), patch( @@ -455,7 +463,7 @@ class TestAgentBrowserPostSetup: run.assert_called_once() assert run.call_args.args[0] == [ - "/usr/bin/npx", "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", + "/usr/bin/npx", "--ignore-scripts", "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", ] def test_installs_chromium_via_npx_resolved_only_through_extended_path(self): @@ -483,7 +491,7 @@ class TestAgentBrowserPostSetup: run.assert_called_once() assert run.call_args.args[0] == [ - hermes_npx, "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", + hermes_npx, "--ignore-scripts", "-y", AGENT_BROWSER_NPX_SPEC, "install", "--with-deps", ] def test_warns_instead_of_crashing_when_npx_unresolvable_after_all(self): @@ -537,6 +545,8 @@ class TestAgentBrowserPostSetup: import tools.browser_tool as _bt with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "tools.browser_tool.node_tool_runnable", return_value=True + ), patch( "subprocess.run", return_value=SimpleNamespace(returncode=0, stdout="", stderr=""), ), patch( @@ -560,6 +570,8 @@ class TestAgentBrowserPostSetup: import tools.browser_tool as _bt with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "tools.browser_tool.node_tool_runnable", return_value=True + ), patch( "subprocess.run", return_value=SimpleNamespace( returncode=1, stdout="", stderr="line1\nline2\nfatal: network error" @@ -586,6 +598,8 @@ class TestAgentBrowserPostSetup: def test_install_timeout_warns_without_raising(self): with patch("shutil.which", return_value="/usr/bin/npx"), patch( + "tools.browser_tool.node_tool_runnable", return_value=True + ), patch( "subprocess.run", side_effect=subprocess.TimeoutExpired(cmd=["npx"], timeout=600), ), patch( diff --git a/tests/tools/test_browser_chromium_autoinstall.py b/tests/tools/test_browser_chromium_autoinstall.py index 6e0cfa4de4..0c60f6ecd0 100644 --- a/tests/tools/test_browser_chromium_autoinstall.py +++ b/tests/tools/test_browser_chromium_autoinstall.py @@ -64,6 +64,7 @@ class TestInstall: monkeypatch.setattr(bt, "_build_browser_env", lambda: {}) monkeypatch.setattr(bt, "_chromium_installed", lambda: True) monkeypatch.setattr(bt.shutil, "which", lambda _, path=None: "/usr/bin/npx") + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: True) captured = {} monkeypatch.setattr( @@ -72,7 +73,9 @@ class TestInstall: ) assert bt._maybe_autoinstall_chromium() is True - assert captured["cmd"] == ["/usr/bin/npx", "-y", bt.AGENT_BROWSER_NPX_SPEC, "install"] + assert captured["cmd"] == [ + "/usr/bin/npx", "--ignore-scripts", "-y", bt.AGENT_BROWSER_NPX_SPEC, "install", + ] assert "--with-deps" not in captured["cmd"] def test_nonzero_exit_returns_false(self, monkeypatch): diff --git a/tests/tools/test_browser_homebrew_paths.py b/tests/tools/test_browser_homebrew_paths.py index ea28150376..1467d2cbaa 100644 --- a/tests/tools/test_browser_homebrew_paths.py +++ b/tests/tools/test_browser_homebrew_paths.py @@ -220,6 +220,7 @@ class TestFindAgentBrowser: with patch("shutil.which", side_effect=mock_which), \ patch("os.path.isdir", return_value=False), \ patch.object(Path, "exists", mock_path_exists), \ + patch("tools.browser_tool.node_tool_runnable", return_value=True), \ patch( "tools.browser_tool._discover_homebrew_node_dirs", return_value=[], @@ -399,10 +400,11 @@ class TestRunBrowserCommandPathConstruction: _run_browser_command("test-task", "navigate", ["https://example.com"]) assert captured_cmd is not None - assert captured_cmd[:4] == [ - "/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, + assert captured_cmd[:5] == [ + "/opt/hermes/node/bin/npx", "--ignore-scripts", "--prefer-offline", "-y", + AGENT_BROWSER_NPX_SPEC, ] - assert captured_cmd[4:8] == ["--session", "test-session", "--json", "navigate"] + assert captured_cmd[5:9] == ["--session", "test-session", "--json", "navigate"] def test_subprocess_path_includes_termux_fallback_dirs(self, tmp_path): """Termux fallback dirs should survive browser PATH rebuilding.""" @@ -484,9 +486,95 @@ class TestRunChromeFallbackCommandNpxResolution: assert captured_cmds, "expected at least one Popen call for the chrome-fallback session" first_cmd = captured_cmds[0] - assert first_cmd[:4] == [ - "/opt/hermes/node/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, + assert first_cmd[:5] == [ + "/opt/hermes/node/bin/npx", "--ignore-scripts", "--prefer-offline", "-y", + AGENT_BROWSER_NPX_SPEC, ] - assert first_cmd[4] == "--engine" and first_cmd[5] == "chrome" - assert first_cmd[6] == "--session" and first_cmd[7].startswith("h_cfb_") - assert first_cmd[8] == "--json" + assert first_cmd[5] == "--engine" and first_cmd[6] == "chrome" + assert first_cmd[7] == "--session" and first_cmd[8].startswith("h_cfb_") + assert first_cmd[9] == "--json" + + +class TestResolveNpxBinPriority: + """The extended/managed search must be checked before a bare ambient + PATH lookup, so a broken/unexpected system npx can't shadow a healthy + Hermes-managed one — and each candidate must be validated (actually + runs) before being trusted, mirroring _find_agent_browser's own + validation discipline for agent-browser itself.""" + + def test_prefers_managed_extended_path_over_bare_path(self, monkeypatch): + import tools.browser_tool as bt + + monkeypatch.setattr(bt, "_merge_browser_path", lambda _p: "/hermes/node/bin") + monkeypatch.setattr( + bt.shutil, "which", + lambda cmd, path=None: ( + "/hermes/node/bin/npx" if path == "/hermes/node/bin" + else "/usr/local/bin/npx" + ), + ) + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: True) + + assert bt._resolve_npx_bin() == "/hermes/node/bin/npx" + + def test_falls_back_to_bare_path_when_managed_candidate_is_broken(self, monkeypatch): + import tools.browser_tool as bt + + monkeypatch.setattr(bt, "_merge_browser_path", lambda _p: "/hermes/node/bin") + monkeypatch.setattr( + bt.shutil, "which", + lambda cmd, path=None: ( + "/hermes/node/bin/npx" if path == "/hermes/node/bin" + else "/usr/local/bin/npx" + ), + ) + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: p == "/usr/local/bin/npx") + + assert bt._resolve_npx_bin() == "/usr/local/bin/npx" + + def test_returns_none_when_nothing_runnable(self, monkeypatch): + import tools.browser_tool as bt + + monkeypatch.setattr(bt, "_merge_browser_path", lambda _p: "") + monkeypatch.setattr(bt.shutil, "which", lambda cmd, path=None: "/usr/local/bin/npx") + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: False) + + assert bt._resolve_npx_bin() is None + + def test_skips_extended_lookup_when_merge_browser_path_returns_empty(self, monkeypatch): + """_merge_browser_path("") returning a falsy string (no extended + candidate dirs found on disk) must short-circuit straight to the + bare-PATH rung — shutil.which must not be called with a path="" + kwarg (which would silently mean "search cwd only" on some + platforms rather than "no extended search"), and node_tool_runnable + must only be asked about the one real candidate.""" + import tools.browser_tool as bt + + which_calls = [] + + def fake_which(cmd, path=None): + which_calls.append((cmd, path)) + return "/usr/bin/npx" if path is None else None + + monkeypatch.setattr(bt, "_merge_browser_path", lambda _p: "") + monkeypatch.setattr(bt.shutil, "which", fake_which) + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: p == "/usr/bin/npx") + + assert bt._resolve_npx_bin() == "/usr/bin/npx" + assert which_calls == [("npx", None)] + + def test_falls_back_to_bare_path_when_extended_dir_has_no_npx(self, monkeypatch): + """A non-empty extended search PATH that simply doesn't contain an + npx binary (shutil.which returns None there) must fall through to + the bare-PATH rung rather than treating "no extended npx" the same + as "extended npx found but broken".""" + import tools.browser_tool as bt + + monkeypatch.setattr(bt, "_merge_browser_path", lambda _p: "/hermes/node/bin") + monkeypatch.setattr( + bt.shutil, "which", + lambda cmd, path=None: None if path == "/hermes/node/bin" else "/usr/bin/npx", + ) + monkeypatch.setattr(bt, "node_tool_runnable", lambda p: True) + + assert bt._resolve_npx_bin() == "/usr/bin/npx" diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 8a8e3f49b6..b831efbec5 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -54,6 +54,7 @@ import functools import json import logging import os +import signal import re import subprocess import shutil @@ -70,6 +71,7 @@ from hermes_constants import ( get_hermes_home, get_hermes_home_override, hermes_home_key, + node_tool_runnable, ) from utils import env_int, is_truthy_value from hermes_cli.config import DEFAULT_CONFIG, cfg_get @@ -1237,7 +1239,10 @@ def _run_chrome_fallback_command( # WinError 193. if _is_npx_agent_browser_sentinel(browser_cmd): _npx_bin = _resolve_npx_bin() or "npx" - cmd_prefix = [_npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] + # --ignore-scripts: AGENT_BROWSER_NPX_SPEC is a floating ^0.26.0 range, + # not an exact pin — a compromised future 0.26.x patch must not get to + # run its own install-time lifecycle scripts on this machine. + cmd_prefix = [_npx_bin, "--ignore-scripts", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] else: cmd_prefix = [browser_cmd] base_args = cmd_prefix + ["--engine", "chrome", "--session", tmp_session, "--json"] @@ -2371,19 +2376,23 @@ def _agent_browser_candidate_present(path: str | None) -> bool: def _resolve_npx_bin() -> Optional[str]: - """Resolve the npx binary via the same PATH + extended-PATH cascade - _find_agent_browser uses, so callers that need npx's actual path - (rather than the "npx agent-browser" sentinel) can't diverge from what - _find_agent_browser itself would have found. A bare ``shutil.which("npx")`` - misses Hermes-managed-Node-only setups where npx only resolves via the - extended fallback PATH (Homebrew, $HERMES_HOME/node, etc.). + """Resolve a runnable npx binary, preferring the Hermes-managed/Homebrew + extended search over a bare ambient PATH lookup. + + Checking bare PATH first would let a broken or unrelated system npx + shadow a healthy Hermes-managed one with no recovery — every candidate + is therefore validated with ``node_tool_runnable`` (the same check + ``find_hermes_node_executable`` uses to self-heal a managed Node tree) + before being trusted, falling through to the next candidate otherwise. """ - npx_path = shutil.which("npx") - if npx_path: - return npx_path extended_path = _merge_browser_path("") if extended_path: - return shutil.which("npx", path=extended_path) + extended_npx = shutil.which("npx", path=extended_path) + if extended_npx and node_tool_runnable(extended_npx): + return extended_npx + npx_path = shutil.which("npx") + if npx_path and node_tool_runnable(npx_path): + return npx_path return None @@ -2508,6 +2517,68 @@ def _find_agent_browser(*, validate: bool = True) -> str: ) +def _kill_process_tree(proc: "subprocess.Popen") -> None: + """Best-effort kill of *proc* and any descendants it spawned. + + ``Popen.kill()`` only signals the direct child PID. npm/npx routinely + fork further processes (registry-fetch helpers, npm's own lifecycle + runner, agent-browser's own detached daemon grandchild) that can survive + a plain ``kill()`` of the top-level PID and keep a ``capture_output``-style + pipe open, hanging the caller's ``communicate()`` past the nominal + timeout — the same orphaned-pipe hazard already hit in production on + POSIX (see ``tools/process_registry.py``'s ``_reader_loop``, issue + #68915: a backgrounded grandchild inheriting a pipe's write end kept it + from ever reaching EOF). That hazard is cross-platform, not + Windows-specific; what *is* Windows-specific is the lack of a remedy + other than killing the tree — anonymous pipes there don't support + overlapped I/O, so there's no ``select()``-style non-blocking read to + poll around a stuck grandchild the way POSIX can. Killing the whole + process group/tree the child was launched into reaches those + descendants on both platforms. + + Fires SIGTERM then SIGKILL back-to-back with no grace period between + them (unlike ``tools/mcp_stdio_watchdog.py``'s ``_terminate_process_group``, + which waits between signals because it's reacting to a live daemon being + orphaned). By the time this is called, the caller has already burned its + full timeout budget waiting for a graceful exit — there's nothing to gain + from waiting again here, only more delay on an already-timed-out call. + """ + if os.name == "nt": + try: + subprocess.run( + ["taskkill", "/PID", str(proc.pid), "/T", "/F"], + check=False, + capture_output=True, + stdin=subprocess.DEVNULL, + ) + except Exception: + pass + return + # os.killpg/signal.SIGKILL don't exist on Windows; this branch is + # POSIX-only (the `os.name == "nt"` check above already returns first + # on Windows), but resolve them defensively via getattr anyway so an + # accidental future refactor that drops that guard degrades to a plain + # kill() instead of AttributeError — same discipline as + # tools/mcp_stdio_watchdog.py's _terminate_process_group. + killpg = getattr(os, "killpg", None) + if killpg is None: # windows-footgun: ok - non-POSIX fallback + try: + proc.kill() + except Exception: + pass + return + try: + pgid = os.getpgid(proc.pid) + except (ProcessLookupError, OSError): + return + sigkill = getattr(signal, "SIGKILL", signal.SIGTERM) + for sig in (signal.SIGTERM, sigkill): + try: + killpg(pgid, sig) + except (ProcessLookupError, PermissionError, OSError): + return + + def warm_agent_browser_npx_cache(timeout: float = 60.0) -> bool: """Best-effort pre-fetch of the agent-browser npm package via npx. @@ -2521,6 +2592,16 @@ def warm_agent_browser_npx_cache(timeout: float = 60.0) -> bool: property agent-browser had while it was an eager root dependency — without re-entangling it with the workspace graph. + Runs a credential-scrubbed, PATH-propagated environment matching every + other agent-browser subprocess spawn (see ``_build_browser_env``) — + this used to inherit the full parent environment, including every + provider/gateway credential Hermes holds, while running registry-fetched + npm code on every ``hermes update`` (the GHSA-m4m8-xjp4-5rmm class of + risk ``_build_browser_env`` exists specifically to prevent). Runs in its + own process group and kills the *whole* group — not just the top-level + npx PID — on timeout, since a surviving descendant can otherwise hold a + capture pipe open past the nominal deadline (see ``_kill_process_tree``). + Fire-and-forget: never raises, always safe to call opportunistically. Returns True only if npx actually ran successfully (npx unavailable, a timeout, or a nonzero exit all return False silently). @@ -2528,22 +2609,54 @@ def warm_agent_browser_npx_cache(timeout: float = 60.0) -> bool: npx_bin = _resolve_npx_bin() if not npx_bin: return False + + env = _build_browser_env() + env["PATH"] = _merge_browser_path(env.get("PATH", "")) + + popen_kwargs: dict = { + "stdout": subprocess.PIPE, + "stderr": subprocess.PIPE, + "text": True, + "env": env, + "creationflags": windows_hide_flags(), + } + if os.name == "posix": + popen_kwargs["start_new_session"] = True + else: + popen_kwargs["creationflags"] |= getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + + cmd = [ + npx_bin, + # --ignore-scripts: AGENT_BROWSER_NPX_SPEC is a floating ^0.26.0 + # range, not an exact pin — a compromised future 0.26.x patch must + # not get to run its own install-time lifecycle scripts here. + "--ignore-scripts", + # --prefer-offline: once cached, repeat `hermes update`/`doctor + # --fix` runs shouldn't hit the registry just to re-confirm + # "latest" is still latest — that would defeat the point of + # warming the cache in the first place. + "--prefer-offline", + "-y", + AGENT_BROWSER_NPX_SPEC, + "--version", + ] try: - result = subprocess.run( - # --prefer-offline: once cached, repeat `hermes update`/`doctor - # --fix` runs shouldn't hit the registry just to re-confirm - # "latest" is still latest — that would defeat the point of - # warming the cache in the first place. - [npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, "--version"], - capture_output=True, - text=True, - timeout=timeout, - check=False, - creationflags=windows_hide_flags(), - ) - return result.returncode == 0 + proc = subprocess.Popen(cmd, stdin=subprocess.DEVNULL, **popen_kwargs) except Exception: return False + try: + proc.communicate(timeout=timeout) + return proc.returncode == 0 + except subprocess.TimeoutExpired: + _kill_process_tree(proc) + try: + proc.communicate(timeout=5) + except Exception: + pass + return False + except Exception: + _kill_process_tree(proc) + return False def _extract_screenshot_path_from_text(text: str) -> Optional[str]: @@ -2671,7 +2784,8 @@ def _run_browser_command( # shutil.which("npx") is wrong here). if _is_npx_agent_browser_sentinel(browser_cmd): _npx_bin = _resolve_npx_bin() or "npx" - cmd_prefix = [_npx_bin, "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] + # --ignore-scripts: see _run_chrome_fallback_command's identical comment. + cmd_prefix = [_npx_bin, "--ignore-scripts", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC] else: cmd_prefix = [browser_cmd] @@ -4989,7 +5103,9 @@ def _maybe_autoinstall_chromium() -> bool: return False if _is_npx_agent_browser_sentinel(browser_cmd): - install_cmd = [_resolve_npx_bin() or "npx", "-y", AGENT_BROWSER_NPX_SPEC, "install"] + install_cmd = [ + _resolve_npx_bin() or "npx", "--ignore-scripts", "-y", AGENT_BROWSER_NPX_SPEC, "install", + ] else: install_cmd = [browser_cmd, "install"]