[radoering]

Pull Request Check List

• Added tests for changed code.
• Updated documentation for changed code.

There is a significant performance regression due to #10792, which is mostly caused by the two calls of Path.resolve().

The time to install a sample project with some dependencies increased from 8 seconds (Poetry 2.3.2) to 13 seconds (Poetry 2.3.3).

With the fix, installation is almost as fast as before (less than half a second slower).

It should be safe to use os.path.abspath() instead of Path.resolve(). At least, I could not craft a wheel that triggers a path traversal via symlinks (see new test). Further, os.path.abspath() is recommended in https://docs.python.org/3/library/zipfile.html#zipfile.Path and (if I do not miss anything) pip also uses os.path.abspath() (and not Path.resolve()): https://github.com/pypa/pip/blob/8c5468dc9695c023c0a6428e630126aa14c9db4e/src/pip/_internal/utils/unpacking.py#L79-L87

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Previously is_relative_to from poetry.utils._compat had a specific workaround for Windows long-path prefix mismatches; switching to Path.is_relative_to may reintroduce those edge cases, so it would be good to confirm and, if necessary, preserve the compatibility helper or equivalent logic.
• The new comment references using os.path.commonpath() for validation, but the implementation uses Path.is_relative_to() instead; consider aligning the comment with the actual approach or updating the code to match the documented strategy to avoid future confusion.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Previously `is_relative_to` from `poetry.utils._compat` had a specific workaround for Windows long-path prefix mismatches; switching to `Path.is_relative_to` may reintroduce those edge cases, so it would be good to confirm and, if necessary, preserve the compatibility helper or equivalent logic.
- The new comment references using `os.path.commonpath()` for validation, but the implementation uses `Path.is_relative_to()` instead; consider aligning the comment with the actual approach or updating the code to match the documented strategy to avoid future confusion.

## Individual Comments

### Comment 1
<location path="tests/installation/test_wheel_installer.py" line_range="178-187" />
<code_context>
+    return wheel
+
+
+def test_no_path_traversal_via_symlink(
+    tmp_path: Path, env: MockEnv, wheel_with_symlink: Path
+) -> None:
+    target = tmp_path / "target"
+    target.mkdir()
+
+    installer = WheelInstaller(env)
+    with pytest.raises(FileNotFoundError if WINDOWS else NotADirectoryError):
+        installer.install(wheel_with_symlink)
+
+    traversal_link = Path(env.paths["purelib"]) / "symlink" / "traversal_link"
+    assert traversal_link.exists()
+    assert not traversal_link.is_symlink()  # not even extracted as symlink
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that no file is created at the symlink target path outside the installation directory.

The test already verifies that the symlink itself is not extracted and that installation fails. To more clearly prove traversal is blocked, also assert that the external target (e.g. `tmp_path / "target" / "traversal.txt"`) does not exist after installation, confirming no file is written outside the install root.
</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.}

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/installation/test_wheel_installer.py" line_range="134-143" />
<code_context>


-def test_path_traversal(env: MockEnv, wheel_with_path_traversal: Path) -> None:
+@pytest.mark.parametrize("existing", [False, True])
+def test_no_path_traversal(
+    env: MockEnv, wheel_with_path_traversal: Path, existing: bool
+) -> None:
+    target = env.path.parent / "traversal.txt"
+    if existing:
+        target.write_text("original", encoding="utf-8")
     installer = WheelInstaller(env)
     with pytest.raises(ValueError):
         installer.install(wheel_with_path_traversal)
-    assert not (env.path.parent / "traversal.txt").exists()
+
+    if existing:
+        assert target.exists()
+        assert target.read_text(encoding="utf-8") == "original"
+    else:
+        assert not target.exists()
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a positive test for a safe traversal-like path that stays within the target directory

The current tests cover rejecting traversal attempts and preserving/omitting files outside the env. To fully exercise the `abspath`-based checks, add a case where a ZIP entry contains `..` but still resolves inside the target directory (e.g. `"pkg/../in_pkg.txt"`), and assert that it is successfully written. This demonstrates that the logic permits safe normalized paths while blocking unsafe ones.

Suggested implementation:

```python
from poetry.installation.wheel_installer import WheelInstaller
from poetry.utils._compat import WINDOWS
from poetry.utils.env import MockEnv



@pytest.fixture
def env(tmp_path: Path) -> MockEnv:
    return MockEnv(path=tmp_path / "env")


def test_safe_normalized_path_allowed(
    env: MockEnv, wheel_with_safe_traversal: Path
) -> None:
    """
    A ZIP entry containing '..' that still normalizes inside the target directory
    should be accepted and written to disk.
    """
    installer = WheelInstaller(env)
    installer.install(wheel_with_safe_traversal)

    # The wheel should contain an entry like "pkg/../in_pkg.txt" which
    # normalizes to "in_pkg.txt" within the target directory.
    # Adjust the expected target path to match the MockEnv layout.
    target = env.site_packages / "in_pkg.txt"  # type: ignore[attr-defined]
    assert target.exists()
    assert target.read_text(encoding="utf-8") == "safe traversal"

```

To fully wire this test in, you will also need to:

1. **Add a `wheel_with_safe_traversal` fixture** in this file, next to `wheel_with_path_traversal`, for example:
   - Use the same wheel-building helper used by `wheel_with_path_traversal` (e.g. `_build_wheel`, `_make_wheel`, or similar).
   - Construct the wheel file contents with at least:
     - A safe, traversal-like path such as `"pkg/../in_pkg.txt": b"safe traversal"`.
     - Minimal package metadata so `WheelInstaller` accepts it (similar to the other wheel fixtures).
   - Return the path to the wheel file from the fixture.

2. **Confirm the target path** in the assertion:
   - If `WheelInstaller` installs into `env.site_packages`, keep `env.site_packages / "in_pkg.txt"` as written.
   - If it installs elsewhere (e.g. `env.site_packages / "traversal" / "in_pkg.txt"` or directly under `env.path`), adjust the `target = ...` expression accordingly.

3. **Ensure imports are sufficient**:
   - If `Path` or `pytest` are not already imported at the top of this file (they likely are), add:
     - `from pathlib import Path`
     - `import pytest`

With these adjustments, the new test will exercise the positive case where a path containing `..` resolves inside the target directory and is allowed by the `abspath`-based checks.
</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.}

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.