fix: respect Content-Disposition for URL dependencies

[wanxiankai]

Pull Request Check List

Resolves: #4472

Summary

Direct URL dependencies may point to endpoints such as /content or /master, while the response provides the real archive name in Content-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:

• uses the response's Content-Disposition filename when resolving direct URL dependencies;
• sanitizes both POSIX and Windows path components before writing to the artifact cache;
• lets the cache retain the downloader's final path; and
• reuses the locked package filename during installation, so the original URL can remain unchanged.

Testing

• Full test suite: 3180 passed, 27 skipped

• mypy

• pre-commit run --files <changed files>

• Added tests for changed code.

• Updated documentation for changed code. (Not required; no user-facing option or workflow changed.)

[sourcery-ai]

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>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[wanxiankai]

Addressed the review feedback in 7b38c77 and fc6aa52:

• persist and reuse the Content-Disposition filename for later strict cache lookups, preventing repeated downloads;
• constrain returned download paths to the artifact cache directory;
• harden remote filenames against invalid characters and excessive length;
• add regression coverage for repeated URL resolution, filename header edge cases, cache path boundaries, and zero/one/multiple locked files.

All review threads are resolved and the full GitHub Actions matrix is green. @Sanjays2402, could you please take another look when you have a chance?