fix(browser): harden npx agent-browser resolution

- --ignore-scripts on every real npx agent-browser invocation.
  AGENT_BROWSER_NPX_SPEC is a floating ^0.26.0 range, not an exact
  pin, and none of these sites passed it (unlike install.sh/
  install.ps1's own npm install of the same package). Verified against
  the real CLI: `npx --ignore-scripts --prefer-offline -y
  "agent-browser@^0.26.0" --version` resolves cleanly on npm
  11.19.0/node 26.
- _resolve_npx_bin() now checks the Hermes-managed/extended search
  before a bare ambient PATH lookup, validating each candidate with
  node_tool_runnable before trusting it — a bare PATH-first lookup let
  a broken system npx shadow a healthy managed one with no recovery.
- warm_agent_browser_npx_cache() now runs a credential-scrubbed,
  PATH-propagated environment (matching every other agent-browser
  subprocess spawn) instead of inheriting the full parent environment
  including every provider/gateway credential Hermes holds, and kills
  the whole process tree (not just the top-level npx PID) on timeout
  via the new _kill_process_tree helper, since a surviving descendant
  can otherwise hold a capture pipe open past the nominal deadline.
This commit is contained in:
Zak B. Elep
2026-08-11 17:27:34 +08:00
committed by Teknium
parent 7cb113d6c8
commit 03cdc3b20c
5 changed files with 260 additions and 39 deletions

View File

@@ -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"]

View File

@@ -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(

View File

@@ -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):

View File

@@ -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"

View File

@@ -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"]