fix: respect Content-Disposition for URL dependencies#10980
Open
wanxiankai wants to merge 1 commit into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/poetry/utils/helpers.py" line_range="206-215" />
<code_context>
+ return downloader.destination if use_content_disposition else None
+
+
+def _filename_from_content_disposition(content_disposition: str, default: str) -> str:
+ message = Message()
+ message["Content-Disposition"] = content_disposition
+ filename = message.get_filename()
+ if not filename:
+ return default
+
+ # Content-Disposition is controlled by the remote server. Strip both POSIX and
+ # Windows path components so it cannot write outside of the cache directory.
+ filename = filename.replace("\\", "/").rsplit("/", 1)[-1]
+ return default if filename in {"", ".", ".."} else filename
+
</code_context>
<issue_to_address>
**suggestion:** Harden `_filename_from_content_disposition` against pathological or invalid filenames.
Given that `Content-Disposition` is remote-controlled, it would be good to also guard against a few remaining edge cases: very long filenames (to avoid pathological filesystem behavior) and characters that are invalid on some platforms (e.g., NUL, `:` on Windows). A simple length cap and rejecting filenames with clearly invalid characters before returning them would make this more robust against malicious or buggy servers.
Suggested implementation:
```python
from email.message import Message
from requests.exceptions import ChunkedEncodingError
```
```python
return downloader.destination if use_content_disposition else None
```
```python
+ return downloader.destination if use_content_disposition else None
+
+
+_MAX_REMOTE_FILENAME_LENGTH = 255
+_INVALID_REMOTE_FILENAME_CHARS = {"\x00", ":"}
+
+
+def _filename_from_content_disposition(content_disposition: str, default: str) -> str:
+ message = Message()
+ message["Content-Disposition"] = content_disposition
+ filename = message.get_filename()
+ if not filename:
+ return default
+
+ # Content-Disposition is controlled by the remote server. Strip both POSIX and
+ # Windows path components so it cannot write outside of the cache directory.
+ filename = filename.replace("\\", "/").rsplit("/", 1)[-1]
+
+ # Reject obviously unsafe or pathological filenames:
+ # - empty / parent-directory components
+ # - too long (can cause pathological filesystem behavior)
+ # - clearly invalid characters (e.g. NUL, ":" on Windows)
+ if filename in {"", ".", ".."}:
+ return default
+
+ if len(filename) > _MAX_REMOTE_FILENAME_LENGTH:
+ return default
+
+ if any(ch in filename for ch in _INVALID_REMOTE_FILENAME_CHARS):
+ return default
+
+ return filename
+
```
1. If `Message` is already imported elsewhere in this module, you should remove the duplicate import in the `from email.message import Message` change above.
2. If the project uses a centralized constants style instead of module-level constants, you may want to move `_MAX_REMOTE_FILENAME_LENGTH` and `_INVALID_REMOTE_FILENAME_CHARS` to that location and reference them here.
3. If there are existing tests around `_filename_from_content_disposition`, extend them to cover:
- filenames longer than `_MAX_REMOTE_FILENAME_LENGTH`
- filenames containing `"\x00"` and `":"`
- benign filenames at exactly the max length that are still accepted.
</issue_to_address>
### Comment 2
<location path="src/poetry/utils/cache.py" line_range="270-272" />
<code_context>
cache_dir.mkdir(parents=True, exist_ok=True)
try:
- download_func(link.url, cached_archive)
+ downloaded_archive = download_func(link.url, cached_archive)
+ if downloaded_archive is not None:
+ cached_archive = downloaded_archive
except BaseException:
cached_archive.unlink(missing_ok=True)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify expectations that `download_func` may return a different path than `dest`.
With the new `Callable[[str, Path], Path | None]` contract, `download_func` can now return a `Path` different from `cached_archive`, and you propagate that by reassigning `cached_archive`. While this is needed for `use_content_disposition`, it also broadens the API surface. If the intent is “same directory, maybe different filename”, consider enforcing (or at least asserting) that the returned path has the same parent as `cached_archive` so callers can’t accidentally redirect to an arbitrary location.
</issue_to_address>
### Comment 3
<location path="tests/utils/test_helpers.py" line_range="145-154" />
<code_context>
+ assert link.filename == filename
+
+
@pytest.mark.parametrize(
(
"is_sdist_cached",
</code_context>
<issue_to_address>
**suggestion (testing):** Add cases for empty/malformed Content-Disposition and mixed filename/filename* parameters
To further harden the test and document behavior around ambiguous headers, please add cases for:
- `Content-Disposition` with an empty filename (e.g. `attachment; filename=`) to assert we fall back to the default dest name.
- A header containing both `filename` and `filename*` to confirm which one is used and guard against future changes in `get_filename()` semantics.
These will better cover `_filename_from_content_disposition` for malformed/ambiguous headers.
</issue_to_address>
### Comment 4
<location path="tests/installation/test_executor.py" line_range="1081-1105" />
<code_context>
assert dest.exists(), "cached file should not be deleted"
+def test_executor_uses_locked_filename_for_url_dependency(
+ executor: Executor,
+ fixture_dir: FixtureDirGetter,
+ mocker: MockerFixture,
+) -> None:
+ filename = "demo-0.1.0-py2.py3-none-any.whl"
+ package = Package(
+ "demo",
+ "0.1.0",
+ source_type="url",
+ source_url="https://files.example.org/content",
+ )
+ package.files = [{"file": filename, "hash": "sha256:unused"}]
+ download_link = mocker.patch.object(
+ executor,
+ "_download_link",
+ return_value=fixture_dir("distributions") / filename,
+ )
+ mocker.patch.object(executor._wheel_installer, "install")
+
+ executor._install(Install(package))
+
+ link = download_link.call_args.args[1]
+ assert link.url == package.source_url
+ assert link.filename == filename
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for multiple or missing locked files when constructing the URL Link
Since the implementation branches on `len(package.files) == 1` and otherwise leaves `filename=None`, it would be helpful to add two more tests:
- `package.files` empty: assert we call `Link(package.source_url)` and `filename` stays `None`.
- `package.files` with multiple entries (e.g. sdist + wheel): assert we still pass `filename=None` and don’t pick any specific file.
This will ensure all URL-locked metadata shapes are covered and protect the special-case logic from regressions.
```suggestion
def test_executor_uses_locked_filename_for_url_dependency(
executor: Executor,
fixture_dir: FixtureDirGetter,
mocker: MockerFixture,
) -> None:
filename = "demo-0.1.0-py2.py3-none-any.whl"
package = Package(
"demo",
"0.1.0",
source_type="url",
source_url="https://files.example.org/content",
)
package.files = [{"file": filename, "hash": "sha256:unused"}]
download_link = mocker.patch.object(
executor,
"_download_link",
return_value=fixture_dir("distributions") / filename,
)
mocker.patch.object(executor._wheel_installer, "install")
executor._install(Install(package))
link = download_link.call_args.args[1]
assert link.url == package.source_url
assert link.filename == filename
def test_executor_uses_no_locked_filename_for_url_dependency_with_no_files(
executor: Executor,
fixture_dir: FixtureDirGetter,
mocker: MockerFixture,
) -> None:
package = Package(
"demo",
"0.1.0",
source_type="url",
source_url="https://files.example.org/content",
)
# Explicitly ensure no locked files metadata is present
package.files = []
filename = "demo-0.1.0-py2.py3-none-any.whl"
download_link = mocker.patch.object(
executor,
"_download_link",
return_value=fixture_dir("distributions") / filename,
)
mocker.patch.object(executor._wheel_installer, "install")
executor._install(Install(package))
link = download_link.call_args.args[1]
assert link.url == package.source_url
# When there are no locked files, we must not pin a filename
assert link.filename is None
def test_executor_uses_no_locked_filename_for_url_dependency_with_multiple_files(
executor: Executor,
fixture_dir: FixtureDirGetter,
mocker: MockerFixture,
) -> None:
wheel_filename = "demo-0.1.0-py2.py3-none-any.whl"
sdist_filename = "demo-0.1.0.tar.gz"
package = Package(
"demo",
"0.1.0",
source_type="url",
source_url="https://files.example.org/content",
)
# Multiple locked files (e.g. sdist + wheel) should not pick any specific one
package.files = [
{"file": wheel_filename, "hash": "sha256:unused"},
{"file": sdist_filename, "hash": "sha256:unused"},
]
download_link = mocker.patch.object(
executor,
"_download_link",
return_value=fixture_dir("distributions") / wheel_filename,
)
mocker.patch.object(executor._wheel_installer, "install")
executor._install(Install(package))
link = download_link.call_args.args[1]
assert link.url == package.source_url
# With multiple locked files, the implementation should leave filename unset
assert link.filename is None
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+206
to
+215
| def _filename_from_content_disposition(content_disposition: str, default: str) -> str: | ||
| message = Message() | ||
| message["Content-Disposition"] = content_disposition | ||
| filename = message.get_filename() | ||
| if not filename: | ||
| return default | ||
|
|
||
| # Content-Disposition is controlled by the remote server. Strip both POSIX and | ||
| # Windows path components so it cannot write outside of the cache directory. | ||
| filename = filename.replace("\\", "/").rsplit("/", 1)[-1] |
There was a problem hiding this comment.
suggestion: Harden _filename_from_content_disposition against pathological or invalid filenames.
Given that Content-Disposition is remote-controlled, it would be good to also guard against a few remaining edge cases: very long filenames (to avoid pathological filesystem behavior) and characters that are invalid on some platforms (e.g., NUL, : on Windows). A simple length cap and rejecting filenames with clearly invalid characters before returning them would make this more robust against malicious or buggy servers.
Suggested implementation:
from email.message import Message
from requests.exceptions import ChunkedEncodingError return downloader.destination if use_content_disposition else None+ return downloader.destination if use_content_disposition else None
+
+
+_MAX_REMOTE_FILENAME_LENGTH = 255
+_INVALID_REMOTE_FILENAME_CHARS = {"\x00", ":"}
+
+
+def _filename_from_content_disposition(content_disposition: str, default: str) -> str:
+ message = Message()
+ message["Content-Disposition"] = content_disposition
+ filename = message.get_filename()
+ if not filename:
+ return default
+
+ # Content-Disposition is controlled by the remote server. Strip both POSIX and
+ # Windows path components so it cannot write outside of the cache directory.
+ filename = filename.replace("\\", "/").rsplit("/", 1)[-1]
+
+ # Reject obviously unsafe or pathological filenames:
+ # - empty / parent-directory components
+ # - too long (can cause pathological filesystem behavior)
+ # - clearly invalid characters (e.g. NUL, ":" on Windows)
+ if filename in {"", ".", ".."}:
+ return default
+
+ if len(filename) > _MAX_REMOTE_FILENAME_LENGTH:
+ return default
+
+ if any(ch in filename for ch in _INVALID_REMOTE_FILENAME_CHARS):
+ return default
+
+ return filename
+- If
Messageis already imported elsewhere in this module, you should remove the duplicate import in thefrom email.message import Messagechange above. - If the project uses a centralized constants style instead of module-level constants, you may want to move
_MAX_REMOTE_FILENAME_LENGTHand_INVALID_REMOTE_FILENAME_CHARSto that location and reference them here. - If there are existing tests around
_filename_from_content_disposition, extend them to cover:- filenames longer than
_MAX_REMOTE_FILENAME_LENGTH - filenames containing
"\x00"and":" - benign filenames at exactly the max length that are still accepted.
- filenames longer than
Comment on lines
+270
to
+272
| downloaded_archive = download_func(link.url, cached_archive) | ||
| if downloaded_archive is not None: | ||
| cached_archive = downloaded_archive |
There was a problem hiding this comment.
suggestion (bug_risk): Clarify expectations that download_func may return a different path than dest.
With the new Callable[[str, Path], Path | None] contract, download_func can now return a Path different from cached_archive, and you propagate that by reassigning cached_archive. While this is needed for use_content_disposition, it also broadens the API surface. If the intent is “same directory, maybe different filename”, consider enforcing (or at least asserting) that the returned path has the same parent as cached_archive so callers can’t accidentally redirect to an arbitrary location.
Comment on lines
+145
to
+154
| @pytest.mark.parametrize( | ||
| ("content_disposition", "expected_filename"), | ||
| [ | ||
| ("attachment; filename=demo-0.1.0.tar.gz", "demo-0.1.0.tar.gz"), | ||
| ( | ||
| "attachment; filename=../../demo-0.1.0.tar.gz", | ||
| "demo-0.1.0.tar.gz", | ||
| ), | ||
| ( | ||
| "attachment; filename=..\\\\..\\\\demo-0.1.0.tar.gz", |
There was a problem hiding this comment.
suggestion (testing): Add cases for empty/malformed Content-Disposition and mixed filename/filename* parameters
To further harden the test and document behavior around ambiguous headers, please add cases for:
Content-Dispositionwith an empty filename (e.g.attachment; filename=) to assert we fall back to the default dest name.- A header containing both
filenameandfilename*to confirm which one is used and guard against future changes inget_filename()semantics.
These will better cover _filename_from_content_disposition for malformed/ambiguous headers.
Comment on lines
+1081
to
+1105
| def test_executor_uses_locked_filename_for_url_dependency( | ||
| executor: Executor, | ||
| fixture_dir: FixtureDirGetter, | ||
| mocker: MockerFixture, | ||
| ) -> None: | ||
| filename = "demo-0.1.0-py2.py3-none-any.whl" | ||
| package = Package( | ||
| "demo", | ||
| "0.1.0", | ||
| source_type="url", | ||
| source_url="https://files.example.org/content", | ||
| ) | ||
| package.files = [{"file": filename, "hash": "sha256:unused"}] | ||
| download_link = mocker.patch.object( | ||
| executor, | ||
| "_download_link", | ||
| return_value=fixture_dir("distributions") / filename, | ||
| ) | ||
| mocker.patch.object(executor._wheel_installer, "install") | ||
|
|
||
| executor._install(Install(package)) | ||
|
|
||
| link = download_link.call_args.args[1] | ||
| assert link.url == package.source_url | ||
| assert link.filename == filename |
There was a problem hiding this comment.
suggestion (testing): Add tests for multiple or missing locked files when constructing the URL Link
Since the implementation branches on len(package.files) == 1 and otherwise leaves filename=None, it would be helpful to add two more tests:
package.filesempty: assert we callLink(package.source_url)andfilenamestaysNone.package.fileswith multiple entries (e.g. sdist + wheel): assert we still passfilename=Noneand don’t pick any specific file.
This will ensure all URL-locked metadata shapes are covered and protect the special-case logic from regressions.
Suggested change
| def test_executor_uses_locked_filename_for_url_dependency( | |
| executor: Executor, | |
| fixture_dir: FixtureDirGetter, | |
| mocker: MockerFixture, | |
| ) -> None: | |
| filename = "demo-0.1.0-py2.py3-none-any.whl" | |
| package = Package( | |
| "demo", | |
| "0.1.0", | |
| source_type="url", | |
| source_url="https://files.example.org/content", | |
| ) | |
| package.files = [{"file": filename, "hash": "sha256:unused"}] | |
| download_link = mocker.patch.object( | |
| executor, | |
| "_download_link", | |
| return_value=fixture_dir("distributions") / filename, | |
| ) | |
| mocker.patch.object(executor._wheel_installer, "install") | |
| executor._install(Install(package)) | |
| link = download_link.call_args.args[1] | |
| assert link.url == package.source_url | |
| assert link.filename == filename | |
| def test_executor_uses_locked_filename_for_url_dependency( | |
| executor: Executor, | |
| fixture_dir: FixtureDirGetter, | |
| mocker: MockerFixture, | |
| ) -> None: | |
| filename = "demo-0.1.0-py2.py3-none-any.whl" | |
| package = Package( | |
| "demo", | |
| "0.1.0", | |
| source_type="url", | |
| source_url="https://files.example.org/content", | |
| ) | |
| package.files = [{"file": filename, "hash": "sha256:unused"}] | |
| download_link = mocker.patch.object( | |
| executor, | |
| "_download_link", | |
| return_value=fixture_dir("distributions") / filename, | |
| ) | |
| mocker.patch.object(executor._wheel_installer, "install") | |
| executor._install(Install(package)) | |
| link = download_link.call_args.args[1] | |
| assert link.url == package.source_url | |
| assert link.filename == filename | |
| def test_executor_uses_no_locked_filename_for_url_dependency_with_no_files( | |
| executor: Executor, | |
| fixture_dir: FixtureDirGetter, | |
| mocker: MockerFixture, | |
| ) -> None: | |
| package = Package( | |
| "demo", | |
| "0.1.0", | |
| source_type="url", | |
| source_url="https://files.example.org/content", | |
| ) | |
| # Explicitly ensure no locked files metadata is present | |
| package.files = [] | |
| filename = "demo-0.1.0-py2.py3-none-any.whl" | |
| download_link = mocker.patch.object( | |
| executor, | |
| "_download_link", | |
| return_value=fixture_dir("distributions") / filename, | |
| ) | |
| mocker.patch.object(executor._wheel_installer, "install") | |
| executor._install(Install(package)) | |
| link = download_link.call_args.args[1] | |
| assert link.url == package.source_url | |
| # When there are no locked files, we must not pin a filename | |
| assert link.filename is None | |
| def test_executor_uses_no_locked_filename_for_url_dependency_with_multiple_files( | |
| executor: Executor, | |
| fixture_dir: FixtureDirGetter, | |
| mocker: MockerFixture, | |
| ) -> None: | |
| wheel_filename = "demo-0.1.0-py2.py3-none-any.whl" | |
| sdist_filename = "demo-0.1.0.tar.gz" | |
| package = Package( | |
| "demo", | |
| "0.1.0", | |
| source_type="url", | |
| source_url="https://files.example.org/content", | |
| ) | |
| # Multiple locked files (e.g. sdist + wheel) should not pick any specific one | |
| package.files = [ | |
| {"file": wheel_filename, "hash": "sha256:unused"}, | |
| {"file": sdist_filename, "hash": "sha256:unused"}, | |
| ] | |
| download_link = mocker.patch.object( | |
| executor, | |
| "_download_link", | |
| return_value=fixture_dir("distributions") / wheel_filename, | |
| ) | |
| mocker.patch.object(executor._wheel_installer, "install") | |
| executor._install(Install(package)) | |
| link = download_link.call_args.args[1] | |
| assert link.url == package.source_url | |
| # With multiple locked files, the implementation should leave filename unset | |
| assert link.filename is None |
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
1 participant
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.
Pull Request Check List
Resolves: #4472
Summary
Direct URL dependencies may point to endpoints such as
/contentor/master, while the response provides the real archive name inContent-Disposition. Poetry previously cached those downloads under the URL path name, so package inspection or installation could not recognize the wheel or source archive.This change:
Content-Dispositionfilename when resolving direct URL dependencies;Testing
Full test suite: 3180 passed, 27 skipped
mypypre-commit run --files <changed files>Added tests for changed code.
Updated documentation for changed code. (Not required; no user-facing option or workflow changed.)