Skip to content

Commit 4b1a13f

Browse files
committed
address review round 7: post-create resolve, mkdir error wrap, more payloads
Production: - Wrap the entire ancestor validation/create loop in OSError handling so create-time races (PermissionError, EEXIST from another process) surface as a clean CLI error instead of an unhandled exception. - Re-resolve every newly created component against project_root_resolved too (not just existing ones). A parent swapped to a junction / mount point / symlink during creation can land the new directory outside the project even when the new component itself is not a symlink. Mirrors shared_infra._ensure_safe_shared_directory:115-118. - Add a final 'download_dir.resolve() under project_root_resolved' guard immediately before opening the ZIP. Defends the Windows fallback path of _safe_open_download_zip (no dir_fd/O_NOFOLLOW) against an ancestor swap between validation and write that the existing zip_path.resolve().relative_to(download_dir.resolve()) check would silently follow. Tests: - Add deep relative traversal payload (../../../../../etc/shadow) that escapes from the 4-level cache directory all the way past project_root. - Add Windows-relevant absolute payloads: C:\tmp\evil, C:/tmp/evil, and \\server\share\evil. CI runs these on windows-latest. - Extract _is_absolute_like() helper covering POSIX absolute, Windows drive-letter, and UNC roots; use it to filter the delete-side test so sentinel creation never escapes the pytest sandbox on any platform.
1 parent db4e3b3 commit 4b1a13f

2 files changed

Lines changed: 80 additions & 24 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3687,41 +3687,69 @@ def extension_add(
36873687
# (consistent with shared_infra._ensure_safe_shared_directory):
36883688
# `mkdir(parents=True)` would silently traverse a symlinked ancestor,
36893689
# so each component is checked and created individually instead.
3690-
# Each existing component is also re-resolved and required to land
3691-
# under project_root, which catches non-symlink directory aliases
3692-
# (e.g. Windows junctions / mount points) that resolve outside.
3690+
# Each component (existing or newly created) is also re-resolved
3691+
# and required to land under project_root, which catches
3692+
# non-symlink directory aliases (e.g. Windows junctions / mount
3693+
# points) that resolve outside, including ones that may appear
3694+
# mid-creation via a parent swap.
36933695
import uuid as _uuid
36943696
download_dir = project_root / ".specify" / "extensions" / ".cache" / "downloads"
36953697
project_root_resolved = project_root.resolve()
3696-
_current = project_root
3697-
for _part in download_dir.relative_to(project_root).parts:
3698-
_current = _current / _part
3699-
if _current.is_symlink():
3700-
console.print("[red]Error:[/red] Refusing to use symlinked download cache directory")
3701-
raise typer.Exit(1)
3702-
if _current.exists():
3703-
if not _current.is_dir():
3704-
console.print(
3705-
f"[red]Error:[/red] Download cache path is not a directory: {_current}"
3706-
)
3698+
try:
3699+
_current = project_root
3700+
for _part in download_dir.relative_to(project_root).parts:
3701+
_current = _current / _part
3702+
if _current.is_symlink():
3703+
console.print("[red]Error:[/red] Refusing to use symlinked download cache directory")
3704+
raise typer.Exit(1)
3705+
if _current.exists():
3706+
if not _current.is_dir():
3707+
console.print(
3708+
f"[red]Error:[/red] Download cache path is not a directory: {_current}"
3709+
)
3710+
raise typer.Exit(1)
3711+
try:
3712+
_current.resolve().relative_to(project_root_resolved)
3713+
except (OSError, ValueError):
3714+
console.print("[red]Error:[/red] Download cache directory escapes project root")
3715+
raise typer.Exit(1)
3716+
continue
3717+
_current.mkdir()
3718+
if _current.is_symlink():
3719+
console.print("[red]Error:[/red] Refusing to use symlinked download cache directory")
37073720
raise typer.Exit(1)
3721+
# Post-create containment check: a parent swapped to a
3722+
# junction / mount / symlink during creation could land
3723+
# the new directory outside project_root even though
3724+
# this component itself is not a symlink.
37083725
try:
37093726
_current.resolve().relative_to(project_root_resolved)
37103727
except (OSError, ValueError):
37113728
console.print("[red]Error:[/red] Download cache directory escapes project root")
37123729
raise typer.Exit(1)
3713-
continue
3714-
_current.mkdir()
3715-
if _current.is_symlink():
3716-
console.print("[red]Error:[/red] Refusing to use symlinked download cache directory")
3717-
raise typer.Exit(1)
3730+
except OSError as _mkdir_err:
3731+
console.print(
3732+
f"[red]Error:[/red] Could not prepare download cache directory: {_mkdir_err}"
3733+
)
3734+
raise typer.Exit(1)
37183735

37193736
safe_name = Path(extension).name.replace("/", "_").replace("\\", "_") or "download"
37203737
safe_name = safe_name[:64] # cap length to avoid filesystem errors
37213738
zip_filename = f"{safe_name}-{_uuid.uuid4().hex[:8]}.zip"
37223739
zip_path = download_dir / zip_filename
37233740

3724-
# Guard: resolved path must stay inside download_dir (CWE-22)
3741+
# Guard: resolved zip_path must stay inside the validated cache
3742+
# directory *and* the cache directory must still resolve under
3743+
# project_root. The second check defends the Windows fallback
3744+
# path of `_safe_open_download_zip` (no `dir_fd`/`O_NOFOLLOW`)
3745+
# against an ancestor swapped to a junction/symlink between the
3746+
# validation loop above and this point: comparing only against
3747+
# `download_dir.resolve()` would silently follow the redirect.
3748+
try:
3749+
download_dir.resolve().relative_to(project_root_resolved)
3750+
except (OSError, ValueError):
3751+
console.print("[red]Error:[/red] Download cache directory escapes project root")
3752+
raise typer.Exit(1)
37253753
try:
37263754
zip_path.resolve().relative_to(download_dir.resolve())
37273755
except ValueError:

tests/test_extension_add_path_traversal.py

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,40 @@ def _require_symlinks(tmp_path) -> None:
3939
except OSError:
4040
pass
4141

42+
def _is_absolute_like(payload: str) -> bool:
43+
"""Detect payloads that may be treated as an absolute path on any supported OS.
44+
45+
Used to skip the delete-side regression test for those payloads, since
46+
constructing a sentinel relative to ``cache_dir`` for an absolute-like
47+
payload would land outside the pytest sandbox (especially on Windows
48+
where ``Path('C:\\\\...')`` and ``\\\\\\\\server\\\\share`` are roots).
49+
The write-side test still covers containment for these cases.
50+
"""
51+
if payload.startswith(("/", "\\")):
52+
return True
53+
# Drive letter forms: C:\..., C:/...
54+
if len(payload) >= 2 and payload[1] == ":" and payload[0].isalpha():
55+
return True
56+
return False
57+
58+
4259
TRAVERSAL_PAYLOADS = [
4360
"../pwned",
4461
"../../etc/passwd",
4562
"subdir/../../escape",
4663
"..\\pwned",
4764
"..\\..\\etc\\passwd",
4865
"/tmp/evil",
66+
# Deep relative traversal long enough to escape from
67+
# `.specify/extensions/.cache/downloads` (4 segments) all the way out
68+
# of project_root and beyond.
69+
"../../../../../etc/shadow",
70+
# Windows absolute path forms. The CI matrix runs these tests on
71+
# windows-latest, so the sanitiser must reject drive-letter and UNC
72+
# roots in addition to POSIX absolute paths.
73+
"C:\\tmp\\evil",
74+
"C:/tmp/evil",
75+
"\\\\server\\share\\evil",
4976
]
5077

5178

@@ -85,13 +112,14 @@ def test_traversal_payload_writes_inside_cache(self, project_dir, bad_name):
85112
zip_arg = mock_install.call_args[0][0] # positional arg: zip_path
86113
zip_arg.resolve().relative_to(cache_dir.resolve()) # raises ValueError if outside
87114

88-
@pytest.mark.parametrize("bad_name", [p for p in TRAVERSAL_PAYLOADS if not p.startswith(("/", "\\"))])
115+
@pytest.mark.parametrize("bad_name", [p for p in TRAVERSAL_PAYLOADS if not _is_absolute_like(p)])
89116
def test_traversal_payload_cannot_delete_outside_cache(self, project_dir, bad_name):
90117
"""The finally-block cleanup must not delete files outside the cache dir.
91118
92-
Absolute-path payloads are excluded here to avoid writing sentinels
93-
outside the pytest sandbox (e.g. ``/tmp``); the write-side test above
94-
already proves the production guard contains absolute payloads.
119+
Absolute-like payloads (POSIX absolute, Windows drive letter, UNC) are
120+
excluded here to avoid writing sentinels outside the pytest sandbox;
121+
the write-side test above already proves the production guard contains
122+
absolute payloads.
95123
"""
96124
# Place a sentinel at the path the pre-fix code would have constructed
97125
cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"

0 commit comments

Comments
 (0)