From e9bb262be737bf480f86b15e57b1fcd8680a83a8 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 15:43:54 -0700 Subject: [PATCH 1/2] fix(runs): validate run ids and stop a run ref escaping the runs dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A positional run ref reached `run_dir_for`'s exact branch raw, which recomposes `project / RUNS_DIR / ref` — so `bmad-loop delete ../../x` rmtree'd any outside directory that happened to hold a state.json. The hidden `--run-id` flag on run/sweep was never checked either, and lands in three positions at once: a directory name, a multiplexer session name, and a git ref component. A run id has exactly one legitimate producer (`new_run_id`), so validate and reject rather than sanitize — a coerced id would no longer name the run the caller asked for, breaking the id<->path<->session bijection. - `runs.RUN_ID_RE` + `is_valid_run_id`: identity for every `new_run_id()` output; excludes separators, `..`, the Windows illegal set and reserved device basenames, `.`/`:` (session-name mangling), and whitespace. - `cmd_run` / `cmd_sweep` reject an invalid supplied `--run-id` at the boundary, before the preflight side effects and before any path. - `resolve_run_dir` skips the exact branch for a ref that is absolute, climbs with `..`, or carries a separator; it falls through to partial matching, which only yields names `list_run_dirs` enumerated. - `prunable_sessions` skips session names whose stripped id is invalid, so a foreign tmux session cannot steer a run-dir path. `platform_util._MAX_SEGMENT` is promoted to a public `MAX_SEGMENT` — the run-id cap is the same filesystem-segment cap, and must stay in lockstep. Closes #104 --- CHANGELOG.md | 9 +++ src/bmad_loop/cli.py | 20 ++++++ src/bmad_loop/platform_util.py | 10 +-- src/bmad_loop/runs.py | 65 ++++++++++++++++-- tests/test_cli.py | 39 +++++++++++ tests/test_engine.py | 2 +- tests/test_platform_util.py | 6 +- tests/test_runs.py | 121 ++++++++++++++++++++++++++++++++- 8 files changed, 257 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 182c04c..495b4a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,15 @@ breaking changes may land in a minor release. ### Fixed +- **Run ids are validated, so a run ref can no longer escape the runs directory.** A positional ref + (`delete`, `stop`, `archive`, `resume`, `status`) was recomposed into a path raw, so + `bmad-loop delete ../../x` deleted any outside directory holding a `state.json`; the hidden + `--run-id` flag on `run`/`sweep` reached a directory name, a multiplexer session name and a git ref + unchecked. A supplied id must now match `[A-Za-z0-9][A-Za-z0-9_-]*` (≤ 120 chars, no reserved + Windows device name) — rejected, never sanitized, so ids stay bijective with paths and sessions — + and a ref that is absolute, climbs with `..`, or carries a separator skips the exact-match branch, + falling through to partial matching over enumerated run dirs only. Partial refs unaffected (#104). + - **An abandoned patch-restore no longer smuggles its files into the corrected story's commit.** Re-arming a story whose previous re-drive had already applied a restore patch snapshotted that patch's new (untracked) files as _pre-existing_, so every later rollback preserved them and diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 83ea91a..6cb36ef 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -32,6 +32,7 @@ from .engine import Engine from .journal import Journal, load_state, save_state from .model import RunState +from .platform_util import MAX_SEGMENT from .process_host import ProcessHostError from .runs import RUNS_DIR from .stories_engine import StoriesEngine @@ -48,6 +49,21 @@ def _policy_path(project: Path) -> Path: return project / POLICY_FILE +def _reject_bad_run_id(run_id: str | None) -> int | None: + """Guard the hidden ``--run-id`` flag before the id becomes a directory name, a + multiplexer session name and a git ref component. Rejects rather than sanitizes + (see ``runs.is_valid_run_id``): a coerced id would no longer name the run the + caller asked for. Returns 1 to abort, None to proceed.""" + if run_id is not None and not runs.is_valid_run_id(run_id): + print( + f"error: invalid --run-id {run_id!r} — expected {runs.RUN_ID_RE.pattern} " + f"(at most {MAX_SEGMENT} characters, not a reserved device name)", + file=sys.stderr, + ) + return 1 + return None + + def _reconcile_stale(project: Path, paths: bmadconfig.ProjectPaths, pol) -> None: """Tear down worktrees leaked by a prior run that stopped mid-flight, before starting a new run/sweep — the clean-finish GC never reached them. Gated on @@ -360,6 +376,8 @@ def _validate_stories_queue( def cmd_run(args: argparse.Namespace) -> int: + if (rc := _reject_bad_run_id(args.run_id)) is not None: + return rc project = _project(args) paths = bmadconfig.load_paths(project) pol = policy_mod.load(_policy_path(project)) @@ -646,6 +664,8 @@ def factory(trigger: str) -> None: def cmd_sweep(args: argparse.Namespace) -> int: + if (rc := _reject_bad_run_id(args.run_id)) is not None: + return rc project = _project(args) paths = bmadconfig.load_paths(project) pol = policy_mod.load(_policy_path(project)) diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index b763730..cbe7a15 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -51,7 +51,7 @@ | {f"LPT{s}" for s in "¹²³"} ) _ILLEGAL_SEGMENT_CHARS = re.compile(r'[<>:"/\\|?*\x00-\x1f]') -_MAX_SEGMENT = 120 # keep segment (incl. any collision suffix) well under the 255 limit +MAX_SEGMENT = 120 # keep segment (incl. any collision suffix) well under the 255 limit # git-check-ref-format(1) rejects these anywhere in a ref component: ASCII control # chars and space (\x00-\x20), DEL, and `~ ^ : ? * [ \`. `/` is added because it @@ -183,7 +183,7 @@ def safe_segment(name: str) -> str: clean name that happens to look like a sanitized-plus-digest name passes through verbatim, and case-insensitive NTFS collisions between clean names remain the caller's concern. Never raises.""" - cleaned = _ILLEGAL_SEGMENT_CHARS.sub("_", name).rstrip(". ")[:_MAX_SEGMENT] + cleaned = _ILLEGAL_SEGMENT_CHARS.sub("_", name).rstrip(". ")[:MAX_SEGMENT] if _is_reserved_basename(cleaned): cleaned = "_" + cleaned if not cleaned: @@ -191,7 +191,7 @@ def safe_segment(name: str) -> str: if cleaned == name: return name # already a legal segment — keep it byte-identical suffix = _digest_suffix(name) - return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix + return cleaned[: MAX_SEGMENT - len(suffix)] + suffix def _is_clean_ref_segment(seg: str) -> bool: @@ -202,7 +202,7 @@ def _is_clean_ref_segment(seg: str) -> bool: directory built from the same key.""" return ( bool(seg) - and len(seg) <= _MAX_SEGMENT + and len(seg) <= MAX_SEGMENT and not _ILLEGAL_REF_CHARS.search(seg) and ".." not in seg and "@{" not in seg @@ -241,4 +241,4 @@ def safe_ref_segment(name: str) -> str: if not cleaned or cleaned == "@": cleaned = "_" suffix = _digest_suffix(name) - return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix + return cleaned[: MAX_SEGMENT - len(suffix)] + suffix diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 3fd8065..f0bf08c 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -4,6 +4,7 @@ import math import os +import re import secrets import shutil import tarfile @@ -14,7 +15,13 @@ from .adapters.multiplexer import get_multiplexer from .journal import STATE_FILE, Journal, load_state, save_state from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask -from .platform_util import atomic_replace +from .platform_util import ( + MAX_SEGMENT, + atomic_replace, + has_parent_ref, + is_absolute_path, + safe_segment, +) from .process_host import get_process_host RUNS_DIR = Path(".bmad-loop") / "runs" @@ -39,6 +46,36 @@ def new_run_id() -> str: return time.strftime("%Y%m%d-%H%M%S") + "-" + secrets.token_hex(2) +# A run id is a lookup key with exactly one legitimate producer (new_run_id), and it +# lands in three positions at once: a directory name under RUNS_DIR, a multiplexer +# session name (bmad-loop-), and a git ref component (bmad-loop//). +# So an id supplied from outside is *rejected*, never sanitized — coercing it would +# break the id<->path<->session bijection the CLI relies on to find a run again. +# +# The charset is a superset of every new_run_id() output and excludes, by +# construction: path separators and `..` (traversal), `<>:"|?*` plus trailing dots +# and spaces (Windows), `.` and `:` (multiplexer session-name mangling), and all +# whitespace/control characters. It is also identity under safe_ref_segment, so the +# unit branch a run produces reads back verbatim — hence no ref check below. +RUN_ID_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9_-]*") + + +def is_valid_run_id(value: str) -> bool: + """True when ``value`` is a run id we would have produced ourselves — the guard + every externally-supplied ``--run-id`` and every id recomposed from the outside + world (a foreign multiplexer session name) must pass before it touches a path. + + The length cap is ``platform_util.MAX_SEGMENT``: a run id is a directory name. + The ``safe_segment`` identity check adds the one rule ``RUN_ID_RE`` cannot + express — the reserved Windows device basenames (``CON``, ``NUL``, ``COM1``…), + which are legal-looking ids that no filesystem will accept as a directory.""" + return ( + bool(RUN_ID_RE.fullmatch(value)) + and len(value) <= MAX_SEGMENT + and safe_segment(value) == value + ) + + def list_run_dirs(project: Path) -> list[Path]: """All run dirs containing a state.json, oldest first (run ids sort chronologically).""" @@ -100,14 +137,30 @@ def short_ref(run_id: str) -> str: return run_id.rsplit("-", 1)[-1] +def _is_path_escape(ref: str) -> bool: + """True when ``ref`` would steer ``run_dir_for``'s recomposition outside the + runs dir — it is absolute/drive-qualified, climbs with ``..``, or carries a + path separator of either flavour. Sub-check of the run-id charset rather than + `is_valid_run_id` itself: a run dir created by an older version (or by hand) + may bear a name we would no longer mint, and must stay addressable.""" + return is_absolute_path(ref) or has_parent_ref(ref) or "/" in ref or "\\" in ref + + def resolve_run_dir(project: Path, ref: str) -> Path: """Full or partial run id -> its run dir. An exact id wins outright; otherwise a partial matches when the trailing segment starts with `ref` or the full id ends with `ref` (run ids are date-prefixed, so the tail is what - distinguishes them). Raises RunRefError on no match / ambiguity.""" - exact = run_dir_for(project, ref) - if is_run(exact): - return exact + distinguishes them). Raises RunRefError on no match / ambiguity. + + The exact branch recomposes a path from the raw ref, so it is skipped for any + ref that could escape the runs dir (`bmad-loop delete ../../x` would otherwise + rmtree an outside directory that happens to hold a state.json). Such a ref + falls through to partial matching, which can only ever yield a name + `list_run_dirs` enumerated — and so cannot escape.""" + if not _is_path_escape(ref): + exact = run_dir_for(project, ref) + if is_run(exact): + return exact matches = [ d for d in list_run_dirs(project) @@ -253,6 +306,8 @@ def prunable_sessions(project: Path) -> tuple[list[str], list[str], set[str]]: if name == CTL_SESSION or not name.startswith(_SESSION_PREFIX): continue run_id = name[len(_SESSION_PREFIX) :] + if not is_valid_run_id(run_id): + continue # a foreign/mangled session name must not steer a run-dir path run_dir = run_dir_for(project, run_id) tag = tags.get(name, "") if tag: diff --git a/tests/test_cli.py b/tests/test_cli.py index 7107576..93c5b56 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -603,6 +603,45 @@ def test_run_honors_preassigned_run_id_and_writes_pid(project, monkeypatch): assert (run_dir / "engine.pid").read_text().split()[0] == str(os.getpid()) +_BAD_RUN_IDS = [ + "../../escape", # traversal out of the runs dir + "..", + "/etc/passwd", # posix absolute + "C:\\windows", # windows drive-absolute + "a/b", # path separators + "a\\b", + "a.b", # dot/colon mangle a multiplexer session name + "a:b", + "-lead", # leading dash + "a b", # whitespace + "", # empty + "CON", # reserved windows device basename +] + + +@pytest.mark.parametrize("bad", _BAD_RUN_IDS) +@pytest.mark.parametrize("command", ["run", "sweep"]) +def test_start_rejects_invalid_run_id(project, monkeypatch, capsys, command, bad): + """The hidden --run-id flag is a lookup key that becomes a directory name, a + multiplexer session name and a git ref. Reject at the boundary — before any + directory is created, and before the preflight side effects run.""" + install_bmad_config(project) + monkeypatch.setattr(cli, "Engine", _StubEngine) + monkeypatch.setattr(cli, "SweepEngine", _StubEngine) + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {r: None for r in cli.ROLES}) + + # `--run-id=` (not a separate argv token) so argparse doesn't first reject + # a leading-dash value as an unknown option — the guard is what must reject it. + assert cli.main([command, "--project", str(project.project), f"--run-id={bad}"]) == 1 + err = capsys.readouterr().err + # the stderr message is the real pin: an unguarded run/sweep would also return 1 + # here (dirty tree / missing base skills), just for the wrong reason. + assert "invalid --run-id" in err and repr(bad) in err + # rejected before the id reached a path: no run dir, inside or outside the runs dir + assert not (project.project / ".bmad-loop" / "runs").exists() + assert not (project.project / "escape").exists() + + def test_run_aborts_when_base_skills_missing(project, monkeypatch, capsys): """The orchestrator depends on the non-bundled upstream skills (bmad-dev-auto + the review hunters); a run must fail loudly at preflight (not stall mid-run) diff --git a/tests/test_engine.py b/tests/test_engine.py index 428d143..dde75d9 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -502,7 +502,7 @@ def test_run_session_labeled_task_id_capped_as_a_whole(project): engine._run_session(task, role="dev", prompt="p", seq=1, label="l" * 110) (record,) = task.sessions - assert len(record.task_id) <= platform_util._MAX_SEGMENT + assert len(record.task_id) <= platform_util.MAX_SEGMENT @pytest.mark.parametrize("key", ["6-4:cli?list", "k" * 130]) diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 465ed24..970bb3d 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -189,7 +189,7 @@ def test_retrying_unlink_propagates_missing_file(tmp_path): def _is_legal_segment(seg: str) -> bool: return ( bool(seg) - and len(seg) <= platform_util._MAX_SEGMENT + and len(seg) <= platform_util.MAX_SEGMENT and not platform_util._ILLEGAL_SEGMENT_CHARS.search(seg) and not seg.endswith((" ", ".")) and not platform_util._is_reserved_basename(seg) @@ -239,7 +239,7 @@ def test_safe_segment_distinct_dirty_keys_never_collide(): def test_safe_segment_caps_length(): out = platform_util.safe_segment("x" * 500) - assert len(out) <= platform_util._MAX_SEGMENT + assert len(out) <= platform_util.MAX_SEGMENT assert _is_legal_segment(out) @@ -356,7 +356,7 @@ def test_safe_ref_segment_distinct_dirty_keys_never_collide(): def test_safe_ref_segment_caps_length(): - assert len(platform_util.safe_ref_segment("x" * 500)) <= platform_util._MAX_SEGMENT + assert len(platform_util.safe_ref_segment("x" * 500)) <= platform_util.MAX_SEGMENT @pytest.mark.parametrize("value", _REF_CORPUS) diff --git a/tests/test_runs.py b/tests/test_runs.py index 58a2d4b..4082038 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -9,7 +9,7 @@ import pytest from conftest import escalated_run, git -from bmad_loop import runs +from bmad_loop import platform_util, runs from bmad_loop.adapters import tmux_base from bmad_loop.journal import load_state, save_state from bmad_loop.model import RunState @@ -102,6 +102,65 @@ def test_new_run_id_format(): assert re.fullmatch(r"\d{8}-\d{6}-[0-9a-f]{4}", runs.new_run_id()) +def test_is_valid_run_id_is_identity_for_generated_ids(): + """The validator must accept everything our one legitimate producer emits — + and those ids must survive both sanitizers byte-for-byte, since a run id is a + directory name (safe_segment) and a git ref component (safe_ref_segment) at once.""" + for _ in range(20): + run_id = runs.new_run_id() + assert runs.is_valid_run_id(run_id) + assert platform_util.safe_segment(run_id) == run_id + assert platform_util.safe_ref_segment(run_id) == run_id + + +@pytest.mark.parametrize("value", ["r1", "RID", "a", "A_b-C9", "x" * platform_util.MAX_SEGMENT]) +def test_is_valid_run_id_accepts(value): + assert runs.is_valid_run_id(value) + + +@pytest.mark.parametrize( + "value", + [ + "", # empty + "..", # traversal + "../x", + "..\\x", + "/etc/passwd", # posix absolute + "C:\\windows", # windows drive-absolute + "C:rel", # windows drive-relative + "a/b", # posix separator + "a\\b", # windows separator + "-lead", # leading dash (git porcelain option-lookalike) + "_lead", # leading underscore: only [A-Za-z0-9] may start an id + ".hidden", # leading dot + "a.b", # dot: mangles a multiplexer session name + "a:b", # colon: mangles a multiplexer session name, illegal in a git ref + "a b", # whitespace + "a\tb", + "a\nb", + "a\x00b", # control char + "trailing.", # windows drops trailing dots + "trailing ", # ...and trailing spaces + 'a"b', + "ab", + "a|b", + "a?b", + "a*b", + "a~b", + "a^b", + "a[b", + "a@{b", + "CON", # reserved windows device basenames, any case, with or without ext + "nul", + "COM1", + "x" * (platform_util.MAX_SEGMENT + 1), # over the segment cap + ], +) +def test_is_valid_run_id_rejects(value): + assert not runs.is_valid_run_id(value) + + def test_write_pid(tmp_path): runs.write_pid(tmp_path) tokens = (tmp_path / "engine.pid").read_text().split() @@ -164,6 +223,48 @@ def test_resolve_run_dir_ambiguous(tmp_path): runs.resolve_run_dir(tmp_path, "a1") +@pytest.mark.parametrize("ref", ["../../outside", "../outside", "a/b", "a\\b"]) +def test_resolve_run_dir_never_escapes_the_runs_dir(tmp_path, ref): + """The exact branch recomposes `project / RUNS_DIR / ref` from the raw ref, so a + ref carrying separators or `..` must never reach it — otherwise + `bmad-loop delete ../../x` rmtree's any outside directory that happens to hold a + state.json. Such refs fall through to partial matching, which can only yield a + name `list_run_dirs` enumerated, i.e. an immediate child of the runs dir. + + Stated as containment rather than no-match because `a\\b` is one legal directory + name on POSIX (inside the runs dir, so it legitimately resolves) and a nested + path on Windows (where it must not).""" + project = tmp_path / "proj" + _make_run(project, "20260620-143025-a1b2") + runs_dir = (project / ".bmad-loop" / "runs").resolve() + # plant a state.json exactly where the un-gated exact branch would land + planted = (project / ".bmad-loop" / "runs" / ref).resolve() + planted.mkdir(parents=True, exist_ok=True) + (planted / "state.json").write_text("{}") + + try: + got = runs.resolve_run_dir(project, ref) + except runs.RunRefError as e: + assert "no such run" in str(e) + else: + assert got.resolve().parent == runs_dir # an enumerated run dir, never an escape + assert (planted / "state.json").is_file() # never consumed as a run + + +def test_resolve_run_dir_absolute_ref_never_escapes(tmp_path): + """`run_dir_for(project, "/abs")` is `Path("/abs")` — `/`-join discards the + project prefix entirely, so an absolute ref escapes without needing a `..`.""" + project = tmp_path / "proj" + _make_run(project, "20260620-143025-a1b2") + outside = tmp_path / "outside" + outside.mkdir() + (outside / "state.json").write_text("{}") + + with pytest.raises(runs.RunRefError, match="no such run"): + runs.resolve_run_dir(project, str(outside)) + assert (outside / "state.json").is_file() + + def test_resolve_run_dir_exact_wins_over_ambiguity(tmp_path): # An exact id resolves even when another run's id ends with it (which would # otherwise be an ambiguous partial match). @@ -480,6 +581,24 @@ def test_prunable_sessions_partitions(tmp_path, monkeypatch): assert unknown == set() +def test_prunable_sessions_skips_invalid_run_ids(tmp_path, monkeypatch): + """A session name is untrusted input (anyone can create one). Stripping the + prefix off `bmad-loop-../../x` would hand `run_dir_for` a traversing id, and a + tagged session would then steer engine_liveness — and prune_sessions' kill — at + a path outside the runs dir. Reject before recomposing.""" + mine = runs.project_tag(tmp_path) + good = _make_state_run(tmp_path, "fin-1") + (good / "engine.pid").write_text(str(_dead_pid())) + + sessions = ["bmad-loop-fin-1", "bmad-loop-../../x", "bmad-loop-a.b", "bmad-loop-"] + monkeypatch.setattr(runs, "tmux_sessions", lambda: sessions) + monkeypatch.setattr(runs, "session_project_tags", lambda: dict.fromkeys(sessions, mine)) + + prunable, live, unknown = runs.prunable_sessions(tmp_path) + assert prunable == ["fin-1"] + assert live == [] and unknown == set() + + def test_prunable_sessions_flags_unknown(tmp_path, monkeypatch): # live pid, unreadable identity (win32 ERROR_ACCESS_DENIED) → prunable anyway # (unknown never blocks cleanup) but flagged so frontends can warn. From 5641d4730d52734c3c28f9845f3eb0bff3c0b891 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 16:53:20 -0700 Subject: [PATCH 2/2] fix(tui): validate the run id recovered from a ctl-window name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _ctl_window_candidates reverse-maps - control-session window names through a fully permissive (.+) group, so a foreign window named run-../../x steered run_dir_for's liveness read — and, for an untagged window, the run-dir ownership fallback — at a path outside the runs dir. Same untrusted-name class prunable_sessions already guards; apply the same is_valid_run_id gate before recomposing. Legit windows are unaffected: the TUI is the only creator and names them from enumerated run ids. --- CHANGELOG.md | 4 +++- src/bmad_loop/tui/launch.py | 2 ++ tests/test_tui_launch.py | 45 +++++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 495b4a4..8e53e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,9 @@ breaking changes may land in a minor release. unchecked. A supplied id must now match `[A-Za-z0-9][A-Za-z0-9_-]*` (≤ 120 chars, no reserved Windows device name) — rejected, never sanitized, so ids stay bijective with paths and sessions — and a ref that is absolute, climbs with `..`, or carries a separator skips the exact-match branch, - falling through to partial matching over enumerated run dirs only. Partial refs unaffected (#104). + falling through to partial matching over enumerated run dirs only. Ids recovered from the outside + world — a `bmad-loop-` session name, a `-` control-session window name — pass the + same validator before they steer a path. Partial refs unaffected (#104). - **An abandoned patch-restore no longer smuggles its files into the corrected story's commit.** Re-arming a story whose previous re-drive had already applied a restore patch snapshotted that diff --git a/src/bmad_loop/tui/launch.py b/src/bmad_loop/tui/launch.py index 28517f6..024bf96 100644 --- a/src/bmad_loop/tui/launch.py +++ b/src/bmad_loop/tui/launch.py @@ -203,6 +203,8 @@ def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]: m = _CTL_WINDOW_RE.match(name) if m is None: continue # not a run window (e.g. the session's initial shell) + if not runs.is_valid_run_id(m.group(1)): + continue # a foreign/mangled window name must not steer a run-dir path run_dir = runs.run_dir_for(project, m.group(1)) if tag: if tag != mine: diff --git a/tests/test_tui_launch.py b/tests/test_tui_launch.py index e990661..177f4eb 100644 --- a/tests/test_tui_launch.py +++ b/tests/test_tui_launch.py @@ -299,6 +299,51 @@ def fake(argv, **kwargs): assert killed == [["tmux", "kill-window", "-t", "@3"]] +def test_prune_ctl_windows_skips_invalid_run_ids(monkeypatch, tmp_path: Path): + """A ctl-window name is untrusted input (anyone can rename a tmux window). + Stripping the kind prefix off `run-../../x` would hand run_dir_for a + traversing id, steering the liveness read — and, for an untagged window, + the run-dir ownership fallback — at a path outside the runs dir. Reject + before recomposing (mirrors runs.prunable_sessions).""" + from bmad_loop import runs + + mine = runs.project_tag(tmp_path) + # a real runs dir, so the traversal has an existing anchor to climb from + (tmp_path / ".bmad-loop" / "runs").mkdir(parents=True) + # where the un-gated recomposition of `run-../../planted` would land: an + # outside dir whose state.json would otherwise claim the untagged window + planted = tmp_path / "planted" + planted.mkdir() + (planted / "state.json").write_text("{}") + + windows = ( + f"@2\tsweep-20260101-000000-dead\t{mine}\n" # legit orphan — still killed + f"@3\trun-../../x\t{mine}\n" # traversal — skipped + f"@5\tsweep-a.b\t{mine}\n" # invalid charset — skipped + "@6\trun-../../planted\t\n" # untagged — outside state.json must not claim it + ) + killed: list[list[str]] = [] + + def fake(argv, **kwargs): + verb = argv[1] + if verb == "has-session": + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + if verb == "display-message": # current window is none of the rows + return subprocess.CompletedProcess(argv, 0, stdout="@1\n", stderr="") + if verb == "list-windows": + return subprocess.CompletedProcess(argv, 0, stdout=windows, stderr="") + if verb == "kill-window": + killed.append(list(argv)) + return subprocess.CompletedProcess(argv, 0, stdout="", stderr="") + + monkeypatch.setattr(tmux_base.subprocess, "run", fake) + monkeypatch.setattr(tmux_base.shutil, "which", lambda name: f"/usr/bin/{name}") + + assert launch.prunable_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] + assert launch.prune_ctl_windows(tmp_path) == ["sweep-20260101-000000-dead"] + assert killed == [["tmux", "kill-window", "-t", "@2"]] + + def test_prune_ctl_windows_no_session(monkeypatch, tmp_path: Path): def fake(argv, **kwargs): # has-session reports the ctl session is gone return subprocess.CompletedProcess(argv, 1, stdout="", stderr="")