diff --git a/tests/computer_use/test_cua_spawn_env_sanitization.py b/tests/computer_use/test_cua_spawn_env_sanitization.py new file mode 100644 index 000000000000..98be4176bf5c --- /dev/null +++ b/tests/computer_use/test_cua_spawn_env_sanitization.py @@ -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`` (`` 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 diff --git a/tools/computer_use/cua_backend.py b/tools/computer_use/cua_backend.py index a2a9314ea3dd..32dae8b7b861 100644 --- a/tools/computer_use/cua_backend.py +++ b/tools/computer_use/cua_backend.py @@ -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 diff --git a/tools/computer_use/doctor.py b/tools/computer_use/doctor.py index 1d557cd7d98f..38c1b43e91d8 100644 --- a/tools/computer_use/doctor.py +++ b/tools/computer_use/doctor.py @@ -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 diff --git a/tools/computer_use/permissions.py b/tools/computer_use/permissions.py index ab97b60ee662..d6fd0f3f91fd 100644 --- a/tools/computer_use/permissions.py +++ b/tools/computer_use/permissions.py @@ -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: