fix(#37878): scrub operator environment before launching cua-driver MCP
- Use _sanitize_subprocess_env() to filter Hermes-managed credentials from the cua-driver subprocess environment (issue #37878) - Prevents credential exfiltration to the third-party cua-driver binary - Aligns with existing pattern used by browser-tool and other tools - Add regression test to verify environment sanitization The cua-driver is a lower-trust MCP subprocess per SECURITY.md §2.3. Its inherited environment is now scrubbed by default, removing provider API keys, gateway tokens, and platform credentials that should not leak to third-party binaries. Fixes #37878
This commit is contained in:
parent
b39ec2fc37
commit
2e5c04aaf7
@ -1450,3 +1450,77 @@ class TestFocusAppFilterNoMatch:
|
|||||||
assert res.ok is True
|
assert res.ok is True
|
||||||
assert backend._active_pid == 200
|
assert backend._active_pid == 200
|
||||||
assert backend._active_window_id == 2
|
assert backend._active_window_id == 2
|
||||||
|
|
||||||
|
|
||||||
|
class TestCuaEnvironmentScrubbing:
|
||||||
|
"""Verify that cua-driver subprocess environment is sanitized (issue #37878)."""
|
||||||
|
|
||||||
|
def test_cua_session_sanitizes_provider_env_vars(self):
|
||||||
|
"""_CuaDriverSession._aenter() must sanitize sensitive env vars.
|
||||||
|
|
||||||
|
The cua-driver MCP subprocess should not inherit Hermes-managed credentials
|
||||||
|
or other sensitive environment variables — only runtime-required vars.
|
||||||
|
This is a regression test for issue #37878.
|
||||||
|
"""
|
||||||
|
from unittest.mock import MagicMock, patch, AsyncMock
|
||||||
|
from tools.computer_use.cua_backend import _CuaDriverSession, _AsyncBridge
|
||||||
|
import asyncio
|
||||||
|
|
||||||
|
bridge = _AsyncBridge()
|
||||||
|
session = _CuaDriverSession(bridge)
|
||||||
|
|
||||||
|
captured_env = {}
|
||||||
|
|
||||||
|
async def test_aenter():
|
||||||
|
# Set up test environment with both safe and blocked vars
|
||||||
|
test_env = {
|
||||||
|
"OPENAI_API_KEY": "sk-secret", # blocked
|
||||||
|
"PATH": "/usr/bin:/bin", # safe
|
||||||
|
"HOME": "/home/user", # safe
|
||||||
|
"SAFE_VAR": "allowed", # safe
|
||||||
|
}
|
||||||
|
|
||||||
|
with patch.dict(os.environ, test_env, clear=True):
|
||||||
|
with patch("tools.computer_use.cua_backend.cua_driver_binary_available",
|
||||||
|
return_value=True):
|
||||||
|
# Mock StdioServerParameters to capture the env arg
|
||||||
|
def capture_env(**kwargs):
|
||||||
|
captured_env.update(kwargs.get("env", {}))
|
||||||
|
# Return mock that works with async context manager
|
||||||
|
mock = MagicMock()
|
||||||
|
mock.__aenter__ = AsyncMock(return_value=(MagicMock(), MagicMock()))
|
||||||
|
mock.__aexit__ = AsyncMock(return_value=None)
|
||||||
|
return mock
|
||||||
|
|
||||||
|
with patch("mcp.StdioServerParameters", side_effect=capture_env), \
|
||||||
|
patch("mcp.client.stdio.stdio_client") as mock_stdio, \
|
||||||
|
patch("mcp.ClientSession") as mock_session_class, \
|
||||||
|
patch("contextlib.AsyncExitStack"):
|
||||||
|
|
||||||
|
# Setup mocks for stdio_client and ClientSession
|
||||||
|
mock_read = MagicMock()
|
||||||
|
mock_write = MagicMock()
|
||||||
|
mock_stdio.return_value.__aenter__ = AsyncMock(
|
||||||
|
return_value=(mock_read, mock_write))
|
||||||
|
mock_stdio.return_value.__aexit__ = AsyncMock(return_value=None)
|
||||||
|
|
||||||
|
mock_session = MagicMock()
|
||||||
|
mock_session.initialize = AsyncMock()
|
||||||
|
mock_session_class.return_value.__aenter__ = AsyncMock(
|
||||||
|
return_value=mock_session)
|
||||||
|
mock_session_class.return_value.__aexit__ = AsyncMock(return_value=None)
|
||||||
|
|
||||||
|
try:
|
||||||
|
await session._aenter()
|
||||||
|
except Exception:
|
||||||
|
pass # Mocks may raise, but env should be captured
|
||||||
|
|
||||||
|
asyncio.run(test_aenter())
|
||||||
|
|
||||||
|
# Verify blocked credentials are not in the passed env
|
||||||
|
assert "OPENAI_API_KEY" not in captured_env, \
|
||||||
|
"OPENAI_API_KEY should be stripped from cua-driver subprocess"
|
||||||
|
|
||||||
|
# Verify PATH is preserved (safe var)
|
||||||
|
assert "PATH" in captured_env or "SAFE_VAR" in captured_env, \
|
||||||
|
"At least one safe environment variable should be preserved"
|
||||||
|
|||||||
@ -270,6 +270,7 @@ class _CuaDriverSession:
|
|||||||
from contextlib import AsyncExitStack
|
from contextlib import AsyncExitStack
|
||||||
from mcp import ClientSession, StdioServerParameters
|
from mcp import ClientSession, StdioServerParameters
|
||||||
from mcp.client.stdio import stdio_client
|
from mcp.client.stdio import stdio_client
|
||||||
|
from tools.environments.local import _sanitize_subprocess_env
|
||||||
|
|
||||||
if not cua_driver_binary_available():
|
if not cua_driver_binary_available():
|
||||||
raise RuntimeError(cua_driver_install_hint())
|
raise RuntimeError(cua_driver_install_hint())
|
||||||
@ -277,7 +278,7 @@ class _CuaDriverSession:
|
|||||||
params = StdioServerParameters(
|
params = StdioServerParameters(
|
||||||
command=_CUA_DRIVER_CMD,
|
command=_CUA_DRIVER_CMD,
|
||||||
args=_CUA_DRIVER_ARGS,
|
args=_CUA_DRIVER_ARGS,
|
||||||
env={**os.environ},
|
env=_sanitize_subprocess_env(dict(os.environ)),
|
||||||
)
|
)
|
||||||
stack = AsyncExitStack()
|
stack = AsyncExitStack()
|
||||||
read, write = await stack.enter_async_context(stdio_client(params))
|
read, write = await stack.enter_async_context(stdio_client(params))
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user