Validate release version in filename (pypi#15795)

* Use meta.version instead of POST data

* Use parse_wheel_filename and parse_sdist_filename

* Add a test for mismatched filename/meta version

* Linting

* Remove version field from UploadForm

* Add back tests around invalid versions

Author: di

tests/unit/forklift/test_forms.py

@@ -11,26 +11,12 @@
 # limitations under the License.
 
 
-import pretend
 import pytest
 
 from webob.multidict import MultiDict
 from wtforms.validators import ValidationError
 
-from warehouse.forklift.forms import UploadForm, _validate_pep440_version
-
-
-class TestValidation:
-    @pytest.mark.parametrize("version", ["1.0", "30a1", "1!1", "1.0-1", "v1.0"])
-    def test_validates_valid_pep440_version(self, version):
-        form, field = pretend.stub(), pretend.stub(data=version)
-        _validate_pep440_version(form, field)
-
-    @pytest.mark.parametrize("version", ["dog", "1.0.dev.a1"])
-    def test_validates_invalid_pep440_version(self, version):
-        form, field = pretend.stub(), pretend.stub(data=version)
-        with pytest.raises(ValidationError):
-            _validate_pep440_version(form, field)
+from warehouse.forklift.forms import UploadForm
 
 
 class TestUploadForm:

tests/unit/forklift/test_legacy.py

@@ -482,21 +482,40 @@ def test_fails_invalid_version(self, pyramid_config, pyramid_request, version):
             ),
             # version errors.
             (
-                {"metadata_version": "1.2", "name": "example"},
-                "'' is an invalid value for Version. "
-                "Error: This field is required. "
-                "See "
-                "https://packaging.python.org/specifications/core-metadata"
-                " for more information.",
+                {
+                    "metadata_version": "1.2",
+                    "name": "example",
+                    "version": "",
+                    "md5_digest": "bad",
+                    "filetype": "sdist",
+                },
+                "'version' is a required field. See "
+                "https://packaging.python.org/specifications/core-metadata for "
+                "more information.",
             ),
             (
-                {"metadata_version": "1.2", "name": "example", "version": "dog"},
-                "'dog' is an invalid value for Version. "
-                "Error: Start and end with a letter or numeral "
-                "containing only ASCII numeric and '.', '_' and '-'. "
-                "See "
-                "https://packaging.python.org/specifications/core-metadata"
-                " for more information.",
+                {
+                    "metadata_version": "1.2",
+                    "name": "example",
+                    "version": "dog",
+                    "md5_digest": "bad",
+                    "filetype": "sdist",
+                },
+                "'dog' is invalid for 'version'. See "
+                "https://packaging.python.org/specifications/core-metadata for "
+                "more information.",
+            ),
+            (
+                {
+                    "metadata_version": "1.2",
+                    "name": "example",
+                    "version": "1.0.dev.a1",
+                    "md5_digest": "bad",
+                    "filetype": "sdist",
+                },
+                "'1.0.dev.a1' is invalid for 'version'. See "
+                "https://packaging.python.org/specifications/core-metadata for "
+                "more information.",
             ),
             # filetype/pyversion errors.
             (
@@ -2000,11 +2019,18 @@ def test_upload_fails_with_diff_filename_same_blake2(
             ("no-way-{version}.tar.gz", "sdist", "no"),
             ("no_way-{version}-py3-none-any.whl", "bdist_wheel", "no"),
             # multiple delimiters
-            ("foo__bar-{version}-py3-none-any.whl", "bdist_wheel", "foo-.bar"),
+            ("foobar-{version}-py3-none-any.whl", "bdist_wheel", "foo-.bar"),
         ],
     )
-    def test_upload_fails_with_wrong_filename(
-        self, pyramid_config, db_request, metrics, filename, filetype, project_name
+    def test_upload_fails_with_wrong_filename_project_name(
+        self,
+        monkeypatch,
+        pyramid_config,
+        db_request,
+        metrics,
+        filename,
+        filetype,
+        project_name,
     ):
         user = UserFactory.create()
         pyramid_config.testing_securitypolicy(identity=user)
@@ -2020,6 +2046,7 @@ def test_upload_fails_with_wrong_filename(
             IFileStorage: storage_service,
             IMetricsService: metrics,
         }.get(svc)
+        monkeypatch.setattr(legacy, "_is_valid_dist_file", lambda *a, **kw: True)
 
         db_request.POST = MultiDict(
             {
@@ -2055,6 +2082,57 @@ def test_upload_fails_with_wrong_filename(
             )
         )
 
+    @pytest.mark.parametrize(
+        "filename", ["wutang-6.6.6.tar.gz", "wutang-6.6.6-py3-none-any.whl"]
+    )
+    def test_upload_fails_with_wrong_filename_version(
+        self, monkeypatch, pyramid_config, db_request, metrics, filename
+    ):
+        user = UserFactory.create()
+        pyramid_config.testing_securitypolicy(identity=user)
+        db_request.user = user
+        db_request.user_agent = "warehouse-tests/6.6.6"
+        EmailFactory.create(user=user)
+        project = ProjectFactory.create(name="wutang")
+        RoleFactory.create(user=user, project=project)
+
+        storage_service = pretend.stub(store=lambda path, filepath, meta: None)
+        db_request.find_service = lambda svc, name=None, context=None: {
+            IFileStorage: storage_service,
+            IMetricsService: metrics,
+        }.get(svc)
+        monkeypatch.setattr(legacy, "_is_valid_dist_file", lambda *a, **kw: True)
+
+        filetype = "sdist" if filename.endswith(".tar.gz") else "bdist_wheel"
+        db_request.POST = MultiDict(
+            {
+                "metadata_version": "1.2",
+                "name": project.name,
+                "version": "1.2.3",
+                "filetype": filetype,
+                "md5_digest": _TAR_GZ_PKG_MD5,
+                "pyversion": {
+                    "bdist_wheel": "1.0",
+                    "bdist_egg": "1.0",
+                    "sdist": "source",
+                }[filetype],
+                "content": pretend.stub(
+                    filename=filename,
+                    file=io.BytesIO(_TAR_GZ_PKG_TESTDATA),
+                    type="application/tar",
+                ),
+            }
+        )
+        db_request.help_url = lambda **kw: "/the/help/url/"
+
+        with pytest.raises(HTTPBadRequest) as excinfo:
+            legacy.file_upload(db_request)
+
+        resp = excinfo.value
+
+        assert resp.status_code == 400
+        assert resp.status == ("400 Version in filename should be '1.2.3' not '6.6.6'.")
+
     @pytest.mark.parametrize(
         "filetype, extension",
         [

warehouse/forklift/forms.py

@@ -12,10 +12,6 @@
 
 import re
 
-import packaging.requirements
-import packaging.specifiers
-import packaging.utils
-import packaging.version
 import wtforms
 import wtforms.validators
 
@@ -28,17 +24,6 @@
 }
 
 
-def _validate_pep440_version(form, field):
-    # Check that this version is a valid PEP 440 version at all.
-    try:
-        packaging.version.parse(field.data)
-    except packaging.version.InvalidVersion:
-        raise wtforms.validators.ValidationError(
-            "Start and end with a letter or numeral containing only "
-            "ASCII numeric and '.', '_' and '-'."
-        )
-
-
 # NOTE: This form validation runs prior to ensuring that the current identity
 #       is authorized to upload for the given project, so it should not validate
 #       against anything other than what the user themselves have provided.
@@ -47,8 +32,8 @@ def _validate_pep440_version(form, field):
 #       occur elsewhere so that they can happen after we've authorized the request
 #       to upload for the given project.
 class UploadForm(forms.Form):
-    # The name and version fields are duplicated out of the general metadata handling,
-    # to be part of the upload form as well so that we can use them prior to extracting
+    # The name field is duplicated out of the general metadata handling, to be
+    # part of the upload form as well so that we can use it prior to extracting
     # the metadata from the uploaded artifact.
     #
     # NOTE: We don't need to fully validate these values here, as we will be validating
@@ -68,17 +53,6 @@ class UploadForm(forms.Form):
             ),
         ],
     )
-    version = wtforms.StringField(
-        description="Version",
-        validators=[
-            wtforms.validators.InputRequired(),
-            wtforms.validators.Regexp(
-                r"^(?!\s).*(?<!\s)$",
-                message="Can't have leading or trailing whitespace.",
-            ),
-            _validate_pep440_version,
-        ],
-    )
 
     # File metadata
     pyversion = wtforms.StringField(validators=[wtforms.validators.Optional()])

warehouse/forklift/legacy.py

@@ -693,7 +693,7 @@ def file_upload(request):
         release = (
             request.db.query(Release)
             .filter(
-                (Release.project == project) & (Release.version == form.version.data)
+                (Release.project == project) & (Release.version == str(meta.version))
             )
             .one()
         )
@@ -831,44 +831,6 @@ def file_upload(request):
     # Ensure the filename doesn't contain any characters that are too 🌶️spicy🥵
     _validate_filename(filename, filetype=form.filetype.data)
 
-    # Extract the project name from the filename and normalize it.
-    filename_prefix = (
-        # For wheels, the project name is normalized and won't contain hyphens, so
-        # we can split on the first hyphen.
-        filename.partition("-")[0]
-        if filename.endswith(".whl")
-        # For source releases, the version might contain a hyphen as a
-        # post-release separator, so we get the prefix by removing the provided
-        # version.
-        # Per 625, the version should be normalized, but we aren't currently
-        # enforcing this, so we permit a filename with either the exact
-        # provided version if it contains a hyphen, or any version that doesn't
-        # contain a hyphen.
-        else (
-            # A hyphen is being used for a post-release separator, so partition
-            # the prefix twice
-            filename.rpartition("-")[0].rpartition("-")[0]
-            # Check if the provided version contains a hyphen and the same
-            # version is being used in the filename
-            if "-" in form.version.data
-            and filename.endswith(f"-{form.version.data}.tar.gz")
-            # The only hyphen should be between the prefix and the version, so
-            # we only need to partition the prefix once
-            else filename.rpartition("-")[0]
-        )
-    )
-
-    # Normalize the prefix in the filename. Eventually this should be unnecessary once
-    # we become more restrictive in what we permit
-    filename_prefix = filename_prefix.lower().replace(".", "_").replace("-", "_")
-
-    # Make sure that our filename matches the project that it is being uploaded to.
-    if (prefix := project.normalized_name.replace("-", "_")) != filename_prefix:
-        raise _exc_with_message(
-            HTTPBadRequest,
-            f"Start filename for {project.name!r} with {prefix!r}.",
-        )
-
     # Check the content type of what is being uploaded
     if not request.POST["content"].type or request.POST["content"].type.startswith(
         "image/"
@@ -994,10 +956,57 @@ def file_upload(request):
         if not _is_valid_dist_file(temporary_filename, form.filetype.data):
             raise _exc_with_message(HTTPBadRequest, "Invalid distribution file.")
 
+        # Check that the sdist filename is correct
+        if filename.endswith(".tar.gz"):
+            # Extract the project name and version from the filename and check it.
+            # Per PEP 625, both should be normalized, but we aren't currently
+            # enforcing this, so we permit a filename with a project name and
+            # version that normalizes to be what we expect
+
+            name, version = packaging.utils.parse_sdist_filename(filename)
+
+            # The previous function fails to accomodate the edge case where
+            # versions may contain hyphens, so we handle that here based on
+            # what we were expecting
+            if (
+                meta.version.is_postrelease
+                and packaging.utils.canonicalize_name(name) != meta.name
+            ):
+                # The distribution is a source distribution, the version is a
+                # postrelease, and the project name doesn't match, so
+                # there may be a hyphen in the version. Split the filename on the
+                # second to last hyphen instead.
+                name = filename.rpartition("-")[0].rpartition("-")[0]
+                version = packaging.version.Version(
+                    filename[len(name) + 1 : -len(".tar.gz")]
+                )
+
+            # Normalize the prefix in the filename. Eventually this should be
+            # unnecessary once we become more restrictive in what we permit
+            filename_prefix = name.lower().replace(".", "_").replace("-", "_")
+
+            # Make sure that our filename matches the project that it is being
+            # uploaded to.
+            if (prefix := project.normalized_name.replace("-", "_")) != filename_prefix:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Start filename for {project.name!r} with {prefix!r}.",
+                )
+
+            # Make sure that the version in the filename matches the metadata
+            if version != meta.version:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Version in filename should be {str(meta.version)!r} not "
+                    f"{str(version)!r}.",
+                )
+
         # Check that if it's a binary wheel, it's on a supported platform
         if filename.endswith(".whl"):
             try:
-                _, __, ___, tags = packaging.utils.parse_wheel_filename(filename)
+                name, version, ___, tags = packaging.utils.parse_wheel_filename(
+                    filename
+                )
             except packaging.utils.InvalidWheelFilename as e:
                 raise _exc_with_message(
                     HTTPBadRequest,
@@ -1012,6 +1021,20 @@ def file_upload(request):
                         f"platform tag '{tag.platform}'.",
                     )
 
+            if (canonical_name := packaging.utils.canonicalize_name(meta.name)) != name:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Start filename for {project.name!r} with "
+                    f"{canonical_name.replace('-', '_')!r}.",
+                )
+
+            if meta.version != version:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Version in filename should be {str(meta.version)!r} not "
+                    f"{str(version)!r}.",
+                )
+
             """
             Extract METADATA file from a wheel and return it as a content.
             The name of the .whl file is used to find the corresponding .dist-info dir.