From 12a51c4d23fd9b7e331a90ccc6be563545a279dd Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:07:26 -0500 Subject: [PATCH 1/5] feat: add scoped FileHelper foundation Add filesystem-owning primitives with explicit symlink policy, scoped containment, atomic target updates, recursive deletion preflight, and Windows path/reparse rejection. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e --- design/file-helper.md | 120 ++++++ src/specify_cli/file_helper.py | 232 +++++++++++ tests/test_file_helper.py | 702 +++++++++++++++++++++++++++++++++ 3 files changed, 1054 insertions(+) create mode 100644 design/file-helper.md create mode 100644 src/specify_cli/file_helper.py create mode 100644 tests/test_file_helper.py diff --git a/design/file-helper.md b/design/file-helper.md new file mode 100644 index 0000000000..b7aae100d1 --- /dev/null +++ b/design/file-helper.md @@ -0,0 +1,120 @@ +# Scoped FileHelper foundation + +`specify_cli.file_helper.FileHelper` owns filesystem operations under one +explicitly established root. This is the first migration stage: existing CLI +commands retain their current behavior and do not yet use this helper. There is +no global CLI flag or process-global policy. + +The user-directed persistence requirement belongs to the dependent +project-policy wiring stage: save the setting in `.specify/init-options.json`, +then load and validate it for future invocations and pass the resulting policy +explicitly to helper instances. Only `specify init` exposes and persists the +setting. Every other CLI command reads the current project configuration on +each invocation, so manual edits take effect without a cached policy. There +is no root/global flag and no per-command override flag. The low-level helper +does not read configuration. Strict persisted-policy loading and configuration- +read bootstrap semantics must be coordinated before migration. + +```python +from specify_cli.file_helper import FileHelper + +files = FileHelper(project_root) # deny symlinks by default +files.mkdir("generated", exist_ok=True) +files.create_text("generated/config.txt", "initial\n") +files.write_text("generated/config.txt", "updated\n") +content = files.read_text("generated/config.txt") +files.delete("generated", recursive=True) +``` + +## Boundary and policy + +Each helper has a frozen root and `allow_symlinks` setting. Relative operation +paths are root-relative. Absolute paths must be beneath the supplied root or +its canonical equivalent. User operation paths containing `..` are rejected +rather than normalized before inspection. Link targets may contain `..`, but +are walked component by component and may not leave the root, even temporarily. +Windows rooted-but-not-absolute paths (`/outside/file`, `\outside\file`) and +drive-relative paths (`C:outside/file`, `C:`) are rejected for both operation +paths and link targets. They are not root-relative: joining them can reset the +root or drive. Fully absolute paths still require scoped root matching. + +The caller must establish a trusted, existing directory root. Its alias is +resolved once, including OS and worktree aliases; the root itself and OS +ancestors above it are not subjected to the traversal policy. Thus explicitly +establishing a linked directory as a root trusts that alias even in deny mode. +Links *below* that boundary are checked in both the original accessed path and +any followed target hierarchy. An unrelated project symlink does not prevent +an operation. A helper cannot cross to another root; callers establish separate +helpers for independent project/source/managed boundaries. + +| Operation | Default deny | Explicit allow | +|---|---|---| +| Read file | Reject accessed parent or leaf links | Follow only existing contained regular-file targets | +| Create directory | Reject accessed links before creating parents | Follow contained directory links; reject dangling targets | +| Exclusively create file | Reject accessed links | Follow contained parents; an existing leaf link is never overwritten | +| Atomic write/upsert | Reject accessed links | Replace the resolved regular-file target and preserve every link | +| Delete leaf link | Reject without mutation | Unlink the link, including dangling, cyclic, or external-target links | +| Delete through linked parent | Reject without mutation | Delete the contained target entry, leaving the parent link intact | +| Recursive delete | Preflight all descendants and reject any link before mutation | Preflight all descendants; unlink links without traversing their targets | +| Create symlink | Reject | Require an existing contained file/directory target | + +## API and failures + +- `mkdir(path, parents=False, exist_ok=False)` creates directories. +- `read_bytes(path)` / `read_text(path, encoding="utf-8")` read regular files. +- `create_bytes(path, content, mode=0o644)` / `create_text(...)` exclusively + create files with existing parents. +- `write_bytes(path, content, mode=0o644)` / `write_text(...)` atomically + create or update files with existing parents. Text variants also accept + `encoding="utf-8"`. Replacement files use the supplied mode; metadata and + hard-link identity are not preserved. Atomic means same-directory + `os.replace`, not crash durability. +- `delete(path, recursive=False)` unlinks files/links or removes directories. + Nonrecursive directory deletion requires an empty directory. The established + root cannot be deleted. +- `symlink(target, path)` creates a link exclusively. Relative targets are + interpreted relative to the link parent, not the helper root. + +`SymlinkDeniedError`, `PathEscapeError`, and `UnsupportedPathError` derive from +`FileHelperError` and describe policy failures. Missing paths (including +dangling read/update targets), permissions, existing exclusive-create +destinations, non-directory parents, and OS I/O failures retain native Python +exceptions. Nothing prints, warns-and-skips, or exits the CLI. Callers own +translation into their normal command error envelopes. + +Only regular files, directories, and explicitly allowed symlinks are supported. +Below the established root, Windows directory junctions and other non-symlink +reparse entries are unsupported and rejected with `UnsupportedPathError` in +**both** policy modes. This includes contained junctions, leaf deletion, and +recursive deletion: preflight rejects them without traversing or removing +them. `allow_symlinks=True` does not authorize junction traversal or deletion. +The trusted root alias boundary may itself resolve through a junction just as +it may through a symlink; this does not authorize junctions below that root. +Read/update cycles and excessive link chains fail explicitly. A recursive +delete includes the entire selected tree; there is no exclusion filter in this +foundation. Future operations with exclusions must inspect only included +entries, not reject a whole project for excluded/unrelated links. + +## Safety limits and migration + +Preflight detects static policy failures before recursive deletion or parent +creation. Atomic writes recheck the original path before replacement, and +deletion rechecks each accessed entry. Portable path checks are **not** a +race-free sandbox: another process can exchange parents or entries between +checks and syscalls, and an I/O failure during mutation can leave partial +results. Protect against concurrent untrusted mutation separately; descriptor- +relative traversal or platform-specific mechanisms are future work, not a +claim of this API. The helper does not constrain subprocesses or third-party +code. Exclusive creation uses `O_EXCL` and `O_NOFOLLOW` where available. + +Existing safe-write mechanisms informed this helper, but existing shared +infrastructure warning/skip behavior and development-mode link creation are +unchanged. Migrate one owned operation boundary at a time, carrying original +paths and an explicit root/policy into the helper before any early resolution. +CLI option wiring, linked command directories, and development-mode +registration remain dependent stages. + +Windows path classification and directory-mode reparse rejection have portable +regression tests. Native Windows path and junction tests also exist and are +skipped on other hosts; running the portable cases on macOS does not validate +Windows filesystem syscalls or junction behavior. diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py new file mode 100644 index 0000000000..29837edf21 --- /dev/null +++ b/src/specify_cli/file_helper.py @@ -0,0 +1,232 @@ +"""Scoped filesystem operations; see design/file-helper.md for the contract.""" + +from dataclasses import dataclass, field +import os +from pathlib import Path, PurePath +import stat +import tempfile + + +class FileHelperError(ValueError): + """An operation violates the scoped filesystem contract.""" + + +class SymlinkDeniedError(FileHelperError): + """An accessed symlink requires explicit traversal permission.""" + + +class PathEscapeError(FileHelperError): + """An accessed path is outside the established root.""" + + +class UnsupportedPathError(FileHelperError): + """An accessed object cannot be used by the requested operation.""" + + +@dataclass(frozen=True) +class FileHelper: + """Own operations beneath one trusted root with an immutable link policy. + + Relative operation paths are relative to ``root``, not the process cwd. + The root itself is a trusted alias boundary, resolved once on construction. + """ + + root: Path + allow_symlinks: bool = False + _canonical_root: Path = field(init=False, repr=False) + + def __post_init__(self) -> None: + root = Path(self.root).absolute() + canonical = root.resolve(strict=True) + if not canonical.is_dir(): + raise NotADirectoryError(root) + object.__setattr__(self, "root", root) + object.__setattr__(self, "_canonical_root", canonical) + + def _parts(self, path: PurePath) -> tuple[str, ...]: + if not path.is_absolute(): + if path.anchor: + raise PathEscapeError(f"Rooted-relative or drive-relative path is not permitted: {path}") + return path.parts + for root in (self.root, self._canonical_root): + try: + return path.relative_to(root).parts + except ValueError: + continue + raise PathEscapeError(f"Path is outside root {self.root}: {path}") + + @staticmethod + def _entry_mode(path: Path) -> int: + entry = path.lstat() + if ( + getattr(entry, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT + and not stat.S_ISLNK(entry.st_mode) + ): + raise UnsupportedPathError( + f"Unsupported Windows reparse point (including directory junctions): {path}" + ) + return entry.st_mode + + def _walk( + self, + parts: tuple[str, ...], + *, + allow_missing: bool = False, + follow_leaf: bool = True, + links: tuple[Path, ...] = (), + ) -> Path: + current = self._canonical_root + for index, part in enumerate(parts): + if not current.is_dir(): + if not (allow_missing and not current.exists()): + raise NotADirectoryError(current) + if part == "..": + if current == self._canonical_root: + raise PathEscapeError(f"Symlink target escapes root {self.root}") + current = current.parent + continue + candidate = current / part + try: + mode = self._entry_mode(candidate) + except FileNotFoundError: + if not allow_missing: + raise + current = candidate + continue + if stat.S_ISLNK(mode): + if not self.allow_symlinks: + raise SymlinkDeniedError( + f"Refusing symlink {candidate}; explicitly allow symlinks " + "on this FileHelper to use supported contained targets" + ) + if not follow_leaf and index == len(parts) - 1: + return candidate + if candidate in links or len(links) >= 40: + raise UnsupportedPathError(f"Symlink cycle or excessive chain: {candidate}") + target = Path(os.readlink(candidate)) + target_parts = self._parts(target) + if not target.is_absolute(): + target_parts = current.relative_to(self._canonical_root).parts + target_parts + # Resolve the link target strictly, even when creating a new child. + current = self._walk(target_parts, links=links + (candidate,)) + elif stat.S_ISDIR(mode) or stat.S_ISREG(mode): + current = candidate + else: + raise UnsupportedPathError(f"Unsupported filesystem object: {candidate}") + return current + + def _path( + self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True + ) -> Path: + original = Path(path) + if ".." in original.parts: + raise PathEscapeError(f"Parent traversal is not an operation path: {original}") + return self._walk( + self._parts(original), allow_missing=allow_missing, follow_leaf=follow_leaf + ) + + @staticmethod + def _require_file(path: Path) -> None: + if not stat.S_ISREG(path.stat().st_mode): + raise UnsupportedPathError(f"Expected a regular file: {path}") + + def mkdir( + self, path: Path | str, *, parents: bool = False, exist_ok: bool = False + ) -> None: + """Create a directory, preflighting the entire requested hierarchy.""" + destination = self._path(path, allow_missing=True) + destination.mkdir(parents=parents, exist_ok=exist_ok) + + def read_bytes(self, path: Path | str) -> bytes: + destination = self._path(path) + self._require_file(destination) + return destination.read_bytes() + + def read_text(self, path: Path | str, *, encoding: str = "utf-8") -> str: + return self.read_bytes(path).decode(encoding) + + def create_bytes(self, path: Path | str, content: bytes, *, mode: int = 0o644) -> None: + """Exclusively create a file; never overwrite an existing entry.""" + destination = self._path(path, allow_missing=True, follow_leaf=False) + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL + flags |= getattr(os, "O_NOFOLLOW", 0) + fd = os.open(destination, flags, mode) + with os.fdopen(fd, "wb") as stream: + stream.write(content) + + def create_text( + self, path: Path | str, content: str, *, encoding: str = "utf-8", mode: int = 0o644 + ) -> None: + self.create_bytes(path, content.encode(encoding), mode=mode) + + def write_bytes(self, path: Path | str, content: bytes, *, mode: int = 0o644) -> None: + """Atomically create/replace a file, preserving any allowed leaf link.""" + destination = self._path(path, allow_missing=True) + if destination.exists(): + self._require_file(destination) + fd, temporary = tempfile.mkstemp(prefix=f".{destination.name}.", dir=destination.parent) + temporary_path = Path(temporary) + try: + with os.fdopen(fd, "wb") as stream: + stream.write(content) + temporary_path.chmod(mode) + if self._path(path, allow_missing=True) != destination: + raise FileHelperError(f"Destination changed during write: {path}") + os.replace(temporary_path, destination) + finally: + temporary_path.unlink(missing_ok=True) + + def write_text( + self, path: Path | str, content: str, *, encoding: str = "utf-8", mode: int = 0o644 + ) -> None: + self.write_bytes(path, content.encode(encoding), mode=mode) + + def symlink(self, target: Path | str, path: Path | str) -> None: + """Create a link to an existing contained regular file or directory. + + Relative targets use the link's parent, matching ``Path.symlink_to``. + """ + if not self.allow_symlinks: + raise SymlinkDeniedError("Symlink creation requires explicit allow_symlinks=True") + destination = self._path(path, allow_missing=True, follow_leaf=False) + target = Path(target) + target_parts = self._parts(target) + if not target.is_absolute(): + target_parts = destination.parent.relative_to(self._canonical_root).parts + target_parts + resolved = self._walk(target_parts) + destination.symlink_to(target, target_is_directory=resolved.is_dir()) + + def _deletion_plan(self, path: Path, *, recursive: bool) -> list[tuple[Path, bool]]: + mode = self._entry_mode(path) + if stat.S_ISLNK(mode): + if not self.allow_symlinks: + raise SymlinkDeniedError(f"Refusing to delete symlink: {path}") + return [(path, False)] + if stat.S_ISREG(mode): + return [(path, False)] + if not stat.S_ISDIR(mode): + raise UnsupportedPathError(f"Unsupported filesystem object: {path}") + plan: list[tuple[Path, bool]] = [] + if recursive: + with os.scandir(path) as entries: + children = sorted((Path(entry.path) for entry in entries)) + for child in children: + plan.extend(self._deletion_plan(child, recursive=True)) + plan.append((path, True)) + return plan + + def delete(self, path: Path | str, *, recursive: bool = False) -> None: + """Delete an entry; recursive deletion never follows descendant links.""" + destination = self._path(path, follow_leaf=False) + if destination == self._canonical_root: + raise FileHelperError(f"Refusing to delete the established root: {self.root}") + plan = self._deletion_plan(destination, recursive=recursive) + for entry, directory in plan: + # Recheck accessed parents/leaf before each mutation, not just preflight. + checked = self._path(entry, follow_leaf=False) + if checked != entry: + raise FileHelperError(f"Destination changed during deletion: {entry}") + if directory: + entry.rmdir() + else: + entry.unlink() diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py new file mode 100644 index 0000000000..1163058fe0 --- /dev/null +++ b/tests/test_file_helper.py @@ -0,0 +1,702 @@ +"""Operation-level evidence for the scoped FileHelper foundation.""" + +from dataclasses import FrozenInstanceError +import os +from pathlib import Path, PureWindowsPath +import subprocess +from types import SimpleNamespace + +import pytest + +from specify_cli.file_helper import ( + FileHelper, + FileHelperError, + PathEscapeError, + SymlinkDeniedError, + UnsupportedPathError, +) + + +@pytest.fixture +def root(tmp_path): + path = tmp_path / "project" + path.mkdir() + return path + + +@pytest.fixture +def link(root): + """Skip link-specific tests only on hosts unable to create symlinks.""" + probe = root / "probe" + try: + probe.symlink_to(root, target_is_directory=True) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"Symlink creation unavailable: {exc}") + probe.unlink() + + def create(path, target): + path.symlink_to(target, target_is_directory=Path(target).is_dir()) + return path + + return create + + +@pytest.mark.parametrize("allow", [False, True]) +def test_plain_operations(root, allow): + files = FileHelper(root, allow_symlinks=allow) + files.mkdir("nested/child", parents=True) + files.mkdir("nested/child", exist_ok=True) + files.create_text("nested/child/text", "caf\u00e9\n") + assert files.read_text("nested/child/text") == "caf\u00e9\n" + files.write_text("nested/child/text", "updated") + assert files.read_bytes(root / "nested/child/text") == b"updated" + files.write_bytes("nested/child/new", b"\x00\xff") + assert files.read_bytes("nested/child/new") == b"\x00\xff" + files.create_bytes("binary", b"\xff", mode=0o600) + files.delete("binary") + assert not (root / "binary").exists() + files.delete("nested", recursive=True) + assert list(root.iterdir()) == [] + + +@pytest.mark.parametrize("operation", ["read_bytes", "write_bytes", "create_bytes", "mkdir", "delete"]) +@pytest.mark.parametrize("kind", ["leaf", "parent", "dangling"]) +def test_deny_rejects_accessed_links_without_mutation(root, link, operation, kind): + target = root / "target" + target.mkdir() + (target / "file").write_bytes(b"old") + if kind == "parent": + path = link(root / "alias", target) / "file" + else: + path = link(root / "alias", target / ("missing" if kind == "dangling" else "file")) + files = FileHelper(root) + args = (path, b"new") if operation in ("write_bytes", "create_bytes") else (path,) + with pytest.raises(SymlinkDeniedError, match="symlink"): + getattr(files, operation)(*args) + assert (root / "alias").is_symlink() + assert (target / "file").read_bytes() == b"old" + assert not (target / "missing").exists() + + +def test_allow_read_and_atomic_update_preserve_chain_and_target(root, link, monkeypatch): + target = root / "target" + target.write_bytes(b"old") + first = link(root / "first", "target") + second = link(root / "second", "first") + files = FileHelper(root, allow_symlinks=True) + assert files.read_text(second) == "old" + replacements = [] + replace = os.replace + + def observe(source, destination): + replacements.append((Path(source), destination)) + assert first.is_symlink() and second.is_symlink() + assert target.read_bytes() == b"old" + replace(source, destination) + + monkeypatch.setattr("specify_cli.file_helper.os.replace", observe) + files.write_text(second, "new") + assert len(replacements) == 1 + assert replacements[0][0].parent == target.parent + assert replacements[0][1] == target + assert os.readlink(first) == "target" + assert os.readlink(second) == "first" + assert target.read_bytes() == b"new" + assert sorted(p.name for p in root.iterdir()) == ["first", "second", "target"] + + +def test_allow_operations_through_linked_parent(root, link): + target = root / "target" + target.mkdir() + alias = link(root / "alias", target) + files = FileHelper(root, allow_symlinks=True) + files.mkdir(alias / "created/child", parents=True) + files.create_text(alias / "created/file", "old") + assert files.read_text(alias / "created/file") == "old" + files.write_text(alias / "created/file", "new") + assert (target / "created/file").read_text() == "new" + files.delete(alias / "created", recursive=True) + assert alias.is_symlink() and target.is_dir() + assert list(target.iterdir()) == [] + + +def test_delete_through_parent_link_removes_real_file_not_parent_link(root, link): + target = root / "target" + target.mkdir() + (target / "file").write_text("delete") + alias = link(root / "alias", target) + FileHelper(root, allow_symlinks=True).delete(alias / "file") + assert alias.is_symlink() + assert target.is_dir() + assert not (target / "file").exists() + + +@pytest.mark.parametrize("allow", [False, True]) +def test_empty_directory_nonrecursive_delete(root, allow): + files = FileHelper(root, allow_symlinks=allow) + files.mkdir("empty") + files.delete("empty") + assert not (root / "empty").exists() + + +@pytest.mark.parametrize("operation", ["mkdir", "create_bytes", "write_bytes", "delete"]) +def test_dangling_parent_cannot_be_used_for_mutation(root, link, operation): + alias = link(root / "alias", root / "missing") + files = FileHelper(root, allow_symlinks=True) + path = alias / "child" + args = (path, b"bad") if operation in ("create_bytes", "write_bytes") else (path,) + with pytest.raises(FileNotFoundError): + getattr(files, operation)(*args) + assert alias.is_symlink() + assert not (root / "missing").exists() + + +def test_multi_link_cycle_and_excessive_chain_fail_explicitly(root, link): + link(root / "first", "second") + link(root / "second", "first") + files = FileHelper(root, allow_symlinks=True) + with pytest.raises(UnsupportedPathError, match="cycle"): + files.read_bytes("first") + (root / "target").write_text("ok") + for index in reversed(range(41)): + link(root / f"chain-{index}", "target" if index == 40 else f"chain-{index + 1}") + with pytest.raises(UnsupportedPathError, match="excessive chain"): + files.read_bytes("chain-0") + + +@pytest.mark.parametrize("operation", ["read_bytes", "write_bytes", "mkdir", "create_bytes", "delete"]) +def test_external_parent_is_never_traversed(root, link, operation): + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_bytes(b"old") + alias = link(root / "alias", outside) + files = FileHelper(root, allow_symlinks=True) + path = alias / "file" + args = (path, b"new") if operation in ("write_bytes", "create_bytes") else (path,) + with pytest.raises(PathEscapeError): + getattr(files, operation)(*args) + assert (outside / "file").read_bytes() == b"old" + assert alias.is_symlink() + + +@pytest.mark.parametrize("kind", ["external", "dangling", "cycle"]) +@pytest.mark.parametrize("operation", ["read_bytes", "write_bytes"]) +def test_allow_reads_and_updates_reject_invalid_targets(root, link, kind, operation): + if kind == "external": + target = root.parent / "outside" + target.write_bytes(b"old") + error = PathEscapeError + elif kind == "dangling": + target = root / "missing" + error = FileNotFoundError + else: + target = root / "alias" + error = UnsupportedPathError + alias = link(root / "alias", target) + args = (alias, b"new") if operation == "write_bytes" else (alias,) + with pytest.raises(error): + getattr(FileHelper(root, allow_symlinks=True), operation)(*args) + assert alias.is_symlink() + if kind == "external": + assert target.read_bytes() == b"old" + if kind == "dangling": + assert not target.exists() + + +@pytest.mark.parametrize("kind", ["file", "directory", "external", "dangling", "cycle"]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_allow_leaf_delete_unlinks_only_link(root, link, kind, recursive): + target = root / "target" + if kind == "file": + target.write_bytes(b"keep") + elif kind == "directory": + target.mkdir() + (target / "keep").write_bytes(b"keep") + elif kind == "external": + target = root.parent / "outside" + target.write_bytes(b"keep") + elif kind == "cycle": + target = root / "alias" + alias = link(root / "alias", target) + FileHelper(root, allow_symlinks=True).delete(alias, recursive=recursive) + assert not os.path.lexists(alias) + if kind in ("file", "external"): + assert target.read_bytes() == b"keep" + elif kind == "directory": + assert (target / "keep").read_bytes() == b"keep" + + +def test_recursive_deny_preflight_leaves_entire_tree_untouched(root, link): + tree = root / "tree" + tree.mkdir() + (tree / "a-file").write_bytes(b"keep") + (tree / "b-dir").mkdir() + (tree / "b-dir/keep").write_bytes(b"keep") + alias = link(tree / "z-link", root / "missing") + with pytest.raises(SymlinkDeniedError): + FileHelper(root).delete(tree, recursive=True) + assert (tree / "a-file").read_bytes() == b"keep" + assert (tree / "b-dir/keep").read_bytes() == b"keep" + assert alias.is_symlink() + + +def test_recursive_allow_does_not_descend_through_links(root, link): + tree = root / "tree" + tree.mkdir() + target = root / "retained" + target.mkdir() + (target / "keep").write_bytes(b"keep") + outside = root.parent / "outside" + outside.mkdir() + (outside / "keep").write_bytes(b"keep") + link(tree / "internal", target) + link(tree / "external", outside) + link(tree / "dangling", root / "missing") + link(tree / "cycle", tree) + (tree / "ordinary").write_bytes(b"delete") + FileHelper(root, allow_symlinks=True).delete(tree, recursive=True) + assert not tree.exists() + assert (target / "keep").read_bytes() == b"keep" + assert (outside / "keep").read_bytes() == b"keep" + + +def test_deletion_rechecks_parents_after_preflight(root, link, monkeypatch): + tree = root / "tree" + (tree / "nested").mkdir(parents=True) + (tree / "nested/file").write_text("keep") + plan = FileHelper._deletion_plan + + def exchange(self, path, *, recursive): + result = plan(self, path, recursive=recursive) + if path == tree: + (tree / "nested").rename(root / "retained") + link(tree / "nested", root / "retained") + return result + + monkeypatch.setattr(FileHelper, "_deletion_plan", exchange) + with pytest.raises(FileHelperError, match="changed during deletion"): + FileHelper(root, allow_symlinks=True).delete(tree, recursive=True) + assert (root / "retained/file").read_text() == "keep" + assert (tree / "nested").is_symlink() + + +@pytest.mark.parametrize("allow", [False, True]) +def test_unrelated_symlink_is_outside_operation_scope(root, link, allow): + link(root / "unrelated", root.parent / "missing") + files = FileHelper(root, allow_symlinks=allow) + files.create_text("selected", "works") + assert files.read_text("selected") == "works" + files.delete("selected") + assert (root / "unrelated").is_symlink() + + +@pytest.mark.parametrize("allow", [False, True]) +def test_trusted_root_alias_and_os_ancestors_are_not_rejected(root, link, allow): + alias = link(root.parent / "root-alias", root) + ancestor = link(root.parent / "ancestor-alias", root.parent) + for supplied in (alias, ancestor / root.name): + files = FileHelper(supplied, allow_symlinks=allow) + files.write_text("file", "works") + assert files.read_text(supplied / "file") == "works" + assert files.read_text(root / "file") == "works" + + +@pytest.mark.parametrize("path", ["../escape", "inside/../escape"]) +def test_operation_parent_traversal_is_rejected(root, path): + with pytest.raises(PathEscapeError): + FileHelper(root).mkdir(path, parents=True) + assert list(root.iterdir()) == [] + + +def test_absolute_escape_and_root_deletion_rejected(root): + files = FileHelper(root) + with pytest.raises(PathEscapeError): + files.write_text(root.parent / "outside", "bad") + with pytest.raises(FileHelperError, match="established root"): + files.delete(".", recursive=True) + assert root.is_dir() + + +def test_relative_link_targets_are_walked_without_losing_evidence(root, link): + directory = root / "directory" + directory.mkdir() + (root / "file").write_text("ok") + link(directory / "valid", "../file") + files = FileHelper(root, allow_symlinks=True) + assert files.read_text("directory/valid") == "ok" + link(directory / "escape", "../../outside") + with pytest.raises(PathEscapeError): + files.read_text("directory/escape") + link(root / "inner", root.parent) + link(root / "indirect", "inner/project/file") + with pytest.raises(PathEscapeError): + files.read_text("indirect") + link(root / "invalid", "file/../file") + with pytest.raises(NotADirectoryError): + files.read_text("invalid") + + +def test_intermediate_target_link_is_checked(root, link): + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_text("bad") + link(root / "indirection", outside) + link(root / "alias", root / "indirection/file") + with pytest.raises(PathEscapeError): + FileHelper(root, allow_symlinks=True).read_text("alias") + + +@pytest.mark.parametrize("allow", [False, True]) +def test_exclusive_create_never_overwrites_file_or_link(root, link, allow): + (root / "file").write_bytes(b"keep") + alias = link(root / "alias", root / "file") + dangling = link(root / "dangling", root / "missing") + files = FileHelper(root, allow_symlinks=allow) + for path in (root / "file", alias, dangling): + error = SymlinkDeniedError if path.is_symlink() and not allow else FileExistsError + with pytest.raises(error): + files.create_bytes(path, b"bad") + assert (root / "file").read_bytes() == b"keep" + assert alias.is_symlink() and dangling.is_symlink() + assert not (root / "missing").exists() + + +def test_symlink_creation_requires_permission_and_contained_target(root, link): + (root / "file").write_text("ok") + with pytest.raises(SymlinkDeniedError): + FileHelper(root).symlink("file", "denied") + assert not os.path.lexists(root / "denied") + files = FileHelper(root, allow_symlinks=True) + files.symlink("file", "allowed") + assert os.readlink(root / "allowed") == "file" + assert files.read_text("allowed") == "ok" + files.mkdir("directory") + files.symlink("../file", "directory/relative") + assert files.read_text("directory/relative") == "ok" + files.symlink("directory", "directory-alias") + assert (root / "directory-alias").is_dir() + with pytest.raises(FileNotFoundError): + files.symlink("missing", "dangling") + with pytest.raises(PathEscapeError): + files.symlink(root.parent / "outside", "escape") + with pytest.raises(FileExistsError): + files.symlink("file", "allowed") + assert not os.path.lexists(root / "dangling") + assert not os.path.lexists(root / "escape") + + +def test_missing_wrong_type_and_nonempty_failures(root): + files = FileHelper(root) + with pytest.raises(FileNotFoundError): + files.read_bytes("missing") + with pytest.raises(FileNotFoundError): + files.delete("missing") + with pytest.raises(FileNotFoundError): + files.create_text("missing/file", "bad") + with pytest.raises(FileNotFoundError): + files.write_text("missing/file", "bad") + files.mkdir("directory") + files.create_text("directory/file", "keep") + for operation in (lambda: files.read_bytes("directory"), + lambda: files.write_text("directory", "bad")): + with pytest.raises(UnsupportedPathError): + operation() + with pytest.raises(NotADirectoryError): + files.mkdir("directory/file/child", parents=True) + with pytest.raises(OSError): + files.delete("directory") + assert files.read_text("directory/file") == "keep" + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="POSIX special files required") +@pytest.mark.parametrize("allow", [False, True]) +def test_special_objects_fail_before_read_update_or_recursive_mutation(root, allow): + files = FileHelper(root, allow_symlinks=allow) + files.mkdir("tree") + files.create_text("tree/a-file", "keep") + os.mkfifo(root / "tree/z-fifo") + for operation in (lambda: files.read_bytes("tree/z-fifo"), + lambda: files.write_bytes("tree/z-fifo", b"bad"), + lambda: files.delete("tree", recursive=True)): + with pytest.raises(UnsupportedPathError): + operation() + assert files.read_text("tree/a-file") == "keep" + assert (root / "tree/z-fifo").exists() + + +def test_atomic_failure_cleans_temp_and_preserves_target(root, monkeypatch): + files = FileHelper(root) + files.create_text("file", "old") + + def fail(*args): + raise PermissionError("replacement denied") + + monkeypatch.setattr("specify_cli.file_helper.os.replace", fail) + with pytest.raises(PermissionError, match="replacement denied"): + files.write_text("file", "new") + assert files.read_text("file") == "old" + assert list(root.iterdir()) == [root / "file"] + + +def test_native_permission_failure_is_not_suppressed(root, monkeypatch): + files = FileHelper(root) + + def fail(*args, **kwargs): + raise PermissionError("creation denied") + + monkeypatch.setattr("specify_cli.file_helper.os.open", fail) + with pytest.raises(PermissionError, match="creation denied"): + files.create_bytes("file", b"bad") + assert list(root.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits required") +def test_atomic_target_replacement_uses_requested_mode(root, link): + target = root / "target" + target.write_text("old") + alias = link(root / "alias", target) + FileHelper(root, allow_symlinks=True).write_bytes(alias, b"new", mode=0o600) + assert target.stat().st_mode & 0o777 == 0o600 + assert alias.is_symlink() + + +def test_atomic_write_rechecks_original_link(root, link, monkeypatch): + first = root / "first" + first.write_text("first") + second = root / "second" + second.write_text("second") + alias = link(root / "alias", first) + chmod = Path.chmod + + def exchange(path, mode, **kwargs): + chmod(path, mode, **kwargs) + alias.unlink() + alias.symlink_to(second) + + monkeypatch.setattr(Path, "chmod", exchange) + with pytest.raises(FileHelperError, match="changed during write"): + FileHelper(root, allow_symlinks=True).write_text(alias, "bad") + assert first.read_text() == "first" + assert second.read_text() == "second" + assert sorted(p.name for p in root.iterdir()) == ["alias", "first", "second"] + + +def test_policy_is_immutable_and_helpers_are_independent(root, link): + (root / "file").write_text("ok") + link(root / "alias", "file") + deny = FileHelper(root) + allow = FileHelper(root, allow_symlinks=True) + with pytest.raises(FrozenInstanceError): + deny.allow_symlinks = True + assert allow.read_text("alias") == "ok" + with pytest.raises(SymlinkDeniedError): + deny.read_text("alias") + + +def test_root_must_be_existing_directory(root): + with pytest.raises(FileNotFoundError): + FileHelper(root / "missing") + (root / "file").write_text("file") + with pytest.raises(NotADirectoryError): + FileHelper(root / "file") + + +@pytest.mark.parametrize("spelling", ["/outside/file", r"\outside\file", "C:outside/file", "C:"]) +def test_windows_anchored_relative_forms_rejected_portably(root, spelling): + path = PureWindowsPath(spelling) + assert path.anchor and not path.is_absolute() + with pytest.raises(PathEscapeError): + FileHelper(root)._parts(path) + + +@pytest.mark.parametrize("spelling", ["inside/file", r"inside\file", "."]) +def test_windows_unanchored_relative_forms_are_root_relative(root, spelling): + path = PureWindowsPath(spelling) + assert FileHelper(root)._parts(path) == path.parts + + +@pytest.mark.parametrize("spelling", ["C:/outside/file", r"\\server\share\outside"]) +def test_windows_true_absolute_forms_do_not_bypass_scoped_matching(root, spelling): + with pytest.raises(PathEscapeError): + FileHelper(root)._parts(PureWindowsPath(spelling)) + + +@pytest.mark.parametrize( + "spelling", + ["D:/project/inside/file", "d:/PROJECT/inside/file", r"\\server\share\project\inside\file"], +) +def test_windows_contained_absolute_forms_match_roots_portably(spelling): + boundary = SimpleNamespace( + root=PureWindowsPath("D:/project"), + _canonical_root=PureWindowsPath(r"\\server\share\project"), + ) + assert FileHelper._parts(boundary, PureWindowsPath(spelling)) == ("inside", "file") + + +@pytest.mark.parametrize( + "spelling", + ["D:/project2/file", "D:/outside/file", "C:/project/file", r"\\server\share\project2\file"], +) +def test_windows_absolute_forms_outside_declared_roots_rejected_portably(spelling): + boundary = SimpleNamespace( + root=PureWindowsPath("D:/project"), + _canonical_root=PureWindowsPath(r"\\server\share\project"), + ) + with pytest.raises(PathEscapeError): + FileHelper._parts(boundary, PureWindowsPath(spelling)) + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows path semantics required") +@pytest.mark.parametrize("spelling", ["/outside/file", r"\outside\file", "C:outside/file", "C:"]) +@pytest.mark.parametrize("operation", ["read_bytes", "create_bytes", "write_bytes", "mkdir", "delete"]) +@pytest.mark.parametrize("allow", [False, True]) +def test_native_windows_anchored_relative_operations_rejected(root, spelling, operation, allow): + args = (spelling, b"bad") if operation in ("create_bytes", "write_bytes") else (spelling,) + with pytest.raises(PathEscapeError): + getattr(FileHelper(root, allow_symlinks=allow), operation)(*args) + assert list(root.iterdir()) == [] + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows path semantics required") +@pytest.mark.parametrize("spelling", ["/outside/file", r"\outside\file", "C:outside/file", "C:"]) +def test_native_windows_invalid_link_targets_rejected(root, link, monkeypatch, spelling): + (root / "file").write_text("keep") + alias = link(root / "alias", root / "file") + files = FileHelper(root, allow_symlinks=True) + with pytest.raises(PathEscapeError): + files.symlink(spelling, "new") + monkeypatch.setattr("specify_cli.file_helper.os.readlink", lambda path: spelling) + with pytest.raises(PathEscapeError): + files.read_text(alias) + with pytest.raises(PathEscapeError): + files.write_text(alias, "bad") + assert (root / "file").read_text() == "keep" + assert not os.path.lexists(root / "new") + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows path semantics required") +def test_native_windows_absolute_and_relative_scoping(root): + files = FileHelper(root) + files.write_text(root / "absolute", "ok") + files.write_text("relative", "ok") + assert files.read_text(root / "absolute") == "ok" + assert files.read_text("relative") == "ok" + with pytest.raises(PathEscapeError): + files.write_text(root.parent / "outside", "bad") + + +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_directory_reparse_entries_rejected_before_traversal_portably( + root, monkeypatch, allow, recursive +): + (root / "tree").mkdir() + (root / "tree/a-file").write_text("keep") + junction = root / "tree/z-junction" + junction.mkdir() + (junction / "keep").write_text("keep") + lstat = Path.lstat + + def directory_reparse(path, **kwargs): + result = lstat(path, **kwargs) + if path == junction: + return SimpleNamespace( + st_mode=result.st_mode, + st_file_attributes=0x400, + st_reparse_tag=0xA0000003, + ) + return result + + monkeypatch.setattr(Path, "lstat", directory_reparse) + files = FileHelper(root, allow_symlinks=allow) + for operation in ( + lambda: files.read_bytes(junction / "keep"), + lambda: files.write_bytes(junction / "keep", b"bad"), + lambda: files.create_bytes(junction / "new", b"bad"), + lambda: files.mkdir(junction / "new-directory"), + lambda: files.delete(junction, recursive=recursive), + lambda: files.delete("tree", recursive=True), + ): + with pytest.raises(UnsupportedPathError, match="reparse"): + operation() + assert (root / "tree/a-file").read_text() == "keep" + assert (junction / "keep").read_text() == "keep" + assert not (junction / "new").exists() + assert not (junction / "new-directory").exists() + + +@pytest.mark.parametrize("allow", [False, True]) +def test_symlink_reparse_entries_retain_normal_symlink_policy_portably(root, link, monkeypatch, allow): + (root / "file").write_text("keep") + alias = link(root / "alias", root / "file") + lstat = Path.lstat + + def symlink_reparse(path, **kwargs): + result = lstat(path, **kwargs) + if path == alias: + return SimpleNamespace( + st_mode=result.st_mode, st_file_attributes=0x400, st_reparse_tag=0xA000000C, + ) + return result + + monkeypatch.setattr(Path, "lstat", symlink_reparse) + files = FileHelper(root, allow_symlinks=allow) + if allow: + assert files.read_text(alias) == "keep" + files.write_text(alias, "updated") + assert (root / "file").read_text() == "updated" + files.delete(alias) + assert not os.path.lexists(alias) + else: + with pytest.raises(SymlinkDeniedError): + files.read_text(alias) + with pytest.raises(SymlinkDeniedError): + files.delete(alias) + assert alias.is_symlink() + assert (root / "file").read_text() == "keep" + + +@pytest.fixture +def junction(root): + if os.name != "nt": + pytest.skip("Native Windows junction creation required") + + def create(path, target): + result = subprocess.run( + ["cmd.exe", "/c", "mklink", "/J", str(path), str(target)], + capture_output=True, text=True, check=False, timeout=30, + ) + if result.returncode: + pytest.skip(f"Windows junction creation unavailable: {result.stderr or result.stdout}") + return path + + return create + + +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("external", [False, True]) +def test_native_windows_junctions_never_traversed_or_deleted(root, junction, allow, external): + target = root.parent / "outside" if external else root / "target" + target.mkdir() + (target / "keep").write_text("keep") + tree = root / "tree" + tree.mkdir() + (tree / "a-file").write_text("keep") + alias = junction(tree / "z-junction", target) + files = FileHelper(root, allow_symlinks=allow) + for operation in ( + lambda: files.read_text(alias / "keep"), + lambda: files.write_text(alias / "keep", "bad"), + lambda: files.create_text(alias / "new", "bad"), + lambda: files.mkdir(alias / "new-directory"), + lambda: files.delete(alias / "keep"), + lambda: files.delete(alias), + lambda: files.delete(alias, recursive=True), + lambda: files.delete(tree, recursive=True), + ): + with pytest.raises(UnsupportedPathError, match="reparse"): + operation() + assert alias.exists() + assert (target / "keep").read_text() == "keep" + assert (tree / "a-file").read_text() == "keep" + assert sorted(p.name for p in target.iterdir()) == ["keep"] From 369fa47c0410054e0aa46703b59b834adbc6f4b3 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:29:53 -0500 Subject: [PATCH 2/5] fix: normalize Windows extended FileHelper targets Reuse the existing drive/UNC prefix normalization for root comparisons while retaining accessed hierarchy evidence and rejecting escaping or unsupported targets. Add portable regression and native Windows operation cases. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e --- design/file-helper.md | 7 ++ src/specify_cli/file_helper.py | 27 ++++++-- src/specify_cli/integration_status.py | 17 +---- tests/test_file_helper.py | 95 +++++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 20 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index b7aae100d1..2d7cd86d2f 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -37,6 +37,13 @@ Windows rooted-but-not-absolute paths (`/outside/file`, `\outside\file`) and drive-relative paths (`C:outside/file`, `C:`) are rejected for both operation paths and link targets. They are not root-relative: joining them can reset the root or drive. Fully absolute paths still require scoped root matching. +For Windows containment comparisons, extended-length drive and UNC spellings +(`\\?\D:\project\file`, `\\?\UNC\server\share\project\file`) are compared with +their ordinary equivalents on both the target and root sides. This does not +resolve links or collapse `..`: accessed hierarchy evidence is retained for +the component walk. External targets, ambiguous rooted-relative paths, and +unsupported device namespaces are still rejected. POSIX filenames are not +reinterpreted as Windows path spellings. The caller must establish a trusted, existing directory root. Its alias is resolved once, including OS and worktree aliases; the root itself and OS diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 29837edf21..64cd49840d 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -2,9 +2,23 @@ from dataclasses import dataclass, field import os -from pathlib import Path, PurePath +from pathlib import Path, PurePath, PureWindowsPath import stat import tempfile +from typing import TypeVar + + +_PathType = TypeVar("_PathType", bound=PurePath) + + +def _strip_extended_length_prefix(path: _PathType) -> _PathType: + """Normalize Windows extended drive/UNC spellings for comparison only.""" + raw = str(path) + if raw.startswith("\\\\?\\UNC\\"): + return type(path)("\\\\" + raw[len("\\\\?\\UNC\\"):]) + if raw.startswith("\\\\?\\"): + return type(path)(raw[len("\\\\?\\"):]) + return path class FileHelperError(ValueError): @@ -44,16 +58,21 @@ def __post_init__(self) -> None: object.__setattr__(self, "_canonical_root", canonical) def _parts(self, path: PurePath) -> tuple[str, ...]: + original = path + if isinstance(path, PureWindowsPath): + path = _strip_extended_length_prefix(path) if not path.is_absolute(): - if path.anchor: - raise PathEscapeError(f"Rooted-relative or drive-relative path is not permitted: {path}") + if original.anchor or path.anchor: + raise PathEscapeError(f"Rooted-relative, drive-relative, or device path is not permitted: {original}") return path.parts for root in (self.root, self._canonical_root): + if isinstance(root, PureWindowsPath): + root = _strip_extended_length_prefix(root) try: return path.relative_to(root).parts except ValueError: continue - raise PathEscapeError(f"Path is outside root {self.root}: {path}") + raise PathEscapeError(f"Path is outside root {self.root}: {original}") @staticmethod def _entry_mode(path: Path) -> int: diff --git a/src/specify_cli/integration_status.py b/src/specify_cli/integration_status.py index 050f0629d3..91666b4cae 100644 --- a/src/specify_cli/integration_status.py +++ b/src/specify_cli/integration_status.py @@ -8,6 +8,7 @@ from pathlib import Path from typing import Any +from .file_helper import _strip_extended_length_prefix from .integration_state import ( INTEGRATION_JSON, INTEGRATION_STATE_SCHEMA, @@ -95,22 +96,6 @@ def _sha256_file(path: Path) -> str: return h.hexdigest() -def _strip_extended_length_prefix(path: Path) -> Path: - """Drop the Windows ``\\\\?\\`` extended-length prefix for path comparison. - - ``os.readlink`` and ``Path.resolve`` can return extended-length paths on - Windows (e.g. ``\\\\?\\C:\\proj``). Comparing such a path against a plain - ``C:\\proj`` root via :meth:`Path.relative_to` would spuriously fail, so we - normalise both sides through this helper before containment checks. - """ - raw = str(path) - if raw.startswith("\\\\?\\UNC\\"): - return Path("\\\\" + raw[len("\\\\?\\UNC\\"):]) - if raw.startswith("\\\\?\\"): - return Path(raw[len("\\\\?\\"):]) - return path - - def _is_within_project(project_root_resolved: Path, candidate: Path) -> bool: """Return ``True`` when *candidate* stays within *project_root_resolved*. diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 1163058fe0..70ea763126 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -546,6 +546,101 @@ def test_windows_absolute_forms_outside_declared_roots_rejected_portably(spellin FileHelper._parts(boundary, PureWindowsPath(spelling)) +@pytest.mark.parametrize("unc", [False, True]) +@pytest.mark.parametrize("root_extended", [False, True]) +@pytest.mark.parametrize("target_extended", [False, True]) +def test_windows_extended_contained_targets_match_roots_portably( + unc, root_extended, target_extended +): + plain = r"\\server\share\project" if unc else r"D:\project" + extended = r"\\?\UNC\server\share\project" if unc else r"\\?\D:\project" + root_path = PureWindowsPath(extended if root_extended else plain) + target = PureWindowsPath(extended if target_extended else plain) / "inside/file" + boundary = SimpleNamespace(root=root_path, _canonical_root=root_path) + assert FileHelper._parts(boundary, target) == ("inside", "file") + + +@pytest.mark.parametrize( + "spelling", + [ + r"\\?\D:\project2\file", + r"\\?\D:\outside\file", + r"\\?\C:\project\file", + r"\\?\UNC\server\share\project2\file", + r"\\?\UNC\other\share\project\file", + r"\\?\GLOBALROOT\Device\HarddiskVolume1\file", + r"\\?\C:relative\file", + ], +) +def test_windows_extended_external_or_unsupported_targets_rejected_portably(spelling): + boundary = SimpleNamespace( + root=PureWindowsPath("D:/project"), + _canonical_root=PureWindowsPath(r"\\server\share\project"), + ) + with pytest.raises(PathEscapeError): + FileHelper._parts(boundary, PureWindowsPath(spelling)) + + +def test_windows_extended_comparison_preserves_accessed_hierarchy_portably(): + boundary = SimpleNamespace( + root=PureWindowsPath("D:/project"), + _canonical_root=PureWindowsPath("D:/project"), + ) + assert FileHelper._parts( + boundary, PureWindowsPath(r"\\?\D:\project\linked\..\file") + ) == ("linked", "..", "file") + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX literal backslash filename semantics required") +def test_posix_windows_like_filenames_are_not_reinterpreted(root, link): + name = r"\\?\D:\literal" + files = FileHelper(root, allow_symlinks=True) + files.create_text(name, "old") + files.symlink(name, "alias") + assert files.read_text("alias") == "old" + files.write_text("alias", "new") + assert (root / name).read_text() == "new" + assert (root / "alias").is_symlink() + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows symlink semantics required") +@pytest.mark.parametrize("root_extended", [False, True]) +@pytest.mark.parametrize("external", [False, True]) +def test_native_windows_extended_symlink_read_update_and_creation( + root, link, root_extended, external +): + target = root.parent / "outside" if external else root / "target" + target.write_text("old") + + def extended(path): + raw = str(path) + return "\\\\?\\UNC\\" + raw[2:] if raw.startswith("\\\\") else "\\\\?\\" + raw + + alias = link(root / "alias", extended(target)) + original_target = os.readlink(alias) + files = FileHelper(Path(extended(root)) if root_extended else root, allow_symlinks=True) + if external: + with pytest.raises(PathEscapeError): + files.read_text("alias") + with pytest.raises(PathEscapeError): + files.write_text("alias", "bad") + with pytest.raises(PathEscapeError): + files.symlink(extended(target), "new") + assert target.read_text() == "old" + assert not os.path.lexists(root / "new") + else: + assert files.read_text("alias") == "old" + files.write_text("alias", "new") + assert target.read_text() == "new" + assert alias.is_symlink() + assert os.readlink(alias) == original_target + files.symlink(extended(target), "created") + assert files.read_text("created") == "new" + files.delete("alias") + assert not os.path.lexists(alias) + assert target.exists() + + @pytest.mark.skipif(os.name != "nt", reason="Native Windows path semantics required") @pytest.mark.parametrize("spelling", ["/outside/file", r"\outside\file", "C:outside/file", "C:"]) @pytest.mark.parametrize("operation", ["read_bytes", "create_bytes", "write_bytes", "mkdir", "delete"]) From a4c857807e88120845722c12e4c440d106da7631 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:42:31 -0500 Subject: [PATCH 3/5] fix: scope parent traversal checks below trusted roots Derive operation-relative parts before rejecting parent traversal so paths built from trusted roots containing parent components remain usable. Preserve lexical root evidence and reject traversal in operation suffixes; add positive and negative regression coverage. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e --- design/file-helper.md | 7 +++- src/specify_cli/file_helper.py | 5 ++- tests/test_file_helper.py | 74 ++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 3 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 2d7cd86d2f..311a27d564 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -31,7 +31,12 @@ files.delete("generated", recursive=True) Each helper has a frozen root and `allow_symlinks` setting. Relative operation paths are root-relative. Absolute paths must be beneath the supplied root or its canonical equivalent. User operation paths containing `..` are rejected -rather than normalized before inspection. Link targets may contain `..`, but +in their root-relative suffix rather than normalized before inspection. A +caller-established root spelling may itself contain `..`; absolute operation +paths derived from that exact supplied root prefix are supported. Only that +trusted prefix is removed before checking operation components, without +lexically collapsing the root or ignoring traversal in the remaining suffix. +Link targets may contain `..`, but are walked component by component and may not leave the root, even temporarily. Windows rooted-but-not-absolute paths (`/outside/file`, `\outside\file`) and drive-relative paths (`C:outside/file`, `C:`) are rejected for both operation diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 64cd49840d..6b40d9c098 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -138,10 +138,11 @@ def _path( self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True ) -> Path: original = Path(path) - if ".." in original.parts: + parts = self._parts(original) + if ".." in parts: raise PathEscapeError(f"Parent traversal is not an operation path: {original}") return self._walk( - self._parts(original), allow_missing=allow_missing, follow_leaf=follow_leaf + parts, allow_missing=allow_missing, follow_leaf=follow_leaf ) @staticmethod diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 70ea763126..5b62ff7c6a 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -301,6 +301,80 @@ def test_trusted_root_alias_and_os_ancestors_are_not_rejected(root, link, allow) assert files.read_text(root / "file") == "works" +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("relative_root", [False, True]) +def test_operations_derived_from_trusted_parent_traversal_root(root, monkeypatch, allow, relative_root): + anchor = root.parent / "anchor" + anchor.mkdir() + if relative_root: + monkeypatch.chdir(anchor) + supplied = Path("../project") + else: + supplied = anchor / ".." / root.name + files = FileHelper(supplied, allow_symlinks=allow) + assert ".." in files.root.parts + directory = files.root / "created" + files.mkdir(directory) + files.create_text(directory / "file", "old") + assert files.read_text(directory / "file") == "old" + files.write_text(directory / "file", "new") + assert (root / "created/file").read_text() == "new" + assert files.read_text(root.resolve() / "created/file") == "new" + files.delete(directory / "file") + files.write_bytes(directory / "new", b"new") + files.delete(directory, recursive=True) + assert not (root / "created").exists() + with pytest.raises(FileHelperError, match="established root"): + files.delete(files.root, recursive=True) + assert root.is_dir() + + +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("relative_root", [False, True]) +def test_trusted_parent_traversal_root_does_not_permit_operation_suffix_traversal( + root, monkeypatch, allow, relative_root +): + anchor = root.parent / "anchor" + anchor.mkdir() + outside = root.parent / "outside" + outside.mkdir() + (outside / "keep").write_bytes(b"keep") + if relative_root: + monkeypatch.chdir(anchor) + supplied = Path("../project") + else: + supplied = anchor / ".." / root.name + files = FileHelper(supplied, allow_symlinks=allow) + for path in (files.root / "../outside/keep", Path("../outside/keep"), + files.root / "inside/../../outside/keep"): + for operation in ( + lambda: files.read_bytes(path), + lambda: files.write_bytes(path, b"bad"), + lambda: files.create_bytes(path, b"bad"), + lambda: files.mkdir(path, parents=True), + lambda: files.delete(path), + ): + with pytest.raises(PathEscapeError, match="Parent traversal"): + operation() + assert (outside / "keep").read_bytes() == b"keep" + assert list(root.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink/parent traversal semantics required") +@pytest.mark.parametrize("allow", [False, True]) +def test_trusted_root_parent_traversal_is_not_lexically_collapsed(root, link, allow): + container = root.parent / "different/container" + container.mkdir(parents=True) + selected = root.parent / "different/project" + selected.mkdir() + alias = link(root.parent / "root-entry", container) + files = FileHelper(alias / "../project", allow_symlinks=allow) + files.create_text(files.root / "file", "selected") + assert (selected / "file").read_text() == "selected" + assert list(root.iterdir()) == [] + assert alias.is_symlink() + + @pytest.mark.parametrize("path", ["../escape", "inside/../escape"]) def test_operation_parent_traversal_is_rejected(root, path): with pytest.raises(PathEscapeError): From ae29ace7a591465ebd9fadd854337a6211fd4422 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:24:08 -0500 Subject: [PATCH 4/5] fix: handle mixed-case Windows UNC namespaces Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e --- design/file-helper.md | 4 +++- src/specify_cli/file_helper.py | 2 +- tests/test_file_helper.py | 20 ++++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 311a27d564..b2b9dd426e 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -46,7 +46,9 @@ For Windows containment comparisons, extended-length drive and UNC spellings (`\\?\D:\project\file`, `\\?\UNC\server\share\project\file`) are compared with their ordinary equivalents on both the target and root sides. This does not resolve links or collapse `..`: accessed hierarchy evidence is retained for -the component walk. External targets, ambiguous rooted-relative paths, and +the component walk. The UNC namespace marker is matched case-insensitively, +while the remaining path's original spelling is preserved. External targets, +ambiguous rooted-relative paths, and unsupported device namespaces are still rejected. POSIX filenames are not reinterpreted as Windows path spellings. diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 6b40d9c098..ffdc907101 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -14,7 +14,7 @@ def _strip_extended_length_prefix(path: _PathType) -> _PathType: """Normalize Windows extended drive/UNC spellings for comparison only.""" raw = str(path) - if raw.startswith("\\\\?\\UNC\\"): + if raw.lower().startswith("\\\\?\\unc\\"): return type(path)("\\\\" + raw[len("\\\\?\\UNC\\"):]) if raw.startswith("\\\\?\\"): return type(path)(raw[len("\\\\?\\"):]) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 5b62ff7c6a..3a39ec01be 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -634,6 +634,26 @@ def test_windows_extended_contained_targets_match_roots_portably( assert FileHelper._parts(boundary, target) == ("inside", "file") +@pytest.mark.parametrize("marker", ["unc", "uNc", "UnC", "UNC"]) +@pytest.mark.parametrize("prefixed_root", [False, True]) +def test_windows_mixed_case_unc_prefixes_preserve_contained_targets(marker, prefixed_root): + plain = r"\\Server\Share\Project" + prefixed = "\\\\?\\" + marker + r"\Server\Share\Project" + root_path = PureWindowsPath(prefixed if prefixed_root else plain) + boundary = SimpleNamespace(root=root_path, _canonical_root=root_path) + target = PureWindowsPath(prefixed + r"\ChIlD\FiLe") + assert FileHelper._parts(boundary, target) == ("ChIlD", "FiLe") + + +@pytest.mark.parametrize("marker", ["unc", "uNc", "UnC", "UNC"]) +def test_windows_mixed_case_unc_prefixes_do_not_allow_external_targets(marker): + root_path = PureWindowsPath(r"\\Server\Share\Project") + boundary = SimpleNamespace(root=root_path, _canonical_root=root_path) + target = PureWindowsPath("\\\\?\\" + marker + r"\Server\Share\ProjectSibling\file") + with pytest.raises(PathEscapeError): + FileHelper._parts(boundary, target) + + @pytest.mark.parametrize( "spelling", [ From 2ad33b7663752bd8c531174b1ddcfca6e0b67efc Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Sat, 10 Oct 2026 05:50:18 -0500 Subject: [PATCH 5/5] fix: reject existing leaves before exclusive file creation Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e8758245-6031-47dd-8cd9-4ed60611181e --- design/file-helper.md | 8 +++- src/specify_cli/file_helper.py | 7 ++++ tests/test_file_helper.py | 67 +++++++++++++++++++++++++++++++--- 3 files changed, 75 insertions(+), 7 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index b2b9dd426e..fa9d4dd0ac 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -65,7 +65,7 @@ helpers for independent project/source/managed boundaries. |---|---|---| | Read file | Reject accessed parent or leaf links | Follow only existing contained regular-file targets | | Create directory | Reject accessed links before creating parents | Follow contained directory links; reject dangling targets | -| Exclusively create file | Reject accessed links | Follow contained parents; an existing leaf link is never overwritten | +| Exclusively create file | Reject accessed links | Follow contained parents; reject an existing leaf without opening its target | | Atomic write/upsert | Reject accessed links | Replace the resolved regular-file target and preserve every link | | Delete leaf link | Reject without mutation | Unlink the link, including dangling, cyclic, or external-target links | | Delete through linked parent | Reject without mutation | Delete the contained target entry, leaving the parent link intact | @@ -77,7 +77,11 @@ helpers for independent project/source/managed boundaries. - `mkdir(path, parents=False, exist_ok=False)` creates directories. - `read_bytes(path)` / `read_text(path, encoding="utf-8")` read regular files. - `create_bytes(path, content, mode=0o644)` / `create_text(...)` exclusively - create files with existing parents. + create files with existing parents. A no-follow `lstat` preflight rejects + existing leaves, including dangling links, before opening the destination. + This avoids target creation on backends that follow dangling links despite + exclusive-open flags. `O_EXCL` and `O_NOFOLLOW` (where available) remain in use; + the static preflight does not eliminate concurrent path-change races. - `write_bytes(path, content, mode=0o644)` / `write_text(...)` atomically create or update files with existing parents. Text variants also accept `encoding="utf-8"`. Replacement files use the supplied mode; metadata and diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index ffdc907101..ba135135d0 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -1,6 +1,7 @@ """Scoped filesystem operations; see design/file-helper.md for the contract.""" from dataclasses import dataclass, field +import errno import os from pathlib import Path, PurePath, PureWindowsPath import stat @@ -168,6 +169,12 @@ def read_text(self, path: Path | str, *, encoding: str = "utf-8") -> str: def create_bytes(self, path: Path | str, content: bytes, *, mode: int = 0o644) -> None: """Exclusively create a file; never overwrite an existing entry.""" destination = self._path(path, allow_missing=True, follow_leaf=False) + try: + self._entry_mode(destination) + except FileNotFoundError: + pass + else: + raise FileExistsError(errno.EEXIST, os.strerror(errno.EEXIST), str(destination)) flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL flags |= getattr(os, "O_NOFOLLOW", 0) fd = os.open(destination, flags, mode) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 3a39ec01be..c7da7239b9 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1,6 +1,7 @@ """Operation-level evidence for the scoped FileHelper foundation.""" from dataclasses import FrozenInstanceError +import errno import os from pathlib import Path, PureWindowsPath import subprocess @@ -421,20 +422,76 @@ def test_intermediate_target_link_is_checked(root, link): @pytest.mark.parametrize("allow", [False, True]) -def test_exclusive_create_never_overwrites_file_or_link(root, link, allow): +@pytest.mark.parametrize("entry", ["file", "alias", "dangling"]) +def test_exclusive_create_never_overwrites_file_or_link(root, link, allow, entry): (root / "file").write_bytes(b"keep") alias = link(root / "alias", root / "file") dangling = link(root / "dangling", root / "missing") files = FileHelper(root, allow_symlinks=allow) - for path in (root / "file", alias, dangling): - error = SymlinkDeniedError if path.is_symlink() and not allow else FileExistsError - with pytest.raises(error): - files.create_bytes(path, b"bad") + path = root / entry + error = SymlinkDeniedError if path.is_symlink() and not allow else FileExistsError + with pytest.raises(error): + files.create_bytes(path, b"bad") assert (root / "file").read_bytes() == b"keep" assert alias.is_symlink() and dangling.is_symlink() assert not (root / "missing").exists() +@pytest.mark.parametrize("operation", ["create_bytes", "create_text"]) +@pytest.mark.parametrize("cyclic", [False, True]) +def test_exclusive_create_rejects_existing_leaf_before_open( + root, link, monkeypatch, operation, cyclic +): + target = root / "target" + target.write_bytes(b"keep") + alias = link(root / "alias", root / "alias" if cyclic else target) + original_target = os.readlink(alias) + opened = [] + real_open = os.open + + def recording_open(path, flags, mode): + opened.append(Path(path)) + return real_open(path, flags, mode) + + monkeypatch.setattr("specify_cli.file_helper.os.open", recording_open) + content = b"bad" if operation == "create_bytes" else "bad" + with pytest.raises(FileExistsError) as error: + getattr(FileHelper(root, allow_symlinks=True), operation)(alias, content) + assert error.value.errno == errno.EEXIST + assert opened == [] + assert alias.is_symlink() + assert os.readlink(alias) == original_target + assert target.read_bytes() == b"keep" + + +@pytest.mark.parametrize("operation", ["create_bytes", "create_text"]) +@pytest.mark.parametrize("external", [False, True]) +def test_exclusive_create_rejects_dangling_leaf_with_windows_backend( + root, link, monkeypatch, operation, external +): + target = root.parent / "external-missing" if external else root / "missing" + alias = link(root / "alias", target) + original_target = os.readlink(alias) + opened = [] + real_open = os.open + monkeypatch.delattr("specify_cli.file_helper.os.O_NOFOLLOW", raising=False) + + def windows_open(path, flags, mode): + opened.append(Path(path)) + assert flags & os.O_CREAT and flags & os.O_EXCL + return real_open(Path(os.readlink(path)), flags, mode) + + monkeypatch.setattr("specify_cli.file_helper.os.open", windows_open) + content = b"bad" if operation == "create_bytes" else "bad" + with pytest.raises(FileExistsError) as error: + getattr(FileHelper(root, allow_symlinks=True), operation)(alias, content) + assert error.value.errno == errno.EEXIST + assert opened == [] + assert alias.is_symlink() + assert os.readlink(alias) == original_target + assert not os.path.lexists(target) + + def test_symlink_creation_requires_permission_and_contained_target(root, link): (root / "file").write_text("ok") with pytest.raises(SymlinkDeniedError):