fix: sanitize extension name in download path to prevent path traversal#2582
Open
mnriem wants to merge 10 commits into
Open
fix: sanitize extension name in download path to prevent path traversal#2582mnriem wants to merge 10 commits into
mnriem wants to merge 10 commits into
Conversation
…al (GHSA-67q9-p54f-7cpr) The extension argument in 'specify extension add --from' was interpolated directly into the temporary ZIP download path. Absolute paths or ../ segments could escape .specify/extensions/.cache/downloads/, causing arbitrary file writes and deletes. Fix: strip directory components via Path.name, replace residual separators, and add a resolve().relative_to() containment guard before any I/O. CWE-22, CWE-73
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a path traversal vulnerability in URL-based extension installation by sanitizing the extension name before using it in the temporary ZIP download path and adding a containment guard.
Changes:
- Sanitizes
extensionwithPath(extension).nameplus separator replacement before constructing the download filename. - Adds a resolved-path containment check for the download ZIP path.
- Adds regression tests for traversal payloads and clean extension names.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/__init__.py |
Sanitizes and validates the URL download ZIP path in extension add --from. |
tests/test_extension_add_path_traversal.py |
Adds path traversal regression tests for URL-based extension installation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 7
This comment was marked as outdated.
This comment was marked as outdated.
mnriem
changed the title
…lete-vector + containment tests - Mock open_url in all traversal tests to avoid network dependency - Use uuid-based temp filename to prevent concurrent collisions - Add backslash traversal payloads (..\pwned, ..\..\etc\passwd) - Add sentinel-file test covering the delete vector - Use Path.relative_to() instead of str.startswith() for containment - Assert exit_code and mock_install.assert_called_once() in clean-name test
- Reject symlinked download cache directory (consistent with shared_infra.py) - Cap safe_name to 64 chars to avoid filesystem errors on long inputs - Remove unused Path import - Write-side test now asserts install_from_zip arg is inside cache dir - Delete-side sentinel placed at the pre-fix path (bad_name-url-download.zip) - Clean-name test verifies zip_path via mock call_args
…l assert - Symlink check now walks all ancestors (.specify, extensions, .cache, downloads) consistent with shared_infra.py pattern - Write-side test: unconditionally assert exit_code == 0 and mock_install called - Delete-side test: unconditionally assert sentinel exists (not conditional skip) - Remove unused 'call' import
- mkdir(parents=True) was following symlinked ancestors before the symlink check; replace with per-component walk that validates each ancestor and creates missing components individually (mirrors shared_infra._ensure_safe_shared_directory). - Add TestExtensionAddFromSymlinkedCache regression test covering symlinks at .specify, .specify/extensions, .cache, and downloads. - Skip sentinel creation for absolute-path payloads in the delete-side test to avoid writing /tmp/evil-url-download.zip into the runner; write-side test still proves containment for those payloads.
… write - Per-component walk now also re-resolves each existing ancestor and requires it to land under project_root.resolve(). This catches non-symlink directory aliases (e.g. Windows junctions / mount points) that resolve outside the project even though no component is itself a symlink. - ZIP write now uses os.open(O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0o600). O_NOFOLLOW (POSIX) refuses a symlink swapped in between the cache validation and the write, closing the TOCTOU window. O_EXCL guarantees the unique UUID filename is freshly created and cannot be pre-staged. O_NOFOLLOW falls back to 0 on Windows. - Add TestExtensionAddFromAncestorEscape: simulates a junction-like alias by patching Path.resolve() so .cache resolves outside; command must refuse with 'escapes project root'. - Add TestExtensionAddFromTOCTOUWrite: races a symlink swap into the cache between validation and write; O_NOFOLLOW/O_EXCL must reject.
…rrors Production: - Add _safe_open_download_zip helper. On POSIX it walks each cache ancestor with dir_fd + O_NOFOLLOW and creates the leaf relative to the deepest fd with O_EXCL|O_NOFOLLOW. This closes the ancestor-swap TOCTOU window (O_NOFOLLOW alone only protects the leaf). On Windows (no dir_fd/O_NOFOLLOW), falls back to plain O_EXCL; symlink attacks there require elevated privileges and are covered by the per-component validation walk. - Add is_dir() check on existing cache ancestors so a stray file at e.g. .specify/extensions yields a clean CLI error instead of a later NotADirectoryError from mkdir(). - Wrap the safe-open call and download write in OSError handlers so EEXIST/ELOOP/EACCES from the hardened open path surface as controlled CLI errors, not raw tracebacks. Tests: - Add _require_symlinks() helper that probes symlink creation and pytest.skip()s when unavailable (Windows without dev mode); apply to every test that creates symlinks. - Add TestExtensionAddFromTOCTOUWrite::test_swapped_ancestor_symlink_is_refused covering ancestor-swap; previous leaf-only test missed this vector. - Rewrite test_swapped_zip_path_symlink_is_refused to wrap the helper itself (no fragile os.open patching), and gate both TOCTOU tests on POSIX O_NOFOLLOW + dir_fd availability so Windows runners skip cleanly. - Add TestExtensionAddFromCacheAncestorIsFile to verify the new is_dir check produces a clean 'not a directory' error.
…ayloads 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.
…safe unlink - Move ExtensionManager construction after cache validation for --from installs so .specify/extensions/.registry is never read through a symlinked ancestor (review comment 5). - Add _validate_safe_cache_dir helper (extracted from inline walk) and _safe_unlink_download_zip helper that mirrors _safe_open_download_zip with dir_fd + O_NOFOLLOW on POSIX and re-validation on Windows, so cleanup cannot follow a swapped cache ancestor (review comment 2). - Harden _safe_open_download_zip Windows fallback to re-walk the cache ancestor chain immediately before opening the leaf, narrowing the validation->write TOCTOU window on platforms without dir_fd / O_NOFOLLOW (review comment 1). - Narrow the OSError handler around the cache write so it no longer wraps manager.install_from_zip(); install errors now propagate to the generic ExtensionError handler with their proper messages instead of being misreported as 'Failed to write download cache' (review comment 3). - Fix delete-side regression test to capture the runner result and assert exit_code == 0 before checking the sentinel, so a regression that crashes the install before cleanup cannot pass silently (review comment 4).
…ail-closed Windows fallback - Make _validate_safe_cache_dir tolerate concurrent installs: catch FileExistsError from current.mkdir() and re-run is_dir / symlink / containment checks against whatever is now there. Two parallel 'extension add --from' processes can no longer fatally race on cache directory creation (review comment 1). - Restructure the URL-install pipeline so _safe_unlink_download_zip runs in a single outer finally that wraps both the cache write and install_from_zip. A failed _zf.write() (e.g. ENOSPC) now triggers cleanup of the partial ZIP instead of leaving it behind (review comment 2). The narrow per-phase except blocks still classify which phase failed so error messages remain accurate. - Make _safe_open_download_zip fail closed on platforms without dir_fd / O_NOFOLLOW: there is no standard-library primitive that atomically binds a leaf open to a previously-validated ancestor chain, so a path-based open after a separate validation walk is racy (junction creation on Windows does not require elevation). Surface a clear error directing users to --dev or catalog installs (review comment 3). - Make _safe_unlink_download_zip a no-op on the same platforms for the symmetric reason: a path-based unlink could be redirected through a swapped ancestor onto a same-named file outside the project. In practice this branch is unreachable in the normal install flow because the open helper has already failed closed, so no ZIP exists to clean up (review comment 4).
Comment on lines
+3412
to
+3434
| leaf open, redirecting the write outside the project root. To avoid that | ||
| silent escape, this helper fails closed on such platforms instead. | ||
| Callers should surface a clear error and direct users to ``--dev`` or | ||
| catalog-based installs as alternatives. | ||
|
|
||
| Returns an open file descriptor; the caller owns and must close it. | ||
| Raises ``OSError`` (e.g. ``ELOOP``, ``EEXIST``, ``EACCES``) if any | ||
| component is a symlink or the leaf already exists, or | ||
| ``NotImplementedError`` on platforms lacking ``dir_fd`` support. | ||
| """ | ||
| o_nofollow = getattr(os, "O_NOFOLLOW", 0) | ||
| o_directory = getattr(os, "O_DIRECTORY", 0) | ||
| leaf_flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | o_nofollow | ||
|
|
||
| if os.open not in os.supports_dir_fd: | ||
| # Fail closed: see docstring. No safe primitive available here. | ||
| raise NotImplementedError( | ||
| "URL-based extension installs require POSIX-style " | ||
| "dir_fd + O_NOFOLLOW support, which is not available on this " | ||
| "platform. Use --dev for a local directory, or install from " | ||
| "the bundled catalog instead." | ||
| ) | ||
|
|
| # the symlink before any guard ran. Performing the safe per-component | ||
| # walk first ensures the manager only ever opens registry files inside a | ||
| # validated, project-contained tree. | ||
| if from_url: |
Comment on lines
+3347
to
+3387
| try: | ||
| current = project_root | ||
| for part in download_dir.relative_to(project_root).parts: | ||
| current = current / part | ||
| if current.is_symlink(): | ||
| console.print("[red]Error:[/red] Refusing to use symlinked download cache directory") | ||
| raise typer.Exit(1) | ||
| if current.exists(): | ||
| if not current.is_dir(): | ||
| console.print( | ||
| f"[red]Error:[/red] Download cache path is not a directory: {current}" | ||
| ) | ||
| raise typer.Exit(1) | ||
| try: | ||
| current.resolve().relative_to(project_root_resolved) | ||
| except (OSError, ValueError): | ||
| console.print("[red]Error:[/red] Download cache directory escapes project root") | ||
| raise typer.Exit(1) | ||
| continue | ||
| # Race-tolerant create: a concurrent `extension add --from` | ||
| # process may have created the same component between the | ||
| # `current.exists()` check above and this line. Treat | ||
| # `FileExistsError` as success and re-run the symlink / | ||
| # containment / is_dir checks against whatever is now there. | ||
| try: | ||
| current.mkdir() | ||
| except FileExistsError: | ||
| pass | ||
| if not current.is_dir(): | ||
| console.print( | ||
| f"[red]Error:[/red] Download cache path is not a directory: {current}" | ||
| ) | ||
| raise typer.Exit(1) | ||
| if current.is_symlink(): | ||
| console.print("[red]Error:[/red] Refusing to use symlinked download cache directory") | ||
| raise typer.Exit(1) | ||
| try: | ||
| current.resolve().relative_to(project_root_resolved) | ||
| except (OSError, ValueError): | ||
| console.print("[red]Error:[/red] Download cache directory escapes project root") | ||
| raise typer.Exit(1) |
Comment on lines
+3933
to
+3935
| manifest = manager.install_from_zip( | ||
| zip_path, speckit_version, priority=priority | ||
| ) |
Comment on lines
+3422
to
+3427
| o_nofollow = getattr(os, "O_NOFOLLOW", 0) | ||
| o_directory = getattr(os, "O_DIRECTORY", 0) | ||
| leaf_flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | o_nofollow | ||
|
|
||
| if os.open not in os.supports_dir_fd: | ||
| # Fail closed: see docstring. No safe primitive available here. |
Comment on lines
+3472
to
+3475
| o_nofollow = getattr(os, "O_NOFOLLOW", 0) | ||
| o_directory = getattr(os, "O_DIRECTORY", 0) | ||
|
|
||
| if os.open not in os.supports_dir_fd: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the path traversal vulnerability
The
extensionargument inspecify extension add <name> --from <url>was interpolated directly into the temporary ZIP download path:Absolute paths or
../segments in the extension argument could escape.specify/extensions/.cache/downloads/, causing the CLI to write downloaded bytes to an attacker-chosen local path and then delete that path in cleanup.Fix
Path(extension).namestrips all directory components (handles../traversal and absolute paths)/and\→_) as a belt-and-suspenders measureresolve().relative_to()containment guard rejects anything that still escapes — consistent with the existing Zip Slip guard pattern inextensions.pyThe fix is applied before any I/O, so both the write and delete vectors are eliminated.
Tests
New test file
tests/test_extension_add_path_traversal.py:../pwned,../../etc/passwd,subdir/../../escape,/tmp/evil)All existing path traversal and extension tests continue to pass.