From 10c5b22ebeb0c55573bc2eb2e6cdc51073917a16 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:45:17 -0600 Subject: [PATCH 01/16] fix: use handle-based FileHelper deletion Anchor recursive enumeration and entry removal to directory handles while preserving supported symlink behavior. Share the Windows handle primitive and document deletion semantics. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 29 +- src/specify_cli/_file_handles.py | 60 ++++ src/specify_cli/file_helper.py | 55 +++- src/specify_cli/workflows/_catalog_cache.py | 59 +--- tests/test_file_handles.py | 69 +++++ tests/test_file_helper.py | 311 ++++++++++++++++++++ 6 files changed, 510 insertions(+), 73 deletions(-) create mode 100644 src/specify_cli/_file_handles.py create mode 100644 tests/test_file_handles.py diff --git a/design/file-helper.md b/design/file-helper.md index b9dbcddd2f..3658ff45dc 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -181,7 +181,12 @@ helpers for independent project/source/managed boundaries. This is not hard-link confinement: a hard-linked alias can observe updates. - `delete(path, recursive=False)` unlinks files/links or removes directories. Nonrecursive directory deletion requires an empty directory. The established - root cannot be deleted. + root cannot be deleted. Recursive enumeration and each removal use anchored + directories: POSIX uses no-follow directory descriptors and descriptor-relative + `unlink`/`rmdir`; Windows holds the resolved ancestors without delete sharing. + Allowed contained parent links are resolved before anchoring, and leaf links + are still unlinked rather than followed. Platforms lacking the required + directory-descriptor operations fail with `ENOTSUP`, not pathname-only deletion. - `symlink(target, path)` creates a link exclusively. Relative targets are interpreted relative to the link parent, not the helper root. @@ -209,13 +214,17 @@ entries, not reject a whole project for excluded/unrelated links. 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. +deletion rechecks each accessed entry. Deletion also anchors directory +enumeration and removal so exchanging an accessed parent for a link cannot +redirect those operations. POSIX removal stays attached to the opened directory +even if that directory is renamed; Windows prevents ancestor renames while +handles are held. Concurrent changes may still cause deletion to fail or leave +partial results, and preflight is not a transaction or an immutable tree snapshot. +Leaf removal acts on the current entry in the anchored parent, not a pinned leaf +identity. Other mutations retain pathname-based checks and are **not** a race-free +sandbox. Protect those operations against concurrent untrusted mutation +separately. 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 @@ -229,4 +238,6 @@ path replacement. 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. +Windows filesystem syscalls or junction behavior. Deletion replacement tests +exercise swaps at enumeration and removal boundaries on the native host; Windows +handle-sharing behavior requires a native Windows run. diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py new file mode 100644 index 0000000000..e453d82ff5 --- /dev/null +++ b/src/specify_cli/_file_handles.py @@ -0,0 +1,60 @@ +"""Private platform handles for scoped filesystem operations.""" + +import errno +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path + + +@contextmanager +def _windows_handle(path: Path, *, directory: bool) -> Iterator[int]: + """Pin a non-reparse path by withholding Windows delete sharing.""" + import ctypes + from ctypes import wintypes + + class AttributeTagInfo(ctypes.Structure): + _fields_ = [("attributes", wintypes.DWORD), ("tag", wintypes.DWORD)] + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + create = kernel.CreateFileW + create.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + wintypes.LPVOID, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ] + create.restype = wintypes.HANDLE + info = kernel.GetFileInformationByHandleEx + info.argtypes = [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] + info.restype = wintypes.BOOL + close = kernel.CloseHandle + close.argtypes = [wintypes.HANDLE] + close.restype = wintypes.BOOL + + # Attribute-only handles do not prevent directory renames. Include + # FILE_LIST_DIRECTORY, and inspect reparse entries without following them. + handle = create( + str(path), + (0x80 | 0x1) if directory else 0x80000000, + 0x3, + None, + 3, + 0x00200000 | 0x02000000, + None, + ) + if handle == wintypes.HANDLE(-1).value: + raise ctypes.WinError(ctypes.get_last_error()) + try: + attributes = AttributeTagInfo() + if not info(handle, 9, ctypes.byref(attributes), ctypes.sizeof(attributes)): + raise ctypes.WinError(ctypes.get_last_error()) + if attributes.attributes & 0x400: + raise OSError(errno.ELOOP, "Refusing a reparse-point path", str(path)) + if bool(attributes.attributes & 0x10) != directory: + raise OSError(errno.EINVAL, "Unexpected file type", str(path)) + yield handle + finally: + close(handle) diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 49441ce494..dca7a82124 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -1,5 +1,7 @@ """Scoped filesystem operations; see design/file-helper.md for the contract.""" +from collections.abc import Iterator +from contextlib import ExitStack, contextmanager from dataclasses import dataclass, field import codecs import errno @@ -9,8 +11,14 @@ import tempfile from typing import TypeVar +from specify_cli._file_handles import _windows_handle + _PathType = TypeVar("_PathType", bound=PurePath) +_DELETE_DIR_FD_SUPPORTED = ( + {os.open, os.unlink, os.rmdir} <= os.supports_dir_fd + and os.scandir in os.supports_fd +) def _strip_extended_length_prefix(path: _PathType) -> _PathType: @@ -238,6 +246,37 @@ def symlink(self, target: Path | str, path: Path | str) -> None: resolved = self._walk(target_parts) destination.symlink_to(target, target_is_directory=resolved.is_dir()) + @contextmanager + def _deletion_directory(self, path: Path) -> Iterator[int | Path]: + """Anchor a resolved directory without following replacement links.""" + parts = path.relative_to(self._canonical_root).parts + if os.name == "nt": + with ExitStack() as stack: + current = self._canonical_root + stack.enter_context(_windows_handle(current, directory=True)) + for part in parts: + current = current / part + stack.enter_context(_windows_handle(current, directory=True)) + yield current + return + + if not ( + _DELETE_DIR_FD_SUPPORTED + and getattr(os, "O_NOFOLLOW", 0) + and getattr(os, "O_DIRECTORY", 0) + ): + raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") + flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + parent = os.open(self._canonical_root, flags) + try: + for part in parts: + child = os.open(part, flags, dir_fd=parent) + os.close(parent) + parent = child + yield parent + finally: + os.close(parent) + def _deletion_plan(self, path: Path, *, recursive: bool) -> list[tuple[Path, bool]]: mode = self._entry_mode(path) if stat.S_ISLNK(mode): @@ -250,8 +289,9 @@ def _deletion_plan(self, path: Path, *, recursive: bool) -> list[tuple[Path, boo 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)) + with self._deletion_directory(path) as directory: + with os.scandir(directory) as entries: + children = sorted(path / entry.name for entry in entries) for child in children: plan.extend(self._deletion_plan(child, recursive=True)) plan.append((path, True)) @@ -268,7 +308,10 @@ def delete(self, path: Path | str, *, recursive: bool = False) -> None: 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() + with self._deletion_directory(entry.parent) as parent: + leaf = parent / entry.name if isinstance(parent, Path) else entry.name + directory_fd = None if isinstance(parent, Path) else parent + if directory: + os.rmdir(leaf, dir_fd=directory_fd) + else: + os.unlink(leaf, dir_fd=directory_fd) diff --git a/src/specify_cli/workflows/_catalog_cache.py b/src/specify_cli/workflows/_catalog_cache.py index 5a64609bc0..784e7fb97d 100644 --- a/src/specify_cli/workflows/_catalog_cache.py +++ b/src/specify_cli/workflows/_catalog_cache.py @@ -12,64 +12,7 @@ from typing import Any from uuid import uuid4 - -@contextmanager -def _windows_handle(path: Path, *, directory: bool) -> Iterator[int]: - """Pin a non-reparse path by withholding Windows delete sharing.""" - import ctypes - from ctypes import wintypes - - class AttributeTagInfo(ctypes.Structure): - _fields_ = [("attributes", wintypes.DWORD), ("tag", wintypes.DWORD)] - - kernel = ctypes.WinDLL("kernel32", use_last_error=True) - create = kernel.CreateFileW - create.argtypes = [ - wintypes.LPCWSTR, - wintypes.DWORD, - wintypes.DWORD, - wintypes.LPVOID, - wintypes.DWORD, - wintypes.DWORD, - wintypes.HANDLE, - ] - create.restype = wintypes.HANDLE - info = kernel.GetFileInformationByHandleEx - info.argtypes = [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] - info.restype = wintypes.BOOL - close = kernel.CloseHandle - close.argtypes = [wintypes.HANDLE] - close.restype = wintypes.BOOL - - # OPEN_REPARSE_POINT inspects the link itself; BACKUP_SEMANTICS permits - # directory handles. Holding every ancestor without FILE_SHARE_DELETE - # prevents a junction/symlink swap during path-based create/replace. - # Attribute-only handles bypass share checking: include FILE_LIST_DIRECTORY - # with FILE_READ_ATTRIBUTES so even an empty directory cannot be renamed. - handle = create( - str(path), - (0x80 | 0x1) if directory else 0x80000000, - 0x3, - None, - 3, - 0x00200000 | 0x02000000, - None, - ) - if handle == wintypes.HANDLE(-1).value: - raise ctypes.WinError(ctypes.get_last_error()) - try: - attributes = AttributeTagInfo() - if not info(handle, 9, ctypes.byref(attributes), ctypes.sizeof(attributes)): - raise ctypes.WinError(ctypes.get_last_error()) - if attributes.attributes & 0x400: - raise OSError( - errno.ELOOP, "Refusing a reparse-point catalog cache path", str(path) - ) - if bool(attributes.attributes & 0x10) != directory: - raise OSError(errno.EINVAL, "Unexpected catalog cache file type", str(path)) - yield handle - finally: - close(handle) +from specify_cli._file_handles import _windows_handle @contextmanager diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py new file mode 100644 index 0000000000..a5396b9511 --- /dev/null +++ b/tests/test_file_handles.py @@ -0,0 +1,69 @@ +"""Portable contract checks for the shared Windows handle primitive.""" + +import ctypes +import errno +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from specify_cli._file_handles import _windows_handle + + +@pytest.mark.parametrize("directory", [False, True]) +@pytest.mark.parametrize("outcome", ["success", "reparse", "wrong-type", "query-failure", "body-failure", "open-failure"]) +def test_windows_handle_flags_validation_and_cleanup(monkeypatch, directory, outcome): + calls = [] + + def create(*args): + calls.append(("open", args)) + return ctypes.c_void_p(-1).value if outcome == "open-failure" else 123 + + def info(handle, kind, buffer, size): + assert handle == 123 and kind == 9 + attributes = 0x10 if directory else 0 + if outcome == "reparse": + attributes |= 0x400 + if outcome == "wrong-type": + attributes ^= 0x10 + buffer._obj.attributes = attributes + return outcome != "query-failure" + + def close(handle): + calls.append(("close", handle)) + return True + + # Real ctypes functions accept signature attributes; plain Python functions + # do too, allowing this contract to run without loading a Windows DLL. + kernel = SimpleNamespace( + CreateFileW=create, GetFileInformationByHandleEx=info, CloseHandle=close + ) + monkeypatch.setattr(ctypes, "WinDLL", lambda *args, **kwargs: kernel, raising=False) + monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) + monkeypatch.setattr( + ctypes, "WinError", lambda code: PermissionError(code, "handle denied"), raising=False + ) + + def use(): + with _windows_handle(Path("selected"), directory=directory) as handle: + assert handle == 123 + assert calls == [calls[0]] + if outcome == "body-failure": + raise RuntimeError("body failed") + + if outcome == "success": + use() + elif outcome == "body-failure": + with pytest.raises(RuntimeError, match="body failed"): + use() + else: + with pytest.raises(OSError) as exc: + use() + if outcome in ("reparse", "wrong-type"): + assert exc.value.errno == (errno.ELOOP if outcome == "reparse" else errno.EINVAL) + args = calls[0][1] + assert args == ( + "selected", 0x81 if directory else 0x80000000, + 0x3, None, 3, 0x00200000 | 0x02000000, None, + ) + assert calls[1:] == ([] if outcome == "open-failure" else [("close", 123)]) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index a04b9e0a34..83c694a3db 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -2,6 +2,7 @@ from dataclasses import FrozenInstanceError import errno +from contextlib import contextmanager import os from pathlib import Path, PureWindowsPath import subprocess @@ -385,6 +386,316 @@ def exchange(self, path, *, recursive): assert (tree / "nested").is_symlink() +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("kind", ["file", "directory", "recursive-file", "recursive-directory"]) +def test_delete_parent_exchange_at_removal(root, link, monkeypatch, allow, kind): + tree = root / "tree" + parent = tree / "parent" + parent.mkdir(parents=True) + outside = root.parent / "outside" + outside.mkdir() + victim = parent / "victim" + external = outside / "victim" + directory = kind in ("directory", "recursive-directory") + if directory: + victim.mkdir() + external.mkdir() + else: + external.write_text("keep") + victim.write_text("selected") + retained = root / "retained" + primitive = "rmdir" if directory else "unlink" + remove = getattr(os, primitive) + attempted = False + + def exchange(path, *args, **kwargs): + nonlocal attempted + if not attempted and (Path(path) == victim or str(path) == victim.name): + attempted = True + try: + parent.rename(retained) + except PermissionError: + if os.name != "nt": + raise + else: + link(parent, outside) + return remove(path, *args, **kwargs) + + monkeypatch.setattr(os, primitive, exchange) + try: + FileHelper(root, allow_symlinks=allow).delete( + tree if kind.startswith("recursive-") else victim, + recursive=kind.startswith("recursive-"), + ) + except (FileHelperError, OSError): + # A later recursive entry may reject the exchanged parent. + if not attempted: + raise + assert attempted + if directory: + assert external.is_dir() + else: + assert external.read_text() == "keep" + if retained.exists(): + assert not os.path.lexists(retained / "victim") + + +@pytest.mark.parametrize("allow", [False, True]) +def test_recursive_delete_parent_exchange_at_scan(root, link, monkeypatch, allow): + tree = root / "tree" + tree.mkdir() + (tree / "selected").write_text("selected") + outside = root.parent / "outside" + outside.mkdir() + (outside / "keep").write_text("keep") + retained = root / "retained" + scan = os.scandir + attempted = False + observed = [] + + def exchange(path): + nonlocal attempted + if not attempted: + attempted = True + try: + tree.rename(retained) + except PermissionError: + if os.name != "nt": + raise + else: + link(tree, outside) + entries = scan(path) + # Capture enumeration as well as removal: no outside entry is visited. + with entries: + children = list(entries) + observed.extend(entry.name for entry in children) + return scanned(children) + + @contextmanager + def scanned(entries): + yield iter(entries) + + monkeypatch.setattr(os, "scandir", exchange) + try: + FileHelper(root, allow_symlinks=allow).delete(tree, recursive=True) + except (FileHelperError, OSError): + if not attempted: + raise + assert attempted + assert observed == ["selected"] + assert (outside / "keep").read_text() == "keep" + + +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_link_parent_exchange_at_removal(root, link, monkeypatch, recursive): + parent = root / "parent" + parent.mkdir() + outside = root.parent / "outside" + outside.mkdir() + (outside / "victim").write_text("keep") + link(parent / "victim", outside / "victim") + retained = root / "retained" + unlink = os.unlink + attempted = False + + def exchange(path, *args, **kwargs): + nonlocal attempted + if not attempted: + attempted = True + try: + parent.rename(retained) + except PermissionError: + if os.name != "nt": + raise + else: + link(parent, outside) + return unlink(path, *args, **kwargs) + + monkeypatch.setattr(os, "unlink", exchange) + FileHelper(root, allow_symlinks=True).delete(parent / "victim", recursive=recursive) + assert attempted + assert (outside / "victim").read_text() == "keep" + assert not os.path.lexists((retained if retained.exists() else parent) / "victim") + + +@pytest.mark.parametrize("allow", [False, True]) +def test_delete_exchange_after_final_validation(root, link, monkeypatch, allow): + parent = root / "parent" + parent.mkdir() + (parent / "victim").write_text("selected") + outside = root.parent / "outside" + outside.mkdir() + (outside / "victim").write_text("keep") + retained = root / "retained" + check = FileHelper._path + checks = 0 + + def exchange(self, path, **kwargs): + nonlocal checks + result = check(self, path, **kwargs) + checks += 1 + if checks == 2: + parent.rename(retained) + link(parent, outside) + return result + + monkeypatch.setattr(FileHelper, "_path", exchange) + with pytest.raises(OSError): + FileHelper(root, allow_symlinks=allow).delete(parent / "victim") + assert checks == 2 + assert (outside / "victim").read_text() == "keep" + assert (retained / "victim").read_text() == "selected" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor traversal") +@pytest.mark.parametrize("allow", [False, True]) +def test_delete_exchange_during_parent_open(root, link, monkeypatch, allow): + parent = root / "parent" + parent.mkdir() + (parent / "victim").write_text("selected") + outside = root.parent / "outside" + outside.mkdir() + (outside / "victim").write_text("keep") + retained = root / "retained" + open_directory = os.open + + def exchange(path, flags, *args, **kwargs): + if path == "parent": + parent.rename(retained) + link(parent, outside) + return open_directory(path, flags, *args, **kwargs) + + monkeypatch.setattr(os, "open", exchange) + with pytest.raises(OSError): + FileHelper(root, allow_symlinks=allow).delete(parent / "victim") + assert (outside / "victim").read_text() == "keep" + assert (retained / "victim").read_text() == "selected" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("failure", [False, True]) +def test_delete_closes_directory_descriptors(root, monkeypatch, failure): + parent = root / "parent" + parent.mkdir() + (parent / "victim").write_text("selected") + opened = [] + open_directory = os.open + + def observe(*args, **kwargs): + fd = open_directory(*args, **kwargs) + opened.append(fd) + return fd + + def denied(*args, **kwargs): + raise PermissionError("removal denied") + + monkeypatch.setattr(os, "open", observe) + if failure: + monkeypatch.setattr(os, "unlink", denied) + with pytest.raises(PermissionError, match="removal denied"): + FileHelper(root).delete(parent / "victim") + assert (parent / "victim").read_text() == "selected" + else: + FileHelper(root).delete(parent / "victim") + assert not (parent / "victim").exists() + assert len(opened) == 2 + for fd in opened: + with pytest.raises(OSError) as exc: + os.fstat(fd) + assert exc.value.errno == errno.EBADF + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX capability check") +def test_delete_unsupported_backend_does_not_remove(root, monkeypatch): + (root / "victim").write_text("keep") + monkeypatch.setattr("specify_cli.file_helper._DELETE_DIR_FD_SUPPORTED", False) + with pytest.raises(OSError) as exc: + FileHelper(root).delete("victim") + assert exc.value.errno == errno.ENOTSUP + assert (root / "victim").read_text() == "keep" + + +@pytest.mark.parametrize("failure", [False, True]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_windows_delete_holds_ancestors_until_io_completes_portably( + root, link, monkeypatch, failure, recursive +): + import specify_cli.file_helper as module + + parent = root / "target/nested" + parent.mkdir(parents=True) + (parent / "victim").write_text("selected") + alias = link(root / "alias", root / "target") + active = [] + events = [] + unlink = os.unlink + rmdir = os.rmdir + scan = os.scandir + + @contextmanager + def handle(path, *, directory): + assert directory + active.append(path) + try: + yield 123 + finally: + assert active.pop() == path + + def assert_anchored(path): + path = Path(path) + assert active[0] == root.resolve() + assert active[-1] == path + assert all(not part.is_symlink() for part in active) + events.append(path) + + def observe_scan(path): + assert_anchored(path) + return scan(path) + + def remove(path, *, dir_fd=None): + assert dir_fd is None + assert_anchored(Path(path).parent) + if failure: + raise PermissionError("removal denied") + unlink(path) + + def remove_directory(path, *, dir_fd=None): + assert dir_fd is None + assert_anchored(Path(path).parent) + rmdir(path) + + backend = SimpleNamespace(**vars(os)) + backend.name = "nt" + backend.unlink = remove + backend.rmdir = remove_directory + backend.scandir = observe_scan + monkeypatch.setattr(module, "os", backend) + monkeypatch.setattr(module, "_windows_handle", handle) + files = FileHelper(root, allow_symlinks=True) + selected = alias / "nested" if recursive else alias / "nested/victim" + if failure: + with pytest.raises(PermissionError, match="removal denied"): + files.delete(selected, recursive=recursive) + assert (parent / "victim").read_text() == "selected" + else: + files.delete(selected, recursive=recursive) + assert not (parent / "victim").exists() + assert events + assert active == [] + assert alias.is_symlink() + + +@pytest.mark.parametrize("allow", [False, True]) +def test_delete_through_trusted_root_alias(root, link, allow): + alias = link(root.parent / "alias", root) + (root / "tree/nested").mkdir(parents=True) + (root / "tree/nested/file").write_text("selected") + FileHelper(alias, allow_symlinks=allow).delete(alias / "tree", recursive=True) + assert alias.is_symlink() + assert root.is_dir() + assert not (root / "tree").exists() + + @pytest.mark.parametrize("allow", [False, True]) def test_unrelated_symlink_is_outside_operation_scope(root, link, allow): link(root / "unrelated", root.parent / "missing") From 12046535cc7809d35874a7bd8d0d4bffa8ed17ce Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:14:49 -0600 Subject: [PATCH 02/16] fix: retain deletion roots and preserve directory permissions Reuse one identity-checked root anchor throughout deletion. Use search-only POSIX descriptors for traversal and request read access only for recursive enumeration. Keep Windows canonical ancestors held for the operation. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 20 +++- src/specify_cli/file_helper.py | 100 +++++++++++----- tests/test_file_helper.py | 202 ++++++++++++++++++++++++++++++++- 3 files changed, 285 insertions(+), 37 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 3658ff45dc..5bfff91db0 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -182,11 +182,22 @@ helpers for independent project/source/managed boundaries. - `delete(path, recursive=False)` unlinks files/links or removes directories. Nonrecursive directory deletion requires an empty directory. The established root cannot be deleted. Recursive enumeration and each removal use anchored - directories: POSIX uses no-follow directory descriptors and descriptor-relative - `unlink`/`rmdir`; Windows holds the resolved ancestors without delete sharing. + directories. Each deletion acquires its root once, checks that it matches the + root identity recorded at construction, and retains that anchor through + preflight and all removals. POSIX uses no-follow directory descriptors and + descriptor-relative `unlink`/`rmdir`; Windows holds the full canonical ancestry, + including ancestors above the root, without delete sharing while using absolute + paths. This does not change the trusted treatment of aliases resolved at root + establishment. Allowed contained parent links are resolved before anchoring, and leaf links are still unlinked rather than followed. Platforms lacking the required directory-descriptor operations fail with `ENOTSUP`, not pathname-only deletion. + POSIX traversal and removal use search/path-only descriptors (`O_SEARCH` or + `O_PATH`) where available, so known entries in write-and-execute-only parents + can still be removed. Read-capable descriptors are requested only for directories + being enumerated; systems without search/path-only flags retain read-capable + traversal. Recursive enumeration still requires directory read permission, and + removal still requires the operating system's normal directory permissions. - `symlink(target, path)` creates a link exclusively. Relative targets are interpreted relative to the link parent, not the helper root. @@ -216,8 +227,9 @@ 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. Deletion also anchors directory enumeration and removal so exchanging an accessed parent for a link cannot -redirect those operations. POSIX removal stays attached to the opened directory -even if that directory is renamed; Windows prevents ancestor renames while +redirect those operations. Reopening the absolute root pathname for each entry is +avoided; a replaced root is rejected before use. POSIX removal stays attached to +the opened directory even if that directory is renamed; Windows prevents ancestor renames while handles are held. Concurrent changes may still cause deletion to fail or leave partial results, and preflight is not a transaction or an immutable tree snapshot. Leaf removal acts on the current entry in the anchored parent, not a pinned leaf diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index dca7a82124..b774546ab9 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -58,14 +58,17 @@ class FileHelper: root: Path allow_symlinks: bool = False _canonical_root: Path = field(init=False, repr=False) + _root_stat: os.stat_result = field(init=False, repr=False, compare=False) def __post_init__(self) -> None: root = Path(self.root).absolute() canonical = root.resolve(strict=True) - if not canonical.is_dir(): + root_stat = canonical.stat() + if not stat.S_ISDIR(root_stat.st_mode): raise NotADirectoryError(root) object.__setattr__(self, "root", root) object.__setattr__(self, "_canonical_root", canonical) + object.__setattr__(self, "_root_stat", root_stat) def _parts(self, path: PurePath) -> tuple[str, ...]: original = path @@ -246,18 +249,25 @@ def symlink(self, target: Path | str, path: Path | str) -> None: resolved = self._walk(target_parts) destination.symlink_to(target, target_is_directory=resolved.is_dir()) + @staticmethod + def _deletion_flags() -> int: + access = getattr(os, "O_SEARCH", getattr(os, "O_PATH", os.O_RDONLY)) + return access | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + + def _check_deletion_root(self, current: os.stat_result) -> None: + if not os.path.samestat(self._root_stat, current): + raise FileHelperError(f"Established root changed during deletion: {self.root}") + @contextmanager - def _deletion_directory(self, path: Path) -> Iterator[int | Path]: - """Anchor a resolved directory without following replacement links.""" - parts = path.relative_to(self._canonical_root).parts + def _deletion_root(self) -> Iterator[int | Path]: + """Bind the whole deletion to the root identity established at construction.""" if os.name == "nt": with ExitStack() as stack: - current = self._canonical_root - stack.enter_context(_windows_handle(current, directory=True)) - for part in parts: - current = current / part + # Absolute Windows I/O also needs stable ancestors above the root. + for current in reversed([self._canonical_root, *self._canonical_root.parents]): stack.enter_context(_windows_handle(current, directory=True)) - yield current + self._check_deletion_root(self._canonical_root.stat()) + yield self._canonical_root return if not ( @@ -266,18 +276,47 @@ def _deletion_directory(self, path: Path) -> Iterator[int | Path]: and getattr(os, "O_DIRECTORY", 0) ): raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") - flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) - parent = os.open(self._canonical_root, flags) + root = os.open(self._canonical_root, self._deletion_flags()) + try: + self._check_deletion_root(os.fstat(root)) + yield root + finally: + os.close(root) + + @contextmanager + def _deletion_directory( + self, path: Path, *, root: int | Path, enumerate_entries: bool = False + ) -> Iterator[int | Path]: + """Traverse from the operation's root, requesting read access only for scans.""" + parts = path.relative_to(self._canonical_root).parts + if isinstance(root, Path): + with ExitStack() as stack: + current = root + for part in parts: + current = current / part + stack.enter_context(_windows_handle(current, directory=True)) + yield current + return + + flags = self._deletion_flags() + parent = os.dup(root) try: for part in parts: child = os.open(part, flags, dir_fd=parent) os.close(parent) parent = child + if enumerate_entries: + read_flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + child = os.open(".", read_flags, dir_fd=parent) + os.close(parent) + parent = child yield parent finally: os.close(parent) - def _deletion_plan(self, path: Path, *, recursive: bool) -> list[tuple[Path, bool]]: + def _deletion_plan( + self, path: Path, *, recursive: bool, root: int | Path + ) -> list[tuple[Path, bool]]: mode = self._entry_mode(path) if stat.S_ISLNK(mode): if not self.allow_symlinks: @@ -289,29 +328,30 @@ def _deletion_plan(self, path: Path, *, recursive: bool) -> list[tuple[Path, boo raise UnsupportedPathError(f"Unsupported filesystem object: {path}") plan: list[tuple[Path, bool]] = [] if recursive: - with self._deletion_directory(path) as directory: + with self._deletion_directory(path, root=root, enumerate_entries=True) as directory: with os.scandir(directory) as entries: children = sorted(path / entry.name for entry in entries) for child in children: - plan.extend(self._deletion_plan(child, recursive=True)) + plan.extend(self._deletion_plan(child, recursive=True, root=root)) 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}") - with self._deletion_directory(entry.parent) as parent: - leaf = parent / entry.name if isinstance(parent, Path) else entry.name - directory_fd = None if isinstance(parent, Path) else parent - if directory: - os.rmdir(leaf, dir_fd=directory_fd) - else: - os.unlink(leaf, dir_fd=directory_fd) + with self._deletion_root() as root: + 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, root=root) + 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}") + with self._deletion_directory(entry.parent, root=root) as parent: + leaf = parent / entry.name if isinstance(parent, Path) else entry.name + directory_fd = None if isinstance(parent, Path) else parent + if directory: + os.rmdir(leaf, dir_fd=directory_fd) + else: + os.unlink(leaf, dir_fd=directory_fd) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 83c694a3db..fde48acc43 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -372,8 +372,8 @@ def test_deletion_rechecks_parents_after_preflight(root, link, monkeypatch): (tree / "nested/file").write_text("keep") plan = FileHelper._deletion_plan - def exchange(self, path, *, recursive): - result = plan(self, path, recursive=recursive) + def exchange(self, path, *, recursive, **kwargs): + result = plan(self, path, recursive=recursive, **kwargs) if path == tree: (tree / "nested").rename(root / "retained") link(tree / "nested", root / "retained") @@ -615,6 +615,201 @@ def test_delete_unsupported_backend_does_not_remove(root, monkeypatch): assert (root / "victim").read_text() == "keep" +@pytest.mark.skipif(os.name == "nt", reason="POSIX root descriptor acquisition") +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_rejects_root_ancestor_exchange_at_open(root, link, monkeypatch, allow, recursive): + ancestor = root / "ancestor" + project = ancestor / "project" + project.mkdir(parents=True) + (project / "tree").mkdir() + (project / "tree/victim").write_text("selected") + outside = root / "outside" + (outside / "project/tree").mkdir(parents=True) + (outside / "project/tree/victim").write_text("keep") + files = FileHelper(project, allow_symlinks=allow) + retained = root / "retained" + open_directory = os.open + attempted = False + opened = [] + + def exchange(path, flags, *args, **kwargs): + nonlocal attempted + if not attempted and path == project: + attempted = True + ancestor.rename(retained) + link(ancestor, outside) + fd = open_directory(path, flags, *args, **kwargs) + opened.append(fd) + return fd + + monkeypatch.setattr(os, "open", exchange) + with pytest.raises(FileHelperError, match="root changed"): + files.delete("tree" if recursive else "tree/victim", recursive=recursive) + assert attempted + assert (outside / "project/tree/victim").read_text() == "keep" + assert (retained / "project/tree/victim").read_text() == "selected" + assert len(opened) == 1 + with pytest.raises(OSError) as exc: + os.fstat(opened[0]) + assert exc.value.errno == errno.EBADF + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX root descriptor lifetime") +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_reuses_root_after_ancestor_exchange(root, link, monkeypatch, allow, recursive): + ancestor = root / "ancestor" + project = ancestor / "project" + project.mkdir(parents=True) + (project / "tree").mkdir() + (project / "tree/victim").write_text("selected") + outside = root / "outside" + (outside / "project/tree").mkdir(parents=True) + (outside / "project/tree/victim").write_text("keep") + files = FileHelper(project, allow_symlinks=allow) + retained = root / "retained" + check = FileHelper._path + attempted = False + + def exchange(self, path, **kwargs): + nonlocal attempted + result = check(self, path, **kwargs) + if not attempted: + attempted = True + ancestor.rename(retained) + link(ancestor, outside) + return result + + monkeypatch.setattr(FileHelper, "_path", exchange) + files.delete("tree" if recursive else "tree/victim", recursive=recursive) + assert attempted + assert (outside / "project/tree/victim").read_text() == "keep" + assert not (retained / "project/tree/victim").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") +@pytest.mark.parametrize("parent_is_root", [False, True]) +@pytest.mark.parametrize("kind", ["file", "empty-directory", "recursive-tree"]) +def test_delete_through_write_execute_only_parent(root, parent_is_root, kind): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory read permissions") + parent = root if parent_is_root else root / "parent" + parent.mkdir(exist_ok=True) + control = parent / "control" + control.write_text("control") + victim = parent / "victim" + if kind == "file": + victim.write_text("selected") + else: + victim.mkdir() + if kind == "recursive-tree": + (victim / "child").write_text("selected") + files = FileHelper(root) + parent.chmod(0o300) + try: + control.unlink() + files.delete(victim, recursive=kind == "recursive-tree") + assert not victim.exists() + finally: + parent.chmod(0o700) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") +def test_recursive_delete_still_requires_read_permission_for_enumeration(root): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory read permissions") + tree = root / "tree" + tree.mkdir() + (tree / "victim").write_text("keep") + tree.chmod(0o300) + try: + with pytest.raises(PermissionError): + FileHelper(root).delete(tree, recursive=True) + assert (tree / "victim").read_text() == "keep" + finally: + tree.chmod(0o700) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") +def test_delete_still_requires_write_permission_on_parent(root): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory write permissions") + parent = root / "parent" + parent.mkdir() + (parent / "victim").write_text("keep") + parent.chmod(0o100) + try: + with pytest.raises(PermissionError): + FileHelper(root).delete(parent / "victim") + assert (parent / "victim").read_text() == "keep" + finally: + parent.chmod(0o700) + + +@pytest.mark.parametrize("allow", [False, True]) +def test_delete_rejects_replaced_root_before_validation(root, monkeypatch, allow): + (root / "victim").write_text("selected") + files = FileHelper(root, allow_symlinks=allow) + retained = root.parent / "retained" + root.rename(retained) + root.mkdir() + (root / "victim").write_text("keep") + + def unexpected(*args, **kwargs): + pytest.fail("A replacement root must be rejected before path validation") + + monkeypatch.setattr(FileHelper, "_path", unexpected) + with pytest.raises(FileHelperError, match="root changed"): + files.delete("victim") + assert (root / "victim").read_text() == "keep" + assert (retained / "victim").read_text() == "selected" + + +def test_windows_delete_rejects_replaced_root_during_acquisition_portably( + root, link, monkeypatch +): + import specify_cli.file_helper as module + + ancestor = root / "ancestor" + project = ancestor / "project" + project.mkdir(parents=True) + (project / "victim").write_text("selected") + outside = root / "outside" + (outside / "project").mkdir(parents=True) + (outside / "project/victim").write_text("keep") + files = FileHelper(project) + retained = root / "retained" + active = [] + attempted = False + + @contextmanager + def handle(path, *, directory): + nonlocal attempted + assert directory + active.append(path) + try: + if path == project: + # Simulate a replacement just before the root handle is acquired. + attempted = True + ancestor.rename(retained) + link(ancestor, outside) + yield 123 + finally: + assert active.pop() == path + + backend = SimpleNamespace(**vars(os)) + backend.name = "nt" + monkeypatch.setattr(module, "os", backend) + monkeypatch.setattr(module, "_windows_handle", handle) + with pytest.raises(FileHelperError, match="root changed"): + files.delete("victim") + assert attempted + assert active == [] + assert (outside / "project/victim").read_text() == "keep" + assert (retained / "project/victim").read_text() == "selected" + + @pytest.mark.parametrize("failure", [False, True]) @pytest.mark.parametrize("recursive", [False, True]) def test_windows_delete_holds_ancestors_until_io_completes_portably( @@ -643,7 +838,8 @@ def handle(path, *, directory): def assert_anchored(path): path = Path(path) - assert active[0] == root.resolve() + assert active[0] == Path(root.anchor) + assert root.resolve() in active assert active[-1] == path assert all(not part.is_symlink() for part in active) events.append(path) From aec2fa56cfe17493d9b3b47480fa028fa699e1a7 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:52:47 -0600 Subject: [PATCH 03/16] fix: resolve deletion paths through the retained root Use descriptor-relative metadata and link reads in the existing path walker for POSIX deletion. Keep initial validation, recursive classification, and per-entry checks on the same retained directory tree as removal, while preserving supported symlink behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 24 ++-- src/specify_cli/file_helper.py | 52 ++++++-- tests/test_file_helper.py | 221 ++++++++++++++++++++++++++++++++- 3 files changed, 269 insertions(+), 28 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 5bfff91db0..67c8daa932 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -184,14 +184,17 @@ helpers for independent project/source/managed boundaries. root cannot be deleted. Recursive enumeration and each removal use anchored directories. Each deletion acquires its root once, checks that it matches the root identity recorded at construction, and retains that anchor through - preflight and all removals. POSIX uses no-follow directory descriptors and - descriptor-relative `unlink`/`rmdir`; Windows holds the full canonical ancestry, - including ancestors above the root, without delete sharing while using absolute - paths. This does not change the trusted treatment of aliases resolved at root - establishment. - Allowed contained parent links are resolved before anchoring, and leaf links - are still unlinked rather than followed. Platforms lacking the required - directory-descriptor operations fail with `ENOTSUP`, not pathname-only deletion. + preflight and all removals. POSIX uses this retained root for component + classification, link-target reads and resolution, recursive preflight, and + per-entry revalidation, as well as descriptor-relative enumeration and + `unlink`/`rmdir`. Canonical pathnames identify root-relative entries but are not + reopened for metadata or link resolution. Windows holds the full canonical + ancestry, including ancestors above the root, without delete sharing while using + absolute paths. This does not change the trusted treatment of aliases resolved + at root establishment. Allowed contained parent links are resolved within the + retained boundary, and leaf links are still unlinked rather than followed. + Platforms lacking the required directory-descriptor operations fail with + `ENOTSUP`, not pathname-only deletion. POSIX traversal and removal use search/path-only descriptors (`O_SEARCH` or `O_PATH`) where available, so known entries in write-and-execute-only parents can still be removed. Read-capable descriptors are requested only for directories @@ -229,8 +232,9 @@ deletion rechecks each accessed entry. Deletion also anchors directory enumeration and removal so exchanging an accessed parent for a link cannot redirect those operations. Reopening the absolute root pathname for each entry is avoided; a replaced root is rejected before use. POSIX removal stays attached to -the opened directory even if that directory is renamed; Windows prevents ancestor renames while -handles are held. Concurrent changes may still cause deletion to fail or leave +the opened directory even if that directory is renamed; validation continues to +inspect that same directory tree. Windows prevents ancestor renames while handles +are held. Concurrent changes may still cause deletion to fail or leave partial results, and preflight is not a transaction or an immutable tree snapshot. Leaf removal acts on the current entry in the anchored parent, not a pinned leaf identity. Other mutations retain pathname-based checks and are **not** a race-free diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index b774546ab9..1176a807a7 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -16,7 +16,8 @@ _PathType = TypeVar("_PathType", bound=PurePath) _DELETE_DIR_FD_SUPPORTED = ( - {os.open, os.unlink, os.rmdir} <= os.supports_dir_fd + {os.open, os.stat, os.readlink, os.unlink, os.rmdir} <= os.supports_dir_fd + and os.stat in os.supports_follow_symlinks and os.scandir in os.supports_fd ) @@ -88,8 +89,11 @@ def _parts(self, path: PurePath) -> tuple[str, ...]: raise PathEscapeError(f"Path is outside root {self.root}: {original}") @staticmethod - def _entry_mode(path: Path) -> int: - entry = path.lstat() + def _entry_mode(path: Path, *, dir_fd: int | None = None) -> int: + entry = ( + path.lstat() if dir_fd is None + else os.stat(path.name, dir_fd=dir_fd, follow_symlinks=False) + ) if ( getattr(entry, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT and not stat.S_ISLNK(entry.st_mode) @@ -106,10 +110,15 @@ def _walk( allow_missing: bool = False, follow_leaf: bool = True, links: tuple[Path, ...] = (), + root_fd: int | None = None, ) -> Path: current = self._canonical_root for index, part in enumerate(parts): - if not current.is_dir(): + if root_fd is not None and part == "..": + # Do not let ".." skip a non-directory link-target component. + with self._deletion_directory(current, root=root_fd): + pass + elif root_fd is None and not current.is_dir(): if not (allow_missing and not current.exists()): raise NotADirectoryError(current) if part == "..": @@ -119,7 +128,10 @@ def _walk( continue candidate = current / part try: - mode = self._entry_mode(candidate) + mode = ( + self._entry_mode(candidate) if root_fd is None + else self._deletion_entry_mode(candidate, root=root_fd) + ) except FileNotFoundError: if not allow_missing: raise @@ -135,12 +147,20 @@ def _walk( return candidate if candidate in links or len(links) >= 40: raise UnsupportedPathError(f"Symlink cycle or excessive chain: {candidate}") - target = Path(os.readlink(candidate)) + if root_fd is None: + target = Path(os.readlink(candidate)) + else: + with self._deletion_directory(current, root=root_fd) as directory: + leaf = directory / part if isinstance(directory, Path) else part + directory_fd = None if isinstance(directory, Path) else directory + target = Path(os.readlink(leaf, dir_fd=directory_fd)) 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,)) + current = self._walk( + target_parts, links=links + (candidate,), root_fd=root_fd + ) elif stat.S_ISDIR(mode) or stat.S_ISREG(mode): current = candidate else: @@ -148,14 +168,15 @@ def _walk( return current def _path( - self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True + self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True, + root_fd: int | None = None, ) -> Path: original = Path(path) parts = self._parts(original) if ".." in parts: raise PathEscapeError(f"Parent traversal is not an operation path: {original}") return self._walk( - parts, allow_missing=allow_missing, follow_leaf=follow_leaf + parts, allow_missing=allow_missing, follow_leaf=follow_leaf, root_fd=root_fd ) @staticmethod @@ -314,10 +335,16 @@ def _deletion_directory( finally: os.close(parent) + def _deletion_entry_mode(self, path: Path, *, root: int | Path) -> int: + with self._deletion_directory(path.parent, root=root) as directory: + if isinstance(directory, Path): + return self._entry_mode(directory / path.name) + return self._entry_mode(path, dir_fd=directory) + def _deletion_plan( self, path: Path, *, recursive: bool, root: int | Path ) -> list[tuple[Path, bool]]: - mode = self._entry_mode(path) + mode = self._deletion_entry_mode(path, root=root) if stat.S_ISLNK(mode): if not self.allow_symlinks: raise SymlinkDeniedError(f"Refusing to delete symlink: {path}") @@ -339,13 +366,14 @@ def _deletion_plan( def delete(self, path: Path | str, *, recursive: bool = False) -> None: """Delete an entry; recursive deletion never follows descendant links.""" with self._deletion_root() as root: - destination = self._path(path, follow_leaf=False) + root_fd = None if isinstance(root, Path) else root + destination = self._path(path, follow_leaf=False, root_fd=root_fd) if destination == self._canonical_root: raise FileHelperError(f"Refusing to delete the established root: {self.root}") plan = self._deletion_plan(destination, recursive=recursive, root=root) for entry, directory in plan: # Recheck accessed parents/leaf before each mutation, not just preflight. - checked = self._path(entry, follow_leaf=False) + checked = self._path(entry, follow_leaf=False, root_fd=root_fd) if checked != entry: raise FileHelperError(f"Destination changed during deletion: {entry}") with self._deletion_directory(entry.parent, root=root) as parent: diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index fde48acc43..092928d67e 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -579,17 +579,27 @@ def test_delete_closes_directory_descriptors(root, monkeypatch, failure): parent.mkdir() (parent / "victim").write_text("selected") opened = [] + root_opens = [] open_directory = os.open + duplicate = os.dup def observe(*args, **kwargs): fd = open_directory(*args, **kwargs) opened.append(fd) + if args[0] == root.resolve(): + root_opens.append(fd) return fd + def observe_duplicate(fd): + result = duplicate(fd) + opened.append(result) + return result + def denied(*args, **kwargs): raise PermissionError("removal denied") monkeypatch.setattr(os, "open", observe) + monkeypatch.setattr(os, "dup", observe_duplicate) if failure: monkeypatch.setattr(os, "unlink", denied) with pytest.raises(PermissionError, match="removal denied"): @@ -598,7 +608,8 @@ def denied(*args, **kwargs): else: FileHelper(root).delete(parent / "victim") assert not (parent / "victim").exists() - assert len(opened) == 2 + assert len(root_opens) == 1 + assert len(opened) >= 2 for fd in opened: with pytest.raises(OSError) as exc: os.fstat(fd) @@ -688,6 +699,193 @@ def exchange(self, path, **kwargs): assert not (retained / "project/tree/victim").exists() +@pytest.mark.skipif(os.name == "nt", reason="POSIX retained-root path resolution") +@pytest.mark.parametrize("kind", ["file", "directory", "recursive-tree"]) +@pytest.mark.parametrize("target", ["relative", "absolute", "chain", "parent-traversal"]) +def test_delete_resolves_links_in_retained_root(root, link, monkeypatch, kind, target): + ancestor = root / "ancestor" + project = ancestor / "project" + outside = root / "outside/project" + for boundary in (project, outside): + for name in ("selected", "unrelated"): + directory = boundary / name + directory.mkdir(parents=True) + victim = directory / "victim" + if kind == "file": + victim.write_text("keep") + else: + victim.mkdir() + if kind == "recursive-tree": + (victim / "child").write_text("keep") + for boundary, selected in ((project, "selected"), (outside, "unrelated")): + if target == "absolute": + destination = boundary / selected + elif target == "chain": + link(boundary / "first", selected) + destination = "first" + elif target == "parent-traversal": + (boundary / "nested").mkdir() + destination = f"nested/../{selected}" + else: + destination = selected + link(boundary / "alias", destination) + files = FileHelper(project, allow_symlinks=True) + retained = root / "retained" + acquire = FileHelper._deletion_root + + @contextmanager + def exchange(self): + with acquire(self) as handle: + ancestor.rename(retained) + link(ancestor, outside.parent) + yield handle + + monkeypatch.setattr(FileHelper, "_deletion_root", exchange) + files.delete("alias/victim", recursive=kind == "recursive-tree") + assert not (retained / "project/selected/victim").exists() + assert (retained / "project/unrelated/victim").exists() + assert (retained / "project/alias").is_symlink() + assert (outside / "selected/victim").exists() + assert (outside / "unrelated/victim").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX retained-root preflight") +def test_delete_classifies_descendants_in_retained_root(root, link, monkeypatch): + ancestor = root / "ancestor" + project = ancestor / "project" + outside = root / "outside/project" + for boundary in (project, outside): + (boundary / "tree").mkdir(parents=True) + (boundary / "tree/a-file").write_text("keep") + link(project / "tree/z-link", root / "missing") + (outside / "tree/z-link").write_text("replacement") + files = FileHelper(project) + retained = root / "retained" + acquire = FileHelper._deletion_root + + @contextmanager + def exchange(self): + with acquire(self) as handle: + ancestor.rename(retained) + link(ancestor, outside.parent) + yield handle + + monkeypatch.setattr(FileHelper, "_deletion_root", exchange) + with pytest.raises(SymlinkDeniedError): + files.delete("tree", recursive=True) + assert (retained / "project/tree/a-file").read_text() == "keep" + assert (retained / "project/tree/z-link").is_symlink() + assert (outside / "tree/z-link").read_text() == "replacement" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor-relative metadata") +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_metadata_and_links_use_retained_descriptors(root, link, monkeypatch, recursive): + (root / "selected/tree").mkdir(parents=True) + (root / "selected/tree/victim").write_text("selected") + link(root / "alias", "selected") + files = FileHelper(root, allow_symlinks=True) + acquire = FileHelper._deletion_root + stat_entry = os.stat + readlink = os.readlink + calls = [] + + def relative_stat(path, *args, **kwargs): + assert isinstance(kwargs.get("dir_fd"), int) + assert kwargs.get("follow_symlinks") is False + assert not Path(path).is_absolute() + calls.append("stat") + return stat_entry(path, *args, **kwargs) + + def relative_readlink(path, *args, **kwargs): + assert isinstance(kwargs.get("dir_fd"), int) + assert not Path(path).is_absolute() + calls.append("readlink") + return readlink(path, *args, **kwargs) + + def no_path_metadata(*args, **kwargs): + pytest.fail("Deletion metadata must not reopen an absolute pathname") + + @contextmanager + def observe(self): + with acquire(self) as handle, monkeypatch.context() as scoped: + scoped.setattr(Path, "lstat", no_path_metadata) + scoped.setattr(os, "stat", relative_stat) + scoped.setattr(os, "readlink", relative_readlink) + yield handle + + monkeypatch.setattr(FileHelper, "_deletion_root", observe) + files.delete("alias/tree" if recursive else "alias/tree/victim", recursive=recursive) + assert "stat" in calls and "readlink" in calls + assert not (root / "selected/tree/victim").exists() + assert (root / "alias").is_symlink() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX retained-root revalidation") +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_revalidates_entries_in_retained_root(root, link, monkeypatch, recursive): + ancestor = root / "ancestor" + project = ancestor / "project" + (project / "selected").mkdir(parents=True) + (project / "selected/victim").write_text("selected") + outside = root / "outside/project" + outside.mkdir(parents=True) + link(outside / "selected", root / "missing") + files = FileHelper(project) + retained = root / "retained" + plan = FileHelper._deletion_plan + selected = project / ("selected" if recursive else "selected/victim") + attempted = False + + def exchange(self, path, **kwargs): + nonlocal attempted + result = plan(self, path, **kwargs) + if path == selected: + attempted = True + ancestor.rename(retained) + link(ancestor, outside.parent) + return result + + monkeypatch.setattr(FileHelper, "_deletion_plan", exchange) + files.delete(selected, recursive=recursive) + assert attempted + assert not (retained / "project/selected/victim").exists() + assert (outside / "selected").is_symlink() + + +@pytest.mark.parametrize( + ("kind", "error"), + [ + ("dangling", FileNotFoundError), + ("cycle", UnsupportedPathError), + ("excessive", UnsupportedPathError), + ("non-directory", NotADirectoryError), + ("escape-and-return", PathEscapeError), + ], +) +def test_delete_parent_link_targets_preserve_failure_contract(root, link, kind, error): + (root / "target").mkdir() + (root / "target/victim").write_text("keep") + if kind == "dangling": + target = "missing" + elif kind == "cycle": + target = "alias" + elif kind == "excessive": + for index in reversed(range(41)): + link(root / f"chain-{index}", "target" if index == 40 else f"chain-{index + 1}") + target = "chain-0" + elif kind == "non-directory": + (root / "file").write_text("keep") + target = "file/../target" + else: + target = f"../{root.name}/target" + link(root / "alias", target) + with pytest.raises(error): + FileHelper(root, allow_symlinks=True).delete("alias/victim") + assert (root / "target/victim").read_text() == "keep" + assert (root / "alias").is_symlink() + + @pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") @pytest.mark.parametrize("parent_is_root", [False, True]) @pytest.mark.parametrize("kind", ["file", "empty-directory", "recursive-tree"]) @@ -1453,18 +1651,29 @@ def test_directory_reparse_entries_rejected_before_traversal_portably( junction.mkdir() (junction / "keep").write_text("keep") lstat = Path.lstat + stat_entry = os.stat + + def as_reparse(result): + return SimpleNamespace( + st_mode=result.st_mode, + st_file_attributes=0x400, + st_reparse_tag=0xA0000003, + ) 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 as_reparse(result) + return result + + def directory_reparse_stat(path, *args, **kwargs): + result = stat_entry(path, *args, **kwargs) + if kwargs.get("dir_fd") is not None and Path(path).name == junction.name: + return as_reparse(result) return result monkeypatch.setattr(Path, "lstat", directory_reparse) + monkeypatch.setattr(os, "stat", directory_reparse_stat) files = FileHelper(root, allow_symlinks=allow) for operation in ( lambda: files.read_bytes(junction / "keep"), From d631ce1c1fe938a41f8982103e0c4394d38a19a3 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 07:58:05 -0600 Subject: [PATCH 04/16] fix: retain root handles and bound deletion traversal Keep helper roots pinned until explicit close or context exit, and close bootstrap consumers deterministically. Carry parent descriptors through component resolution and both recursive deletion passes. Cover root cleanup, bounded directory opens, and POSIX backends without search-only flags. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 48 ++++-- src/specify_cli/_file_policy.py | 47 ++--- src/specify_cli/file_helper.py | 245 ++++++++++++++++----------- src/specify_cli/integration_state.py | 87 +++++----- tests/test_file_helper.py | 167 ++++++++++++++++-- 5 files changed, 409 insertions(+), 185 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 67c8daa932..7eeeced0f3 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -67,7 +67,9 @@ the policy. `specify_cli._project.resolve_specify_project_files()` performs the normal existing-project resolution and returns the same handoff for new CLI adapters. Commands using another established root call `project_files` on their actually selected root. Retain a helper only for the current operation; -a later invocation resolves a new helper, so manual edits take effect. +a later invocation resolves a new helper, so manual edits take effect. Use the +returned owner as a context manager, or call `close()` in the owner's cleanup +path, to release retained root handles at that operation boundary. The `specify_cli.integration_state` helpers are an explicit managed-metadata exception to this handoff. Their reads and writes of `.specify/integration.json` @@ -101,12 +103,12 @@ existing lifecycle rollback, including prior policy and unrelated options. ```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) +with FileHelper(project_root) as files: # 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 @@ -144,6 +146,19 @@ 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. +On supported deletion backends, construction pins the canonical root with an +open directory handle. That handle remains owned by the helper until `close()` +or context-manager exit. Keeping the original directory open also prevents its +identity from being reused for a different directory while the helper is alive; +stored device/inode numbers alone are not the binding. Windows additionally +retains the canonical ancestors without delete sharing. Close the helper before +external code renames or removes that root or its ancestors. Automatic +finalization is a compatibility fallback, not a substitute for deterministic +cleanup. Bootstrap configuration and managed integration-state consumers close +their own helpers explicitly; consumers receiving `project_files()` own its +lifetime. Pinning the root does not make other pathname-based operations +descriptor-relative or change their documented concurrency limitations. + | Operation | Default deny | Explicit allow | |---|---|---| | Read file | Reject accessed parent or leaf links | Follow only existing contained regular-file targets | @@ -157,6 +172,9 @@ helpers for independent project/source/managed boundaries. ## API and failures +- `close()` releases the helper's retained root handles and is idempotent. + `with FileHelper(...) as files:` closes on both success and failure. Operations + on an explicitly closed helper raise `FileHelperError`. - `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 @@ -182,8 +200,8 @@ helpers for independent project/source/managed boundaries. - `delete(path, recursive=False)` unlinks files/links or removes directories. Nonrecursive directory deletion requires an empty directory. The established root cannot be deleted. Recursive enumeration and each removal use anchored - directories. Each deletion acquires its root once, checks that it matches the - root identity recorded at construction, and retains that anchor through + directories. Each deletion borrows the root handle retained at construction, + checks the currently named root for replacement, and uses that original anchor through preflight and all removals. POSIX uses this retained root for component classification, link-target reads and resolution, recursive preflight, and per-entry revalidation, as well as descriptor-relative enumeration and @@ -201,6 +219,15 @@ helpers for independent project/source/managed boundaries. being enumerated; systems without search/path-only flags retain read-capable traversal. Recursive enumeration still requires directory read permission, and removal still requires the operating system's normal directory permissions. + On systems using the read-capable fallback, root establishment also requires + read permission on the root. + Component resolution retains opened parent descriptors instead of reopening + every prefix. Recursive preflight and mutation carry each parent descriptor + through its children, opening directories a bounded number of times across + the two passes. Descriptor-open counts grow linearly with visited directories + and operation-path components; recursive descriptor lifetimes grow with tree + depth rather than total tree size. The complete plan is still preflighted + before any removal. - `symlink(target, path)` creates a link exclusively. Relative targets are interpreted relative to the link parent, not the helper root. @@ -231,7 +258,8 @@ creation. Atomic writes recheck the original path before replacement, and deletion rechecks each accessed entry. Deletion also anchors directory enumeration and removal so exchanging an accessed parent for a link cannot redirect those operations. Reopening the absolute root pathname for each entry is -avoided; a replaced root is rejected before use. POSIX removal stays attached to +avoided; the established root handle is never rebound to a replacement, even if +device/inode values are reused. POSIX removal stays attached to the opened directory even if that directory is renamed; validation continues to inspect that same directory tree. Windows prevents ancestor renames while handles are held. Concurrent changes may still cause deletion to fail or leave diff --git a/src/specify_cli/_file_policy.py b/src/specify_cli/_file_policy.py index e675c9b614..1adae7c28e 100644 --- a/src/specify_cli/_file_policy.py +++ b/src/specify_cli/_file_policy.py @@ -33,12 +33,13 @@ def read_project_options(project_root: Path) -> dict[str, Any]: """Read without following configuration links; only absence defaults.""" try: files = FileHelper(project_root) - except (OSError, RuntimeError) as exc: + except (OSError, FileHelperError, RuntimeError) as exc: raise ProjectPolicyError( f"Cannot establish project root for {INIT_OPTIONS_FILE}: {exc}" ) from exc try: - text = files.read_text(INIT_OPTIONS_FILE) + with files: + text = files.read_text(INIT_OPTIONS_FILE) except FileNotFoundError: return {} except (OSError, UnicodeError, FileHelperError) as exc: @@ -60,7 +61,7 @@ def project_files(project_root: Path) -> FileHelper: options = read_project_options(project_root) try: return FileHelper(project_root, allow_symlinks=options.get("allow_symlinks", False)) - except (OSError, RuntimeError) as exc: + except (OSError, FileHelperError, RuntimeError) as exc: raise ProjectPolicyError(f"Cannot establish project filesystem boundary: {exc}") from exc @@ -74,19 +75,19 @@ def save_project_options(project_root: Path, options: dict[str, Any]) -> None: except (TypeError, ValueError, RecursionError) as exc: raise ProjectPolicyError(f"Cannot encode {INIT_OPTIONS_FILE} as JSON: {exc}") from exc try: - files = FileHelper(project_root) - # Validate the entire destination before creating even .specify. - try: - files.read_text(INIT_OPTIONS_FILE) - except FileNotFoundError: - pass - from .integrations._file_changes import after_file_change, before_file_change + with FileHelper(project_root) as files: + # Validate the entire destination before creating even .specify. + try: + files.read_text(INIT_OPTIONS_FILE) + except FileNotFoundError: + pass + from .integrations._file_changes import after_file_change, before_file_change - destination = project_root / INIT_OPTIONS_FILE - before_file_change(destination) - files.mkdir(".specify", exist_ok=True) - files.overwrite_text(INIT_OPTIONS_FILE, content) - after_file_change(destination) + destination = project_root / INIT_OPTIONS_FILE + before_file_change(destination) + files.mkdir(".specify", exist_ok=True) + files.overwrite_text(INIT_OPTIONS_FILE, content) + after_file_change(destination) except (OSError, UnicodeError, FileHelperError, RuntimeError) as exc: raise ProjectPolicyError( f"Cannot write {INIT_OPTIONS_FILE}: {exc}. " @@ -127,11 +128,11 @@ def preflight_init_root(target: Path) -> Path: if target.exists() or target.is_symlink(): read_project_options(target) else: - files = FileHelper(_existing_init_parent(target)) - try: - files.read_bytes(target) - except FileNotFoundError: - pass + with FileHelper(_existing_init_parent(target)) as files: + try: + files.read_bytes(target) + except FileNotFoundError: + pass return target.resolve() except (OSError, FileHelperError, RuntimeError) as exc: raise ProjectPolicyError( @@ -148,7 +149,9 @@ def prepare_init_root(target: Path) -> Path: target = target.absolute() _validate_init_traversal(target) if target.exists() or target.is_symlink(): - FileHelper(target) + with FileHelper(target): + pass return target - FileHelper(_existing_init_parent(target)).mkdir(target, parents=True, exist_ok=True) + with FileHelper(_existing_init_parent(target)) as files: + files.mkdir(target, parents=True, exist_ok=True) return target diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 1176a807a7..f014ffef2b 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -10,6 +10,7 @@ import stat import tempfile from typing import TypeVar +from weakref import finalize from specify_cli._file_handles import _windows_handle @@ -48,6 +49,33 @@ class UnsupportedPathError(FileHelperError): """An accessed object cannot be used by the requested operation.""" +@dataclass +class _WalkDirectories: + root: Path + root_fd: int + flags: int + resources: ExitStack + opened: dict[Path, int] = field(default_factory=dict) + + def directory(self, path: Path) -> int: + if path == self.root: + return self.root_fd + path.relative_to(self.root) + if path not in self.opened: + parent = self.directory(path.parent) + fd = os.open(path.name, self.flags, dir_fd=parent) + self.resources.callback(os.close, fd) + self.opened[path] = fd + return self.opened[path] + + +@dataclass(frozen=True) +class _DeletionEntry: + path: Path + directory: bool + children: tuple["_DeletionEntry", ...] = () + + @dataclass(frozen=True) class FileHelper: """Own operations beneath one trusted root with an immutable link policy. @@ -60,6 +88,8 @@ class FileHelper: allow_symlinks: bool = False _canonical_root: Path = field(init=False, repr=False) _root_stat: os.stat_result = field(init=False, repr=False, compare=False) + _root_anchor: int | Path | None = field(init=False, repr=False, compare=False) + _root_cleanup: finalize = field(init=False, repr=False, compare=False) def __post_init__(self) -> None: root = Path(self.root).absolute() @@ -70,6 +100,38 @@ def __post_init__(self) -> None: object.__setattr__(self, "root", root) object.__setattr__(self, "_canonical_root", canonical) object.__setattr__(self, "_root_stat", root_stat) + with ExitStack() as resources: + anchor: int | Path | None = None + if os.name == "nt": + for current in reversed([canonical, *canonical.parents]): + resources.enter_context(_windows_handle(current, directory=True)) + self._check_deletion_root(canonical.stat()) + anchor = canonical + elif ( + _DELETE_DIR_FD_SUPPORTED + and getattr(os, "O_NOFOLLOW", 0) + and getattr(os, "O_DIRECTORY", 0) + ): + anchor = os.open(canonical, self._deletion_flags()) + resources.callback(os.close, anchor) + self._check_deletion_root(os.fstat(anchor)) + object.__setattr__(self, "_root_anchor", anchor) + object.__setattr__(self, "_root_cleanup", finalize(self, resources.pop_all().close)) + + def close(self) -> None: + """Release retained root handles; closing more than once is harmless.""" + self._root_cleanup() + + def _require_open(self) -> None: + if not self._root_cleanup.alive: + raise FileHelperError("FileHelper is closed") + + def __enter__(self) -> "FileHelper": + self._require_open() + return self + + def __exit__(self, *args: object) -> None: + self.close() def _parts(self, path: PurePath) -> tuple[str, ...]: original = path @@ -110,15 +172,12 @@ def _walk( allow_missing: bool = False, follow_leaf: bool = True, links: tuple[Path, ...] = (), - root_fd: int | None = None, + directories: _WalkDirectories | None = None, ) -> Path: current = self._canonical_root for index, part in enumerate(parts): - if root_fd is not None and part == "..": - # Do not let ".." skip a non-directory link-target component. - with self._deletion_directory(current, root=root_fd): - pass - elif root_fd is None and not current.is_dir(): + directory_fd = directories.directory(current) if directories is not None else None + if directories is None and not current.is_dir(): if not (allow_missing and not current.exists()): raise NotADirectoryError(current) if part == "..": @@ -128,10 +187,7 @@ def _walk( continue candidate = current / part try: - mode = ( - self._entry_mode(candidate) if root_fd is None - else self._deletion_entry_mode(candidate, root=root_fd) - ) + mode = self._entry_mode(candidate, dir_fd=directory_fd) except FileNotFoundError: if not allow_missing: raise @@ -147,19 +203,16 @@ def _walk( return candidate if candidate in links or len(links) >= 40: raise UnsupportedPathError(f"Symlink cycle or excessive chain: {candidate}") - if root_fd is None: + if directory_fd is None: target = Path(os.readlink(candidate)) else: - with self._deletion_directory(current, root=root_fd) as directory: - leaf = directory / part if isinstance(directory, Path) else part - directory_fd = None if isinstance(directory, Path) else directory - target = Path(os.readlink(leaf, dir_fd=directory_fd)) + target = Path(os.readlink(part, dir_fd=directory_fd)) 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,), root_fd=root_fd + target_parts, links=links + (candidate,), directories=directories ) elif stat.S_ISDIR(mode) or stat.S_ISREG(mode): current = candidate @@ -171,13 +224,19 @@ def _path( self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True, root_fd: int | None = None, ) -> Path: + self._require_open() original = Path(path) parts = self._parts(original) if ".." in parts: raise PathEscapeError(f"Parent traversal is not an operation path: {original}") - return self._walk( - parts, allow_missing=allow_missing, follow_leaf=follow_leaf, root_fd=root_fd - ) + with ExitStack() as resources: + directories = ( + _WalkDirectories(self._canonical_root, root_fd, self._deletion_flags(), resources) + if root_fd is not None else None + ) + return self._walk( + parts, allow_missing=allow_missing, follow_leaf=follow_leaf, directories=directories + ) @staticmethod def _require_file(path: Path) -> None: @@ -281,87 +340,89 @@ def _check_deletion_root(self, current: os.stat_result) -> None: @contextmanager def _deletion_root(self) -> Iterator[int | Path]: - """Bind the whole deletion to the root identity established at construction.""" - if os.name == "nt": - with ExitStack() as stack: - # Absolute Windows I/O also needs stable ancestors above the root. - for current in reversed([self._canonical_root, *self._canonical_root.parents]): - stack.enter_context(_windows_handle(current, directory=True)) - self._check_deletion_root(self._canonical_root.stat()) - yield self._canonical_root - return - - if not ( - _DELETE_DIR_FD_SUPPORTED - and getattr(os, "O_NOFOLLOW", 0) - and getattr(os, "O_DIRECTORY", 0) - ): + """Borrow the root pinned when this helper was established.""" + self._require_open() + if self._root_anchor is None: raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") - root = os.open(self._canonical_root, self._deletion_flags()) - try: - self._check_deletion_root(os.fstat(root)) - yield root - finally: - os.close(root) + self._check_deletion_root(self._canonical_root.stat()) + yield self._root_anchor @contextmanager - def _deletion_directory( - self, path: Path, *, root: int | Path, enumerate_entries: bool = False + def _child_deletion_directory( + self, parent: int | Path, name: str, *, readable: bool = False ) -> Iterator[int | Path]: - """Traverse from the operation's root, requesting read access only for scans.""" - parts = path.relative_to(self._canonical_root).parts - if isinstance(root, Path): - with ExitStack() as stack: - current = root - for part in parts: - current = current / part - stack.enter_context(_windows_handle(current, directory=True)) - yield current + if isinstance(parent, Path): + child = parent / name + with _windows_handle(child, directory=True): + yield child return - - flags = self._deletion_flags() - parent = os.dup(root) + flags = ( + os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + if readable else self._deletion_flags() + ) + child = os.open(name, flags, dir_fd=parent) try: + yield child + finally: + os.close(child) + + @contextmanager + def _deletion_directory(self, path: Path, *, root: int | Path) -> Iterator[int | Path]: + parts = path.relative_to(self._canonical_root).parts + with ExitStack() as resources: + parent = root for part in parts: - child = os.open(part, flags, dir_fd=parent) - os.close(parent) - parent = child - if enumerate_entries: - read_flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) - child = os.open(".", read_flags, dir_fd=parent) - os.close(parent) - parent = child + parent = resources.enter_context(self._child_deletion_directory(parent, part)) yield parent - finally: - os.close(parent) - def _deletion_entry_mode(self, path: Path, *, root: int | Path) -> int: - with self._deletion_directory(path.parent, root=root) as directory: - if isinstance(directory, Path): - return self._entry_mode(directory / path.name) - return self._entry_mode(path, dir_fd=directory) + def _deletion_entry_mode(self, path: Path, *, parent: int | Path) -> int: + if isinstance(parent, Path): + return self._entry_mode(parent / path.name) + return self._entry_mode(path, dir_fd=parent) - def _deletion_plan( - self, path: Path, *, recursive: bool, root: int | Path - ) -> list[tuple[Path, bool]]: - mode = self._deletion_entry_mode(path, root=root) + def _deletion_kind(self, path: Path, mode: int) -> bool: if stat.S_ISLNK(mode): if not self.allow_symlinks: raise SymlinkDeniedError(f"Refusing to delete symlink: {path}") - return [(path, False)] + return False if stat.S_ISREG(mode): - return [(path, False)] + return False if not stat.S_ISDIR(mode): raise UnsupportedPathError(f"Unsupported filesystem object: {path}") - plan: list[tuple[Path, bool]] = [] - if recursive: - with self._deletion_directory(path, root=root, enumerate_entries=True) as directory: - with os.scandir(directory) as entries: - children = sorted(path / entry.name for entry in entries) - for child in children: - plan.extend(self._deletion_plan(child, recursive=True, root=root)) - plan.append((path, True)) - return plan + return True + + def _deletion_plan( + self, path: Path, *, recursive: bool, parent: int | Path + ) -> _DeletionEntry: + directory = self._deletion_kind(path, self._deletion_entry_mode(path, parent=parent)) + children: tuple[_DeletionEntry, ...] = () + if directory and recursive: + with self._child_deletion_directory(parent, path.name) as child_directory: + with self._child_deletion_directory(child_directory, ".", readable=True) as readable: + with os.scandir(readable) as entries: + names = sorted(entry.name for entry in entries) + children = tuple( + self._deletion_plan(path / name, recursive=True, parent=child_directory) + for name in names + ) + return _DeletionEntry(path, directory, children) + + def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: + directory = self._deletion_kind( + entry.path, self._deletion_entry_mode(entry.path, parent=parent) + ) + if directory != entry.directory: + raise FileHelperError(f"Destination changed during deletion: {entry.path}") + if directory and entry.children: + with self._child_deletion_directory(parent, entry.path.name) as child_directory: + for child in entry.children: + self._delete_plan(child, parent=child_directory) + leaf = parent / entry.path.name if isinstance(parent, Path) else entry.path.name + directory_fd = None if isinstance(parent, Path) else parent + if directory: + os.rmdir(leaf, dir_fd=directory_fd) + else: + os.unlink(leaf, dir_fd=directory_fd) def delete(self, path: Path | str, *, recursive: bool = False) -> None: """Delete an entry; recursive deletion never follows descendant links.""" @@ -370,16 +431,6 @@ def delete(self, path: Path | str, *, recursive: bool = False) -> None: destination = self._path(path, follow_leaf=False, root_fd=root_fd) if destination == self._canonical_root: raise FileHelperError(f"Refusing to delete the established root: {self.root}") - plan = self._deletion_plan(destination, recursive=recursive, root=root) - for entry, directory in plan: - # Recheck accessed parents/leaf before each mutation, not just preflight. - checked = self._path(entry, follow_leaf=False, root_fd=root_fd) - if checked != entry: - raise FileHelperError(f"Destination changed during deletion: {entry}") - with self._deletion_directory(entry.parent, root=root) as parent: - leaf = parent / entry.name if isinstance(parent, Path) else entry.name - directory_fd = None if isinstance(parent, Path) else parent - if directory: - os.rmdir(leaf, dir_fd=directory_fd) - else: - os.unlink(leaf, dir_fd=directory_fd) + with self._deletion_directory(destination.parent, root=root) as parent: + plan = self._deletion_plan(destination, recursive=recursive, parent=parent) + self._delete_plan(plan, parent=parent) diff --git a/src/specify_cli/integration_state.py b/src/specify_cli/integration_state.py index ae29b833a8..5e9afb8347 100644 --- a/src/specify_cli/integration_state.py +++ b/src/specify_cli/integration_state.py @@ -46,7 +46,7 @@ def _read_integration_json_data( files = FileHelper(project_root) except FileNotFoundError: return None, None - except (OSError, RuntimeError) as exc: + except (OSError, FileHelperError, RuntimeError) as exc: return None, IntegrationReadError( kind="os", detail=f"Cannot establish project root for {INTEGRATION_JSON}: {exc}" ) @@ -57,7 +57,8 @@ def _read_integration_json_data( # other OSErrors (which become loud errors via the IntegrationReadError # path). try: - raw = files.read_text(INTEGRATION_JSON, encoding="utf-8") + with files: + raw = files.read_text(INTEGRATION_JSON, encoding="utf-8") except FileNotFoundError: return None, None except IsADirectoryError as exc: @@ -245,44 +246,44 @@ def write_integration_json( State reads and writes deny links below that root, independently of any lifecycle transaction or operational symlink opt-in. """ - files = FileHelper(project_root) - # Reject the full destination before an observer can snapshot its target. - try: - files.read_bytes(INTEGRATION_JSON) - except FileNotFoundError: - pass - except SymlinkDeniedError as exc: - raise SymlinkDeniedError(_SYMLINK_STATE_ERROR) from exc - dest = project_root / INTEGRATION_JSON - - integration_key = clean_integration_key(integration_key) - installed = dedupe_integration_keys(installed_integrations or []) - if integration_key and integration_key not in installed: - installed.insert(0, integration_key) - if not integration_key and installed: - integration_key = installed[0] - - normalized_settings = normalize_integration_settings(settings or {}) - normalized_settings = { - key: normalized_settings[key] for key in installed if key in normalized_settings - } - - data: dict[str, Any] = { - "version": version, - "integration_state_schema": INTEGRATION_STATE_SCHEMA, - "installed_integrations": installed, - "integration_settings": normalized_settings, - } - if integration_key: - data["integration"] = integration_key - data["default_integration"] = integration_key - - from .integrations._file_changes import after_file_change, before_file_change - - try: - before_file_change(dest) - files.mkdir(".specify", exist_ok=True) - files.overwrite_text(INTEGRATION_JSON, json.dumps(data, indent=2) + "\n", encoding="utf-8") - after_file_change(dest) - except SymlinkDeniedError as exc: - raise SymlinkDeniedError(_SYMLINK_STATE_ERROR) from exc + with FileHelper(project_root) as files: + # Reject the full destination before an observer can snapshot its target. + try: + files.read_bytes(INTEGRATION_JSON) + except FileNotFoundError: + pass + except SymlinkDeniedError as exc: + raise SymlinkDeniedError(_SYMLINK_STATE_ERROR) from exc + dest = project_root / INTEGRATION_JSON + + integration_key = clean_integration_key(integration_key) + installed = dedupe_integration_keys(installed_integrations or []) + if integration_key and integration_key not in installed: + installed.insert(0, integration_key) + if not integration_key and installed: + integration_key = installed[0] + + normalized_settings = normalize_integration_settings(settings or {}) + normalized_settings = { + key: normalized_settings[key] for key in installed if key in normalized_settings + } + + data: dict[str, Any] = { + "version": version, + "integration_state_schema": INTEGRATION_STATE_SCHEMA, + "installed_integrations": installed, + "integration_settings": normalized_settings, + } + if integration_key: + data["integration"] = integration_key + data["default_integration"] = integration_key + + from .integrations._file_changes import after_file_change, before_file_change + + try: + before_file_change(dest) + files.mkdir(".specify", exist_ok=True) + files.overwrite_text(INTEGRATION_JSON, json.dumps(data, indent=2) + "\n", encoding="utf-8") + after_file_change(dest) + except SymlinkDeniedError as exc: + raise SymlinkDeniedError(_SYMLINK_STATE_ERROR) from exc diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 092928d67e..3e74e74ff1 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -2,11 +2,13 @@ from dataclasses import FrozenInstanceError import errno -from contextlib import contextmanager +import gc +from contextlib import contextmanager, nullcontext import os from pathlib import Path, PureWindowsPath import subprocess from types import SimpleNamespace +import weakref import pytest @@ -527,7 +529,7 @@ def test_delete_exchange_after_final_validation(root, link, monkeypatch, allow): outside.mkdir() (outside / "victim").write_text("keep") retained = root / "retained" - check = FileHelper._path + check = FileHelper._deletion_entry_mode checks = 0 def exchange(self, path, **kwargs): @@ -535,16 +537,21 @@ def exchange(self, path, **kwargs): result = check(self, path, **kwargs) checks += 1 if checks == 2: - parent.rename(retained) - link(parent, outside) + try: + parent.rename(retained) + except PermissionError: + if os.name != "nt": + raise + else: + link(parent, outside) return result - monkeypatch.setattr(FileHelper, "_path", exchange) - with pytest.raises(OSError): - FileHelper(root, allow_symlinks=allow).delete(parent / "victim") + monkeypatch.setattr(FileHelper, "_deletion_entry_mode", exchange) + with FileHelper(root, allow_symlinks=allow) as files: + files.delete(parent / "victim") assert checks == 2 assert (outside / "victim").read_text() == "keep" - assert (retained / "victim").read_text() == "selected" + assert not ((retained if retained.exists() else parent) / "victim").exists() @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor traversal") @@ -572,6 +579,128 @@ def exchange(path, flags, *args, **kwargs): assert (retained / "victim").read_text() == "selected" +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("depth", [8, 32, 64]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_delete_directory_open_count_is_linear(root, monkeypatch, depth, recursive): + tree = root / "tree" + deepest = tree.joinpath(*("nested" for _ in range(depth))) + deepest.mkdir(parents=True) + (deepest / "victim").write_text("selected") + open_directory = os.open + opened = 0 + + def observe(*args, **kwargs): + nonlocal opened + opened += 1 + return open_directory(*args, **kwargs) + + monkeypatch.setattr(os, "open", observe) + FileHelper(root).delete(tree if recursive else deepest / "victim", recursive=recursive) + assert not (deepest / "victim").exists() + assert opened <= 8 * depth + 16 + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX retained root after unlink") +def test_delete_does_not_rebind_when_root_identity_is_reused(root, monkeypatch): + files = FileHelper(root) + root.rmdir() + root.mkdir() + (root / "victim").write_text("keep") + monkeypatch.setattr(os.path, "samestat", lambda *args: True) + with pytest.raises((FileNotFoundError, FileHelperError)): + files.delete("victim") + assert (root / "victim").read_text() == "keep" + files.close() + + +@pytest.mark.parametrize("failure", [False, True]) +def test_file_helper_context_releases_root_and_rejects_reuse(root, failure): + expected = pytest.raises(RuntimeError, match="operation failed") if failure else nullcontext() + with expected: + with FileHelper(root) as files: + files.create_text("victim", "selected") + files.delete("victim") + assert files._root_cleanup.alive + if failure: + raise RuntimeError("operation failed") + assert not files._root_cleanup.alive + files.close() + for operation in (lambda: files.read_text("victim"), lambda: files.delete("victim")): + with pytest.raises(FileHelperError, match="closed"): + operation() + renamed = root.parent / "renamed" + root.rename(renamed) + renamed.rename(root) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +def test_file_helper_finalizer_does_not_retain_helper_or_leak_root(root): + files = FileHelper(root) + descriptor = files._root_anchor + assert isinstance(descriptor, int) + reference = weakref.ref(files) + del files + gc.collect() + assert reference() is None + with pytest.raises(OSError) as exc: + os.fstat(descriptor) + assert exc.value.errno == errno.EBADF + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX retained root descriptor") +def test_file_helper_reuses_root_handle_across_deletions(root, monkeypatch): + open_directory = os.open + roots = [] + + def observe(path, *args, **kwargs): + fd = open_directory(path, *args, **kwargs) + if path == root: + roots.append(fd) + return fd + + monkeypatch.setattr(os, "open", observe) + with FileHelper(root) as files: + for name in ("first", "second"): + files.create_text(name, "selected") + files.delete(name) + assert len(roots) == 1 + assert os.fstat(roots[0]).st_ino == root.stat().st_ino + with pytest.raises(OSError) as exc: + os.fstat(roots[0]) + assert exc.value.errno == errno.EBADF + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX search-flag fallback") +def test_delete_without_search_flags_supports_readable_parents(root, monkeypatch): + monkeypatch.delattr(os, "O_SEARCH", raising=False) + monkeypatch.delattr(os, "O_PATH", raising=False) + with FileHelper(root) as files: + files.mkdir("tree/nested", parents=True) + files.create_text("tree/nested/victim", "selected") + files.delete("tree", recursive=True) + assert not (root / "tree").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX search-flag fallback") +def test_delete_without_search_flags_reports_unreadable_parent(root, monkeypatch): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory read permissions") + monkeypatch.delattr(os, "O_SEARCH", raising=False) + monkeypatch.delattr(os, "O_PATH", raising=False) + parent = root / "parent" + parent.mkdir() + (parent / "victim").write_text("keep") + with FileHelper(root) as files: + parent.chmod(0o300) + try: + with pytest.raises(PermissionError): + files.delete(parent / "victim") + assert (parent / "victim").read_text() == "keep" + finally: + parent.chmod(0o700) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") @pytest.mark.parametrize("failure", [False, True]) def test_delete_closes_directory_descriptors(root, monkeypatch, failure): @@ -638,7 +767,6 @@ def test_delete_rejects_root_ancestor_exchange_at_open(root, link, monkeypatch, outside = root / "outside" (outside / "project/tree").mkdir(parents=True) (outside / "project/tree/victim").write_text("keep") - files = FileHelper(project, allow_symlinks=allow) retained = root / "retained" open_directory = os.open attempted = False @@ -656,7 +784,8 @@ def exchange(path, flags, *args, **kwargs): monkeypatch.setattr(os, "open", exchange) with pytest.raises(FileHelperError, match="root changed"): - files.delete("tree" if recursive else "tree/victim", recursive=recursive) + with FileHelper(project, allow_symlinks=allow) as files: + files.delete("tree" if recursive else "tree/victim", recursive=recursive) assert attempted assert (outside / "project/tree/victim").read_text() == "keep" assert (retained / "project/tree/victim").read_text() == "selected" @@ -890,6 +1019,8 @@ def test_delete_parent_link_targets_preserve_failure_contract(root, link, kind, @pytest.mark.parametrize("parent_is_root", [False, True]) @pytest.mark.parametrize("kind", ["file", "empty-directory", "recursive-tree"]) def test_delete_through_write_execute_only_parent(root, parent_is_root, kind): + if not (getattr(os, "O_SEARCH", 0) or getattr(os, "O_PATH", 0)): + pytest.skip("Search/path-only directory descriptors are unavailable") if os.geteuid() == 0: pytest.skip("Root bypasses directory read permissions") parent = root if parent_is_root else root / "parent" @@ -950,6 +1081,12 @@ def test_delete_rejects_replaced_root_before_validation(root, monkeypatch, allow (root / "victim").write_text("selected") files = FileHelper(root, allow_symlinks=allow) retained = root.parent / "retained" + if os.name == "nt": + with pytest.raises(PermissionError): + root.rename(retained) + assert (root / "victim").read_text() == "selected" + files.close() + return root.rename(retained) root.mkdir() (root / "victim").write_text("keep") @@ -976,7 +1113,6 @@ def test_windows_delete_rejects_replaced_root_during_acquisition_portably( outside = root / "outside" (outside / "project").mkdir(parents=True) (outside / "project/victim").write_text("keep") - files = FileHelper(project) retained = root / "retained" active = [] attempted = False @@ -1001,7 +1137,8 @@ def handle(path, *, directory): monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) with pytest.raises(FileHelperError, match="root changed"): - files.delete("victim") + with FileHelper(project) as files: + files.delete("victim") assert attempted assert active == [] assert (outside / "project/victim").read_text() == "keep" @@ -1075,6 +1212,8 @@ def remove_directory(path, *, dir_fd=None): files.delete(selected, recursive=recursive) assert not (parent / "victim").exists() assert events + assert root.resolve() in active + files.close() assert active == [] assert alias.is_symlink() @@ -1255,6 +1394,7 @@ def test_exclusive_create_rejects_existing_leaf_before_open( target.write_bytes(b"keep") alias = link(root / "alias", root / "alias" if cyclic else target) original_target = os.readlink(alias) + files = FileHelper(root, allow_symlinks=True) opened = [] real_open = os.open @@ -1265,7 +1405,8 @@ def recording_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) + getattr(files, operation)(alias, content) + files.close() assert error.value.errno == errno.EEXIST assert opened == [] assert alias.is_symlink() From 065b56ded9a9dddfe1dd4939122b523357945a35 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:44:36 -0600 Subject: [PATCH 05/16] fix: preserve helper access and close operation owners Share Darwin search-only flags, preserve deep deletion planning, and keep Windows root identity references distinct from operation-time ancestry pins. Close workflow owners on every exit and cover constructor diagnostics, retained-error cleanup, recursive handles, and overwrite failures. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 35 ++- src/specify_cli/_file_handles.py | 33 ++- src/specify_cli/file_helper.py | 70 +++-- src/specify_cli/workflows/_operation_list.py | 52 ++-- tests/integrations/test_integration_state.py | 56 +++- tests/specify_cli/test_file_policy.py | 62 ++++- .../workflows/test_operation_list.py | 41 +++ tests/test_file_handles.py | 35 ++- tests/test_file_helper.py | 249 +++++++++++++++--- 9 files changed, 535 insertions(+), 98 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 96a49a3699..06525065ae 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -146,17 +146,31 @@ 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. -On supported deletion backends, construction pins the canonical root with an -open directory handle. That handle remains owned by the helper until `close()` -or context-manager exit. Keeping the original directory open also prevents its -identity from being reused for a different directory while the helper is alive; -stored device/inode numbers alone are not the binding. Windows additionally -retains the canonical ancestors without delete sharing. Close the helper before -external code renames or removes that root or its ancestors. Automatic +On supported deletion backends, construction retains a handle to the canonical +root until `close()` or context-manager exit. Keeping the original directory open +prevents its identity from being reused for a different directory while the +helper is alive; stored device/inode numbers alone are not the binding. On POSIX, +the handle uses search-only access where available. On Windows, construction +requests only root metadata access (`FILE_READ_ATTRIBUTES`) and permits delete +sharing; it does not request directory-list access on the canonical ancestors. +The retained reference records the full volume/file identity from that handle. +If that reference or identity query is unavailable, the native capability error +is deferred to `delete()` rather than preventing ordinary read/write operations. + +Windows deletion separately pins the canonical ancestry through the drive/share +root without delete sharing, then compares the opened root's full identity with +the retained reference. Those operation-time handles require both +`FILE_LIST_DIRECTORY` and `FILE_READ_ATTRIBUTES` on every canonical ancestor, +including those above the helper root, and reject reparse points. An ACL allowing +traversal but not listing can therefore prevent deletion while still allowing +bootstrap reads. These locks are released at the end of deletion; the +metadata-only reference remains until the helper closes. Automatic finalization is a compatibility fallback, not a substitute for deterministic cleanup. Bootstrap configuration and managed integration-state consumers close -their own helpers explicitly; consumers receiving `project_files()` own its -lifetime. Pinning the root does not make other pathname-based operations +their own helpers explicitly. Installed-workflow listing also closes its +`project_files()` owner on every validation, registry-read, and successful exit; +other consumers receiving `project_files()` own its lifetime. +Pinning the root does not make other pathname-based operations descriptor-relative or change their documented concurrency limitations. | Operation | Default deny | Explicit allow | @@ -230,7 +244,8 @@ descriptor-relative or change their documented concurrency limitations. Platforms lacking the required directory-descriptor operations fail with `ENOTSUP`, not pathname-only deletion. POSIX traversal and removal use search/path-only descriptors (`O_SEARCH` or - `O_PATH`) where available, so known entries in write-and-execute-only parents + `O_PATH`) where available, including Darwin's native `O_SEARCH` value on Python + builds that do not expose it, so known entries in write-and-execute-only parents can still be removed. Read-capable descriptors are requested only for directories being enumerated; systems without search/path-only flags retain read-capable traversal. Recursive enumeration still requires directory read permission, and diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index 460e25525c..3bb53c9b9f 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -7,8 +7,8 @@ @contextmanager -def _windows_handle(path: Path, *, directory: bool) -> Iterator[int]: - """Pin a non-reparse path by withholding Windows delete sharing.""" +def _windows_handle(path: Path, *, directory: bool, pin: bool = True) -> Iterator[int]: + """Open a non-reparse object, optionally preventing directory replacement.""" import ctypes from ctypes import wintypes @@ -37,12 +37,12 @@ class AttributeTagInfo(ctypes.Structure): # OPEN_REPARSE_POINT inspects the link itself; BACKUP_SEMANTICS permits # directory handles. Holding every ancestor without FILE_SHARE_DELETE # prevents a junction/symlink swap during path-based create/replace. - # Attribute-only handles bypass share checking: include FILE_LIST_DIRECTORY - # with FILE_READ_ATTRIBUTES so even an empty directory cannot be renamed. + # Pinning needs FILE_LIST_DIRECTORY as well as FILE_READ_ATTRIBUTES. + # An attribute-only reference preserves identity without locking ancestors. handle = create( str(path), - (0x80 | 0x1) if directory else 0x80000000, - 0x3, + ((0x80 | 0x1) if directory else 0x80000000) if pin else 0x80, + 0x3 if pin else 0x7, None, 3, 0x00200000 | 0x02000000, @@ -61,3 +61,24 @@ class AttributeTagInfo(ctypes.Structure): yield handle finally: close(handle) + + +def _windows_file_id(handle: int) -> tuple[int, bytes]: + """Read the volume and complete file identity from an already opened object.""" + import ctypes + from ctypes import wintypes + + class FileIdInfo(ctypes.Structure): + _fields_ = [ + ("volume", ctypes.c_ulonglong), + ("file_id", ctypes.c_ubyte * 16), + ] + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + info = kernel.GetFileInformationByHandleEx + info.argtypes = [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] + info.restype = wintypes.BOOL + identity = FileIdInfo() + if not info(handle, 18, ctypes.byref(identity), ctypes.sizeof(identity)): + raise ctypes.WinError(ctypes.get_last_error()) + return identity.volume, bytes(identity.file_id) diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 08b4cfdf6a..e651454ca4 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -13,7 +13,7 @@ from typing import TypeVar from weakref import finalize -from specify_cli._file_handles import _windows_handle +from specify_cli._file_handles import _windows_file_id, _windows_handle _PathType = TypeVar("_PathType", bound=PurePath) @@ -90,6 +90,8 @@ class FileHelper: _canonical_root: Path = field(init=False, repr=False) _root_stat: os.stat_result = field(init=False, repr=False, compare=False) _root_anchor: int | Path | None = field(init=False, repr=False, compare=False) + _root_anchor_error: OSError | None = field(init=False, repr=False, compare=False) + _windows_root_id: tuple[int, bytes] | None = field(init=False, repr=False, compare=False) _root_cleanup: finalize = field(init=False, repr=False, compare=False) def __post_init__(self) -> None: @@ -103,20 +105,34 @@ def __post_init__(self) -> None: object.__setattr__(self, "_root_stat", root_stat) with ExitStack() as resources: anchor: int | Path | None = None + anchor_error: OSError | None = None + windows_root_id: tuple[int, bytes] | None = None if os.name == "nt": - for current in reversed([canonical, *canonical.parents]): - resources.enter_context(_windows_handle(current, directory=True)) - self._check_deletion_root(canonical.stat()) - anchor = canonical + with ExitStack() as reference: + try: + handle = reference.enter_context( + _windows_handle(canonical, directory=True, pin=False) + ) + windows_root_id = _windows_file_id(handle) + except OSError as exc: + # Other operations do not require deletion's handle capability. + exc.__traceback__ = None + anchor_error = exc + else: + self._check_deletion_root(canonical.stat()) + resources.enter_context(reference.pop_all()) + anchor = canonical elif ( _DELETE_DIR_FD_SUPPORTED and getattr(os, "O_NOFOLLOW", 0) and getattr(os, "O_DIRECTORY", 0) ): - anchor = os.open(canonical, self._deletion_flags()) + anchor = os.open(canonical, self._directory_flags()) resources.callback(os.close, anchor) self._check_deletion_root(os.fstat(anchor)) object.__setattr__(self, "_root_anchor", anchor) + object.__setattr__(self, "_root_anchor_error", anchor_error) + object.__setattr__(self, "_windows_root_id", windows_root_id) object.__setattr__(self, "_root_cleanup", finalize(self, resources.pop_all().close)) def close(self) -> None: @@ -232,7 +248,7 @@ def _path( raise PathEscapeError(f"Parent traversal is not an operation path: {original}") with ExitStack() as resources: directories = ( - _WalkDirectories(self._canonical_root, root_fd, self._deletion_flags(), resources) + _WalkDirectories(self._canonical_root, root_fd, self._directory_flags(), resources) if root_fd is not None else None ) return self._walk( @@ -279,11 +295,7 @@ def _creation_directory(self, directory: Path) -> Iterator[int | Path]: and os.open in os.supports_dir_fd ): raise OSError(errno.ENOTSUP, "Anchored file creation is unsupported on this platform") - access = getattr(os, "O_PATH", 0) or getattr(os, "O_SEARCH", 0) - if not access: - # Darwin's O_SEARCH ABI is missing from some older Python builds. - access = (0x40000000 | os.O_DIRECTORY) if sys.platform == "darwin" else os.O_RDONLY - flags = access | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + flags = self._directory_flags() parent = os.open(self._canonical_root, flags) try: for part in parts: @@ -375,8 +387,11 @@ def symlink(self, target: Path | str, path: Path | str) -> None: destination.symlink_to(target, target_is_directory=resolved.is_dir()) @staticmethod - def _deletion_flags() -> int: - access = getattr(os, "O_SEARCH", getattr(os, "O_PATH", os.O_RDONLY)) + def _directory_flags() -> int: + access = getattr(os, "O_PATH", 0) or getattr(os, "O_SEARCH", 0) + if not access: + # Some older Python builds omit Darwin's native search-only flag. + access = (0x40000000 | os.O_DIRECTORY) if sys.platform == "darwin" else os.O_RDONLY return access | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) def _check_deletion_root(self, current: os.stat_result) -> None: @@ -388,9 +403,22 @@ def _deletion_root(self) -> Iterator[int | Path]: """Borrow the root pinned when this helper was established.""" self._require_open() if self._root_anchor is None: + if self._root_anchor_error is not None: + raise self._root_anchor_error raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") self._check_deletion_root(self._canonical_root.stat()) - yield self._root_anchor + if isinstance(self._root_anchor, Path): + with ExitStack() as resources: + for ancestor in reversed(self._root_anchor.parents): + resources.enter_context(_windows_handle(ancestor, directory=True)) + handle = resources.enter_context( + _windows_handle(self._root_anchor, directory=True) + ) + if _windows_file_id(handle) != self._windows_root_id: + raise FileHelperError(f"Established root changed during deletion: {self.root}") + yield self._root_anchor + else: + yield self._root_anchor @contextmanager def _child_deletion_directory( @@ -403,7 +431,7 @@ def _child_deletion_directory( return flags = ( os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) - if readable else self._deletion_flags() + if readable else self._directory_flags() ) child = os.open(name, flags, dir_fd=parent) try: @@ -446,10 +474,12 @@ def _deletion_plan( with self._child_deletion_directory(child_directory, ".", readable=True) as readable: with os.scandir(readable) as entries: names = sorted(entry.name for entry in entries) - children = tuple( - self._deletion_plan(path / name, recursive=True, parent=child_directory) - for name in names - ) + child_plans = [] + for name in names: + child_plans.append( + self._deletion_plan(path / name, recursive=True, parent=child_directory) + ) + children = tuple(child_plans) return _DeletionEntry(path, directory, children) def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: diff --git a/src/specify_cli/workflows/_operation_list.py b/src/specify_cli/workflows/_operation_list.py index 3f0fa902d5..4844207a42 100644 --- a/src/specify_cli/workflows/_operation_list.py +++ b/src/specify_cli/workflows/_operation_list.py @@ -3,6 +3,7 @@ from __future__ import annotations from dataclasses import dataclass +from contextlib import ExitStack from itertools import islice from pathlib import Path import stat @@ -95,33 +96,34 @@ def list_workflows(request: WorkflowListRequest) -> WorkflowListResult: raise WorkflowListError( "invalid_limit", f"limit must be between 1 and {WORKFLOW_LIST_MAX_LIMIT}" ) - try: - # Unlike is_dir(), stat() preserves permission errors on Python 3.14. + with ExitStack() as owners: try: - root_is_directory = stat.S_ISDIR(root.stat().st_mode) - except (FileNotFoundError, NotADirectoryError, ValueError): - root_is_directory = False - if not root_is_directory: - raise WorkflowListError( - "not_a_spec_kit_project", - "Not a Spec Kit project (no .specify/ directory)", - ) - files = project_files(root) - if not (root / ".specify").is_dir(): - raise WorkflowListError( - "not_a_spec_kit_project", - "Not a Spec Kit project (no .specify/ directory)", - ) - validate_workflow_storage(root) - except (ProjectPolicyError, WorkflowStorageError, OSError) as exc: - raise WorkflowListError("unsafe_workflow_storage", str(exc)) from exc + # Unlike is_dir(), stat() preserves permission errors on Python 3.14. + try: + root_is_directory = stat.S_ISDIR(root.stat().st_mode) + except (FileNotFoundError, NotADirectoryError, ValueError): + root_is_directory = False + if not root_is_directory: + raise WorkflowListError( + "not_a_spec_kit_project", + "Not a Spec Kit project (no .specify/ directory)", + ) + files = owners.enter_context(project_files(root)) + if not (root / ".specify").is_dir(): + raise WorkflowListError( + "not_a_spec_kit_project", + "Not a Spec Kit project (no .specify/ directory)", + ) + validate_workflow_storage(root) + except (ProjectPolicyError, WorkflowStorageError, OSError) as exc: + raise WorkflowListError("unsafe_workflow_storage", str(exc)) from exc - try: - installed = WorkflowRegistry(root, read_files=files).list() - except OSError as exc: - raise WorkflowListError( - "workflow_registry_read_failed", f"Failed to read workflow registry: {exc}" - ) from exc + try: + installed = WorkflowRegistry(root, read_files=files).list() + except OSError as exc: + raise WorkflowListError( + "workflow_registry_read_failed", f"Failed to read workflow registry: {exc}" + ) from exc items: list[WorkflowListEntry | WorkflowListWarning] = [] selected = installed.items() if limit is None else islice(installed.items(), limit) diff --git a/tests/integrations/test_integration_state.py b/tests/integrations/test_integration_state.py index ceaa9cc845..c3f53fcbc8 100644 --- a/tests/integrations/test_integration_state.py +++ b/tests/integrations/test_integration_state.py @@ -5,7 +5,8 @@ import pytest from specify_cli._file_policy import project_files, save_project_options -from specify_cli.file_helper import SymlinkDeniedError +from specify_cli.file_helper import FileHelper, FileHelperError, SymlinkDeniedError +from specify_cli import integration_state from specify_cli.integration_state import ( INTEGRATION_JSON, default_integration_key, @@ -18,6 +19,59 @@ from specify_cli.integrations._file_changes import file_change_observer +@pytest.mark.parametrize("reader", [try_read_integration_json, try_read_integration_json_with_raw]) +def test_state_readers_translate_constructor_contract_failure(tmp_path, monkeypatch, reader): + def failed(*args, **kwargs): + raise FileHelperError("root binding failed") + + monkeypatch.setattr(integration_state, "FileHelper", failed) + *states, error = reader(tmp_path) + assert all(state is None for state in states) + assert error is not None and error.kind == "os" + assert "root binding failed" in error.detail + + +@pytest.mark.parametrize("operation", ["read", "read-with-raw", "write"]) +@pytest.mark.parametrize("failure", [False, True]) +def test_managed_state_consumers_close_retained_helpers(tmp_path, monkeypatch, operation, failure): + (tmp_path / ".specify").mkdir() + (tmp_path / INTEGRATION_JSON).write_text('{"integration": "claude"}') + captured = [] + + def owner(*args, **kwargs): + files = FileHelper(*args, **kwargs) + captured.append(files) + return files + + def denied(*args, **kwargs): + raise PermissionError("managed state access denied") + + monkeypatch.setattr(integration_state, "FileHelper", owner) + if failure: + monkeypatch.setattr(FileHelper, "overwrite_text" if operation == "write" else "read_text", denied) + retained_error = None + if operation == "write": + if failure: + with pytest.raises(PermissionError, match="managed state access denied") as raised: + write_integration_json(tmp_path, version="1", integration_key="claude") + retained_error = raised.value + else: + write_integration_json(tmp_path, version="1", integration_key="claude") + else: + reader = try_read_integration_json if operation == "read" else try_read_integration_json_with_raw + *states, retained_error = reader(tmp_path) + if failure: + assert all(state is None for state in states) + assert retained_error is not None and retained_error.kind == "os" + else: + assert all(state is not None for state in states) + assert retained_error is None + assert len(captured) == 1 + assert not captured[0]._root_cleanup.alive + if failure: + assert retained_error is not None + + def test_normalize_integration_state_strips_default_key_without_duplicates(): state = normalize_integration_state( { diff --git a/tests/specify_cli/test_file_policy.py b/tests/specify_cli/test_file_policy.py index 3f7f356545..24df7c6f13 100644 --- a/tests/specify_cli/test_file_policy.py +++ b/tests/specify_cli/test_file_policy.py @@ -11,7 +11,7 @@ from typer.testing import CliRunner from specify_cli import app -from specify_cli.file_helper import FileHelper +from specify_cli.file_helper import FileHelper, FileHelperError def policy_module(): @@ -20,6 +20,66 @@ def policy_module(): return _file_policy +@pytest.mark.parametrize("operation", ["read_project_options", "project_files"]) +def test_policy_translates_constructor_contract_failure(tmp_path, monkeypatch, operation): + policy = policy_module() + if operation == "project_files": + monkeypatch.setattr(policy, "read_project_options", lambda _: {}) + + def failed(*args, **kwargs): + raise FileHelperError("root binding failed") + + monkeypatch.setattr(policy, "FileHelper", failed) + with pytest.raises(policy.ProjectPolicyError, match="root binding failed") as raised: + getattr(policy, operation)(tmp_path) + assert isinstance(raised.value.__cause__, FileHelperError) + + +@pytest.mark.parametrize( + ("operation", "failing_method"), + [ + ("read_project_options", "read_text"), + ("save_project_options", "overwrite_text"), + ("preflight_init_root", "read_bytes"), + ("prepare_init_root", "mkdir"), + ], +) +@pytest.mark.parametrize("failure", [False, True]) +def test_bootstrap_consumers_close_retained_helpers( + tmp_path, monkeypatch, operation, failing_method, failure +): + policy = policy_module() + (tmp_path / ".specify").mkdir() + (tmp_path / ".specify/init-options.json").write_text("{}") + captured = [] + + def owner(*args, **kwargs): + files = FileHelper(*args, **kwargs) + captured.append(files) + return files + + def denied(*args, **kwargs): + raise PermissionError("bootstrap access denied") + + monkeypatch.setattr(policy, "FileHelper", owner) + if failure: + monkeypatch.setattr(FileHelper, failing_method, denied) + target = tmp_path / "missing/project" if operation.endswith("init_root") else tmp_path + args = (target, {}) if operation == "save_project_options" else (target,) + retained_error = None + if failure: + error_type = PermissionError if operation == "prepare_init_root" else policy.ProjectPolicyError + with pytest.raises(error_type, match="bootstrap access denied") as raised: + getattr(policy, operation)(*args) + retained_error = raised.value + else: + getattr(policy, operation)(*args) + assert len(captured) == 1 + assert not captured[0]._root_cleanup.alive + if failure: + assert retained_error is not None + + @pytest.fixture def symlinks(tmp_path): probe = tmp_path / "probe" diff --git a/tests/specify_cli/workflows/test_operation_list.py b/tests/specify_cli/workflows/test_operation_list.py index c2730dcb23..3379e2cd66 100644 --- a/tests/specify_cli/workflows/test_operation_list.py +++ b/tests/specify_cli/workflows/test_operation_list.py @@ -52,6 +52,47 @@ def test_missing_registry_is_empty_without_creating_storage(tmp_path): assert list((tmp_path / ".specify").iterdir()) == [] +@pytest.mark.parametrize("outcome", ["success", "missing-marker", "storage-error", "registry-error"]) +def test_list_workflows_closes_owner_on_every_exit(tmp_path, monkeypatch, outcome): + if outcome != "missing-marker": + _registry(tmp_path, {}) + captured = [] + original = operation.project_files + + def owner(root): + files = original(root) + captured.append(files) + return files + + def storage_error(*args): + raise operation.WorkflowStorageError("storage unavailable") + + def registry_error(*args): + raise OSError("registry unavailable") + + monkeypatch.setattr(operation, "project_files", owner) + if outcome == "storage-error": + monkeypatch.setattr(operation, "validate_workflow_storage", storage_error) + elif outcome == "registry-error": + monkeypatch.setattr(operation.WorkflowRegistry, "list", registry_error) + error = None + if outcome == "success": + assert operation.list_workflows(operation.WorkflowListRequest(tmp_path)).items == () + else: + with pytest.raises(operation.WorkflowListError) as raised: + operation.list_workflows(operation.WorkflowListRequest(tmp_path)) + error = raised.value + assert error.code == { + "missing-marker": "not_a_spec_kit_project", + "storage-error": "unsafe_workflow_storage", + "registry-error": "workflow_registry_read_failed", + }[outcome] + assert len(captured) == 1 + assert not captured[0]._root_cleanup.alive + if outcome != "success": + assert error is not None + + @pytest.mark.parametrize("contents", ["{", "[]", '{"workflows": []}', "\udcff"]) def test_corrupt_registry_fails_closed(tmp_path, contents): path = _registry(tmp_path, {}) diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index e81e39037f..94f39ece30 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -8,12 +8,13 @@ import pytest -from specify_cli._file_handles import _windows_handle +from specify_cli._file_handles import _windows_file_id, _windows_handle @pytest.mark.parametrize("directory", [False, True]) +@pytest.mark.parametrize("pin", [False, True]) @pytest.mark.parametrize("outcome", ["success", "reparse", "wrong-type", "query-failure", "body-failure", "open-failure"]) -def test_windows_handle_flags_validation_and_cleanup(monkeypatch, directory, outcome): +def test_windows_handle_flags_validation_and_cleanup(monkeypatch, directory, pin, outcome): calls = [] def create(*args): @@ -46,7 +47,7 @@ def close(handle): ) def use(): - with _windows_handle(Path("selected"), directory=directory) as handle: + with _windows_handle(Path("selected"), directory=directory, pin=pin) as handle: assert handle == 123 assert calls == [calls[0]] if outcome == "body-failure": @@ -64,12 +65,36 @@ def use(): assert exc.value.errno == (errno.ELOOP if outcome == "reparse" else errno.EINVAL) args = calls[0][1] assert args == ( - "selected", 0x81 if directory else 0x80000000, - 0x3, None, 3, 0x00200000 | 0x02000000, None, + "selected", (0x81 if directory else 0x80000000) if pin else 0x80, + 0x3 if pin else 0x7, None, 3, 0x00200000 | 0x02000000, None, ) assert calls[1:] == ([] if outcome == "open-failure" else [("close", 123)]) +@pytest.mark.parametrize("failure", [False, True]) +def test_windows_file_identity_uses_full_volume_and_file_id(monkeypatch, failure): + def info(handle, kind, buffer, size): + assert handle == 0x1FFFFFFFF and kind == 18 and size == 24 + buffer._obj.volume = 0x1FFFFFFFF + buffer._obj.file_id[:] = bytes(range(16)) + return not failure + + monkeypatch.setattr( + ctypes, "WinDLL", + lambda *args, **kwargs: SimpleNamespace(GetFileInformationByHandleEx=info), + raising=False, + ) + monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) + monkeypatch.setattr(ctypes, "WinError", lambda code: PermissionError(code, "identity denied"), raising=False) + if failure: + with pytest.raises(PermissionError, match="identity denied"): + _windows_file_id(0x1FFFFFFFF) + else: + assert _windows_file_id(0x1FFFFFFFF) == (0x1FFFFFFFF, bytes(range(16))) + assert info.argtypes == [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] + assert info.restype is wintypes.BOOL + + @pytest.mark.parametrize("directory", [False, True]) def test_windows_handle_declares_complete_createfile_prototype(tmp_path, monkeypatch, directory): closed = [] diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 11f49e3f94..2274b46ae6 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -6,6 +6,7 @@ from contextlib import contextmanager, nullcontext import os from pathlib import Path, PureWindowsPath +import shutil import subprocess import sys from types import SimpleNamespace @@ -151,12 +152,20 @@ def test_overwrite_text_encoding_errors_propagate(root, allow): @pytest.mark.parametrize("allow", [False, True]) def test_overwrite_text_io_errors_propagate(root, monkeypatch, allow): + files = FileHelper(root, allow_symlinks=allow) + calls = [] + def denied(*args, **kwargs): + assert Path(args[0]) == root / "new" + assert args[1] & os.O_TRUNC + calls.append(args[0]) raise PermissionError("denied") monkeypatch.setattr("specify_cli.file_helper.os.open", denied) - with pytest.raises(PermissionError, match="denied"): - FileHelper(root, allow_symlinks=allow).overwrite_text("new", "text") + with files: + with pytest.raises(PermissionError, match="denied"): + files.overwrite_text("new", "text") + assert len(calls) == 1 assert not (root / "new").exists() @@ -581,6 +590,62 @@ def exchange(path, flags, *args, **kwargs): assert (retained / "victim").read_text() == "selected" +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +def test_recursive_delete_preserves_deep_tree_support(root): + import resource + + depth = 550 + if resource.getrlimit(resource.RLIMIT_NOFILE)[0] < depth + 100: + pytest.skip("Descriptor ceiling is too low for the deep-tree fixture") + tree = root / "deep" + tree.mkdir() + parent = os.open(tree, os.O_RDONLY | os.O_DIRECTORY) + try: + for _ in range(depth): + os.mkdir("d", dir_fd=parent) + child = os.open("d", os.O_RDONLY | os.O_DIRECTORY, dir_fd=parent) + os.close(parent) + parent = child + finally: + os.close(parent) + try: + with FileHelper(root) as files: + files.delete(tree, recursive=True) + assert not tree.exists() + finally: + if tree.exists(): + shutil.rmtree(tree) + + +@pytest.mark.skipif(sys.platform != "darwin", reason="Native Darwin search-only fallback") +@pytest.mark.parametrize("operation", ["read_text", "overwrite_text", "create_text", "mkdir", "delete"]) +def test_legacy_darwin_establishes_write_search_only_root(root, monkeypatch, operation): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory permission checks") + (root / "victim").write_text("selected") + monkeypatch.delattr(os, "O_PATH", raising=False) + monkeypatch.delattr(os, "O_SEARCH", raising=False) + root.chmod(0o300) + try: + with FileHelper(root) as files: + if operation == "read_text": + assert files.read_text("victim") == "selected" + elif operation == "overwrite_text": + files.overwrite_text("victim", "updated") + assert (root / "victim").read_text() == "updated" + elif operation == "create_text": + files.create_text("new", "created") + assert (root / "new").read_text() == "created" + elif operation == "mkdir": + files.mkdir("new") + assert (root / "new").is_dir() + else: + files.delete("victim") + assert not (root / "victim").exists() + finally: + root.chmod(0o700) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") @pytest.mark.parametrize("depth", [8, 32, 64]) @pytest.mark.parametrize("recursive", [False, True]) @@ -691,6 +756,7 @@ def test_delete_without_search_flags_reports_unreadable_parent(root, monkeypatch pytest.skip("Root bypasses directory read permissions") monkeypatch.delattr(os, "O_SEARCH", raising=False) monkeypatch.delattr(os, "O_PATH", raising=False) + monkeypatch.setattr(file_helper, "sys", SimpleNamespace(platform="generic-posix")) parent = root / "parent" parent.mkdir() (parent / "victim").write_text("keep") @@ -705,15 +771,18 @@ def test_delete_without_search_flags_reports_unreadable_parent(root, monkeypatch @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") -@pytest.mark.parametrize("failure", [False, True]) -def test_delete_closes_directory_descriptors(root, monkeypatch, failure): +@pytest.mark.parametrize( + ("recursive", "failure"), + [(False, None), (False, "unlink"), (True, None), (True, "unlink"), (True, "scan")], +) +def test_delete_closes_directory_descriptors(root, monkeypatch, recursive, failure): parent = root / "parent" - parent.mkdir() - (parent / "victim").write_text("selected") + (parent / "nested").mkdir(parents=True) + victim = parent / "nested/victim" + victim.write_text("selected") opened = [] root_opens = [] open_directory = os.open - duplicate = os.dup def observe(*args, **kwargs): fd = open_directory(*args, **kwargs) @@ -722,24 +791,22 @@ def observe(*args, **kwargs): root_opens.append(fd) return fd - def observe_duplicate(fd): - result = duplicate(fd) - opened.append(result) - return result - def denied(*args, **kwargs): raise PermissionError("removal denied") monkeypatch.setattr(os, "open", observe) - monkeypatch.setattr(os, "dup", observe_duplicate) - if failure: + if failure == "unlink": monkeypatch.setattr(os, "unlink", denied) - with pytest.raises(PermissionError, match="removal denied"): - FileHelper(root).delete(parent / "victim") - assert (parent / "victim").read_text() == "selected" + elif failure == "scan": + monkeypatch.setattr(os, "scandir", denied) + with FileHelper(root) as files: + expected = pytest.raises(PermissionError, match="removal denied") if failure else nullcontext() + with expected: + files.delete(parent if recursive else victim, recursive=recursive) + if failure: + assert victim.read_text() == "selected" else: - FileHelper(root).delete(parent / "victim") - assert not (parent / "victim").exists() + assert not victim.exists() assert len(root_opens) == 1 assert len(opened) >= 2 for fd in opened: @@ -1022,7 +1089,7 @@ def test_delete_parent_link_targets_preserve_failure_contract(root, link, kind, @pytest.mark.parametrize("parent_is_root", [False, True]) @pytest.mark.parametrize("kind", ["file", "empty-directory", "recursive-tree"]) def test_delete_through_write_execute_only_parent(root, parent_is_root, kind): - if not (getattr(os, "O_SEARCH", 0) or getattr(os, "O_PATH", 0)): + if not (sys.platform == "darwin" or getattr(os, "O_SEARCH", 0) or getattr(os, "O_PATH", 0)): pytest.skip("Search/path-only directory descriptors are unavailable") if os.geteuid() == 0: pytest.skip("Root bypasses directory read permissions") @@ -1084,12 +1151,6 @@ def test_delete_rejects_replaced_root_before_validation(root, monkeypatch, allow (root / "victim").write_text("selected") files = FileHelper(root, allow_symlinks=allow) retained = root.parent / "retained" - if os.name == "nt": - with pytest.raises(PermissionError): - root.rename(retained) - assert (root / "victim").read_text() == "selected" - files.close() - return root.rename(retained) root.mkdir() (root / "victim").write_text("keep") @@ -1121,7 +1182,7 @@ def test_windows_delete_rejects_replaced_root_during_acquisition_portably( attempted = False @contextmanager - def handle(path, *, directory): + def handle(path, *, directory, pin=True): nonlocal attempted assert directory active.append(path) @@ -1139,6 +1200,7 @@ def handle(path, *, directory): backend.name = "nt" monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) + monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) with pytest.raises(FileHelperError, match="root changed"): with FileHelper(project) as files: files.delete("victim") @@ -1148,6 +1210,123 @@ def handle(path, *, directory): assert (retained / "project/victim").read_text() == "selected" +def test_windows_read_does_not_require_ancestor_listing_portably(root, monkeypatch): + (root / "victim").write_text("keep") + active = [] + + @contextmanager + def handle(path, *, directory, pin=True): + assert directory + if pin and path != root: + raise PermissionError("ancestor listing denied") + active.append((path, pin)) + try: + yield 123 + finally: + assert active.pop() == (path, pin) + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", handle) + monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"root"), raising=False) + with FileHelper(root) as files: + assert files.read_text("victim") == "keep" + with pytest.raises(PermissionError, match="ancestor listing denied"): + files.delete("victim") + assert files.read_text("victim") == "keep" + assert active == [] + + +@pytest.mark.parametrize("failure", ["open", "identity"]) +def test_windows_read_preserves_native_deletion_capability_error(root, monkeypatch, failure): + (root / "victim").write_text("keep") + active = [] + + @contextmanager + def denied(*args, **kwargs): + if failure == "open": + raise PermissionError("root reference denied") + active.append(123) + try: + yield 123 + finally: + active.pop() + + def identity(*args): + raise PermissionError("root reference denied") + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", denied) + monkeypatch.setattr(file_helper, "_windows_file_id", identity) + with FileHelper(root) as files: + assert active == [] + assert files.read_text("victim") == "keep" + with pytest.raises(PermissionError, match="root reference denied"): + files.delete("victim") + assert not files._root_cleanup.alive + + +def test_windows_delete_checks_retained_handle_identity_portably(root, monkeypatch): + (root / "victim").write_text("keep") + active = [] + identities = {} + + @contextmanager + def handle(path, *, directory, pin=True): + value = len(identities) + 1 + identities[value] = (1, b"replacement" if pin else b"original") + active.append(value) + try: + yield value + finally: + assert active.pop() == value + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", handle) + monkeypatch.setattr(file_helper, "_windows_file_id", lambda handle: identities[handle]) + with FileHelper(root) as files: + with pytest.raises(FileHelperError, match="root changed"): + files.delete("victim") + assert len(active) == 1 + assert files.read_text("victim") == "keep" + assert active == [] + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows directory ACL semantics") +@pytest.mark.parametrize("consumer", ["helper", "policy", "state"]) +def test_windows_readonly_helper_supports_traverse_only_ancestor(root, consumer): + ancestor = root / "ancestor" + project = ancestor / "project" + project.mkdir(parents=True) + (project / "victim").write_text("keep") + (project / ".specify").mkdir() + (project / ".specify/init-options.json").write_text("{}") + (project / ".specify/integration.json").write_text('{"integration": "claude"}') + user = subprocess.check_output(["whoami"], text=True).strip() + subprocess.run( + ["icacls", str(ancestor), "/deny", f"{user}:(RD)"], + check=True, capture_output=True, text=True, + ) + try: + with FileHelper(project) as files: + if consumer == "helper": + assert files.read_text("victim") == "keep" + elif consumer == "policy": + from specify_cli._file_policy import read_project_options + assert read_project_options(project) == {} + else: + from specify_cli.integration_state import try_read_integration_json + state, error = try_read_integration_json(project) + assert error is None and state is not None and state["integration"] == "claude" + with pytest.raises(PermissionError): + files.delete("victim") + assert files.read_text("victim") == "keep" + finally: + subprocess.run( + ["icacls", str(ancestor), "/remove:d", user], + check=True, capture_output=True, text=True, + ) + + @pytest.mark.parametrize("failure", [False, True]) @pytest.mark.parametrize("recursive", [False, True]) def test_windows_delete_holds_ancestors_until_io_completes_portably( @@ -1160,14 +1339,22 @@ def test_windows_delete_holds_ancestors_until_io_completes_portably( (parent / "victim").write_text("selected") alias = link(root / "alias", root / "target") active = [] + references = [] events = [] unlink = os.unlink rmdir = os.rmdir scan = os.scandir @contextmanager - def handle(path, *, directory): + def handle(path, *, directory, pin=True): assert directory + if not pin: + references.append(path) + try: + yield 123 + finally: + assert references.pop() == path + return active.append(path) try: yield 123 @@ -1205,6 +1392,7 @@ def remove_directory(path, *, dir_fd=None): backend.scandir = observe_scan monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) + monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) files = FileHelper(root, allow_symlinks=True) selected = alias / "nested" if recursive else alias / "nested/victim" if failure: @@ -1215,9 +1403,10 @@ def remove_directory(path, *, dir_fd=None): files.delete(selected, recursive=recursive) assert not (parent / "victim").exists() assert events - assert root.resolve() in active - files.close() assert active == [] + assert references == [root.resolve()] + files.close() + assert references == [] assert alias.is_symlink() From 2824a4c5e58911f00bf9e0b77a5ed73216a614f8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 10:28:39 -0600 Subject: [PATCH 06/16] fix: validate acquired root metadata and isolate native errors Compare metadata from the borrowed Windows root handle with the initial root observation. Use a temporary non-inheritable duplicate for native fstat without closing the original reference. Raise fresh copies of saved capability errors and cover acquisition restoration, error independence, and duplicate cleanup. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 7 ++++ src/specify_cli/_file_handles.py | 21 ++++++++++ src/specify_cli/file_helper.py | 6 ++- tests/test_file_handles.py | 58 +++++++++++++++++++++++++- tests/test_file_helper.py | 71 ++++++++++++++++++++++++++++++++ 5 files changed, 160 insertions(+), 3 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 06525065ae..73ae3621d8 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -154,8 +154,15 @@ the handle uses search-only access where available. On Windows, construction requests only root metadata access (`FILE_READ_ATTRIBUTES`) and permits delete sharing; it does not request directory-list access on the canonical ancestors. The retained reference records the full volume/file identity from that handle. +Before accepting it, construction compares Python-native metadata obtained from +the acquired handle with the initially observed root metadata. This comparison +does not reopen the root pathname, so replacement followed by restoration cannot +substitute a different retained object. The metadata query uses a temporary +non-inheritable duplicate and leaves the original reference open. If that reference or identity query is unavailable, the native capability error is deferred to `delete()` rather than preventing ordinary read/write operations. +The cached error remains traceback-free; each failed deletion raises an +independent shallow copy retaining the native error type and filesystem details. Windows deletion separately pins the canonical ancestry through the drive/share root without delete sharing, then compares the opened root's full identity with diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index 3bb53c9b9f..d3bef2a152 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -1,6 +1,7 @@ """Private platform handles for confined filesystem operations.""" import errno +import os from collections.abc import Iterator from contextlib import contextmanager from pathlib import Path @@ -82,3 +83,23 @@ class FileIdInfo(ctypes.Structure): if not info(handle, 18, ctypes.byref(identity), ctypes.sizeof(identity)): raise ctypes.WinError(ctypes.get_last_error()) return identity.volume, bytes(identity.file_id) + + +def _windows_stat(handle: int) -> os.stat_result: + """Get Python's native metadata without taking ownership of a borrowed handle.""" + import _winapi + import msvcrt + + process = _winapi.GetCurrentProcess() + duplicate = _winapi.DuplicateHandle( + process, handle, process, 0, False, _winapi.DUPLICATE_SAME_ACCESS + ) + try: + fd = msvcrt.open_osfhandle(duplicate, os.O_RDONLY | os.O_NOINHERIT) + except BaseException: + _winapi.CloseHandle(duplicate) + raise + try: + return os.fstat(fd) + finally: + os.close(fd) diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index e651454ca4..3c84bae863 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -2,6 +2,7 @@ from collections.abc import Iterator from contextlib import ExitStack, contextmanager +from copy import copy from dataclasses import dataclass, field import codecs import errno @@ -13,7 +14,7 @@ from typing import TypeVar from weakref import finalize -from specify_cli._file_handles import _windows_file_id, _windows_handle +from specify_cli._file_handles import _windows_file_id, _windows_handle, _windows_stat _PathType = TypeVar("_PathType", bound=PurePath) @@ -113,6 +114,7 @@ def __post_init__(self) -> None: handle = reference.enter_context( _windows_handle(canonical, directory=True, pin=False) ) + self._check_deletion_root(_windows_stat(handle)) windows_root_id = _windows_file_id(handle) except OSError as exc: # Other operations do not require deletion's handle capability. @@ -404,7 +406,7 @@ def _deletion_root(self) -> Iterator[int | Path]: self._require_open() if self._root_anchor is None: if self._root_anchor_error is not None: - raise self._root_anchor_error + raise copy(self._root_anchor_error) raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") self._check_deletion_root(self._canonical_root.stat()) if isinstance(self._root_anchor, Path): diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index 94f39ece30..f4f3a10c31 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -3,12 +3,15 @@ import ctypes from ctypes import wintypes import errno +import os from pathlib import Path +import sys from types import SimpleNamespace import pytest -from specify_cli._file_handles import _windows_file_id, _windows_handle +from specify_cli import _file_handles as handles +from specify_cli._file_handles import _windows_file_id, _windows_handle, _windows_stat @pytest.mark.parametrize("directory", [False, True]) @@ -95,6 +98,59 @@ def info(handle, kind, buffer, size): assert info.restype is wintypes.BOOL +@pytest.mark.parametrize("failure", [None, "duplicate", "convert", "stat"]) +def test_windows_stat_preserves_borrowed_handle_and_closes_duplicate(tmp_path, monkeypatch, failure): + expected = tmp_path.stat() + closed = [] + + def duplicate(source_process, handle, target_process, access, inherit, options): + assert source_process == target_process == 0x1FFFFFFFF + assert handle == 0x2FFFFFFFF + assert access == 0 and inherit is False and options == 2 + if failure == "duplicate": + raise OSError("duplicate failed") + return 456 + + def convert(handle, flags): + assert handle == 456 and flags == 128 + if failure == "convert": + raise OSError("convert failed") + return 789 + + def fstat(fd): + assert fd == 789 + if failure == "stat": + raise OSError("stat failed") + return expected + + monkeypatch.setitem(sys.modules, "_winapi", SimpleNamespace( + GetCurrentProcess=lambda: 0x1FFFFFFFF, + DuplicateHandle=duplicate, + DUPLICATE_SAME_ACCESS=2, + CloseHandle=lambda handle: closed.append(("handle", handle)), + )) + monkeypatch.setitem(sys.modules, "msvcrt", SimpleNamespace(open_osfhandle=convert)) + monkeypatch.setattr(handles, "os", SimpleNamespace( + O_RDONLY=0, O_NOINHERIT=128, fstat=fstat, close=lambda fd: closed.append(("fd", fd)), + )) + if failure: + with pytest.raises(OSError, match=f"{failure} failed"): + _windows_stat(0x2FFFFFFFF) + else: + assert _windows_stat(0x2FFFFFFFF) is expected + assert closed == { + None: [("fd", 789)], "duplicate": [], "convert": [("handle", 456)], "stat": [("fd", 789)], + }[failure] + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows directory-handle metadata") +def test_native_windows_stat_matches_path_metadata_without_closing_reference(tmp_path): + with _windows_handle(tmp_path, directory=True, pin=False) as handle: + identity = _windows_file_id(handle) + assert os.path.samestat(tmp_path.stat(), _windows_stat(handle)) + assert _windows_file_id(handle) == identity + + @pytest.mark.parametrize("directory", [False, True]) def test_windows_handle_declares_complete_createfile_prototype(tmp_path, monkeypatch, directory): closed = [] diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 2274b46ae6..fe184accda 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1201,6 +1201,7 @@ def handle(path, *, directory, pin=True): monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) + monkeypatch.setattr(module, "_windows_stat", lambda _: project.stat()) with pytest.raises(FileHelperError, match="root changed"): with FileHelper(project) as files: files.delete("victim") @@ -1210,6 +1211,72 @@ def handle(path, *, directory, pin=True): assert (retained / "project/victim").read_text() == "selected" +def test_windows_constructor_rejects_replaced_then_restored_root(root, monkeypatch): + project = root / "project" + replacement = root / "replacement" + saved = root / "saved" + project.mkdir() + replacement.mkdir() + (project / "victim").write_text("selected") + (replacement / "victim").write_text("keep") + acquired = {} + active = [] + + @contextmanager + def handle(path, *, directory, pin=True): + assert path == project and directory and not pin + project.rename(saved) + replacement.rename(project) + acquired[123] = project.stat() + project.rename(replacement) + saved.rename(project) + active.append(123) + try: + yield 123 + finally: + assert active.pop() == 123 + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", handle) + monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"replacement")) + monkeypatch.setattr(file_helper, "_windows_stat", lambda value: acquired[value], raising=False) + with pytest.raises(FileHelperError, match="root changed"): + with FileHelper(project): + pytest.fail("A handle to the replacement root must not be accepted") + assert active == [] + assert (project / "victim").read_text() == "selected" + assert (replacement / "victim").read_text() == "keep" + + +def test_windows_cached_capability_error_has_independent_tracebacks(root, monkeypatch): + native = PermissionError(errno.EACCES, "reference denied", "source", 5, "destination") + if not hasattr(native, "winerror"): + native.winerror = 5 + + @contextmanager + def denied(*args, **kwargs): + raise native + yield + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", denied) + with FileHelper(root) as files: + errors = [] + for _ in range(3): + with pytest.raises(PermissionError) as raised: + files.delete("victim") + errors.append(raised.value) + assert files._root_anchor_error is native + assert native.__traceback__ is None + assert len({id(error) for error in errors}) == 3 + for error in errors: + assert error is not native and type(error) is type(native) + assert (error.errno, error.strerror, error.filename, error.filename2, error.winerror) == ( + native.errno, native.strerror, native.filename, native.filename2, native.winerror, + ) + assert error.__traceback__ is not None + + def test_windows_read_does_not_require_ancestor_listing_portably(root, monkeypatch): (root / "victim").write_text("keep") active = [] @@ -1228,6 +1295,7 @@ def handle(path, *, directory, pin=True): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"root"), raising=False) + monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) with FileHelper(root) as files: assert files.read_text("victim") == "keep" with pytest.raises(PermissionError, match="ancestor listing denied"): @@ -1257,6 +1325,7 @@ def identity(*args): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", denied) monkeypatch.setattr(file_helper, "_windows_file_id", identity) + monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) with FileHelper(root) as files: assert active == [] assert files.read_text("victim") == "keep" @@ -1283,6 +1352,7 @@ def handle(path, *, directory, pin=True): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) monkeypatch.setattr(file_helper, "_windows_file_id", lambda handle: identities[handle]) + monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) with FileHelper(root) as files: with pytest.raises(FileHelperError, match="root changed"): files.delete("victim") @@ -1393,6 +1463,7 @@ def remove_directory(path, *, dir_fd=None): monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) + monkeypatch.setattr(module, "_windows_stat", lambda _: root.stat()) files = FileHelper(root, allow_symlinks=True) selected = alias / "nested" if recursive else alias / "nested/victim" if failure: From 0108a882f39b1056e19d9aaa004fe631d440c9f8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 10:52:01 -0600 Subject: [PATCH 07/16] fix: preserve the Windows native error code when copying Restore OSError.winerror explicitly after shallow copying, since Windows omits that built-in slot from its copy protocol. Add a portable regression for lossy copies while keeping independent traceback ownership. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/file_helper.py | 6 +++++- tests/test_file_helper.py | 14 +++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 3c84bae863..fa4efb6d20 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -406,7 +406,11 @@ def _deletion_root(self) -> Iterator[int | Path]: self._require_open() if self._root_anchor is None: if self._root_anchor_error is not None: - raise copy(self._root_anchor_error) + error = copy(self._root_anchor_error) + # OSError's copy protocol omits its native Windows error slot. + if hasattr(self._root_anchor_error, "winerror"): + error.winerror = self._root_anchor_error.winerror + raise error raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") self._check_deletion_root(self._canonical_root.stat()) if isinstance(self._root_anchor, Path): diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index fe184accda..556b5609a0 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1248,10 +1248,22 @@ def handle(path, *, directory, pin=True): assert (replacement / "victim").read_text() == "keep" -def test_windows_cached_capability_error_has_independent_tracebacks(root, monkeypatch): +@pytest.mark.parametrize("simulate_missing_winerror", [False, True]) +def test_windows_cached_capability_error_has_independent_tracebacks( + root, monkeypatch, simulate_missing_winerror +): native = PermissionError(errno.EACCES, "reference denied", "source", 5, "destination") if not hasattr(native, "winerror"): native.winerror = 5 + if simulate_missing_winerror: + original_copy = file_helper.copy + + def copy_without_winerror(error): + result = original_copy(error) + result.winerror = None + return result + + monkeypatch.setattr(file_helper, "copy", copy_without_winerror) @contextmanager def denied(*args, **kwargs): From 2fd99080982dab17fc286bed4f92332c3908a712 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 11:54:06 -0600 Subject: [PATCH 08/16] fix: anchor Windows parent lookup during deletion Pin accessed Windows directories before inspecting children or reading link targets, reusing the anchored component walker. Exercise swaps only during pinned root acquisition, distinguish replacement handles, and cover parent replacement for file and recursive removal while preserving native error contracts. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 8 ++- src/specify_cli/file_helper.py | 40 +++++++---- tests/test_file_helper.py | 125 +++++++++++++++++++++++++++++++-- 3 files changed, 153 insertions(+), 20 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 73ae3621d8..63cf89c733 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -170,7 +170,13 @@ the retained reference. Those operation-time handles require both `FILE_LIST_DIRECTORY` and `FILE_READ_ATTRIBUTES` on every canonical ancestor, including those above the helper root, and reject reparse points. An ACL allowing traversal but not listing can therefore prevent deletion while still allowing -bootstrap reads. These locks are released at the end of deletion; the +bootstrap reads. Windows deletion also retains non-reparse handles to accessed +parent directories throughout link resolution, before classifying child entries +or reading link targets. Resolving a contained parent link does not consult +metadata through an unpinned replacement parent. These accessed directories also +require directory-list/read-attribute access. The resolution handles are released +after selection; resolved mutation parents and root ancestry remain pinned during +preflight and removal. These locks are released at the end of deletion; the metadata-only reference remains until the helper closes. Automatic finalization is a compatibility fallback, not a substitute for deterministic cleanup. Bootstrap configuration and managed integration-state consumers close diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index fa4efb6d20..137fc42e3d 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -54,20 +54,31 @@ class UnsupportedPathError(FileHelperError): @dataclass class _WalkDirectories: root: Path - root_fd: int + root_handle: int | Path flags: int resources: ExitStack - opened: dict[Path, int] = field(default_factory=dict) + opened: dict[Path, int | Path] = field(default_factory=dict) - def directory(self, path: Path) -> int: + def directory(self, path: Path) -> int | Path: if path == self.root: - return self.root_fd + return self.root_handle path.relative_to(self.root) if path not in self.opened: parent = self.directory(path.parent) - fd = os.open(path.name, self.flags, dir_fd=parent) - self.resources.callback(os.close, fd) - self.opened[path] = fd + if isinstance(parent, Path): + try: + self.resources.enter_context(_windows_handle(path, directory=True)) + except OSError as exc: + if exc.errno == errno.EINVAL and getattr(exc, "winerror", None) is None: + raise NotADirectoryError( + errno.ENOTDIR, "Expected a directory", str(path) + ) from exc + raise + self.opened[path] = path + else: + fd = os.open(path.name, self.flags, dir_fd=parent) + self.resources.callback(os.close, fd) + self.opened[path] = fd return self.opened[path] @@ -195,7 +206,8 @@ def _walk( ) -> Path: current = self._canonical_root for index, part in enumerate(parts): - directory_fd = directories.directory(current) if directories is not None else None + directory = directories.directory(current) if directories is not None else None + directory_fd = directory if isinstance(directory, int) else None if directories is None and not current.is_dir(): if not (allow_missing and not current.exists()): raise NotADirectoryError(current) @@ -241,7 +253,7 @@ def _walk( def _path( self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True, - root_fd: int | None = None, + root_handle: int | Path | None = None, ) -> Path: self._require_open() original = Path(path) @@ -250,8 +262,11 @@ def _path( raise PathEscapeError(f"Parent traversal is not an operation path: {original}") with ExitStack() as resources: directories = ( - _WalkDirectories(self._canonical_root, root_fd, self._directory_flags(), resources) - if root_fd is not None else None + _WalkDirectories( + self._canonical_root, root_handle, + 0 if isinstance(root_handle, Path) else self._directory_flags(), resources, + ) + if root_handle is not None else None ) return self._walk( parts, allow_missing=allow_missing, follow_leaf=follow_leaf, directories=directories @@ -508,8 +523,7 @@ def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: def delete(self, path: Path | str, *, recursive: bool = False) -> None: """Delete an entry; recursive deletion never follows descendant links.""" with self._deletion_root() as root: - root_fd = None if isinstance(root, Path) else root - destination = self._path(path, follow_leaf=False, root_fd=root_fd) + destination = self._path(path, follow_leaf=False, root_handle=root) if destination == self._canonical_root: raise FileHelperError(f"Refusing to delete the established root: {self.root}") with self._deletion_directory(destination.parent, root=root) as parent: diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 556b5609a0..e01f6bd2e7 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1179,20 +1179,25 @@ def test_windows_delete_rejects_replaced_root_during_acquisition_portably( (outside / "project/victim").write_text("keep") retained = root / "retained" active = [] + identities = {} + metadata = {} attempted = False @contextmanager def handle(path, *, directory, pin=True): nonlocal attempted assert directory + value = len(identities) + 1 active.append(path) try: - if path == project: + if pin and path == project: # Simulate a replacement just before the root handle is acquired. attempted = True ancestor.rename(retained) link(ancestor, outside) - yield 123 + metadata[value] = path.stat() + identities[value] = (1, b"replacement" if attempted else b"original") + yield value finally: assert active.pop() == path @@ -1200,17 +1205,125 @@ def handle(path, *, directory, pin=True): backend.name = "nt" monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) - monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) - monkeypatch.setattr(module, "_windows_stat", lambda _: project.stat()) - with pytest.raises(FileHelperError, match="root changed"): - with FileHelper(project) as files: + monkeypatch.setattr(module, "_windows_file_id", lambda value: identities[value]) + monkeypatch.setattr(module, "_windows_stat", lambda value: metadata[value]) + with FileHelper(project) as files: + assert not attempted + with pytest.raises(FileHelperError, match="root changed"): files.delete("victim") + assert active == [project] assert attempted assert active == [] assert (outside / "project/victim").read_text() == "keep" assert (retained / "project/victim").read_text() == "selected" +@pytest.mark.parametrize("kind", ["file", "directory", "recursive"]) +def test_windows_link_resolution_pins_accessed_parents_portably( + root, link, monkeypatch, kind +): + parent = root / "parent" + parent.mkdir() + outside = root.parent / "outside" + outside.mkdir() + selected = root / "selected" + unrelated = root / "unrelated" + for directory in (selected, unrelated): + directory.mkdir() + if kind == "file": + (directory / "victim").write_text("keep") + else: + (directory / "victim").mkdir() + if kind == "recursive": + (directory / "victim/child").write_text("keep") + link(parent / "alias", selected) + link(outside / "alias", unrelated) + retained = root / "retained" + pinned = [] + attempted = False + blocked = False + lstat = Path.lstat + + @contextmanager + def handle(path, *, directory, pin=True): + assert directory + if pin: + pinned.append(path) + try: + yield 123 + finally: + if pin: + assert pinned.pop() == path + + def exchange(path, **kwargs): + nonlocal attempted, blocked + if path == parent / "alias" and not attempted: + attempted = True + blocked = parent in pinned + if not blocked: + parent.rename(retained) + link(parent, outside) + return lstat(path, **kwargs) + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", handle) + monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"root")) + monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) + monkeypatch.setattr(Path, "lstat", exchange) + with FileHelper(root, allow_symlinks=True) as files: + files.delete("parent/alias/victim", recursive=kind == "recursive") + assert attempted + assert (unrelated / "victim").exists() + assert not (selected / "victim").exists() + assert blocked + assert (parent / "alias").is_symlink() + assert pinned == [] + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows directory-handle semantics") +def test_native_windows_link_resolution_blocks_accessed_parent_exchange(root, link, monkeypatch): + parent = root / "parent" + parent.mkdir() + selected = root / "selected" + selected.mkdir() + (selected / "victim").write_text("selected") + link(parent / "alias", selected) + attempted = False + lstat = Path.lstat + + def exchange(path, **kwargs): + nonlocal attempted + if path == parent / "alias" and not attempted: + attempted = True + with pytest.raises(PermissionError): + parent.rename(root / "retained") + return lstat(path, **kwargs) + + monkeypatch.setattr(Path, "lstat", exchange) + with FileHelper(root, allow_symlinks=True) as files: + files.delete("parent/alias/victim") + assert attempted + assert not (selected / "victim").exists() + parent.rename(root / "retained") + + +@pytest.mark.parametrize("error_number", [errno.EINVAL, errno.EACCES]) +def test_windows_walk_preserves_parent_type_and_access_errors_portably( + root, monkeypatch, error_number +): + @contextmanager + def denied(*args, **kwargs): + raise OSError(error_number, "directory unavailable") + yield + + monkeypatch.setattr(file_helper, "_windows_handle", denied) + expected = NotADirectoryError if error_number == errno.EINVAL else PermissionError + with file_helper.ExitStack() as resources: + directories = file_helper._WalkDirectories(root, root, 0, resources) + with pytest.raises(expected): + directories.directory(root / "child") + + def test_windows_constructor_rejects_replaced_then_restored_root(root, monkeypatch): project = root / "project" replacement = root / "replacement" From 3877f0d9ba20c7f29d0f99607bfe758729309cd1 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:54:35 -0600 Subject: [PATCH 09/16] fix: preserve recursive directory permission behavior Open enumerated children directly with readable no-follow directory handles instead of requiring a search-only open and dot reopening. Cover readable empty directories without search permission, nested preflight behavior, and preserved unreadable/nonempty failures. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 6 ++++ src/specify_cli/file_helper.py | 7 ++-- tests/test_file_helper.py | 62 +++++++++++++++++++++++++++++++--- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 63cf89c733..65efd8cb06 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -263,6 +263,12 @@ descriptor-relative or change their documented concurrency limitations. being enumerated; systems without search/path-only flags retain read-capable traversal. Recursive enumeration still requires directory read permission, and removal still requires the operating system's normal directory permissions. + Preflight opens the enumerated child directly as readable, relative to its + anchored parent, without a search-only open or `"."` reopening first. An empty + readable directory therefore does not need its own search permission to be + removed; accessing children in a nonempty directory still requires search + permission. The parent still needs the permissions required to remove its + directory entry. On systems using the read-capable fallback, root establishment also requires read permission on the root. Component resolution retains opened parent descriptors instead of reopening diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 137fc42e3d..73d04c39ce 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -491,10 +491,9 @@ def _deletion_plan( directory = self._deletion_kind(path, self._deletion_entry_mode(path, parent=parent)) children: tuple[_DeletionEntry, ...] = () if directory and recursive: - with self._child_deletion_directory(parent, path.name) as child_directory: - with self._child_deletion_directory(child_directory, ".", readable=True) as readable: - with os.scandir(readable) as entries: - names = sorted(entry.name for entry in entries) + with self._child_deletion_directory(parent, path.name, readable=True) as child_directory: + with os.scandir(child_directory) as entries: + names = sorted(entry.name for entry in entries) child_plans = [] for name in names: child_plans.append( diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index e01f6bd2e7..be2a7f490b 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1115,17 +1115,69 @@ def test_delete_through_write_execute_only_parent(root, parent_is_root, kind): @pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") -def test_recursive_delete_still_requires_read_permission_for_enumeration(root): +@pytest.mark.parametrize("mode", [0o400, 0o600, 0o444, 0o644]) +@pytest.mark.parametrize("nested", [False, True]) +@pytest.mark.parametrize("allow", [False, True]) +def test_recursive_delete_readable_empty_directory_without_search(root, mode, nested, allow): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory permission checks") + tree = root / "tree" + tree.mkdir() + restricted = tree + if nested: + (tree / "a-first").write_text("selected") + restricted = tree / "z-empty" + restricted.mkdir() + restricted.chmod(mode) + try: + with FileHelper(root, allow_symlinks=allow) as files: + files.delete(tree, recursive=True) + assert not tree.exists() + finally: + if restricted.exists(): + restricted.chmod(0o700) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") +@pytest.mark.parametrize("mode", [0o400, 0o600]) +@pytest.mark.parametrize("allow", [False, True]) +def test_recursive_delete_nonempty_directory_without_search_fails_preflight(root, mode, allow): + if os.geteuid() == 0: + pytest.skip("Root bypasses directory permission checks") + tree = root / "tree" + restricted = tree / "z-restricted" + restricted.mkdir(parents=True) + (tree / "a-first").write_text("keep") + (restricted / "victim").write_text("keep") + restricted.chmod(mode) + try: + with FileHelper(root, allow_symlinks=allow) as files: + with pytest.raises(PermissionError): + files.delete(tree, recursive=True) + assert (tree / "a-first").read_text() == "keep" + finally: + restricted.chmod(0o700) + assert (restricted / "victim").read_text() == "keep" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions") +@pytest.mark.parametrize("nonempty", [False, True]) +def test_recursive_delete_still_requires_read_permission_for_enumeration(root, nonempty): if os.geteuid() == 0: pytest.skip("Root bypasses directory read permissions") tree = root / "tree" tree.mkdir() - (tree / "victim").write_text("keep") + if nonempty: + (tree / "victim").write_text("keep") tree.chmod(0o300) try: - with pytest.raises(PermissionError): - FileHelper(root).delete(tree, recursive=True) - assert (tree / "victim").read_text() == "keep" + with FileHelper(root) as files: + with pytest.raises(PermissionError): + files.delete(tree, recursive=True) + if nonempty: + assert (tree / "victim").read_text() == "keep" + else: + assert tree.is_dir() finally: tree.chmod(0o700) From 55432cb75de8520e31637c40cf26e7cc9523a49f Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:57:51 -0600 Subject: [PATCH 10/16] fix: bound descriptors during POSIX deletion traversal Keep only the current POSIX traversal descriptor while retaining the root anchor, and reuse bounded traversal for final parent acquisition. Re-anchor link-target jumps and parent components through the retained root, preserve Windows ancestry pinning, and cover deep paths under a 64-descriptor process limit. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 10 ++- src/specify_cli/file_helper.py | 67 ++++++++++++++----- tests/test_file_helper.py | 116 +++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 19 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 056c97bc69..ed1d3d8800 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -298,8 +298,14 @@ descriptor-relative or change their documented concurrency limitations. directory entry. On systems using the read-capable fallback, root establishment also requires read permission on the root. - Component resolution retains opened parent descriptors instead of reopening - every prefix. Recursive preflight and mutation carry each parent descriptor + POSIX component resolution and final parent anchoring retain only the current + traversal descriptor, opening its child before releasing the preceding one. + Their live descriptor count is bounded independently of operation-path depth, + in addition to the helper's retained root. Link-target jumps and `..` are + re-anchored through that root rather than following an opened directory's + physical parent after a rename. Windows retains the accessed ancestor handles + because its path-based access requires those directories to stay pinned. + Recursive preflight and mutation carry each parent descriptor through its children, opening directories a bounded number of times across the two passes. Descriptor-open counts grow linearly with visited directories and operation-path components; recursive descriptor lifetimes grow with tree diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 7390ec7b2c..7df0f3090c 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -60,28 +60,54 @@ class _WalkDirectories: root_handle: int | Path flags: int resources: ExitStack - opened: dict[Path, int | Path] = field(default_factory=dict) + opened: dict[Path, Path] = field(default_factory=dict) + current: tuple[Path, int] | None = field(default=None, init=False) + + def __post_init__(self) -> None: + if isinstance(self.root_handle, int): + self.resources.callback(self._release_current) + + def _release_current(self) -> None: + current, self.current = self.current, None + if current is not None: + os.close(current[1]) + + def _posix_directory(self, path: Path, root_fd: int) -> int: + current_path, parent = self.current or (self.root, root_fd) + if path == current_path: + return parent + if path.parent != current_path: + # Re-anchor jumps, including "..", rather than following a moved + # directory's physical parent outside the retained root. + self._release_current() + current_path, parent = self.root, root_fd + for part in path.relative_to(current_path).parts: + child = os.open(part, self.flags, dir_fd=parent) + previous = self.current + current_path = current_path / part + self.current = (current_path, child) + if previous is not None: + os.close(previous[1]) + parent = child + return parent def directory(self, path: Path) -> int | Path: + path.relative_to(self.root) + if isinstance(self.root_handle, int): + return self._posix_directory(path, self.root_handle) if path == self.root: return self.root_handle - path.relative_to(self.root) if path not in self.opened: - parent = self.directory(path.parent) - if isinstance(parent, Path): - try: - self.resources.enter_context(_windows_handle(path, directory=True)) - except OSError as exc: - if exc.errno == errno.EINVAL and getattr(exc, "winerror", None) is None: - raise NotADirectoryError( - errno.ENOTDIR, "Expected a directory", str(path) - ) from exc - raise - self.opened[path] = path - else: - fd = os.open(path.name, self.flags, dir_fd=parent) - self.resources.callback(os.close, fd) - self.opened[path] = fd + self.directory(path.parent) + try: + self.resources.enter_context(_windows_handle(path, directory=True)) + except OSError as exc: + if exc.errno == errno.EINVAL and getattr(exc, "winerror", None) is None: + raise NotADirectoryError( + errno.ENOTDIR, "Expected a directory", str(path) + ) from exc + raise + self.opened[path] = path return self.opened[path] @@ -513,6 +539,13 @@ def _child_deletion_directory( @contextmanager def _deletion_directory(self, path: Path, *, root: int | Path) -> Iterator[int | Path]: parts = path.relative_to(self._canonical_root).parts + if isinstance(root, int): + with ExitStack() as resources: + directories = _WalkDirectories( + self._canonical_root, root, self._directory_flags(), resources + ) + yield directories.directory(path) + return with ExitStack() as resources: parent = root for part in parts: diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 4ce69f8492..bc8670f088 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -602,6 +602,122 @@ def exchange(path, flags, *args, **kwargs): assert (retained / "victim").read_text() == "selected" +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("stage", ["resolve", "anchor", "delete"]) +@pytest.mark.parametrize("allow", [False, True]) +def test_deep_nonrecursive_delete_with_low_descriptor_limit(root, stage, allow): + import resource + + if resource.getrlimit(resource.RLIMIT_NOFILE)[1] < 64: + pytest.skip("Hard descriptor limit is below the regression budget") + deepest = root / "tree" + for _ in range(80): + deepest /= "d" + deepest.mkdir(parents=True) + (deepest / "direct").write_text("control") + (deepest / "victim").write_text("selected") + script = """ +import os +from pathlib import Path +import resource +import sys +from specify_cli.file_helper import FileHelper + +root, deepest = map(Path, sys.argv[1:3]) +stage, allow = sys.argv[3], sys.argv[4] == "True" +_, hard = resource.getrlimit(resource.RLIMIT_NOFILE) +resource.setrlimit(resource.RLIMIT_NOFILE, (64, hard)) +(deepest / "direct").unlink() +with FileHelper(root, allow_symlinks=allow) as files: + if stage == "resolve": + with files._deletion_root() as anchor: + assert files._path(deepest / "victim", follow_leaf=False, root_handle=anchor) == deepest / "victim" + files.delete(deepest / "victim") + elif stage == "anchor": + with files._deletion_root() as anchor: + with files._deletion_directory(deepest, root=anchor) as parent: + os.unlink("victim", dir_fd=parent) + else: + files.delete(deepest / "victim") +assert not (deepest / "victim").exists() +""" + result = subprocess.run( + [sys.executable, "-c", script, str(root), str(deepest), stage, str(allow)], + capture_output=True, text=True, timeout=60, check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert not (deepest / "direct").exists() + assert not (deepest / "victim").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("target_kind", ["absolute", "parent-traversal"]) +def test_deep_contained_link_delete_with_low_descriptor_limit(root, link, target_kind): + import resource + + if resource.getrlimit(resource.RLIMIT_NOFILE)[1] < 64: + pytest.skip("Hard descriptor limit is below the regression budget") + deepest = root / "tree" + for _ in range(80): + deepest /= "d" + deepest.mkdir(parents=True) + (deepest / "victim").write_text("selected") + target = deepest if target_kind == "absolute" else ( + deepest.relative_to(root) / ".." / "d" + ) + alias = link(root / "alias", target) + script = """ +from pathlib import Path +import resource +import sys +from specify_cli.file_helper import FileHelper + +root = Path(sys.argv[1]) +_, hard = resource.getrlimit(resource.RLIMIT_NOFILE) +resource.setrlimit(resource.RLIMIT_NOFILE, (64, hard)) +with FileHelper(root, allow_symlinks=True) as files: + files.delete("alias/victim") +assert (root / "alias").is_symlink() +""" + result = subprocess.run( + [sys.executable, "-c", script, str(root)], + capture_output=True, text=True, timeout=60, check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert alias.is_symlink() + assert not (deepest / "victim").exists() + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor-relative parent traversal") +def test_link_parent_traversal_reanchors_after_directory_moves_outside_root(root, link, monkeypatch): + branch = root / "branch" + branch.mkdir() + (root / "selected").mkdir() + (root / "selected/victim").write_text("selected") + outside = root.parent / "outside" + (outside / "selected").mkdir(parents=True) + (outside / "selected/victim").write_text("keep") + alias = link(root / "alias", "branch/../selected") + original = os.open + attempted = False + + def move_after_open(path, flags, *args, **kwargs): + nonlocal attempted + fd = original(path, flags, *args, **kwargs) + if path == "branch" and not attempted: + attempted = True + branch.rename(outside / "moved") + return fd + + monkeypatch.setattr(os, "open", move_after_open) + with FileHelper(root, allow_symlinks=True) as files: + files.delete(alias / "victim") + assert attempted + assert not (root / "selected/victim").exists() + assert (outside / "selected/victim").read_text() == "keep" + assert alias.is_symlink() + + @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") def test_recursive_delete_preserves_deep_tree_support(root): import resource From ddd74cd98be3d8dc6da49eaef967804cc5553958 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 15:18:59 -0600 Subject: [PATCH 11/16] fix: bound recursive POSIX deletion handles Use explicit preflight and postorder worklists with bounded directory caches. Retain the selected parent and root, reopen evicted prefixes without following links, and preserve complete preflight and Windows ancestry locks. Cover both recursive phases under a 64-descriptor process limit, cache eviction, cleanup, and measured open-work budgets. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 19 ++-- src/specify_cli/file_helper.py | 110 ++++++++++++++++++++++ tests/test_file_helper.py | 162 ++++++++++++++++++++++++++++++--- 3 files changed, 274 insertions(+), 17 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index ed1d3d8800..7e15feb180 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -305,12 +305,19 @@ descriptor-relative or change their documented concurrency limitations. re-anchored through that root rather than following an opened directory's physical parent after a rename. Windows retains the accessed ancestor handles because its path-based access requires those directories to stay pinned. - Recursive preflight and mutation carry each parent descriptor - through its children, opening directories a bounded number of times across - the two passes. Descriptor-open counts grow linearly with visited directories - and operation-path components; recursive descriptor lifetimes grow with tree - depth rather than total tree size. The complete plan is still preflighted - before any removal. + POSIX recursive preflight and mutation use explicit worklists and separate + eight-directory handle caches. The selected parent and established root remain + anchored outside those caches. An uncached parent is reacquired component by + component from the nearest retained prefix with no-follow flags, releasing + least-recently-used handles as the cache fills. Removed directories are evicted, + and preflight handles are not reused for mutation. Live descriptors are bounded + independently of recursive depth, including transient opens and enumeration; + worklist and complete-plan memory still scale with the selected tree. + Reacquiring evicted prefixes trades additional opens for that fixed handle + budget. Total opens can grow with the sum of visited path lengths, rather than + being unconditionally linear in the directory count. Windows retains its + ancestry-locking traversal instead of evicting handles needed for path-based + operations. The complete plan is still preflighted before any removal. - `symlink(target, path)` creates a link exclusively. Relative targets are interpreted relative to the link parent, not the helper root. diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 7df0f3090c..11362ada49 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -6,6 +6,7 @@ import stat import sys import tempfile +from collections import OrderedDict from collections.abc import Iterator from contextlib import ExitStack, contextmanager from copy import copy @@ -26,6 +27,7 @@ and os.scandir in os.supports_fd ) _OVERWRITE_DIR_FD_SUPPORTED = os.open in os.supports_dir_fd +_DELETE_DIRECTORY_CACHE_SIZE = 8 def _strip_extended_length_prefix(path: _PathType) -> _PathType: @@ -118,6 +120,48 @@ class _DeletionEntry: children: tuple["_DeletionEntry", ...] = () +@dataclass +class _DeletionDirectories: + root: Path + root_fd: int + flags: int + resources: ExitStack + opened: OrderedDict[Path, int] = field(default_factory=OrderedDict) + + def __post_init__(self) -> None: + self.resources.callback(self.close) + + def close(self) -> None: + while self.opened: + _, fd = self.opened.popitem() + os.close(fd) + + def discard(self, path: Path) -> None: + fd = self.opened.pop(path, None) + if fd is not None: + os.close(fd) + + def directory(self, path: Path) -> int: + path.relative_to(self.root) + ancestor = path + suffix = [] + while ancestor != self.root and ancestor not in self.opened: + suffix.append(ancestor.name) + ancestor = ancestor.parent + parent = self.root_fd if ancestor == self.root else self.opened[ancestor] + if ancestor in self.opened: + self.opened.move_to_end(ancestor) + for name in reversed(suffix): + child = os.open(name, self.flags, dir_fd=parent) + ancestor = ancestor / name + self.opened[ancestor] = child + if len(self.opened) > _DELETE_DIRECTORY_CACHE_SIZE: + _, oldest = self.opened.popitem(last=False) + os.close(oldest) + parent = child + return parent + + @dataclass(frozen=True) class FileHelper: """Own operations beneath one trusted root with an immutable link policy. @@ -571,6 +615,8 @@ def _deletion_kind(self, path: Path, mode: int) -> bool: def _deletion_plan( self, path: Path, *, recursive: bool, parent: int | Path ) -> _DeletionEntry: + if isinstance(parent, int): + return self._posix_deletion_plan(path, recursive=recursive, parent=parent) directory = self._deletion_kind(path, self._deletion_entry_mode(path, parent=parent)) children: tuple[_DeletionEntry, ...] = () if directory and recursive: @@ -585,7 +631,43 @@ def _deletion_plan( children = tuple(child_plans) return _DeletionEntry(path, directory, children) + def _posix_deletion_plan( + self, path: Path, *, recursive: bool, parent: int + ) -> _DeletionEntry: + pending: list[tuple[Path, list[str] | None]] = [(path, None)] + completed: dict[Path, _DeletionEntry] = {} + with ExitStack() as resources: + directories = _DeletionDirectories( + path.parent, parent, self._directory_flags(), resources + ) + while pending: + entry, children = pending.pop() + if children is not None: + completed[entry] = _DeletionEntry( + entry, True, tuple(completed.pop(entry / name) for name in children) + ) + continue + entry_parent = directories.directory(entry.parent) + directory = self._deletion_kind( + entry, self._deletion_entry_mode(entry, parent=entry_parent) + ) + if not (directory and recursive): + completed[entry] = _DeletionEntry(entry, directory) + continue + with self._child_deletion_directory( + entry_parent, entry.name, readable=True + ) as readable: + with os.scandir(readable) as entries: + names = sorted(child.name for child in entries) + pending.append((entry, names)) + for name in reversed(names): + pending.append((entry / name, None)) + return completed[path] + def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: + if isinstance(parent, int): + self._posix_delete_plan(entry, parent=parent) + return directory = self._deletion_kind( entry.path, self._deletion_entry_mode(entry.path, parent=parent) ) @@ -602,6 +684,34 @@ def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: else: os.unlink(leaf, dir_fd=directory_fd) + def _posix_delete_plan(self, entry: _DeletionEntry, *, parent: int) -> None: + pending = [(entry, False)] + with ExitStack() as resources: + # Do not reuse preflight handles: parent replacements must be checked + # afresh before any removal. + directories = _DeletionDirectories( + entry.path.parent, parent, self._directory_flags(), resources + ) + while pending: + selected, ready = pending.pop() + selected_parent = directories.directory(selected.path.parent) + directory = self._deletion_kind( + selected.path, + self._deletion_entry_mode(selected.path, parent=selected_parent), + ) + if directory != selected.directory: + raise FileHelperError(f"Destination changed during deletion: {selected.path}") + if directory and selected.children and not ready: + pending.append((selected, True)) + for child in reversed(selected.children): + pending.append((child, False)) + continue + if directory: + os.rmdir(selected.path.name, dir_fd=selected_parent) + directories.discard(selected.path) + else: + os.unlink(selected.path.name, dir_fd=selected_parent) + def delete(self, path: Path | str, *, recursive: bool = False) -> None: """Delete an entry; recursive deletion never follows descendant links.""" with self._deletion_root() as root: diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index bc8670f088..61ae987052 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -602,6 +602,118 @@ def exchange(path, flags, *args, **kwargs): assert (retained / "victim").read_text() == "selected" +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("stage", ["preflight", "mutation", "delete"]) +@pytest.mark.parametrize("depth", [80, 160]) +@pytest.mark.parametrize("allow", [False, True]) +def test_recursive_delete_with_low_descriptor_limit(root, stage, depth, allow): + import resource + + if resource.getrlimit(resource.RLIMIT_NOFILE)[1] < 64: + pytest.skip("Hard descriptor limit is below the regression budget") + deepest = root / "tree" + for _ in range(depth): + deepest /= "d" + deepest.mkdir(parents=True) + (deepest / "direct").write_text("control") + (deepest / "victim").write_text("selected") + if allow: + (root / "alias").symlink_to(".", target_is_directory=True) + script = """ +import os +from pathlib import Path +import resource +import sys +from specify_cli.file_helper import FileHelper + +root, deepest = map(Path, sys.argv[1:3]) +stage, allow = sys.argv[3], sys.argv[4] == "True" +_, hard = resource.getrlimit(resource.RLIMIT_NOFILE) +(deepest / "direct").unlink() +original_open, original_close = os.open, os.close +active = set() +peak = 0 + +def observe_open(*args, **kwargs): + global peak + fd = original_open(*args, **kwargs) + active.add(fd) + peak = max(peak, len(active)) + return fd + +def observe_close(fd): + original_close(fd) + active.discard(fd) + +with FileHelper(root, allow_symlinks=allow) as files: + with files._deletion_root() as anchor: + if stage == "mutation": + plan = files._deletion_plan(root / "tree", recursive=True, parent=anchor) + resource.setrlimit(resource.RLIMIT_NOFILE, (64, hard)) + os.open, os.close = observe_open, observe_close + try: + if stage == "preflight": + files._deletion_plan(root / "tree", recursive=True, parent=anchor) + elif stage == "mutation": + files._delete_plan(plan, parent=anchor) + else: + files.delete("alias/tree" if allow else "tree", recursive=True) + finally: + os.open, os.close = original_open, original_close + assert not active, active + assert peak <= 12, peak +assert (root / "tree").exists() == (stage == "preflight") +if allow: + assert (root / "alias").is_symlink() +""" + result = subprocess.run( + [sys.executable, "-c", script, str(root), str(deepest), stage, str(allow)], + capture_output=True, text=True, timeout=60, check=False, + ) + assert result.returncode == 0, result.stdout + result.stderr + assert (deepest / "victim").exists() == (stage == "preflight") + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX bounded directory cache") +def test_recursive_cache_eviction_reacquires_without_following_replacement(root, link): + branch = root / "branch" + deepest = branch.joinpath(*("d" for _ in range(20))) + deepest.mkdir(parents=True) + outside = root.parent / "outside" + outside.mkdir() + (outside / "victim").write_text("keep") + with FileHelper(root) as files, files._deletion_root() as anchor: + assert isinstance(anchor, int) + with file_helper.ExitStack() as resources: + directories = file_helper._DeletionDirectories( + root, anchor, files._directory_flags(), resources + ) + directories.directory(deepest) + assert branch not in directories.opened + branch.rename(root / "retained") + link(branch, outside) + with pytest.raises(OSError): + directories.directory(branch) + assert (outside / "victim").read_text() == "keep" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX bounded recursive preflight") +def test_recursive_cache_eviction_keeps_deny_preflight_before_any_removal(root, link): + tree = root / "tree" + deepest = tree.joinpath(*("d" for _ in range(20))) + deepest.mkdir(parents=True) + (tree / "a-first").write_text("keep") + outside = root.parent / "outside" + outside.write_text("keep") + alias = link(deepest / "link", outside) + with FileHelper(root) as files: + with pytest.raises(SymlinkDeniedError): + files.delete(tree, recursive=True) + assert alias.is_symlink() + assert (tree / "a-first").read_text() == "keep" + assert outside.read_text() == "keep" + + @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") @pytest.mark.parametrize("stage", ["resolve", "anchor", "delete"]) @pytest.mark.parametrize("allow", [False, True]) @@ -775,25 +887,42 @@ def test_legacy_darwin_establishes_write_search_only_root(root, monkeypatch, ope @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") -@pytest.mark.parametrize("depth", [8, 32, 64]) +@pytest.mark.parametrize("depth", [8, 32, 64, 160]) @pytest.mark.parametrize("recursive", [False, True]) -def test_delete_directory_open_count_is_linear(root, monkeypatch, depth, recursive): +def test_delete_directory_open_work_and_live_descriptors_stay_bounded( + root, monkeypatch, depth, recursive +): tree = root / "tree" - deepest = tree.joinpath(*("nested" for _ in range(depth))) + deepest = tree.joinpath(*("d" for _ in range(depth))) deepest.mkdir(parents=True) (deepest / "victim").write_text("selected") open_directory = os.open + close_directory = os.close opened = 0 + active = set() + peak = 0 def observe(*args, **kwargs): - nonlocal opened + nonlocal opened, peak + fd = open_directory(*args, **kwargs) opened += 1 - return open_directory(*args, **kwargs) + active.add(fd) + peak = max(peak, len(active)) + return fd + + def close(fd): + close_directory(fd) + active.discard(fd) monkeypatch.setattr(os, "open", observe) - FileHelper(root).delete(tree if recursive else deepest / "victim", recursive=recursive) + monkeypatch.setattr(os, "close", close) + with FileHelper(root) as files: + files.delete(tree if recursive else deepest / "victim", recursive=recursive) assert not (deepest / "victim").exists() - assert opened <= 8 * depth + 16 + assert not active + assert peak <= 12 + budget = 4 * depth + depth * depth // 8 + 32 if recursive else 8 * depth + 16 + assert opened <= budget @pytest.mark.skipif(os.name == "nt", reason="POSIX retained root after unlink") @@ -899,18 +1028,22 @@ def test_delete_without_search_flags_reports_unreadable_parent(root, monkeypatch @pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor lifetime") +@pytest.mark.parametrize("depth", [2, 20]) @pytest.mark.parametrize( ("recursive", "failure"), [(False, None), (False, "unlink"), (True, None), (True, "unlink"), (True, "scan")], ) -def test_delete_closes_directory_descriptors(root, monkeypatch, recursive, failure): +def test_delete_closes_directory_descriptors(root, monkeypatch, depth, recursive, failure): parent = root / "parent" - (parent / "nested").mkdir(parents=True) - victim = parent / "nested/victim" + deepest = parent.joinpath(*("d" for _ in range(depth))) + deepest.mkdir(parents=True) + victim = deepest / "victim" victim.write_text("selected") opened = [] root_opens = [] open_directory = os.open + scan_directory = os.scandir + scans = 0 def observe(*args, **kwargs): fd = open_directory(*args, **kwargs) @@ -922,11 +1055,18 @@ def observe(*args, **kwargs): def denied(*args, **kwargs): raise PermissionError("removal denied") + def scan(path): + nonlocal scans + scans += 1 + if scans == min(depth + 1, 12): + raise PermissionError("removal denied") + return scan_directory(path) + monkeypatch.setattr(os, "open", observe) if failure == "unlink": monkeypatch.setattr(os, "unlink", denied) elif failure == "scan": - monkeypatch.setattr(os, "scandir", denied) + monkeypatch.setattr(os, "scandir", scan) with FileHelper(root) as files: expected = pytest.raises(PermissionError, match="removal denied") if failure else nullcontext() with expected: From b3b1cdc5548dae739dabfc86a9706883fa7b6e70 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:02:30 -0600 Subject: [PATCH 12/16] fix: support legacy Windows file identity queries Prefer tagged extended IDs and use reliable legacy volume/index identities when the filesystem does not support FileIdInfo. Keep the retained root reference, query both live handles using the selected scheme, and preserve native capability and permission failures. Cover fallback selection, zero/ambiguous IDs, rename/reused-index cases, and native Windows legacy APIs. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 19 ++- src/specify_cli/_file_handles.py | 103 +++++++++++++- src/specify_cli/file_helper.py | 21 ++- tests/test_file_handles.py | 221 ++++++++++++++++++++++++++++++- tests/test_file_helper.py | 88 +++++++++++- 5 files changed, 432 insertions(+), 20 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 61938fee8a..11a0206cc4 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -153,7 +153,17 @@ helper is alive; stored device/inode numbers alone are not the binding. On POSIX the handle uses search-only access where available. On Windows, construction requests only root metadata access (`FILE_READ_ATTRIBUTES`) and permits delete sharing; it does not request directory-list access on the canonical ancestors. -The retained reference records the full volume/file identity from that handle. +The retained reference selects a tagged identity scheme from that handle. +The full 128-bit `FileIdInfo` identity is preferred. If that query reports +`ERROR_INVALID_FUNCTION`, `ERROR_NOT_SUPPORTED`, or `ERROR_INVALID_PARAMETER`, +or returns an all-zero ID, the helper queries the opened volume's filesystem +and uses `GetFileInformationByHandle`'s volume serial plus nonzero 64-bit file +index only for FAT, FAT32, exFAT, or NTFS. The volume query is handle-relative +and works through the filesystem driver rather than a pathname-based volume +lookup, including when a network driver exposes the necessary information. +ReFS and unrecognized filesystems do not fall back to potentially ambiguous +64-bit indexes. Access-denied, sharing, and other unrelated errors still +propagate rather than selecting a weaker identity. Before accepting it, construction compares Python-native metadata obtained from the acquired handle with the initially observed root metadata. This comparison does not reopen the root pathname, so replacement followed by restoration cannot @@ -165,8 +175,11 @@ The cached error remains traceback-free; each failed deletion raises an independent shallow copy retaining the native error type and filesystem details. Windows deletion separately pins the canonical ancestry through the drive/share -root without delete sharing, then compares the opened root's full identity with -the retained reference. Those operation-time handles require both +root without delete sharing, then queries both that root handle and the retained +reference using the already selected scheme. It compares their live identities, +not a cached legacy index: FAT IDs can change after renaming. It neither +renegotiates identity schemes mid-operation nor compares identities from +different schemes. Those operation-time handles require both `FILE_LIST_DIRECTORY` and `FILE_READ_ATTRIBUTES` on every canonical ancestor, including those above the helper root, and reject reparse points. An ACL allowing traversal but not listing can therefore prevent deletion while still allowing diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index 87cada9093..4f0493ce4d 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -8,6 +8,16 @@ import stat import tempfile from pathlib import Path, PureWindowsPath +from typing import Literal, NamedTuple + + +_IdentityKind = Literal["fileid128", "legacy64"] + + +class _WindowsFileIdentity(NamedTuple): + kind: _IdentityKind + volume: int + file_id: bytes def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, sharing: int) -> int: @@ -151,8 +161,85 @@ def _windows_handle( _windows_api().CloseHandle(handle) -def _windows_file_id(handle: int) -> tuple[int, bytes]: - """Read the volume and complete file identity from an already opened object.""" +def _windows_filesystem(handle: int) -> str: + """Query the opened volume, including network filesystems, without a pathname.""" + import ctypes + from ctypes import wintypes + from . import _windows_abi as abi + + class FilesystemAttributes(ctypes.Structure): + _fields_ = [ + ("attributes", ctypes.c_uint32), + ("maximum_component", ctypes.c_int32), + ("name_length", ctypes.c_uint32), + ("name", ctypes.c_uint16 * 261), + ] + + native = ctypes.WinDLL("ntdll") + query = native.NtQueryVolumeInformationFile + query.argtypes = [ + wintypes.HANDLE, abi._IoStatusBlockPointer, wintypes.LPVOID, + ctypes.c_uint32, ctypes.c_int, + ] + query.restype = ctypes.c_int32 + dos_error = native.RtlNtStatusToDosError + dos_error.argtypes = [ctypes.c_int32] + dos_error.restype = ctypes.c_uint32 + attributes = FilesystemAttributes() + status = abi._IoStatusBlock() + result = query( + handle, ctypes.byref(status), ctypes.byref(attributes), + ctypes.sizeof(attributes), 5, # FileFsAttributeInformation. + ) + if result != 0: + raise ctypes.WinError(dos_error(result)) + length = attributes.name_length + if not length or length % 2 or length > ctypes.sizeof(attributes.name): + raise OSError(errno.EINVAL, "Invalid filesystem name from volume query") + return bytes(attributes.name)[:length].decode("utf-16-le") + + +def _windows_legacy_file_id(handle: int) -> _WindowsFileIdentity: + """Query a live legacy identity only on filesystems with usable 64-bit IDs.""" + import ctypes + from ctypes import wintypes + + filesystem = _windows_filesystem(handle).upper() + # ReFS needs 128 bits; unknown drivers may report non-identifying indexes. + if filesystem not in {"FAT", "FAT32", "EXFAT", "NTFS"}: + raise OSError(errno.ENOTSUP, f"No reliable legacy file identity for {filesystem}") + + class FileInformation(ctypes.Structure): + _fields_ = [ + ("attributes", ctypes.c_uint32), + ("times", ctypes.c_uint32 * 6), + ("volume", ctypes.c_uint32), + ("size_high", ctypes.c_uint32), + ("size_low", ctypes.c_uint32), + ("links", ctypes.c_uint32), + ("index_high", ctypes.c_uint32), + ("index_low", ctypes.c_uint32), + ] + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + query = kernel.GetFileInformationByHandle + query.argtypes = [wintypes.HANDLE, wintypes.LPVOID] + query.restype = wintypes.BOOL + information = FileInformation() + if not query(handle, ctypes.byref(information)): + raise ctypes.WinError(ctypes.get_last_error()) + index = (information.index_high << 32) | information.index_low + if not index: + raise OSError(errno.ENOTSUP, "No usable legacy file identity") + return _WindowsFileIdentity("legacy64", information.volume, index.to_bytes(8, "little")) + + +def _windows_file_id( + handle: int, *, kind: _IdentityKind | None = None +) -> _WindowsFileIdentity: + """Select a supported identity once, or query the already selected scheme.""" + if kind == "legacy64": + return _windows_legacy_file_id(handle) import ctypes from ctypes import wintypes @@ -168,8 +255,16 @@ class FileIdInfo(ctypes.Structure): info.restype = wintypes.BOOL identity = FileIdInfo() if not info(handle, 18, ctypes.byref(identity), ctypes.sizeof(identity)): - raise ctypes.WinError(ctypes.get_last_error()) - return identity.volume, bytes(identity.file_id) + error = ctypes.get_last_error() + if kind is None and error in (1, 50, 87): + return _windows_legacy_file_id(handle) + raise ctypes.WinError(error) + value = bytes(identity.file_id) + if not any(value): + if kind is None: + return _windows_legacy_file_id(handle) + raise OSError(errno.ENOTSUP, "No usable extended file identity") + return _WindowsFileIdentity("fileid128", identity.volume, value) def _windows_stat(handle: int) -> os.stat_result: diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 26ad02490c..4a68403ebb 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -16,7 +16,8 @@ from weakref import finalize from specify_cli._file_handles import ( - _windows_file_id, _windows_handle, _windows_overwrite_fd, _windows_stat, windows_read_fd, + _WindowsFileIdentity, _windows_file_id, _windows_handle, + _windows_overwrite_fd, _windows_stat, windows_read_fd, ) @@ -177,7 +178,8 @@ class FileHelper: _root_stat: os.stat_result = field(init=False, repr=False, compare=False) _root_anchor: int | Path | None = field(init=False, repr=False, compare=False) _root_anchor_error: OSError | None = field(init=False, repr=False, compare=False) - _windows_root_id: tuple[int, bytes] | None = field(init=False, repr=False, compare=False) + _windows_root_id: _WindowsFileIdentity | None = field(init=False, repr=False, compare=False) + _windows_root_handle: int | None = field(init=False, repr=False, compare=False) _root_cleanup: finalize = field(init=False, repr=False, compare=False) def __post_init__(self) -> None: @@ -192,7 +194,8 @@ def __post_init__(self) -> None: with ExitStack() as resources: anchor: int | Path | None = None anchor_error: OSError | None = None - windows_root_id: tuple[int, bytes] | None = None + windows_root_id: _WindowsFileIdentity | None = None + windows_root_handle: int | None = None if os.name == "nt": with ExitStack() as reference: try: @@ -209,6 +212,7 @@ def __post_init__(self) -> None: self._check_deletion_root(canonical.stat()) resources.enter_context(reference.pop_all()) anchor = canonical + windows_root_handle = handle elif ( _DELETE_DIR_FD_SUPPORTED and getattr(os, "O_NOFOLLOW", 0) @@ -220,6 +224,7 @@ def __post_init__(self) -> None: object.__setattr__(self, "_root_anchor", anchor) object.__setattr__(self, "_root_anchor_error", anchor_error) object.__setattr__(self, "_windows_root_id", windows_root_id) + object.__setattr__(self, "_windows_root_handle", windows_root_handle) object.__setattr__(self, "_root_cleanup", finalize(self, resources.pop_all().close)) def close(self) -> None: @@ -567,18 +572,24 @@ def _deletion_root(self) -> Iterator[int | Path]: error.winerror = self._root_anchor_error.winerror raise error raise OSError(errno.ENOTSUP, "Handle-based deletion is unsupported on this platform") - self._check_deletion_root(self._canonical_root.stat()) if isinstance(self._root_anchor, Path): + reference = self._windows_root_handle + identity = self._windows_root_id + if reference is None or identity is None: + raise FileHelperError("Missing retained Windows root identity") with ExitStack() as resources: for ancestor in reversed(self._root_anchor.parents): resources.enter_context(_windows_handle(ancestor, directory=True)) handle = resources.enter_context( _windows_handle(self._root_anchor, directory=True) ) - if _windows_file_id(handle) != self._windows_root_id: + selected = _windows_file_id(handle, kind=identity.kind) + retained = _windows_file_id(reference, kind=identity.kind) + if selected != retained: raise FileHelperError(f"Established root changed during deletion: {self.root}") yield self._root_anchor else: + self._check_deletion_root(self._canonical_root.stat()) yield self._root_anchor @contextmanager diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index 7b068ed8a3..fb5201a8bf 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -17,7 +17,9 @@ from specify_cli import _file_handles, file_helper from specify_cli import _file_handles as handles -from specify_cli._file_handles import _windows_file_id, _windows_handle, _windows_stat +from specify_cli._file_handles import ( + _WindowsFileIdentity, _windows_file_id, _windows_handle, _windows_stat, +) from specify_cli.file_helper import FileHelper, UnsupportedPathError @@ -100,11 +102,187 @@ def info(handle, kind, buffer, size): with pytest.raises(PermissionError, match="identity denied"): _windows_file_id(0x1FFFFFFFF) else: - assert _windows_file_id(0x1FFFFFFFF) == (0x1FFFFFFFF, bytes(range(16))) + assert _windows_file_id(0x1FFFFFFFF) == _WindowsFileIdentity( + "fileid128", 0x1FFFFFFFF, bytes(range(16)) + ) assert info.argtypes == [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] assert info.restype is wintypes.BOOL +@pytest.fixture +def identity_kernel(monkeypatch): + state = SimpleNamespace( + extended_error=87, legacy_error=0, last_error=0, filesystem="FAT32", + volume=0xABCDEF12, index=0x123456789ABCDEF, zero_extended=False, + ) + calls = [] + + def extended(handle, kind, output, size): + assert handle == 0x1FFFFFFFF and kind == 18 and size == 24 + calls.append("extended") + if state.extended_error: + state.last_error = state.extended_error + return False + output._obj.volume = state.volume + output._obj.file_id[:] = bytes(16) if state.zero_extended else bytes(range(16)) + return True + + def legacy(handle, output): + assert handle == 0x1FFFFFFFF + assert ctypes.sizeof(output._obj) == 52 + calls.append("legacy") + if state.legacy_error: + state.last_error = state.legacy_error + return False + output._obj.volume = state.volume + output._obj.index_high = state.index >> 32 + output._obj.index_low = state.index & 0xFFFFFFFF + return True + + def native_error(code): + error = OSError(errno.EACCES if code == 5 else errno.ENOTSUP, f"native error {code}") + error.winerror = code + return error + + monkeypatch.setattr(ctypes, "WinDLL", lambda *args, **kwargs: SimpleNamespace( + GetFileInformationByHandleEx=extended, GetFileInformationByHandle=legacy, + ), raising=False) + monkeypatch.setattr(ctypes, "get_last_error", lambda: state.last_error, raising=False) + monkeypatch.setattr(ctypes, "WinError", native_error, raising=False) + monkeypatch.setattr( + handles, "_windows_filesystem", lambda _: state.filesystem, raising=False + ) + return state, calls + + +@pytest.mark.parametrize("unsupported", [1, 50, 87]) +@pytest.mark.parametrize("filesystem", ["FAT", "FAT32", "exFAT", "NTFS"]) +def test_windows_identity_falls_back_only_for_supported_legacy_filesystems( + identity_kernel, unsupported, filesystem +): + state, calls = identity_kernel + state.extended_error = unsupported + state.filesystem = filesystem + identity = _windows_file_id(0x1FFFFFFFF) + assert identity.kind == "legacy64" + assert identity.volume == state.volume + assert identity.file_id == state.index.to_bytes(8, "little") + assert calls == ["extended", "legacy"] + + +@pytest.mark.parametrize("error_code", [5, 6, 32]) +def test_windows_identity_does_not_hide_unrelated_native_errors(identity_kernel, error_code): + state, calls = identity_kernel + state.extended_error = error_code + with pytest.raises(OSError) as raised: + _windows_file_id(0x1FFFFFFFF) + assert raised.value.winerror == error_code + assert calls == ["extended"] + + +@pytest.mark.parametrize("filesystem", ["ReFS", "unknown"]) +def test_windows_identity_rejects_unreliable_legacy_scheme(identity_kernel, filesystem): + state, calls = identity_kernel + state.filesystem = filesystem + with pytest.raises(OSError, match="legacy.*identity"): + _windows_file_id(0x1FFFFFFFF) + assert calls == ["extended"] + + +@pytest.mark.parametrize("scheme", ["legacy64", "fileid128"]) +def test_windows_identity_uses_selected_scheme_without_renegotiating(identity_kernel, scheme): + state, calls = identity_kernel + state.extended_error = 0 + identity = _windows_file_id(0x1FFFFFFFF, kind=scheme) + assert identity.kind == scheme + assert calls == (["legacy"] if scheme == "legacy64" else ["extended"]) + + +def test_windows_identity_selected_extended_scheme_does_not_downgrade(identity_kernel): + state, calls = identity_kernel + with pytest.raises(OSError) as raised: + _windows_file_id(0x1FFFFFFFF, kind="fileid128") + assert raised.value.winerror == state.extended_error + assert calls == ["extended"] + + +@pytest.mark.parametrize("invalid", [False, True]) +def test_windows_filesystem_query_is_handle_relative_and_preserves_abi(monkeypatch, invalid): + from specify_cli import _windows_abi as abi + + def query(handle, status, output, size, kind): + assert handle == 0x1FFFFFFFF and size == 536 and kind == 5 + assert isinstance(status._obj, abi._IoStatusBlock) + name = "exFAT".encode("utf-16-le") + output._obj.name_length = 1 if invalid else len(name) + ctypes.memmove(output._obj.name, name, len(name)) + return 0 + + def dos_error(status): + pytest.fail("A successful native query must not query an error") + + monkeypatch.setattr(ctypes, "WinDLL", lambda *args: SimpleNamespace( + NtQueryVolumeInformationFile=query, RtlNtStatusToDosError=dos_error, + ), raising=False) + if invalid: + with pytest.raises(OSError, match="Invalid filesystem name"): + handles._windows_filesystem(0x1FFFFFFFF) + else: + assert handles._windows_filesystem(0x1FFFFFFFF) == "exFAT" + assert query.argtypes == [ + wintypes.HANDLE, abi._IoStatusBlockPointer, wintypes.LPVOID, + ctypes.c_uint32, ctypes.c_int, + ] + assert query.restype is ctypes.c_int32 + + +def test_windows_filesystem_query_preserves_native_failure(monkeypatch): + def query(*args): + return -1 + + def dos_error(status): + assert status == -1 + return 5 + + def win_error(code): + assert code == 5 + error = PermissionError("volume query denied") + error.winerror = code + return error + + monkeypatch.setattr(ctypes, "WinDLL", lambda *args: SimpleNamespace( + NtQueryVolumeInformationFile=query, RtlNtStatusToDosError=dos_error, + ), raising=False) + monkeypatch.setattr(ctypes, "WinError", win_error, raising=False) + with pytest.raises(PermissionError, match="volume query denied"): + handles._windows_filesystem(123) + + +def test_windows_identity_zero_extended_id_uses_reliable_legacy_identity(identity_kernel): + state, calls = identity_kernel + state.extended_error = 0 + state.zero_extended = True + identity = _windows_file_id(0x1FFFFFFFF) + assert identity.kind == "legacy64" + assert calls == ["extended", "legacy"] + + +def test_windows_identity_zero_legacy_id_is_not_accepted(identity_kernel): + state, calls = identity_kernel + state.index = 0 + with pytest.raises(OSError, match="identity"): + _windows_file_id(0x1FFFFFFFF) + assert calls == ["extended", "legacy"] + + +def test_windows_identity_legacy_query_error_remains_native(identity_kernel): + state, _ = identity_kernel + state.legacy_error = 5 + with pytest.raises(OSError) as raised: + _windows_file_id(0x1FFFFFFFF) + assert raised.value.winerror == 5 + + @pytest.mark.parametrize("failure", [None, "duplicate", "convert", "stat"]) def test_windows_stat_preserves_borrowed_handle_and_closes_duplicate(tmp_path, monkeypatch, failure): expected = tmp_path.stat() @@ -158,6 +336,45 @@ def test_native_windows_stat_matches_path_metadata_without_closing_reference(tmp assert _windows_file_id(handle) == identity +@pytest.mark.skipif(os.name != "nt", reason="Native Windows legacy identity APIs") +@pytest.mark.parametrize("recursive", [False, True]) +@pytest.mark.parametrize("allow", [False, True]) +def test_native_windows_delete_uses_legacy_identity_when_extended_query_is_unsupported( + tmp_path, monkeypatch, recursive, allow +): + parent = tmp_path / "tree/nested" + parent.mkdir(parents=True) + (parent / "victim").write_text("selected") + native = handles._windows_api() + win_dll = ctypes.WinDLL + extended_queries = [] + + def query(handle, kind, output, size): + if kind == 18: + extended_queries.append(handle) + ctypes.set_last_error(87) + return False + return native.GetFileInformationByHandleEx(handle, kind, output, size) + + kernel = SimpleNamespace( + CreateFileW=native.CreateFileW, + CloseHandle=native.CloseHandle, + GetFileInformationByHandleEx=query, + GetFileInformationByHandle=native.GetFileInformationByHandle, + ) + + def load(name, *args, **kwargs): + return kernel if name == "kernel32" else win_dll(name, *args, **kwargs) + + monkeypatch.setattr(ctypes, "WinDLL", load) + with FileHelper(tmp_path, allow_symlinks=allow) as files: + assert files._windows_root_id is not None + assert files._windows_root_id.kind == "legacy64" + files.delete("tree" if recursive else "tree/nested/victim", recursive=recursive) + assert len(extended_queries) == 1 + assert not (parent / "victim").exists() + + @pytest.mark.skipif(os.name == "nt", reason="POSIX startup must not require Windows ctypes support") @pytest.mark.parametrize("operation", ["help", "files"]) def test_posix_usage_without_optional_ctypes(tmp_path, operation): diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 035055208f..7b446d7296 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -15,6 +15,7 @@ import pytest from specify_cli import file_helper +from specify_cli._file_handles import _WindowsFileIdentity from specify_cli.file_helper import ( FileHelper, FileHelperError, @@ -2059,7 +2060,10 @@ def handle(path, *, directory, pin=True): backend.name = "nt" monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) - monkeypatch.setattr(module, "_windows_file_id", lambda value: identities[value]) + monkeypatch.setattr( + module, "_windows_file_id", + lambda value, **kwargs: _WindowsFileIdentity("fileid128", *identities[value]), + ) monkeypatch.setattr(module, "_windows_stat", lambda value: metadata[value]) with FileHelper(project) as files: assert not attempted @@ -2121,7 +2125,10 @@ def exchange(path, **kwargs): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) - monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"root")) + monkeypatch.setattr( + file_helper, "_windows_file_id", + lambda _, **kwargs: _WindowsFileIdentity("fileid128", 1, b"root"), + ) monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) monkeypatch.setattr(Path, "lstat", exchange) with FileHelper(root, allow_symlinks=True) as files: @@ -2205,7 +2212,10 @@ def handle(path, *, directory, pin=True): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) - monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"replacement")) + monkeypatch.setattr( + file_helper, "_windows_file_id", + lambda _, **kwargs: _WindowsFileIdentity("fileid128", 1, b"replacement"), + ) monkeypatch.setattr(file_helper, "_windows_stat", lambda value: acquired[value], raising=False) with pytest.raises(FileHelperError, match="root changed"): with FileHelper(project): @@ -2284,7 +2294,10 @@ def handle(path, *, directory, pin=True, parent=None, traverse=False): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) - monkeypatch.setattr(file_helper, "_windows_file_id", lambda _: (1, b"root"), raising=False) + monkeypatch.setattr( + file_helper, "_windows_file_id", + lambda _, **kwargs: _WindowsFileIdentity("fileid128", 1, b"root"), + ) monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) with FileHelper(root) as files: assert files.read_text("victim") == "keep" @@ -2345,7 +2358,10 @@ def handle(path, *, directory, pin=True, parent=None, traverse=False): monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) monkeypatch.setattr(file_helper, "_windows_handle", handle) - monkeypatch.setattr(file_helper, "_windows_file_id", lambda handle: identities[handle]) + monkeypatch.setattr( + file_helper, "_windows_file_id", + lambda value, **kwargs: _WindowsFileIdentity("fileid128", *identities[value]), + ) monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) with FileHelper(root) as files: with pytest.raises(FileHelperError, match="root changed"): @@ -2355,6 +2371,63 @@ def handle(path, *, directory, pin=True, parent=None, traverse=False): assert active == [] +@pytest.mark.parametrize("scenario", ["same", "renamed-reference", "replacement-reused-id"]) +@pytest.mark.parametrize("recursive", [False, True]) +def test_windows_legacy_root_identity_is_compared_live_with_the_selected_scheme( + root, monkeypatch, scenario, recursive +): + (root / "tree").mkdir() + (root / "tree/victim").write_text("keep") + active = [] + reference = None + queries = [] + changed = False + + @contextmanager + def handle(path, *, directory, pin=True): + nonlocal reference + assert directory + value = len(queries) + len(active) + 1 + active.append(value) + if not pin: + reference = value + try: + yield value + finally: + assert active.pop() == value + + def identity(value, *, kind=None): + queries.append((value, kind)) + assert kind in (None, "legacy64") + index = 1 + if changed and scenario != "same": + index = 2 if value == reference or scenario == "renamed-reference" else 1 + return _WindowsFileIdentity("legacy64", 1, index.to_bytes(8, "little")) + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "name": "nt"})) + monkeypatch.setattr(file_helper, "_windows_handle", handle) + monkeypatch.setattr(file_helper, "_windows_file_id", identity) + monkeypatch.setattr(file_helper, "_windows_stat", lambda _: root.stat()) + with FileHelper(root) as files: + assert files._windows_root_id is not None + assert files._windows_root_id.kind == "legacy64" + changed = True + selected = "tree" if recursive else "tree/victim" + if scenario == "replacement-reused-id": + with pytest.raises(FileHelperError, match="root changed"): + files.delete(selected, recursive=recursive) + assert (root / "tree/victim").read_text() == "keep" + else: + files.delete(selected, recursive=recursive) + assert not (root / "tree/victim").exists() + assert active == [reference] + assert active == [] + assert queries[0] == (reference, None) + assert len(queries) == 3 + assert queries[-1] == (reference, "legacy64") + assert all(kind == "legacy64" for _, kind in queries[1:]) + + @pytest.mark.skipif(os.name != "nt", reason="Native Windows directory ACL semantics") @pytest.mark.parametrize("consumer", ["helper", "policy", "state"]) def test_windows_readonly_helper_supports_traverse_only_ancestor(root, consumer): @@ -2456,7 +2529,10 @@ def remove_directory(path, *, dir_fd=None): backend.scandir = observe_scan monkeypatch.setattr(module, "os", backend) monkeypatch.setattr(module, "_windows_handle", handle) - monkeypatch.setattr(module, "_windows_file_id", lambda _: (1, b"root")) + monkeypatch.setattr( + module, "_windows_file_id", + lambda _, **kwargs: _WindowsFileIdentity("fileid128", 1, b"root"), + ) monkeypatch.setattr(module, "_windows_stat", lambda _: root.stat()) files = FileHelper(root, allow_symlinks=True) selected = alias / "nested" if recursive else alias / "nested/victim" From 0a45f0d204aa369846107042567f4d67db237cfb Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:16:30 -0600 Subject: [PATCH 13/16] fix: close catalog HTTP errors before translating Release HTTP error responses deterministically before raising catalog errors so delayed cleanup cannot leak resources or warnings into later tests. Preserve transient-status fallback, definitive client errors, original exception causes, and already-closed response behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/catalogs.md | 5 ++++- src/specify_cli/bundles/adapters.py | 13 ++++++----- tests/specify_cli/bundles/test_adapters.py | 26 ++++++++++++++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/design/catalogs.md b/design/catalogs.md index ff8b42cd41..66d4b82e63 100644 --- a/design/catalogs.md +++ b/design/catalogs.md @@ -204,7 +204,10 @@ Current behavior differs by primitive: sources fetch repository catalogs when online and fall back to packaged snapshots only for catalog-unavailable failures; malformed or unsafe content is not masked by the snapshot. Bundle stacks memoize parsed payloads for the - lifetime of one stack instance. + lifetime of one stack instance. When translating an HTTP status error, the + fetcher closes its response body before propagating the catalog error, while + retaining the original exception as its cause and preserving status-based + fallback classification. A future shared fetch boundary may share URL validation, redirect validation, bounded reads, cache records, and error types. It must preserve each diff --git a/src/specify_cli/bundles/adapters.py b/src/specify_cli/bundles/adapters.py index b88969400d..7853254d3e 100644 --- a/src/specify_cli/bundles/adapters.py +++ b/src/specify_cli/bundles/adapters.py @@ -268,13 +268,16 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: # server-side availability responses (408, 429, 5xx) fall back to the # snapshot. Any other 4xx (404, 403, ...) is definitive and must # surface as a hard error. - if exc.code in _TRANSIENT_HTTP_STATUS_CODES or exc.code >= 500: - raise _CatalogUnavailable( + try: + if exc.code in _TRANSIENT_HTTP_STATUS_CODES or exc.code >= 500: + raise _CatalogUnavailable( + f"Failed to fetch catalog from {url}: HTTP {exc.code} {exc.reason}" + ) from exc + raise BundlerError( f"Failed to fetch catalog from {url}: HTTP {exc.code} {exc.reason}" ) from exc - raise BundlerError( - f"Failed to fetch catalog from {url}: HTTP {exc.code} {exc.reason}" - ) from exc + finally: + exc.close() except urllib.error.URLError as exc: # urllib wraps a TLS handshake failure as URLError(reason=); # a certificate-verification failure is a security failure, not a diff --git a/tests/specify_cli/bundles/test_adapters.py b/tests/specify_cli/bundles/test_adapters.py index 40a9dd0688..3eac29dad9 100644 --- a/tests/specify_cli/bundles/test_adapters.py +++ b/tests/specify_cli/bundles/test_adapters.py @@ -132,6 +132,32 @@ def fake_open_url(url, timeout=10, extra_headers=None, redirect_validator=None): assert not isinstance(excinfo.value, adapters._CatalogUnavailable) +@pytest.mark.parametrize("status", [403, 404, 408, 429, 500, 503]) +@pytest.mark.parametrize("already_closed", [False, True]) +def test_http_error_response_is_closed_before_catalog_error_translation( + monkeypatch, status, already_closed +): + body = io.BytesIO(b"error response") + error = urllib.error.HTTPError( + "https://example.com/catalog.json", status, "response failed", {}, body, + ) + if already_closed: + error.close() + + def failed(*args, **kwargs): + raise error + + monkeypatch.setattr("specify_cli.authentication.http.open_url", failed) + with pytest.raises(BundlerError, match=f"HTTP {status}") as raised: + adapters._http_get_json("team", error.url) + assert raised.value.__cause__ is error + assert isinstance(raised.value, adapters._CatalogUnavailable) == ( + status in (408, 429) or status >= 500 + ) + assert body.closed + assert error.closed + + def test_http_get_json_preserves_malformed_json(monkeypatch): def fake_open_url(url, timeout=10, extra_headers=None, redirect_validator=None): return _FakeResponse(b"not json", url) From 7e2c3592ec51a359019ca14a87bbdfae189a4471 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 16:46:32 -0600 Subject: [PATCH 14/16] fix: use native handles for Windows deletion operations Carry directory handles through entry classification, link reads, enumeration and removal. Mark the classified leaf handle for deletion instead of reopening its pathname, preserving supported links and explicit native errors. Cover in-place junction changes and fix operation cleanup tests so constructor calls cannot satisfy their fstat failures. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 36 ++-- src/specify_cli/_file_handles.py | 127 +++++++++++- src/specify_cli/file_helper.py | 108 +++++++---- tests/test_file_handles.py | 289 +++++++++++++++++++++++++++- tests/test_file_helper.py | 228 +++++++++++++++++----- tests/test_file_helper_overwrite.py | 12 +- 6 files changed, 686 insertions(+), 114 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 11a0206cc4..a429721f1e 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -185,12 +185,16 @@ including those above the helper root, and reject reparse points. An ACL allowin traversal but not listing can therefore prevent deletion while still allowing bootstrap reads. Windows deletion also retains non-reparse handles to accessed parent directories throughout link resolution, before classifying child entries -or reading link targets. Resolving a contained parent link does not consult -metadata through an unpinned replacement parent. These accessed directories also -require directory-list/read-attribute access. The resolution handles are released -after selection; resolved mutation parents and root ancestry remain pinned during -preflight and removal. These locks are released at the end of deletion; the -metadata-only reference remains until the helper closes. Automatic +or reading link targets. Each child is opened relative to its native parent +handle, not by reconstructing an absolute pathname. Metadata and symbolic-link +targets are read from opened leaf handles; directory names are enumerated from +the directory handle. These accessed directories also require +directory-list/read-attribute access. The resolution handles are released after +selection; native mutation parents and root ancestry remain retained during +preflight and removal. Directory sharing locks alone do not prevent in-place +reparse changes: handle-relative operations retain the opened directory object +instead of following its changed pathname. These locks are released at the end of +deletion; the metadata-only reference remains until the helper closes. Automatic finalization is a compatibility fallback, not a substitute for deterministic cleanup. Bootstrap configuration and managed integration-state consumers close their own helpers explicitly. Installed-workflow listing also closes its @@ -289,9 +293,12 @@ descriptor-relative or change their documented concurrency limitations. classification, link-target reads and resolution, recursive preflight, and per-entry revalidation, as well as descriptor-relative enumeration and `unlink`/`rmdir`. Canonical pathnames identify root-relative entries but are not - reopened for metadata or link resolution. Windows holds the full canonical - ancestry, including ancestors above the root, without delete sharing while using - absolute paths. This does not change the trusted treatment of aliases resolved + reopened for metadata or link resolution. Windows verifies the full canonical + ancestry during root acquisition and then carries native directory handles + through child opens, metadata queries, link-target reads, and enumeration. + Removal opens the leaf itself with `DELETE` access and marks that same handle + with `FileDispositionInfo`; it does not call pathname `unlink` or `rmdir`. + This does not change the trusted treatment of aliases resolved at root establishment. Allowed contained parent links are resolved within the retained boundary, and leaf links are still unlinked rather than followed. Platforms lacking the required directory-descriptor operations fail with @@ -316,8 +323,8 @@ descriptor-relative or change their documented concurrency limitations. Their live descriptor count is bounded independently of operation-path depth, in addition to the helper's retained root. Link-target jumps and `..` are re-anchored through that root rather than following an opened directory's - physical parent after a rename. Windows retains the accessed ancestor handles - because its path-based access requires those directories to stay pinned. + physical parent after a rename. Windows retains native directory handles for + its child-relative traversal, including during recursive preflight and removal. POSIX recursive preflight and mutation use explicit worklists and separate eight-directory handle caches. The selected parent and established root remain anchored outside those caches. An uncached parent is reacquired component by @@ -329,7 +336,7 @@ descriptor-relative or change their documented concurrency limitations. Reacquiring evicted prefixes trades additional opens for that fixed handle budget. Total opens can grow with the sum of visited path lengths, rather than being unconditionally linear in the directory count. Windows retains its - ancestry-locking traversal instead of evicting handles needed for path-based + native-handle traversal instead of evicting handles needed for child-relative operations. The complete plan is still preflighted before any removal. - `symlink(target, path)` creates a link exclusively. Relative targets are interpreted relative to the link parent, not the helper root. @@ -398,8 +405,9 @@ the opened directory even if that directory is renamed; validation continues to inspect that same directory tree. Windows prevents ancestor renames while handles are held. Concurrent changes may still cause deletion to fail or leave partial results, and preflight is not a transaction or an immutable tree snapshot. -Leaf removal acts on the current entry in the anchored parent, not a pinned leaf -identity. No operation is a general race-free sandbox. For operations that still +POSIX leaf removal acts on the current entry in the anchored parent; Windows +removal acts on its opened leaf handle. No operation is a general race-free +sandbox. For operations that still use validated paths (reads, directory creation, atomic writes, and symlink creation), portable path checks are **not** a race-free sandbox: another process can exchange parents or entries between diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index 4f0493ce4d..c908d60945 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -6,6 +6,7 @@ import os import secrets import stat +import struct import tempfile from pathlib import Path, PureWindowsPath from typing import Literal, NamedTuple @@ -20,7 +21,10 @@ class _WindowsFileIdentity(NamedTuple): file_id: bytes -def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, sharing: int) -> int: +def _open_windows_child( + parent: int, name: str, *, directory: bool | None, access: int, + sharing: int, allow_reparse: bool = False, +) -> int: """Open one component relative to an already validated directory handle.""" if PureWindowsPath(name).parts != (name,) or PureWindowsPath(name).anchor or name == ".." or "\0" in name: raise ValueError(f"Expected a single relative Windows path component: {name!r}") @@ -51,11 +55,13 @@ def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, # OBJ_DONT_REPARSE and FILE_OPEN_REPARSE_POINT keep resolution on this handle. attributes = abi._ObjectAttributes( ctypes.sizeof(abi._ObjectAttributes), parent, ctypes.pointer(unicode_name), - 0x40 | 0x1000, None, None, + 0x40 | (0 if allow_reparse else 0x1000), None, None, ) handle = wintypes.HANDLE() status = abi._IoStatusBlock() - options = 0x00200000 | (0x1 if directory else 0x40 | 0x20) + options = 0x00200000 | ( + 0x1 if directory is True else (0x40 | 0x20 if directory is False else 0x20) + ) result = create( ctypes.byref(handle), access | (0 if directory else 0x100000), ctypes.byref(attributes), ctypes.byref(status), None, 0, @@ -68,6 +74,121 @@ def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, return handle.value +@contextmanager +def _windows_entry(parent: int, name: str, *, deleting: bool = False) -> Iterator[int]: + """Open the leaf itself relative to its directory, including allowed links.""" + handle = _open_windows_child( + parent, name, directory=None, access=0x80 | (0x10000 if deleting else 0), + sharing=0x7, allow_reparse=True, + ) + try: + yield handle + finally: + _windows_api().CloseHandle(handle) + + +def _windows_mode(handle: int) -> int: + import ctypes + + class Attributes(ctypes.Structure): + _fields_ = [("attributes", ctypes.c_uint32), ("tag", ctypes.c_uint32)] + + info = Attributes() + if not _windows_api().GetFileInformationByHandleEx( + handle, 9, ctypes.byref(info), ctypes.sizeof(info) + ): + raise ctypes.WinError(ctypes.get_last_error()) + if info.attributes & 0x400: + if info.tag != 0xA000000C: + raise OSError(errno.ELOOP, "Unsupported Windows reparse point") + return stat.S_IFLNK + return stat.S_IFDIR if info.attributes & 0x10 else stat.S_IFREG + + +def _windows_readlink(handle: int) -> str: + import ctypes + from ctypes import wintypes + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + control = kernel.DeviceIoControl + control.argtypes = [ + wintypes.HANDLE, wintypes.DWORD, wintypes.LPVOID, wintypes.DWORD, + wintypes.LPVOID, wintypes.DWORD, wintypes.LPDWORD, wintypes.LPVOID, + ] + control.restype = wintypes.BOOL + buffer = ctypes.create_string_buffer(16384) + length = wintypes.DWORD() + if not control( + handle, 0x900A8, None, 0, buffer, len(buffer), ctypes.byref(length), None + ): + raise ctypes.WinError(ctypes.get_last_error()) + data = buffer.raw[:length.value] + if len(data) < 20: + raise OSError(errno.EIO, "Incomplete Windows symlink data") + tag, size, _, offset, count, _, _, flags = struct.unpack_from(" len(data) or end > 8 + size + ): + raise OSError(errno.EIO, "Invalid Windows symlink data") + target = data[20 + offset:end].decode("utf-16-le", errors="surrogatepass") + if "\0" in target: + raise OSError(errno.EIO, "Invalid Windows symlink target") + if not flags & 1 and target.startswith("\\??\\"): + target = "\\\\?\\" + target[4:] + return target + + +def _windows_directory_names(handle: int) -> list[str]: + import ctypes + + buffer = ctypes.create_string_buffer(131072) + names = [] + kind = 15 # FileFullDirectoryRestartInfo, then FileFullDirectoryInfo. + while True: + if not _windows_api().GetFileInformationByHandleEx(handle, kind, buffer, len(buffer)): + error = ctypes.get_last_error() + if error == 18: # ERROR_NO_MORE_FILES. + return names + raise ctypes.WinError(error) + kind = 14 + data = buffer.raw + offset = 0 + while True: + if offset + 68 > len(data): + raise OSError(errno.EIO, "Incomplete Windows directory entry") + next_entry = struct.unpack_from(" len(data): + raise OSError(errno.EIO, "Invalid Windows directory entry") + name = data[offset + 68:end].decode("utf-16-le", errors="surrogatepass") + if name not in (".", ".."): + if PureWindowsPath(name).parts != (name,) or PureWindowsPath(name).anchor or "\0" in name: + raise OSError(errno.EIO, "Invalid Windows directory component") + names.append(name) + if not next_entry: + break + if next_entry % 8 or next_entry < 68 + size: + raise OSError(errno.EIO, "Invalid Windows directory entry offset") + offset += next_entry + + +def _windows_delete(handle: int) -> None: + """Mark the opened object for removal, never reopen its pathname.""" + import ctypes + from ctypes import wintypes + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + dispose = kernel.SetFileInformationByHandle + dispose.argtypes = [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD] + dispose.restype = wintypes.BOOL + delete = ctypes.c_int32(True) + if not dispose(handle, 4, ctypes.byref(delete), ctypes.sizeof(delete)): + raise ctypes.WinError(ctypes.get_last_error()) + + def _windows_api(): import ctypes from ctypes import wintypes diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 4a68403ebb..af15246f66 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -16,8 +16,9 @@ from weakref import finalize from specify_cli._file_handles import ( - _WindowsFileIdentity, _windows_file_id, _windows_handle, - _windows_overwrite_fd, _windows_stat, windows_read_fd, + _WindowsFileIdentity, _windows_delete, _windows_directory_names, _windows_entry, + _windows_file_id, _windows_handle, _windows_mode, _windows_overwrite_fd, + _windows_readlink, _windows_stat, windows_read_fd, ) @@ -58,13 +59,19 @@ class UnsupportedPathError(FileHelperError): """An accessed object cannot be used by the requested operation.""" +@dataclass(frozen=True) +class _WindowsDirectory: + path: Path + handle: int + + @dataclass class _WalkDirectories: root: Path - root_handle: int | Path + root_handle: int | _WindowsDirectory flags: int resources: ExitStack - opened: dict[Path, Path] = field(default_factory=dict) + opened: dict[Path, _WindowsDirectory] = field(default_factory=dict) current: tuple[Path, int] | None = field(default=None, init=False) def __post_init__(self) -> None: @@ -95,23 +102,27 @@ def _posix_directory(self, path: Path, root_fd: int) -> int: parent = child return parent - def directory(self, path: Path) -> int | Path: + def directory(self, path: Path) -> int | _WindowsDirectory: path.relative_to(self.root) if isinstance(self.root_handle, int): return self._posix_directory(path, self.root_handle) if path == self.root: return self.root_handle if path not in self.opened: - self.directory(path.parent) + parent = self.directory(path.parent) + if not isinstance(parent, _WindowsDirectory): + raise FileHelperError("Expected a native Windows directory") try: - self.resources.enter_context(_windows_handle(path, directory=True)) + handle = self.resources.enter_context(_windows_handle( + Path(path.name), directory=True, parent=parent.handle + )) except OSError as exc: if exc.errno == errno.EINVAL and getattr(exc, "winerror", None) is None: raise NotADirectoryError( errno.ENOTDIR, "Expected a directory", str(path) ) from exc raise - self.opened[path] = path + self.opened[path] = _WindowsDirectory(path, handle) return self.opened[path] @@ -297,7 +308,9 @@ def _walk( continue candidate = current / part try: - mode = self._entry_mode(candidate, dir_fd=directory_fd) + mode = self._deletion_entry_mode(candidate, parent=directory) if ( + isinstance(directory, _WindowsDirectory) + ) else self._entry_mode(candidate, dir_fd=directory_fd) except FileNotFoundError: if not allow_missing: raise @@ -313,7 +326,10 @@ def _walk( return candidate if candidate in links or len(links) >= 40: raise UnsupportedPathError(f"Symlink cycle or excessive chain: {candidate}") - if directory_fd is None: + if isinstance(directory, _WindowsDirectory): + with _windows_entry(directory.handle, part) as handle: + target = Path(_windows_readlink(handle)) + elif directory_fd is None: target = Path(os.readlink(candidate)) else: target = Path(os.readlink(part, dir_fd=directory_fd)) @@ -332,7 +348,7 @@ def _walk( def _path( self, path: Path | str, *, allow_missing: bool = False, follow_leaf: bool = True, - root_handle: int | Path | None = None, + root_handle: int | _WindowsDirectory | None = None, ) -> Path: self._require_open() original = Path(path) @@ -343,7 +359,7 @@ def _path( directories = ( _WalkDirectories( self._canonical_root, root_handle, - 0 if isinstance(root_handle, Path) else self._directory_flags(), resources, + 0 if isinstance(root_handle, _WindowsDirectory) else self._directory_flags(), resources, ) if root_handle is not None else None ) @@ -561,7 +577,7 @@ def _check_deletion_root(self, current: os.stat_result) -> None: raise FileHelperError(f"Established root changed during deletion: {self.root}") @contextmanager - def _deletion_root(self) -> Iterator[int | Path]: + def _deletion_root(self) -> Iterator[int | _WindowsDirectory]: """Borrow the root pinned when this helper was established.""" self._require_open() if self._root_anchor is None: @@ -587,19 +603,20 @@ def _deletion_root(self) -> Iterator[int | Path]: retained = _windows_file_id(reference, kind=identity.kind) if selected != retained: raise FileHelperError(f"Established root changed during deletion: {self.root}") - yield self._root_anchor + yield _WindowsDirectory(self._canonical_root, handle) else: self._check_deletion_root(self._canonical_root.stat()) yield self._root_anchor @contextmanager def _child_deletion_directory( - self, parent: int | Path, name: str, *, readable: bool = False - ) -> Iterator[int | Path]: - if isinstance(parent, Path): - child = parent / name - with _windows_handle(child, directory=True): - yield child + self, parent: int | _WindowsDirectory, name: str, *, readable: bool = False + ) -> Iterator[int | _WindowsDirectory]: + if isinstance(parent, _WindowsDirectory): + with _windows_handle( + Path(name), directory=True, parent=parent.handle + ) as handle: + yield _WindowsDirectory(parent.path / name, handle) return flags = ( os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) @@ -612,7 +629,9 @@ def _child_deletion_directory( os.close(child) @contextmanager - def _deletion_directory(self, path: Path, *, root: int | Path) -> Iterator[int | Path]: + def _deletion_directory( + self, path: Path, *, root: int | _WindowsDirectory + ) -> Iterator[int | _WindowsDirectory]: parts = path.relative_to(self._canonical_root).parts if isinstance(root, int): with ExitStack() as resources: @@ -627,9 +646,19 @@ def _deletion_directory(self, path: Path, *, root: int | Path) -> Iterator[int | parent = resources.enter_context(self._child_deletion_directory(parent, part)) yield parent - def _deletion_entry_mode(self, path: Path, *, parent: int | Path) -> int: - if isinstance(parent, Path): - return self._entry_mode(parent / path.name) + @staticmethod + def _windows_entry_mode(handle: int, path: Path) -> int: + try: + return _windows_mode(handle) + except OSError as exc: + if exc.errno == errno.ELOOP and getattr(exc, "winerror", None) is None: + raise UnsupportedPathError(f"Unsupported Windows reparse point: {path}") from exc + raise + + def _deletion_entry_mode(self, path: Path, *, parent: int | _WindowsDirectory) -> int: + if isinstance(parent, _WindowsDirectory): + with _windows_entry(parent.handle, path.name) as handle: + return self._windows_entry_mode(handle, path) return self._entry_mode(path, dir_fd=parent) def _deletion_kind(self, path: Path, mode: int) -> bool: @@ -644,7 +673,7 @@ def _deletion_kind(self, path: Path, mode: int) -> bool: return True def _deletion_plan( - self, path: Path, *, recursive: bool, parent: int | Path + self, path: Path, *, recursive: bool, parent: int | _WindowsDirectory ) -> _DeletionEntry: if isinstance(parent, int): return self._posix_deletion_plan(path, recursive=recursive, parent=parent) @@ -652,8 +681,9 @@ def _deletion_plan( children: tuple[_DeletionEntry, ...] = () if directory and recursive: with self._child_deletion_directory(parent, path.name, readable=True) as child_directory: - with os.scandir(child_directory) as entries: - names = sorted(entry.name for entry in entries) + if not isinstance(child_directory, _WindowsDirectory): + raise FileHelperError("Expected a native Windows directory") + names = sorted(_windows_directory_names(child_directory.handle)) child_plans = [] for name in names: child_plans.append( @@ -695,25 +725,21 @@ def _posix_deletion_plan( pending.append((entry / name, None)) return completed[path] - def _delete_plan(self, entry: _DeletionEntry, *, parent: int | Path) -> None: + def _delete_plan(self, entry: _DeletionEntry, *, parent: int | _WindowsDirectory) -> None: if isinstance(parent, int): self._posix_delete_plan(entry, parent=parent) return - directory = self._deletion_kind( - entry.path, self._deletion_entry_mode(entry.path, parent=parent) - ) - if directory != entry.directory: - raise FileHelperError(f"Destination changed during deletion: {entry.path}") - if directory and entry.children: - with self._child_deletion_directory(parent, entry.path.name) as child_directory: + with _windows_entry(parent.handle, entry.path.name, deleting=True) as handle: + directory = self._deletion_kind( + entry.path, self._windows_entry_mode(handle, entry.path) + ) + if directory != entry.directory: + raise FileHelperError(f"Destination changed during deletion: {entry.path}") + if directory and entry.children: + child_directory = _WindowsDirectory(entry.path, handle) for child in entry.children: self._delete_plan(child, parent=child_directory) - leaf = parent / entry.path.name if isinstance(parent, Path) else entry.path.name - directory_fd = None if isinstance(parent, Path) else parent - if directory: - os.rmdir(leaf, dir_fd=directory_fd) - else: - os.unlink(leaf, dir_fd=directory_fd) + _windows_delete(handle) def _posix_delete_plan(self, entry: _DeletionEntry, *, parent: int) -> None: pending = [(entry, False)] diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index fb5201a8bf..a7d13d5bc2 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -1,15 +1,17 @@ """Ownership and native Windows evidence for shared handle primitives.""" from concurrent.futures import ThreadPoolExecutor -from contextlib import contextmanager +from contextlib import contextmanager, nullcontext import ctypes from ctypes import wintypes import errno import gc import os +import stat from pathlib import Path import sys import subprocess +import struct from types import SimpleNamespace import weakref @@ -361,6 +363,8 @@ def query(handle, kind, output, size): CloseHandle=native.CloseHandle, GetFileInformationByHandleEx=query, GetFileInformationByHandle=native.GetFileInformationByHandle, + SetFileInformationByHandle=native.SetFileInformationByHandle, + DeviceIoControl=native.DeviceIoControl, ) def load(name, *args, **kwargs): @@ -1131,6 +1135,203 @@ def test_windows_child_open_rejects_oversized_unicode_name(): assert error.value.errno == errno.ENAMETOOLONG +@pytest.mark.parametrize("deleting", [False, True]) +@pytest.mark.parametrize("body_failure", [False, True]) +def test_windows_entry_open_uses_native_parent_and_closes_its_handle( + monkeypatch, deleting, body_failure +): + closed = [] + + def create(output, access, attributes, status, allocation, file_attributes, + sharing, disposition, options, ea, ea_length): + assert access == 0x100080 | (0x10000 if deleting else 0) + assert attributes._obj.RootDirectory == 51 + assert attributes._obj.Attributes == 0x40 + assert attributes._obj.ObjectName.contents.Buffer == "entry" + assert sharing == 7 and disposition == 1 and options == 0x200020 + output._obj.value = 77 + return 0 + + def dos_error(status): + pytest.fail("A successful native open must not query an error") + + monkeypatch.setattr(ctypes, "WinDLL", lambda *args: SimpleNamespace( + NtCreateFile=create, RtlNtStatusToDosError=dos_error, + ), raising=False) + monkeypatch.setattr(handles, "_windows_api", lambda: SimpleNamespace(CloseHandle=closed.append)) + expected = pytest.raises(RuntimeError, match="body failed") if body_failure else nullcontext() + with expected: + with handles._windows_entry(51, "entry", deleting=deleting) as handle: + assert handle == 77 and not closed + if body_failure: + raise RuntimeError("body failed") + assert closed == [77] + + +@pytest.mark.parametrize( + ("attributes", "tag", "mode"), + [(0, 0, stat.S_IFREG), (0x10, 0, stat.S_IFDIR), (0x400, 0xA000000C, stat.S_IFLNK)], +) +def test_windows_mode_queries_the_opened_object(monkeypatch, attributes, tag, mode): + def query(handle, kind, output, size): + assert handle == 51 and kind == 9 and size == 8 + output._obj.attributes = attributes + output._obj.tag = tag + return True + + monkeypatch.setattr(handles, "_windows_api", lambda: SimpleNamespace( + GetFileInformationByHandleEx=query, + )) + assert handles._windows_mode(51) == mode + + +def test_windows_mode_rejects_non_symlink_reparse_points(monkeypatch): + def query(handle, kind, output, size): + output._obj.attributes = 0x410 + output._obj.tag = 0xA0000003 + return True + + monkeypatch.setattr(handles, "_windows_api", lambda: SimpleNamespace( + GetFileInformationByHandleEx=query, + )) + with pytest.raises(OSError, match="Unsupported Windows reparse"): + handles._windows_mode(51) + + +@pytest.mark.parametrize("target", ["directory/../caf\u00e9", "name\ud800", "\\??\\C:\\target"]) +def test_windows_readlink_reads_the_opened_reparse_buffer(monkeypatch, target): + encoded = target.encode("utf-16-le", errors="surrogatepass") + relative = not target.startswith("\\??\\") + data = struct.pack(" Date: Tue, 10 Nov 2026 17:04:53 -0600 Subject: [PATCH 15/16] test: exercise native Windows deletion boundaries Hook parent replacement at native entry disposition and enumeration instead of pathname functions that Windows deletion no longer uses. Simulate reparse metadata through native handle queries and retain actual acquired-parent and final-validation coverage. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_file_helper.py | 107 ++++++++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 17 deletions(-) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 82e716adb8..59ef0233f7 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -95,6 +95,30 @@ def remove(handle): ) +def _before_windows_removal(monkeypatch, name, before): + selected = set() + original_entry = file_helper._windows_entry + original_delete = file_helper._windows_delete + + @contextmanager + def entry(parent, child, *, deleting=False): + with original_entry(parent, child, deleting=deleting) as handle: + if deleting and child == name: + selected.add(handle) + try: + yield handle + finally: + selected.discard(handle) + + def remove(handle): + if handle in selected: + before() + original_delete(handle) + + monkeypatch.setattr(file_helper, "_windows_entry", entry) + monkeypatch.setattr(file_helper, "_windows_delete", remove) + + @pytest.mark.parametrize("allow", [False, True]) def test_plain_operations(root, allow): files = FileHelper(root, allow_symlinks=allow) @@ -1019,9 +1043,9 @@ def test_delete_parent_exchange_at_removal(root, link, monkeypatch, allow, kind) remove = getattr(os, primitive) attempted = False - def exchange(path, *args, **kwargs): + def exchange_parent(): nonlocal attempted - if not attempted and (Path(path) == victim or str(path) == victim.name): + if not attempted: attempted = True try: parent.rename(retained) @@ -1030,9 +1054,15 @@ def exchange(path, *args, **kwargs): raise else: link(parent, outside) + def exchange(path, *args, **kwargs): + if Path(path) == victim or str(path) == victim.name: + exchange_parent() return remove(path, *args, **kwargs) - monkeypatch.setattr(os, primitive, exchange) + if os.name == "nt": + _before_windows_removal(monkeypatch, victim.name, exchange_parent) + else: + monkeypatch.setattr(os, primitive, exchange) try: FileHelper(root, allow_symlinks=allow).delete( tree if kind.startswith("recursive-") else victim, @@ -1064,7 +1094,7 @@ def test_recursive_delete_parent_exchange_at_scan(root, link, monkeypatch, allow attempted = False observed = [] - def exchange(path): + def exchange_parent(): nonlocal attempted if not attempted: attempted = True @@ -1075,6 +1105,8 @@ def exchange(path): raise else: link(tree, outside) + def exchange(path): + exchange_parent() entries = scan(path) # Capture enumeration as well as removal: no outside entry is visited. with entries: @@ -1086,7 +1118,18 @@ def exchange(path): def scanned(entries): yield iter(entries) - monkeypatch.setattr(os, "scandir", exchange) + if os.name == "nt": + native_names = file_helper._windows_directory_names + + def exchange_native(handle): + exchange_parent() + names = native_names(handle) + observed.extend(names) + return names + + monkeypatch.setattr(file_helper, "_windows_directory_names", exchange_native) + else: + monkeypatch.setattr(os, "scandir", exchange) try: FileHelper(root, allow_symlinks=allow).delete(tree, recursive=True) except (FileHelperError, OSError): @@ -1109,7 +1152,7 @@ def test_delete_link_parent_exchange_at_removal(root, link, monkeypatch, recursi unlink = os.unlink attempted = False - def exchange(path, *args, **kwargs): + def exchange_parent(): nonlocal attempted if not attempted: attempted = True @@ -1120,9 +1163,14 @@ def exchange(path, *args, **kwargs): raise else: link(parent, outside) + def exchange(path, *args, **kwargs): + exchange_parent() return unlink(path, *args, **kwargs) - monkeypatch.setattr(os, "unlink", exchange) + if os.name == "nt": + _before_windows_removal(monkeypatch, "victim", exchange_parent) + else: + monkeypatch.setattr(os, "unlink", exchange) FileHelper(root, allow_symlinks=True).delete(parent / "victim", recursive=recursive) assert attempted assert (outside / "victim").read_text() == "keep" @@ -1155,10 +1203,23 @@ def exchange(self, path, **kwargs): link(parent, outside) return result - monkeypatch.setattr(FileHelper, "_deletion_entry_mode", exchange) + if os.name == "nt": + def after_validation(): + nonlocal checks + checks += 1 + try: + parent.rename(retained) + except PermissionError: + pass + else: + link(parent, outside) + + _before_windows_removal(monkeypatch, "victim", after_validation) + else: + monkeypatch.setattr(FileHelper, "_deletion_entry_mode", exchange) with FileHelper(root, allow_symlinks=allow) as files: files.delete(parent / "victim") - assert checks == 2 + assert checks == (1 if os.name == "nt" else 2) assert (outside / "victim").read_text() == "keep" assert not ((retained if retained.exists() else parent) / "victim").exists() @@ -2203,17 +2264,19 @@ def test_native_windows_link_resolution_blocks_accessed_parent_exchange(root, li (selected / "victim").write_text("selected") link(parent / "alias", selected) attempted = False - lstat = Path.lstat + original_entry = file_helper._windows_entry - def exchange(path, **kwargs): + @contextmanager + def exchange(directory, name, *, deleting=False): nonlocal attempted - if path == parent / "alias" and not attempted: - attempted = True - with pytest.raises(PermissionError): - parent.rename(root / "retained") - return lstat(path, **kwargs) + with original_entry(directory, name, deleting=deleting) as handle: + if name == "alias" and not attempted: + attempted = True + with pytest.raises(PermissionError): + parent.rename(root / "retained") + yield handle - monkeypatch.setattr(Path, "lstat", exchange) + monkeypatch.setattr(file_helper, "_windows_entry", exchange) with FileHelper(root, allow_symlinks=True) as files: files.delete("parent/alias/victim") assert attempted @@ -3640,6 +3703,7 @@ def test_directory_reparse_entries_rejected_before_traversal_portably( (junction / "keep").write_text("keep") lstat = Path.lstat stat_entry = os.stat + junction_identity = lstat(junction) def as_reparse(result): return SimpleNamespace( @@ -3662,6 +3726,15 @@ def directory_reparse_stat(path, *args, **kwargs): monkeypatch.setattr(Path, "lstat", directory_reparse) monkeypatch.setattr(os, "stat", directory_reparse_stat) + if os.name == "nt": + native_mode = file_helper._windows_mode + + def reparse_mode(handle): + if os.path.samestat(file_helper._windows_stat(handle), junction_identity): + raise OSError(errno.ELOOP, "Unsupported Windows reparse point") + return native_mode(handle) + + monkeypatch.setattr(file_helper, "_windows_mode", reparse_mode) files = FileHelper(root, allow_symlinks=allow) for operation in ( lambda: files.read_bytes(junction / "keep"), From b81ebb8bdb3952e8de04aebffe7ff8202728b898 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:52:33 -0600 Subject: [PATCH 16/16] fix: synchronize relative Windows directory I/O Request FILE_SYNCHRONOUS_IO_NONALERT together with SYNCHRONIZE for relative directory opens, preserving existing type and no-follow flags. Update native ABI assertions and cover the actual Windows directory handle mode and enumeration contract. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 5 ++++- src/specify_cli/_file_handles.py | 8 ++++---- tests/test_file_handles.py | 32 ++++++++++++++++++++++++++++++-- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index a429721f1e..6fad90eb33 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -188,7 +188,10 @@ parent directories throughout link resolution, before classifying child entries or reading link targets. Each child is opened relative to its native parent handle, not by reconstructing an absolute pathname. Metadata and symbolic-link targets are read from opened leaf handles; directory names are enumerated from -the directory handle. These accessed directories also require +the directory handle. Relative native opens request both +`FILE_SYNCHRONOUS_IO_NONALERT` and `SYNCHRONIZE`, including directories, so +enumeration cannot consume a buffer before a pending directory query completes. +These accessed directories also require directory-list/read-attribute access. The resolution handles are released after selection; native mutation parents and root ancestry remain retained during preflight and removal. Directory sharing locks alone do not prevent in-place diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index c908d60945..cdeddafb27 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -59,11 +59,11 @@ def _open_windows_child( ) handle = wintypes.HANDLE() status = abi._IoStatusBlock() - options = 0x00200000 | ( - 0x1 if directory is True else (0x40 | 0x20 if directory is False else 0x20) - ) + options = 0x00200000 | 0x20 # FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT. + if directory is not None: + options |= 0x1 if directory else 0x40 result = create( - ctypes.byref(handle), access | (0 if directory else 0x100000), + ctypes.byref(handle), access | 0x100000, # SYNCHRONIZE is required for synchronous I/O. ctypes.byref(attributes), ctypes.byref(status), None, 0, sharing, 1, options, None, 0, ) diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index a7d13d5bc2..520edb437a 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -1043,9 +1043,9 @@ def create_child(output, access, attributes, status, allocation, file_attributes name = attributes.ObjectName.contents length = len(child_name.encode("utf-16-le", errors="surrogatepass")) assert (name.Buffer, name.Length, name.MaximumLength) == (child_name, length, length + 2) - assert access == (0xA0 if directory else 0x80100000) + assert access == (0x1000A0 if directory else 0x80100000) assert sharing == 3 and disposition == 1 - assert options == (0x200001 if directory else 0x200060) + assert options == (0x200021 if directory else 0x200060) assert allocation is None and file_attributes == 0 and ea is None and ea_length == 0 assert ctypes.sizeof(status._obj) == 2 * ctypes.sizeof(ctypes.c_void_p) if failure == "status": @@ -1096,6 +1096,34 @@ def load(name, **kwargs): assert {role: len(declarations) for role, declarations in abi_types.items()} == dict.fromkeys(abi_types, 1) +@pytest.mark.skipif(os.name != "nt", reason="Native Windows synchronous directory I/O") +@pytest.mark.parametrize("traverse", [False, True]) +def test_native_windows_relative_directory_handle_has_synchronous_mode(tmp_path, traverse): + from specify_cli import _windows_abi as abi + + directory = tmp_path / "directory" + directory.mkdir() + (directory / "file").write_text("selected") + native = ctypes.WinDLL("ntdll") + query = native.NtQueryInformationFile + query.argtypes = [ + wintypes.HANDLE, abi._IoStatusBlockPointer, wintypes.LPVOID, + ctypes.c_uint32, ctypes.c_int, + ] + query.restype = ctypes.c_int32 + status = abi._IoStatusBlock() + mode = ctypes.c_uint32() + with _windows_handle(tmp_path, directory=True) as parent: + with _windows_handle( + Path("directory"), directory=True, parent=parent, traverse=traverse, + ) as child: + result = query(child, ctypes.byref(status), ctypes.byref(mode), 4, 16) + assert result == 0 + assert mode.value & 0x20 # FILE_SYNCHRONOUS_IO_NONALERT. + if not traverse: + assert handles._windows_directory_names(child) == ["file"] + + @pytest.mark.skipif(os.name != "nt", reason="Native Windows repeated-read lifetime regression") @pytest.mark.parametrize("allow", [False, True]) def test_repeated_windows_reads_do_not_accumulate_abi_types(tmp_path, allow):