mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(terminal): stop shared bash snapshot leaking HERMES_SESSION_* across sessions
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 sources it before every command. 'export -p' dumped the FIRST session's HERMES_SESSION_ID into the snapshot, so every LATER session sourced 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. Confirmed on staging-fp: session A read its own id, session B (same backend) read A's id. Reproduced locally with two threads sharing one LocalEnvironment; the snapshot held 'declare -x HERMES_SESSION_ID=...' from the first session. Fix: strip the per-session bridged vars (HERMES_SESSION_* / HERMES_UI_SESSION_ID / HERMES_CRON_AUTO_DELIVER_*, i.e. gateway.session_context._VAR_MAP prefixes) from the snapshot at both dump sites in base.py. They are re-injected fresh on every command, so a snapshot should carry only the user's own shell state, not Hermes' per-turn session identity. Complements the _set_session_context session_id fix: that ensures the ContextVar carries the right id; this ensures the shared snapshot can't override it with a neighbour's. Adds tests/tools/test_snapshot_session_id_leak.py (regex unit + real two-session LocalEnvironment integration) and updates the export-shape assertions in test_base_environment.py for the new 'export -p | grep' dump.
This commit is contained in:
parent
3e340d93f4
commit
f2e32ceead
3 changed files with 153 additions and 9 deletions
|
|
@ -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:
|
||||
|
|
|
|||
106
tests/tools/test_snapshot_session_id_leak.py
Normal file
106
tests/tools/test_snapshot_session_id_leak.py
Normal file
|
|
@ -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()
|
||||
|
|
@ -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"
|
||||
)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue