fix: harden subprocess env handling (scrub secrets, isolate browser sessions)

tests/tools/test_base_environment.py

@@ -6,7 +6,11 @@
 
 from unittest.mock import MagicMock
 
-from tools.environments.base import BaseEnvironment, _BoundedOutputCollector
+from tools.environments.base import (
+    BaseEnvironment,
+    _BoundedOutputCollector,
+    _snapshot_export_command,
+)
 
 
 class _TestableEnv(BaseEnvironment):
@@ -67,7 +71,8 @@ def test_basic_shape(self):
         assert "cd -- /tmp" in wrapped or "cd -- '/tmp'" in wrapped
         assert "eval 'echo hello'" in wrapped
         assert "__hermes_ec=$?" in wrapped
-        assert "export -p >" in wrapped
+        # env snapshot is secret-scrubbed (export -p | grep -Eiv ...) before the redirect
+        assert "export -p |" in wrapped
         # cwd travels via the stdout marker only — no temp-file write.
         assert "pwd -P >" not in wrapped
         assert env._cwd_marker in wrapped
@@ -140,8 +145,9 @@ def test_wrap_command_uses_atomic_temp_then_mv(self):
         env = _TestableEnv()
         env._snapshot_ready = True
         wrapped = env._wrap_command("echo hi", "/tmp")
-        # Env dump goes to a temp file, not directly over the live snapshot.
-        assert "export -p > " in wrapped
+        # Env dump is secret-scrubbed and goes to a temp file, not directly over
+        # the live snapshot.
+        assert "export -p |" in wrapped
         assert ".tmp." in wrapped
         # Then an atomic rename onto the real snapshot path.
         assert "mv -f " in wrapped
@@ -186,7 +192,7 @@ def test_wrap_command_mv_chained_on_export_success(self):
         env = _TestableEnv()
         env._snapshot_ready = True
         wrapped = env._wrap_command("echo hi", "/tmp")
-        assert "export -p > " in wrapped and "&& mv -f " in wrapped
+        assert "export -p |" in wrapped and "&& mv -f " in wrapped
         assert "rm -f " in wrapped  # temp cleanup on failure
 
     def test_init_session_bootstrap_also_atomic_and_bashpid(self):
@@ -217,7 +223,7 @@ def test_snapshot_writes_use_private_umask_after_user_command(self):
 
         assert "umask 077" in wrapped
         assert wrapped.index("eval 'echo hi'") < wrapped.index("umask 077")
-        assert wrapped.index("umask 077") < wrapped.index("export -p >")
+        assert wrapped.index("umask 077") < wrapped.index("export -p |")
 
     def test_init_session_bootstrap_uses_private_umask(self):
         env = _TestableEnv()
@@ -234,7 +240,7 @@ def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
             pass
         boot = captured.get("cmd", "")
         assert "umask 077" in boot
-        assert boot.index("umask 077") < boot.index("export -p >")
+        assert boot.index("umask 077") < boot.index("export -p |")
 
 
 class TestAtomicSnapshotConcurrencyBehavioral:
@@ -300,16 +306,50 @@ def test_failed_export_does_not_destroy_good_snapshot(self, tmp_path):
         snap = str(tmp_path / "snap.sh")
         _q = shlex.quote
         self._run(f"echo 'export GOOD=1' > {_q(snap)}")  # seed good snapshot
-        # Redirect export into an unwritable dir so the export side fails; mv
-        # must then NOT run (&&) and not clobber snap.
-        bad_tmp = _q("/nonexistent-dir/snap.tmp.") + "$BASHPID"
+        snap_tmp = _q(snap + ".tmp.") + "$BASHPID"
+        # Shadow the export builtin so ``export -p`` itself emits partial output
+        # and then fails.  The scrub pipeline must propagate that first-stage
+        # failure instead of publishing grep's successful output.
         script = (
-            f"{{ export -p > {bad_tmp} && mv -f {bad_tmp} {_q(snap)}; }} "
-            f"2>/dev/null || rm -f {bad_tmp} 2>/dev/null || true"
+            "export() { printf 'declare -x PARTIAL=bad\\n'; return 42; }; "
+            f"{{ {_snapshot_export_command(snap_tmp)} && "
+            f"mv -f {snap_tmp} {_q(snap)}; }} "
+            f"2>/dev/null || rm -f {snap_tmp} 2>/dev/null || true"
         )
         self._run(script)
         out = self._run(f"cat {_q(snap)}")
