mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-18 14:52:04 +00:00
fix(computer-use): sanitize env on the 4 remaining cua-driver spawn sites (#59165)
PR #58889 fixed the CLI-fallback transport; review of that fix found the same leak class at four sibling spawn sites of the third-party cua-driver binary: - _resolve_mcp_invocation (cua-driver manifest): no env= at all — full parent environment inherited - cua_driver_update_check (check-update --json): telemetry env but no secret sanitization - doctor._drive_health_report (<binary> mcp Popen): telemetry env only - permissions._run (every macOS/Linux permission probe): telemetry env only All now route through _sanitize_subprocess_env(cua_driver_child_env()), matching the sanctioned MCP spawn and the #53503/#55709/#58889 strip-by- default policy for non-terminal spawns. Sanitization degrades gracefully (falls back to the telemetry env) so doctor/permission probes never break on an import error. 4 regression tests covering each site.
This commit is contained in:
parent
65117671e3
commit
0823230545
4 changed files with 161 additions and 5 deletions
120
tests/computer_use/test_cua_spawn_env_sanitization.py
Normal file
120
tests/computer_use/test_cua_spawn_env_sanitization.py
Normal file
|
|
@ -0,0 +1,120 @@
|
|||
"""Regression tests: every remaining cua-driver spawn site must sanitize the
|
||||
subprocess environment.
|
||||
|
||||
PR #58889 fixed the CLI-fallback transport; review of that fix found four
|
||||
sibling spawn sites still handing the third-party ``cua-driver`` binary the
|
||||
full parent environment (provider API keys included):
|
||||
|
||||
- ``cua_backend._resolve_mcp_invocation`` (``cua-driver manifest``) — no
|
||||
``env=`` at all
|
||||
- ``cua_backend.cua_driver_update_check`` (``check-update --json``) —
|
||||
telemetry env but no secret sanitization
|
||||
- ``doctor._drive_health_report`` (``<binary> mcp``) — telemetry env only
|
||||
- ``permissions._run`` (every permission probe) — telemetry env only
|
||||
"""
|
||||
|
||||
import json
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
SECRET = "sk-super-secret-should-not-leak"
|
||||
|
||||
|
||||
def _fake_completed_process(stdout: str) -> MagicMock:
|
||||
proc = MagicMock()
|
||||
proc.stdout = stdout
|
||||
proc.stderr = ""
|
||||
proc.returncode = 0
|
||||
return proc
|
||||
|
||||
|
||||
def _capture_run(captured, stdout=""):
|
||||
def fake_run(cmd, **kwargs):
|
||||
captured["cmd"] = cmd
|
||||
captured["env"] = kwargs.get("env")
|
||||
return _fake_completed_process(stdout)
|
||||
return fake_run
|
||||
|
||||
|
||||
def _assert_sanitized(captured):
|
||||
env = captured["env"]
|
||||
assert env is not None, "subprocess must receive an explicit env="
|
||||
assert "ANTHROPIC_API_KEY" not in env
|
||||
# Sanitization filters secrets, not everything — ordinary vars survive.
|
||||
assert env.get("PATH") == "/usr/bin:/bin"
|
||||
# Confirms the telemetry helper still ran (default: telemetry disabled).
|
||||
assert env.get("CUA_DRIVER_RS_TELEMETRY_ENABLED") == "0"
|
||||
|
||||
|
||||
def test_resolve_mcp_invocation_sanitizes_env(monkeypatch):
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", SECRET)
|
||||
monkeypatch.setenv("PATH", "/usr/bin:/bin")
|
||||
monkeypatch.delenv("HERMES_CUA_TELEMETRY", raising=False)
|
||||
|
||||
from tools.computer_use import cua_backend
|
||||
|
||||
captured = {}
|
||||
manifest = json.dumps({"mcp_invocation": {"command": "cua-driver", "args": ["mcp"]}})
|
||||
monkeypatch.setattr(
|
||||
cua_backend.subprocess, "run", _capture_run(captured, stdout=manifest)
|
||||
)
|
||||
|
||||
cmd, args = cua_backend._resolve_mcp_invocation("cua-driver")
|
||||
assert cmd == "cua-driver"
|
||||
_assert_sanitized(captured)
|
||||
|
||||
|
||||
def test_update_check_sanitizes_env(monkeypatch):
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", SECRET)
|
||||
monkeypatch.setenv("PATH", "/usr/bin:/bin")
|
||||
monkeypatch.delenv("HERMES_CUA_TELEMETRY", raising=False)
|
||||
|
||||
from tools.computer_use import cua_backend
|
||||
|
||||
captured = {}
|
||||
payload = json.dumps({
|
||||
"current_version": "1.0.0",
|
||||
"latest_version": "1.0.0",
|
||||
"update_available": False,
|
||||
})
|
||||
monkeypatch.setattr(
|
||||
cua_backend.subprocess, "run", _capture_run(captured, stdout=payload)
|
||||
)
|
||||
|
||||
cua_backend.cua_driver_update_check(timeout=1.0)
|
||||
_assert_sanitized(captured)
|
||||
|
||||
|
||||
def test_permissions_run_sanitizes_env(monkeypatch):
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", SECRET)
|
||||
monkeypatch.setenv("PATH", "/usr/bin:/bin")
|
||||
monkeypatch.delenv("HERMES_CUA_TELEMETRY", raising=False)
|
||||
|
||||
from tools.computer_use import permissions
|
||||
|
||||
captured = {}
|
||||
monkeypatch.setattr(
|
||||
permissions.subprocess, "run", _capture_run(captured, stdout="{}")
|
||||
)
|
||||
|
||||
permissions._run("cua-driver", "doctor", "--json", timeout=1.0)
|
||||
_assert_sanitized(captured)
|
||||
|
||||
|
||||
def test_doctor_sanitized_env_helper(monkeypatch):
|
||||
"""_drive_health_report spawns via Popen; assert the env helper it uses
|
||||
strips secrets (mocking the whole JSON-RPC handshake is not worth it)."""
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", SECRET)
|
||||
monkeypatch.setenv("PATH", "/usr/bin:/bin")
|
||||
monkeypatch.delenv("HERMES_CUA_TELEMETRY", raising=False)
|
||||
|
||||
from tools.computer_use import doctor
|
||||
import inspect
|
||||
|
||||
env = doctor._sanitized_cua_env()
|
||||
assert "ANTHROPIC_API_KEY" not in env
|
||||
assert env.get("PATH") == "/usr/bin:/bin"
|
||||
assert env.get("CUA_DRIVER_RS_TELEMETRY_ENABLED") == "0"
|
||||
|
||||
# The Popen spawn site must actually use the sanitized helper.
|
||||
src = inspect.getsource(doctor._drive_health_report)
|
||||
assert "_sanitized_cua_env()" in src
|
||||
|
|
@ -160,10 +160,15 @@ def _resolve_mcp_invocation(
|
|||
not refuse to start just because the discovery hop failed.
|
||||
"""
|
||||
try:
|
||||
from tools.environments.local import _sanitize_subprocess_env
|
||||
proc = subprocess.run(
|
||||
[driver_cmd, "manifest"],
|
||||
capture_output=True, text=True, timeout=timeout,
|
||||
stdin=subprocess.DEVNULL,
|
||||
# cua-driver is a third-party binary — never hand it provider
|
||||
# API keys via inherited env (same policy as the MCP and CLI
|
||||
# fallback spawns below; #53503/#55709/#58889 lineage).
|
||||
env=_sanitize_subprocess_env(cua_driver_child_env()),
|
||||
)
|
||||
except Exception:
|
||||
return driver_cmd, list(_CUA_DRIVER_ARGS)
|
||||
|
|
@ -246,6 +251,7 @@ def cua_driver_update_check(*, timeout: float = 8.0) -> Optional[Dict[str, Any]]
|
|||
raises.
|
||||
"""
|
||||
try:
|
||||
from tools.environments.local import _sanitize_subprocess_env
|
||||
proc = subprocess.run(
|
||||
[_CUA_DRIVER_CMD, "check-update", "--json"],
|
||||
capture_output=True, text=True, timeout=timeout,
|
||||
|
|
@ -253,7 +259,9 @@ def cua_driver_update_check(*, timeout: float = 8.0) -> Optional[Dict[str, Any]]
|
|||
# stdin-reading mode rather than erroring — DEVNULL gives them EOF
|
||||
# so they exit fast instead of blocking until the timeout.
|
||||
stdin=subprocess.DEVNULL,
|
||||
env=cua_driver_child_env(),
|
||||
# Sanitized like every other cua-driver spawn: third-party
|
||||
# binary, no inherited provider keys (#53503/#55709/#58889).
|
||||
env=_sanitize_subprocess_env(cua_driver_child_env()),
|
||||
)
|
||||
except Exception:
|
||||
return None
|
||||
|
|
|
|||
|
|
@ -52,6 +52,23 @@ def _cua_child_env() -> Dict[str, str]:
|
|||
return dict(os.environ)
|
||||
|
||||
|
||||
def _sanitized_cua_env() -> Dict[str, str]:
|
||||
"""Telemetry-policy env with Hermes provider secrets stripped.
|
||||
|
||||
cua-driver is a third-party binary — it must never inherit provider
|
||||
API keys (#53503/#55709/#58889 lineage). Falls back to the unsanitized
|
||||
telemetry env if the sanitizer can't be imported, so doctor keeps
|
||||
working in stripped-down environments.
|
||||
"""
|
||||
env = _cua_child_env()
|
||||
try:
|
||||
from tools.environments.local import _sanitize_subprocess_env
|
||||
|
||||
return _sanitize_subprocess_env(env)
|
||||
except Exception:
|
||||
return env
|
||||
|
||||
|
||||
def _drive_health_report(
|
||||
binary: str,
|
||||
*,
|
||||
|
|
@ -87,7 +104,7 @@ def _drive_health_report(
|
|||
encoding="utf-8",
|
||||
errors="replace",
|
||||
bufsize=1,
|
||||
env=_cua_child_env(),
|
||||
env=_sanitized_cua_env(),
|
||||
)
|
||||
try:
|
||||
# 1. initialize
|
||||
|
|
|
|||
|
|
@ -47,13 +47,24 @@ def _driver_cmd(override: Optional[str]) -> str:
|
|||
|
||||
|
||||
def _child_env() -> Dict[str, str]:
|
||||
"""cua-driver child env honoring the Hermes telemetry opt-in policy."""
|
||||
"""cua-driver child env: telemetry opt-in policy + secret sanitization.
|
||||
|
||||
cua-driver is a third-party binary — it must never inherit provider
|
||||
API keys (#53503/#55709/#58889 lineage). Each layer degrades
|
||||
gracefully so permission probes never break on a helper import error.
|
||||
"""
|
||||
try:
|
||||
from tools.computer_use.cua_backend import cua_driver_child_env
|
||||
|
||||
return cua_driver_child_env()
|
||||
env = cua_driver_child_env()
|
||||
except Exception:
|
||||
return dict(os.environ)
|
||||
env = dict(os.environ)
|
||||
try:
|
||||
from tools.environments.local import _sanitize_subprocess_env
|
||||
|
||||
return _sanitize_subprocess_env(env)
|
||||
except Exception:
|
||||
return env
|
||||
|
||||
|
||||
def _run(binary: str, *args: str, timeout: float) -> subprocess.CompletedProcess:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue