-
Notifications
You must be signed in to change notification settings - Fork 9.2k
fix: sanitize extension name in download path to prevent path traversal #2582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mnriem
wants to merge
10
commits into
github:main
Choose a base branch
from
mnriem:fix/extension-add-path-traversal
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+664
−11
Open
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f1c646f
fix: sanitize extension name in download path to prevent path travers…
mnriem 62499d7
address review: mock network, unique filename, backslash payloads, de…
mnriem 24bae93
address review round 2: symlink guard, length cap, mock-based assertions
mnriem 44bde39
address review round 3: walk ancestor symlinks, unconditional sentine…
mnriem 77b083c
address review round 4: TOCTOU-safe ancestor walk + sentinel hygiene
mnriem f43e2ce
address review round 5: ancestor-resolves-outside check + TOCTOU-safe…
mnriem db4e3b3
address review round 6: dir_fd-relative open + is_dir guard + clean e…
mnriem 4b1a13f
address review round 7: post-create resolve, mkdir error wrap, more p…
mnriem b3c21d3
address review round 8: delay manager construction, narrow handlers, …
mnriem 61e23ac
address review round 9: concurrent-safe mkdir, write-phase cleanup, f…
mnriem File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| """Tests for path traversal guard in `specify extension add --from` (GHSA-67q9-p54f-7cpr). | ||
|
|
||
| The extension argument is used to construct a temporary ZIP download path. | ||
| Without sanitisation, absolute paths or ``../`` segments in the argument can | ||
| escape the intended cache directory, causing arbitrary file writes and deletes. | ||
| """ | ||
|
|
||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
| from typer.testing import CliRunner | ||
|
|
||
| from specify_cli import app | ||
|
|
||
| runner = CliRunner() | ||
|
|
||
| TRAVERSAL_PAYLOADS = [ | ||
| "../pwned", | ||
| "../../etc/passwd", | ||
| "subdir/../../escape", | ||
|
mnriem marked this conversation as resolved.
|
||
| "..\\pwned", | ||
| "..\\..\\etc\\passwd", | ||
| "/tmp/evil", | ||
|
mnriem marked this conversation as resolved.
|
||
| ] | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def project_dir(tmp_path, monkeypatch): | ||
| proj = tmp_path / "project" | ||
| proj.mkdir() | ||
| (proj / ".specify").mkdir() | ||
| monkeypatch.chdir(proj) | ||
| return proj | ||
|
|
||
|
|
||
| def _mock_open_url(): | ||
| """Return a mock open_url that yields fake ZIP bytes.""" | ||
| mock_response = MagicMock() | ||
| mock_response.read.return_value = b"PK\x03\x04fake" | ||
| mock_response.__enter__ = MagicMock(return_value=mock_response) | ||
| mock_response.__exit__ = MagicMock(return_value=False) | ||
| return mock_response | ||
|
|
||
|
|
||
| class TestExtensionAddFromPathTraversal: | ||
| """Path traversal payloads in the extension name must not escape the download cache.""" | ||
|
|
||
| @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS) | ||
| def test_traversal_payload_writes_inside_cache(self, project_dir, bad_name): | ||
| """The zip_path passed to install_from_zip must resolve inside the cache dir.""" | ||
| cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads" | ||
| with patch("specify_cli.authentication.http.open_url", return_value=_mock_open_url()), \ | ||
| patch("specify_cli.extensions.ExtensionManager.install_from_zip", return_value=MagicMock(id="x", name="X", version="1.0.0")) as mock_install: | ||
| result = runner.invoke( | ||
| app, | ||
| ["extension", "add", bad_name, "--from", "https://example.com/ext.zip"], | ||
| ) | ||
| assert result.exit_code == 0, result.output | ||
| mock_install.assert_called_once() | ||
| zip_arg = mock_install.call_args[0][0] # positional arg: zip_path | ||
| zip_arg.resolve().relative_to(cache_dir.resolve()) # raises ValueError if outside | ||
|
|
||
| @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS) | ||
| def test_traversal_payload_cannot_delete_outside_cache(self, project_dir, bad_name): | ||
| """The finally-block cleanup must not delete files outside the cache dir.""" | ||
| # Place a sentinel at the path the pre-fix code would have constructed | ||
| cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads" | ||
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
| pre_fix_path = cache_dir / f"{bad_name}-url-download.zip" | ||
| try: | ||
| pre_fix_path.parent.mkdir(parents=True, exist_ok=True) | ||
| pre_fix_path.write_text("sentinel") | ||
| except OSError: | ||
| # Absolute payloads like /tmp/evil can't be created relative to cache | ||
| pre_fix_path = None | ||
|
mnriem marked this conversation as resolved.
Outdated
|
||
|
|
||
| with patch("specify_cli.authentication.http.open_url", return_value=_mock_open_url()), \ | ||
| patch("specify_cli.extensions.ExtensionManager.install_from_zip", return_value=MagicMock(id="x", name="X", version="1.0.0")): | ||
| runner.invoke( | ||
| app, | ||
| ["extension", "add", bad_name, "--from", "https://example.com/ext.zip"], | ||
| ) | ||
|
mnriem marked this conversation as resolved.
Outdated
|
||
| if pre_fix_path is not None: | ||
| assert pre_fix_path.exists(), f"Sentinel deleted by cleanup: {pre_fix_path}" | ||
| assert pre_fix_path.read_text() == "sentinel" | ||
|
|
||
| def test_clean_name_is_unaffected(self, project_dir): | ||
| """A normal extension name should not trigger the guard.""" | ||
| with patch("specify_cli.authentication.http.open_url", return_value=_mock_open_url()), \ | ||
| patch("specify_cli.extensions.ExtensionManager.install_from_zip") as mock_install: | ||
| mock_install.return_value = MagicMock(id="my-ext", name="My Ext", version="1.0.0") | ||
| result = runner.invoke( | ||
| app, | ||
| ["extension", "add", "my-ext", "--from", "https://example.com/ext.zip"], | ||
| ) | ||
|
|
||
| assert result.exit_code == 0, result.output | ||
| mock_install.assert_called_once() | ||
| # Verify the zip path is inside the cache | ||
| zip_arg = mock_install.call_args[0][0] | ||
| cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads" | ||
| zip_arg.resolve().relative_to(cache_dir.resolve()) | ||
| assert "path traversal" not in (result.output or "").lower() | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.