-        assert "export GOOD=1" in out.stdout, "good snapshot was destroyed by a failed export"
+        assert out.stdout == "export GOOD=1\n", "good snapshot was destroyed by a failed export"
+
+    def test_init_session_failed_export_preserves_existing_snapshot(self, tmp_path):
+        import subprocess
+
+        class FailingExportEnv(BaseEnvironment):
+            def get_temp_dir(self):
+                return str(tmp_path)
+
+            def _run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None):
+                failed_export = (
+                    "export() { printf 'declare -x PARTIAL=bad\\n'; return 42; }; "
+                )
+                return subprocess.Popen(
+                    ["/bin/bash", "-c", failed_export + cmd_string],
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.STDOUT,
+                    stdin=subprocess.DEVNULL,
+                    text=True,
+                )
+
+            def cleanup(self):
+                pass
+
+        env = FailingExportEnv(cwd=str(tmp_path), timeout=10)
+        snapshot = tmp_path / f"hermes-snap-{env._session_id}.sh"
+        snapshot.write_text("export GOOD=1\n")
+
+        env.init_session()
+
+        assert not env._snapshot_ready
+        assert snapshot.read_text() == "export GOOD=1\n"
 
 
 class TestSnapshotFileModes:

tests/tools/test_browser_tool_process_isolation.py

@@ -0,0 +1,81 @@
+import json
+import os
+
+import tools.browser_tool as browser_tool
+import tools.interrupt as interrupt
+
+
+def test_browser_popen_extra_starts_new_session_on_posix(monkeypatch):
+    monkeypatch.setattr(browser_tool.os, "name", "posix")
+
+    assert browser_tool._browser_popen_extra() == {"start_new_session": True}
+
+
+def test_browser_popen_extra_keeps_windows_no_console_without_new_group(monkeypatch):
+    class FakeStartupInfo:
+        def __init__(self):
+            self.dwFlags = 0
+
+    monkeypatch.setattr(browser_tool.os, "name", "nt")
+    monkeypatch.setattr(browser_tool.subprocess, "STARTUPINFO", FakeStartupInfo, raising=False)
+    monkeypatch.setattr(browser_tool.subprocess, "STARTF_USESTDHANDLES", 0x100, raising=False)
+
+    extra = browser_tool._browser_popen_extra()
+
+    assert extra["creationflags"] == 0x08000000
+    assert extra["close_fds"] is True
+    assert isinstance(extra["startupinfo"], FakeStartupInfo)
+    assert extra["startupinfo"].dwFlags == 0x100
+    assert "start_new_session" not in extra
+
+
+def test_browser_process_isolation_kwargs_cover_both_popen_paths(monkeypatch, tmp_path):
+    """Both production launch paths must pass the shared isolation kwargs."""
+    popen_calls = []
+    isolation = {"start_new_session": "sentinel"}
+
+    class FakeProcess:
+        returncode = 0
+
+        def __init__(self, argv, **kwargs):
+            popen_calls.append((argv, kwargs))
+            os.write(kwargs["stdout"], json.dumps({"success": True}).encode())
+
+        def wait(self, timeout=None):
+            return self.returncode
+
+        def kill(self):
+            self.returncode = -9
+
+    monkeypatch.setattr(browser_tool, "_browser_popen_extra", lambda: isolation)
+    monkeypatch.setattr(browser_tool.subprocess, "Popen", FakeProcess)
+    monkeypatch.setattr(browser_tool, "_find_agent_browser", lambda: "/bin/agent-browser")
+    monkeypatch.setattr(browser_tool, "_requires_real_termux_browser_install", lambda command: False)
+    monkeypatch.setattr(browser_tool, "_is_local_mode", lambda: False)
+    monkeypatch.setattr(browser_tool, "_get_browser_engine", lambda: "auto")
+    monkeypatch.setattr(browser_tool, "_get_session_info", lambda task_id: {
+        "session_name": "normal", "cdp_url": "ws://example.invalid"
+    })
+    monkeypatch.setattr(browser_tool, "_socket_safe_tmpdir", lambda: str(tmp_path))
+    monkeypatch.setattr(browser_tool, "_write_owner_pid", lambda *args: None)
+    monkeypatch.setattr(browser_tool, "_build_browser_env", lambda: {})
+    monkeypatch.setattr(browser_tool, "_merge_browser_path", lambda value: value)
+    monkeypatch.setattr(interrupt, "is_interrupted", lambda: False)
+
+    result = browser_tool._run_browser_command("task", "snapshot")
+    assert result["success"] is True
+    assert len(popen_calls) == 1
+    assert popen_calls[0][1]["start_new_session"] == "sentinel"
+
+    popen_calls.clear()
+    monkeypatch.setattr(
+        browser_tool,
+        "_run_browser_command",
+        lambda *args, **kwargs: {"success": True, "data": {"result": "https://example.com"}},
+    )
+    monkeypatch.setattr(browser_tool, "_chromium_installed", lambda: True)
+
+    result = browser_tool._run_chrome_fallback_command("task", "snapshot", [], 5)
+    assert result["success"] is True
+    assert len(popen_calls) == 3  # open, requested command, close
+    assert all(call[1]["start_new_session"] == "sentinel" for call in popen_calls)

tests/tools/test_docker_environment.py

@@ -5,6 +5,7 @@
 import pytest
 
 from tools.environments import docker as docker_env
+from tools.environments import base as base_env
 
 
 def _mock_subprocess_run(monkeypatch):
@@ -403,6 +404,39 @@ def test_docker_env_and_forward_env_merge_in_init_args(monkeypatch):
     assert "TOKEN=secret123" in args_str
 
 
+def test_docker_exec_forwards_explicit_env_on_every_command_without_snapshot_persistence(monkeypatch):
+    """Secret-like opt-ins remain command-scoped instead of relying on snapshots."""
+    env_name = "EXAMPLE_API_" + "TOKEN"
+    env = _make_execute_only_env(forward_env=[env_name])
+    env._env = {"STATIC_CONTAINER_VAR": "container-value"}
+    monkeypatch.setenv(env_name, "forwarded-value")
+    monkeypatch.setattr(docker_env, "_load_hermes_env_vars", lambda: {})
+    env._init_env_args = env._build_init_env_args()
+
+    calls = []
+    monkeypatch.setattr(
+        docker_env,
+        "_popen_bash",
+        lambda command, stdin_data: calls.append(command) or _FakePopen(command),
+    )
+
+    env._run_bash("printf command1", login=False)
+    env._run_bash("printf command2", login=False)
+
+    assert len(calls) == 2
+    assert all(f"{env_name}=forwarded-value" in call for call in calls)
+    assert all("STATIC_CONTAINER_VAR" not in call for call in calls)
+    snapshot_lines = f'declare -x {env_name}="forwarded-value"\n'
+    scrubbed = subprocess.run(
+        ["grep", "-Eiv", base_env._SNAPSHOT_SECRET_ENV_RE],
+        input=snapshot_lines,
+        text=True,
+        capture_output=True,
+        check=False,
+    ).stdout
+    assert env_name not in scrubbed
+
+
 
 def test_normalize_env_dict_filters_invalid_keys():
     """_normalize_env_dict should reject invalid variable names."""

tests/tools/test_init_session_cwd_respect.py

