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.
This commit is contained in:
Zak B. Elep
Teknium
parent
737e7aa562
commit
047a45e410
|
|
@ -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
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue