diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c47b5a4..80ed778cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,9 @@ breaking changes may land in a minor release. orchestrator resolved (`BMAD_LOOP_LEDGER`) rather than re-deriving it from the YAML (#154, #769). +- Refuse `init` when `.bmad-loop`, its `policy.toml` or `.gitignore` resolves outside the + project, before any setup write (#771). + - Replace stale installed relay hooks when a project moves between Windows and POSIX. - Report stale or unverifiable Codex hook trust in `validate` and `probe-adapter` diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index bf6c575f7..6f9e93624 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -2948,6 +2948,22 @@ def install_into( return 1 bmad_loop_dir = project / ".bmad-loop" + policy_path = bmad_loop_dir / "policy.toml" + gitignore = project / ".gitignore" + # 0. confinement, before the FIRST write (#771). `_register_hooks` and + # `_copy_skills` guard their own destinations, but these three were written + # through whatever link sat at the name — a `.bmad-loop` or `.gitignore` + # symlink (a junction on Windows) out of the tree, or a dangling `policy.toml` + # link that fails `is_file()` below and is then written through. Checked up + # front, not at each write, so a refusal leaves no hook config or skills behind + # either. Strictly-below is right for all three: none may BE the project root — + # a `.bmad-loop` resolving to the root would drop policy.toml at top level, and + # the other two are files, which the root never is. An in-project link still + # passes and is written through, as before. + for target in (bmad_loop_dir, policy_path, gitignore): + if not _confined_to(target, project): + print(f"FAIL: init target escapes the project: {target}") + return 1 bmad_loop_dir.mkdir(parents=True, exist_ok=True) # 1. per-CLI hook registration @@ -2967,10 +2983,15 @@ def install_into( return 1 # 4. policy template - policy_path = bmad_loop_dir / "policy.toml" if policy_path.is_file(): print(" policy exists, leaving untouched") else: + # write_text, not atomic_write_text: #379 is about a truncating REWRITE of + # contents someone owns, and this branch only runs when no regular file is + # there — a short write loses nothing but our own template, and the torn + # TOML fails loudly at the next policy load. atomic_write_text would also + # mint the new file mkstemp's 0600 instead of the umask default, a mode + # change nothing asked for. policy_path.write_text(POLICY_TEMPLATE, encoding="utf-8") print(f" policy written: {policy_path}") @@ -2979,7 +3000,6 @@ def install_into( # Library (.bmad-loop/cache/), and the policy file itself — policy.toml is # per-machine-per-repo (it carries this machine's [mux] backend choice, and # the TUI settings editor rewrites it), so it must never travel to teammates. - gitignore = project / ".gitignore" existing = gitignore.read_text(encoding="utf-8") if gitignore.is_file() else "" have = set(existing.splitlines()) to_add = [ @@ -2993,7 +3013,9 @@ def install_into( if line not in have ] if to_add: - with gitignore.open("a", encoding="utf-8") as f: + # An append, never a replace: it keeps the operator's file mode and an + # in-project link a link. Opened by its resolved name, the one step 0 confined. + with gitignore.resolve().open("a", encoding="utf-8") as f: if existing and not existing.endswith("\n"): f.write("\n") f.write("\n".join(to_add) + "\n") diff --git a/tests/test_install.py b/tests/test_install.py index e9406849c..147639392 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -1344,6 +1344,142 @@ def test_register_hooks_refuses_a_config_path_symlinked_out_of_the_project(tmp_p assert "escapes the project" in capsys.readouterr().out +def _worktree_snapshot(root: Path) -> str: + # every path git can see, ignored ones included: an init write anywhere in the + # sandbox — hook config, skills, a top-level policy.toml — changes this listing + return git(root, "status", "--porcelain", "--ignored", "--untracked-files=all") + + +def _assert_init_left_no_state(root: Path, before: str, capsys) -> None: + # the #771 refusal runs before init's first write: the sandbox reads exactly as + # it did once the link was planted — and no `init complete` over a failed setup + out = capsys.readouterr().out + assert "FAIL: init target escapes the project" in out + assert "init complete" not in out + assert _worktree_snapshot(root) == before + claude = get_profile("claude") + assert not (root / claude.hooks.config_path).exists() + assert not (root / claude.skill_tree).exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_init_refuses_a_bmad_loop_dir_symlinked_out_of_the_project(project, tmp_path, capsys): + root, outside = project.project, tmp_path / "outside" + outside.mkdir() + (root / ".bmad-loop").symlink_to(outside, target_is_directory=True) + before = _worktree_snapshot(root) + + assert install_into(root) == 1 + + assert list(outside.iterdir()) == [] # no policy.toml through the link + _assert_init_left_no_state(root, before, capsys) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_init_refuses_a_bmad_loop_dir_that_resolves_to_the_project_root(project, capsys): + # The row that grades the `.bmad-loop` check ALONE: an out-of-project link is + # also caught by the policy.toml check (it resolves through the same link), but + # a link back to the root leaves policy.toml strictly below the project — only + # the strictly-below test on the directory itself refuses the top-level write. + root = project.project + (root / ".bmad-loop").symlink_to(root, target_is_directory=True) + before = _worktree_snapshot(root) + + assert install_into(root) == 1 + + assert not (root / "policy.toml").exists() + _assert_init_left_no_state(root, before, capsys) + + +@pytest.mark.skipif(sys.platform != "win32", reason="Windows junctions") +def test_init_refuses_a_bmad_loop_dir_junctioned_out_of_the_project(project, tmp_path, capsys): + # the redirect an unprivileged Windows session can plant without elevation + import _winapi + + root, outside = project.project, tmp_path / "outside" + outside.mkdir() + _winapi.CreateJunction(str(outside), str(root / ".bmad-loop")) + before = _worktree_snapshot(root) + + assert install_into(root) == 1 + + assert list(outside.iterdir()) == [] + _assert_init_left_no_state(root, before, capsys) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_init_refuses_a_dangling_policy_symlink_out_of_the_project(project, tmp_path, capsys): + # dangling: `is_file()` is False, so the unguarded path wrote THROUGH it + root, outside = project.project, tmp_path / "outside" + (root / ".bmad-loop").mkdir() + outside.mkdir() + (root / ".bmad-loop" / "policy.toml").symlink_to(outside / "policy.toml") + before = _worktree_snapshot(root) + + assert install_into(root) == 1 + + assert not (outside / "policy.toml").exists() + _assert_init_left_no_state(root, before, capsys) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_init_refuses_a_gitignore_symlinked_out_of_the_project(project, tmp_path, capsys): + root, outside = project.project, tmp_path / "outside.gitignore" + outside.write_text("node_modules/", encoding="utf-8") + before_bytes = outside.read_bytes() + (root / ".gitignore").unlink() # the sandbox's tracked one, swapped for the link + (root / ".gitignore").symlink_to(outside) + before = _worktree_snapshot(root) + + assert install_into(root) == 1 + + assert outside.read_bytes() == before_bytes + _assert_init_left_no_state(root, before, capsys) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_init_appends_through_a_gitignore_symlinked_inside_the_project(project): + # an in-repo indirection the operator arranged: appended through, still a link, + # the target's content and mode intact — the ablation partner of the refusal + root = project.project + real = root / "config" / "ignore" + real.parent.mkdir() + real.write_text("node_modules/\n", encoding="utf-8") + real.chmod(0o640) + link = root / ".gitignore" + link.unlink() + link.symlink_to(real) + + assert install_into(root, skills=False) == 0 + + assert link.is_symlink() + text = real.read_text(encoding="utf-8") + assert text.startswith("node_modules/\n") + assert ".bmad-loop/runs/" in text and ".bmad-loop/policy.toml" in text + assert stat.S_IMODE(real.stat().st_mode) == 0o640 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes") +def test_init_gitignore_append_preserves_content_mode_and_is_idempotent(project): + # characterization of the ordinary-file path the #771 guard must not cost + gitignore = project.project / ".gitignore" + gitignore.write_bytes(b"node_modules/\n*.log") # no trailing newline + gitignore.chmod(0o640) + + assert install_into(project.project, skills=False) == 0 + + assert gitignore.read_bytes() == ( + b"node_modules/\n*.log\n" + b".bmad-loop/runs/\n.bmad-loop/cache/\n.bmad-loop/policy.toml\n" + + f"{RENDER_DIR_REL}/\n".encode() + ) + assert stat.S_IMODE(gitignore.stat().st_mode) == 0o640 + after_first = gitignore.read_bytes() + + assert install_into(project.project, skills=False) == 0 + assert gitignore.read_bytes() == after_first + + # ------------------------------------------------ atomic hook-config rewrite (#379) # # `_register_hooks` and `provision_worktree` both PARSE the operator's hook config,