Skip to content

Commit f75d937

Browse files
mnriemCopilot
mnriem
and
Copilot
authored
fix(workflows): confine workflow and step catalog cache writes (#4931)
* 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 * 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 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 85ee2fff-5185-406e-b4e5-c961e0ed0938
1 parent 60ac849 commit f75d937

5 files changed

Lines changed: 789 additions & 61 deletions

File tree

‎docs/reference/workflows.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,14 @@ installed locally.
439439

440440
Workflow catalogs control where `search` and `add` look for workflows. Catalogs are checked in priority order.
441441

442+
Workflow and step catalogs cache fetched JSON and freshness metadata for one
443+
hour in `.specify/workflows/.cache/` and `.specify/workflows/steps/.cache/`,
444+
respectively. Cache I/O does not follow symlinked directory components or
445+
cache files. Writes atomically replace cache files rather than truncate their
446+
referents, so a pre-existing cache-file symlink can be replaced without changing
447+
its target. If a cache cannot be safely written, a warning is logged and
448+
discovery continues using the fetched catalog.
449+
442450
> **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)).
443451

444452
### List Catalogs

‎src/specify_cli/workflows/_catalog_cache.py‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
"""Confined filesystem I/O shared by workflow and step catalog caches."""
2+
3+
from __future__ import annotations
4+
5+
import errno
6+
import json
7+
import os
8+
import stat
9+
from collections.abc import Callable, Iterator
10+
from contextlib import ExitStack, contextmanager
11+
from pathlib import Path
12+
from typing import Any
13+
from uuid import uuid4
14+
15+
16+
@contextmanager
17+
def _windows_handle(path: Path, *, directory: bool) -> Iterator[int]:
18+
"""Pin a non-reparse path by withholding Windows delete sharing."""
19+
import ctypes
20+
from ctypes import wintypes
21+
22+
class AttributeTagInfo(ctypes.Structure):
23+
_fields_ = [("attributes", wintypes.DWORD), ("tag", wintypes.DWORD)]
24+
25+
kernel = ctypes.WinDLL("kernel32", use_last_error=True)
26+
create = kernel.CreateFileW
27+
create.argtypes = [
28+
wintypes.LPCWSTR,
29+
wintypes.DWORD,
30+
wintypes.DWORD,
31+
wintypes.LPVOID,
32+
wintypes.DWORD,
33+
wintypes.DWORD,
34+
wintypes.HANDLE,
35+
]
36+
create.restype = wintypes.HANDLE
37+
info = kernel.GetFileInformationByHandleEx
38+
info.argtypes = [wintypes.HANDLE, ctypes.c_int, wintypes.LPVOID, wintypes.DWORD]
39+
info.restype = wintypes.BOOL
40+
close = kernel.CloseHandle
41+
close.argtypes = [wintypes.HANDLE]
42+
close.restype = wintypes.BOOL
43+
44+
# OPEN_REPARSE_POINT inspects the link itself; BACKUP_SEMANTICS permits
45+
# directory handles. Holding every ancestor without FILE_SHARE_DELETE
46+
# prevents a junction/symlink swap during path-based create/replace.
47+
# Attribute-only handles bypass share checking: include FILE_LIST_DIRECTORY
48+
# with FILE_READ_ATTRIBUTES so even an empty directory cannot be renamed.
49+
handle = create(
50+
str(path),
51+
(0x80 | 0x1) if directory else 0x80000000,
52+
0x3,
53+
None,
54+
3,
55+
0x00200000 | 0x02000000,
56+
None,
57+
)
58+
if handle == wintypes.HANDLE(-1).value:
59+
raise ctypes.WinError(ctypes.get_last_error())
60+
try:
61+
attributes = AttributeTagInfo()
62+
if not info(handle, 9, ctypes.byref(attributes), ctypes.sizeof(attributes)):
63+
raise ctypes.WinError(ctypes.get_last_error())
64+
if attributes.attributes & 0x400:
65+
raise OSError(
66+
errno.ELOOP, "Refusing a reparse-point catalog cache path", str(path)
67+
)
68+
if bool(attributes.attributes & 0x10) != directory:
69+
raise OSError(errno.EINVAL, "Unexpected catalog cache file type", str(path))
70+
yield handle
71+
finally:
72+
close(handle)
73+
74+
75+
@contextmanager
76+
def _cache_directory(
77+
root: Path, directory: Path, *, create: bool
78+
) -> Iterator[int | Path]:
79+
relative = directory.relative_to(root)
80+
if ".." in relative.parts:
81+
raise OSError(errno.EINVAL, "Catalog cache escapes project root")
82+
root = root.resolve()
83+
if os.name == "nt":
84+
with ExitStack() as stack:
85+
current = root
86+
stack.enter_context(_windows_handle(current, directory=True))
87+
for part in relative.parts:
88+
current = current / part
89+
try:
90+
stack.enter_context(_windows_handle(current, directory=True))
91+
except FileNotFoundError:
92+
if not create:
93+
raise
94+
try:
95+
current.mkdir()
96+
except FileExistsError:
97+
pass
98+
stack.enter_context(_windows_handle(current, directory=True))
99+
yield current
100+
return
101+
102+
if not (
103+
getattr(os, "O_NOFOLLOW", 0)
104+
and getattr(os, "O_DIRECTORY", 0)
105+
and {os.open, os.mkdir, os.rename, os.unlink} <= os.supports_dir_fd
106+
):
107+
raise OSError(
108+
errno.ENOTSUP, "Safe catalog cache I/O is unsupported on this platform"
109+
)
110+
flags = os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW | getattr(os, "O_CLOEXEC", 0)
111+
parent = os.open(root, flags)
112+
try:
113+
for part in relative.parts:
114+
try:
115+
child = os.open(part, flags, dir_fd=parent)
116+
except FileNotFoundError:
117+
if not create:
118+
raise
119+
try:
120+
os.mkdir(part, dir_fd=parent)
121+
except FileExistsError:
122+
pass
123+
child = os.open(part, flags, dir_fd=parent)
124+
os.close(parent)
125+
parent = child
126+
yield parent
127+
finally:
128+
os.close(parent)
129+
130+
131+
def read_cache_json(
132+
root: Path,
133+
path: Path,
134+
*,
135+
object_pairs_hook: Callable[[list[tuple[str, Any]]], dict[str, Any]] | None = None,
136+
) -> Any:
137+
"""Read a regular cache leaf without following links or swapped ancestors."""
138+
with _cache_directory(root, path.parent, create=False) as directory_fd:
139+
if isinstance(directory_fd, Path):
140+
path = directory_fd / path.name
141+
with (
142+
_windows_handle(path, directory=False),
143+
open(path, encoding="utf-8") as stream,
144+
):
145+
return json.load(stream, object_pairs_hook=object_pairs_hook)
146+
flags = (
147+
os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK | getattr(os, "O_CLOEXEC", 0)
148+
)
149+
fd = os.open(path.name, flags, dir_fd=directory_fd)
150+
try:
151+
if not stat.S_ISREG(os.fstat(fd).st_mode):
152+
raise OSError(
153+
errno.EINVAL, "Catalog cache leaf is not a regular file", str(path)
154+
)
155+
stream = os.fdopen(fd, encoding="utf-8")
156+
except BaseException:
157+
os.close(fd)
158+
raise
159+
with stream:
160+
return json.load(stream, object_pairs_hook=object_pairs_hook)
161+
162+
163+
def write_cache_json(
164+
root: Path, path: Path, payload: dict[str, Any], *, indent: int | None = None
165+
) -> None:
166+
"""Atomically replace a cache leaf, never truncating its previous inode."""
167+
content = json.dumps(payload, indent=indent)
168+
with _cache_directory(root, path.parent, create=True) as directory:
169+
directory_fd = None if isinstance(directory, Path) else directory
170+
temporary = f".{path.name}.{uuid4().hex}.tmp"
171+
staged = directory / temporary if isinstance(directory, Path) else temporary
172+
destination = (
173+
directory / path.name if isinstance(directory, Path) else path.name
174+
)
175+
flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_CLOEXEC", 0)
176+
flags |= getattr(os, "O_NOFOLLOW", 0)
177+
fd = os.open(staged, flags, 0o600, dir_fd=directory_fd)
178+
try:
179+
try:
180+
stream = os.fdopen(fd, "w", encoding="utf-8")
181+
except BaseException:
182+
os.close(fd)
183+
raise
184+
with stream:
185+
stream.write(content)
186+
# replace operates on directory entries, not the destination's
187+
# referent, even if a symlink is installed just before this call.
188+
os.replace(
189+
staged, destination, src_dir_fd=directory_fd, dst_dir_fd=directory_fd
190+
)
191+
finally:
192+
try:
193+
os.unlink(staged, dir_fd=directory_fd)
194+
except FileNotFoundError:
195+
pass

‎src/specify_cli/workflows/catalog/_domain.py‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import hashlib
1313
import json
14+
import logging
1415
import os
1516
import stat
1617
import tempfile
@@ -25,6 +26,9 @@
2526
MAX_JSON_CATALOG_BYTES as MAX_JSON_CATALOG_BYTES,
2627
read_response_limited,
2728
)
29+
from .._catalog_cache import read_cache_json, write_cache_json
30+
31+
logger = logging.getLogger(__name__)
2832

2933

3034
def _max_json_catalog_bytes() -> int:
@@ -515,16 +519,13 @@ def _get_cache_paths(self, url: str) -> tuple[Path, Path]:
515519
def _is_url_cache_valid(self, url: str) -> bool:
516520
"""Check if cached data for a URL is still fresh."""
517521
_, meta_file = self._get_cache_paths(url)
518-
if not meta_file.exists():
519-
return False
520522
try:
521-
with open(meta_file, encoding="utf-8") as f:
522-
meta = json.load(f)
523+
meta = read_cache_json(self.project_root, meta_file)
523524
if not isinstance(meta, dict):
524525
return False
525526
fetched_at = float(meta.get("fetched_at", 0))
526527
return (time.time() - fetched_at) < self.CACHE_DURATION
527-
except (json.JSONDecodeError, OSError, TypeError, ValueError):
528+
except (UnicodeError, json.JSONDecodeError, OSError, TypeError, ValueError):
528529
return False
529530

530531
def _fetch_single_catalog(
@@ -535,8 +536,7 @@ def _fetch_single_catalog(
535536

536537
if not force_refresh and self._is_url_cache_valid(entry.url):
537538
try:
538-
with open(cache_file, encoding="utf-8") as f:
539-
cached = json.load(f)
539+
cached = read_cache_json(self.project_root, cache_file)
540540
if isinstance(cached, dict):
541541
return cached
542542
except (UnicodeDecodeError, json.JSONDecodeError, OSError):
@@ -600,15 +600,13 @@ def _validate_redirect(_old_url: str, new_url: str) -> None:
600600
)
601601
except Exception as exc:
602602
# Fall back to cache if available
603-
if cache_file.exists():
604-
try:
605-
with open(cache_file, encoding="utf-8") as f:
606-
cached = json.load(f)
607-
if isinstance(cached, dict):
608-
return cached
609-
except (json.JSONDecodeError, ValueError, OSError):
610-
# Stale-cache read failed; let the original fetch error propagate.
611-
pass
603+
try:
604+
cached = read_cache_json(self.project_root, cache_file)
605+
if isinstance(cached, dict):
606+
return cached
607+
except (UnicodeError, json.JSONDecodeError, ValueError, OSError):
608+
# Stale-cache read failed; let the original fetch error propagate.
609+
pass
612610
raise WorkflowCatalogError(
613611
f"Failed to fetch catalog from {entry.url}: {exc}"
614612
) from exc
@@ -620,13 +618,13 @@ def _validate_redirect(_old_url: str, new_url: str) -> None:
620618

621619
# Write cache
622620
try:
623-
self.cache_dir.mkdir(parents=True, exist_ok=True)
624-
with open(cache_file, "w", encoding="utf-8") as f:
625-
json.dump(data, f, indent=2)
626-
with open(meta_file, "w", encoding="utf-8") as f:
627-
json.dump({"url": entry.url, "fetched_at": time.time()}, f)
628-
except OSError:
629-
pass # Proceed without caching if disk write fails
621+
write_cache_json(self.project_root, cache_file, data, indent=2)
622+
write_cache_json(
623+
self.project_root, meta_file,
624+
{"url": entry.url, "fetched_at": time.time()},
625+
)
626+
except OSError as exc:
627+
logger.warning("Could not safely cache workflow catalog %s: %s", entry.url, exc)
630628

631629
return data
632630

0 commit comments

Comments
 (0)