Scrubbing credentials from subprocess environments

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #53937 fix(security): centralize credential-safe subprocess env (#29157)

  merged to main

  files: agent/copilot_acp_client.py, hermes_cli/dep_ensure.py, tests/tools/test_hermes_subprocess_env.py, tools/browser_tool.py, tools/environments/local.py, tools/lazy_deps.py, tui_gateway/server.py

  Show the diff

  • agent/copilot_acp_client.py

    @@ -23,6 +23,7 @@
     
     from agent.file_safety import get_read_block_error, is_write_denied
     from agent.redact import redact_sensitive_text
    +from tools.environments.local import hermes_subprocess_env
     
     ACP_MARKER_BASE_URL = "acp://copilot"
     _DEFAULT_TIMEOUT_SECONDS = 900.0
    @@ -94,7 +95,10 @@ def _resolve_home_dir() -> str:
     
     
     def _build_subprocess_env() -> dict[str, str]:
    -    env = os.environ.copy()
    +    # Copilot ACP is a model-driving CLI executor: it legitimately needs LLM
    +    # provider credentials. Route through the central helper so Tier-1 secrets
    +    # (gateway bot tokens, GitHub auth, infra) are still stripped (#29157).
    +    env = hermes_subprocess_env(inherit_credentials=True)
         home = _resolve_home_dir()
         env["HOME"] = home
         from hermes_constants import apply_subprocess_home_env

  • hermes_cli/dep_ensure.py

    @@ -23,6 +23,7 @@
     from pathlib import Path
     
     from hermes_constants import agent_browser_runnable
    +from tools.environments.local import hermes_subprocess_env
     
     _IS_WINDOWS = platform.system() == "Windows"
     
    @@ -148,7 +149,8 @@ def ensure_dependency(
         else:
             cmd = ["bash", str(script), "--ensure", dep]
     
    -    run_env = {**os.environ, "IS_INTERACTIVE": "false"}
    +    run_env = hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
    +    run_env["IS_INTERACTIVE"] = "false"
         result = subprocess.run(
             cmd,
             env=run_env,

  • tests/tools/test_hermes_subprocess_env.py

    @@ -0,0 +1,151 @@
    +"""Tests for hermes_subprocess_env() — the centralized credential-safe env
    +builder for the non-terminal subprocess spawn surface.
    +
    +Covers GHSA-m4m8-xjp4-5rmm / issue #29157: subprocesses spawned by the
    +gateway/browser/ACP/installer paths must not blindly inherit the operator's
    +full credential environment. Two tiers:
    +
    +  * Tier 1 (_ALWAYS_STRIP_KEYS): gateway bot tokens, GitHub auth, infra
    +    secrets — stripped even when inherit_credentials=True.
    +  * Tier 2 (_HERMES_PROVIDER_ENV_BLOCKLIST): LLM provider/tool keys — stripped
    +    unless the caller opts into inherit_credentials=True.
    +"""
    +
    +import os
    +from unittest.mock import patch
    +
    +from tools.environments.local import (
    +    hermes_subprocess_env,
    +    _ALWAYS_STRIP_KEYS,
    +    _HERMES_PROVIDER_ENV_FORCE_PREFIX,
    +)
    +
    +
    +_TIER1_SAMPLE = {
    +    "GH_TOKEN": "[redacted:secret-assignment]",
    +    "TELEGRAM_BOT_TOKEN": "[redacted:secret-assignment]",
    +    "SLACK_APP_TOKEN": "[redacted:secret-assignment]",
    +    "MODAL_TOKEN_SECRET": "[redacted:secret-assignment]",
    +    "HERMES_DASHBOARD_SESSION_TOKEN": "[redacted:secret-assignment]",
    +}
    +
    +_PROVIDER_SAMPLE = {
    +    "OPENAI_API_KEY": "[redacted:secret-assignment]",
    +    "ANTHROPIC_API_KEY": "[redacted:secret-assignment]",
    +    "OPENROUTER_API_KEY": "[redacted:secret-assignment]",
    +}
    +
    +_SAFE_SAMPLE = {
    +    "PATH": "/usr/bin:/bin",
    +    "HOME": "/home/user",
    +    "USER": "testuser",
    +    "MY_APP_VAR": "keep-me",
    +}
    +
    +
    +def _build(extra=None, *, inherit_credentials=[redacted:secret-assignment]
    +    env = dict(_SAFE_SAMPLE)
    +    if extra:
    +        env.update(extra)
    +    with patch.dict(os.environ, env, clear=True):
    +        return hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
    +
    +
    +class TestStripByDefault:
    +    def test_provider_keys_stripped_by_default(self):
    +        result = _build(_PROVIDER_SAMPLE)
    +        for var in _PROVIDER_SAMPLE:
    +            assert var not in result, f"{var} leaked with inherit_credentials=False"
    +
    +    def test_tier1_secrets_stripped_by_default(self):
    +        result = _build(_TIER1_SAMPLE)
    +        for var in _TIER1_SAMPLE:
    +            assert var not in result, f"{var} leaked (Tier-1) with inherit_credentials=False"
    +
    +    def test_safe_vars_preserved(self):
    +        result = _build()
    +        assert result["HOME"] == "/home/user"
    +        assert result["USER"] == "testuser"
    +        assert "PATH" in result
    +        assert result["MY_APP_VAR"] == "keep-me"
    +
    +    def test_force_prefix_hints_stripped(self):
    +        result = _build({f"{_HERMES_PROVIDER_ENV_FORCE_PREFIX}OPENAI_API_KEY": "sk-x"})
    +        assert f"{_HERMES_PROVIDER_ENV_FORCE_PREFIX}OPENAI_API_KEY" not in result
    +        assert "OPENAI_API_KEY" not in result
    +
    +    def test_pythonutf8_set(self):
    +        result = _build()
    +        assert result.get("PYTHONUTF8") == "1"
    +
    +
    +class TestInheritCredentials:
    +    def test_provider_keys_preserved_when_inheriting(self):
    +        result = _build(_PROVIDER_SAMPLE, inherit_credentials=True)
    +        for var, val in _PROVIDER_SAMPLE.items():
    +            assert result.get(var) == val, f"{var} should survive inherit_credentials=True"
    +
    +    def test_tier1_secrets_stripped_even_when_inheriting(self):
    +        """The whole point of Tier 1: gateway/GitHub/infra secrets never reach
    +        a child, even a model-driving CLI that legitimately needs provider keys."""
    +        result = _build({**_PROVIDER_SAMPLE, **_TIER1_SAMPLE}, inherit_credentials=True)
    +        for var in _TIER1_SAMPLE:
    +            assert var not in result, (
    +                f"{var} (Tier-1) must be stripped even with inherit_credentials=True"
    +            )
    +        # ...while provider keys survive.
    +        for var in _PROVIDER_SAMPLE:
    +            assert var in result
    +
    +    def test_pythonutf8_set_when_inheriting(self):
    +        assert _build(inherit_credentials=[redacted:secret-assignment]"PYTHONUTF8") == "1"
    +
    +
    +class TestTierInvariants:
    +    def test_tier1_always_stripped_both_paths(self):
    +        """Behavioral invariant: every Tier-1 key is stripped on BOTH the
    +        default path and the inherit_credentials=True path. This is what
    +        guarantees no gap, regardless of whether the key also happens to be
    +        in the provider blocklist."""
    +        sample = {k: f"secret-{k}" for k in _ALWAYS_STRIP_KEYS}
    +        for inherit in (False, True):
    +            result = _build(sample, inherit_credentials=[redacted:secret-assignment]
    +            leaked = {k for k in _ALWAYS_STRIP_KEYS if k in result}
    +            assert not leaked, (
    +                f"Tier-1 keys leaked with inherit_credentials={inherit}: {sorted(leaked)}"
    +            )
    +
    +    def test_tier1_covers_gateway_bot_token(self):
    +        assert "TELEGRAM_BOT_TOKEN" in _ALWAYS_STRIP_KEYS
    +
    +    def test_tier1_covers_github_auth(self):
    +        assert {"GH_TOKEN", "GITHUB_TOKEN"} <= _ALWAYS_STRIP_KEYS
    +
    +    def test_tier1_covers_infra_secrets(self):
    +        assert {"MODAL_TOKEN_ID", "MODAL_TOKEN_SECRET", "DAYTONA_API_KEY"} <= _ALWAYS_STRIP_KEYS
    +
    +
    +class TestBrowserPassthroughPattern:
    +    def test_browser_keys_recoverable_after_strip(self):
    +        """Browser tool pattern: strip everything, then re-add the browser
    +        backend keys agent-browser actually needs."""
    +        from tools.browser_tool import _BROWSER_PASSTHROUGH_KEYS
    +
    +        leaked = {
    +            "BROWSERBASE_API_KEY": "[redacted:secret-assignment]",
    +            "BROWSERBASE_PROJECT_ID": "bb-proj",
    +            "FIRECRAWL_API_KEY": "[redacted:secret-assignment]",
    +            "ANTHROPIC_API_KEY": "[redacted:secret-assignment]",
    +            "TELEGRAM_BOT_TOKEN": "[redacted:secret-assignment]",
    +        }
    +        with patch.dict(os.environ, {**_SAFE_SAMPLE, **leaked}, clear=True):
    +            env = hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
    +            for key in _BROWSER_PASSTHROUGH_KEYS:
    +                if key in os.environ:
    +                    env[key] = os.environ[key]
    +
    +        assert env["BROWSERBASE_API_KEY"] == "bb-key"
    +        assert env["FIRECRAWL_API_KEY"] == "fc-key"
    +        # Provider + gateway secrets must NOT come back.
    +        assert "ANTHROPIC_API_KEY" not in env
    +        assert "TELEGRAM_BOT_TOKEN" not in env

  • tools/browser_tool.py

    @@ -70,6 +70,39 @@
     from hermes_cli.config import DEFAULT_CONFIG, cfg_get
     from hermes_cli._subprocess_compat import windows_hide_flags
     
    +# Browser-specific tool keys passed through to the agent-browser subprocess
    +# AFTER credential stripping.  agent-browser is a Node process loading npm
    +# deps; handing it the full operator keyring (#29157 / GHSA-m4m8-xjp4-5rmm)
    +# means a compromised transitive dependency could read every Hermes secret
    +# straight out of process.env.  Strip by default, then re-add only the
    +# browser-backend keys the worker legitimately needs.
    +_BROWSER_PASSTHROUGH_KEYS: tuple[str, ...] = (
    +    "BROWSERBASE_API_KEY",
    +    "BROWSERBASE_PROJECT_ID",
    +    "BROWSER_USE_API_KEY",
    +    "FIRECRAWL_API_KEY",
    +    "FIRECRAWL_API_URL",
    +    "FIRECRAWL_BROWSER_TTL",
    +)
    +
    +
    +def _build_browser_env() -> dict:
    +    """Credential-scrubbed env for an agent-browser subprocess.
    +
    +    Strips Hermes-managed secrets (provider keys, gateway tokens, GitHub auth,
    +    infra secrets) then re-adds only the browser-backend keys the worker needs.
    +    The ``hermes_subprocess_env`` import is deferred to keep ``browser_tool``
    +    importable under test harnesses that load it against a stubbed ``tools``
    +    package (tests/tools/test_managed_browserbase_and_modal.py).
    +    """
    +    from tools.environments.local import hermes_subprocess_env
    +
    +    env = hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
    +    for _key in _BROWSER_PASSTHROUGH_KEYS:
    +        if _key in os.environ:
    +            env[_key] = os.environ[_key]
    +    return env
    +
     try:
         from tools.website_policy import check_website_access
     except Exception:
    @@ -872,7 +905,8 @@ def _run_chrome_fallback_command(
     
         task_socket_dir = os.path.join(_socket_safe_tmpdir(), f"agent-browser-{tmp_session}")
         os.makedirs(task_socket_dir, mode=0o700, exist_ok=True)
    -    browser_env = {**os.environ, "AGENT_BROWSER_SOCKET_DIR": task_socket_dir}
    +    browser_env = _build_browser_env()
    +    browser_env["AGENT_BROWSER_SOCKET_DIR"] = task_socket_dir
         browser_env["PATH"] = _merge_browser_path(browser_env.get("PATH", ""))
     
         if "AGENT_BROWSER_IDLE_TIMEOUT_MS" not in browser_env:
    @@ -2125,7 +2159,7 @@ def _run_browser_command(
             logger.debug("browser cmd=%s task=%s socket_dir=%s (%d chars)",
                          command, task_id, task_socket_dir, len(task_socket_dir))
     
    -        browser_env = {**os.environ}
    +        browser_env = _build_browser_env()
     
             # Ensure subprocesses inherit the same browser-specific PATH fallbacks
             # used during CLI discovery.

  • tools/environments/local.py

    @@ -250,6 +250,100 @@ def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = Non
         return sanitized
     
     
    +# Tier-1 secrets: stripped from EVERY spawned subprocess unconditionally —
    +# even when the caller opts into credential inheritance for a model-driving
    +# CLI (claude / codex / gemini).  These are not LLM provider credentials; no
    +# legitimate child Hermes spawns needs them, and they are the highest-value
    +# secrets to keep out of a compromised dependency's reach (gateway bot tokens,
    +# GitHub auth, remote-compute tokens, dashboard session secret).  The set is a
    +# narrow subset of _HERMES_PROVIDER_ENV_BLOCKLIST; provider keys are handled by
    +# the conditional Tier-2 strip in hermes_subprocess_env().
    +_ALWAYS_STRIP_KEYS: frozenset[str] = frozenset({
    +    # GitHub auth
    +    "GH_TOKEN",
    +    "GITHUB_TOKEN",
    +    "GITHUB_APP_ID",
    +    "GITHUB_APP_PRIVATE_KEY_PATH",
    +    "GITHUB_APP_INSTALLATION_ID",
    +    # Gateway / messaging bot tokens and access control
    +    "TELEGRAM_BOT_TOKEN",
    +    "DISCORD_BOT_TOKEN",
    +    "SLACK_BOT_TOKEN",
    +    "SLACK_APP_TOKEN",
    +    "SLACK_SIGNING_SECRET",
    +    "GATEWAY_ALLOWED_USERS",
    +    "GATEWAY_ALLOW_ALL_USERS",
    +    "HASS_TOKEN",
    +    "EMAIL_PASSWORD",
    +    "HERMES_DASHBOARD_SESSION_TOKEN",
    +    # Remote-compute / infrastructure secrets
    +    "MODAL_TOKEN_ID",
    +    "MODAL_TOKEN_SECRET",
    +    "DAYTONA_API_KEY",
    +})
    +
    +
    +def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str]:
    +    """Build a sanitized environment dict for a spawned subprocess.
    +
    +    Centralized helper for the **non-terminal** spawn surface (browser,
    +    ACP/CLI executors, computer-use driver, dep-ensure, TUI Node host,
    +    detached gateway).  Use this instead of copying ``os.environ`` directly
    +    so strip-by-default is the uniform policy across every spawn site, with a
    +    single source of truth (``_HERMES_PROVIDER_ENV_BLOCKLIST``).  The terminal
    +    / execute_code path keeps using :func:`_sanitize_subprocess_env`, which is
    +    skill-aware (``env_passthrough``); this helper is for spawns that have no
    +    skill-passthrough concept.
    +
    +    Two-tier stripping:
    +
    +    * **Tier 1 (always):** ``_ALWAYS_STRIP_KEYS`` — gateway bot tokens, GitHub
    +      auth, and remote-compute secrets are removed regardless of
    +      ``inherit_credentials``.  No child Hermes spawns legitimately needs them.
    +    * **Tier 2 (conditional):** the rest of ``_HERMES_PROVIDER_ENV_BLOCKLIST``
    +      (LLM provider API keys, tool secrets) is removed unless the caller passes
    +      ``inherit_credentials=[redacted:secret-assignment]
    +
    +    Pass ``inherit_credentials=[redacted:secret-assignment] **only** when the child legitimately
    +    needs LLM provider credentials — a user-blessed ``claude`` / ``codex`` /
    +    ``gemini`` CLI executor, or the TUI Node host that makes model calls.  The
    +    flag is grep-able for audit: ``grep -rn 'inherit_credentials=True'`` lists
    +    every spawn site that still receives provider credentials.
    +
    +    Callers that need a *specific* non-provider secret (e.g. the browser worker
    +    needs ``BROWSERBASE_API_KEY`` / ``FIRECRAWL_API_KEY``) should call with
    +    ``inherit_credentials=[redacted:secret-assignment] and copy just those keys back from
    +    ``os.environ`` into the returned dict.
    +    """
    +    env = os.environ.copy()
    +
    +    # Tier 1 — always strip.
    +    for key in _ALWAYS_STRIP_KEYS:
    +        env.pop(key, None)
    +    # Internal routing hints must never reach a child.
    +    for key in list(env):
    +        if key.startswith(_HERMES_PROVIDER_ENV_FORCE_PREFIX):
    +            env.pop(key, None)
    +
    +    if not inherit_credentials:
    +        # Tier 2 — strip provider/tool credentials unless explicitly inherited.
    +        for key in _HERMES_PROVIDER_ENV_BLOCKLIST:
    +            env.pop(key, None)
    +
    +    # Windows UTF-8 safety for spawned processes (#31420).
    +    env.setdefault("PYTHONUTF8", "1")
    +
    +    _inject_context_hermes_home(env)
    +    from hermes_constants import apply_subprocess_home_env
    +    apply_subprocess_home_env(env)
    +
    +    # Active-venv markers must not clobber another project's environment.
    +    for _marker in _ACTIVE_VENV_MARKER_VARS:
    +        env.pop(_marker, None)
    +
    +    return env
    +
    +
     def _find_bash() -> str:
         """Find bash for command execution."""
         if not _IS_WINDOWS:

  • tools/lazy_deps.py

    @@ -613,7 +613,9 @@ def _venv_pip_install(specs: tuple[str, ...], *, timeout: int = 300) -> _Install
     
         try:
             venv_root = Path(sys.executable).parent.parent
    -        uv_env = {**os.environ, "VIRTUAL_ENV": str(venv_root)}
    +        from tools.environments.local import hermes_subprocess_env
    +        uv_env = hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
    +        uv_env["VIRTUAL_ENV"] = str(venv_root)
     
             # Tier 1: uv (preferred — fast, doesn't need pip in the venv)
             uv_bin = shutil.which("uv")

  • tui_gateway/server.py

    @@ -25,6 +25,7 @@
     )
     from hermes_cli.env_loader import load_hermes_dotenv
     from utils import is_truthy_value
    +from tools.environments.local import hermes_subprocess_env
     from tui_gateway import git_probe
     from tui_gateway.transport import (
         StdioTransport,
    @@ -284,7 +285,9 @@ def __init__(self, session_key: str, model: str):
                 text=True,
                 bufsize=1,
                 cwd=os.getcwd(),
    -            env=os.environ.copy(),
    +            # slash_worker runs the Hermes agent → needs provider credentials.
    +            # Tier-1 secrets (gateway/GitHub/infra) are still stripped (#29157).
    +            env=hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
             )
             threading.Thread(target=self._drain_stdout, daemon=True).start()
             threading.Thread(target=self._drain_stderr, daemon=True).start()
    @@ -11131,7 +11134,9 @@ def _(rid, params: dict) -> dict:
                 text=True,
                 timeout=min(int(params.get("timeout", 240)), 600),
                 cwd=os.getcwd(),
    -            env=os.environ.copy(),
    +            # cli.exec runs `python -m hermes_cli.main` (can drive the agent) →
    +            # needs provider credentials. Tier-1 secrets still stripped (#29157).
    +            env=hermes_subprocess_env(inherit_credentials=[redacted:secret-assignment]
                 stdin=subprocess.DEVNULL,
             )
             parts = [r.stdout or "", r.stderr or ""]