diff --git a/tests/tools/test_bot_relay_windows_paths.py b/tests/tools/test_bot_relay_windows_paths.py new file mode 100644 index 0000000000..01064b313f --- /dev/null +++ b/tests/tools/test_bot_relay_windows_paths.py @@ -0,0 +1,147 @@ +"""Windows-path viability and venv CLI resolution for bot relay (#93590). + +Two failures on a Windows desktop install talking to a remote gateway: + +1. ``waiter_command`` embeds the reply path into generated ``python -c`` + source with ``!r``. repr escapes each backslash, but the Windows + execution layer the waiter runs under folds ``\\`` back to ``\`` — + ``\\U`` in ``C:\\Users\\...`` then parses as a unicode escape and + SyntaxErrors the whole script. The raw-string prefix keeps the folded + single backslash a literal; POSIX paths contain no backslashes, so it + is a no-op there, and ``\\'`` inside a raw literal still cannot + terminate the string, so the injection defense from #93091's + python -c hardening is unchanged. + +2. ``local_delivery_command`` hardcoded ``"hermes"``, relying on PATH — + which service contexts (systemd units, desktop launchers, non-login + SSH shells) do not provide, so delivery died with ENOENT. It now + resolves the CLI next to this gateway's own interpreter (the venv + bin/Scripts sibling), falling back to the bare name. The #93091 + turn-lock recognition in bot_mode_dm matches the CLI element by + basename so resolved absolute paths (and ``hermes.exe``) still take + the per-profile lock. +""" + +import ast +import shlex +from pathlib import Path + +import tools.bot_mode_dm as bot_mode_dm +import tools.bot_relay as bot_relay + + +ENV = {"id": "d" * 32, "target_handle": "researcher", "target_connection": "ssh-vps"} + + +def _waiter_code(root, env=None) -> str: + cmd = bot_relay.waiter_command(root, env or ENV) + parts = shlex.split(cmd) + return parts[parts.index("-c") + 1] + + +def test_waiter_windows_path_compiles_after_backslash_folding(): + """A Windows reply path must survive the execution layer folding the + repr-escaped double backslash back to a single one — the exact shape + that SyntaxErrored with ``\\U`` on #93590's reporter setup.""" + code = _waiter_code("C:\\Users\\joshu\\.hermes") + assert "C:" in code # sanity: the Windows path made it into the payload + folded = code.replace("\\\\", "\\") + # Raw literals: `p = r'C:\Users\joshu\...'` — no unicode-escape crash. + compile(folded, "", "exec") + + +def test_waiter_posix_path_and_label_values_roundtrip(): + """On POSIX (backslash-free paths) the raw prefix changes nothing.""" + root = Path("/tmp/hermes-home") + code = _waiter_code(root) + assigns = { + t.targets[0].id: t.value + for t in ast.parse(code).body + if isinstance(t, ast.Assign) and isinstance(t.targets[0], ast.Name) + } + expected = str(root / "bot_relay" / "replies" / f"{ENV['id']}.json") + assert assigns["p"].value == expected + assert assigns["label"].value == "@researcher on ssh-vps" + # The literals are raw-prefixed in the generated source. + assert "\np = r'" in code + assert "\nlabel = r'" in code + + +def test_waiter_raw_prefix_keeps_injection_defense(): + """Hostile roster fields must stay data under the raw prefix too.""" + inj = { + "id": "e" * 32, + "target_handle": "researcher", + "target_connection": "x'); __import__('sys').exit(2); print('x", + } + code = _waiter_code(Path("/tmp/hermes-home"), inj) + compile(code, "", "exec") + calls = [ + n.func.id + for n in ast.walk(ast.parse(code)) + if isinstance(n, ast.Call) and isinstance(n.func, ast.Name) + ] + # The generated waiter only calls str/print/compile-free builtins by + # name; the payload's __import__ must remain a string literal, not a + # live call — parse it back and confirm it stayed data. + assert "__import__" not in calls + assert "x'); __import__('sys').exit(2); print('x" in code + + +def test_local_delivery_resolves_sibling_hermes(tmp_path, monkeypatch): + bin_dir = tmp_path / "venv" / "bin" + bin_dir.mkdir(parents=True) + sibling = bin_dir / "hermes" + sibling.touch() + sibling.chmod(0o755) + monkeypatch.setattr("sys.executable", str(bin_dir / "python")) + + argv = bot_relay.local_delivery_command("ops", "query.json") + assert argv[0] == str(sibling) + assert argv[1:3] == ["-p", "ops"] + assert argv[argv.index("--query-file") + 1] == "query.json" + + +def test_local_delivery_falls_back_to_bare_name(tmp_path, monkeypatch): + empty = tmp_path / "nowhere" + empty.mkdir(parents=True) + monkeypatch.setattr("sys.executable", str(empty / "python")) + + argv = bot_relay.local_delivery_command("ops", "query.json") + assert argv[0] == "hermes" + assert argv[1:3] == ["-p", "ops"] + + +def test_delivery_lock_recognizes_resolved_cli_paths(tmp_path, monkeypatch): + """The #93091 per-profile turn lock must keep matching delivery argvs + now that argv[0] may be a resolved absolute path (or hermes.exe).""" + acquired = [] + + class _Ctx: + def __enter__(self): + acquired.append("locked") + return self + + def __exit__(self, *exc): + return False + + monkeypatch.setattr(bot_relay, "acquire_turn_lock", lambda root, profile: _Ctx()) + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + with bot_mode_dm._delivery_lock( + [str(tmp_path / "venv" / "bin" / "hermes"), "-p", "ops", "chat"], + stdin_file=False, + ): + pass + with bot_mode_dm._delivery_lock(["hermes", "-p", "ops", "chat"], stdin_file=False): + pass + with bot_mode_dm._delivery_lock( + ["C:\\venv\\Scripts\\hermes.exe", "-p", "ops", "chat"], stdin_file=False + ): + pass + assert acquired == ["locked", "locked", "locked"] + + # Unrelated argvs still bypass the lock entirely. + with bot_mode_dm._delivery_lock(["python", "-m", "whatever"], stdin_file=False): + pass + assert acquired == ["locked", "locked", "locked"] diff --git a/tests/tools/test_bot_turn_lock.py b/tests/tools/test_bot_turn_lock.py index 5323595231..1b02ae6feb 100644 --- a/tests/tools/test_bot_turn_lock.py +++ b/tests/tools/test_bot_turn_lock.py @@ -247,13 +247,17 @@ def test_peer_stdin_delivery_skips_local_lock(root, tmp_path, monkeypatch): def test_local_delivery_command_never_reenters_the_lock(): """The gateway deliver handler runs local_delivery_command ALREADY holding - the profile lock. That argv must stay a raw `hermes -p … chat` invocation: + the profile lock. That argv must stay a raw hermes CLI invocation: routing it through the --run-delivery wrapper would make the child hit - _delivery_lock (argv[0]=='hermes' and argv[1]=='-p'), burn the full wait + _delivery_lock (hermes CLI + '-p'), burn the full wait budget against its parent's flock, and fail every relay delivery with - target_busy.""" + target_busy. argv[0] may be a resolved venv path (#93590) — the lock + matcher and this assertion both go by basename.""" + from pathlib import Path + argv = bot_relay.local_delivery_command("ops", "/tmp/q.txt") - assert argv[:3] == ["hermes", "-p", "ops"] + assert argv[1:3] == ["-p", "ops"] + assert Path(argv[0]).name in ("hermes", "hermes.exe") assert "--run-delivery" not in argv assert not any("bot_mode_dm" in part for part in argv) diff --git a/tools/bot_mode_dm.py b/tools/bot_mode_dm.py index 6d88082f58..5b27cbbacc 100644 --- a/tools/bot_mode_dm.py +++ b/tools/bot_mode_dm.py @@ -527,7 +527,19 @@ def _delivery_lock(argv: list[str], *, stdin_file: bool): lock in ``tools.bot_relay``. Peer transports (stdin mode) run on the remote gateway; their turn is locked THERE by its own deliver path. """ - if stdin_file or len(argv) < 3 or argv[0] != "hermes" or argv[1] != "-p": + # The CLI element is matched by basename: local_delivery_command now + # resolves the venv-relative hermes next to this gateway's interpreter + # (#93590 — service contexts lack PATH), so argv[0] may be an absolute + # path (and on Windows carries the .exe suffix). Split on both + # separators so the shape matches regardless of which platform built + # the argv. + cli = (argv[0] if argv else "").rsplit("\\", 1)[-1].rsplit("/", 1)[-1] + if ( + stdin_file + or len(argv) < 3 + or cli not in ("hermes", "hermes.exe") + or argv[1] != "-p" + ): return contextlib.nullcontext() from tools.bot_relay import acquire_turn_lock diff --git a/tools/bot_relay.py b/tools/bot_relay.py index 5c8c76f0a1..8abc53762e 100644 --- a/tools/bot_relay.py +++ b/tools/bot_relay.py @@ -496,10 +496,18 @@ def waiter_command(root: Path | str, envelope: dict) -> str: ) # Encode label with !r so roster fields cannot break out of the generated # python -c source (quotes, parens, or extra statements in connection_id). + # The raw-string prefix keeps Windows paths viable: repr escapes each + # backslash ("C:\\Users\\..."), but the Windows execution layer the + # waiter runs under folds "\\" back to "\", which turns "\U" into an + # invalid unicode escape and SyntaxErrors the whole script (#93590). + # With the r prefix the folded single backslash parses as a literal. + # POSIX paths contain no backslashes, so the prefix is a no-op there, + # and \' inside a raw literal still cannot terminate the string, so + # the injection defense above is unchanged. code = ( "import json,os,sys,time\n" - f"p = {reply_path!r}\n" - f"label = {label!r}\n" + f"p = r{reply_path!r}\n" + f"label = r{label!r}\n" f"deadline = time.time() + {REPLY_WAIT_SECONDS}\n" "while time.time() < deadline:\n" " if os.path.exists(p):\n" @@ -526,10 +534,26 @@ def waiter_command(root: Path | str, envelope: dict) -> str: # ── delivery command (used by the deliver RPC on the TARGET gateway) ──────── +def _hermes_cli() -> str: + """Resolve the hermes CLI beside this gateway's own interpreter. + + The deliver RPC runs on the target gateway, whose process is the venv + python — its bin/Scripts directory holds the matching ``hermes`` + entrypoint. A bare ``"hermes"`` relies on PATH, which is exactly what + service contexts (systemd units, desktop launchers, non-login SSH + shells) do not provide, so delivery died with ENOENT there (#93590). + Falls back to the bare name when no sibling exists (e.g. running from + a source tree without an installed script), preserving PATH lookup. + """ + exe = Path(sys.executable or "") + sibling = exe.parent / ("hermes.exe" if os.name == "nt" else "hermes") + return str(sibling) if sibling.is_file() else "hermes" + + def local_delivery_command(profile: str, query_file: str) -> list[str]: """argv that delivers a DM into ``profile``'s Bot Chat on THIS gateway.""" return [ - "hermes", + _hermes_cli(), "-p", profile, "chat",