Validate topology and destructive path boundaries (#7)

* Validate topology and destructive path boundaries

- Canonicalize sources/backups_dir and reject overlap in either direction
- Reject duplicate sources and destination basename collisions
- Reject non-normalized relative paths in nest_hardlink
- Give each backup repository a persistent identity, recorded in every
  snapshot's completion manifest
- Verify repo identity, directory type (no symlinks), containment, and
  device before any recursive snapshot/staging-dir deletion, refusing to
  cross mount points

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Address Copilot review: root-path check, symlink/TOCTOU-safe writes, legacy marker adoption

- _is_same_or_within: drop the redundant, root-mishandling startswith
  check; commonpath() alone is already component-aware
- _ensure_repo_id/_write_manifest: write via temp-file + atomic rename
  (O_NOFOLLOW, O_EXCL on the temp file) instead of open+write in place,
  so writes can never go through a pre-existing symlink and are never
  left half-written
- _verify_safe_to_delete: distinguish a legacy empty marker (pre-dates
  the manifest format, can only be a pre-migration artifact) from a
  non-empty-but-invalid one (corrupt or forged); adopt the former into
  the repository instead of refusing to delete it forever

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Maxim Shulga <maxim.shulga@alludo.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

IMPROVEMENTS.md

@@ -56,15 +56,15 @@ Acceptance criteria:
 
 ### Validate topology and destructive path boundaries
 
-- [ ] Canonicalize source and backup paths and reject every overlap.
-- [ ] Reject duplicate sources and destination basename collisions, or require
+- [x] Canonicalize source and backup paths and reject every overlap.
+- [x] Reject duplicate sources and destination basename collisions, or require
   explicit source labels.
-- [ ] Require normalized relative paths for nested operations.
-- [ ] Verify `commonpath` before every write, rename, or deletion.
-- [ ] Give each backup repository an identity recorded in its manifest.
-- [ ] Validate repository identity, snapshot type, marker, and device before
+- [x] Require normalized relative paths for nested operations.
+- [x] Verify `commonpath` before every write, rename, or deletion.
+- [x] Give each backup repository an identity recorded in its manifest.
+- [x] Validate repository identity, snapshot type, marker, and device before
   recursive deletion.
-- [ ] Refuse to cross mount points during deletion.
+- [x] Refuse to cross mount points during deletion.
 
 Acceptance criteria:
 

curateipsum/backup.py

@@ -1,12 +1,14 @@
 """
 Module with backup functions.
 """
+import json
 import logging
 import os
 import shutil
 import time
+import uuid
 from datetime import datetime, timedelta
-from typing import Dict, Optional, Iterable, Union
+from typing import Dict, List, Optional, Iterable, Union
 
 try:
     import fcntl
@@ -24,6 +26,7 @@ LOCK_FILE = ".backups_lock"
 DELTA_DIR = ".backup_delta"
 BACKUP_MARKER = ".backup_finished"
 STAGING_PREFIX = ".incomplete-"
+REPO_ID_FILE = ".curateipsum_repo_id"
 _lg = logging.getLogger(__name__)
 
 
@@ -33,10 +36,209 @@ class BackupFailedError(Exception):
     Any failure while building a snapshot (copy error, parse error,
     unsupported entry, nonzero subprocess exit) is fatal: the partially
     built snapshot is discarded and this is the only way that failure is
-    reported to callers.
+    reported to callers. Topology validation and destructive-path
+    boundary failures also raise this, since both mean the run cannot
+    proceed safely.
     """
 
 
+def _canonical(path: str) -> str:
+    """Absolute, symlink-resolved path used for boundary comparisons."""
+    return os.path.realpath(os.path.abspath(path))
+
+
+def _is_same_or_within(inner: str, outer: str) -> bool:
+    """
+    True if canonical path `inner` equals or is nested under `outer`.
+    commonpath() compares path components, not raw string prefixes, so
+    this is correct even when `outer` is the filesystem root - unlike a
+    startswith(outer + sep) check, which mishandles root.
+    """
+    return os.path.commonpath([inner, outer]) == outer
+
+
+def validate_topology(sources: List[str], backups_dir_abs: str) -> None:
+    """
+    Reject a source/backups_dir layout that could lead to a source being
+    backed up into itself, a backup silently overwriting another source,
+    or two sources colliding at the same destination path.
+
+    :raises BackupFailedError: if any boundary or collision check fails.
+    """
+    backups_canon = _canonical(backups_dir_abs)
+
+    seen_canon: Dict[str, str] = {}
+    seen_basename: Dict[str, str] = {}
+    for src in sources:
+        src_canon = _canonical(src)
+
+        if _is_same_or_within(src_canon, backups_canon):
+            raise BackupFailedError(
+                "Source %s is inside the backups directory %s"
+                % (src, backups_dir_abs)
+            )
+        if _is_same_or_within(backups_canon, src_canon):
+            raise BackupFailedError(
+                "Backups directory %s is inside source %s"
+                % (backups_dir_abs, src)
+            )
+
+        if src_canon in seen_canon:
+            raise BackupFailedError(
+                "Duplicate source: %s and %s both resolve to %s"
+                % (seen_canon[src_canon], src, src_canon)
+            )
+        seen_canon[src_canon] = src
+
+        basename = os.path.basename(src_canon)
+        if basename in seen_basename:
+            raise BackupFailedError(
+                "Sources %s and %s both back up to the same destination "
+                "name %r; use distinct directory names"
+                % (seen_basename[basename], src, basename)
+            )
+        seen_basename[basename] = src
+
+
+_O_NOFOLLOW = getattr(os, "O_NOFOLLOW", 0)
+
+
+def _read_small_file_no_follow(path: str, max_bytes: int = 4096
+                               ) -> Optional[str]:
+    """
+    Read a small file, refusing to follow a symlink at `path`. Returns
+    None if the path doesn't exist, isn't a regular file, or is a
+    symlink.
+    """
+    try:
+        fd = os.open(path, os.O_RDONLY | _O_NOFOLLOW)
+    except OSError:
+        return None
+    try:
+        return os.read(fd, max_bytes).decode()
+    finally:
+        os.close(fd)
+
+
+def _write_small_file_no_follow(path: str, content: str):
+    """
+    Atomically (create-or-replace) write a small file at `path`. Writes
+    to a sibling temp file and renames it into place: rename() replaces
+    whatever directory entry currently sits at `path` - including a
+    symlink itself, never the symlink's target - so this can never write
+    through a pre-existing symlink. It also means a concurrent reader
+    never observes a partially-written or stale-plus-new-content file.
+    """
+    tmp_path = "%s.tmp-%d" % (path, os.getpid())
+    fd = os.open(tmp_path,
+                os.O_CREAT | os.O_EXCL | os.O_WRONLY | _O_NOFOLLOW, 0o644)
+    try:
+        os.write(fd, content.encode())
+        os.fsync(fd)
+    finally:
+        os.close(fd)
+    os.replace(tmp_path, path)
+
+
+def _ensure_repo_id(backups_dir: str) -> str:
+    """
+    Return this backups directory's identity, generating and persisting
+    one on first use. Recorded in every snapshot's manifest so a deletion
+    can later verify a directory genuinely belongs to this repository
+    before it is recursively removed.
+    """
+    id_path = os.path.join(backups_dir, REPO_ID_FILE)
+    repo_id = _read_small_file_no_follow(id_path)
+    if repo_id:
+        return repo_id.strip()
+
+    repo_id = uuid.uuid4().hex
+    _write_small_file_no_follow(id_path, repo_id)
+    return repo_id
+
+
+def _write_manifest(marker_path: str, backups_dir: str, snapshot_name: str):
+    """Write and fsync the completion manifest for a finished snapshot."""
+    manifest = json.dumps({
+        "repo_id": _ensure_repo_id(backups_dir),
+        "snapshot": snapshot_name,
+    })
+    _write_small_file_no_follow(marker_path, manifest)
+
+
+# Sentinel for a marker that exists but predates the JSON manifest
+# format (an empty touch file, as set_backup_marker() used to write).
+# Distinguished from corrupt/forged content: an empty marker can only
+# be a pre-migration artifact, never a deliberately altered one.
+_LEGACY_MANIFEST = object()
+
+
+def _read_manifest(marker_path: str) -> Union[dict, object, None]:
+    """
+    Read a snapshot manifest. Returns _LEGACY_MANIFEST for a pre-migration
+    empty marker, or None if the marker is missing, unreadable, or holds
+    content that fails to parse as JSON (corrupt or forged).
+    """
+    try:
+        with open(marker_path, "r") as f:
+            content = f.read()
+    except OSError:
+        return None
+    if not content:
+        return _LEGACY_MANIFEST
+    try:
+        return json.loads(content)
+    except ValueError:
+        return None
+
+
+def _verify_safe_to_delete(entry: os.DirEntry, backups_dir_abs: str,
+                           repo_id: str) -> bool:
+    """
+    Verify a snapshot directory entry may be recursively deleted: it must
+    be a real, non-symlinked directory living directly inside
+    backups_dir_abs, on the same filesystem (never crossing a mount
+    point), and carrying a manifest whose repo_id matches this
+    repository's identity.
+    """
+    if entry.is_symlink() or not entry.is_dir(follow_symlinks=False):
+        _lg.error("Refusing to delete %s: not a real directory", entry.path)
+        return False
+
+    entry_canon = _canonical(entry.path)
+    if os.path.dirname(entry_canon) != backups_dir_abs:
+        _lg.error("Refusing to delete %s: escapes backups directory %s",
+                  entry.path, backups_dir_abs)
+        return False
+
+    try:
+        entry_dev = os.stat(entry.path).st_dev
+        backups_dev = os.stat(backups_dir_abs).st_dev
+    except OSError as err:
+        _lg.error("Refusing to delete %s: stat failed: %s", entry.path, err)
+        return False
+    if entry_dev != backups_dev:
+        _lg.error("Refusing to delete %s: crosses a mount point",
+                  entry.path)
+        return False
+
+    manifest = _read_manifest(_get_backup_marker(entry).path)
+    if manifest is _LEGACY_MANIFEST:
+        _lg.warning(
+            "%s has a pre-migration marker with no repository identity; "
+            "adopting it into repository %s", entry.path, repo_id
+        )
+        return True
+    if manifest is None or manifest.get("repo_id") != repo_id:
+        _lg.error(
+            "Refusing to delete %s: missing, corrupt, or mismatched "
+            "repository identity in manifest", entry.path
+        )
+        return False
+
+    return True
+
+
 def _get_backup_marker(
         backup_entry: Union[os.DirEntry, fs.PseudoDirEntry]
 ) -> fs.PseudoDirEntry:
@@ -203,7 +405,8 @@ def set_backup_marker(backup_entry: Union[os.DirEntry, fs.PseudoDirEntry]):
     """Create finished backup marker file in backup's directory."""
     backup_marker = _get_backup_marker(backup_entry)
     if not os.path.exists(backup_marker.path):
-        open(backup_marker.path, "a").close()
+        backups_dir = os.path.dirname(os.path.normpath(backup_entry.path))
+        _write_manifest(backup_marker.path, backups_dir, backup_entry.name)
 
 
 _O_DIRECTORY = getattr(os, "O_DIRECTORY", 0)
@@ -242,10 +445,34 @@ def cleanup_stale_staging_dirs(backups_dir: str):
     backup run. Must be called before a new backup starts so a stale
     staging directory can never be mistaken for a real snapshot.
     """
+    backups_dir_abs = _canonical(backups_dir)
+    try:
+        backups_dev = os.stat(backups_dir_abs).st_dev
+    except OSError as err:
+        _lg.error("Cannot stat backups directory %s: %s", backups_dir, err)
+        return
+
     with os.scandir(backups_dir) as it:
         entry: os.DirEntry
         for entry in it:
-            if entry.name.startswith(STAGING_PREFIX) and entry.is_dir():
+            if not entry.name.startswith(STAGING_PREFIX):
+                continue
+            if entry.is_symlink() or not entry.is_dir(follow_symlinks=False):
+                _lg.error("Refusing to remove %s: not a real directory",
+                          entry.path)
+                continue
+            if os.path.dirname(_canonical(entry.path)) != backups_dir_abs:
+                _lg.error("Refusing to remove %s: escapes backups directory",
+                          entry.path)
+                continue
+            try:
+                if os.stat(entry.path).st_dev != backups_dev:
+                    _lg.error("Refusing to remove %s: crosses a mount point",
+                              entry.path)
+                    continue
+            except OSError as err:
+                _lg.error("Cannot stat %s: %s", entry.path, err)
+                continue
             _lg.warning("Removing abandoned staging directory: %s",
                        entry.name)
             shutil.rmtree(entry.path, ignore_errors=True)
@@ -321,6 +548,9 @@ def cleanup_old_backups(backups_dir: str,
             .strftime(BACKUP_ENT_FMT)
         )
 
+    backups_dir_abs = _canonical(backups_dir)
+    repo_id = _ensure_repo_id(backups_dir)
+
     prev_backup = all_backups[0]
     to_remove = {b: False for b in all_backups}
 
@@ -365,10 +595,13 @@ def cleanup_old_backups(backups_dir: str,
         to_remove[backup] = True
 
     for backup, do_delete in to_remove.items():
-        if do_delete:
+        if not do_delete:
+            continue
         if dry_run:
             _lg.info("Would remove old backup %s", backup.name)
-            else:
+            continue
+        if not _verify_safe_to_delete(backup, backups_dir_abs, repo_id):
+            continue
         _lg.info("Removing old backup %s", backup.name)
         shutil.rmtree(backup.path)
 
@@ -510,15 +743,11 @@ def initiate_backup(sources,
         shutil.rmtree(staging.path, ignore_errors=True)
         return None
 
-    # write and fsync completion marker only after every source succeeded,
+    # write and fsync completion manifest only after every source succeeded,
     # then atomically rename the staging dir into its final snapshot name
     marker_name = "%s_%s" % (BACKUP_MARKER, snapshot_name)
-    marker_fd = os.open(os.path.join(staging.path, marker_name),
-                        os.O_CREAT | os.O_WRONLY, 0o644)
-    try:
-        os.fsync(marker_fd)
-    finally:
-        os.close(marker_fd)
+    _write_manifest(os.path.join(staging.path, marker_name),
+                    backups_dir, snapshot_name)
     _fsync_dir(staging.path)
 
     final_path = os.path.join(backups_dir, snapshot_name)

curateipsum/cli.py

@@ -92,6 +92,12 @@ def main():
             _lg.error("Source directory %s does not exist", src_dir)
             return 1
 
+    try:
+        backup.validate_topology(args.sources, backups_dir_abs)
+    except backup.BackupFailedError as err:
+        _lg.error("Unsafe source/backups layout: %s", err)
+        return 1
+
     start_time = time.time()
 
     if not backup.set_backups_lock(backups_dir_abs, args.force):

curateipsum/fs.py

@@ -561,6 +561,11 @@ def nest_hardlink(src_dir: str, src_relpath: str, dst_dir: str):
     """
     _lg.debug("Nested hardlinking: %s%s%s -> %s",
               src_dir, os.path.sep, src_relpath, dst_dir)
+    if os.path.isabs(src_relpath) or os.path.pardir in src_relpath.split(
+            os.path.sep):
+        raise RuntimeError(
+            "Refusing non-normalized relative path: %s" % src_relpath)
+
     src_dir_abs = os.path.abspath(src_dir)
     src_full_path = os.path.join(src_dir_abs, src_relpath)
     dst_dir_abs = os.path.abspath(dst_dir)

tests/conftest.py

@@ -106,7 +106,10 @@ def check_backup_not_empty():
 def check_backups(backup_dir, check_backup_not_empty):
     """Helper to verify expected backups exist."""
     def _check(expected_backups):
-        backups_list = os.listdir(str(backup_dir))
+        backups_list = [
+            name for name in os.listdir(str(backup_dir))
+            if name != bk.REPO_ID_FILE
+        ]
         assert sorted(b.name for b in expected_backups) == sorted(backups_list)
         for b in expected_backups:
             assert check_backup_not_empty(b)

tests/test_backups.py

@@ -1,4 +1,5 @@
 import fcntl
+import json
 import os
 import subprocess
 import sys
@@ -741,6 +742,20 @@ class TestBackupIteration:
         marker_path = backup_path / marker_name
         assert marker_path.exists()
 
+    def test_set_backup_marker_records_repo_id(self, backup_dir):
+        """Test set_backup_marker records the repository identity."""
+        backup_dir.mkdir()
+        backup_path = backup_dir / "20210101_120000"
+        backup_path.mkdir()
+
+        backup_entry = bk.fs.PseudoDirEntry(str(backup_path))
+        bk.set_backup_marker(backup_entry)
+
+        marker_path = backup_path / f"{bk.BACKUP_MARKER}_20210101_120000"
+        manifest = bk._read_manifest(str(marker_path))
+        assert manifest["repo_id"] == bk._ensure_repo_id(str(backup_dir))
+        assert manifest["snapshot"] == "20210101_120000"
+
     def test_set_backup_marker_idempotent(self, backup_dir):
         """Test set_backup_marker is idempotent"""
         backup_dir.mkdir()
@@ -755,3 +770,164 @@ class TestBackupIteration:
         marker_name = f"{bk.BACKUP_MARKER}_20210101_120000"
         marker_path = backup_path / marker_name
         assert marker_path.exists()
+
+
+class TestValidateTopology:
+    """Tests for validate_topology source/backups_dir boundary checks."""
+
+    def test_disjoint_sources_ok(self, tmp_path):
+        src1 = tmp_path / "src1"
+        src2 = tmp_path / "src2"
+        backups = tmp_path / "backups"
+        src1.mkdir()
+        src2.mkdir()
+        backups.mkdir()
+
+        bk.validate_topology([str(src1), str(src2)], str(backups))
+
+    def test_source_inside_backups_dir_rejected(self, tmp_path):
+        backups = tmp_path / "backups"
+        src = backups / "src"
+        backups.mkdir()
+        src.mkdir()
+
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology([str(src)], str(backups))
+
+    def test_backups_dir_inside_source_rejected(self, tmp_path):
+        src = tmp_path / "src"
+        backups = src / "backups"
+        src.mkdir()
+        backups.mkdir()
+
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology([str(src)], str(backups))
+
+    def test_source_equals_backups_dir_rejected(self, tmp_path):
+        shared = tmp_path / "shared"
+        shared.mkdir()
+
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology([str(shared)], str(shared))
+
+    def test_duplicate_source_via_symlink_rejected(self, tmp_path):
+        src = tmp_path / "src"
+        backups = tmp_path / "backups"
+        alias = tmp_path / "alias"
+        src.mkdir()
+        backups.mkdir()
+        alias.symlink_to(src)
+
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology([str(src), str(alias)], str(backups))
+
+    def test_duplicate_basename_rejected(self, tmp_path):
+        parent_a = tmp_path / "a"
+        parent_b = tmp_path / "b"
+        backups = tmp_path / "backups"
+        (parent_a / "data").mkdir(parents=True)
+        (parent_b / "data").mkdir(parents=True)
+        backups.mkdir()
+
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology(
+                [str(parent_a / "data"), str(parent_b / "data")],
+                str(backups),
+            )
+
+    def test_parent_traversal_rejected(self, tmp_path):
+        backups = tmp_path / "backups"
+        outside = tmp_path / "outside"
+        backups.mkdir()
+        outside.mkdir()
+        traversal_src = str(backups / ".." / "outside")
+
+        bk.validate_topology([traversal_src], str(backups))
+        # sanity: same directory reached directly is rejected as an alias
+        with pytest.raises(bk.BackupFailedError):
+            bk.validate_topology([traversal_src, str(outside)], str(backups))
+
+
+class TestDeletionBoundarySafety:
+    """Tests that recursive deletion refuses unsafe targets."""
+
+    def test_refuses_to_delete_symlinked_snapshot_dir(
+            self, backup_dir, add_backup, run_cleanup, tmp_path
+    ):
+        """A symlink standing in for a snapshot dir must never be
+        recursively deleted, since shutil.rmtree would follow it."""
+        real_target = tmp_path / "outside_target"
+        real_target.mkdir()
+        (real_target / "canary").write_text("do-not-delete")
+
+        add_backup("20220101_0000")  # kept, forces the old one to be
+        # evaluated for removal
+        old_name = "20200101_0000"
+        old_marker = f"{bk.BACKUP_MARKER}_{old_name}"
+        os.symlink(str(real_target), os.path.join(str(backup_dir), old_name))
+        # marker must exist for _is_backup(), even though this is a symlink
+        os.makedirs(
+            os.path.join(str(real_target)), exist_ok=True
+        )
+        (real_target / old_marker).write_text("")
+
+        run_cleanup(keep_all=0)
+
+        assert os.path.lexists(os.path.join(str(backup_dir), old_name))
+        assert (real_target / "canary").exists()
+
+    def test_refuses_to_delete_snapshot_with_wrong_repo_id(
+            self, backup_dir, add_backup, run_cleanup
+    ):
+        add_backup("20220101_0000")
+        old = add_backup("20200101_0000")
+
+        marker_path = bk._get_backup_marker(old).path
+        with open(marker_path, "w") as f:
+            f.write(json.dumps({"repo_id": "forged", "snapshot": old.name}))
+
+        run_cleanup(keep_all=0)
+
+        assert os.path.isdir(old.path)
+
+    def test_deletes_legitimate_old_backup(
+            self, backup_dir, add_backup, run_cleanup
+    ):
+        add_backup("20220101_0000")
+        old = add_backup("20200101_0000")
+
+        run_cleanup(keep_all=0)
+
+        assert not os.path.exists(old.path)
+
+    def test_cleanup_stale_staging_ignores_symlinked_staging_dir(
+            self, backup_dir, tmp_path
+    ):
+        backup_dir.mkdir()
+        real_target = tmp_path / "outside_target"
+        real_target.mkdir()
+        (real_target / "canary").write_text("do-not-delete")
+
+        staging_name = f"{bk.STAGING_PREFIX}20220101_0000"
+        os.symlink(str(real_target),
+                  os.path.join(str(backup_dir), staging_name))
+
+        bk.cleanup_stale_staging_dirs(str(backup_dir))
+
+        assert os.path.lexists(os.path.join(str(backup_dir), staging_name))
+        assert (real_target / "canary").exists()
+
+    def test_deletes_legacy_backup_with_empty_marker(
+            self, backup_dir, add_backup, run_cleanup
+    ):
+        """A pre-migration empty marker (no manifest) is adopted, not
+        permanently protected, since it can't be a forged one."""
+        add_backup("20220101_0000")
+        old = add_backup("20200101_0000")
+
+        marker_path = bk._get_backup_marker(old).path
+        open(marker_path, "w").close()  # simulate pre-migration marker
+
+        run_cleanup(keep_all=0)
+
+        assert not os.path.exists(old.path)

tests/test_cli.py

@@ -384,10 +384,9 @@ class TestDirectoryValidation:
         source = tmp_path / "src"
         source.mkdir()
 
+        with mock.patch('os.getcwd', return_value=str(tmp_path)):
             with mock.patch('sys.argv', ['cura-te-ipsum', '-b', 'backups',
                                           str(source)]):
-            with mock.patch('os.path.abspath',
-                            return_value=str(backups_dir)) as m_abspath:
                 with mock.patch('os.path.isdir', return_value=True):
                     with mock.patch('curateipsum.backup.set_backups_lock',
                                     return_value=True) as m_lock:
@@ -399,7 +398,6 @@ class TestDirectoryValidation:
                                     'curateipsum.backup.release_backups_lock'):
                                     cli.main()
 
-        m_abspath.assert_called_with('backups')
         m_lock.assert_called_once_with(str(backups_dir), False)