Index: pip-26.1.2/news/14110.bugfix.rst =================================================================== --- /dev/null +++ pip-26.1.2/news/14110.bugfix.rst @@ -0,0 +1 @@ +Fix ``Link.filename`` decoding the URL path twice. Index: pip-26.1.2/src/pip/_internal/models/link.py =================================================================== --- pip-26.1.2.orig/src/pip/_internal/models/link.py +++ pip-26.1.2/src/pip/_internal/models/link.py @@ -14,6 +14,7 @@ from dataclasses import dataclass from typing import ( Any, NamedTuple, + NewType, ) from pip._internal.exceptions import InvalidEggFragment @@ -31,6 +32,49 @@ from pip._internal.utils.urls import pat logger = logging.getLogger(__name__) +# A single path component: percent-decoded once and reduced to a basename, so it +# contains no path separator and is not a ``.`` or ``..`` reference. The empty +# string means "no component". +PathComponent = NewType("PathComponent", str) + + +def _to_path_component(name: str) -> PathComponent: + """Reduce ``name`` to a single path component, or ``""`` if it has none. + + ``os.path.basename`` drops any directory part, drive letter, or separator; + a ``.``, ``..``, or empty result is not a component and becomes ``""``. + """ + name = os.path.basename(name) + if name in ("", os.curdir, os.pardir): + return PathComponent("") + + return PathComponent(name) + + +def as_path_component(name: str) -> PathComponent: + """Like ``_to_path_component`` but reject the empty result. + + Use where a file is about to be written, so a missing name is an error + rather than a silent fallback to the directory itself. + """ + component = _to_path_component(name) + if not component: + raise ValueError(f"Unexpected file name derived from URL: {name!r}") + + return component + + +def join_within_directory(directory: str, component: PathComponent) -> str: + """Join a single path ``component`` onto ``directory``. + + ``component`` is a :data:`PathComponent`, so by type it has no separator and + is not a ``.`` or ``..`` reference; the result can never escape ``directory``. + Requiring ``PathComponent`` rather than ``str`` lets the type checker enforce + at the call site that the name was reduced to a safe component beforehand. + """ + return os.path.join(directory, component) + + # Order matters, earlier hashes have a precedence over later hashes for what # we will pick to use. _SUPPORTED_HASHES = ("sha512", "sha384", "sha256", "sha224", "sha1", "md5") @@ -423,18 +467,13 @@ class Link: return redact_auth_from_url(self.url) @property - def filename(self) -> str: - path = self.path.rstrip("/") - name = posixpath.basename(path) - if not name: - # Make sure we don't leak auth information if the netloc - # includes a username and password. - netloc, user_pass = split_auth_from_netloc(self.netloc) - return netloc - - name = urllib.parse.unquote(name) - assert name, f"URL {self._url!r} produced no filename" - return name + def filename(self) -> PathComponent: + name = _to_path_component(posixpath.basename(self.path.rstrip("/"))) + if name: + return name + + # No component in the path; fall back to the netloc, dropping any auth. + return _to_path_component(split_auth_from_netloc(self.netloc)[0]) @property def file_path(self) -> str: Index: pip-26.1.2/src/pip/_internal/network/download.py =================================================================== --- pip-26.1.2.orig/src/pip/_internal/network/download.py +++ pip-26.1.2/src/pip/_internal/network/download.py @@ -19,7 +19,12 @@ from pip._vendor.urllib3.exceptions impo from pip._internal.cli.progress_bars import BarType, get_download_progress_renderer from pip._internal.exceptions import IncompleteDownloadError, NetworkConnectionError -from pip._internal.models.link import Link +from pip._internal.models.link import ( + Link, + PathComponent, + as_path_component, + join_within_directory, +) from pip._internal.network.cache import SafeFileCache, is_from_cache from pip._internal.network.session import CacheControlAdapter, PipSession from pip._internal.network.utils import HEADERS, raise_for_status, response_chunks @@ -116,11 +121,14 @@ def parse_content_disposition(content_di return filename or default_filename -def _get_http_response_filename(resp: Response, link: Link) -> str: +def _get_http_response_filename(resp: Response, link: Link) -> PathComponent: """Get an ideal filename from the given HTTP response, falling back to the link filename if not provided. + + The result is validated as a single path component, so it can be joined onto + a download directory without escaping it. """ - filename = link.filename # fallback + filename: str = link.filename # fallback # Have a look at the Content-Disposition header for a better guess content_disposition = resp.headers.get("content-disposition") if content_disposition: @@ -134,7 +142,7 @@ def _get_http_response_filename(resp: Re ext = os.path.splitext(resp.url)[1] if ext: filename += ext - return filename + return as_path_component(filename) @dataclass @@ -187,7 +195,9 @@ class Downloader: resp = self._http_get(link) download_size = _get_http_response_size(resp) - filepath = os.path.join(location, _get_http_response_filename(resp, link)) + filepath = join_within_directory( + location, _get_http_response_filename(resp, link) + ) with open(filepath, "wb") as content_file: download = _FileDownload(link, content_file, download_size) self._process_response(download, resp) Index: pip-26.1.2/src/pip/_internal/operations/prepare.py =================================================================== --- pip-26.1.2.orig/src/pip/_internal/operations/prepare.py +++ pip-26.1.2/src/pip/_internal/operations/prepare.py @@ -29,7 +29,7 @@ from pip._internal.exceptions import ( from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata import BaseDistribution, get_metadata_distribution from pip._internal.models.direct_url import ArchiveInfo, DirectUrl -from pip._internal.models.link import Link +from pip._internal.models.link import Link, join_within_directory from pip._internal.models.wheel import Wheel from pip._internal.network.download import Downloader from pip._internal.network.lazy_wheel import ( @@ -201,7 +201,7 @@ def _check_download_dir( """Check download_dir for previously downloaded file with correct hash If a correct file is found return its path else None """ - download_path = os.path.join(download_dir, link.filename) + download_path = join_within_directory(download_dir, link.filename) if not os.path.exists(download_path): return None @@ -687,7 +687,7 @@ class RequirementPreparer: # No distribution was downloaded for this requirement. return - download_location = os.path.join(self.download_dir, link.filename) + download_location = join_within_directory(self.download_dir, link.filename) if not os.path.exists(download_location): shutil.copy(req.local_file_path, download_location) download_path = display_path(download_location) Index: pip-26.1.2/tests/unit/test_link.py =================================================================== --- pip-26.1.2.orig/tests/unit/test_link.py +++ pip-26.1.2/tests/unit/test_link.py @@ -1,9 +1,17 @@ from __future__ import annotations +import os +import posixpath + import pytest from pip._internal.exceptions import InvalidEggFragment, PipError -from pip._internal.models.link import Link, links_equivalent +from pip._internal.models.link import ( + Link, + as_path_component, + join_within_directory, + links_equivalent, +) from pip._internal.utils.hashes import Hashes @@ -29,6 +37,13 @@ class TestLink: ("https://example.com/path/page.html", "page.html"), # Test a quoted character. ("https://example.com/path/page%231.html", "page#1.html"), + # A doubly-encoded separator must stay encoded: the path is decoded + # exactly once, so the file name keeps its literal "%2F" instead of + # collapsing into a "/". + ( + "https://example.com/a%252Fb.whl", + "a%2Fb.whl", + ), ( "http://yo/myproject-1.0%2Bfoobar.0-py2.py3-none-any.whl", "myproject-1.0+foobar.0-py2.py3-none-any.whl", @@ -49,6 +64,52 @@ class TestLink: link = Link(url) assert link.filename == expected + @pytest.mark.parametrize( + "url", + [ + "https://example.com/a%252Fb.whl", + "https://example.com/%252e%252e%252fb.whl", + ], + ) + def test_filename_decoded_once_stays_single_component(self, url: str) -> None: + # The path is decoded exactly once, so an encoded separator stays + # encoded and the file name remains a single path component rather + # than collapsing into a "/"-separated path. + filename = Link(url).filename + assert not posixpath.isabs(filename) + assert posixpath.basename(filename) == filename + + @pytest.mark.parametrize( + "url", + [ + "https://example.com/..", + "https://example.com/.", + "https://example.com/foo/%2e%2e", + ], + ) + def test_filename_parent_reference_falls_back_to_netloc(self, url: str) -> None: + # A path that is only a "." or ".." reference has no usable file name, + # so filename falls back to the netloc rather than handing back a + # traversal component that could escape a download directory. + assert Link(url).filename == "example.com" + + @pytest.mark.parametrize( + "url", + [ + # A path-less URL whose authority looks like a traversal: the netloc + # fallback must still reduce to a single path component. + "http://..\\..\\..\\evil.whl", + "http://../", + "http://..", + ], + ) + def test_filename_is_always_a_path_component(self, url: str) -> None: + # filename must never carry a separator or parent reference, so joining + # it onto a directory can never escape that directory. + name = Link(url).filename + assert os.path.basename(name) == name + assert name not in (os.curdir, os.pardir) + def test_splitext(self) -> None: assert ("wheel", ".whl") == Link("http://yo/wheel.whl").splitext() @@ -244,3 +305,53 @@ def test_links_equivalent(url1: str, url ) def test_links_equivalent_false(url1: str, url2: str) -> None: assert not links_equivalent(Link(url1), Link(url2)) + + +@pytest.mark.parametrize( + "name", + [ + "wheel.whl", + "myproject-1.0+foobar.0-py2.py3-none-any.whl", + # A literal "%2F" is a normal file name, not a separator. + "a%2Fb.whl", + ], +) +def test_as_path_component_keeps_plain_name(name: str) -> None: + assert as_path_component(name) == name + + +@pytest.mark.parametrize( + "name", + [ + os.path.join(os.sep, "abs", "pkg.whl"), + os.path.join("..", "pkg.whl"), + os.path.join("nested", "pkg.whl"), + ], +) +def test_as_path_component_reduces_to_basename(name: str) -> None: + # A name carrying directory components is reduced to its basename, so the + # result always stays inside the directory it is later joined onto. + assert as_path_component(name) == os.path.basename(name) + + +@pytest.mark.parametrize("name", ["", ".", "..", "/", os.path.join("sub", "..")]) +def test_as_path_component_rejects_empty_or_parent_reference(name: str) -> None: + with pytest.raises(ValueError): + as_path_component(name) + + +@pytest.mark.parametrize( + "name", + [ + "pkg.whl", + # A literal "%2F" is a normal file name, not a separator. + "a%2Fb.whl", + ], +) +def test_join_within_directory_stays_inside(name: str) -> None: + # The component is joined onto the directory as its final element, so the + # result stays inside the directory. + directory = os.path.join("base", "downloads") + joined = join_within_directory(directory, as_path_component(name)) + assert joined == os.path.join(directory, name) + assert os.path.basename(joined) == name