From d3e2ace1dde9f1d279f99c9ebc6bce2e761b025d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:49:01 -0700 Subject: [PATCH] fix(profiles): profile delete refuses to kill another profile's gateway (#89315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes profile delete` read the target profile's gateway.pid raw and SIGTERMed it. When that pid file was poisoned by a sibling profile's gateway (the #89315 shape), deleting profile A killed profile B's running gateway. - gateway/status.py: `_pid_record_belongs_to_profile()` helper — a pid record whose recorded home differs from the expected profile home is not ours; legacy records without a home prove nothing and are left alone. - hermes_cli/profiles.py: `_stop_gateway_process` refuses (and says so) when the record belongs to another profile; still stops its own gateway. The stop/restart paths in hermes_cli/gateway.py did not need a guard: `get_running_pid()` already filters cross-profile records and unlinks the poisoned pid file before any kill can happen — verified live; the test for that path now pins the real contract (returns False, other process alive, poisoned pid file gone). Live repro (unpatched main): `_stop_gateway_process(tim_home)` -> "Gateway stopped (PID ...)" and the OTHER profile's process exits -15. After: "Refusing to stop PID ..." and the process stays alive. 8 tests; sabotage (guard removed) fails 1. --- gateway/status.py | 35 +++ hermes_cli/profiles.py | 13 ++ .../test_cross_profile_kill_refusal.py | 217 ++++++++++++++++++ 3 files changed, 265 insertions(+) create mode 100644 tests/hermes_cli/test_cross_profile_kill_refusal.py diff --git a/gateway/status.py b/gateway/status.py index cc611509ef..0df8708158 100644 --- a/gateway/status.py +++ b/gateway/status.py @@ -157,6 +157,41 @@ def _same_hermes_home(left: Path | str, right: Path | str) -> bool: ) +def recorded_gateway_home_conflicts( + record: Optional[dict[str, Any]], + *, + expected_home: Optional[Path | str] = None, +) -> bool: + """True when a persisted gateway record names a DIFFERENT HERMES_HOME. + + Cross-profile kill refusal (#89315): a poisoned/contaminated PID record + inside one profile's home can truthfully name ANOTHER profile's live + gateway (its ``hermes_home`` stamp records the real owner). Any + destructive caller about to signal the recorded PID must consult this + first and refuse when the record positively proves the target belongs to + a different profile — otherwise ``gateway stop``/``restart``/``profile + delete`` from profile B SIGTERMs profile A's gateway and the supervisors + enter the mutual restart loop from the issue report. + + ``expected_home`` overrides the comparison base (e.g. ``profile delete`` + stopping a TARGET profile's gateway rather than the current process's). + Legacy records without a ``hermes_home`` stamp return False — they prove + nothing either way, and destructive callers already pair this with the + exact PID + start-time identity guards. A comparison failure returns True + (destructive action + unprovable ownership ⇒ fail closed). + """ + if not isinstance(record, dict): + return False + recorded_home = record.get("hermes_home") + if not isinstance(recorded_home, str) or not recorded_home.strip(): + return False + try: + base = expected_home if expected_home is not None else _get_process_hermes_home() + return not _same_hermes_home(recorded_home, base) + except Exception: + return True + + # Mirrors hermes_cli.profiles._PROFILE_ID_RE — duplicated here because gateway # identity code must stay import-light (hermes_constants + stdlib only). _PROFILE_LABEL_RE = re.compile(r"^[a-z0-9][a-z0-9_-]{0,63}$") diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 9948b07f20..a10a7182cd 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -2020,6 +2020,19 @@ def _stop_gateway_process(profile_dir: Path) -> None: raw = pid_file.read_text(encoding="utf-8").strip() data = json.loads(raw) if raw.startswith("{") else {"pid": int(raw)} pid = int(data["pid"]) + # Cross-profile kill refusal (#89315): the record's hermes_home stamp + # names the gateway's TRUE owner. A contaminated/poisoned gateway.pid + # inside this profile dir can point at another profile's live gateway + # — killing it starts the mutual SIGTERM restart loop from the issue. + from gateway.status import recorded_gateway_home_conflicts + + if recorded_gateway_home_conflicts(data, expected_home=profile_dir): + print( + f"✗ Refusing to stop PID {pid}: its recorded HERMES_HOME " + f"belongs to a different profile than {profile_dir} " + "(stale/poisoned PID record, #89315)." + ) + return # Route through terminate_pid so Windows uses the appropriate # primitive (taskkill / TerminateProcess) — raw os.kill with # _signal.SIGKILL raises AttributeError at import time on Windows, diff --git a/tests/hermes_cli/test_cross_profile_kill_refusal.py b/tests/hermes_cli/test_cross_profile_kill_refusal.py new file mode 100644 index 0000000000..7b05e98d65 --- /dev/null +++ b/tests/hermes_cli/test_cross_profile_kill_refusal.py @@ -0,0 +1,217 @@ +"""Cross-profile kill refusal regression tests (#89315). + +A poisoned/contaminated ``gateway.pid`` inside one profile's HERMES_HOME can +truthfully name ANOTHER profile's live gateway (its ``hermes_home`` stamp +records the real owner). ``gateway stop`` / the restart force-kill escalation +/ ``profile delete`` must refuse to signal such a PID instead of starting the +mutual cross-profile SIGTERM restart loop from the issue report. + +These tests exercise the REAL code paths against real PID files, a real +flock-held gateway lock, and a real dummy child process — no mocks of the +code under test. +""" + +import json +import os +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from gateway.status import recorded_gateway_home_conflicts + + +def _spawn_gateway_lookalike(bin_dir: Path, lock_path: Path) -> subprocess.Popen: + """Real child process whose argv matches the gateway runtime matcher.""" + bin_dir.mkdir(parents=True, exist_ok=True) + lock_path.parent.mkdir(parents=True, exist_ok=True) + script = bin_dir / "hermes" + if sys.platform == "win32": + body = "import time\ntime.sleep(120)\n" + else: + body = ( + "import fcntl, time\n" + f"fh = open({str(lock_path)!r}, 'a+')\n" + "fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)\n" + "time.sleep(120)\n" + ) + script.write_text(f"#!{sys.executable}\n{body}", encoding="utf-8") + if sys.platform != "win32": + script.chmod(0o755) + cmd = [str(script), "gateway", "run"] + else: + cmd = [sys.executable, str(script), "gateway", "run"] + proc = subprocess.Popen( + cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL + ) + deadline = time.monotonic() + 10.0 + while time.monotonic() < deadline and not lock_path.exists(): + if proc.poll() is not None: + raise RuntimeError("gateway lookalike died at startup") + time.sleep(0.05) + return proc + + +def _pid_record(proc: subprocess.Popen, script: Path, owner_home: Path) -> dict: + from gateway.status import get_process_start_time + + return { + "pid": proc.pid, + "kind": "hermes-gateway", + "argv": [str(script), "gateway", "run"], + "start_time": get_process_start_time(proc.pid), + "hermes_home": str(owner_home), + } + + +class TestRecordedGatewayHomeConflicts: + def test_conflicting_home_detected(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "profiles" / "tim")) + record = {"pid": 1, "hermes_home": str(tmp_path)} + assert recorded_gateway_home_conflicts(record) is True + + def test_same_home_accepted(self, tmp_path, monkeypatch): + home = tmp_path / "profiles" / "tim" + monkeypatch.setenv("HERMES_HOME", str(home)) + record = {"pid": 1, "hermes_home": str(home)} + assert recorded_gateway_home_conflicts(record) is False + + def test_legacy_record_without_home_proves_nothing(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + assert recorded_gateway_home_conflicts({"pid": 1}) is False + assert recorded_gateway_home_conflicts(None) is False + assert recorded_gateway_home_conflicts({"pid": 1, "hermes_home": " "}) is False + + def test_expected_home_override(self, tmp_path): + target = tmp_path / "profiles" / "tim" + record = {"pid": 1, "hermes_home": str(tmp_path)} + assert ( + recorded_gateway_home_conflicts(record, expected_home=target) is True + ) + assert ( + recorded_gateway_home_conflicts(record, expected_home=tmp_path) is False + ) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX flock harness") +class TestCrossProfileStopRefusal: + def test_stop_profile_gateway_refuses_other_profiles_pid( + self, tmp_path, monkeypatch + ): + """Profile B's ``gateway stop`` must not SIGTERM profile A's gateway. + + On main this path is already safe upstream of any guard: + ``get_running_pid()`` filters a pid record owned by another profile + (and unlinks the poisoned pid file) before ``stop_profile_gateway`` + ever sees a pid — so the contract here is "returns False, other + profile's process untouched, poisoned pid file gone", not a printed + refusal. + """ + root_home = tmp_path / "root-home" + tim_home = tmp_path / "root-home" / "profiles" / "tim" + tim_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tim_home)) + + proc = _spawn_gateway_lookalike( + tmp_path / "bin", tim_home / "gateway.lock" + ) + try: + record = _pid_record(proc, tmp_path / "bin" / "hermes", root_home) + (tim_home / "gateway.pid").write_text(json.dumps(record)) + + from hermes_cli import gateway as gateway_cli + + assert gateway_cli.stop_profile_gateway() is False + assert not (tim_home / "gateway.pid").exists(), ( + "poisoned cross-profile pid file should have been unlinked" + ) + time.sleep(0.5) + assert proc.poll() is None, ( + "cross-profile SIGTERM fired: profile A's gateway was killed" + ) + finally: + proc.kill() + proc.wait(timeout=10) + + def test_stop_profile_gateway_still_stops_own_gateway( + self, tmp_path, monkeypatch + ): + """Same-home records keep stopping normally (no false refusal).""" + tim_home = tmp_path / "profiles" / "tim" + tim_home.mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(tim_home)) + + proc = _spawn_gateway_lookalike( + tmp_path / "bin", tim_home / "gateway.lock" + ) + try: + record = _pid_record(proc, tmp_path / "bin" / "hermes", tim_home) + (tim_home / "gateway.pid").write_text(json.dumps(record)) + + from hermes_cli import gateway as gateway_cli + + assert gateway_cli.stop_profile_gateway() is True + deadline = time.monotonic() + 15.0 + while time.monotonic() < deadline and proc.poll() is None: + time.sleep(0.1) + assert proc.poll() is not None, "own gateway was not stopped" + finally: + if proc.poll() is None: + proc.kill() + proc.wait(timeout=10) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX flock harness") +class TestProfileDeleteStopRefusal: + def test_stop_gateway_process_refuses_other_profiles_pid( + self, tmp_path, capsys + ): + """``profile delete`` must not kill a gateway owned by another home.""" + root_home = tmp_path / "root-home" + tim_home = root_home / "profiles" / "tim" + tim_home.mkdir(parents=True) + + proc = _spawn_gateway_lookalike( + tmp_path / "bin", tim_home / "gateway.lock" + ) + try: + record = _pid_record(proc, tmp_path / "bin" / "hermes", root_home) + (tim_home / "gateway.pid").write_text(json.dumps(record)) + + from hermes_cli.profiles import _stop_gateway_process + + _stop_gateway_process(tim_home) + out = capsys.readouterr().out + assert "Refusing to stop" in out + time.sleep(0.5) + assert proc.poll() is None, ( + "profile delete killed another profile's gateway" + ) + finally: + proc.kill() + proc.wait(timeout=10) + + def test_stop_gateway_process_still_stops_own_gateway(self, tmp_path): + tim_home = tmp_path / "profiles" / "tim" + tim_home.mkdir(parents=True) + + proc = _spawn_gateway_lookalike( + tmp_path / "bin", tim_home / "gateway.lock" + ) + try: + record = _pid_record(proc, tmp_path / "bin" / "hermes", tim_home) + (tim_home / "gateway.pid").write_text(json.dumps(record)) + + from hermes_cli.profiles import _stop_gateway_process + + _stop_gateway_process(tim_home) + deadline = time.monotonic() + 15.0 + while time.monotonic() < deadline and proc.poll() is None: + time.sleep(0.1) + assert proc.poll() is not None, "own gateway was not stopped" + finally: + if proc.poll() is None: + proc.kill() + proc.wait(timeout=10)