diff --git a/cli.py b/cli.py index cacc75908..46cf3a209 100755 --- a/cli.py +++ b/cli.py @@ -517,6 +517,15 @@ def _git_repo_root() -> Optional[str]: return None +def _path_is_within_root(path: Path, root: Path) -> bool: + """Return True when a resolved path stays within the expected root.""" + try: + path.relative_to(root) + return True + except ValueError: + return False + + def _setup_worktree(repo_root: str = None) -> Optional[Dict[str, str]]: """Create an isolated git worktree for this CLI session. @@ -570,12 +579,29 @@ def _setup_worktree(repo_root: str = None) -> Optional[Dict[str, str]]: include_file = Path(repo_root) / ".worktreeinclude" if include_file.exists(): try: + repo_root_resolved = Path(repo_root).resolve() + wt_path_resolved = wt_path.resolve() for line in include_file.read_text().splitlines(): entry = line.strip() if not entry or entry.startswith("#"): continue src = Path(repo_root) / entry dst = wt_path / entry + # Prevent path traversal and symlink escapes: both the resolved + # source and the resolved destination must stay inside their + # expected roots before any file or symlink operation happens. + try: + src_resolved = src.resolve(strict=False) + dst_resolved = dst.resolve(strict=False) + except (OSError, ValueError): + logger.debug("Skipping invalid .worktreeinclude entry: %s", entry) + continue + if not _path_is_within_root(src_resolved, repo_root_resolved): + logger.warning("Skipping .worktreeinclude entry outside repo root: %s", entry) + continue + if not _path_is_within_root(dst_resolved, wt_path_resolved): + logger.warning("Skipping .worktreeinclude entry that escapes worktree: %s", entry) + continue if src.is_file(): dst.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(str(src), str(dst)) @@ -583,7 +609,7 @@ def _setup_worktree(repo_root: str = None) -> Optional[Dict[str, str]]: # Symlink directories (faster, saves disk) if not dst.exists(): dst.parent.mkdir(parents=True, exist_ok=True) - os.symlink(str(src.resolve()), str(dst)) + os.symlink(str(src_resolved), str(dst)) except Exception as e: logger.debug("Error copying .worktreeinclude entries: %s", e) diff --git a/tests/test_worktree_security.py b/tests/test_worktree_security.py new file mode 100644 index 000000000..73a242e0f --- /dev/null +++ b/tests/test_worktree_security.py @@ -0,0 +1,130 @@ +"""Security-focused integration tests for CLI worktree setup.""" + +import subprocess +from pathlib import Path + +import pytest + + +@pytest.fixture +def git_repo(tmp_path): + """Create a temporary git repo for testing real cli._setup_worktree behavior.""" + repo = tmp_path / "test-repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True, capture_output=True) + subprocess.run(["git", "config", "user.email", "test@test.com"], cwd=repo, check=True, capture_output=True) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True, capture_output=True) + (repo / "README.md").write_text("# Test Repo\n") + subprocess.run(["git", "add", "."], cwd=repo, check=True, capture_output=True) + subprocess.run(["git", "commit", "-m", "Initial commit"], cwd=repo, check=True, capture_output=True) + return repo + + +def _force_remove_worktree(info: dict | None) -> None: + if not info: + return + subprocess.run( + ["git", "worktree", "remove", info["path"], "--force"], + cwd=info["repo_root"], + capture_output=True, + check=False, + ) + subprocess.run( + ["git", "branch", "-D", info["branch"]], + cwd=info["repo_root"], + capture_output=True, + check=False, + ) + + +class TestWorktreeIncludeSecurity: + def test_rejects_parent_directory_file_traversal(self, git_repo): + import cli as cli_mod + + outside_file = git_repo.parent / "sensitive.txt" + outside_file.write_text("SENSITIVE DATA") + (git_repo / ".worktreeinclude").write_text("../sensitive.txt\n") + + info = None + try: + info = cli_mod._setup_worktree(str(git_repo)) + assert info is not None + + wt_path = Path(info["path"]) + assert not (wt_path.parent / "sensitive.txt").exists() + assert not (wt_path / "../sensitive.txt").resolve().exists() + finally: + _force_remove_worktree(info) + + def test_rejects_parent_directory_directory_traversal(self, git_repo): + import cli as cli_mod + + outside_dir = git_repo.parent / "outside-dir" + outside_dir.mkdir() + (outside_dir / "secret.txt").write_text("SENSITIVE DIR DATA") + (git_repo / ".worktreeinclude").write_text("../outside-dir\n") + + info = None + try: + info = cli_mod._setup_worktree(str(git_repo)) + assert info is not None + + wt_path = Path(info["path"]) + escaped_dir = wt_path.parent / "outside-dir" + assert not escaped_dir.exists() + assert not escaped_dir.is_symlink() + finally: + _force_remove_worktree(info) + + def test_rejects_symlink_that_resolves_outside_repo(self, git_repo): + import cli as cli_mod + + outside_file = git_repo.parent / "linked-secret.txt" + outside_file.write_text("LINKED SECRET") + (git_repo / "leak.txt").symlink_to(outside_file) + (git_repo / ".worktreeinclude").write_text("leak.txt\n") + + info = None + try: + info = cli_mod._setup_worktree(str(git_repo)) + assert info is not None + + assert not (Path(info["path"]) / "leak.txt").exists() + finally: + _force_remove_worktree(info) + + def test_allows_valid_file_include(self, git_repo): + import cli as cli_mod + + (git_repo / ".env").write_text("SECRET=***\n") + (git_repo / ".worktreeinclude").write_text(".env\n") + + info = None + try: + info = cli_mod._setup_worktree(str(git_repo)) + assert info is not None + + copied = Path(info["path"]) / ".env" + assert copied.exists() + assert copied.read_text() == "SECRET=***\n" + finally: + _force_remove_worktree(info) + + def test_allows_valid_directory_include(self, git_repo): + import cli as cli_mod + + assets_dir = git_repo / ".venv" / "lib" + assets_dir.mkdir(parents=True) + (assets_dir / "marker.txt").write_text("venv marker") + (git_repo / ".worktreeinclude").write_text(".venv\n") + + info = None + try: + info = cli_mod._setup_worktree(str(git_repo)) + assert info is not None + + linked_dir = Path(info["path"]) / ".venv" + assert linked_dir.is_symlink() + assert (linked_dir / "lib" / "marker.txt").read_text() == "venv marker" + finally: + _force_remove_worktree(info)