diff --git a/fixos/cli/_cleanup_space.py b/fixos/cli/_cleanup_space.py index ceffa28..3987039 100644 --- a/fixos/cli/_cleanup_space.py +++ b/fixos/cli/_cleanup_space.py @@ -10,6 +10,7 @@ from __future__ import annotations import json +import os import re import shutil import subprocess @@ -151,6 +152,7 @@ def _confirm_remove() -> bool: def _remove_path(path: Path) -> tuple[bool, str]: + path = Path(path) try: if path.is_symlink() or path.is_file(): path.unlink() @@ -160,6 +162,8 @@ def _remove_path(path: Path) -> tuple[bool, str]: except PermissionError as exc: return False, f"PERMISSION_DENIED:{exc}" except OSError as exc: + if exc.errno in (1, 13) or "permission denied" in str(exc).lower(): + return False, f"PERMISSION_DENIED:{exc}" if path.is_dir() and not path.is_symlink(): shutil.rmtree(path, ignore_errors=True) if not path.exists(): @@ -224,6 +228,195 @@ def _batch_sudo_remove_tmp(items: list[dict], path_key: str = "path") -> tuple[i return removed, freed +def _project_artifact_allowed( + path: Path | str | dict[str, Any], + project_root: Path | str | None = None, +) -> bool: + """Strictly validate that path is a safe project artifact directory inside project_root. + + The path must: + - be an existing directory, + - not be a symlink (neither the artifact itself nor any ancestor up to project root), + - resolve strictly inside resolved project root (not equal to project root, not escaping), + - not be a mount point and contain no nested mounts, + - have the same filesystem device as project root. + """ + if isinstance(path, dict): + if project_root is None: + project_root = path.get("project") + path = path.get("path") + + if path is None: + return False + + try: + path = Path(path) + if project_root is None: + project_root = path.parent + else: + project_root = Path(project_root) + except (TypeError, ValueError, OSError): + return False + + # Path and project_root must be existing directories and not symlinks + if path.is_symlink() or project_root.is_symlink(): + return False + if not path.is_dir() or not project_root.is_dir(): + return False + + # Disallow forbidden critical names + if path.name in (".git", "", ".", ".."): + return False + + # Check structural relationship before resolving + try: + rel = path.absolute().relative_to(project_root.absolute()) + except ValueError: + return False + + if rel == Path(".") or not rel.parts: + return False + + # Ensure no symlinks in path segments from project_root down to path + curr = project_root.absolute() + for part in rel.parts: + curr = curr / part + if curr.is_symlink(): + return False + + # Check fully resolved paths to prevent symlink traversal or escaping + try: + resolved_proj = project_root.resolve() + resolved_path = path.resolve() + except (OSError, RuntimeError): + return False + + if resolved_path.is_symlink() or resolved_proj.is_symlink(): + return False + + try: + rel_resolved = resolved_path.relative_to(resolved_proj) + except ValueError: + return False + + if rel_resolved == Path(".") or not rel_resolved.parts: + return False + + # Check plain path + from fixos.diagnostics.venv_cleanup import _plain_path + if not _plain_path(resolved_path): + return False + + # Check mount points + if path.is_mount() or resolved_path.is_mount(): + return False + + # Check filesystem devices + try: + dev_art = path.stat().st_dev + dev_proj = project_root.stat().st_dev + if dev_art != dev_proj: + return False + except OSError: + pass + + # Check nested mounts inside path + try: + device = path.stat().st_dev + for parent, dirs, _ in os.walk(path, followlinks=False): + for name in dirs: + child = Path(parent) / name + if child.is_symlink(): + continue + if child.is_mount(): + return False + try: + if child.stat().st_dev != device: + return False + except OSError: + pass + except OSError: + pass + + # Check system mount points via psutil if available + try: + import psutil + mounts = {Path(part.mountpoint).resolve() for part in psutil.disk_partitions(all=True)} + if any(m == resolved_path or resolved_path in m.parents for m in mounts): + return False + except (ImportError, OSError, RuntimeError, ValueError): + pass + + return True + + +def _batch_sudo_remove_project_artifacts( + items: list[dict], path_key: str = "path" +) -> tuple[int, int, list[dict]]: + """Remove root-owned stale project artifacts in batches using non-interactive sudo if available. + + Only items that pass strict `_project_artifact_allowed` validation are removed. + Returns (removed_count, freed_bytes, unhandled_items). + """ + if not items: + return 0, 0, [] + + project_items: list[dict] = [] + for item in items: + p = Path(item[path_key]) + if p.parent == Path("/tmp"): + continue + if "project" in item or (p.parent != Path("/") and p.parent != Path(".")): + project_items.append(item) + + if not project_items: + return 0, 0, [] + + validated_safe: list[dict] = [] + unhandled: list[dict] = [] + + for item in project_items: + p = Path(item[path_key]) + proj = item.get("project") or p.parent + if _project_artifact_allowed(p, proj): + validated_safe.append(item) + else: + unhandled.append(item) + click.echo( + click.style( + f" Pominięto usuwanie sudo dla {item.get('description', str(p))}: " + f"ścieżka nie przeszła weryfikacji bezpieczeństwa (symlink lub punkt montowania)", + fg="yellow", + ) + ) + + if not validated_safe: + return 0, 0, unhandled + + if shutil.which("sudo") is None: + unhandled.extend(validated_safe) + return 0, 0, unhandled + + removed = 0 + freed = 0 + chunk_size = 50 + for start in range(0, len(validated_safe), chunk_size): + chunk = validated_safe[start : start + chunk_size] + cmd = ["sudo", "-n", "rm", "-rf", "--", *[str(it[path_key]) for it in chunk]] + try: + res = _run_cmd(cmd, timeout=60) + if res.returncode == 0: + removed += len(chunk) + freed += sum(int(it.get("size_bytes", 0) or 0) for it in chunk) + else: + unhandled.extend(chunk) + except (OSError, subprocess.TimeoutExpired): + unhandled.extend(chunk) + break + + return removed, freed, unhandled + + def _remove_selected( candidates: list[dict], selected: list[int], path_key: str = "path" ) -> None: @@ -260,6 +453,55 @@ def _remove_selected( fg="green", ) ) + + proj_removed, proj_freed, unhandled_proj = _batch_sudo_remove_project_artifacts( + perm_denied_items, path_key=path_key + ) + if proj_removed > 0: + removed_count += proj_removed + freed_bytes += proj_freed + failed_count = max(0, failed_count - proj_removed) + if verbose: + click.echo( + click.style( + f" Usunięto przez sudo (projekty): {proj_removed} pozycji ({_format_bytes(proj_freed)})", + fg="green", + ) + ) + if unhandled_proj: + click.echo( + click.style( + f" Brak uprawnień (PermissionError) dla {len(unhandled_proj)} artefaktów projektów:", + fg="yellow", + ) + ) + for it in unhandled_proj[:_MAX_DISPLAY_CANDIDATES]: + click.echo( + click.style( + f" - {it.get('description', it.get(path_key))}: {it.get(path_key)}", + fg="yellow", + ) + ) + remediation_paths = [ + str(it[path_key]) + for it in unhandled_proj + if not Path(it[path_key]).is_symlink() and not Path(it[path_key]).is_mount() + ] + if remediation_paths: + click.echo( + click.style( + " Polecenie naprawcze (uruchom ręcznie z sudo):", + fg="cyan", + ) + ) + paths_arg = " ".join(f"'{p}'" if " " in p else p for p in remediation_paths[:20]) + click.echo( + click.style( + f" sudo rm -rf -- {paths_arg}", + fg="cyan", + bold=True, + ) + ) if not verbose: click.echo( click.style( diff --git a/project/TICKETS.md b/project/TICKETS.md index ec097be..2464515 100644 --- a/project/TICKETS.md +++ b/project/TICKETS.md @@ -93,7 +93,18 @@ This file indexes governance tickets without taking ownership of | **ticket-089** | [`README.md`](./ticket-089/README.md) | - | - | - | - | - | | **ticket-090** | [`README.md`](./ticket-090/README.md) | - | - | - | - | - | | **ticket-091** | [`README.md`](./ticket-091/README.md) | - | - | - | - | - | +| **ticket-092** | [`README.md`](./ticket-092/README.md) | - | - | - | - | - | +| **ticket-093** | [`README.md`](./ticket-093/README.md) | - | - | - | - | - | +| **ticket-094** | [`README.md`](./ticket-094/README.md) | - | - | - | - | - | +| **ticket-095** | [`README.md`](./ticket-095/README.md) | - | - | - | - | - | +| **ticket-096** | [`README.md`](./ticket-096/README.md) | - | - | - | - | - | +| **ticket-097** | [`README.md`](./ticket-097/README.md) | - | - | - | - | - | +| **ticket-100** | [`README.md`](./ticket-100/README.md) | - | - | - | - | - | +| **ticket-101** | [`README.md`](./ticket-101/README.md) | - | - | - | - | - | +| **ticket-102** | [`README.md`](./ticket-102/README.md) | - | - | - | - | - | +| **ticket-103** | [`README.md`](./ticket-103/README.md) | - | - | - | - | - | | **ticket-104** | [`README.md`](./ticket-104/README.md) | - | - | - | - | - | | **ticket-105** | [`README.md`](./ticket-105/README.md) | - | - | - | - | - | | **ticket-107** | [`README.md`](./ticket-107/README.md) | - | - | - | - | - | +| **ticket-108** | [`README.md`](./ticket-108/README.md) | - | - | - | - | - | diff --git a/project/ticket-108/README.md b/project/ticket-108/README.md new file mode 100644 index 0000000..75f5b08 --- /dev/null +++ b/project/ticket-108/README.md @@ -0,0 +1,26 @@ +# Ticket 108: Handle root-owned project artifacts in projects-old cleanup with bounded non-interactive sudo fallback + +- **ID**: ticket-108 +- **Owner**: unresolved:human +- **Status**: IN_PROGRESS +- **Workflow state**: EDIT +- **Created**: 2026-10-26 + +## Goal and scope + +1. In `fixos/cli/_cleanup_space.py`, handle `PermissionError` during project artifact cleanup (`_cleanup_projects_old` / `_remove_selected`) when build tools or Docker leave root-owned artifacts (e.g. `node_modules`, `target`, `.pytest_cache`). +2. Add strict `_project_artifact_allowed` validation ensuring paths resolve strictly within the project root, without following symlinks or escaping the project root, and avoiding mount points. +3. Use bounded non-interactive sudo (`sudo -n rm -rf -- ...`) when available, or emit an actionable remediation command. +4. Add comprehensive unit tests in `tests/unit/test_cleanup_space.py` covering `PermissionError` handling on stale project artifacts without following symlinks or escaping the project root. + +SESSION_EXECUTION_AUTHORIZATION: Governed Subactor publication ticket PLF-20295 (GitHub issue semcod/fixos#132). + +## Acceptance criteria + +- [x] AC-01: When `shutil.rmtree` raises `PermissionError` in `_cleanup_projects_old` (or `_remove_selected`), clearly report permission-denied items and, when `sudo -n` is available and target paths pass strict `_project_artifact_allowed` validation, allow bounded non-interactive removal or emit an actionable remediation command. +- [x] AC-02: Unit tests in `tests/unit/test_cleanup_space.py` cover `PermissionError` handling on stale project artifacts without following symlinks or escaping the project root. + +## Tracking boundary + +This directory contains the minimal reviewed intent. Optional participant prose +and raw command logs are not required delivery output. diff --git a/project/ticket-108/intent.json b/project/ticket-108/intent.json new file mode 100644 index 0000000..84ef30a --- /dev/null +++ b/project/ticket-108/intent.json @@ -0,0 +1,81 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-108", + "summary": "Handle root-owned project artifacts in projects-old cleanup with bounded non-interactive sudo fallback", + "workstream": "application", + "classification": { + "kind": "FEATURE", + "priority": "P2", + "origin": "requested" + }, + "allowedPaths": [ + "project/ticket-108/**", + "TODO.md", + "project/TICKETS.md", + "fixos/cli/_cleanup_space.py", + "tests/unit/test_cleanup_space.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [ + "python" + ], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "acceptedBaseSha": "0ca688a1977bc9f8948b75404f308299f70b82b1", + "targetBranch": "main", + "outcome": "Handle root-owned project artifacts in projects-old cleanup with bounded non-interactive sudo fallback", + "nonGoals": [ + "Do not follow symlinks or escape project root", + "Do not remove non-project files" + ], + "complexity": "S", + "estimatedMinutes": 30, + "budgets": { + "maxImplementationFiles": 5, + "maxAffectedComponents": 2, + "maxPublicInterfaceChanges": 0, + "maxRuntimeDependencies": 0 + }, + "architecture": { + "status": "accepted", + "decision": "Validate root-owned project artifacts with _project_artifact_allowed and attempt bounded sudo -n removal or emit remediation command when PermissionError occurs in projects-old cleanup.", + "components": [ + { + "name": "cleanup-space", + "paths": [ + "fixos/cli/_cleanup_space.py" + ] + }, + { + "name": "tests", + "paths": [ + "tests/unit/test_cleanup_space.py" + ] + } + ], + "responsibilityChanges": false, + "interfaceChanges": [], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Revert ticket-108 commit via protected publication." + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-01", + "commands": [ + "pytest tests/unit/test_cleanup_space.py -q" + ], + "evidence": "project/ticket-108/README.md" + } + ] + } +} diff --git a/tests/unit/test_cleanup_space.py b/tests/unit/test_cleanup_space.py index efb9fea..24c442d 100644 --- a/tests/unit/test_cleanup_space.py +++ b/tests/unit/test_cleanup_space.py @@ -1,6 +1,9 @@ """Tests for fixos.cli._cleanup_space disk-reclaim handlers.""" +import os +import shutil import subprocess +import time from pathlib import Path import click @@ -308,8 +311,6 @@ def test_cli_rejects_space_flag_with_foreign_cleanup(monkeypatch): def _make_unused_project(base: Path, name: str, age_days: float) -> Path: - import os - import time proj = base / name (proj / ".git").mkdir(parents=True) @@ -337,8 +338,6 @@ def _make_unused_project(base: Path, name: str, age_days: float) -> Path: def test_projects_old_reclaims_only_unused_7d_safe_artifacts(monkeypatch, tmp_path): - import os - import time from fixos.diagnostics import venv_cleanup as vc @@ -376,3 +375,202 @@ def test_projects_old_reclaims_only_unused_7d_safe_artifacts(monkeypatch, tmp_pa assert (fresh_proj / ".venv").is_dir() assert (active_proj / ".venv").is_dir() + +def test_project_artifact_allowed_validation(tmp_path, monkeypatch): + proj = tmp_path / "myproj" + proj.mkdir() + (proj / ".git").mkdir() + (proj / "node_modules").mkdir() + (proj / "src").mkdir() + (proj / "file.txt").write_text("hello") + + # Valid artifact + assert space._project_artifact_allowed(proj / "node_modules", proj) is True + # Dict input + assert space._project_artifact_allowed({"path": proj / "node_modules", "project": proj}) is True + assert space._project_artifact_allowed({"path": proj / "node_modules"}) is True + + # Project root itself + assert space._project_artifact_allowed(proj, proj) is False + # Parent of project root + assert space._project_artifact_allowed(proj.parent, proj) is False + # Regular file + assert space._project_artifact_allowed(proj / "file.txt", proj) is False + # Forbidden .git directory + assert space._project_artifact_allowed(proj / ".git", proj) is False + # Non-existent directory + assert space._project_artifact_allowed(proj / "nonexistent", proj) is False + + # Direct symlink + sym = proj / "symlink_art" + sym.symlink_to(tmp_path) + assert space._project_artifact_allowed(sym, proj) is False + + # Symlink pointing inside project + sym_inside = proj / "symlink_inside" + sym_inside.symlink_to(proj / "node_modules") + assert space._project_artifact_allowed(sym_inside, proj) is False + + # Intermediate symlink directory + sym_parent = proj / "sym_parent" + real_target = tmp_path / "real_target" + real_target.mkdir() + (real_target / "nested_art").mkdir() + sym_parent.symlink_to(real_target) + assert space._project_artifact_allowed(sym_parent / "nested_art", proj) is False + + # Mount point checks + orig_is_mount = Path.is_mount + monkeypatch.setattr( + Path, "is_mount", lambda p: p == (proj / "node_modules") or orig_is_mount(p) + ) + assert space._project_artifact_allowed(proj / "node_modules", proj) is False + + # Child mount point inside artifact + child_dir = proj / "node_modules" / "submount" + child_dir.mkdir(parents=True) + monkeypatch.setattr( + Path, "is_mount", lambda p: p == child_dir or orig_is_mount(p) + ) + assert space._project_artifact_allowed(proj / "node_modules", proj) is False + + +def test_projects_old_permission_error_with_sudo_success(monkeypatch, tmp_path): + from fixos.diagnostics import venv_cleanup as vc + + monkeypatch.setattr(vc, "active_paths", lambda: (set(), [])) + monkeypatch.setenv("FIXOS_NATIVE", "off") + + stale_proj = _make_unused_project(tmp_path, "stale-app", age_days=10) + target_art = stale_proj / "node_modules" + + orig_rmtree = shutil.rmtree + + def mock_rmtree(path, *args, **kwargs): + if Path(path) == target_art: + raise PermissionError(f"[Errno 13] Permission denied: '{path}'") + return orig_rmtree(path, *args, **kwargs) + + calls = [] + + def runner(argv, timeout=60): + calls.append(list(argv)) + if argv[:4] == ["sudo", "-n", "rm", "-rf"]: + orig_rmtree(target_art, ignore_errors=True) + return _cp(stdout="", returncode=0) + return _cp(stdout="") + + monkeypatch.setattr(shutil, "rmtree", mock_rmtree) + monkeypatch.setattr(shutil, "which", lambda cmd: "/usr/bin/sudo" if cmd == "sudo" else None) + monkeypatch.setattr(space, "_run_cmd", runner) + + result = CliRunner().invoke( + cleanup_cmd.cleanup_services, + ["--projects-old", "--projects-path", str(tmp_path), "--yes"], + ) + assert result.exit_code == 0, result.output + assert any( + c[:4] == ["sudo", "-n", "rm", "-rf"] and str(target_art) in c + for c in calls + ) + assert "Usunięto przez sudo (projekty)" in result.output + assert not target_art.exists() + assert not (stale_proj / ".venv").exists() + + +def test_projects_old_permission_error_without_sudo_reports_and_remediates(monkeypatch, tmp_path): + from fixos.diagnostics import venv_cleanup as vc + + monkeypatch.setattr(vc, "active_paths", lambda: (set(), [])) + monkeypatch.setenv("FIXOS_NATIVE", "off") + + stale_proj = _make_unused_project(tmp_path, "stale-app", age_days=10) + target_art = stale_proj / "node_modules" + + orig_rmtree = shutil.rmtree + + def mock_rmtree(path, *args, **kwargs): + if Path(path) == target_art: + raise PermissionError(f"[Errno 13] Permission denied: '{path}'") + return orig_rmtree(path, *args, **kwargs) + + monkeypatch.setattr(shutil, "rmtree", mock_rmtree) + monkeypatch.setattr(shutil, "which", lambda cmd: None) + + result = CliRunner().invoke( + cleanup_cmd.cleanup_services, + ["--projects-old", "--projects-path", str(tmp_path), "--yes"], + ) + assert "Brak uprawnień (PermissionError)" in result.output + assert "node_modules" in result.output + assert "Polecenie naprawcze" in result.output + assert f"sudo rm -rf -- '{target_art}'" in result.output or f"sudo rm -rf -- {target_art}" in result.output + + +def test_projects_old_permission_error_sudo_fails_reports_and_remediates(monkeypatch, tmp_path): + from fixos.diagnostics import venv_cleanup as vc + + monkeypatch.setattr(vc, "active_paths", lambda: (set(), [])) + monkeypatch.setenv("FIXOS_NATIVE", "off") + + stale_proj = _make_unused_project(tmp_path, "stale-app", age_days=10) + target_art = stale_proj / "node_modules" + + orig_rmtree = shutil.rmtree + + def mock_rmtree(path, *args, **kwargs): + if Path(path) == target_art: + raise PermissionError(f"[Errno 13] Permission denied: '{path}'") + return orig_rmtree(path, *args, **kwargs) + + calls = [] + + def runner(argv, timeout=60): + calls.append(list(argv)) + if argv[:4] == ["sudo", "-n", "rm", "-rf"]: + return _cp(returncode=1, stderr="sudo: a password is required") + return _cp(stdout="") + + monkeypatch.setattr(shutil, "rmtree", mock_rmtree) + monkeypatch.setattr(shutil, "which", lambda cmd: "/usr/bin/sudo" if cmd == "sudo" else None) + monkeypatch.setattr(space, "_run_cmd", runner) + + result = CliRunner().invoke( + cleanup_cmd.cleanup_services, + ["--projects-old", "--projects-path", str(tmp_path), "--yes"], + ) + assert any( + c[:4] == ["sudo", "-n", "rm", "-rf"] and str(target_art) in c + for c in calls + ) + assert "Brak uprawnień (PermissionError)" in result.output + assert "Polecenie naprawcze" in result.output + assert f"sudo rm -rf -- {target_art}" in result.output or f"sudo rm -rf -- '{target_art}'" in result.output + + +def test_projects_old_permission_error_never_sudo_removes_unallowed_artifacts(monkeypatch, tmp_path): + proj = tmp_path / "demo_proj" + proj.mkdir() + sym_art = proj / "node_modules" + sym_art.symlink_to(tmp_path) + + calls = [] + monkeypatch.setattr(space, "_run_cmd", lambda argv, timeout=60: (calls.append(list(argv)), _cp())[1]) + + candidates = [ + { + "description": "demo/node_modules (node)", + "path": sym_art, + "project": proj, + "artifact_name": "node_modules", + "size_bytes": 1024, + } + ] + + monkeypatch.setattr(space, "_remove_path", lambda p: (False, "PERMISSION_DENIED:[Errno 13] Permission denied")) + + space._remove_selected(candidates, [0]) + + assert not any(c[:2] == ["sudo", "-n"] for c in calls) + +