From 047a45e41071f93ad4acd7880f32e3e372c27d6f Mon Sep 17 00:00:00 2001 From: "Zak B. Elep" Date: Tue, 11 Aug 2026 17:34:09 +0800 Subject: [PATCH] test(browser): cover warm_agent_browser_npx_cache's hardened behavior Full rewrite of test_browser_npx_warmup.py for the Popen-based credential-scrubbing, PATH-propagation, and process-tree-kill rework: argv shape, env scrubbing, PATH merge for managed-only npx, POSIX process-group creation, Windows CREATE_NEW_PROCESS_GROUP, whole-tree kill (not just the PID) on timeout with a bounded post-kill drain, and _kill_process_tree's own POSIX/Windows/failure paths directly. Also fixes test_windows_subprocess_no_window_flags.py's matching regression test, which still mocked subprocess.run and a shutil.which signature that didn't accept the path= kwarg _resolve_npx_bin's extended-path rung now passes; its creationflags assertion becomes a bitwise check since Windows now ORs CREATE_NEW_PROCESS_GROUP in alongside the console-hiding flag. --- ...test_windows_subprocess_no_window_flags.py | 28 +- tests/tools/test_browser_npx_warmup.py | 284 +++++++++++++++--- 2 files changed, 261 insertions(+), 51 deletions(-) diff --git a/tests/test_windows_subprocess_no_window_flags.py b/tests/test_windows_subprocess_no_window_flags.py index 060d17f0a2329..cc127452844c7 100644 --- a/tests/test_windows_subprocess_no_window_flags.py +++ b/tests/test_windows_subprocess_no_window_flags.py @@ -190,21 +190,37 @@ def test_shell_hooks_hide_hook_command_windows(monkeypatch): def test_agent_browser_npx_warmup_hides_npx_window(monkeypatch): + """warm_agent_browser_npx_cache spawns via subprocess.Popen (not .run, + since the T3 security-hardening rewrite added process-tree containment + via Popen + communicate()) — the console-hiding flag must still survive + that rewrite. On Windows the real implementation now ORs in + CREATE_NEW_PROCESS_GROUP alongside windows_hide_flags()'s bits (for + _kill_process_tree's taskkill /T to have a coherent tree to kill), so + this checks the CREATE_NO_WINDOW bit is present rather than exact + equality with the whole creationflags value.""" from tools import browser_tool captured = [] - def fake_run(cmd, **kwargs): - captured.append((cmd, kwargs)) - return _Completed(stdout="1.2.3\n") + class _FakePopen: + def __init__(self, cmd, **kwargs): + captured.append((cmd, kwargs)) + self.returncode = 0 - monkeypatch.setattr(browser_tool.shutil, "which", lambda name: "/usr/bin/npx") + def communicate(self, timeout=None): + return ("1.2.3\n", "") + + monkeypatch.setattr( + browser_tool.shutil, "which", + lambda name, path=None: "/usr/bin/npx", + ) + monkeypatch.setattr(browser_tool, "node_tool_runnable", lambda p: True) monkeypatch.setattr(browser_tool, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) - monkeypatch.setattr(browser_tool.subprocess, "run", fake_run) + monkeypatch.setattr(browser_tool.subprocess, "Popen", _FakePopen) assert browser_tool.warm_agent_browser_npx_cache() is True assert captured[0][0][0] == "/usr/bin/npx" - assert captured[0][1]["creationflags"] == _CREATE_NO_WINDOW + assert captured[0][1]["creationflags"] & _CREATE_NO_WINDOW == _CREATE_NO_WINDOW diff --git a/tests/tools/test_browser_npx_warmup.py b/tests/tools/test_browser_npx_warmup.py index 10d1e08dd7e27..2c8c18189023d 100644 --- a/tests/tools/test_browser_npx_warmup.py +++ b/tests/tools/test_browser_npx_warmup.py @@ -1,75 +1,269 @@ -"""Tests for tools.browser_tool.warm_agent_browser_npx_cache (#43564). +"""Tests for tools.browser_tool.warm_agent_browser_npx_cache (#43564, security +hardening follow-up on PR #44772 review). -agent-browser was moved out of root package.json dependencies and now -resolves lazily via `npx agent-browser`. warm_agent_browser_npx_cache() is -the fire-and-forget helper `hermes update` / `hermes doctor --fix` call to -pre-fetch it so the first real browser-tool invocation in a session doesn't -pay npx's registry-lookup cost. It must never raise and must accurately -report success/failure via its return value — hermes_cli/doctor.py's -`--fix` path branches on that return value; hermes_cli/update_cmd.py calls -it fire-and-forget inside its own try/except and ignores the result. +warm_agent_browser_npx_cache() is the fire-and-forget helper `hermes update` / +`hermes doctor --fix` call to pre-fetch agent-browser via npx so the first real +browser-tool invocation in a session doesn't pay npx's registry-lookup cost. +It must never raise, must accurately report success/failure via its return +value, must use a credential-scrubbed and PATH-propagated environment (it +runs registry-fetched, potentially install-scripted npm code on every +`hermes update` — not only when a browser tool is actually used), must pass +--ignore-scripts (AGENT_BROWSER_NPX_SPEC is a floating ^0.26.0 range, not an +exact pin), and must kill the whole process tree — not just the top-level +npx PID — on timeout. """ from __future__ import annotations import subprocess -from unittest.mock import patch +from unittest.mock import MagicMock, patch -from tools.browser_tool import AGENT_BROWSER_NPX_SPEC, warm_agent_browser_npx_cache +from tools.browser_tool import ( + AGENT_BROWSER_NPX_SPEC, + _kill_process_tree, + warm_agent_browser_npx_cache, +) -def test_returns_false_without_calling_subprocess_when_npx_missing(): - with patch("shutil.which", return_value=None), patch( - "subprocess.run" - ) as mock_run: +def _mock_proc(returncode=0, communicate_side_effect=None, pid=4242): + proc = MagicMock() + proc.pid = pid + if communicate_side_effect is not None: + proc.communicate.side_effect = communicate_side_effect + else: + proc.communicate.return_value = ("", "") + proc.returncode = returncode + return proc + + +def test_returns_false_without_spawning_when_npx_unresolvable(): + with patch("tools.browser_tool._resolve_npx_bin", return_value=None), patch( + "subprocess.Popen" + ) as mock_popen: assert warm_agent_browser_npx_cache() is False - mock_run.assert_not_called() + mock_popen.assert_not_called() -def test_invokes_npx_with_prefer_offline_version_check(): - with patch("shutil.which", return_value="/usr/bin/npx"), patch( - "subprocess.run", - return_value=subprocess.CompletedProcess([], 0, stdout="1.2.3\n", stderr=""), - ) as mock_run: +def test_invokes_npx_with_ignore_scripts_prefer_offline_and_pinned_spec(): + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), patch( + "subprocess.Popen", return_value=_mock_proc() + ) as mock_popen: assert warm_agent_browser_npx_cache() is True - mock_run.assert_called_once() - args, kwargs = mock_run.call_args - assert args[0] == ["/usr/bin/npx", "--prefer-offline", "-y", AGENT_BROWSER_NPX_SPEC, "--version"] - assert kwargs.get("check") is False + mock_popen.assert_called_once() + args, _kwargs = mock_popen.call_args + assert args[0] == [ + "/usr/bin/npx", "--ignore-scripts", "--prefer-offline", "-y", + AGENT_BROWSER_NPX_SPEC, "--version", + ] -def test_custom_timeout_is_forwarded_to_subprocess_run(): - with patch("shutil.which", return_value="/usr/bin/npx"), patch( - "subprocess.run", - return_value=subprocess.CompletedProcess([], 0, stdout="", stderr=""), - ) as mock_run: - warm_agent_browser_npx_cache(timeout=5.0) +def test_captures_stdout_and_stderr_instead_of_inheriting_parent_fds(): + """The npx registry fetch runs on every `hermes update` — its stdout/ + stderr must not bleed into the caller's own output (and, on POSIX, an + inherited fd is one more handle a runaway grandchild could hold open).""" + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen: + warm_agent_browser_npx_cache() - assert mock_run.call_args.kwargs.get("timeout") == 5.0 + _args, kwargs = mock_popen.call_args + assert kwargs.get("stdout") == subprocess.PIPE + assert kwargs.get("stderr") == subprocess.PIPE + + +def test_uses_credential_scrubbed_environment(): + """Must not inherit the full parent environment — matching every other + agent-browser subprocess spawn (_build_browser_env), not the ambient + os.environ with every provider/gateway credential Hermes holds.""" + scrubbed_env = {"PATH": "/scrubbed/bin", "SCRUBBED": "1"} + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("tools.browser_tool._build_browser_env", return_value=dict(scrubbed_env)), \ + patch("tools.browser_tool._merge_browser_path", side_effect=lambda p: p), \ + patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen: + warm_agent_browser_npx_cache() + + _args, kwargs = mock_popen.call_args + assert kwargs["env"]["SCRUBBED"] == "1" + assert "OPENAI_API_KEY" not in kwargs["env"] + + +def test_merges_extended_path_so_managed_only_npx_can_find_sibling_node(): + """If npx was resolved via the Hermes-managed/extended search (not the + ambient PATH), the child's own PATH must include that same directory — + npx's #!/usr/bin/env node shebang resolves `node` via the child's PATH + at exec time, not the resolving process's PATH.""" + with patch("tools.browser_tool._resolve_npx_bin", return_value="/opt/hermes/node/bin/npx"), \ + patch("tools.browser_tool._build_browser_env", return_value={"PATH": "/usr/bin"}), \ + patch( + "tools.browser_tool._merge_browser_path", + return_value="/opt/hermes/node/bin:/usr/bin", + ) as mock_merge, \ + patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen: + warm_agent_browser_npx_cache() + + mock_merge.assert_called_once_with("/usr/bin") + _args, kwargs = mock_popen.call_args + assert kwargs["env"]["PATH"] == "/opt/hermes/node/bin:/usr/bin" + + +def test_runs_in_its_own_process_group_on_posix(monkeypatch): + monkeypatch.setattr("os.name", "posix") + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen: + warm_agent_browser_npx_cache() + + _args, kwargs = mock_popen.call_args + assert kwargs.get("start_new_session") is True + + +def test_uses_new_process_group_creationflag_on_windows_instead_of_start_new_session(): + """start_new_session is a POSIX-only Popen kwarg (raises on Windows). + The Windows equivalent for _kill_process_tree's taskkill /T to have a + coherent tree to kill is CREATE_NEW_PROCESS_GROUP via creationflags.""" + with patch("os.name", "nt"), \ + patch("tools.browser_tool._resolve_npx_bin", return_value="C:\\npx.cmd"), \ + patch("tools.browser_tool._build_browser_env", return_value={"PATH": "C:\\Windows"}), \ + patch("tools.browser_tool._merge_browser_path", side_effect=lambda p: p), \ + patch("subprocess.Popen", return_value=_mock_proc()) as mock_popen: + warm_agent_browser_npx_cache() + + _args, kwargs = mock_popen.call_args + assert "start_new_session" not in kwargs + create_new_pgroup = getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0) + assert kwargs["creationflags"] & create_new_pgroup == create_new_pgroup + + +def test_timeout_kills_the_whole_process_tree_not_just_the_pid(): + """subprocess.Popen.kill() only signals the direct child; npm/npx can + fork descendants that survive it and hold a capture pipe open past the + nominal timeout. On timeout, the whole process group/tree must be + killed, not just the top-level PID.""" + proc = _mock_proc( + communicate_side_effect=[ + subprocess.TimeoutExpired(cmd=["npx"], timeout=60.0), ("", ""), + ] + ) + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("subprocess.Popen", return_value=proc), \ + patch("tools.browser_tool._kill_process_tree") as mock_kill: + assert warm_agent_browser_npx_cache(timeout=60.0) is False + + mock_kill.assert_called_once_with(proc) + assert proc.communicate.call_count == 2, ( + "must attempt a second, bounded communicate() after the kill to reap " + "the now-dead process and drain its pipes, not just abandon it" + ) + + +def test_timeout_cleanup_communicate_itself_raising_does_not_propagate(): + """The post-kill drain call is itself best-effort — if the process is + stuck badly enough that even the 5s cleanup communicate() times out (or + raises for any other reason), that must not escape and crash the + fire-and-forget caller (hermes_cli/doctor.py calls this bare).""" + proc = _mock_proc( + communicate_side_effect=[ + subprocess.TimeoutExpired(cmd=["npx"], timeout=60.0), + subprocess.TimeoutExpired(cmd=["npx"], timeout=5), + ] + ) + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("subprocess.Popen", return_value=proc), \ + patch("tools.browser_tool._kill_process_tree") as mock_kill: + assert warm_agent_browser_npx_cache(timeout=60.0) is False + + mock_kill.assert_called_once_with(proc) def test_returns_false_on_nonzero_exit(): - with patch("shutil.which", return_value="/usr/bin/npx"), patch( - "subprocess.run", - return_value=subprocess.CompletedProcess([], 1, stdout="", stderr="registry unreachable"), + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), patch( + "subprocess.Popen", return_value=_mock_proc(returncode=1) ): assert warm_agent_browser_npx_cache() is False -def test_returns_false_instead_of_raising_on_timeout(): - with patch("shutil.which", return_value="/usr/bin/npx"), patch( - "subprocess.run", - side_effect=subprocess.TimeoutExpired(cmd=["npx"], timeout=60.0), +def test_returns_false_instead_of_raising_on_popen_failure(): + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), patch( + "subprocess.Popen", side_effect=OSError("fork failed") ): assert warm_agent_browser_npx_cache() is False -def test_returns_false_instead_of_raising_on_unexpected_exception(): +def test_returns_false_instead_of_raising_on_unexpected_communicate_exception(): """Fire-and-forget contract: hermes_cli/doctor.py calls this bare (no - try/except of its own), so any exception inside the subprocess call - must be swallowed here, not propagated.""" - with patch("shutil.which", return_value="/usr/bin/npx"), patch( - "subprocess.run", side_effect=OSError("fork failed") - ): + try/except of its own), so any exception must be swallowed here.""" + proc = _mock_proc(communicate_side_effect=OSError("broken pipe")) + with patch("tools.browser_tool._resolve_npx_bin", return_value="/usr/bin/npx"), \ + patch("subprocess.Popen", return_value=proc), \ + patch("tools.browser_tool._kill_process_tree") as mock_kill: assert warm_agent_browser_npx_cache() is False + mock_kill.assert_called_once_with(proc) + + +class TestKillProcessTree: + def test_posix_kills_process_group_term_then_kill(self, monkeypatch): + import signal + + proc = MagicMock() + proc.pid = 999 + monkeypatch.setattr("os.name", "posix") + monkeypatch.setattr("os.getpgid", lambda pid: 999) + killpg_calls = [] + monkeypatch.setattr( + "os.killpg", lambda pgid, sig: killpg_calls.append((pgid, sig)) + ) + + _kill_process_tree(proc) + + assert killpg_calls == [(999, signal.SIGTERM), (999, signal.SIGKILL)] + + def test_posix_missing_process_returns_silently(self, monkeypatch): + proc = MagicMock() + proc.pid = 999 + monkeypatch.setattr("os.name", "posix") + + def _raise(pid): + raise ProcessLookupError() + + monkeypatch.setattr("os.getpgid", _raise) + + _kill_process_tree(proc) # must not raise + + def test_posix_sigterm_permission_denied_does_not_attempt_sigkill(self, monkeypatch): + """If SIGTERM itself is rejected (e.g. a stale pgid reused by an + unrelated, unkillable process), the loop must bail out rather than + plow ahead into a second signal against the wrong target.""" + import signal + + proc = MagicMock() + proc.pid = 999 + monkeypatch.setattr("os.name", "posix") + monkeypatch.setattr("os.getpgid", lambda pid: 999) + killpg_calls = [] + + def fake_killpg(pgid, sig): + killpg_calls.append((pgid, sig)) + raise PermissionError() + + monkeypatch.setattr("os.killpg", fake_killpg) + + _kill_process_tree(proc) # must not raise + + assert killpg_calls == [(999, signal.SIGTERM)] + + def test_windows_uses_taskkill_with_tree_and_force_flags(self, monkeypatch): + proc = MagicMock() + proc.pid = 4321 + monkeypatch.setattr("os.name", "nt") + with patch("subprocess.run") as mock_run: + _kill_process_tree(proc) + + mock_run.assert_called_once() + cmd = mock_run.call_args.args[0] + assert cmd == ["taskkill", "/PID", "4321", "/T", "/F"] + + def test_windows_taskkill_failure_does_not_raise(self, monkeypatch): + proc = MagicMock() + proc.pid = 4321 + monkeypatch.setattr("os.name", "nt") + with patch("subprocess.run", side_effect=OSError("taskkill missing")): + _kill_process_tree(proc) # must not raise