diff --git a/tests/tools/test_base_environment.py b/tests/tools/test_base_environment.py index 7b84b15787e..079287a021e 100644 --- a/tests/tools/test_base_environment.py +++ b/tests/tools/test_base_environment.py @@ -67,7 +67,7 @@ class TestWrapCommand: 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 + assert "export -p" in wrapped and "> " 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 @@ -141,14 +141,15 @@ class TestAtomicSnapshotWrite: 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 + assert "export -p" in wrapped and "> " in wrapped assert ".tmp." in wrapped # Then an atomic rename onto the real snapshot path. assert "mv -f " in wrapped # The env-dump must NOT write the live snapshot in place (the bug). snap = env._snapshot_path - assert f"export -p > {snap} " not in wrapped - assert f"export -p > '{snap}'" not in wrapped + assert f"> {snap} " not in wrapped + assert f"> '{snap}'" not in wrapped + assert f"> {snap}\n" not in wrapped def test_temp_path_uses_bashpid_not_dollardollar(self): """The temp name MUST use ``$BASHPID`` (the real subshell PID), not @@ -186,7 +187,7 @@ class TestAtomicSnapshotWrite: 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 "> " 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 +218,7 @@ class TestAtomicSnapshotWrite: 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 +235,7 @@ class TestAtomicSnapshotWrite: 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: diff --git a/tests/tools/test_snapshot_session_id_leak.py b/tests/tools/test_snapshot_session_id_leak.py new file mode 100644 index 00000000000..d48f4f44968 --- /dev/null +++ b/tests/tools/test_snapshot_session_id_leak.py @@ -0,0 +1,106 @@ +"""Cross-session HERMES_SESSION_ID leak via the shared bash snapshot. + +Regression coverage for the bug where a single long-lived backend serves many +sessions through ONE ``_active_environments["default"]`` LocalEnvironment (the +messaging gateway, TUI, and desktop/web dashboard all collapse the terminal to +"default"). That environment persists a bash *session snapshot* file and +``source``s it before every command. ``export -p`` dumped the FIRST session's +``HERMES_SESSION_ID`` into the snapshot, so every LATER session ``source``d that +stale value and its ``echo $HERMES_SESSION_ID`` reported a FOREIGN session's id +— overriding the correct per-command Popen env injected by +``_inject_session_context_env``. + +The fix strips the per-session bridged vars (HERMES_SESSION_* / UI / +CRON_AUTO_DELIVER_) from the snapshot at both dump sites in +``tools/environments/base.py``; they are re-injected fresh on every command. +""" + +import os +import re +import sys + +import pytest + +from tools.environments.base import ( + _SNAPSHOT_EXCLUDED_ENV_REGEX, + _export_dump_excluding_session_vars, +) + + +# --------------------------------------------------------------------------- +# Unit: the exclusion regex matches exactly the bridged vars, nothing else. +# --------------------------------------------------------------------------- + +def test_regex_matches_bridged_session_vars(): + rx = re.compile(_SNAPSHOT_EXCLUDED_ENV_REGEX) + # Every var the gateway bridges must be excluded. + from gateway.session_context import _VAR_MAP + + for name in _VAR_MAP: + line = f'declare -x {name}="whatever"' + assert rx.search(line), f"{name} should be excluded from the snapshot" + + +def test_regex_preserves_user_env(): + rx = re.compile(_SNAPSHOT_EXCLUDED_ENV_REGEX) + for line in ( + 'declare -x PATH="/usr/bin:/bin"', + 'declare -x HOME="/home/user"', + 'declare -x HERMES_HOME="/home/user/.hermes"', # NOT a session var + 'declare -x HERMESX="x"', + 'declare -x MY_HERMES_SESSION_ID="x"', # prefix must anchor after "declare -x " + ): + assert not rx.search(line), f"{line!r} must be preserved in the snapshot" + + +def test_export_snippet_shape(): + snippet = _export_dump_excluding_session_vars("/tmp/snap.tmp.$BASHPID") + assert "export -p" in snippet + assert "grep -vE" in snippet + assert "/tmp/snap.tmp.$BASHPID" in snippet + assert snippet.rstrip().endswith("|| true") + + +# --------------------------------------------------------------------------- +# Integration: real LocalEnvironment, two sessions, no cross-contamination. +# --------------------------------------------------------------------------- + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX bash snapshot path") +def test_shared_snapshot_no_cross_session_leak(tmp_path): + import threading + + from gateway.session_context import _VAR_MAP, _UNSET, set_session_vars + from tools.environments.local import LocalEnvironment + + env = LocalEnvironment(cwd=str(tmp_path), timeout=30) + env.init_session() + try: + def run_as(sid): + out = {} + + def worker(): + for v in _VAR_MAP.values(): + v.set(_UNSET) + set_session_vars(session_key="k" + sid, session_id=sid, source="desktop") + out["r"] = env.execute('echo "[$HERMES_SESSION_ID]"') + + t = threading.Thread(target=worker) + t.start() + t.join() + return out["r"].get("output", "") + + out_a = run_as("SIDAAA") + out_b = run_as("SIDBBB") + + assert "SIDAAA" in out_a, f"session A saw {out_a!r}" + # The core assertion: B must see its OWN id, not A's leaked via snapshot. + assert "SIDBBB" in out_b, f"session B saw {out_b!r}" + assert "SIDAAA" not in out_b, f"session B leaked A's id: {out_b!r}" + + # And the snapshot file must not carry the session id at all. + snap = env._snapshot_path + if os.path.exists(snap): + with open(snap) as f: + assert "HERMES_SESSION_ID" not in f.read() + finally: + env.cleanup() diff --git a/tools/environments/base.py b/tools/environments/base.py index 1b082bd83a0..7199079fb24 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -382,6 +382,42 @@ def _cwd_marker(session_id: str) -> str: return f"__HERMES_CWD_{session_id}__" +# Per-session variables that the gateway bridges freshly onto every command's +# process environment (via tools/environments/local._inject_session_context_env, +# reading gateway.session_context._VAR_MAP). They must NEVER be persisted into +# the shared bash session snapshot: a single long-lived backend serves many +# concurrent sessions (the messaging gateway, TUI, desktop/web dashboard all +# collapse the terminal to one "default" environment), so ``export -p`` dumping +# the FIRST session's HERMES_SESSION_ID into the snapshot makes every LATER +# session ``source`` that stale value and see a FOREIGN session's identity — +# overriding the correct per-command Popen env (issue: cross-session +# HERMES_SESSION_ID leak via the shared snapshot). Stripping them from the +# snapshot is safe because they are re-injected on every command; a snapshot +# should only carry the user's own shell state (PATH, functions, exports they +# set), not Hermes' per-turn session identity. +# +# Kept in sync with gateway.session_context._VAR_MAP: every bridged name starts +# with one of these prefixes. +_SNAPSHOT_EXCLUDED_ENV_REGEX = ( + "^declare -x (HERMES_SESSION_|HERMES_UI_SESSION_ID|HERMES_CRON_AUTO_DELIVER_)" +) + + +def _export_dump_excluding_session_vars(tmp_path: str) -> str: + """Return a shell snippet that dumps ``export -p`` to *tmp_path* minus the + per-session bridged vars (see ``_SNAPSHOT_EXCLUDED_ENV_REGEX``). + + ``export -p`` emits one ``declare -x NAME="value"`` line per exported var. + We drop the HERMES_SESSION_* / UI / CRON_AUTO_DELIVER lines so they never + persist across sessions in the shared snapshot. ``grep -vE`` returns exit 1 + when it filters everything, so ``|| true`` keeps the pipeline's success + contract intact for the callers that chain on it. + """ + return ( + f"export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {tmp_path} || true" + ) + + # --------------------------------------------------------------------------- # BaseEnvironment # --------------------------------------------------------------------------- @@ -498,7 +534,7 @@ class BaseEnvironment(ABC): _snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID" bootstrap = ( f"umask 077\n" - f"export -p > {_snap_tmp}\n" + f"{_export_dump_excluding_session_vars(_snap_tmp)}\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 '^_[^_]'`` @@ -644,7 +680,8 @@ class BaseEnvironment(ABC): # 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"{{ export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {_snap_tmp} " + f"&& mv -f {_snap_tmp} {_quoted_snap}; }} " f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true" )