diff --git a/tests/tools/test_base_environment.py b/tests/tools/test_base_environment.py index 7b84b15787e63..5efc999d3e276 100644 --- a/tests/tools/test_base_environment.py +++ b/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: diff --git a/tests/tools/test_browser_tool_process_isolation.py b/tests/tools/test_browser_tool_process_isolation.py new file mode 100644 index 0000000000000..9efad3fca763a --- /dev/null +++ b/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) diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index c85402a3c2df3..cf8c19621d26f 100644 --- a/tests/tools/test_docker_environment.py +++ b/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.""" diff --git a/tests/tools/test_init_session_cwd_respect.py b/tests/tools/test_init_session_cwd_respect.py index 2adce4b74e382..622a9f8056b2e 100644 --- a/tests/tools/test_init_session_cwd_respect.py +++ b/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 diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 82c248a3f715a..793d9b383e255 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -767,6 +767,27 @@ def _browser_install_hint() -> str: return "npm install -g agent-browser && agent-browser install --with-deps" +def _browser_popen_extra() -> dict: + """Return subprocess isolation kwargs for agent-browser helper commands.""" + if os.name != "nt": + # POSIX: keep agent-browser/Chromium in a separate session so signals + # sent to the Hermes parent process do not cascade into browser daemon + # grandchildren or feed back as parent process interruptions. + return {"start_new_session": True} + + # Windows: CREATE_NO_WINDOW avoids a console for the .cmd shim. Do NOT add + # CREATE_NEW_PROCESS_GROUP here: on Python 3.11 Windows it can cancel the + # running asyncio/proactor loop and surface as KeyboardInterrupt in the CLI. + flags = 0x08000000 + startupinfo = subprocess.STARTUPINFO() + startupinfo.dwFlags |= subprocess.STARTF_USESTDHANDLES + return { + "creationflags": flags, + "close_fds": True, + "startupinfo": startupinfo, + } + + def _requires_real_termux_browser_install(browser_cmd: str) -> bool: return _is_termux_environment() and _is_local_mode() and browser_cmd.strip() == "npx agent-browser" @@ -1068,40 +1089,10 @@ def _run_tmp(cmd: str, cmd_args: List[str]) -> Dict[str, Any]: stdout_fd = os.open(stdout_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) stderr_fd = os.open(stderr_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) try: - # On Windows, launch the child in a new process group so parent - # console Ctrl+C doesn't kill it with STATUS_CONTROL_C_EXIT - # (0xC000013A = rc 3221225786), AND insulate its stdio + handle - # inheritance from the parent. - # - # Additional Windows hardening beyond CREATE_NEW_PROCESS_GROUP: - # * STARTF_USESTDHANDLES + explicit handles → CreateProcess hands - # the child ONLY our three chosen handles (DEVNULL stdin + - # temp-file stdout/stderr). Without this, some parents leak - # console handles that break downstream grandchild spawns — the - # agent-browser Rust binary spawns a detached daemon grandchild, - # and that grandchild's CreateProcess dies silently - # ("Daemon process exited during startup with no error output") - # when inherited parent handles are in a weird state. Observed - # in the Hermes CLI where sys.stdout and sys.stderr both report - # fileno=1 (stderr dup'd onto stdout at the OS level). - # * close_fds=True → block inheritance of every other handle. - # (Default on POSIX; must be explicit on Windows for stdio.) - _popen_extra: dict = {} - if os.name == "nt": - # CREATE_NO_WINDOW → don't attach a console (cmd.exe would - # otherwise briefly allocate one for the .cmd shim). - # Do NOT add CREATE_NEW_PROCESS_GROUP: on Python 3.11 Windows - # it interacts with asyncio's ProactorEventLoop such that the - # subprocess creation cancels the running loop task, which - # surfaces as KeyboardInterrupt in app.run() and tears down - # the CLI mid-turn. The agent thread's subprocess spawn - # unwound MainThread's prompt_toolkit loop that way — see - # diag log: "asyncio.CancelledError → KeyboardInterrupt". - _popen_extra["creationflags"] = windows_hide_flags() - _popen_extra["close_fds"] = True - _si = subprocess.STARTUPINFO() - _si.dwFlags |= subprocess.STARTF_USESTDHANDLES - _popen_extra["startupinfo"] = _si + # Use platform-specific subprocess isolation. On POSIX this + # starts a new session; on Windows it preserves the no-console / + # no-new-process-group behavior required by prompt_toolkit. + _popen_extra = _browser_popen_extra() proc = subprocess.Popen( full, stdout=stdout_fd, stderr=stderr_fd, stdin=subprocess.DEVNULL, env=browser_env, @@ -2425,22 +2416,10 @@ def _run_browser_command( stdout_fd = os.open(stdout_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) stderr_fd = os.open(stderr_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) try: - # See matching comment at the other Popen site above — on - # Windows we put agent-browser in its own process group, force - # STARTF_USESTDHANDLES so CreateProcess hands the child ONLY our - # three explicit handles (no leaked parent-console handles to - # confuse the Rust binary's daemon-spawn), and close_fds=True to - # block inheritance of everything else. - _popen_extra: dict = {} - if os.name == "nt": - # See matching block at the other Popen site — CREATE_NO_WINDOW - # only, NO CREATE_NEW_PROCESS_GROUP (cancels asyncio loop task - # on Python 3.11 Windows → KeyboardInterrupt in CLI MainThread). - _popen_extra["creationflags"] = windows_hide_flags() - _popen_extra["close_fds"] = True - _si = subprocess.STARTUPINFO() - _si.dwFlags |= subprocess.STARTF_USESTDHANDLES - _popen_extra["startupinfo"] = _si + # Use platform-specific subprocess isolation. On POSIX this + # starts a new session; on Windows it preserves the no-console / + # no-new-process-group behavior required by prompt_toolkit. + _popen_extra = _browser_popen_extra() proc = subprocess.Popen( cmd_parts, stdout=stdout_fd, diff --git a/tools/environments/base.py b/tools/environments/base.py index 1b20cfa90ee0c..2494c2460bc24 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -27,6 +27,36 @@ logger = logging.getLogger(__name__) +_SNAPSHOT_SECRET_ENV_RE = ( + r"(^|[^[:alnum:]])(TOKEN|SECRET|PASSWORD|PASSWD|KEY|API_?KEY|ACCESS_?KEY|PRIVATE_?KEY|" + r"CREDENTIALS?|AUTHORIZATION|BEARER)([^[:alnum:]]|$)" +) + + +def _snapshot_export_command(target: str) -> str: + """Write non-secret exported variables to *target* for session replay.""" + # ``export -p`` emits shell-safe declarations. Keep the session snapshot + # useful for PATH/HOME/etc. but do not persist credentials into /tmp-backed + # hermes-snap-*.sh files. Avoid a broad ``AUTH`` match so SSH_AUTH_SOCK and + # XAUTHORITY survive; explicit AUTHORIZATION/BEARER cover HTTP credentials. + # Wrap the pipe in a brace group so the redirect binds to the group (run by + # the current shell), NOT to ``grep`` (which runs in its own pipe subshell). + # Otherwise ``> {target}`` expands ``$BASHPID`` to grep's subshell PID while + # the caller's ``mv {target}`` expands it to the outer shell PID — the temp + # names diverge, the mv finds nothing, and the snapshot is never updated + # (env stops persisting between commands). + # Capture PIPESTATUS immediately: a successful grep must not hide a failed + # ``export -p`` and allow a partial temp file to replace the live snapshot. + # grep status 1 only means that no lines matched and is a valid empty dump. + return ( + "{ export -p | " + f"grep -Eiv {shlex.quote(_SNAPSHOT_SECRET_ENV_RE)}; " + '__hermes_snapshot_status=("${PIPESTATUS[@]}"); ' + "(( __hermes_snapshot_status[0] == 0 && " + "__hermes_snapshot_status[1] <= 1 )); } " + f"> {target}" + ) + # Opt-in debug tracing for the interrupt/activity/poll machinery. Set # HERMES_DEBUG_INTERRUPT=1 to log loop entry/exit, periodic heartbeats, and # every is_interrupted() state change from _wait_for_process. Off by default @@ -498,7 +528,7 @@ def init_session(self): _snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID" bootstrap = ( f"umask 077\n" - f"export -p > {_snap_tmp}\n" + f"if {_snapshot_export_command(_snap_tmp)}; then\n" # Dump function definitions, filtering out private (``_``-prefixed) # helpers — mainly bash-completion internals (``_git``, ``_make``…) # — by NAME, not by line. A naive ``declare -f | grep -vE '^_[^_]'`` @@ -520,7 +550,11 @@ def init_session(self): f"echo 'set +u' >> {_snap_tmp}\n" # Publish atomically only if assembly succeeded; otherwise drop the # partial temp rather than leave it to be sourced or orphaned. - f"mv -f {_snap_tmp} {_quoted_snap} || rm -f {_snap_tmp}\n" + f"mv -f {_snap_tmp} {_quoted_snap} || {{ rm -f {_snap_tmp}; exit 1; }}\n" + f"else\n" + f"rm -f {_snap_tmp}\n" + f"exit 1\n" + f"fi\n" f"builtin cd -- {_quoted_cwd} 2>/dev/null || true\n" f"printf '\\n{self._cwd_marker}%s{self._cwd_marker}\\n' \"$(pwd -P)\"\n" ) @@ -638,13 +672,13 @@ def _wrap_command(self, command: str, cwd: str) -> str: # umask. Snapshot files may contain env-carried secrets. parts.append("umask 077") - # Re-dump env vars to snapshot (atomic replacement to avoid races). + # Re-dump non-secret env vars to snapshot (atomic replacement to avoid races). # Chain mv on the export succeeding so a failed/partial dump never # replaces a good snapshot; drop the temp on failure so it isn't # orphaned (cleaned up wholesale in LocalEnvironment.cleanup too). if self._snapshot_ready: parts.append( - f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_quoted_snap}; }} " + f"{{ {_snapshot_export_command(_snap_tmp)} && mv -f {_snap_tmp} {_quoted_snap}; }} " f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true" ) diff --git a/tools/environments/docker.py b/tools/environments/docker.py index ea4a6ec77e681..894613c72d535 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -1054,6 +1054,23 @@ def _build_init_env_args(self) -> list[str]: args.extend(["-e", f"{key}={exec_env[key]}"]) return args + def _build_command_env_args(self) -> list[str]: + """Keep explicit forward_env values available on every docker exec. + + Secret-like names are intentionally excluded from the reusable shell + snapshot, so explicit operator opt-ins must remain command-scoped. + Implicit passthrough and docker_env values keep their existing init- or + container-scoped behavior. + """ + explicit_keys = set(self._forward_env) + args: list[str] = [] + for flag, assignment in zip( + self._init_env_args[0::2], self._init_env_args[1::2] + ): + if assignment.partition("=")[0] in explicit_keys: + args.extend([flag, assignment]) + return args + def _run_bash(self, cmd_string: str, *, login: bool = False, timeout: int = 120, stdin_data: str | None = None) -> subprocess.Popen: @@ -1063,10 +1080,13 @@ def _run_bash(self, cmd_string: str, *, login: bool = False, if stdin_data is not None: cmd.append("-i") - # Only inject -e env args during init_session (login=True). - # Subsequent commands get env vars from the snapshot. + # The bootstrap receives the full init environment. Later commands only + # receive explicit forward_env opt-ins: secret-like values are scrubbed + # from the reusable snapshot and therefore must remain command-scoped. if login: cmd.extend(self._init_env_args) + else: + cmd.extend(self._build_command_env_args()) cmd.extend([self._container_id])