From 241aab055a8acd929f04c6e81370ba5d3c7a32ff Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 16:28:08 -0600 Subject: [PATCH 01/10] fix: use handle-based FileHelper reads Preserve supported contained links while opening reads through verified filesystem handles. Share Windows handle management, cover path replacement and descriptor cleanup, and document the read 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 | 22 +- src/specify_cli/_file_handles.py | 60 +++++ src/specify_cli/file_helper.py | 38 ++- src/specify_cli/workflows/_catalog_cache.py | 59 +--- .../presets/_skill_cleanup_helpers.py | 7 +- tests/test_file_helper.py | 254 ++++++++++++++++++ 6 files changed, 373 insertions(+), 67 deletions(-) create mode 100644 src/specify_cli/_file_handles.py diff --git a/design/file-helper.md b/design/file-helper.md index b9dbcddd2f..f727b4c511 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -207,15 +207,27 @@ entries, not reject a whole project for excluded/unrelated links. ## Safety limits and migration +Reads resolve supported contained links under the existing policy, then open +the resolved hierarchy without following substituted links. POSIX reads use +descriptor-relative directory traversal with `O_DIRECTORY` and `O_NOFOLLOW`, +and open the leaf with `O_NOFOLLOW` and `O_NONBLOCK`. Windows reads pin each +resolved directory and leaf with non-reparse handles that withhold delete +sharing. Both paths validate the opened file with `fstat` before reading its +descriptor, and release all handles on success or failure. Platforms without +the required safe-open primitives fail explicitly rather than fall back to +unchecked pathname reads. A concurrent replacement can fail the read or leave +it reading the already-open original file, but cannot redirect it through a +substituted symlink. This does not freeze file contents against in-place edits. + 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 +deletion rechecks each accessed entry. Mutation 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. +results. Protect mutations against concurrent untrusted changes separately; +descriptor-relative or platform-specific mutation mechanisms remain future +work. 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 diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py new file mode 100644 index 0000000000..be0c016991 --- /dev/null +++ b/src/specify_cli/_file_handles.py @@ -0,0 +1,60 @@ +"""Handle-based filesystem primitives shared by confined I/O consumers.""" + +from collections.abc import Iterator +from contextlib import contextmanager +import errno +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 + + # OPEN_REPARSE_POINT checks the entry itself. Directory handles need + # FILE_LIST_DIRECTORY: attribute-only handles do not prevent renaming. + 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 filesystem object 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..3e6769aacc 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -1,6 +1,7 @@ """Scoped filesystem operations; see design/file-helper.md for the contract.""" from dataclasses import dataclass, field +from contextlib import ExitStack import codecs import errno import os @@ -9,6 +10,7 @@ import tempfile from typing import TypeVar +from specify_cli._file_handles import windows_handle _PathType = TypeVar("_PathType", bound=PurePath) @@ -162,7 +164,41 @@ def mkdir( def read_bytes(self, path: Path | str) -> bytes: destination = self._path(path) self._require_file(destination) - return destination.read_bytes() + parts = destination.relative_to(self._canonical_root).parts + with ExitStack() as stack: + if os.name == "nt": + current = self._canonical_root + stack.enter_context(windows_handle(current, directory=True)) + for part in parts[:-1]: + current /= part + stack.enter_context(windows_handle(current, directory=True)) + stack.enter_context(windows_handle(destination, directory=False)) + fd = os.open(destination, os.O_RDONLY | os.O_BINARY) + else: + if not ( + getattr(os, "O_NOFOLLOW", 0) + and getattr(os, "O_DIRECTORY", 0) + and getattr(os, "O_NONBLOCK", 0) + and os.open in os.supports_dir_fd + ): + raise OSError(errno.ENOTSUP, "Safe scoped reads are unsupported on this platform") + flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW + flags |= getattr(os, "O_CLOEXEC", 0) + parent = os.open(self._canonical_root, flags) + stack.callback(os.close, parent) + for part in parts[:-1]: + parent = os.open(part, flags, dir_fd=parent) + stack.callback(os.close, parent) + fd = os.open( + parts[-1], + os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0), + dir_fd=parent, + ) + stack.callback(os.close, fd) + if not stat.S_ISREG(os.fstat(fd).st_mode): + raise UnsupportedPathError(f"Expected a regular file: {destination}") + with os.fdopen(fd, "rb", closefd=False) as stream: + return stream.read() def read_text(self, path: Path | str, *, encoding: str = "utf-8") -> str: return self.read_bytes(path).decode(encoding) diff --git a/src/specify_cli/workflows/_catalog_cache.py b/src/specify_cli/workflows/_catalog_cache.py index 5a64609bc0..58697fee6d 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 as _windows_handle @contextmanager diff --git a/tests/specify_cli/presets/_skill_cleanup_helpers.py b/tests/specify_cli/presets/_skill_cleanup_helpers.py index 6695c9b416..d870e525ab 100644 --- a/tests/specify_cli/presets/_skill_cleanup_helpers.py +++ b/tests/specify_cli/presets/_skill_cleanup_helpers.py @@ -65,7 +65,8 @@ def validate_then_swap(self, directory, **kwargs): monkeypatch.setattr(PresetManager, "_validate_skill_subdir", validate_then_swap) if fallback: - monkeypatch.setattr(os, "supports_dir_fd", set()) + # Disable descriptor-relative cleanup without disabling safe project reads. + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd - {os.rmdir}) result = runner.invoke(app, command, catch_exceptions=False) assert swapped @@ -97,7 +98,7 @@ def check_cleanup_preserves_user_files( reference.parent.mkdir() reference.write_bytes(original) if fallback: - monkeypatch.setattr(os, "supports_dir_fd", set()) + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd - {os.rmdir}) result = runner.invoke(app, command, catch_exceptions=False) @@ -133,7 +134,7 @@ def check_cleanup_rollback_preserves_directory_mode( reference.parent.mkdir() reference.write_bytes(b"User reference before cleanup") if fallback: - monkeypatch.setattr(os, "supports_dir_fd", set()) + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd - {os.rmdir}) snapshot = _FileJournal._snapshot snapshotted = [] diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index a04b9e0a34..3588d1beef 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -1,6 +1,7 @@ """Operation-level evidence for the scoped FileHelper foundation.""" from dataclasses import FrozenInstanceError +from contextlib import contextmanager import errno import os from pathlib import Path, PureWindowsPath @@ -9,6 +10,7 @@ import pytest +from specify_cli import file_helper from specify_cli.file_helper import ( FileHelper, FileHelperError, @@ -60,6 +62,258 @@ def test_plain_operations(root, allow): assert list(root.iterdir()) == [] +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("operation", ["read_bytes", "read_text"]) +@pytest.mark.parametrize("kind", ["leaf", "parent", "resolved-link"]) +def test_read_does_not_follow_replacements_after_validation( + root, link, monkeypatch, allow, operation, kind +): + if kind == "resolved-link" and not allow: + pytest.skip("Contained links require allow mode") + directory = root / "directory" + directory.mkdir() + (directory / "file").write_bytes(b"inside") + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_bytes(b"outside-secret") + selected = directory / "file" + if kind == "resolved-link": + selected = link(root / "alias", selected) + original_path = FileHelper._path + + def exchange(self, path, **kwargs): + destination = original_path(self, path, **kwargs) + if kind == "parent": + directory.rename(root / "retained") + link(directory, outside) + else: + destination.unlink() + link(destination, outside / "file") + return destination + + monkeypatch.setattr(FileHelper, "_path", exchange) + try: + content = getattr(FileHelper(root, allow_symlinks=allow), operation)(selected) + except (OSError, FileHelperError): + pass + else: + assert content in (b"inside", "inside") + + +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("kind", ["leaf", "parent"]) +def test_read_uses_opened_file_after_replacement(root, link, monkeypatch, allow, kind): + directory = root / "directory" + directory.mkdir() + selected = directory / "file" + selected.write_bytes(b"inside") + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_bytes(b"outside-secret") + original_fstat = os.fstat + swapped = False + + def exchange(fd): + nonlocal swapped + info = original_fstat(fd) + if swapped: + return info + if kind == "leaf": + source, replacement = selected, outside / "file" + else: + source, replacement = directory, outside + if os.name == "nt": + with pytest.raises(OSError): + source.rename(root / "retained") + else: + source.rename(root / "retained") + link(source, replacement) + swapped = True + return info + + monkeypatch.setattr("specify_cli.file_helper.os.fstat", exchange) + assert FileHelper(root, allow_symlinks=allow).read_bytes(selected) == b"inside" + assert swapped + if os.name == "nt": + source = selected if kind == "leaf" else directory + source.rename(root / "retained") + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor-relative open regression") +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("phase", ["parent", "leaf"]) +def test_read_rejects_replacement_during_open(root, link, monkeypatch, allow, phase): + directory = root / "directory" + directory.mkdir() + selected = directory / "file" + selected.write_bytes(b"inside") + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_bytes(b"outside-secret") + original_open = os.open + swapped = False + + def exchange(path, flags, *args, **kwargs): + nonlocal swapped + if path == ("directory" if phase == "parent" else "file"): + source = directory if phase == "parent" else selected + source.rename(root / "retained") + link(source, outside if phase == "parent" else outside / "file") + swapped = True + return original_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd | {exchange}) + monkeypatch.setattr("specify_cli.file_helper.os.open", exchange) + with pytest.raises(OSError): + FileHelper(root, allow_symlinks=allow).read_bytes(selected) + assert swapped + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor-relative open regression") +def test_read_pins_parent_before_opening_leaf(root, link, monkeypatch): + directory = root / "directory" + directory.mkdir() + (directory / "file").write_bytes(b"inside") + outside = root.parent / "outside" + outside.mkdir() + (outside / "file").write_bytes(b"outside-secret") + original_open = os.open + swapped = False + + def exchange(path, flags, *args, **kwargs): + nonlocal swapped + if path == "file": + directory.rename(root / "retained") + link(directory, outside) + swapped = True + return original_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd | {exchange}) + monkeypatch.setattr("specify_cli.file_helper.os.open", exchange) + assert FileHelper(root).read_bytes("directory/file") == b"inside" + assert swapped + + +@pytest.mark.parametrize("failure", [None, "parent", "leaf", "fstat"]) +def test_windows_read_pins_entire_resolved_hierarchy(root, monkeypatch, failure): + directory = root / "directory" + directory.mkdir() + selected = directory / "file" + selected.write_bytes(b"inside") + pinned = [] + seen = [] + + @contextmanager + def pin(path, *, directory): + if path.name == {"parent": "directory", "leaf": "file"}.get(failure): + raise OSError("injected handle failure") + pinned.append(path) + seen.append((path, directory)) + try: + yield 1 + finally: + assert pinned.pop() == path + + def checked_fstat(fd): + assert pinned == [root.resolve(), directory.resolve(), selected.resolve()] + if failure == "fstat": + raise OSError("injected fstat failure") + return os.fstat(fd) + + windows_os = SimpleNamespace( + name="nt", + O_RDONLY=os.O_RDONLY, + O_BINARY=getattr(os, "O_BINARY", 0), + open=os.open, + close=os.close, + fdopen=os.fdopen, + fstat=checked_fstat, + ) + monkeypatch.setattr(file_helper, "os", windows_os) + monkeypatch.setattr(file_helper, "windows_handle", pin) + files = FileHelper(root) + if failure: + with pytest.raises(OSError, match="injected"): + files.read_bytes(selected) + else: + assert files.read_bytes(selected) == b"inside" + assert seen == [ + (root.resolve(), True), + (directory.resolve(), True), + (selected.resolve(), False), + ] + assert pinned == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor cleanup regression") +@pytest.mark.parametrize("failure", ["parent", "leaf", "fstat", "fdopen", "read"]) +def test_read_closes_descriptors_on_failure(root, monkeypatch, failure): + (root / "directory").mkdir() + selected = root / "directory/file" + selected.write_bytes(b"inside") + opened = [] + original_open = os.open + + def recording_open(path, flags, *args, **kwargs): + if str(path) == {"parent": "directory", "leaf": "file"}.get(failure): + raise PermissionError("injected open failure") + fd = original_open(path, flags, *args, **kwargs) + opened.append(fd) + return fd + + def fail(*args, **kwargs): + raise OSError("injected read failure") + + class FailedRead: + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + read = fail + + with monkeypatch.context() as patch: + patch.setattr(os, "supports_dir_fd", os.supports_dir_fd | {recording_open}) + patch.setattr(os, "open", recording_open) + if failure in ("fstat", "fdopen"): + patch.setattr(os, failure, fail) + elif failure == "read": + patch.setattr(os, "fdopen", lambda *args, **kwargs: FailedRead()) + with pytest.raises(OSError, match="injected"): + FileHelper(root).read_bytes(selected) + assert opened + for fd in opened: + with pytest.raises(OSError) as error: + os.fstat(fd) + assert error.value.errno == errno.EBADF + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX special-file replacement regression") +def test_read_checks_opened_type_without_blocking_on_fifo(root, monkeypatch): + selected = root / "file" + selected.write_bytes(b"inside") + original_check = FileHelper._require_file + + def exchange(path): + original_check(path) + path.unlink() + os.mkfifo(path) + + monkeypatch.setattr(FileHelper, "_require_file", staticmethod(exchange)) + with pytest.raises(UnsupportedPathError, match="regular file"): + FileHelper(root).read_bytes(selected) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX capability refusal regression") +def test_read_refuses_unsafe_platform_fallback(root, monkeypatch): + (root / "file").write_bytes(b"inside") + monkeypatch.setattr(os, "supports_dir_fd", set()) + with pytest.raises(OSError) as error: + FileHelper(root).read_bytes("file") + assert error.value.errno == errno.ENOTSUP + + @pytest.mark.parametrize("allow", [False, True]) def test_overwrite_text_preserves_inode_and_native_text_behavior(root, allow): files = FileHelper(root, allow_symlinks=allow) From e5ad20a7887b6564326d4558cef02f47e84176d0 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:13:41 -0600 Subject: [PATCH 02/10] fix: refine FileHelper handle reads Read directly from duplicated Windows handles, retain search-only directory access, and close superseded POSIX parents. Extend coverage for handle ownership, permissions, descriptor limits, and native Windows 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 | 15 ++-- src/specify_cli/_file_handles.py | 26 +++++- src/specify_cli/file_helper.py | 36 ++++---- tests/test_file_handles.py | 150 +++++++++++++++++++++++++++++++ tests/test_file_helper.py | 140 ++++++++++++++++++++++++++++- 5 files changed, 341 insertions(+), 26 deletions(-) create mode 100644 tests/test_file_handles.py diff --git a/design/file-helper.md b/design/file-helper.md index f727b4c511..9f64cc9f8d 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -209,11 +209,16 @@ entries, not reject a whole project for excluded/unrelated links. Reads resolve supported contained links under the existing policy, then open the resolved hierarchy without following substituted links. POSIX reads use -descriptor-relative directory traversal with `O_DIRECTORY` and `O_NOFOLLOW`, -and open the leaf with `O_NOFOLLOW` and `O_NONBLOCK`. Windows reads pin each -resolved directory and leaf with non-reparse handles that withhold delete -sharing. Both paths validate the opened file with `fstat` before reading its -descriptor, and release all handles on success or failure. Platforms without +descriptor-relative directory traversal with `O_DIRECTORY` and `O_NOFOLLOW`. +Directory descriptors use `O_SEARCH` or `O_PATH` where available so searchable +directories need not be listable, and each superseded parent is closed as soon +as its child is open. The leaf uses `O_NOFOLLOW` and `O_NONBLOCK`. Windows reads +pin each resolved directory and leaf with non-reparse handles that withhold +delete sharing; directory handles also withhold write sharing to prevent +in-place reparse changes during traversal. Reads duplicate the validated leaf +handle into an independently owned descriptor, never reopen its pathname. +Both paths validate the opened file with `fstat` before reading its descriptor, +and release all handles on success or failure. Platforms without the required safe-open primitives fail explicitly rather than fall back to unchecked pathname reads. A concurrent replacement can fail the read or leave it reading the already-open original file, but cannot redirect it through a diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index be0c016991..d5f9bd32a5 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -3,11 +3,12 @@ from collections.abc import Iterator from contextlib import contextmanager import errno +import os from pathlib import Path @contextmanager -def windows_handle(path: Path, *, directory: bool) -> Iterator[int]: +def windows_handle(path: Path, *, directory: bool, share_write: bool = True) -> Iterator[int]: """Pin a non-reparse path by withholding Windows delete sharing.""" import ctypes from ctypes import wintypes @@ -39,7 +40,7 @@ class AttributeTagInfo(ctypes.Structure): handle = create( str(path), (0x80 | 0x1) if directory else 0x80000000, - 0x3, + 0x1 | (0x2 if share_write else 0), None, 3, 0x00200000 | 0x02000000, @@ -58,3 +59,24 @@ class AttributeTagInfo(ctypes.Structure): yield handle finally: close(handle) + + +@contextmanager +def windows_read_fd(handle: int) -> Iterator[int]: + """Read the validated object through an independently owned duplicate.""" + 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_BINARY) + except BaseException: + _winapi.CloseHandle(duplicate) + raise + try: + yield fd + finally: + os.close(fd) diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 3e6769aacc..d8450cdecb 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -10,7 +10,7 @@ import tempfile from typing import TypeVar -from specify_cli._file_handles import windows_handle +from specify_cli._file_handles import windows_handle, windows_read_fd _PathType = TypeVar("_PathType", bound=PurePath) @@ -168,12 +168,12 @@ def read_bytes(self, path: Path | str) -> bytes: with ExitStack() as stack: if os.name == "nt": current = self._canonical_root - stack.enter_context(windows_handle(current, directory=True)) + stack.enter_context(windows_handle(current, directory=True, share_write=False)) for part in parts[:-1]: current /= part - stack.enter_context(windows_handle(current, directory=True)) - stack.enter_context(windows_handle(destination, directory=False)) - fd = os.open(destination, os.O_RDONLY | os.O_BINARY) + stack.enter_context(windows_handle(current, directory=True, share_write=False)) + handle = stack.enter_context(windows_handle(destination, directory=False)) + fd = stack.enter_context(windows_read_fd(handle)) else: if not ( getattr(os, "O_NOFOLLOW", 0) @@ -182,19 +182,23 @@ def read_bytes(self, path: Path | str) -> bytes: and os.open in os.supports_dir_fd ): raise OSError(errno.ENOTSUP, "Safe scoped reads are unsupported on this platform") - flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW + flags = getattr(os, "O_SEARCH", getattr(os, "O_PATH", os.O_RDONLY)) + flags |= os.O_DIRECTORY | os.O_NOFOLLOW flags |= getattr(os, "O_CLOEXEC", 0) parent = os.open(self._canonical_root, flags) - stack.callback(os.close, parent) - for part in parts[:-1]: - parent = os.open(part, flags, dir_fd=parent) - stack.callback(os.close, parent) - fd = os.open( - parts[-1], - os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0), - dir_fd=parent, - ) - stack.callback(os.close, fd) + try: + for part in parts[:-1]: + child = os.open(part, flags, dir_fd=parent) + os.close(parent) + parent = child + fd = os.open( + parts[-1], + os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0), + dir_fd=parent, + ) + stack.callback(os.close, fd) + finally: + os.close(parent) if not stat.S_ISREG(os.fstat(fd).st_mode): raise UnsupportedPathError(f"Expected a regular file: {destination}") with os.fdopen(fd, "rb", closefd=False) as stream: diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py new file mode 100644 index 0000000000..943349ad3f --- /dev/null +++ b/tests/test_file_handles.py @@ -0,0 +1,150 @@ +"""Ownership and native Windows evidence for shared handle primitives.""" + +from contextlib import contextmanager +import os +from pathlib import Path +import struct +import sys +from types import SimpleNamespace + +import pytest + +from specify_cli import _file_handles, file_helper +from specify_cli.file_helper import FileHelper, UnsupportedPathError + + +@pytest.mark.parametrize("failure", [None, "duplicate", "convert", "body"]) +def test_windows_read_fd_duplicates_and_closes_only_its_owned_handle(tmp_path, monkeypatch, failure): + selected = tmp_path / "file" + selected.write_bytes(b"inside") + original = os.open(selected, os.O_RDONLY | getattr(os, "O_BINARY", 0)) + duplicates = [] + closed_handles = [] + + def duplicate(source_process, handle, target_process, access, inherit, options): + assert (source_process, handle, target_process, access, inherit, options) == ( + 7, original, 7, 0, False, 2, + ) + if failure == "duplicate": + raise OSError("injected duplicate failure") + fd = os.dup(handle) + duplicates.append(fd) + return fd + + def convert(handle, flags): + assert handle == duplicates[-1] + assert flags == 0x8000 + if failure == "convert": + raise OSError("injected conversion failure") + return handle + + def close(handle): + closed_handles.append(handle) + os.close(handle) + + monkeypatch.setitem(sys.modules, "_winapi", SimpleNamespace( + GetCurrentProcess=lambda: 7, DuplicateHandle=duplicate, + DUPLICATE_SAME_ACCESS=2, CloseHandle=close, + )) + monkeypatch.setitem(sys.modules, "msvcrt", SimpleNamespace(open_osfhandle=convert)) + monkeypatch.setattr(_file_handles, "os", SimpleNamespace( + O_RDONLY=0, O_BINARY=0x8000, close=os.close, + )) + try: + if failure: + with pytest.raises(OSError, match="injected"): + with _file_handles.windows_read_fd(original): + raise OSError("injected body failure") + else: + with _file_handles.windows_read_fd(original) as fd: + assert os.read(fd, 6) == b"inside" + assert not os.get_inheritable(fd) + assert closed_handles == (duplicates if failure == "convert" else []) + assert os.fstat(original).st_size == 6 + for fd in duplicates: + with pytest.raises(OSError): + os.fstat(fd) + finally: + os.close(original) + + +def _set_windows_symlink_reparse(path: Path, target: Path) -> None: + import ctypes + from ctypes import wintypes + import _winapi + + name = str(target.resolve()) + substitute = "\\??\\UNC\\" + name[2:] if name.startswith("\\\\") else "\\??\\" + name + encoded_substitute = substitute.encode("utf-16-le") + encoded_name = name.encode("utf-16-le") + names = encoded_substitute + b"\0\0" + encoded_name + b"\0\0" + data = struct.pack( + " Date: Mon, 9 Nov 2026 17:22:28 -0600 Subject: [PATCH 03/10] test: pass null handles to Windows fixture API Use integer null arguments required by _winapi.CreateFile on Python 3.11. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_file_handles.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index 943349ad3f..71383ff0fc 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -89,7 +89,7 @@ def _set_windows_symlink_reparse(path: Path, target: Path) -> None: wintypes.LPVOID, wintypes.DWORD, ctypes.POINTER(wintypes.DWORD), wintypes.LPVOID, ] control.restype = wintypes.BOOL - handle = _winapi.CreateFile(str(path), 0x40000000, 0x7, None, 3, 0x02200000, None) + handle = _winapi.CreateFile(str(path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) try: buffer = ctypes.create_string_buffer(data) returned = wintypes.DWORD() @@ -140,7 +140,7 @@ def test_windows_read_directory_pin_prevents_writable_reparse_handle(tmp_path): import _winapi def writable_handle(): - return _winapi.CreateFile(str(tmp_path), 0x40000000, 0x7, None, 3, 0x02200000, None) + return _winapi.CreateFile(str(tmp_path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) _winapi.CloseHandle(writable_handle()) with _file_handles.windows_handle(tmp_path, directory=True, share_write=False): From 16c100d08de55ef93239381ce3c6c04346a66452 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:30:09 -0600 Subject: [PATCH 04/10] fix: retain macOS search access with older Python SDKs Use Darwin's native search-only flag when a Python build omits its binding, with permission regressions covering the missing-constant case. 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 | 8 ++++++-- tests/test_file_helper.py | 9 ++++++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 9f64cc9f8d..73780bb9f1 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -211,8 +211,10 @@ Reads resolve supported contained links under the existing policy, then open the resolved hierarchy without following substituted links. POSIX reads use descriptor-relative directory traversal with `O_DIRECTORY` and `O_NOFOLLOW`. Directory descriptors use `O_SEARCH` or `O_PATH` where available so searchable -directories need not be listable, and each superseded parent is closed as soon -as its child is open. The leaf uses `O_NOFOLLOW` and `O_NONBLOCK`. Windows reads +directories need not be listable, including Darwin's native `O_SEARCH` value +when a Python build uses an older SDK that omits the constant. Each superseded +parent is closed as soon as its child is open. The leaf uses `O_NOFOLLOW` and +`O_NONBLOCK`. Windows reads pin each resolved directory and leaf with non-reparse handles that withhold delete sharing; directory handles also withhold write sharing to prevent in-place reparse changes during traversal. Reads duplicate the validated leaf diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index d8450cdecb..68851576ae 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -7,6 +7,7 @@ import os from pathlib import Path, PurePath, PureWindowsPath import stat +import sys import tempfile from typing import TypeVar @@ -182,8 +183,11 @@ def read_bytes(self, path: Path | str) -> bytes: and os.open in os.supports_dir_fd ): raise OSError(errno.ENOTSUP, "Safe scoped reads are unsupported on this platform") - flags = getattr(os, "O_SEARCH", getattr(os, "O_PATH", os.O_RDONLY)) - flags |= os.O_DIRECTORY | os.O_NOFOLLOW + directory_access = getattr(os, "O_SEARCH", getattr(os, "O_PATH", None)) + if directory_access is None: + # Older SDK builds omit Darwin's O_SEARCH (O_EXEC | O_DIRECTORY). + directory_access = 0x40000000 if sys.platform == "darwin" else os.O_RDONLY + flags = directory_access | os.O_DIRECTORY | os.O_NOFOLLOW flags |= getattr(os, "O_CLOEXEC", 0) parent = os.open(self._canonical_root, flags) try: diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 9c1d281f60..73721f15f0 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -312,9 +312,16 @@ def duplicate(handle): @pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions required") @pytest.mark.parametrize("allow", [False, True]) @pytest.mark.parametrize("restricted", ["root", "parent"]) -def test_read_through_search_only_directories(root, allow, restricted): +@pytest.mark.parametrize("older_darwin_sdk", [ + False, + pytest.param(True, marks=pytest.mark.skipif(sys.platform != "darwin", reason="Darwin SDK regression")), +]) +def test_read_through_search_only_directories(root, monkeypatch, allow, restricted, older_darwin_sdk): if os.geteuid() == 0: pytest.skip("Root bypasses directory permission checks") + if older_darwin_sdk: + for flag in ("O_SEARCH", "O_PATH", "O_EXEC"): + monkeypatch.delattr(os, flag, raising=False) directory = root / "directory" directory.mkdir() selected = directory / "file" From 4b0d4737f8031e3bdca329eed7a909eee6f602a8 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 9 Nov 2026 17:43:25 -0600 Subject: [PATCH 05/10] test: reuse native Windows reparse data in fixtures Copy an OS-generated reparse buffer onto an empty source file and cover the fixture's native calls and cleanup paths. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_file_handles.py | 86 ++++++++++++++++++++++++++++++-------- 1 file changed, 69 insertions(+), 17 deletions(-) diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index 71383ff0fc..848b24dc46 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -3,7 +3,6 @@ from contextlib import contextmanager import os from pathlib import Path -import struct import sys from types import SimpleNamespace @@ -73,15 +72,6 @@ def _set_windows_symlink_reparse(path: Path, target: Path) -> None: from ctypes import wintypes import _winapi - name = str(target.resolve()) - substitute = "\\??\\UNC\\" + name[2:] if name.startswith("\\\\") else "\\??\\" + name - encoded_substitute = substitute.encode("utf-16-le") - encoded_name = name.encode("utf-16-le") - names = encoded_substitute + b"\0\0" + encoded_name + b"\0\0" - data = struct.pack( - " None: wintypes.LPVOID, wintypes.DWORD, ctypes.POINTER(wintypes.DWORD), wintypes.LPVOID, ] control.restype = wintypes.BOOL - handle = _winapi.CreateFile(str(path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) + template = target.with_name("reparse-template") + template.symlink_to(target) try: - buffer = ctypes.create_string_buffer(data) + buffer = ctypes.create_string_buffer(16 * 1024) returned = wintypes.DWORD() - if not control(handle, 0x900A4, buffer, len(data), None, 0, ctypes.byref(returned), None): - raise ctypes.WinError(ctypes.get_last_error()) + source = _winapi.CreateFile(str(template), 0, 0x7, 0, 3, 0x02200000, 0) + try: + if not control(source, 0x900A8, None, 0, buffer, len(buffer), ctypes.byref(returned), None): + raise ctypes.WinError(ctypes.get_last_error()) + finally: + _winapi.CloseHandle(source) + handle = _winapi.CreateFile(str(path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) + try: + if not control(handle, 0x900A4, buffer, returned.value, None, 0, ctypes.byref(returned), None): + raise ctypes.WinError(ctypes.get_last_error()) + finally: + _winapi.CloseHandle(handle) finally: - _winapi.CloseHandle(handle) + template.unlink() + + +@pytest.mark.parametrize("failure", [None, "get", "set"]) +def test_reparse_fixture_copies_native_buffer_and_cleans_up(tmp_path, monkeypatch, failure): + import ctypes + from ctypes import wintypes + + target = tmp_path / "outside" + target.write_bytes(b"outside") + probe = tmp_path / "probe" + try: + probe.symlink_to(target) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"Symlink creation unavailable: {exc}") + probe.unlink() + opened = [] + closed = [] + payload = b"OS-generated reparse buffer" + + def create(path, access, sharing, security, disposition, flags, template): + assert (sharing, security, disposition, flags, template) == (7, 0, 3, 0x02200000, 0) + assert access == (0 if not opened else 0x40000000) + opened.append(len(opened) + 1) + return opened[-1] + + class Control: + def __call__(self, handle, code, source, source_size, destination, size, returned, overlapped): + if code == 0x900A8: + assert handle == 1 and source is None and source_size == 0 + if failure == "get": + return False + ctypes.memmove(destination, payload, len(payload)) + ctypes.cast(returned, ctypes.POINTER(wintypes.DWORD))[0] = len(payload) + else: + assert code == 0x900A4 and handle == 2 + assert ctypes.string_at(source, source_size) == payload + if failure == "set": + return False + return True + + monkeypatch.setitem(sys.modules, "_winapi", SimpleNamespace(CreateFile=create, CloseHandle=closed.append)) + monkeypatch.setattr(ctypes, "WinDLL", lambda *args, **kwargs: SimpleNamespace(DeviceIoControl=Control()), raising=False) + monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) + monkeypatch.setattr(ctypes, "WinError", lambda code: OSError("injected native failure"), raising=False) + if failure: + with pytest.raises(OSError, match="injected native failure"): + _set_windows_symlink_reparse(tmp_path / "file", target) + else: + _set_windows_symlink_reparse(tmp_path / "file", target) + assert closed == opened + assert not (tmp_path / "reparse-template").is_symlink() @pytest.mark.skipif(os.name != "nt", reason="Native Windows reparse changes required") @@ -105,7 +157,7 @@ def test_windows_read_never_reopens_leaf_after_in_place_reparse_change(tmp_path, root = tmp_path / "project" root.mkdir() selected = root / "file" - selected.write_bytes(b"inside") + selected.write_bytes(b"") outside = tmp_path / "outside" outside.write_bytes(b"outside") changed = False @@ -131,7 +183,7 @@ def change_after_validation(path, **kwargs): except UnsupportedPathError: pass else: - assert content == b"inside" + assert content == b"" assert changed From 3bb9c51cad9ea29442032d2adc419e78f259940b Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 09:51:05 -0600 Subject: [PATCH 06/10] fix: open Windows read children relative to validated handles Preserve traversal-only directory access and strengthen permission-revocation, attribute-only junction, and ACL coverage while retaining the merged exclusive-create 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 | 13 +- src/specify_cli/_file_handles.py | 101 ++++++++++++-- src/specify_cli/file_helper.py | 14 +- tests/test_file_handles.py | 232 +++++++++++++++++++++++++++---- tests/test_file_helper.py | 63 +++++++-- 5 files changed, 360 insertions(+), 63 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index e3f700c2aa..0ae2d74e7b 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -230,11 +230,14 @@ Directory descriptors use `O_SEARCH` or `O_PATH` where available so searchable directories need not be listable, including Darwin's native `O_SEARCH` value when a Python build uses an older SDK that omits the constant. Each superseded parent is closed as soon as its child is open. The leaf uses `O_NOFOLLOW` and -`O_NONBLOCK`. Windows reads -pin each resolved directory and leaf with non-reparse handles that withhold -delete sharing; directory handles also withhold write sharing to prevent -in-place reparse changes during traversal. Reads duplicate the validated leaf -handle into an independently owned descriptor, never reopen its pathname. +`O_NONBLOCK`. Windows reads open the canonical root with `FILE_TRAVERSE` and +`FILE_READ_ATTRIBUTES`, then open each resolved child relative to its validated +parent handle with `NtCreateFile`. Directory opens do not request enumeration +access. Relative opens prevent an in-place junction change on an already-open +parent from redirecting the next open; write-sharing restrictions alone cannot +prevent attribute-only reparse updates. Each new handle rejects reparse points, +and handles withhold delete sharing. Reads duplicate the validated leaf handle +into an independently owned descriptor, never reopen its pathname. Both paths validate the opened file with `fstat` before reading its descriptor, and release all handles on success or failure. Platforms without the required safe-open primitives fail explicitly rather than fall back to diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index af3ded2c39..689e7baa36 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -4,11 +4,84 @@ from contextlib import contextmanager import errno import os -from pathlib import Path +from pathlib import Path, PureWindowsPath + + +def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, sharing: int) -> int: + """Open one component relative to an already validated directory handle.""" + import ctypes + from ctypes import wintypes + + 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}") + encoded_length = len(name.encode("utf-16-le", errors="surrogatepass")) + if encoded_length > 65532: + raise OSError(errno.ENAMETOOLONG, "Windows path component is too long", name) + + class UnicodeString(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint16), + ("MaximumLength", ctypes.c_uint16), + ("Buffer", wintypes.LPWSTR), + ] + + class ObjectAttributes(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint32), + ("RootDirectory", wintypes.HANDLE), + ("ObjectName", ctypes.POINTER(UnicodeString)), + ("Attributes", ctypes.c_uint32), + ("SecurityDescriptor", wintypes.LPVOID), + ("SecurityQualityOfService", wintypes.LPVOID), + ] + + class StatusValue(ctypes.Union): + _fields_ = [("Status", ctypes.c_int32), ("Pointer", wintypes.LPVOID)] + + class IoStatusBlock(ctypes.Structure): + _fields_ = [("Value", StatusValue), ("Information", ctypes.c_size_t)] + + native = ctypes.WinDLL("ntdll") + create = native.NtCreateFile + create.argtypes = [ + ctypes.POINTER(wintypes.HANDLE), ctypes.c_uint32, + ctypes.POINTER(ObjectAttributes), ctypes.POINTER(IoStatusBlock), + wintypes.LPVOID, ctypes.c_uint32, ctypes.c_uint32, ctypes.c_uint32, + ctypes.c_uint32, wintypes.LPVOID, ctypes.c_uint32, + ] + create.restype = ctypes.c_int32 + dos_error = native.RtlNtStatusToDosError + dos_error.argtypes = [ctypes.c_int32] + dos_error.restype = ctypes.c_uint32 + buffer = ctypes.create_unicode_buffer(name) + unicode_name = UnicodeString( + encoded_length, encoded_length + 2, ctypes.cast(buffer, wintypes.LPWSTR) + ) + # OBJ_DONT_REPARSE and FILE_OPEN_REPARSE_POINT keep resolution on this handle. + attributes = ObjectAttributes( + ctypes.sizeof(ObjectAttributes), parent, ctypes.pointer(unicode_name), + 0x40 | 0x1000, None, None, + ) + handle = wintypes.HANDLE() + status = IoStatusBlock() + options = 0x00200000 | (0x1 if directory else 0x40 | 0x20) + result = create( + ctypes.byref(handle), access | (0 if directory else 0x100000), + ctypes.byref(attributes), ctypes.byref(status), None, 0, + sharing, 1, options, None, 0, + ) + if result < 0: + raise ctypes.WinError(dos_error(result)) + if not handle.value: + raise OSError(errno.EBADF, "Native child open returned no handle", name) + return handle.value @contextmanager -def _windows_handle(path: Path, *, directory: bool, share_write: bool = True) -> Iterator[int]: +def _windows_handle( + path: Path, *, directory: bool, + parent: int | None = None, traverse: bool = False, +) -> Iterator[int]: """Pin a non-reparse path by withholding Windows delete sharing.""" import ctypes from ctypes import wintypes @@ -35,19 +108,17 @@ class AttributeTagInfo(ctypes.Structure): close.argtypes = [wintypes.HANDLE] close.restype = wintypes.BOOL - # OPEN_REPARSE_POINT checks the entry itself. Directory handles need - # FILE_LIST_DIRECTORY: attribute-only handles do not prevent renaming. - handle = create( - str(path), - (0x80 | 0x1) if directory else 0x80000000, - 0x1 | (0x2 if share_write else 0), - None, - 3, - 0x00200000 | 0x02000000, - None, - ) - if handle == wintypes.HANDLE(-1).value: - raise ctypes.WinError(ctypes.get_last_error()) + access = (0x80 | (0x20 if traverse else 0x1)) if directory else 0x80000000 + sharing = 0x3 + if parent is None: + # FILE_TRAVERSE also participates in sharing checks, unlike attributes alone. + handle = create( + str(path), access, sharing, None, 3, 0x00200000 | 0x02000000, None, + ) + if handle == wintypes.HANDLE(-1).value: + raise ctypes.WinError(ctypes.get_last_error()) + else: + handle = _open_windows_child(parent, str(path), directory=directory, access=access, sharing=sharing) try: attributes = AttributeTagInfo() if not info(handle, 9, ctypes.byref(attributes), ctypes.sizeof(attributes)): diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 82c7f3127e..ee8bc33678 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -169,12 +169,16 @@ def read_bytes(self, path: Path | str) -> bytes: parts = destination.relative_to(self._canonical_root).parts with ExitStack() as stack: if os.name == "nt": - current = self._canonical_root - stack.enter_context(_windows_handle(current, directory=True, share_write=False)) + parent = stack.enter_context(_windows_handle( + self._canonical_root, directory=True, traverse=True, + )) for part in parts[:-1]: - current /= part - stack.enter_context(_windows_handle(current, directory=True, share_write=False)) - handle = stack.enter_context(_windows_handle(destination, directory=False)) + parent = stack.enter_context(_windows_handle( + Path(part), directory=True, parent=parent, traverse=True, + )) + handle = stack.enter_context(_windows_handle( + Path(parts[-1]), directory=False, parent=parent, + )) fd = stack.enter_context(windows_read_fd(handle)) else: if not ( diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index a0da71aa76..e242bf6dee 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -3,9 +3,11 @@ from contextlib import contextmanager import ctypes from ctypes import wintypes +import errno import os from pathlib import Path import sys +import subprocess from types import SimpleNamespace import pytest @@ -70,7 +72,9 @@ def close(handle): os.close(original) -def _set_windows_symlink_reparse(path: Path, target: Path) -> None: +def _set_windows_symlink_reparse( + path: Path, target: Path, *, junction: bool = False, access: int = 0x40000000, +) -> None: import ctypes from ctypes import wintypes import _winapi @@ -83,7 +87,10 @@ def _set_windows_symlink_reparse(path: Path, target: Path) -> None: ] control.restype = wintypes.BOOL template = target.with_name("reparse-template") - template.symlink_to(target) + if junction: + _winapi.CreateJunction(str(target), str(template)) + else: + template.symlink_to(target) try: buffer = ctypes.create_string_buffer(16 * 1024) returned = wintypes.DWORD() @@ -93,36 +100,41 @@ def _set_windows_symlink_reparse(path: Path, target: Path) -> None: raise ctypes.WinError(ctypes.get_last_error()) finally: _winapi.CloseHandle(source) - handle = _winapi.CreateFile(str(path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) + handle = _winapi.CreateFile(str(path), access, 0x7, 0, 3, 0x02200000, 0) try: if not control(handle, 0x900A4, buffer, returned.value, None, 0, ctypes.byref(returned), None): raise ctypes.WinError(ctypes.get_last_error()) finally: _winapi.CloseHandle(handle) finally: - template.unlink() + if junction: + template.rmdir() + else: + template.unlink() @pytest.mark.parametrize("failure", [None, "get", "set"]) -def test_reparse_fixture_copies_native_buffer_and_cleans_up(tmp_path, monkeypatch, failure): +@pytest.mark.parametrize("junction", [False, True]) +def test_reparse_fixture_copies_native_buffer_and_cleans_up(tmp_path, monkeypatch, failure, junction): import ctypes from ctypes import wintypes target = tmp_path / "outside" target.write_bytes(b"outside") - probe = tmp_path / "probe" - try: - probe.symlink_to(target) - except (NotImplementedError, OSError) as exc: - pytest.skip(f"Symlink creation unavailable: {exc}") - probe.unlink() + if not junction: + probe = tmp_path / "probe" + try: + probe.symlink_to(target) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"Symlink creation unavailable: {exc}") + probe.unlink() opened = [] closed = [] payload = b"OS-generated reparse buffer" def create(path, access, sharing, security, disposition, flags, template): assert (sharing, security, disposition, flags, template) == (7, 0, 3, 0x02200000, 0) - assert access == (0 if not opened else 0x40000000) + assert access == (0 if not opened else 0x100 if junction else 0x40000000) opened.append(len(opened) + 1) return opened[-1] @@ -141,17 +153,25 @@ def __call__(self, handle, code, source, source_size, destination, size, returne return False return True - monkeypatch.setitem(sys.modules, "_winapi", SimpleNamespace(CreateFile=create, CloseHandle=closed.append)) + monkeypatch.setitem(sys.modules, "_winapi", SimpleNamespace( + CreateFile=create, CloseHandle=closed.append, + CreateJunction=lambda target, path: Path(path).mkdir(), + )) monkeypatch.setattr(ctypes, "WinDLL", lambda *args, **kwargs: SimpleNamespace(DeviceIoControl=Control()), raising=False) monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) monkeypatch.setattr(ctypes, "WinError", lambda code: OSError("injected native failure"), raising=False) if failure: with pytest.raises(OSError, match="injected native failure"): - _set_windows_symlink_reparse(tmp_path / "file", target) + _set_windows_symlink_reparse( + tmp_path / "file", target, junction=junction, access=0x100 if junction else 0x40000000, + ) else: - _set_windows_symlink_reparse(tmp_path / "file", target) + _set_windows_symlink_reparse( + tmp_path / "file", target, junction=junction, access=0x100 if junction else 0x40000000, + ) assert closed == opened assert not (tmp_path / "reparse-template").is_symlink() + assert not (tmp_path / "reparse-template").exists() @pytest.mark.skipif(os.name != "nt", reason="Native Windows reparse changes required") @@ -171,12 +191,12 @@ def change_after_validation(path, **kwargs): with _file_handles._windows_handle(path, **kwargs) as handle: if not kwargs["directory"]: try: - _set_windows_symlink_reparse(path, outside) + _set_windows_symlink_reparse(selected, outside) except OSError as exc: if exc.winerror in (50, 1314): pytest.skip(f"Symlink reparse updates unavailable: {exc}") raise - assert path.read_bytes() == b"outside" + assert selected.read_bytes() == b"outside" changed = True yield handle @@ -191,20 +211,21 @@ def change_after_validation(path, **kwargs): @pytest.mark.skipif(os.name != "nt", reason="Native Windows sharing semantics required") -def test_windows_read_directory_pin_prevents_writable_reparse_handle(tmp_path): +def test_windows_traversal_pin_allows_writes_but_prevents_rename(tmp_path): import _winapi def writable_handle(): return _winapi.CreateFile(str(tmp_path), 0x40000000, 0x7, 0, 3, 0x02200000, 0) _winapi.CloseHandle(writable_handle()) - with _file_handles._windows_handle(tmp_path, directory=True, share_write=False): + with _file_handles._windows_handle(tmp_path, directory=True, traverse=True): + _winapi.CloseHandle(writable_handle()) with pytest.raises(OSError) as error: - writable_handle() + tmp_path.rename(tmp_path.with_name(tmp_path.name + "-renamed")) assert error.value.winerror == 32 _winapi.CloseHandle(writable_handle()) -@pytest.mark.parametrize("directory", [False, True]) -def test_windows_handle_declares_complete_createfile_prototype(tmp_path, monkeypatch, directory): +@pytest.mark.parametrize("directory,traverse", [(False, False), (True, False), (True, True)]) +def test_windows_handle_declares_complete_createfile_prototype(tmp_path, monkeypatch, directory, traverse): closed = [] def create(*args): @@ -220,7 +241,7 @@ def create(*args): assert create.restype is wintypes.HANDLE assert args == ( str(tmp_path), - (0x80 | 0x1) if directory else 0x80000000, + (0x80 | (0x20 if traverse else 0x1)) if directory else 0x80000000, 0x3, None, 3, @@ -247,7 +268,170 @@ def load_kernel(name, *, use_last_error): ) monkeypatch.setattr(ctypes, "WinDLL", load_kernel, raising=False) - with _windows_handle(tmp_path, directory=directory) as handle: + with _windows_handle(tmp_path, directory=directory, traverse=traverse) as handle: assert handle == 123 assert closed == [] assert closed == [123] + + +@pytest.mark.parametrize("directory", [False, True]) +@pytest.mark.parametrize("failure", [None, "status", "reparse", "type", "info", "empty"]) +@pytest.mark.parametrize("child_name", ["caf\u00e9", "face \U0001f600", "surrogate\ud800"]) +def test_windows_child_open_is_relative_and_closes_handles(monkeypatch, directory, failure, child_name): + closed = [] + + def create_path(*args): + pytest.fail("A child must not be opened by absolute pathname") + + def create_child(output, access, attributes, status, allocation, file_attributes, + sharing, disposition, options, ea, ea_length): + assert create_child.restype is ctypes.c_int32 + attributes = attributes._obj + assert attributes.Length == ctypes.sizeof(type(attributes)) + assert attributes.RootDirectory == 51 + assert attributes.Attributes == 0x1040 + assert attributes.SecurityDescriptor is None + assert attributes.SecurityQualityOfService is None + 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 sharing == 3 and disposition == 1 + assert options == (0x200001 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": + return -1073741790 + if failure != "empty": + output._obj.value = 77 + return 0 + + def info(handle, info_class, buffer, size): + assert handle == 77 and info_class == 9 + attributes = ctypes.cast(buffer, ctypes.POINTER(wintypes.DWORD)) + attributes[0] = 0x10 if directory else 0 + if failure == "reparse": + attributes[0] |= 0x400 + if failure == "type": + attributes[0] ^= 0x10 + return failure != "info" + + def close(handle): + closed.append(handle) + return True + + def error(status): + assert status == -1073741790 + return 5 + + def load(name, **kwargs): + if name == "ntdll": + return SimpleNamespace(NtCreateFile=create_child, RtlNtStatusToDosError=error) + assert name == "kernel32" and kwargs == {"use_last_error": True} + return SimpleNamespace( + CreateFileW=create_path, GetFileInformationByHandleEx=info, CloseHandle=close, + ) + + monkeypatch.setattr(ctypes, "WinDLL", load, raising=False) + monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) + monkeypatch.setattr(ctypes, "WinError", lambda code: PermissionError(errno.EACCES, "native denied"), raising=False) + if failure: + with pytest.raises(OSError): + with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51): + pytest.fail("A failing native open must not yield a handle") + else: + with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51) as handle: + assert handle == 77 and closed == [] + assert closed == ([] if failure in ("status", "empty") else [77]) + + +@pytest.mark.parametrize("name", ["", ".", "..", "../file", "dir/file", "dir\\file", "C:outside", "C:\\outside", "\0"]) +def test_windows_child_open_rejects_non_component_names(name): + with pytest.raises(ValueError, match="single relative"): + _file_handles._open_windows_child(51, name, directory=False, access=0, sharing=3) + + +def test_windows_child_open_rejects_oversized_unicode_name(): + with pytest.raises(OSError) as error: + _file_handles._open_windows_child(51, "a" * 32767, directory=False, access=0, sharing=3) + assert error.value.errno == errno.ENAMETOOLONG + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows attribute-only junction changes required") +@pytest.mark.parametrize("allow", [False, True]) +@pytest.mark.parametrize("level", ["root", "parent"]) +def test_windows_read_anchors_children_after_attribute_only_junction_change(tmp_path, monkeypatch, allow, level): + root = tmp_path / "project" + root.mkdir() + root = root.resolve() + directory = root / "directory" + directory.mkdir() + selected = directory / "file" + selected.write_bytes(b"inside") + outside = tmp_path / "outside" + outside_directory = outside / "directory" if level == "root" else outside + outside_directory.mkdir(parents=True) + (outside_directory / "file").write_bytes(b"outside") + locations = {} + changed = False + + @contextmanager + def exchange(path, **kwargs): + nonlocal changed + parent = kwargs.get("parent") + actual = path if parent is None else locations[parent] / path + with _file_handles._windows_handle(path, **kwargs) as handle: + locations[handle] = actual + try: + if kwargs["directory"] and actual == (root if level == "root" else directory): + displaced = directory if level == "root" else selected + displaced.rename(tmp_path / "retained") + _set_windows_symlink_reparse(actual, outside, junction=True, access=0x100) + changed = True + # The former full-path open follows this same converted parent. + with _windows_handle(selected, directory=False) as old_handle: + with _file_handles.windows_read_fd(old_handle) as fd: + assert os.read(fd, 7) == b"outside" + yield handle + finally: + locations.pop(handle) + + monkeypatch.setattr(file_helper, "_windows_handle", exchange) + try: + content = FileHelper(root, allow_symlinks=allow).read_bytes(selected) + except (OSError, ValueError): + pass + else: + assert content == b"inside" + assert changed + + +@pytest.mark.skipif(os.name != "nt", reason="Native Windows ACL semantics required") +@pytest.mark.parametrize("restricted", ["root", "parent"]) +@pytest.mark.parametrize("allow", [False, True]) +def test_windows_reads_do_not_require_folder_listing(tmp_path, restricted, allow): + from specify_cli._file_policy import read_project_options + + root = tmp_path / "project" + directory = root / ".specify" + directory.mkdir(parents=True) + selected = directory / "init-options.json" + payload = b'{"allow_symlinks": false}' + selected.write_bytes(payload) + limited = root if restricted == "root" else directory + subprocess.run( + ["icacls", str(limited), "/deny", "*S-1-1-0:(RD)"], check=True, capture_output=True, + ) + try: + assert selected.read_bytes() == payload + with pytest.raises(PermissionError): + list(limited.iterdir()) + with pytest.raises(PermissionError): + with _windows_handle(limited, directory=True): + pytest.fail("The former listing-mode pin must be denied by this ACL") + assert FileHelper(root, allow_symlinks=allow).read_bytes(selected) == payload + assert read_project_options(root)["allow_symlinks"] is False + finally: + subprocess.run( + ["icacls", str(limited), "/remove:d", "*S-1-1-0"], check=True, capture_output=True, + ) diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index c3166460dd..da4a765487 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -205,21 +205,28 @@ def test_windows_read_pins_entire_resolved_hierarchy(root, monkeypatch, failure) seen = [] @contextmanager - def pin(path, *, directory, share_write=True): - if path.name == {"parent": "directory", "leaf": "file"}.get(failure): + def pin(path, *, directory, parent=None, traverse=False): + if parent is None: + assert directory and path == root.resolve() + actual = path + else: + assert parent == len(pinned) + 10 + assert path.parts == (path.name,) + actual = pinned[-1] / path + if actual.name == {"parent": "directory", "leaf": "file"}.get(failure): raise OSError("injected handle failure") if directory: - assert not share_write - pinned.append(path) - seen.append((path, directory)) + assert traverse + pinned.append(actual) + seen.append((actual, directory)) try: - yield 1 + yield len(pinned) + 10 finally: - assert pinned.pop() == path + assert pinned.pop() == actual @contextmanager def duplicate(handle): - assert handle == 1 + assert handle == 13 assert pinned == [root.resolve(), directory.resolve(), selected.resolve()] if failure == "duplicate": raise OSError("injected duplicate failure") @@ -275,16 +282,18 @@ def test_windows_read_keeps_validated_object_when_path_changes(root, link, monke changed = False @contextmanager - def pin(path, *, directory, share_write=True): + def pin(path, *, directory, parent=None, traverse=False): nonlocal changed if directory: + assert parent is None and traverse yield 1 else: - fd = os.open(path, os.O_RDONLY | getattr(os, "O_BINARY", 0)) + assert parent == 1 and path == Path("file") + fd = os.open(selected, os.O_RDONLY | getattr(os, "O_BINARY", 0)) try: # Model reparse changes after validation without changing the opened object. - path.unlink() - link(path, outside) + selected.unlink() + link(selected, outside) changed = True yield fd finally: @@ -340,20 +349,46 @@ def test_read_through_search_only_directories(root, monkeypatch, allow, restrict @pytest.mark.skipif(os.name == "nt", reason="POSIX directory permissions required") @pytest.mark.parametrize("restricted", ["root", "parent"]) -def test_read_rejects_non_searchable_directories(root, restricted): +@pytest.mark.parametrize("after_validation", [False, True]) +def test_read_rejects_non_searchable_directories(root, monkeypatch, restricted, after_validation): if os.geteuid() == 0: pytest.skip("Root bypasses directory permission checks") directory = root / "directory" directory.mkdir() selected = directory / "file" selected.write_bytes(b"inside") + expected = selected.resolve() files = FileHelper(root) limited = root if restricted == "root" else directory previous_mode = limited.stat().st_mode & 0o777 - limited.chmod(0) + checked = [] + opened = [] + original_check = FileHelper._require_file + original_open = os.open + + def revoke(path): + original_check(path) + checked.append(path) + limited.chmod(0) + + def recording_open(*args, **kwargs): + opened.append((args, kwargs)) + return original_open(*args, **kwargs) + + if after_validation: + monkeypatch.setattr(FileHelper, "_require_file", staticmethod(revoke)) + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd | {recording_open}) + monkeypatch.setattr(os, "open", recording_open) + else: + limited.chmod(0) try: with pytest.raises(PermissionError): files.read_bytes(selected) + if after_validation: + assert checked == [expected] + assert opened + if restricted == "parent": + assert any(kwargs.get("dir_fd") is not None for args, kwargs in opened) finally: limited.chmod(previous_mode) From 5cd81e677cb7ee9dd68a498eaac1ebab9309e559 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 10:26:58 -0600 Subject: [PATCH 07/10] test: cover safe-read capabilities and clarify creation limits Check each required POSIX read capability independently and document the Windows exclusive-create pathname limitation. 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 | 18 +++++++++++------- tests/test_file_helper.py | 5 +++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 0ae2d74e7b..ce42a04360 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -248,18 +248,22 @@ substituted symlink. This does not freeze file contents against in-place edits. 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. For all operations that still use -validated paths (directory creation, atomic writes, non-atomic -overwrites, symlink creation, and deletion), 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. +validated paths (directory creation, atomic writes, non-atomic overwrites, +symlink creation, deletion, and Windows exclusive file creation), 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. Exclusive file creation anchors its resolved parent hierarchy as described above. On POSIX, renaming an opened directory does not redirect creation to its replacement: creation stays with the opened directory, even if it is renamed out of the root. This is not a guarantee of stable namespace membership under concurrent renames, nor does parent anchoring make every leaf preflight -atomic with the subsequent open. The helper does not constrain subprocesses -or third-party code. +atomic with the subsequent open. +On Windows, exclusive creation still opens its leaf by pathname after pinning +the parent hierarchy. Attribute-only reparse updates can redirect that open; +it does not have the read path's parent-relative handle protection. +The helper does not constrain subprocesses or third-party code. Existing safe-write mechanisms informed this helper, but existing shared infrastructure warning/skip behavior and development-mode link creation are diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index da4a765487..c64d38124d 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -482,9 +482,10 @@ def exchange(path): @pytest.mark.skipif(os.name == "nt", reason="POSIX capability refusal regression") -def test_read_refuses_unsafe_platform_fallback(root, monkeypatch): +@pytest.mark.parametrize("capability", ["O_NOFOLLOW", "O_DIRECTORY", "O_NONBLOCK", "supports_dir_fd"]) +def test_read_refuses_unsafe_platform_fallback(root, monkeypatch, capability): (root / "file").write_bytes(b"inside") - monkeypatch.setattr(os, "supports_dir_fd", set()) + monkeypatch.setattr(os, capability, set() if capability == "supports_dir_fd" else 0) with pytest.raises(OSError) as error: FileHelper(root).read_bytes("file") assert error.value.errno == errno.ENOTSUP From 1607f3cedda597842dff0d484e23b0e576bd3136 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:00:17 -0600 Subject: [PATCH 08/10] fix: reuse immutable Windows ABI types Keep structure and pointer declarations at module scope so repeated child opens reuse type identities. Cover successful and failed opens plus native repeated-read lifetime checks. 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 | 78 ++++++++++++++++++-------------- tests/test_file_handles.py | 57 +++++++++++++++++++---- 3 files changed, 97 insertions(+), 43 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index ce42a04360..bfb499def2 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -239,7 +239,10 @@ prevent attribute-only reparse updates. Each new handle rejects reparse points, and handles withhold delete sharing. Reads duplicate the validated leaf handle into an independently owned descriptor, never reopen its pathname. Both paths validate the opened file with `fstat` before reading its descriptor, -and release all handles on success or failure. Platforms without +and release all handles on success or failure. Windows child-open ABI +structures and pointer types are defined once and reused, avoiding type retention +in older supported Python versions. Handle values and path buffers remain +operation-local. Platforms without the required safe-open primitives fail explicitly rather than fall back to unchecked pathname reads. A concurrent replacement can fail the read or leave it reading the already-open original file, but cannot redirect it through a diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index 689e7baa36..a90f4dcde8 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -2,50 +2,63 @@ from collections.abc import Iterator from contextlib import contextmanager +import ctypes +from ctypes import wintypes import errno import os from pathlib import Path, PureWindowsPath +# Older Python versions retain POINTER types globally; reuse these ABI classes. +class _UnicodeString(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint16), + ("MaximumLength", ctypes.c_uint16), + ("Buffer", wintypes.LPWSTR), + ] + + +_UnicodeStringPointer = ctypes.POINTER(_UnicodeString) + + +class _ObjectAttributes(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint32), + ("RootDirectory", wintypes.HANDLE), + ("ObjectName", _UnicodeStringPointer), + ("Attributes", ctypes.c_uint32), + ("SecurityDescriptor", wintypes.LPVOID), + ("SecurityQualityOfService", wintypes.LPVOID), + ] + + +_ObjectAttributesPointer = ctypes.POINTER(_ObjectAttributes) + + +class _StatusValue(ctypes.Union): + _fields_ = [("Status", ctypes.c_int32), ("Pointer", wintypes.LPVOID)] + + +class _IoStatusBlock(ctypes.Structure): + _fields_ = [("Value", _StatusValue), ("Information", ctypes.c_size_t)] + + +_IoStatusBlockPointer = ctypes.POINTER(_IoStatusBlock) + + def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, sharing: int) -> int: """Open one component relative to an already validated directory handle.""" - import ctypes - from ctypes import wintypes - 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}") encoded_length = len(name.encode("utf-16-le", errors="surrogatepass")) if encoded_length > 65532: raise OSError(errno.ENAMETOOLONG, "Windows path component is too long", name) - class UnicodeString(ctypes.Structure): - _fields_ = [ - ("Length", ctypes.c_uint16), - ("MaximumLength", ctypes.c_uint16), - ("Buffer", wintypes.LPWSTR), - ] - - class ObjectAttributes(ctypes.Structure): - _fields_ = [ - ("Length", ctypes.c_uint32), - ("RootDirectory", wintypes.HANDLE), - ("ObjectName", ctypes.POINTER(UnicodeString)), - ("Attributes", ctypes.c_uint32), - ("SecurityDescriptor", wintypes.LPVOID), - ("SecurityQualityOfService", wintypes.LPVOID), - ] - - class StatusValue(ctypes.Union): - _fields_ = [("Status", ctypes.c_int32), ("Pointer", wintypes.LPVOID)] - - class IoStatusBlock(ctypes.Structure): - _fields_ = [("Value", StatusValue), ("Information", ctypes.c_size_t)] - native = ctypes.WinDLL("ntdll") create = native.NtCreateFile create.argtypes = [ ctypes.POINTER(wintypes.HANDLE), ctypes.c_uint32, - ctypes.POINTER(ObjectAttributes), ctypes.POINTER(IoStatusBlock), + _ObjectAttributesPointer, _IoStatusBlockPointer, wintypes.LPVOID, ctypes.c_uint32, ctypes.c_uint32, ctypes.c_uint32, ctypes.c_uint32, wintypes.LPVOID, ctypes.c_uint32, ] @@ -54,16 +67,16 @@ class IoStatusBlock(ctypes.Structure): dos_error.argtypes = [ctypes.c_int32] dos_error.restype = ctypes.c_uint32 buffer = ctypes.create_unicode_buffer(name) - unicode_name = UnicodeString( + unicode_name = _UnicodeString( encoded_length, encoded_length + 2, ctypes.cast(buffer, wintypes.LPWSTR) ) # OBJ_DONT_REPARSE and FILE_OPEN_REPARSE_POINT keep resolution on this handle. - attributes = ObjectAttributes( - ctypes.sizeof(ObjectAttributes), parent, ctypes.pointer(unicode_name), + attributes = _ObjectAttributes( + ctypes.sizeof(_ObjectAttributes), parent, ctypes.pointer(unicode_name), 0x40 | 0x1000, None, None, ) handle = wintypes.HANDLE() - status = IoStatusBlock() + status = _IoStatusBlock() options = 0x00200000 | (0x1 if directory else 0x40 | 0x20) result = create( ctypes.byref(handle), access | (0 if directory else 0x100000), @@ -83,9 +96,6 @@ def _windows_handle( parent: int | None = None, traverse: bool = False, ) -> 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)] diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index e242bf6dee..842d5b09e1 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -4,11 +4,13 @@ import ctypes from ctypes import wintypes import errno +import gc import os from pathlib import Path import sys import subprocess from types import SimpleNamespace +import weakref import pytest @@ -279,6 +281,10 @@ def load_kernel(name, *, use_last_error): @pytest.mark.parametrize("child_name", ["caf\u00e9", "face \U0001f600", "surrogate\ud800"]) def test_windows_child_open_is_relative_and_closes_handles(monkeypatch, directory, failure, child_name): closed = [] + abi_types = {role: set() for role in ( + "name", "attributes", "status_value", "status", + "name_pointer", "attributes_pointer", "status_pointer", + )} def create_path(*args): pytest.fail("A child must not be opened by absolute pathname") @@ -287,6 +293,13 @@ def create_child(output, access, attributes, status, allocation, file_attributes sharing, disposition, options, ea, ea_length): assert create_child.restype is ctypes.c_int32 attributes = attributes._obj + declarations = ( + type(attributes.ObjectName.contents), type(attributes), type(status._obj.Value), + type(status._obj), type(attributes.ObjectName), + create_child.argtypes[2], create_child.argtypes[3], + ) + for role, declaration in zip(abi_types, declarations): + abi_types[role].add(weakref.ref(declaration)) assert attributes.Length == ctypes.sizeof(type(attributes)) assert attributes.RootDirectory == 51 assert attributes.Attributes == 0x1040 @@ -335,14 +348,42 @@ def load(name, **kwargs): monkeypatch.setattr(ctypes, "WinDLL", load, raising=False) monkeypatch.setattr(ctypes, "get_last_error", lambda: 5, raising=False) monkeypatch.setattr(ctypes, "WinError", lambda code: PermissionError(errno.EACCES, "native denied"), raising=False) - if failure: - with pytest.raises(OSError): - with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51): - pytest.fail("A failing native open must not yield a handle") - else: - with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51) as handle: - assert handle == 77 and closed == [] - assert closed == ([] if failure in ("status", "empty") else [77]) + for iteration in range(32): + if failure: + with pytest.raises(OSError): + with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51): + pytest.fail("A failing native open must not yield a handle") + else: + with _windows_handle(Path(child_name), directory=directory, traverse=directory, parent=51) as handle: + assert handle == 77 and len(closed) == iteration + assert closed == ([] if failure in ("status", "empty") else [77] * 32) + gc.collect() + 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 repeated-read lifetime regression") +@pytest.mark.parametrize("allow", [False, True]) +def test_repeated_windows_reads_do_not_accumulate_abi_types(tmp_path, allow): + root = tmp_path / "project" + selected = root / "nested/file" + selected.parent.mkdir(parents=True) + selected.write_bytes(b"inside") + files = FileHelper(root, allow_symlinks=allow) + + def live_abi_types(): + return { + weakref.ref(obj) for obj in gc.get_objects() + if isinstance(obj, type) and getattr(obj, "__module__", None) == _file_handles.__name__ + } + + for _ in range(32): + assert files.read_bytes("nested/file") == b"inside" + gc.collect() + baseline = live_abi_types() + for _ in range(256): + assert files.read_bytes("nested/file") == b"inside" + gc.collect() + assert live_abi_types() == baseline @pytest.mark.parametrize("name", ["", ".", "..", "../file", "dir/file", "dir\\file", "C:outside", "C:\\outside", "\0"]) From 8a7b619ee5995d6540c1384868d472d670c5ce2d Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 12:50:17 -0600 Subject: [PATCH 09/10] fix: initialize Windows ABI metadata lazily Keep POSIX startup and filesystem operations independent of optional _ctypes while retaining one set of Windows ABI type definitions through the import cache. 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 | 9 ++--- src/specify_cli/_file_handles.py | 56 +++++++------------------------- src/specify_cli/_windows_abi.py | 41 +++++++++++++++++++++++ tests/test_file_handles.py | 50 +++++++++++++++++++++++++++- 4 files changed, 107 insertions(+), 49 deletions(-) create mode 100644 src/specify_cli/_windows_abi.py diff --git a/design/file-helper.md b/design/file-helper.md index bfb499def2..5899e28603 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -240,10 +240,11 @@ and handles withhold delete sharing. Reads duplicate the validated leaf handle into an independently owned descriptor, never reopen its pathname. Both paths validate the opened file with `fstat` before reading its descriptor, and release all handles on success or failure. Windows child-open ABI -structures and pointer types are defined once and reused, avoiding type retention -in older supported Python versions. Handle values and path buffers remain -operation-local. Platforms without -the required safe-open primitives fail explicitly rather than fall back to +structures and pointer types are initialized lazily in a Windows-only module +and reused through Python's import cache. This avoids type retention in older +supported Python versions without requiring `ctypes` for POSIX startup. +Handle values and path buffers remain operation-local. Platforms without the +required safe-open primitives fail explicitly rather than fall back to unchecked pathname reads. A concurrent replacement can fail the read or leave it reading the already-open original file, but cannot redirect it through a substituted symlink. This does not freeze file contents against in-place edits. diff --git a/src/specify_cli/_file_handles.py b/src/specify_cli/_file_handles.py index a90f4dcde8..5a9f81a3d5 100644 --- a/src/specify_cli/_file_handles.py +++ b/src/specify_cli/_file_handles.py @@ -2,50 +2,11 @@ from collections.abc import Iterator from contextlib import contextmanager -import ctypes -from ctypes import wintypes import errno import os from pathlib import Path, PureWindowsPath -# Older Python versions retain POINTER types globally; reuse these ABI classes. -class _UnicodeString(ctypes.Structure): - _fields_ = [ - ("Length", ctypes.c_uint16), - ("MaximumLength", ctypes.c_uint16), - ("Buffer", wintypes.LPWSTR), - ] - - -_UnicodeStringPointer = ctypes.POINTER(_UnicodeString) - - -class _ObjectAttributes(ctypes.Structure): - _fields_ = [ - ("Length", ctypes.c_uint32), - ("RootDirectory", wintypes.HANDLE), - ("ObjectName", _UnicodeStringPointer), - ("Attributes", ctypes.c_uint32), - ("SecurityDescriptor", wintypes.LPVOID), - ("SecurityQualityOfService", wintypes.LPVOID), - ] - - -_ObjectAttributesPointer = ctypes.POINTER(_ObjectAttributes) - - -class _StatusValue(ctypes.Union): - _fields_ = [("Status", ctypes.c_int32), ("Pointer", wintypes.LPVOID)] - - -class _IoStatusBlock(ctypes.Structure): - _fields_ = [("Value", _StatusValue), ("Information", ctypes.c_size_t)] - - -_IoStatusBlockPointer = ctypes.POINTER(_IoStatusBlock) - - def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, sharing: int) -> 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: @@ -54,11 +15,15 @@ def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, if encoded_length > 65532: raise OSError(errno.ENAMETOOLONG, "Windows path component is too long", name) + import ctypes + from ctypes import wintypes + from . import _windows_abi as abi + native = ctypes.WinDLL("ntdll") create = native.NtCreateFile create.argtypes = [ ctypes.POINTER(wintypes.HANDLE), ctypes.c_uint32, - _ObjectAttributesPointer, _IoStatusBlockPointer, + abi._ObjectAttributesPointer, abi._IoStatusBlockPointer, wintypes.LPVOID, ctypes.c_uint32, ctypes.c_uint32, ctypes.c_uint32, ctypes.c_uint32, wintypes.LPVOID, ctypes.c_uint32, ] @@ -67,16 +32,16 @@ def _open_windows_child(parent: int, name: str, *, directory: bool, access: int, dos_error.argtypes = [ctypes.c_int32] dos_error.restype = ctypes.c_uint32 buffer = ctypes.create_unicode_buffer(name) - unicode_name = _UnicodeString( + unicode_name = abi._UnicodeString( encoded_length, encoded_length + 2, ctypes.cast(buffer, wintypes.LPWSTR) ) # OBJ_DONT_REPARSE and FILE_OPEN_REPARSE_POINT keep resolution on this handle. - attributes = _ObjectAttributes( - ctypes.sizeof(_ObjectAttributes), parent, ctypes.pointer(unicode_name), + attributes = abi._ObjectAttributes( + ctypes.sizeof(abi._ObjectAttributes), parent, ctypes.pointer(unicode_name), 0x40 | 0x1000, None, None, ) handle = wintypes.HANDLE() - status = _IoStatusBlock() + status = abi._IoStatusBlock() options = 0x00200000 | (0x1 if directory else 0x40 | 0x20) result = create( ctypes.byref(handle), access | (0 if directory else 0x100000), @@ -96,6 +61,9 @@ def _windows_handle( parent: int | None = None, traverse: bool = False, ) -> 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)] diff --git a/src/specify_cli/_windows_abi.py b/src/specify_cli/_windows_abi.py new file mode 100644 index 0000000000..36fd74af65 --- /dev/null +++ b/src/specify_cli/_windows_abi.py @@ -0,0 +1,41 @@ +"""Windows ABI metadata, initialized once through lazy module imports.""" + +import ctypes +from ctypes import wintypes + + +# Older Python versions retain POINTER types globally; reuse these ABI classes. +class _UnicodeString(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint16), + ("MaximumLength", ctypes.c_uint16), + ("Buffer", wintypes.LPWSTR), + ] + + +_UnicodeStringPointer = ctypes.POINTER(_UnicodeString) + + +class _ObjectAttributes(ctypes.Structure): + _fields_ = [ + ("Length", ctypes.c_uint32), + ("RootDirectory", wintypes.HANDLE), + ("ObjectName", _UnicodeStringPointer), + ("Attributes", ctypes.c_uint32), + ("SecurityDescriptor", wintypes.LPVOID), + ("SecurityQualityOfService", wintypes.LPVOID), + ] + + +_ObjectAttributesPointer = ctypes.POINTER(_ObjectAttributes) + + +class _StatusValue(ctypes.Union): + _fields_ = [("Status", ctypes.c_int32), ("Pointer", wintypes.LPVOID)] + + +class _IoStatusBlock(ctypes.Structure): + _fields_ = [("Value", _StatusValue), ("Information", ctypes.c_size_t)] + + +_IoStatusBlockPointer = ctypes.POINTER(_IoStatusBlock) diff --git a/tests/test_file_handles.py b/tests/test_file_handles.py index 842d5b09e1..7ac4f31f42 100644 --- a/tests/test_file_handles.py +++ b/tests/test_file_handles.py @@ -19,6 +19,52 @@ from specify_cli.file_helper import FileHelper, UnsupportedPathError +@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): + result = subprocess.run( + [sys.executable, "-c", """ +import sys + +class NoCtypes: + def find_spec(self, fullname, path=None, target=None): + if fullname == "_ctypes": + raise ModuleNotFoundError("optional _ctypes unavailable", name="_ctypes") + +for name in tuple(sys.modules): + if name == "_ctypes" or name == "ctypes" or name.startswith("ctypes."): + sys.modules.pop(name) +sys.meta_path.insert(0, NoCtypes()) + +if sys.argv[1] == "help": + from specify_cli import main + sys.argv = ["specify", "--help"] + main() +else: + from pathlib import Path + from specify_cli.file_helper import FileHelper + + root = Path(sys.argv[2]) + for allow in (False, True): + files = FileHelper(root, allow_symlinks=allow) + files.mkdir("nested") + files.create_text("nested/file", "inside") + assert files.read_text("nested/file") == "inside" + files.write_text("nested/file", "updated") + if allow: + files.symlink("file", "nested/alias") + assert files.read_text("nested/alias") == "updated" + files.delete("nested", recursive=True) + assert "specify_cli._windows_abi" not in sys.modules + assert "_ctypes" not in sys.modules +""", operation, str(tmp_path)], + capture_output=True, text=True, check=False, + ) + assert result.returncode == 0, result.stderr + if operation == "help": + assert "Usage" in result.stdout + + @pytest.mark.parametrize("failure", [None, "duplicate", "convert", "body"]) def test_windows_read_fd_duplicates_and_closes_only_its_owned_handle(tmp_path, monkeypatch, failure): selected = tmp_path / "file" @@ -369,17 +415,19 @@ def test_repeated_windows_reads_do_not_accumulate_abi_types(tmp_path, allow): selected.parent.mkdir(parents=True) selected.write_bytes(b"inside") files = FileHelper(root, allow_symlinks=allow) + abi_modules = {_file_handles.__name__, f"{_file_handles.__package__}._windows_abi"} def live_abi_types(): return { weakref.ref(obj) for obj in gc.get_objects() - if isinstance(obj, type) and getattr(obj, "__module__", None) == _file_handles.__name__ + if isinstance(obj, type) and getattr(obj, "__module__", None) in abi_modules } for _ in range(32): assert files.read_bytes("nested/file") == b"inside" gc.collect() baseline = live_abi_types() + assert baseline for _ in range(256): assert files.read_bytes("nested/file") == b"inside" gc.collect() From a948f79069453af81847e77cc1a88ec3ffea36db Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Tue, 10 Nov 2026 13:51:09 -0600 Subject: [PATCH 10/10] refactor: share POSIX parent-directory traversal Centralize directory capabilities, search flags, and descriptor ownership for reads, exclusive creation, and text overwrites. Preserve operation-specific leaf semantics and Windows paths. 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_helper.py | 105 +++++++++++----------------- tests/test_file_helper.py | 100 ++++++++++++++++++++++++++ tests/test_file_helper_overwrite.py | 2 +- 4 files changed, 146 insertions(+), 66 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 6949a09831..77ee80fc4a 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -250,6 +250,11 @@ entries, not reject a whole project for excluded/unrelated links. ## Safety limits and migration +POSIX reads, exclusive file creation, and text overwrites share one parent +directory walk for capability checks, search flags, no-follow traversal, and +descriptor cleanup. Leaf open flags, file-type validation, and truncation +remain operation-specific. + Reads resolve supported contained links under the existing policy, then open the resolved hierarchy without following substituted links. POSIX reads use descriptor-relative directory traversal with `O_DIRECTORY` and `O_NOFOLLOW`. diff --git a/src/specify_cli/file_helper.py b/src/specify_cli/file_helper.py index 08ebff67dd..f293d53b2e 100644 --- a/src/specify_cli/file_helper.py +++ b/src/specify_cli/file_helper.py @@ -15,7 +15,8 @@ from specify_cli._file_handles import _windows_handle, _windows_overwrite_fd, windows_read_fd _PathType = TypeVar("_PathType", bound=PurePath) -_OVERWRITE_DIR_FD_SUPPORTED = os.open in os.supports_dir_fd +# Wrapping os.open must not change the underlying platform capability. +_POSIX_OPEN = os.open def _strip_extended_length_prefix(path: _PathType) -> _PathType: @@ -167,9 +168,9 @@ def mkdir( def read_bytes(self, path: Path | str) -> bytes: destination = self._path(path) self._require_file(destination) - parts = destination.relative_to(self._canonical_root).parts with ExitStack() as stack: if os.name == "nt": + parts = destination.relative_to(self._canonical_root).parts parent = stack.enter_context(_windows_handle( self._canonical_root, directory=True, traverse=True, )) @@ -182,33 +183,16 @@ def read_bytes(self, path: Path | str) -> bytes: )) fd = stack.enter_context(windows_read_fd(handle)) else: - if not ( - getattr(os, "O_NOFOLLOW", 0) - and getattr(os, "O_DIRECTORY", 0) - and getattr(os, "O_NONBLOCK", 0) - and os.open in os.supports_dir_fd - ): - raise OSError(errno.ENOTSUP, "Safe scoped reads are unsupported on this platform") - directory_access = getattr(os, "O_SEARCH", getattr(os, "O_PATH", None)) - if directory_access is None: - # Older SDK builds omit Darwin's O_SEARCH (O_EXEC | O_DIRECTORY). - directory_access = 0x40000000 if sys.platform == "darwin" else os.O_RDONLY - flags = directory_access | os.O_DIRECTORY | os.O_NOFOLLOW - flags |= getattr(os, "O_CLOEXEC", 0) - parent = os.open(self._canonical_root, flags) - try: - for part in parts[:-1]: - child = os.open(part, flags, dir_fd=parent) - os.close(parent) - parent = child + unsupported = "Safe scoped reads are unsupported on this platform" + if not getattr(os, "O_NONBLOCK", 0): + raise OSError(errno.ENOTSUP, unsupported) + with self._posix_directory(destination.parent, unsupported_message=unsupported) as parent: fd = os.open( - parts[-1], + destination.name, os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0), dir_fd=parent, ) stack.callback(os.close, fd) - finally: - os.close(parent) if not stat.S_ISREG(os.fstat(fd).st_mode): raise UnsupportedPathError(f"Expected a regular file: {destination}") with os.fdopen(fd, "rb", closefd=False) as stream: @@ -218,29 +202,19 @@ def read_text(self, path: Path | str, *, encoding: str = "utf-8") -> str: return self.read_bytes(path).decode(encoding) @contextmanager - def _creation_directory(self, directory: Path) -> Iterator[int | Path]: - """Anchor an already policy-resolved parent without following new links.""" + def _posix_directory(self, directory: Path, *, unsupported_message: str) -> Iterator[int]: + """Walk an already policy-resolved parent with bounded descriptor ownership.""" parts = directory.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 ( getattr(os, "O_NOFOLLOW", 0) and getattr(os, "O_DIRECTORY", 0) - and os.open in os.supports_dir_fd + and _POSIX_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) + raise OSError(errno.ENOTSUP, unsupported_message) + access = getattr(os, "O_SEARCH", 0) or getattr(os, "O_PATH", 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 + # Older Darwin Python builds omit the kernel's O_EXEC/O_SEARCH bit. + access = 0x40000000 if sys.platform == "darwin" else os.O_RDONLY flags = access | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) parent = os.open(self._canonical_root, flags) try: @@ -252,6 +226,25 @@ def _creation_directory(self, directory: Path) -> Iterator[int | Path]: finally: os.close(parent) + @contextmanager + def _creation_directory(self, directory: Path) -> Iterator[int | Path]: + """Anchor an already policy-resolved parent without following new links.""" + if os.name == "nt": + parts = directory.relative_to(self._canonical_root).parts + 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 + + with self._posix_directory( + directory, unsupported_message="Anchored file creation is unsupported on this platform", + ) as parent: + yield parent + def create_bytes(self, path: Path | str, content: bytes, *, mode: int = 0o644) -> None: """Exclusively create a file; never overwrite an existing entry.""" destination = self._path(path, allow_missing=True, follow_leaf=False) @@ -321,9 +314,9 @@ def overwrite_text( @contextmanager def _overwrite_descriptor(self, destination: Path) -> Iterator[int]: """Anchor the resolved hierarchy before opening a non-truncating leaf.""" - parts = destination.relative_to(self._canonical_root).parts with ExitStack() as stack: if os.name == "nt": + parts = destination.relative_to(self._canonical_root).parts current = self._canonical_root stack.enter_context(_windows_handle(current, directory=True)) for part in parts[:-1]: @@ -332,37 +325,19 @@ def _overwrite_descriptor(self, destination: Path) -> Iterator[int]: yield stack.enter_context(_windows_overwrite_fd(destination)) return - if not ( - _OVERWRITE_DIR_FD_SUPPORTED - and getattr(os, "O_NOFOLLOW", 0) - and getattr(os, "O_DIRECTORY", 0) - and getattr(os, "O_NONBLOCK", 0) - ): - raise OSError(errno.ENOTSUP, "Scoped text overwrite is unsupported on this platform") - directory_access = getattr(os, "O_SEARCH", getattr(os, "O_PATH", None)) - if directory_access is None: - # Older Darwin Python builds omit the kernel's O_EXEC/O_SEARCH bit. - directory_access = 0x40000000 if sys.platform == "darwin" else os.O_RDONLY - directory_flags = ( - directory_access | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) - ) - parent = os.open(self._canonical_root, directory_flags) - try: - for part in parts[:-1]: - child = os.open(part, directory_flags, dir_fd=parent) - os.close(parent) - parent = child + unsupported = "Scoped text overwrite is unsupported on this platform" + if not getattr(os, "O_NONBLOCK", 0): + raise OSError(errno.ENOTSUP, unsupported) + with self._posix_directory(destination.parent, unsupported_message=unsupported) as parent: flags = ( os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0) ) - fd = os.open(parts[-1], flags, 0o666, dir_fd=parent) + fd = os.open(destination.name, flags, 0o666, dir_fd=parent) try: yield fd finally: os.close(fd) - finally: - os.close(parent) def symlink(self, target: Path | str, path: Path | str) -> None: """Create a link to an existing contained regular file or directory. diff --git a/tests/test_file_helper.py b/tests/test_file_helper.py index 45c28ceec1..d2afd1ac86 100644 --- a/tests/test_file_helper.py +++ b/tests/test_file_helper.py @@ -63,6 +63,106 @@ def test_plain_operations(root, allow): assert list(root.iterdir()) == [] +@pytest.mark.skipif(os.name == "nt", reason="POSIX shared directory traversal") +@pytest.mark.parametrize("operation", ["read_bytes", "create_bytes", "overwrite_text"]) +def test_posix_operations_share_parent_walk_and_preserve_leaf_flags(root, monkeypatch, operation): + parent = root / "parent/nested" + parent.mkdir(parents=True) + selected = parent / "file" + if operation != "create_bytes": + selected.write_bytes(b"inside") + parents = [] + opened = [] + original_walk = FileHelper._posix_directory + original_open = os.open + + @contextmanager + def observe_walk(self, directory, **kwargs): + parents.append(directory) + with original_walk(self, directory, **kwargs) as fd: + yield fd + + def observe_open(path, flags, *args, **kwargs): + opened.append((path, flags, args, kwargs)) + return original_open(path, flags, *args, **kwargs) + + monkeypatch.setattr(FileHelper, "_posix_directory", observe_walk) + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{**vars(os), "open": observe_open})) + files = FileHelper(root) + cloexec = getattr(os, "O_CLOEXEC", 0) + if operation == "read_bytes": + assert files.read_bytes(selected) == b"inside" + flags = os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | cloexec + mode = () + elif operation == "create_bytes": + files.create_bytes(selected, b"new") + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | os.O_NOFOLLOW + mode = (0o644,) + else: + files.overwrite_text(selected, "new") + flags = os.O_WRONLY | os.O_CREAT | os.O_NOFOLLOW | os.O_NONBLOCK | cloexec + mode = (0o666,) + assert parents == [parent.resolve()] + directory_calls = [call for call in opened if call[1] & os.O_DIRECTORY] + assert len(directory_calls) == 3 + assert len({call[1] for call in directory_calls}) == 1 + assert all(call[1] & os.O_NOFOLLOW for call in directory_calls) + leaf_calls = [call for call in opened if not call[1] & os.O_DIRECTORY] + assert len(leaf_calls) == 1 + assert leaf_calls[0][:3] == ("file", flags, mode) + assert leaf_calls[0][3]["dir_fd"] is not None + assert selected.read_bytes() == (b"inside" if operation == "read_bytes" else b"new") + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX shared directory cleanup") +@pytest.mark.parametrize("operation", ["read_bytes", "create_bytes", "overwrite_text"]) +@pytest.mark.parametrize("failure", ["root", "parent", "leaf"]) +def test_posix_parent_walk_closes_descriptors_after_open_failures(root, monkeypatch, operation, failure): + parent = root / "parent/nested" + parent.mkdir(parents=True) + selected = parent / "file" + if operation != "create_bytes": + selected.write_bytes(b"inside") + original_open = os.open + original_close = os.close + live = set() + failed = [] + + def observe_open(path, flags, *args, **kwargs): + fail_here = ( + (failure == "root" and Path(path) == root.resolve()) + or (failure == "parent" and path == "nested") + or (failure == "leaf" and path == "file") + ) + if fail_here: + failed.append(path) + raise PermissionError("injected open failure") + fd = original_open(path, flags, *args, **kwargs) + live.add(fd) + return fd + + def observe_close(fd): + original_close(fd) + live.remove(fd) + + monkeypatch.setattr(file_helper, "os", SimpleNamespace(**{ + **vars(os), "open": observe_open, "close": observe_close, + })) + with pytest.raises(PermissionError, match="injected"): + if operation == "read_bytes": + FileHelper(root).read_bytes(selected) + elif operation == "create_bytes": + FileHelper(root).create_bytes(selected, b"new") + else: + FileHelper(root).overwrite_text(selected, "new") + assert len(failed) == 1 + assert live == set() + if operation == "create_bytes": + assert not selected.exists() + else: + assert selected.read_bytes() == b"inside" + + @pytest.mark.parametrize("allow", [False, True]) @pytest.mark.parametrize("operation", ["read_bytes", "read_text"]) @pytest.mark.parametrize("kind", ["leaf", "parent", "resolved-link"]) diff --git a/tests/test_file_helper_overwrite.py b/tests/test_file_helper_overwrite.py index f433914d7f..6ed2238560 100644 --- a/tests/test_file_helper_overwrite.py +++ b/tests/test_file_helper_overwrite.py @@ -376,7 +376,7 @@ def unexpected_open(*args, **kwargs): attributes = {**vars(os), "open": unexpected_open} if capability == "dir_fd": - monkeypatch.setattr(file_helper, "_OVERWRITE_DIR_FD_SUPPORTED", False) + attributes["supports_dir_fd"] = set() elif value is None: attributes.pop(capability, None) else: