From c8e81f2a3e93f693794d2df0f862d3fa918c0b90 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 12 Oct 2026 10:00:09 -0500 Subject: [PATCH 1/2] fix(workflows): confine workflow and step catalog cache writes Anchor catalog cache traversal and atomically replace JSON data and metadata without following repository-provided symlinks. Preserve discovery, freshness checks, refresh, and safe stale-cache fallback. Add isolated clone/search regressions, race and failure coverage, and native Windows filesystem tests. Document the defensive cache behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85ee2fff-5185-406e-b4e5-c961e0ed0938 --- docs/reference/workflows.md | 8 + src/specify_cli/workflows/_catalog_cache.py | 193 +++++++ src/specify_cli/workflows/catalog/_domain.py | 44 +- .../workflows/step/catalog/_domain.py | 67 +-- .../workflows/test_catalog_cache.py | 536 ++++++++++++++++++ 5 files changed, 787 insertions(+), 61 deletions(-) create mode 100644 src/specify_cli/workflows/_catalog_cache.py create mode 100644 tests/specify_cli/workflows/test_catalog_cache.py diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index b99fbea42c..6d1ae81445 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -423,6 +423,14 @@ installed locally. Workflow catalogs control where `search` and `add` look for workflows. Catalogs are checked in priority order. +Workflow and step catalogs cache fetched JSON and freshness metadata for one +hour in `.specify/workflows/.cache/` and `.specify/workflows/steps/.cache/`, +respectively. Cache I/O does not follow symlinked directory components or +cache files. Writes atomically replace cache files rather than truncate their +referents, so a pre-existing cache-file symlink can be replaced without changing +its target. If a cache cannot be safely written, a warning is logged and +discovery continues using the fetched catalog. + > **A project's `.specify/workflow-catalogs.yml` can point `add` and `search` at a catalog you didn't choose.** Before running a workflow from an unfamiliar project, run `specify workflow catalog list` (and `specify workflow step catalog list` for the step catalogs its steps can pull in) — a project supplying that config is not evidence its workflows or steps were vetted. Maintainers do not audit `run` fields; read a workflow's shell steps yourself before running it (see [Who maintains workflows?](#who-maintains-workflows)). ### List Catalogs diff --git a/src/specify_cli/workflows/_catalog_cache.py b/src/specify_cli/workflows/_catalog_cache.py new file mode 100644 index 0000000000..5270876b4f --- /dev/null +++ b/src/specify_cli/workflows/_catalog_cache.py @@ -0,0 +1,193 @@ +"""Confined filesystem I/O shared by workflow and step catalog caches.""" + +from __future__ import annotations + +import errno +import json +import os +import stat +from collections.abc import Callable, Iterator +from contextlib import ExitStack, contextmanager +from pathlib import Path +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. + handle = create( + str(path), + 0x80 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) + + +@contextmanager +def _cache_directory( + root: Path, directory: Path, *, create: bool +) -> Iterator[int | Path]: + relative = directory.relative_to(root) + if ".." in relative.parts: + raise OSError(errno.EINVAL, "Catalog cache escapes project root") + root = root.resolve() + if os.name == "nt": + with ExitStack() as stack: + current = root + stack.enter_context(_windows_handle(current, directory=True)) + for part in relative.parts: + current = current / part + try: + stack.enter_context(_windows_handle(current, directory=True)) + except FileNotFoundError: + if not create: + raise + try: + current.mkdir() + except FileExistsError: + pass + 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, os.mkdir, os.rename, os.unlink} <= os.supports_dir_fd + ): + raise OSError( + errno.ENOTSUP, "Safe catalog cache I/O is unsupported on this platform" + ) + flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0) + parent = os.open(root, flags) + try: + for part in relative.parts: + try: + child = os.open(part, flags, dir_fd=parent) + except FileNotFoundError: + if not create: + raise + try: + os.mkdir(part, dir_fd=parent) + except FileExistsError: + pass + child = os.open(part, flags, dir_fd=parent) + os.close(parent) + parent = child + yield parent + finally: + os.close(parent) + + +def read_cache_json( + root: Path, + path: Path, + *, + object_pairs_hook: Callable[[list[tuple[str, Any]]], dict[str, Any]] | None = None, +) -> Any: + """Read a regular cache leaf without following links or swapped ancestors.""" + with _cache_directory(root, path.parent, create=False) as directory_fd: + if isinstance(directory_fd, Path): + path = directory_fd / path.name + with ( + _windows_handle(path, directory=False), + open(path, encoding="utf-8") as stream, + ): + return json.load(stream, object_pairs_hook=object_pairs_hook) + flags = ( + os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0) + ) + fd = os.open(path.name, flags, dir_fd=directory_fd) + try: + if not stat.S_ISREG(os.fstat(fd).st_mode): + raise OSError( + errno.EINVAL, "Catalog cache leaf is not a regular file", str(path) + ) + stream = os.fdopen(fd, encoding="utf-8") + except BaseException: + os.close(fd) + raise + with stream: + return json.load(stream, object_pairs_hook=object_pairs_hook) + + +def write_cache_json( + root: Path, path: Path, payload: dict[str, Any], *, indent: int | None = None +) -> None: + """Atomically replace a cache leaf, never truncating its previous inode.""" + content = json.dumps(payload, indent=indent) + with _cache_directory(root, path.parent, create=True) as directory: + directory_fd = None if isinstance(directory, Path) else directory + temporary = f".{path.name}.{uuid4().hex}.tmp" + staged = directory / temporary if isinstance(directory, Path) else temporary + destination = ( + directory / path.name if isinstance(directory, Path) else path.name + ) + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_CLOEXEC", 0) + flags |= getattr(os, "O_NOFOLLOW", 0) + fd = os.open(staged, flags, 0o600, dir_fd=directory_fd) + try: + try: + stream = os.fdopen(fd, "w", encoding="utf-8") + except BaseException: + os.close(fd) + raise + with stream: + stream.write(content) + # replace operates on directory entries, not the destination's + # referent, even if a symlink is installed just before this call. + os.replace( + staged, destination, src_dir_fd=directory_fd, dst_dir_fd=directory_fd + ) + finally: + try: + os.unlink(staged, dir_fd=directory_fd) + except FileNotFoundError: + pass diff --git a/src/specify_cli/workflows/catalog/_domain.py b/src/specify_cli/workflows/catalog/_domain.py index 352618fee3..157f43c85c 100644 --- a/src/specify_cli/workflows/catalog/_domain.py +++ b/src/specify_cli/workflows/catalog/_domain.py @@ -11,6 +11,7 @@ import hashlib import json +import logging import os import stat import tempfile @@ -25,6 +26,9 @@ MAX_JSON_CATALOG_BYTES as MAX_JSON_CATALOG_BYTES, read_response_limited, ) +from .._catalog_cache import read_cache_json, write_cache_json + +logger = logging.getLogger(__name__) def _max_json_catalog_bytes() -> int: @@ -499,16 +503,13 @@ def _get_cache_paths(self, url: str) -> tuple[Path, Path]: def _is_url_cache_valid(self, url: str) -> bool: """Check if cached data for a URL is still fresh.""" _, meta_file = self._get_cache_paths(url) - if not meta_file.exists(): - return False try: - with open(meta_file, encoding="utf-8") as f: - meta = json.load(f) + meta = read_cache_json(self.project_root, meta_file) if not isinstance(meta, dict): return False fetched_at = float(meta.get("fetched_at", 0)) return (time.time() - fetched_at) < self.CACHE_DURATION - except (json.JSONDecodeError, OSError, TypeError, ValueError): + except (UnicodeError, json.JSONDecodeError, OSError, TypeError, ValueError): return False def _fetch_single_catalog( @@ -519,8 +520,7 @@ def _fetch_single_catalog( if not force_refresh and self._is_url_cache_valid(entry.url): try: - with open(cache_file, encoding="utf-8") as f: - cached = json.load(f) + cached = read_cache_json(self.project_root, cache_file) if isinstance(cached, dict): return cached except (UnicodeDecodeError, json.JSONDecodeError, OSError): @@ -584,15 +584,13 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: ) except Exception as exc: # Fall back to cache if available - if cache_file.exists(): - try: - with open(cache_file, encoding="utf-8") as f: - cached = json.load(f) - if isinstance(cached, dict): - return cached - except (json.JSONDecodeError, ValueError, OSError): - # Stale-cache read failed; let the original fetch error propagate. - pass + try: + cached = read_cache_json(self.project_root, cache_file) + if isinstance(cached, dict): + return cached + except (UnicodeError, json.JSONDecodeError, ValueError, OSError): + # Stale-cache read failed; let the original fetch error propagate. + pass raise WorkflowCatalogError( f"Failed to fetch catalog from {entry.url}: {exc}" ) from exc @@ -604,13 +602,13 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: # Write cache try: - self.cache_dir.mkdir(parents=True, exist_ok=True) - with open(cache_file, "w", encoding="utf-8") as f: - json.dump(data, f, indent=2) - with open(meta_file, "w", encoding="utf-8") as f: - json.dump({"url": entry.url, "fetched_at": time.time()}, f) - except OSError: - pass # Proceed without caching if disk write fails + write_cache_json(self.project_root, cache_file, data, indent=2) + write_cache_json( + self.project_root, meta_file, + {"url": entry.url, "fetched_at": time.time()}, + ) + except OSError as exc: + logger.warning("Could not safely cache workflow catalog %s: %s", entry.url, exc) return data diff --git a/src/specify_cli/workflows/step/catalog/_domain.py b/src/specify_cli/workflows/step/catalog/_domain.py index d1f9ecf75c..569d40cdd0 100644 --- a/src/specify_cli/workflows/step/catalog/_domain.py +++ b/src/specify_cli/workflows/step/catalog/_domain.py @@ -4,6 +4,7 @@ import hashlib import json +import logging import os import stat import tempfile @@ -18,6 +19,9 @@ from ...._download_security import ( read_response_limited, ) +from ..._catalog_cache import read_cache_json, write_cache_json + +logger = logging.getLogger(__name__) def _max_json_catalog_bytes() -> int: @@ -270,15 +274,6 @@ def __init__(self, project_root: Path) -> None: self.steps_dir = project_root / ".specify" / "workflows" / "steps" self.cache_dir = self.steps_dir / ".cache" - def _is_cache_path_safe(self) -> bool: - """Return False if any component of the cache path is a symlink.""" - current = self.project_root - for part in (".specify", "workflows", "steps", ".cache"): - current = current / part - if current.is_symlink(): - return False - return True - # -- Catalog resolution ----------------------------------------------- def _validate_catalog_url(self, url: str) -> None: @@ -461,29 +456,26 @@ def _get_cache_paths(self, url: str) -> tuple[Path, Path]: def _is_url_cache_valid(self, url: str) -> bool: """Check if cached data for a URL is still fresh.""" _, meta_file = self._get_cache_paths(url) - if not meta_file.exists(): - return False try: - with open(meta_file, encoding="utf-8") as f: - meta = json.load(f) + meta = read_cache_json(self.project_root, meta_file) if not isinstance(meta, dict): return False fetched_at = float(meta.get("fetched_at", 0)) return (time.time() - fetched_at) < self.CACHE_DURATION - except (json.JSONDecodeError, OSError, TypeError, ValueError): + except (UnicodeError, json.JSONDecodeError, OSError, TypeError, ValueError): return False def _fetch_single_catalog( self, entry: StepCatalogEntry, force_refresh: bool = False ) -> dict[str, Any]: """Fetch a single catalog, using cache when possible.""" - cache_safe = self._is_cache_path_safe() cache_file, meta_file = self._get_cache_paths(entry.url) - if cache_safe and not force_refresh and self._is_url_cache_valid(entry.url): + if not force_refresh and self._is_url_cache_valid(entry.url): try: - with open(cache_file, encoding="utf-8") as f: - cached = json.load(f, object_pairs_hook=_unique_json_fields) + cached = read_cache_json( + self.project_root, cache_file, object_pairs_hook=_unique_json_fields, + ) if isinstance(cached, dict): return cached except _DuplicateCatalogField: @@ -551,17 +543,17 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: except _DuplicateCatalogField: raise except Exception as exc: - if cache_safe and cache_file.exists(): - try: - with open(cache_file, encoding="utf-8") as f: - cached = json.load(f, object_pairs_hook=_unique_json_fields) - if isinstance(cached, dict): - return cached - except _DuplicateCatalogField: - raise - except (json.JSONDecodeError, ValueError, OSError): - # Stale-cache read failed; let the original fetch error propagate. - pass + try: + cached = read_cache_json( + self.project_root, cache_file, object_pairs_hook=_unique_json_fields, + ) + if isinstance(cached, dict): + return cached + except _DuplicateCatalogField: + raise + except (UnicodeError, json.JSONDecodeError, ValueError, OSError): + # Stale-cache read failed; let the original fetch error propagate. + pass raise StepCatalogError( f"Failed to fetch catalog from {entry.url}: {exc}" ) from exc @@ -571,15 +563,14 @@ def _validate_redirect(_old_url: str, new_url: str) -> None: f"Catalog from {entry.url} is not a valid JSON object." ) - if cache_safe: - try: - self.cache_dir.mkdir(parents=True, exist_ok=True) - with open(cache_file, "w", encoding="utf-8") as f: - json.dump(data, f, indent=2) - with open(meta_file, "w", encoding="utf-8") as f: - json.dump({"url": entry.url, "fetched_at": time.time()}, f) - except OSError: - pass # Proceed without caching if disk write fails + try: + write_cache_json(self.project_root, cache_file, data, indent=2) + write_cache_json( + self.project_root, meta_file, + {"url": entry.url, "fetched_at": time.time()}, + ) + except OSError as exc: + logger.warning("Could not safely cache step catalog %s: %s", entry.url, exc) return data diff --git a/tests/specify_cli/workflows/test_catalog_cache.py b/tests/specify_cli/workflows/test_catalog_cache.py new file mode 100644 index 0000000000..c6fe46353a --- /dev/null +++ b/tests/specify_cli/workflows/test_catalog_cache.py @@ -0,0 +1,536 @@ +"""Workflow and step catalog cache confinement regressions.""" + +from __future__ import annotations + +import io +import json +import os +import subprocess +import time +from urllib.error import URLError + +import pytest +import yaml +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.workflows import _catalog_cache +from specify_cli.workflows.catalog import ( + WorkflowCatalog, + WorkflowCatalogEntry, + WorkflowCatalogError, +) +from specify_cli.workflows.step.catalog import ( + StepCatalog, + StepCatalogEntry, + StepCatalogError, +) + +URL = "https://example.com/catalog.json" +SENTINEL = "isolated target must remain unchanged\n" + + +@pytest.fixture(params=["workflow", "step"]) +def catalog_kind(request): + if request.param == "workflow": + return ( + WorkflowCatalog, + WorkflowCatalogEntry, + "workflows", + "workflow-catalogs.yml", + ["workflow", "search"], + ) + return ( + StepCatalog, + StepCatalogEntry, + "steps", + "step-catalogs.yml", + ["workflow", "step", "search"], + ) + + +@pytest.fixture +def fetch(monkeypatch, catalog_kind): + payload = {catalog_kind[2]: {}} + calls = [] + + class Response(io.BytesIO): + def geturl(self): + return URL + + def open_url(*args, **kwargs): + calls.append(args[0]) + return Response(json.dumps(payload).encode()) + + monkeypatch.setattr("specify_cli.authentication.http.open_url", open_url) + return payload, calls + + +def symlink(link, target, *, directory=False): + try: + link.symlink_to(target, target_is_directory=directory) + except (OSError, NotImplementedError) as exc: + pytest.skip(f"Symlink creation unavailable: {exc}") + + +def junction(link, target): + import _winapi + + _winapi.CreateJunction(str(target), str(link)) + + +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +@pytest.mark.parametrize("dangling", [False, True]) +def test_cache_leaf_cannot_redirect_write( + tmp_path, catalog_kind, fetch, leaf, dangling +): + catalog_cls, entry_cls, *_ = catalog_kind + root = tmp_path / "project" + root.mkdir() + catalog = catalog_cls(root) + paths = catalog._get_cache_paths(URL) + paths[0].parent.mkdir(parents=True) + victim = tmp_path / "victim" + if not dangling: + victim.write_text(SENTINEL) + symlink(paths[leaf == "metadata"], victim) + + assert catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + if dangling: + assert not victim.exists() + else: + assert victim.read_text() == SENTINEL + + +@pytest.mark.skipif(os.name == "nt", reason="Git symlink checkout is POSIX-specific") +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +def test_committed_symlink_clone_search_preserves_target( + tmp_path, catalog_kind, fetch, monkeypatch, leaf +): + catalog_cls, _, _, config_name, command = catalog_kind + source = tmp_path / "source" + source.mkdir() + victim = tmp_path / "victim" + victim.write_text(SENTINEL) + paths = catalog_cls(source)._get_cache_paths(URL) + paths[0].parent.mkdir(parents=True) + link = paths[leaf == "metadata"] + symlink(link, victim) + (source / ".specify" / config_name).write_text( + yaml.safe_dump( + { + "catalogs": [ + { + "url": URL, + "name": "test", + "priority": 1, + "install_allowed": False, + } + ], + } + ) + ) + + def git(*args): + return subprocess.run( + ["git", *args], check=True, capture_output=True, text=True + ) + + git("init", "-q", str(source)) + git("-C", str(source), "add", ".") + git( + "-C", + str(source), + "-c", + "user.name=Cache Test", + "-c", + "user.email=cache@example.invalid", + "-c", + "commit.gpgsign=false", + "commit", + "-qm", + "Isolated cache fixture", + ) + clone = tmp_path / "clone" + git("clone", "-q", "--no-local", str(source), str(clone)) + assert (clone / link.relative_to(source)).is_symlink() + monkeypatch.delenv("SPECKIT_WORKFLOW_CATALOG_URL", raising=False) + monkeypatch.delenv("SPECKIT_STEP_CATALOG_URL", raising=False) + monkeypatch.chdir(clone) + + result = CliRunner().invoke(app, command) + + assert result.exit_code == 0, result.output + assert fetch[1] == [URL] + assert victim.read_text() == SENTINEL + + +@pytest.mark.parametrize("ancestor", [".specify", "workflows", "steps", ".cache"]) +def test_symlinked_cache_ancestor_cannot_redirect_write( + tmp_path, catalog_kind, fetch, ancestor +): + catalog_cls, entry_cls, *_ = catalog_kind + if catalog_cls is WorkflowCatalog and ancestor == "steps": + pytest.skip("Workflow caches have no steps ancestor") + root = tmp_path / "project" + root.mkdir() + catalog = catalog_cls(root) + rel = catalog.cache_dir.relative_to(root) + index = rel.parts.index(ancestor) + parent = root.joinpath(*rel.parts[:index]) + parent.mkdir(parents=True, exist_ok=True) + outside = tmp_path / "outside" + outside.mkdir() + symlink(parent / ancestor, outside, directory=True) + + assert catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + assert list(outside.iterdir()) == [] + + +def test_regular_cache_fresh_refresh_and_stale_fallback( + tmp_path, catalog_kind, fetch, monkeypatch +): + catalog_cls, entry_cls, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + entry = entry_cls(URL, "test", 1, False) + data, metadata = catalog._get_cache_paths(URL) + + assert catalog._fetch_single_catalog(entry) == fetch[0] + assert json.loads(data.read_text()) == fetch[0] + assert json.loads(metadata.read_text())["url"] == URL + assert catalog._is_url_cache_valid(URL) + assert catalog._fetch_single_catalog(entry) == fetch[0] + assert fetch[1] == [URL] + assert catalog._fetch_single_catalog(entry, force_refresh=True) == fetch[0] + assert fetch[1] == [URL, URL] + metadata.write_text(json.dumps({"fetched_at": time.time() - 7200})) + + def unavailable(*args, **kwargs): + raise URLError("isolated offline test") + + monkeypatch.setattr("specify_cli.authentication.http.open_url", unavailable) + assert catalog._fetch_single_catalog(entry) == fetch[0] + + +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +@pytest.mark.parametrize("fresh", [False, True]) +def test_symlinked_cache_is_not_read( + tmp_path, catalog_kind, fetch, monkeypatch, leaf, fresh +): + catalog_cls, entry_cls, field, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + data, metadata = catalog._get_cache_paths(URL) + data.parent.mkdir(parents=True) + poisoned = {field: {"outside": {"name": "Outside"}}} + timestamp = time.time() if fresh else 0 + data.write_text(json.dumps(poisoned)) + metadata.write_text(json.dumps({"fetched_at": timestamp})) + target = tmp_path / "outside.json" + path = (data, metadata)[leaf == "metadata"] + target.write_bytes(path.read_bytes()) + path.unlink() + symlink(path, target) + + if fresh: + assert ( + catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + ) + assert fetch[1] == [URL] + else: + + def unavailable(*args, **kwargs): + raise URLError("isolated offline test") + + monkeypatch.setattr("specify_cli.authentication.http.open_url", unavailable) + if leaf == "data": + with pytest.raises( + (WorkflowCatalogError, StepCatalogError), + match="Failed to fetch catalog", + ): + catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) + else: + # An unsafe metadata leaf cannot make the cache fresh, but a safe + # data leaf remains usable as the ordinary stale/offline fallback. + assert ( + catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) + == poisoned + ) + + +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +def test_cache_replacement_preserves_hardlink_target( + tmp_path, catalog_kind, fetch, leaf +): + catalog_cls, entry_cls, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + paths = catalog._get_cache_paths(URL) + paths[0].parent.mkdir(parents=True) + victim = tmp_path / "victim" + victim.write_text(SENTINEL) + os.link(victim, paths[leaf == "metadata"]) + + assert catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + assert victim.read_text() == SENTINEL + assert json.loads(paths[leaf == "metadata"].read_text()) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor swap regression") +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +def test_leaf_swap_at_commit_cannot_redirect_write( + tmp_path, catalog_kind, monkeypatch, leaf +): + catalog_cls, *_ = catalog_kind + path = catalog_cls(tmp_path)._get_cache_paths(URL)[leaf == "metadata"] + victim = tmp_path / "victim" + victim.write_text(SENTINEL) + replace = os.replace + + def swap_then_replace(source, destination, **kwargs): + symlink(path, victim) + return replace(source, destination, **kwargs) + + monkeypatch.setattr(os, "replace", swap_then_replace) + _catalog_cache.write_cache_json(tmp_path, path, {"safe": True}) + + assert victim.read_text() == SENTINEL + assert not path.is_symlink() + assert json.loads(path.read_text()) == {"safe": True} + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor swap regression") +@pytest.mark.parametrize("ancestor", [".specify", "workflows", "steps", ".cache"]) +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +def test_ancestor_swap_at_commit_cannot_redirect_write( + tmp_path, + catalog_kind, + monkeypatch, + ancestor, + leaf, +): + catalog_cls, *_ = catalog_kind + if catalog_cls is WorkflowCatalog and ancestor == "steps": + pytest.skip("Workflow caches have no steps ancestor") + root = tmp_path / "project" + root.mkdir() + path = catalog_cls(root)._get_cache_paths(URL)[leaf == "metadata"] + parts = path.parent.relative_to(root).parts + index = parts.index(ancestor) + swapped = root.joinpath(*parts[: index + 1]) + retained = swapped.with_name(f"{swapped.name}-retained") + outside = tmp_path / "outside" + outside.mkdir() + replace = os.replace + + def swap_then_replace(source, destination, **kwargs): + swapped.rename(retained) + symlink(swapped, outside, directory=True) + return replace(source, destination, **kwargs) + + monkeypatch.setattr(os, "replace", swap_then_replace) + _catalog_cache.write_cache_json(root, path, {"safe": True}) + + assert list(outside.iterdir()) == [] + retained_leaf = retained.joinpath(*parts[index + 1 :], path.name) + assert json.loads(retained_leaf.read_text()) == {"safe": True} + assert not list(retained_leaf.parent.glob("*.tmp")) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX no-follow open regression") +def test_directory_swap_before_open_is_refused(tmp_path, catalog_kind, monkeypatch): + catalog_cls, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + catalog.cache_dir.mkdir(parents=True) + path = catalog._get_cache_paths(URL)[0] + outside = tmp_path / "outside" + outside.mkdir() + original_open = os.open + + def swap_then_open(name, flags, *args, **kwargs): + if name == ".cache": + catalog.cache_dir.rename(catalog.cache_dir.with_name("retained")) + symlink(catalog.cache_dir, outside, directory=True) + return original_open(name, flags, *args, **kwargs) + + monkeypatch.setattr(os, "open", swap_then_open) + monkeypatch.setattr(os, "supports_dir_fd", os.supports_dir_fd | {swap_then_open}) + with pytest.raises(OSError): + _catalog_cache.write_cache_json(tmp_path, path, {"safe": True}) + assert list(outside.iterdir()) == [] + + +@pytest.mark.parametrize("leaf", ["data", "metadata"]) +def test_failed_replace_preserves_existing_cache_and_logs( + tmp_path, + catalog_kind, + fetch, + monkeypatch, + caplog, + leaf, +): + catalog_cls, entry_cls, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + path = catalog._get_cache_paths(URL)[leaf == "metadata"] + path.parent.mkdir(parents=True) + path.write_text(SENTINEL) + original_replace = os.replace + + def fail_replace(source, destination, **kwargs): + if str(destination).endswith(path.name): + raise PermissionError("isolated replace failure") + return original_replace(source, destination, **kwargs) + + monkeypatch.setattr(os, "replace", fail_replace) + assert ( + catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False), True) + == fetch[0] + ) + assert path.read_text() == SENTINEL + assert "Could not safely cache" in caplog.text + assert "isolated replace failure" in caplog.text + assert not list(path.parent.glob("*.tmp")) + + +def test_serialization_failure_preserves_existing_cache(tmp_path, catalog_kind): + catalog_cls, *_ = catalog_kind + path = catalog_cls(tmp_path)._get_cache_paths(URL)[0] + path.parent.mkdir(parents=True) + path.write_text(SENTINEL) + + with pytest.raises(TypeError): + _catalog_cache.write_cache_json(tmp_path, path, {"invalid": object()}) + + assert path.read_text() == SENTINEL + assert not list(path.parent.glob("*.tmp")) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows resolved-root cache regression") +def test_path_io_uses_pinned_directory_not_original_alias(tmp_path, catalog_kind): + catalog_cls, *_ = catalog_kind + root = tmp_path / "project" + root.mkdir() + alias = tmp_path / "original-alias" + junction(alias, root) + path = catalog_cls(alias)._get_cache_paths(URL)[0] + _catalog_cache.write_cache_json(alias, path, {"safe": True}) + + with _catalog_cache._cache_directory(alias, path.parent, create=False) as pinned: + assert pinned == path.parent.resolve() + assert pinned != path.parent + assert _catalog_cache.read_cache_json(alias, path) == {"safe": True} + assert json.loads(catalog_cls(root)._get_cache_paths(URL)[0].read_text()) == { + "safe": True + } + + +@pytest.mark.skipif(os.name != "nt", reason="Windows handle pinning regression") +@pytest.mark.parametrize("ancestor", [".specify", "workflows", "steps", ".cache"]) +def test_windows_directory_handle_prevents_swap(tmp_path, catalog_kind, ancestor): + catalog_cls, *_ = catalog_kind + if catalog_cls is WorkflowCatalog and ancestor == "steps": + pytest.skip("Workflow caches have no steps ancestor") + catalog = catalog_cls(tmp_path) + catalog.cache_dir.mkdir(parents=True) + parts = catalog.cache_dir.relative_to(tmp_path).parts + path = tmp_path.joinpath(*parts[: parts.index(ancestor) + 1]) + with ( + _catalog_cache._cache_directory(tmp_path, catalog.cache_dir, create=False), + pytest.raises(OSError), + ): + path.rename(path.with_name("swapped")) + + path.rename(path.with_name("swapped")) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows native file handle regression") +@pytest.mark.parametrize( + ("kind", "directory", "allowed"), + [ + ("directory", True, True), + ("file", False, True), + ("junction", True, False), + ("symlink", False, False), + ("file", True, False), + ("directory", False, False), + ], +) +def test_windows_handle_rejects_reparse_points_and_wrong_types( + tmp_path, + kind, + directory, + allowed, +): + path = tmp_path / "path" + if kind == "directory": + path.mkdir() + elif kind == "file": + path.write_text(SENTINEL) + elif kind == "junction": + target = tmp_path / "target" + target.mkdir() + junction(path, target) + else: + target = tmp_path / "target" + target.write_text(SENTINEL) + symlink(path, target) + + if allowed: + with _catalog_cache._windows_handle(path, directory=directory) as handle: + assert handle is not None + with pytest.raises(OSError): + path.rename(path.with_name("renamed")) + path.rename(path.with_name("renamed")) + else: + with ( + pytest.raises(OSError), + _catalog_cache._windows_handle( + path, + directory=directory, + ), + ): + pytest.fail("An unsafe Windows path was accepted") + path.rename(path.with_name("renamed")) + + +@pytest.mark.skipif(os.name != "nt", reason="Windows native junction cache regression") +@pytest.mark.parametrize("ancestor", [".specify", "workflows", "steps", ".cache"]) +def test_windows_junction_ancestor_cannot_redirect_write( + tmp_path, + catalog_kind, + fetch, + caplog, + ancestor, +): + catalog_cls, entry_cls, *_ = catalog_kind + if catalog_cls is WorkflowCatalog and ancestor == "steps": + pytest.skip("Workflow caches have no steps ancestor") + root = tmp_path / "project" + root.mkdir() + catalog = catalog_cls(root) + parts = catalog.cache_dir.relative_to(root).parts + index = parts.index(ancestor) + parent = root.joinpath(*parts[:index]) + parent.mkdir(parents=True, exist_ok=True) + outside = tmp_path / "outside" + outside.mkdir() + junction(parent / ancestor, outside) + + assert catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + assert list(outside.iterdir()) == [] + assert "Could not safely cache" in caplog.text + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX capability refusal regression") +def test_unsupported_safe_io_continues_discovery_with_warning( + tmp_path, + catalog_kind, + fetch, + monkeypatch, + caplog, +): + catalog_cls, entry_cls, *_ = catalog_kind + catalog = catalog_cls(tmp_path) + monkeypatch.setattr(os, "supports_dir_fd", set()) + + assert catalog._fetch_single_catalog(entry_cls(URL, "test", 1, False)) == fetch[0] + assert not catalog.cache_dir.exists() + assert "Safe catalog cache I/O is unsupported" in caplog.text From bc88e862146fcb9b051968b46bdad25ec63a224a Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Mon, 12 Oct 2026 11:35:24 -0500 Subject: [PATCH 2/2] fix(workflows): enforce Windows directory delete sharing Include FILE_LIST_DIRECTORY in directory handle access so no-delete sharing participates in Windows share checks. FILE_READ_ATTRIBUTES alone allows empty cache directories to be renamed while held, as demonstrated by the three native Windows CI failures. Keep the native Windows rename, ancestor pinning, and reparse-point regressions unchanged. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85ee2fff-5185-406e-b4e5-c961e0ed0938 --- src/specify_cli/workflows/_catalog_cache.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/_catalog_cache.py b/src/specify_cli/workflows/_catalog_cache.py index 5270876b4f..5a64609bc0 100644 --- a/src/specify_cli/workflows/_catalog_cache.py +++ b/src/specify_cli/workflows/_catalog_cache.py @@ -44,9 +44,11 @@ class AttributeTagInfo(ctypes.Structure): # OPEN_REPARSE_POINT inspects the link itself; BACKUP_SEMANTICS permits # directory handles. Holding every ancestor without FILE_SHARE_DELETE # prevents a junction/symlink swap during path-based create/replace. + # Attribute-only handles bypass share checking: include FILE_LIST_DIRECTORY + # with FILE_READ_ATTRIBUTES so even an empty directory cannot be renamed. handle = create( str(path), - 0x80 if directory else 0x80000000, + (0x80 | 0x1) if directory else 0x80000000, 0x3, None, 3,