diff --git a/gateway/run.py b/gateway/run.py index 83e222d642..b5701aa096 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -30475,6 +30475,190 @@ def _gateway_stderr_formatter() -> logging.Formatter: return RedactingFormatter("%(asctime)s %(levelname)s %(name)s: %(message)s") + # ownership guard inserted below (PR #93084) +def _replace_target_belongs_to_other_profile(existing_pid: int) -> bool: + """Return True when ``--replace`` must refuse to signal ``existing_pid``. + + The PID file is HERMES_HOME-scoped, but a poisoned/stale record can point + at another profile's LIVE gateway; signaling it starts the cross-profile + SIGTERM restart loop this guard exists to prevent (#89315). This is a + destructive-action authority check, so ownership is decided by the + persisted identity record ALONE — exact ``_same_hermes_home`` equality — + and only while that record stays bound to the live target by exact PID + + start-time identity: + + * The authorizing record is whichever source produced the PID for this + destructive decision (PID file, gateway lock record, or runtime-status + fallback). A readable live argv carries no HERMES_HOME (it travels in + the environment), so it can never prove home ownership; it is used only + as an additional CONSISTENCY check — token-exact profile flags that + clearly contradict our home refuse the signal even when the record + agrees. + * Missing, legacy, conflicting, stale-bound, or unprovable identity → + refuse (fail closed). + + Same-home targets keep replacing normally; every refusal path here only + narrows what the legacy start_time check alone used to allow. + """ + try: + from gateway.status import ( + _get_pid_path, + _get_process_hermes_home, + _get_process_start_time, + _pid_from_record, + _read_pid_record, + _record_looks_like_gateway, + _read_process_cmdline, + _same_hermes_home, + ) + + our_home = _get_process_hermes_home() + + # ── Authorize from the persisted identity record ────────────── + # Bound claim: the record must describe THIS pid with THIS live + # start time, otherwise it is stale/poisoned and proves nothing. + record = _read_pid_record(_get_pid_path()) + if not isinstance(record, dict) or not _record_looks_like_gateway(record): + logger.warning( + "Refusing --replace: no valid gateway pid record to prove " + "ownership of PID %s.", + existing_pid, + ) + return True + + record_pid = _pid_from_record(record) + if record_pid != existing_pid: + logger.warning( + "Refusing --replace: pid record names %s, not target %s.", + record_pid, existing_pid, + ) + return True + + recorded_start = record.get("start_time") + if not isinstance(recorded_start, int) or isinstance(recorded_start, bool): + return True + if _get_process_start_time(existing_pid) != recorded_start: + logger.warning( + "Refusing --replace: pid record start-time does not match " + "the live process %s (stale/PID-reuse record).", + existing_pid, + ) + return True + + recorded_home = record.get("hermes_home") + if not isinstance(recorded_home, str) or not recorded_home.strip(): + # Legacy record without hermes_home cannot prove ownership. + logger.warning( + "Refusing --replace: pid record predates hermes_home " + "stampings; ownership of PID %s unprovable.", + existing_pid, + ) + return True + + if not _same_hermes_home(recorded_home, our_home): + logger.error( + "Refusing --replace: pid record belongs to a different " + "HERMES_HOME (%s, ours %s). Remove the stale PID record or " + "stop the owning profile explicitly.", + recorded_home, + our_home, + ) + return True + + # ── Readable-argv consistency check (never authority) ───────── + # An explicit profile flag / HERMES_HOME= on the argv that clearly + # contradicts our home refuses even though the record agreed; a bare + # or matching argv adds nothing either way. + try: + live_cmdline = _read_process_cmdline(existing_pid) + except Exception: + live_cmdline = None # consistency probe failure → record decides + if live_cmdline and _looks_like_profile_conflict_from_cmdline( + live_cmdline, our_home + ): + logger.error( + "Refusing --replace: target PID %s command line explicitly " + "advertises a different profile than HERMES_HOME %s.", + existing_pid, + our_home, + ) + return True + + return False + except Exception: + # Destructive action + unknown ownership => fail closed (#89315). + logger.warning( + "cross-profile --replace ownership probe failed for PID %s; " + "refusing to signal", + existing_pid, + exc_info=True, + ) + return True + + +def _looks_like_profile_conflict_from_cmdline(command: str, our_home) -> bool: + """Token-exact contradiction check between a target argv and our home. + + Authority lives in the pid record; this only catches argv that EXPLICITLY + advertises a different profile than ours. Substring matching is not + identity: ``--profile timothy`` must NOT read as profile ``tim``. Returns + False whenever the argv does not clearly contradict our home. + """ + from gateway.status import _profile_name_for_home + + profile_name = _profile_name_for_home(our_home) + try: + tokens = shlex.split(command) + except ValueError: + tokens = command.split() + + def _flag_value(flag: str) -> Optional[str]: + """Value of ``--flag X`` / ``--flag=X`` occurrences, token-exact.""" + values = [] + i = 0 + while i < len(tokens): + tok = tokens[i] + if tok == flag and i + 1 < len(tokens): + values.append(tokens[i + 1]) + i += 2 + continue + if tok.startswith(flag + "="): + values.append(tok[len(flag) + 1:]) + i += 1 + return values[-1] if values else None + + def _env_home_value() -> Optional[str]: + """HERMES_HOME= env-style assignment on the argv, token-exact.""" + prefix = "HERMES_HOME=" + for tok in reversed(tokens): + if tok.startswith(prefix): + return tok[len(prefix):] + return None + + if profile_name is not None and profile_name != "default": + # Our home is a named profile: any explicit DIFFERENT named profile + # on the argv contradicts it. Bare argv stays consistent (legacy + # default-gateway argv never carried profile flags). + for flag in ("--profile", "-p"): + value = _flag_value(flag) + if value is not None and value != profile_name: + return True + home_value = _flag_value("--hermes-home") or _env_home_value() + if home_value is not None and os.path.normcase(os.path.normpath(home_value)) != os.path.normcase(os.path.normpath(str(our_home))): + return True + return False + + # Our home is the default/root: ANY explicit named-profile flag on the + # argv contradicts it. + if _flag_value("--profile") is not None or _flag_value("-p") is not None: + return True + home_value = _flag_value("--hermes-home") or _env_home_value() + if home_value is not None and os.path.normcase(os.path.normpath(home_value)) != os.path.normcase(os.path.normpath(str(our_home))): + return True + return False + + + async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = False, verbosity: Optional[int] = 0) -> bool: """ Start the gateway and run until interrupted. @@ -30521,6 +30705,21 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = existing_pid = get_running_pid() if existing_pid is not None and existing_pid != os.getpid(): if replace: + # Cross-profile ownership gate (#89315): never signal a live + # process we cannot prove belongs to this HERMES_HOME. A poisoned + # PID record steering --replace at another profile's gateway is + # exactly the restart-loop shape this flow must not allow. + if _replace_target_belongs_to_other_profile(existing_pid): + from gateway.status import _get_process_hermes_home + + logger.error( + "Refusing --replace: PID %d cannot be proven to belong " + "to this profile's gateway (HERMES_HOME %s). Remove the " + "stale PID record or stop the owning profile explicitly.", + existing_pid, + _get_process_hermes_home(), + ) + return False existing_start_time = get_process_start_time(existing_pid) logger.info( "Replacing existing gateway instance (PID %d) with --replace.", @@ -31355,4 +31554,4 @@ def _exit_after_graceful_shutdown(exit_code: int) -> None: if __name__ == "__main__": - main() + main() \ No newline at end of file diff --git a/tests/gateway/test_replace_child_reap.py b/tests/gateway/test_replace_child_reap.py index 969a3b1c69..6409ea2d22 100644 --- a/tests/gateway/test_replace_child_reap.py +++ b/tests/gateway/test_replace_child_reap.py @@ -199,6 +199,21 @@ async def test_start_gateway_replace_reaps_old_gateway_children_posix( "gateway.status.remove_pid_file", lambda: _pid_state.update(alive=False), ) + # Ownership guard (#89315): legitimate same-home replace fixture — + # bound record for target pid 42 in this home. + monkeypatch.setattr( + "gateway.status._read_pid_record", + lambda path=None: { + "pid": 42, + "kind": "hermes-gateway", + "argv": ["python", "-m", "hermes_cli.main", "gateway", "run"], + "start_time": 0, + "hermes_home": str(tmp_path), + }, + ) + monkeypatch.setattr( + "gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None + ) monkeypatch.setattr( "gateway.status.release_all_scoped_locks", lambda **kwargs: 0 ) diff --git a/tests/gateway/test_runner_startup_failures.py b/tests/gateway/test_runner_startup_failures.py index 3655f756cb..b76c39d4c1 100644 --- a/tests/gateway/test_runner_startup_failures.py +++ b/tests/gateway/test_runner_startup_failures.py @@ -138,6 +138,21 @@ async def test_start_gateway_replace_aborts_when_force_killed_pid_still_alive( "gateway.status.terminate_pid", lambda pid, force=False: calls.append((pid, force)), ) + # Ownership guard (#89315): legitimate same-home replace fixture — the + # persisted record is bound to target pid 42 in this home. + monkeypatch.setattr( + "gateway.status._read_pid_record", + lambda path=None: { + "pid": 42, + "kind": "hermes-gateway", + "argv": ["python", "-m", "hermes_cli.main", "gateway", "run"], + "start_time": 0, + "hermes_home": str(tmp_path), + }, + ) + monkeypatch.setattr( + "gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None + ) # _pid_exists never goes False — the force-kill did not take. monkeypatch.setattr("gateway.status._pid_exists", lambda pid: True) monkeypatch.setattr("gateway.run.os.getpid", lambda: 100) @@ -215,6 +230,23 @@ async def test_start_gateway_replace_writes_takeover_marker_before_sigterm( _pid_state["alive"] = False monkeypatch.setattr("gateway.status.get_running_pid", _mock_get_running_pid) monkeypatch.setattr("gateway.status.remove_pid_file", _mock_remove_pid_file) + # Ownership guard (#89315): this test simulates a legitimate same-home + # replace, so the persisted pid record must be a valid BOUND record for + # the target pid in THIS home. start_time 0 matches the legacy fixture's + # convention; the live probe is patched to agree. + monkeypatch.setattr( + "gateway.status._read_pid_record", + lambda path=None: { + "pid": 42, + "kind": "hermes-gateway", + "argv": ["python", "-m", "hermes_cli.main", "gateway", "run"], + "start_time": 0, + "hermes_home": str(tmp_path), + }, + ) + monkeypatch.setattr( + "gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None + ) monkeypatch.setattr( "gateway.status.release_all_scoped_locks", lambda **kwargs: 0, diff --git a/tests/test_89315_replace_ownership_guard.py b/tests/test_89315_replace_ownership_guard.py new file mode 100644 index 0000000000..8bd7b8d428 --- /dev/null +++ b/tests/test_89315_replace_ownership_guard.py @@ -0,0 +1,393 @@ +"""Tests for issue #89315 — ``--replace`` must never signal a gateway it +cannot prove belongs to this HERMES_HOME. + +Design contract (v3, after andrexibiza's second review): ownership is decided +by the persisted identity record ALONE — exact ``_same_hermes_home`` equality +bound to the live target by exact PID + start-time. A readable live argv +carries no HERMES_HOME, so it can never prove home ownership; it only feeds a +token-exact CONSISTENCY check that refuses explicit contradictions. + +Pinned surfaces: + +* record authority — valid+bound same-home allows; missing/legacy/unbound/ + foreign records refuse; +* argv consistency — token-exact: ``--profile timothy`` must NOT read as + ``tim`` (the substring heuristic's false-allow), while an exact different + profile flag contradicts and refuses; +* signal boundary — ``start_gateway(replace=True)`` on unprovable ownership + returns refusal without calling ``terminate_pid`` or writing a takeover + marker; the legitimate bound same-home target still reaches the replace + flow. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest.mock import patch + +import pytest + + +@pytest.fixture() +def profile_env(tmp_path, monkeypatch): + """Isolated HERMES_HOME mirroring tests/hermes_cli/test_profiles.py.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + default_home = tmp_path / ".hermes" + default_home.mkdir(exist_ok=True) + monkeypatch.setenv("HERMES_HOME", str(default_home)) + return tmp_home if (tmp_home := default_home) else default_home + + +def _record(pid=424242, start=111222333, home=None, argv=None): + return { + "pid": pid, + "kind": "hermes-gateway", + "argv": argv or ["python", "-m", "hermes_cli.main", "gateway", "run"], + "start_time": start, + "hermes_home": home, + } + + +class TestRecordAuthority: + def test_valid_bound_same_home_record_allows(self, profile_env): + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch( + "gateway.status._read_pid_record", + return_value=_record(home=str(profile_env / ".hermes")), + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=111222333, + ), + patch("gateway.status._read_process_cmdline", return_value=None), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is False + + def test_foreign_home_record_refuses(self, profile_env): + """Exact-home equality: another root/profile in the record refuses, + even with a bare argv that substring matching would have passed.""" + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch( + "gateway.status._read_pid_record", + return_value=_record( + home="/home/other/.hermes/profiles/timothy" + ), + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=111222333, + ), + patch("gateway.status._read_process_cmdline", return_value=None), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes" / "profiles" / "tim", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + def test_missing_record_refuses(self, profile_env): + """No valid record → ownership unprovable → refuse.""" + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch("gateway.status._read_pid_record", return_value=None), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + def test_legacy_record_without_home_refuses(self, profile_env): + """A pre-hermes_home-stamping record cannot prove ownership.""" + from gateway.run import _replace_target_belongs_to_other_profile + + legacy = _record(home=None) + legacy.pop("hermes_home") + + with ( + patch( + "gateway.status._read_pid_record", + return_value=legacy, + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=111222333, + ), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + def test_unbound_record_wrong_pid_refuses(self, profile_env): + """A record naming a DIFFERENT pid proves nothing (poisoned shape).""" + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch( + "gateway.status._read_pid_record", + return_value=_record(pid=999999, home=str(profile_env / ".hermes")), + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=111222333, + ), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + def test_unbound_record_stale_start_time_refuses(self, profile_env): + """PID reused since the record was written (start_time drift) → the + record no longer describes the live process → refuse.""" + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch( + "gateway.status._read_pid_record", + return_value=_record(start=1, home=str(profile_env / ".hermes")), + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=42, + ), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + def test_probe_exception_fails_closed(self, profile_env): + from gateway.run import _replace_target_belongs_to_other_profile + + with patch( + "gateway.status._read_pid_record", + side_effect=RuntimeError("probe exploded"), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + +class TestArgvConsistencyCheck: + """Readable argv is a consistency check ONLY — never authority.""" + + def test_prefix_collision_is_not_a_conflict(self, profile_env): + """``--profile timothy`` must NOT read as our profile ``tim``: + substring matching would have false-allowed the foreign gateway.""" + from gateway.run import ( + _looks_like_profile_conflict_from_cmdline as conflict, + ) + + tim_home = Path("/home/x/.hermes/profiles/tim") + # Foreign target advertising timothy — NOT ours. + assert ( + conflict("python -m hermes_cli.main --profile timothy gateway run", tim_home) + is True + ) + # Our own exact name stays consistent. + assert ( + conflict("python -m hermes_cli.main --profile tim gateway run", tim_home) + is False + ) + assert ( + conflict("python -m hermes_cli.main -p tim gateway run", tim_home) + is False + ) + + def test_explicit_home_flag_exact_compare(self, profile_env): + """HERMES_HOME= on the argv compares path-exactly, not by prefix.""" + from gateway.run import ( + _looks_like_profile_conflict_from_cmdline as conflict, + ) + + tim_home = Path("/home/x/.hermes/profiles/tim") + assert ( + conflict( + "python -m hermes_cli.main HERMES_HOME=/home/x/.hermes/profiles/timothy gateway run", + tim_home, + ) + is True + ) + assert ( + conflict( + "python -m hermes_cli.main --hermes-home /home/x/.hermes/profiles/tim/ gateway run", + tim_home, + ) + is False # trailing slash normalizes away + ) + + def test_default_home_refuses_any_named_profile_flag(self): + from gateway.run import ( + _looks_like_profile_conflict_from_cmdline as conflict, + ) + + root = Path("/home/x/.hermes") + assert conflict("python -m x --profile sam run", root) is True + assert conflict("python -m x -p sam run", root) is True + assert conflict("python -m x run", root) is False + + def test_consistency_contradiction_refuses_even_with_agreeing_record( + self, profile_env + ): + """Record says same-home but the argv explicitly advertises another + profile → refuse (argv contradiction wins the conservative call).""" + from gateway.run import _replace_target_belongs_to_other_profile + + with ( + patch( + "gateway.status._read_pid_record", + return_value=_record(home=str(profile_env / ".hermes")), + ), + patch( + "gateway.status._get_pid_path", + return_value=profile_env / ".hermes" / "gateway.pid", + ), + patch( + "gateway.status._get_process_start_time", + return_value=111222333, + ), + patch( + "gateway.status._read_process_cmdline", + return_value="python -m hermes_cli.main --profile other-profile gateway run", + ), + patch( + "gateway.status._get_process_hermes_home", + return_value=profile_env / ".hermes", + ), + ): + assert _replace_target_belongs_to_other_profile(424242) is True + + +class TestSignalBoundary: + """Integration witness at the destructive boundary (#89315 review req).""" + + def _run_replace(self, agent_patches): + from gateway import run as gateway_run + + calls = {"terminate": 0, "marker": 0} + + def _fake_terminate(pid, force=False): + calls["terminate"] += 1 + + def _fake_marker(pid): + calls["marker"] += 1 + + base = [ + patch("gateway.status.get_running_pid", return_value=424242), + patch.object(gateway_run, "_replace_target_belongs_to_other_profile"), + patch("gateway.status.terminate_pid", side_effect=_fake_terminate), + patch("gateway.status.write_takeover_marker", side_effect=_fake_marker), + ] + import contextlib + + with contextlib.ExitStack() as stack: + for p in base: + stack.enter_context(p) + # caller configures the guard mock + agent_patches(stack) + try: + result = asyncio_run(gateway_run.start_gateway(replace=True)) + except Exception: + result = "raised" + return result, calls + + def test_unprovable_ownership_never_signals(self, profile_env): + """Unprovable ownership → start_gateway returns False WITHOUT calling + terminate_pid or writing a takeover marker.""" + from unittest.mock import MagicMock + + def configure(stack): + guard = stack.enter_context( + patch( + "gateway.run._replace_target_belongs_to_other_profile", + return_value=True, + ) + ) + return guard + + result, calls = self._run_replace(lambda s: configure(s)) + + assert result is False + assert calls["terminate"] == 0, ( + "--replace must not signal a target whose ownership is unproven" + ) + assert calls["marker"] == 0, ( + "no takeover marker may be written for a refused target" + ) + + def test_provable_same_home_reaches_replace_flow(self, profile_env): + """Counterpart: bound same-home target still enters the replace flow + (terminate attempted) — the fail-closed gate must not disable legit + Windows-style replaces.""" + def configure(stack): + stack.enter_context( + patch( + "gateway.run._replace_target_belongs_to_other_profile", + return_value=False, + ) + ) + stack.enter_context( + patch( + "gateway.status.get_process_start_time", + return_value=111222333, + ) + ) + stack.enter_context(patch("gateway.run.time.sleep")) + + result, calls = self._run_replace(configure) + + assert calls["terminate"] == 1, ( + "a provably same-home target must still be replaceable" + ) + + +def asyncio_run(coro): + import asyncio + + return asyncio.new_event_loop().run_until_complete(_swallow(coro)) + + +async def _swallow(coro): + """Run the coroutine; later machinery (runtime locks etc.) may raise in + unit context — callers inspect side-effect counters, not the outcome.""" + try: + return await coro + except Exception: + return "raised"