fix(profiles): profile delete refuses to kill another profile's gateway (#89315)
`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.
This commit is contained in:
Teknium
@@ -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}$")
|
||||
|
||||
@@ -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,
|
||||
|
||||
217
tests/hermes_cli/test_cross_profile_kill_refusal.py
Normal file
217
tests/hermes_cli/test_cross_profile_kill_refusal.py
Normal file
@@ -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)
|
||||
Reference in New Issue
Block a user