@@ -16,7 +16,11 @@
 from tempfile import TemporaryFile
 from unittest.mock import MagicMock
 
-from tools.environments.base import BaseEnvironment
+from tools.environments.base import (
+    BaseEnvironment,
+    _SNAPSHOT_SECRET_ENV_RE,
+    _snapshot_export_command,
+)
 
 
 class _TestableEnv(BaseEnvironment):
@@ -146,3 +150,82 @@ def mock_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
         assert "'/my project/with spaces'" in bootstrap, (
             "bootstrap cd must properly quote paths with spaces"
         )
+
+    def test_snapshot_capture_filters_secret_env_names(self):
+        env = _TestableEnv(cwd="/tmp")
+        captured = {}
+
+        def mock_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
+            captured["cmd"] = cmd_string
+            mock = MagicMock()
+            mock.poll.return_value = 0
+            mock.returncode = 0
+            stdout = TemporaryFile(mode="w+b")
+            stdout.seek(0)
+            mock.stdout = stdout
+            return mock
+
+        env._run_bash = mock_run_bash
+        env.init_session()
+
+        bootstrap = captured["cmd"]
+        assert "export -p | grep -Eiv" in bootstrap
+        assert "TOKEN" in bootstrap
+        assert "PASSWORD" in bootstrap
+        assert "KEY" in bootstrap
+        assert "CREDENTIALS?" in bootstrap
+        assert "AUTHORIZATION" in bootstrap
+        assert "BEARER" in bootstrap
+
+    def test_snapshot_refresh_filters_secret_env_names(self):
+        env = _TestableEnv(cwd="/tmp")
+        env._snapshot_ready = True
+
+        wrapped = env._wrap_command("true", "/tmp")
+
+        assert "export -p | grep -Eiv" in wrapped
+        assert "TOKEN" in wrapped
+        assert "PASSWORD" in wrapped
+        assert "KEY" in wrapped
+        assert "CREDENTIALS?" in wrapped
+        assert "AUTHORIZATION" in wrapped
+
+    def test_snapshot_filter_does_not_drop_auth_socket_names(self):
+        cmd = _snapshot_export_command("/tmp/snap.sh")
+        assert "AUTHORIZATION" in cmd
+        assert "BEARER" in cmd
+        assert "AUTH($|_)" not in cmd
+
+    def test_snapshot_filter_scrubs_common_secret_names_but_keeps_socket_names(self):
+        lines = "\n".join(
+            [
+                'declare -x OPENAI_API_KEY="secret"',
+                'declare -x api_key="secret"',
+                'declare -x OPENAI_KEY="secret"',
+                'declare -x GOOGLE_APPLICATION_CREDENTIALS="secret"',
+                'declare -x AWS_ACCESS_KEY_ID="secret"',
+                'declare -x NPM_CONFIG__AUTH_TOKEN="secret"',
+                'declare -x SSH_AUTH_SOCK="/tmp/ssh.sock"',
+                'declare -x XAUTHORITY="/tmp/auth"',
+                'declare -x TOKENIZER_PARALLELISM="false"',
+            ]
+        )
+        import subprocess
+
+        proc = subprocess.run(
+            ["grep", "-Eiv", _SNAPSHOT_SECRET_ENV_RE],
+            input=lines,
+            text=True,
+            capture_output=True,
+            check=True,
+        )
+        snapshot = proc.stdout
+        assert "OPENAI_API_KEY" not in snapshot
+        assert "api_key" not in snapshot
+        assert "OPENAI_KEY" not in snapshot
+        assert "GOOGLE_APPLICATION_CREDENTIALS" not in snapshot
+        assert "AWS_ACCESS_KEY_ID" not in snapshot
+        assert "NPM_CONFIG__AUTH_TOKEN" not in snapshot
+        assert "SSH_AUTH_SOCK" in snapshot
+        assert "XAUTHORITY" in snapshot
+        assert "TOKENIZER_PARALLELISM" in snapshot