From db9f12110faa188cb090cb9b8ed09d3ee09e04ae Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Wed, 22 Apr 2026 17:53:20 -0400 Subject: [PATCH 01/12] wip --- tests/unit/forklift/test_decorators.py | 69 +- .../{test_utils.py => test_errors.py} | 10 +- tests/unit/forklift/test_legacy.py | 230 ++---- warehouse/forklift/decorators.py | 132 +++- warehouse/forklift/errors.py | 197 +++++ warehouse/forklift/legacy.py | 698 +++++++++--------- warehouse/forklift/utils.py | 18 - 7 files changed, 736 insertions(+), 618 deletions(-) rename tests/unit/forklift/{test_utils.py => test_errors.py} (79%) create mode 100644 warehouse/forklift/errors.py delete mode 100644 warehouse/forklift/utils.py diff --git a/tests/unit/forklift/test_decorators.py b/tests/unit/forklift/test_decorators.py index 3d696c52cd9f..2fcc67d45f78 100644 --- a/tests/unit/forklift/test_decorators.py +++ b/tests/unit/forklift/test_decorators.py @@ -5,7 +5,6 @@ import pretend import pytest -from pyramid.httpexceptions import HTTPBadRequest, HTTPForbidden from pyramid.testing import DummyRequest from webob.multidict import MultiDict @@ -21,6 +20,9 @@ def test_removes_unknowns(self): "foo": "UNKNOWN", "bar": " UNKNOWN ", "real": "value", + "content": cgi.FieldStorage( + headers={"content-type": "application/octet-stream"}, + ), } ) ) @@ -28,13 +30,24 @@ def test_removes_unknowns(self): @decorators.sanitize def wrapped(context, request): + # Remove the content field as it doesn't compare correctly. + del request.POST["content"] assert MultiDict({"real": "value"}) == request.POST return resp assert wrapped(pretend.stub(), req) is resp def test_escapes_nul_characters(self): - req = DummyRequest(post=MultiDict({"summary": "I want to go to the \x00"})) + req = DummyRequest( + post=MultiDict( + { + "summary": "I want to go to the \x00", + "content": cgi.FieldStorage( + headers={"content-type": "application/octet-stream"}, + ), + } + ) + ) resp = pretend.stub() @decorators.sanitize @@ -46,18 +59,26 @@ def wrapped(context, request): def test_fails_with_fieldstorage(self): req = DummyRequest(post=MultiDict({"keywords": cgi.FieldStorage()})) - req.metrics = pretend.stub(increment=lambda key, tags: None) @decorators.sanitize def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(decorators.InvalidTupleField) as excinfo: wrapped(pretend.stub(), req) - resp = excinfo.value - assert resp.status_code == 400 - assert resp.status == "400 keywords: Should not be a tuple." + assert excinfo.value.values == {"field": "keywords"} + + @pytest.mark.parametrize("content_type", [None, "image/foobar"]) + def test_fails_invalid_content_type(self, content_type): + req = DummyRequest(post=MultiDict({"content": pretend.stub(type=content_type)})) + + @decorators.sanitize + def wrapped(context, request): + pytest.fail("wrapped view should not have been called") + + with pytest.raises(decorators.InvalidContentType): + wrapped(pretend.stub(), req) class TestEnsureUploadsAllowed: @@ -90,58 +111,36 @@ def wrapped(context, request): assert wrapped(pretend.stub(), req) is resp @pytest.mark.parametrize( - ("flag", "error", "help_url"), + ("flag", "error_type"), [ ( AdminFlagValue.READ_ONLY, - "Read-only mode: Uploads are temporarily disabled.", - "", + decorators.ReadOnlyEnabled, ), ( AdminFlagValue.DISALLOW_NEW_UPLOAD, - ( - "New uploads are temporarily disabled. " - "See /help/url/ for more information." - ), - "/help/url/", + decorators.UploadsDisabled, ), ], ) - def test_disallowed_with_admin_flags(self, flag, error, help_url): + def test_disallowed_with_admin_flags(self, flag, error_type): req = DummyRequest() req.flags = pretend.stub(enabled=lambda f: f is flag) - req.help_url = lambda *a, **k: help_url - req.metrics = pretend.stub(increment=lambda key, tags: None) @decorators.ensure_uploads_allowed def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(HTTPForbidden) as excinfo: + with pytest.raises(error_type): wrapped(pretend.stub(), req) - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == f"403 {error}" - def test_fails_without_identity(self): req = DummyRequest() req.flags = pretend.stub(enabled=lambda f: False) - req.help_url = lambda *a, **k: "/path/to/help/" - req.metrics = pretend.stub(increment=lambda key, tags: None) @decorators.ensure_uploads_allowed def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(HTTPForbidden) as excinfo: + with pytest.raises(decorators.MissingIdentity): wrapped(pretend.stub(), req) - - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == ( - "403 Invalid or non-existent authentication information. " - "See /path/to/help/ for more information." - ) diff --git a/tests/unit/forklift/test_utils.py b/tests/unit/forklift/test_errors.py similarity index 79% rename from tests/unit/forklift/test_utils.py rename to tests/unit/forklift/test_errors.py index e9619be8f236..565d3b4fa3f3 100644 --- a/tests/unit/forklift/test_utils.py +++ b/tests/unit/forklift/test_errors.py @@ -4,18 +4,18 @@ from pyramid.httpexceptions import HTTPBadRequest -from warehouse.forklift import utils +from warehouse.forklift import errors class TestExcWithMessage: def test_exc_with_message(self): - exc = utils._exc_with_message(HTTPBadRequest, "My Test Message.") + exc = errors._exc_with_message(HTTPBadRequest, "My Test Message.") assert isinstance(exc, HTTPBadRequest) assert exc.status_code == 400 assert exc.status == "400 My Test Message." def test_exc_with_exotic_message(self): - exc = utils._exc_with_message( + exc = errors._exc_with_message( HTTPBadRequest, "look at these wild chars: аä‗" ) assert isinstance(exc, HTTPBadRequest) @@ -26,8 +26,8 @@ def test_exc_with_missing_message(self, monkeypatch): sentry_sdk = pretend.stub( capture_message=pretend.call_recorder(lambda message: None) ) - monkeypatch.setattr(utils, "sentry_sdk", sentry_sdk) - exc = utils._exc_with_message(HTTPBadRequest, "") + monkeypatch.setattr(errors, "sentry_sdk", sentry_sdk) + exc = errors._exc_with_message(HTTPBadRequest, "") assert isinstance(exc, HTTPBadRequest) assert exc.status_code == 400 assert exc.status == "400 Bad Request" diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 4645ce7609c1..652458dae0a7 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -648,22 +648,10 @@ def test_is_duplicate_false(self, pyramid_config, db_request): class TestFileUpload: @pytest.mark.parametrize("version", ["2", "3", "-1", "0", "dog", "cat"]) - def test_fails_invalid_version(self, pyramid_config, pyramid_request, version): - pyramid_request.POST["protocol_version"] = version - pyramid_request.flags = pretend.stub(enabled=lambda *a: False) - pyramid_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - - user = UserFactory.create(with_verified_primary_email=True) - pyramid_config.testing_securitypolicy(identity=user) - pyramid_request.user = user - - with pytest.raises(HTTPBadRequest) as excinfo: - legacy.file_upload(pyramid_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == "400 Unknown protocol version." + def test_fails_invalid_version(self, version): + with pytest.raises(legacy.InvalidProtocolVersion) as excinfo: + legacy.file_upload(pretend.stub(POST={"protocol_version": version})) + assert excinfo.value.values == {"version": version} @pytest.mark.parametrize( ("post_data", "message"), @@ -963,22 +951,8 @@ def test_fails_with_ultranormalized_names( ) @pytest.mark.parametrize( - ("description_content_type", "description", "message"), - [ - ( - "text/x-rst", - ".. invalid-directive::", - "400 The description failed to render for 'text/x-rst'. " - "See /the/help/url/ for more information.", - ), - ( - None, - ".. invalid-directive::", - "400 The description failed to render in the default format " - "of reStructuredText. " - "See /the/help/url/ for more information.", - ), - ], + ("description_content_type", "description"), + [("text/x-rst", ".. invalid-directive::"), (None, ".. invalid-directive::")], ) def test_fails_invalid_render( self, pyramid_config, db_request, description_content_type, description, message @@ -1007,19 +981,12 @@ def test_fails_invalid_render( if description_content_type is not None: db_request.POST.add("description_content_type", description_content_type) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidDescription) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == message - - assert db_request.help_url.calls == [ - pretend.call(_anchor="description-content-type") - ] + excinfo.value.values == { + "content_type": description_content_type or "text/x-rst" + } @pytest.mark.parametrize( "name", @@ -1309,51 +1276,6 @@ def storage_service_store(path, file_path, *, meta): pretend.call("warehouse.upload.ok", tags=["filetype:sdist"]), ] - @pytest.mark.parametrize("content_type", [None, "image/foobar"]) - def test_upload_fails_invalid_content_type( - self, tmpdir, monkeypatch, pyramid_config, db_request, content_type - ): - monkeypatch.setattr(tempfile, "tempdir", str(tmpdir)) - - user = UserFactory.create() - EmailFactory.create(user=user) - pyramid_config.testing_securitypolicy(identity=user) - db_request.user = user - project = ProjectFactory.create() - release = ReleaseFactory.create(project=project, version="1.0") - RoleFactory.create(user=user, project=project) - - db_request.db.add(Classifier(classifier="Environment :: Other Environment")) - - filename = "{}-{}.tar.gz".format( - project.normalized_name.replace("-", "_"), release.version - ) - - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": project.name, - "version": release.version, - "filetype": "sdist", - "pyversion": "source", - "md5_digest": _TAR_GZ_PKG_MD5, - "content": pretend.stub( - filename=filename, - file=io.BytesIO(_TAR_GZ_PKG_TESTDATA), - type=content_type, - ), - } - ) - db_request.POST.extend([("classifiers", "Environment :: Other Environment")]) - - with pytest.raises(HTTPBadRequest) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == "400 Invalid distribution file." - def test_upload_fails_with_legacy_type(self, pyramid_config, db_request): user = UserFactory.create() EmailFactory.create(user=user) @@ -1467,14 +1389,9 @@ def test_upload_fails_for_second_sdist(self, pyramid_config, db_request): } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.DuplicateSDist): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == "400 Only one sdist may be uploaded per release." - def test_upload_fails_with_invalid_classifier(self, pyramid_config, db_request): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -1676,13 +1593,12 @@ def test_upload_fails_with_invalid_file(self, pyramid_config, db_request): } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidDistFile) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ("400 Invalid distribution file. File is not a zipfile") + assert excinfo.value.values == { + "msg": "File is not a zipfile", + "filetype": "sdist", + } def test_upload_fails_end_of_file_error( self, pyramid_config, db_request, project_service @@ -1721,13 +1637,10 @@ def test_upload_fails_end_of_file_error( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidDistFile) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ("400 Invalid distribution file. File is not a tarfile") + excinfo.value.values == {"msg": "File is not a tarfile"} def test_upload_fails_with_too_large_file(self, pyramid_config, db_request): user = UserFactory.create() @@ -2206,19 +2119,11 @@ def test_upload_fails_with_existing_filename_diff_content( path=f"source/{project.name[0]}/{project.name}/{filename}", ) ) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - with pytest.raises(HTTPBadRequest) as excinfo: + + with pytest.raises(legacy.FileAlreadyExists) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert db_request.help_url.calls == [pretend.call(_anchor="file-name-reuse")] - assert resp.status_code == 400 - assert resp.status == ( - f"400 File already exists ({filename!r}, " - f"with blake2_256 hash {blake2_256_digest!r}). " - "See /the/help/url/ for more information." - ) + excinfo.value.values == {"filename": filename, "blake2_256": blake2_256_digest} def test_upload_fails_with_diff_filename_same_blake2( self, pyramid_config, db_request @@ -2340,37 +2245,24 @@ def test_upload_fails_with_wrong_filename_project_name( ), } ) - db_request.help_url = lambda **kw: "/the/help/url/" - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnnormalizedFilename) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Start filename for {!r} with {!r}.".format( - project.name, - project.normalized_name.replace("-", "_"), - ) - ) + assert excinfo.value.values == { + "project": project.name, + "normalized": project.normalized_name.replace("-", "_"), + "filetype": filetype, + } @pytest.mark.parametrize( - ("filename", "status"), + ("filename", "exc_type", "expected"), [ - ( - "wutang-6.6.6.tar.gz", - "400 Filename 'wutang-6.6.6.tar.gz' is invalid, should be " - "'wutang-1.2.3.tar.gz'.", - ), - ( - "wutang-6.6.6-py3-none-any.whl", - "400 Version in filename should be '1.2.3' not '6.6.6'.", - ), + ("wutang-6.6.6.tar.gz", legacy.UnnormalizedFilename, {}), + ("wutang-6.6.6-py3-none-any.whl", legacy.InvalidFilenameVersion, {}), ], ) def test_upload_fails_with_wrong_filename_version( - self, monkeypatch, pyramid_config, db_request, filename, status + self, monkeypatch, pyramid_config, db_request, filename, exc_type, expected ): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -2408,15 +2300,10 @@ def test_upload_fails_with_wrong_filename_version( ), } ) - db_request.help_url = lambda **kw: "/the/help/url/" - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(exc_type) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == status + assert excinfo.value.values == expected @pytest.mark.parametrize( ("filetype", "extension"), @@ -2501,18 +2388,9 @@ def test_upload_fails_with_invalid_extension(self, pyramid_config, db_request): } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnknownFileExtension): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Invalid file extension: Use .tar.gz, .whl or .zip " - "extension. See https://www.python.org/dev/peps/pep-0527 " - "and https://peps.python.org/pep-0715/ for more information" - ) - @pytest.mark.parametrize("character", ["/", "\\"]) def test_upload_fails_with_unsafe_filename( self, pyramid_config, db_request, character @@ -2583,17 +2461,9 @@ def test_upload_fails_with_disallowed_in_filename( } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnprintableFilename): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Cannot upload a file with non-printable characters (ordinals 0-31) " - "or the DEL character (ordinal 127) in the name." - ) - def test_upload_fails_without_user_permission(self, pyramid_config, db_request): user1 = UserFactory.create() EmailFactory.create(user=user1) @@ -3142,7 +3012,6 @@ def storage_service_store(path, file_path, *, meta): ] assert db_request.metrics.increment.calls == [ - pretend.call("warehouse.upload.attempt"), pretend.call("warehouse.upload.ok", tags=["filetype:bdist_wheel"]), ] @@ -3601,15 +3470,10 @@ def test_upload_fails_with_unsupported_wheel_plat( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidPlatformTag) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert re.match( - "400 Binary wheel .* has an unsupported platform tag .*", resp.status - ) + assert excinfo.value.values == {"filename": filename, "tag": plat} def test_upload_fails_with_missing_record_wheel( self, monkeypatch, pyramid_config, db_request @@ -3652,15 +3516,14 @@ def test_upload_fails_with_missing_record_wheel( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.MissingRecordFile) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert re.match( - "400 Wheel .* does not contain the required RECORD file: .*", resp.status - ) + assert excinfo.value.values == { + "filename": filename, + "record_filename": f"{project.normalized_name.replace("-", "_")}-1.0.dist-info/RECORD", + "filetype": "bdist_wheel", + } def test_upload_warns_with_mismatched_wheel_and_zip_contents( self, monkeypatch, pyramid_config, db_request @@ -4479,14 +4342,9 @@ def test_upload_fails_attestation_error( ) monkeypatch.setattr(HasEvents, "record_event", record_event) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidAttestations) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status.startswith("400 Invalid attestations") - @pytest.mark.parametrize( ("url", "expected"), [ diff --git a/warehouse/forklift/decorators.py b/warehouse/forklift/decorators.py index c410d531b905..89870b1fb36d 100644 --- a/warehouse/forklift/decorators.py +++ b/warehouse/forklift/decorators.py @@ -2,10 +2,91 @@ import cgi -from pyramid.httpexceptions import HTTPBadRequest, HTTPForbidden +from pyramid.httpexceptions import HTTPForbidden from warehouse.admin.flags import AdminFlagValue -from warehouse.forklift.utils import _exc_with_message +from warehouse.forklift.errors import ForkliftError + + +class InvalidTupleField( + ForkliftError, + message="{field}: Should not be a tuple.", + tags={"reason:field-is-tuple", "field:{field}"}, +): + pass + + +class NoFileUpload( + ForkliftError, + message="Upload payload does not have a file.", + tags={"reason:no-file"}, +): + pass + + +class InvalidContentType( + ForkliftError, + message="Invalid distribution file.", + tags={"reason:invalid-content-type"}, +): + pass + + +class ReadOnlyEnabled( + ForkliftError, + error_type=HTTPForbidden, + message="Read-only mode: Uploads are temporarily disabled.", + tags={"reason:read-only"}, +): + pass + + +class UploadsDisabled( + ForkliftError, + error_type=HTTPForbidden, + message="New uploads are temporarily disabled.", + help_anchor="admin-intervention", + tags={"reason:uploads-disabled"}, +): + pass + + +class MissingIdentity( + ForkliftError, + error_type=HTTPForbidden, + message="Invalid or non-existent authentication information.", + help_anchor="invalid-auth", + tags={"reason:no-identity"}, +): + pass + + +class UnverifiedEmail( + ForkliftError, + error_type=HTTPForbidden, + message=( + "User {username!r} does not have a verified primary email address. " + "Please add a verified primary email before attempting to " + "upload to PyPI." + ), + help_anchor="verified-email", + tags={"reason:unverified-email"}, +): + pass + + +class MissingTwoFactor( + ForkliftError, + error_type=HTTPForbidden, + message=( + "User {username!r} does not have two-factor authentication enabled. " + "Please enable two-factor authentication before attempting to " + "upload to PyPI." + ), + help_anchor="two-factor-authentication", + tags={"reason:no-2fa"}, +): + pass def sanitize(wrapped): @@ -42,13 +123,17 @@ def wrapper(context, request): for field in set(request.POST) - {"content", "gpg_signature"}: values = request.POST.getall(field) if any(isinstance(value, cgi.FieldStorage) for value in values): - request.metrics.increment( - "warehouse.upload.failed", - tags=["reason:field-is-tuple", f"field:{field}"], - ) - raise _exc_with_message( - HTTPBadRequest, f"{field}: Should not be a tuple." - ) + raise InvalidTupleField(field=field) + + # Ensure that we have file data in the request. + if "content" not in request.POST: + raise NoFileUpload + + # Check the content type of what is being uploaded + if not request.POST["content"].type or request.POST["content"].type.startswith( + "image/" + ): + raise InvalidContentType # Otherwise, we'll just dispatch to our underlying view return wrapped(context, request) @@ -66,40 +151,17 @@ def wrapper(context, request): # The very first thing we want to check, is whether we're currently in # read only mode, because if we're in read only mode nothing else matters. if request.flags.enabled(AdminFlagValue.READ_ONLY): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:read-only"] - ) - raise _exc_with_message( - HTTPForbidden, "Read-only mode: Uploads are temporarily disabled." - ) + raise ReadOnlyEnabled # After that, we want to check if we're disallowing new uploads, which is # functionally the same as read only mode, but only for the upload endpoint. if request.flags.enabled(AdminFlagValue.DISALLOW_NEW_UPLOAD): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:uploads-disabled"] - ) - raise _exc_with_message( - HTTPForbidden, - "New uploads are temporarily disabled. " - "See {projecthelp} for more information.".format( - projecthelp=request.help_url(_anchor="admin-intervention") - ), - ) + raise UploadsDisabled # Before we do anything else, if there isn't an authenticated identity with # this request, then we'll go ahead and bomb out. if request.identity is None: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:no-identity"] - ) - raise _exc_with_message( - HTTPForbidden, - "Invalid or non-existent authentication information. " - "See {projecthelp} for more information.".format( - projecthelp=request.help_url(_anchor="invalid-auth") - ), - ) + raise MissingIdentity # Otherwise, we'll just dispatch to our underlying view return wrapped(context, request) diff --git a/warehouse/forklift/errors.py b/warehouse/forklift/errors.py new file mode 100644 index 000000000000..8cdb289100d3 --- /dev/null +++ b/warehouse/forklift/errors.py @@ -0,0 +1,197 @@ +# SPDX-License-Identifier: Apache-2.0 + +from collections.abc import Iterable +from typing import Any + +import sentry_sdk + +from pyramid.httpexceptions import HTTPBadRequest, HTTPException + + +# We don't subclass from a HTTP Exception here, because pyramid will automatically +# turn those into a response for us, and we don't want that to happen "silently" +# for these exceptions. +# +# In other words, ForkliftExceptions are only intended to be raised from within +# forklift, and forklift should always have the decorator applied that does the +# translation to what Pyramid expects, so these should never reach Pyramid itself, +# and if they do, then something has gone wrong and we should be alerted to that +# fact. +# +# All of our ForkliftExceptions have certain values that are set at the class +# level (like a message, tags to emit in the metrics, etc) and accept arbitrary +# keyword arguments which will be formatted into those values at runtime. +# +# This means we can have a message (or a tag, or whatever) that says something +# like `message = "this is an error with {foo}` and you can pass in foo as a +# keyword argument to the ForkliftException. +class ForkliftError(Exception): + message: str + tags: set[str] + values: dict[str, Any] + + _message_template: str + _tag_templates: set[str] + _http_exception_type: HTTPException + _help_anchor: str | None + _help_url: str | None + _user_docs_path: str | None + _user_docs_anchor: str | None + + # We use an __init_subclass__ hook to make it impossible for a subclass to + # forget to specify either a message or a set of tags, which they could do + # if we just used "normal" class variables. + # + # This also means that our class variables that we use to smuggle these values + # into the __init__ and as_http_exception methods don't need nice human readable + # names, because they're just part of the internal details of this class. + def __init_subclass__( + cls, + *, + message: str, + tags: Iterable[str], + error_type=HTTPBadRequest, + help_anchor=None, + user_docs_path=None, + user_docs_anchor=None, + help_url=None, + **kwargs, + ): + # This is the template that we'll use to render the base "message" of + # the error, which will get treated as if the user did Foo(message), but + # with the string template expanded. + cls._message_template = message + + # This is the set of tags that we'll use when emitting metrics, again + # this will end up expanded with the kwargs. + cls._tag_templates = set(tags) + + # This is the HttpException that this exception will actually raise when + # it gets translated from a ForkliftException to a HttpException. + cls._http_exception_type = error_type + + # If there is a help anchor (which is pointing to the standad help url) + # associated with this, then when we translate this error we'll also + # automatically include a link to the help url in the message as well. + cls._help_anchor = help_anchor + + # We also have user docs, and we point some of our errors to that. + cls._user_docs_path = user_docs_path + cls._user_docs_anchor = user_docs_anchor + + # If there is a help url, then we assume it is an already resolved url + # that we can include in the response to direct the user towards help. + cls._help_url = help_url + + # Let the normal behavior finish initializing the subclass. + super().__init_subclass__(**kwargs) + + def __init__(self, *args, **kwargs): + # We're going to generate a "real" message by using the kwards to format + # against the message template stored on the class, and then pass that + # into the parent class as if it was called like Foo("result of template"). + self.message = self._message_template.format(**kwargs) + + # Like message, our tags are also formatted by the kwargs, but we're just + # going to store them ourselves because our parent class doesn't have any + # concept of tags that we'd want to mimic. + self.tags = {t.format(**kwargs) for t in self._tag_templates} + + # Store our values that were passed in here so they're easy to introspect + # later. + self.values = kwargs + + # We do this last, so that all of our pre-formatting has already taken + # into effect before this happens. + super().__init__(self.message, *args) + + def as_http_exception(self, request): + # TODO: Is this something we still need to worry about? + if not self.message: + sentry_sdk.capture_message( + "Attempting to _exc_with_message without a message" + ) + + # Append the standard "see X for more info" message to our message if there + # a help url associated with this error. + # TODO: Handle _help_url, and multiples and _user_docs_anchor, _user_docs_path + message = self.message + if self._help_anchor is not None: + message += " See {help_url} for more information.".format( + help_url=request.help_url(_anchor=self._help_anchor) + ) + + # Actually construct the "real" http error, using our now finalized message. + return _exc_with_message(self._http_exception_type, message) + + +# This is kept as a stand alone function to support the handful of non-upload endpoints +# in Forklift, at least until we have a better pattern for handling metrics that would +# let us re-use the translate_error machinery without emitting spurious metrics. +def _exc_with_message(exc, message, **kwargs): + # TODO: Is this something we still need to worry about? + if not message: + sentry_sdk.capture_message("Attempting to _exc_with_message without a message") + + # The crappy old API that PyPI offered uses the status to pass down + # messages to the client, so we'll construct a normal http exception, + # but then set the "status" to our error message as well. + resp = exc(detail=message, **kwargs) + # We need to guard against characters outside of iso-8859-1 per RFC. + # Specifically here, where user-supplied text may appear in the message, + # which our WSGI server may not appropriately handle (indeed gunicorn does not). + status_message = message.encode("iso-8859-1", "replace").decode("iso-8859-1") + resp.status = f"{resp.status_code} {status_message}" + return resp + + +def translate_errors(wrapped): + """ + Wraps a Pyramid view to translate errors into the format expected by the + "legacy" upload API. + + There's a general pattern that gets used wherever we have errors in the + legacy API, and while each individual location doesn't cause tons of lines + of code, collectively this ends up adding a lot of noise to that view + function. + + Essentially this decorator knows how to turn specific exceptions into the + correct HTTP responses, as well as centralizes things like emitting metrics, + etc. + """ + + def wrapper(context, request): + # Log an attempt to upload + # + # NOTE: This technically isn't part of "translating errors", but we + # emit metrics for failure in this decorator, so it makes the most + # sense to keep this here so it's co-located with that. + request.metrics.increment("warehouse.upload.attempt") + + # We're just going to call into our wrapped view and see if we get any + # errors out of it. If we do, and they're one of the kinds of errors + # that we know how to deal with, then we'll catch it and do our translation + # into the error response that the legacy upload API expects. + try: + return wrapped(context, request) + except ForkliftError as exc: + # All errors in the upload API should emit metrics, so we'll go + # ahead and do that first. + request.metrics.increment("warehouse.upload.failed", tags=exc.tags) + + # Use the registered http exception type to raise a new error, rendered + # with the mechanisms that the legacy upload API expects. + # + # We'll use `from exc` so that our traceback properly records the + # original source of the ForkliftException that caused this. + raise exc.as_http_exception(request) from exc + except Exception: + # If we get any other kind of exception, then we'll mark a failed + # upload as well before re-raising that exception to be handled up + # the stack. + request.metrics.increment( + "warehouse.upload.failed", tags=["reason:unhandled-error"] + ) + raise + + return wrapper diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index fad63b3e0d9c..e811adad6696 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -44,8 +44,8 @@ from warehouse.events.tags import EventTag from warehouse.forklift import metadata from warehouse.forklift.decorators import ensure_uploads_allowed, sanitize +from warehouse.forklift.errors import ForkliftError, translate_errors, _exc_with_message from warehouse.forklift.forms import UploadForm, _filetype_extension_mapping -from warehouse.forklift.utils import _exc_with_message from warehouse.macaroons.models import Macaroon from warehouse.packaging.interfaces import IFileStorage, IProjectService from warehouse.packaging.metadata_verification import verify_email, verify_url @@ -87,6 +87,289 @@ # existing descriptions. MAX_DESCRIPTION_LENGTH_TO_BIGQUERY_IN_BYTES = 40000 + +class InvalidProtocolVersion( + ForkliftError, + message="Unknown protocol version: {version!r}", + tags={"reason:unsupported-protocol-version"}, +): + pass + + +class NonUserIdentity( + ForkliftError, + error_type=HTTPForbidden, + message=( + "Non-user identities cannot create new projects. " + "This was probably caused by successfully using a pending " + "publisher but specifying the project name incorrectly " + "(either in the publisher or in your project's metadata). " + "Please ensure that both match." + ), + help_url="https://docs.pypi.org/trusted-publishers/troubleshooting/", + tags={"reason:non-user-identity"}, +): + pass + + +class UserPermissionDenied( + ForkliftError, + error_type=HTTPForbidden, + message="The user {username!r} isn't allowed to upload to project {project!r}.", + help_anchor="project-name", + tags={"reason:permission-denied"}, +): + pass + + +class TokenPermissionDenied( + ForkliftError, + error_type=HTTPForbidden, + message="The given token isn't allowed to upload to project {project!r}.", + help_anchor="project-name", + tags={"reason:permission-denied"}, +): + pass + + +class PermissionDenied( + ForkliftError, + error_type=HTTPForbidden, + # NOTE: This looks wonky, but it's done so the permissions + # system can pass us a message itself that we'll use. + message="{msg}", + tags={"reason:permission-denied"}, +): + pass + + +class OrgInactive( + ForkliftError, + error_type=HTTPForbidden, + message=( + "Organization account owning this project is inactive. " + "This may be due to inactive billing for Company " + "Organizations, or administrator intervention for Community " + "Organizations. Please contact support+orgs@pypi.org." + ), + tags={"reason:org-not-active"}, +): + pass + + +class InvalidDescription( + ForkliftError, + message="The description failed to render as {content_type!r}.", + help_anchor="description-content-type", + tags={"reason:invalid-description"}, +): + pass + + +class UnprintableFilename( + ForkliftError, + message=( + "Cannot upload a file with non-printable " + "characters (ordinals 0-31) or the DEL character " + "(ordinal 127) in the name." + ), + tags={"reason:invalid-filename"}, +): + pass + + +class PathlikeFilename( + ForkliftError, + message="Cannot upload a file with '/' or '\\' in the name.", + tags={"reason:invalid-filename"}, +): + pass + + +class InvalidFileExtension( + ForkliftError, + message=( + "Invalid file extension: Extension {extension} is " + "invalid for filetype {filetype}. See " + "https://www.python.org/dev/peps/pep-0527 for more " + "information." + ), + tags={"reason:invalid-filename"}, +): + pass + + +class UnknownFileExtension( + ForkliftError, + message=( + "Invalid file extension: Use .tar.gz, .whl or .zip " + "extension. See " + "https://www.python.org/dev/peps/pep-0527 and " + "https://peps.python.org/pep-0715/ for more " + "information." + ), + tags={"reason:invalid-filename"}, +): + pass + + +class InvalidFilename( + ForkliftError, + message="Invalid filename: {filename}", + tags={"reason:invalid-filename", "filetype:{filetype}"}, +): + pass + + +class InvalidWheelFilename( + ForkliftError, + message="Invalid filename {filename!r}: {msg}", + tags={"reason:invalid-filename", "filetype:bdist_wheel"}, +): + pass + + +class UnnormalizedFilename( + ForkliftError, + message="Filename for {project!r} should contain the normalized project name {normalized!r}.", + tags={"reason:invalid-filename-normalized", "filetype:{filetype}"}, +): + pass + + +class InvalidFilenameVersion( + ForkliftError, + message="Version in filename should be {expected!r} not {found!r}.", + tags={"reason:invalid-filename-version", "filetype:{filetype}"}, +): + pass + + +class InvalidPlatformTag( + ForkliftError, + message="Binary wheel {filename!r} has an unsupported platform tag {tag!r}.", + tags={"reason:unsupported-platform-tag"}, +): + pass + + +class MissingRecordFile( + ForkliftError, + message="Wheel {filename!r} does not contain the required RECORD file: {record_filename}.", + tags={"reason:missing-record-file", "filetype:{filetype}"}, +): + pass + + +class MissingMetadataFile( + ForkliftError, + message="Wheel {filename!r} does not contain the required METADATA file: {metadata_filename}.", + tags={"reason:missing-metadata-file", "filetype:bdist_wheel"}, +): + pass + + +class FileTooLarge( + ForkliftError, + message="File too large. Limit for project {project!r} is {limit}MB.", + user_docs_path="/project-management/storage-limits", + user_docs_anchor="requesting-a-file-size-limit-increase", + tags={"reason:file-too-large"}, +): + pass + + +class ProjectTooLarge( + ForkliftError, + message="Project size too large. Limit for project {project!r} is {limit}GB.", + user_docs_path="/project-management/storage-limits", + user_docs_anchor="requesting-a-project-size-limit-increase", + tags={"reason:project-too-large"}, +): + pass + + +class FilenameTooLong( + ForkliftError, + message="Filename is too long: {filename!r}", + tags={"reason:filename-too-long", "filetype:{filetype}"}, +): + pass + + +class DigestMismatch( + ForkliftError, + message="The digest supplied does not match a digest calculated from the uploaded file.", + tags={"reason:digest-mismatch"}, +): + pass + + +class FileAlreadyExists( + ForkliftError, + # Note: Changing this error message to something that doesn't + # start with "File already exists" will break the + # --skip-existing functionality in twine + # ref: https://github.com/pypi/warehouse/issues/3482 + # ref: https://github.com/pypa/twine/issues/332 + message="File already exists ({filename!r}, with blake2_256 hash {blake2_256!r}).", + help_anchor="file-name-reuse", + tags={"reason:duplicate-file"}, +): + pass + + +class FilenameReused( + ForkliftError, + message=( + "This filename was previously used by a file that has " + "since been deleted. Use a different version." + ), + help_anchor="file-name-reuse", + tags={"reason:filename-reuse"}, +): + pass + + +class DuplicateSDist( + ForkliftError, + message="Only one sdist may be uploaded per release.", + tags={"reason:duplicate-sdist"}, +): + pass + + +class InvalidDistFile( + ForkliftError, + message="Invalid distribution file: {msg}", + tags={ + "reason:invalid-distribution-file", + "filetype:{filetype}", + "message:{msg}", + }, +): + pass + + +class MissingLicenseFile( + ForkliftError, + message=( + "License-File {license_file} does not exist in " + "distribution file {filename} at {target_file}" + ), + tags={"reason:missing-license-file", "filetype:{filetype}"}, +): + pass + + +class InvalidAttestations( + ForkliftError, + message="Invalid attestations supplied during upload: {msg}", + tags={"reason:invalid-attestations"}, +): + pass + + # Wheel platform checking # Note: defining new platform ABI compatibility tags that don't @@ -235,39 +518,19 @@ def _validate_filename(filename, filetype): # or completely by mistake. disallowed = [*(chr(x) for x in range(32)), chr(127)] if [char for char in filename if char in disallowed]: - raise _exc_with_message( - HTTPBadRequest, - ( - "Cannot upload a file with " - "non-printable characters (ordinals 0-31) " - "or the DEL character (ordinal 127) " - "in the name." - ), - ) + raise UnprintableFilename # Make sure that the filename does not contain any path separators. if "/" in filename or "\\" in filename: - raise _exc_with_message( - HTTPBadRequest, "Cannot upload a file with '/' or '\\' in the name." - ) + raise PathlikeFilename # Make sure the filename ends with an allowed extension. if m := _dist_file_re.match(filename): extension = m.group("extension") if extension not in _filetype_extension_mapping[filetype]: - raise _exc_with_message( - HTTPBadRequest, - f"Invalid file extension: Extension {extension} is invalid for " - f"filetype {filetype}. See https://www.python.org/dev/peps/pep-0527 " - "for more information.", - ) + raise InvalidFileExtension(extension=extension, filetype=filetype) else: - raise _exc_with_message( - HTTPBadRequest, - "Invalid file extension: Use .tar.gz, .whl or .zip " - "extension. See https://www.python.org/dev/peps/pep-0527 " - "and https://peps.python.org/pep-0715/ for more information", - ) + raise UnknownFileExtension def _is_valid_dist_file(filename, filetype, metrics, *, scan=True): @@ -561,22 +824,19 @@ def _ensure_user_can_upload(request: Request) -> None: require_methods=["POST"], has_translations=True, permit_duplicate_post_keys=True, - decorator=[sanitize, ensure_uploads_allowed], + # NOTE: translate_errors must be first, so that it is the "outer" decorator + # and can translate errors from the other decorators, as well as the + # file_upload view itself. + decorator=[translate_errors, sanitize, ensure_uploads_allowed], ) def file_upload(request): - # Log an attempt to upload - request.metrics.increment("warehouse.upload.attempt") - # This is a list of warnings that we'll emit *IF* the request is successful. warnings: list[str] = [] # We require protocol_version 1, it's the only supported version however # passing a different version should raise an error. - if request.POST.get("protocol_version", "1") != "1": - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:unsupported-protocol-version"] - ) - raise _exc_with_message(HTTPBadRequest, "Unknown protocol version.") + if (version := request.POST.get("protocol_version", "1")) != "1": + raise InvalidProtocolVersion(version=version) # Validate and process the incoming file data. form = UploadForm(request.POST) @@ -661,11 +921,6 @@ def file_upload(request): ), ) - # Ensure that we have file data in the request. - if "content" not in request.POST: - request.metrics.increment("warehouse.upload.failed", tags=["reason:no-file"]) - raise _exc_with_message(HTTPBadRequest, "Upload payload does not have a file.") - # Set a statement timeout for this connection to prevent long-running # transactions from holding database locks indefinitely. The upload # operation holds a global advisory lock for journal monotonicity, @@ -693,20 +948,7 @@ def file_upload(request): # produce a valid API token, but the project lookup above uses (2) # and will fail because (1) != (2). if not request.user: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:non-user-identity"] - ) - raise _exc_with_message( - HTTPBadRequest, - ( - "Non-user identities cannot create new projects. " - "This was probably caused by successfully using a pending " - "publisher but specifying the project name incorrectly (either " - "in the publisher or in your project's metadata). Please ensure " - "that both match. " - "See: https://docs.pypi.org/trusted-publishers/troubleshooting/" - ), - ) + raise NonUserIdentity # Enforce email/2FA prerequisites before creating a brand new project, # so we don't leave an empty project record behind on rejection. @@ -735,54 +977,21 @@ def file_upload(request): # added above. allowed = request.has_permission(Permissions.ProjectsUpload, project) if not allowed: - reason = getattr(allowed, "reason", None) - if request.user: - msg = ( - ( - "The user '{}' isn't allowed to upload to project '{}'. " - "See {} for more information." - ).format( - request.user.username, - project.name, - request.help_url(_anchor="project-name"), - ) - if reason is None - else allowed.msg + if getattr(allowed, "reason", None) is not None: + raise PermissionDenied(msg=allowed.msg) + elif request.user: + raise UserPermissionDenied( + project=project.name, username=request.user.username ) else: - msg = ( - ( - "The given token isn't allowed to upload to project '{}'. " - "See {} for more information." - ).format( - project.name, - request.help_url(_anchor="project-name"), - ) - if reason is None - else allowed.msg - ) - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:permission-denied"] - ) - raise _exc_with_message(HTTPForbidden, msg) + raise TokenPermissionDenied(project=project.name) _ensure_user_can_upload(request) # If organization owned project, check if the organization is active. # Inactive organizations cannot upload new releases to their projects. if project.organization and not project.organization.good_standing: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:org-not-active"] - ) - raise _exc_with_message( - HTTPBadRequest, - ( - "Organization account owning this project is inactive. " - "This may be due to inactive billing for Company Organizations, " - "or administrator intervention for Community Organizations. " - "Please contact support+orgs@pypi.org." - ), - ) + raise OrgInactive # If this is a user identity (i.e: API token) but there exists # a trusted publisher for this project, send an email warning that an @@ -814,6 +1023,7 @@ def file_upload(request): project_id=project.id, ) ) + # Update name if it differs but is still equivalent. We don't need to check if # they are equivalent when normalized because that's already been done when we # queried for the project. @@ -832,26 +1042,7 @@ def file_upload(request): # Uploading should prevent broken rendered descriptions. if rendered is None: - if meta.description_content_type: - message = ( - "The description failed to render for " - f"'{description_content_type}'." - ) - else: - message = ( - "The description failed to render " - "in the default format of reStructuredText." - ) - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:invalid-description"] - ) - raise _exc_with_message( - HTTPBadRequest, - "{message} See {projecthelp} for more information.".format( - message=message, - projecthelp=request.help_url(_anchor="description-content-type"), - ), - ) from None + raise InvalidDescription(content_type=description_content_type) # Verify any verifiable URLs project_urls = ( @@ -1035,19 +1226,8 @@ def file_upload(request): filename = request.POST["content"].filename # Ensure the filename doesn't contain any characters that are too 🌶️spicy🥵 - # TODO: refactor to accept `request` and emit metrics, or return a list of errors - # and handle them here and emit metrics. _validate_filename(filename, filetype=form.filetype.data) - # Check the content type of what is being uploaded - if not request.POST["content"].type or request.POST["content"].type.startswith( - "image/" - ): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:invalid-content-type"] - ) - raise _exc_with_message(HTTPBadRequest, "Invalid distribution file.") - # The project may or may not have a file size specified on the project, if # it does then it may or may not be smaller or larger than our global file # size limits. Additionally, if the project belongs to an organization, @@ -1072,30 +1252,14 @@ def file_upload(request): for chunk in iter(lambda: request.POST["content"].file.read(8096), b""): file_size += len(chunk) if file_size > file_size_limit: - raise _exc_with_message( - HTTPBadRequest, - "File too large. " - f"Limit for project {project.name!r} is " - f"{file_size_limit // ONE_MIB} MB. " - "See " - + request.user_docs_url( - "/project-management/storage-limits", - anchor="requesting-a-file-size-limit-increase", - ) - + " for more information.", + raise FileTooLarge( + project=project.name, limit=file_size_limit // ONE_MIB ) if file_size + project.total_size > project_size_limit: - raise _exc_with_message( - HTTPBadRequest, - "Project size too large. Limit for " - f"project {project.name!r} total size is " - f"{project_size_limit // ONE_GIB} GB. " - "See " - + request.user_docs_url( - "/project-management/storage-limits", - anchor="requesting-a-project-size-limit-increase", - ), + raise ProjectTooLarge( + project=project.name, limit=project_size_limit // ONE_GIB ) + fp.write(chunk) for hasher in file_hashes.values(): hasher.update(chunk) @@ -1115,52 +1279,23 @@ def file_upload(request): for digest_name, digest_value in file_hashes.items() if getattr(form, f"{digest_name}_digest").data ): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:digest-mismatch"] - ) - raise _exc_with_message( - HTTPBadRequest, - "The digest supplied does not match a digest calculated " - "from the uploaded file.", - ) + raise DigestMismatch # Check to see if the file that was uploaded exists already or not. is_duplicate = _is_duplicate_file(request.db, filename, file_hashes) if is_duplicate: request.tm.doom() return HTTPOk() - if is_duplicate is not None: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:duplicate-file"] - ) - raise _exc_with_message( - HTTPBadRequest, - # Note: Changing this error message to something that doesn't - # start with "File already exists" will break the - # --skip-existing functionality in twine - # ref: https://github.com/pypi/warehouse/issues/3482 - # ref: https://github.com/pypa/twine/issues/332 - "File already exists " - f"({filename!r}, with blake2_256 hash {file_hashes['blake2_256']!r})." - " See " - + request.help_url(_anchor="file-name-reuse") - + " for more information.", + elif is_duplicate is not None: + raise FileAlreadyExists( + filename=filename, blake2_256=file_hashes["blake2_256"] ) # Check to see if the file that was uploaded exists in our filename log if request.db.query( request.db.query(Filename).filter(Filename.filename == filename).exists() ).scalar(): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:filename-reuse"] - ) - raise _exc_with_message( - HTTPBadRequest, - "This filename was previously used by a file that has since been " - "deleted. Use a different version. See " - + request.help_url(_anchor="file-name-reuse") - + " for more information.", - ) + raise FilenameReused # Check to see if uploading this file would create a duplicate sdist # for the current release. @@ -1172,12 +1307,7 @@ def file_upload(request): .exists() ).scalar() ): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:duplicate-sdist"] - ) - raise _exc_with_message( - HTTPBadRequest, "Only one sdist may be uploaded per release." - ) + raise DuplicateSDist # Check the file to make sure it is a valid distribution file. _scan = not request.flags.enabled(AdminFlagValue.DISABLE_UPLOAD_SCANNING) @@ -1192,17 +1322,7 @@ def file_upload(request): scan=_scan, ) if not _valid: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:invalid-distribution-file", - f"filetype:{form.filetype.data}", - f"message:{_msg}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, f"Invalid distribution file. {_msg}" - ) + raise InvalidDistFile(filetype=form.filetype.data, msg=_msg) # Check that the sdist filename is correct if form.filetype.data == "sdist": @@ -1212,14 +1332,7 @@ def file_upload(request): packaging.utils.parse_sdist_filename(filename) ) except packaging.utils.InvalidSdistFilename: - request.metrics.increment( - "warehouse.upload.failed", - tags=["reason:invalid-filename", f"filetype:{form.filetype.data}"], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Invalid source distribution filename: {filename}", - ) + raise InvalidFilename(filename=filename, filetype=form.filetype.data) # The previous function fails to accomodate the edge case where # versions may contain hyphens, so we handle that here based on @@ -1252,17 +1365,10 @@ def file_upload(request): packaging.utils.canonicalize_name(name_from_filename) != project.normalized_name ): - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:invalid-filename-normalized", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Start filename for {project.name!r} with " - f"{project.normalized_name.replace('-', '_')!r}.", + raise UnnormalizedFilename( + project=project.name, + normalized=project.normalized_name.replace("-", "_"), + filetype=form.filetype.data, ) # PEP 625: Enforcement of source distribution filename @@ -1272,14 +1378,10 @@ def file_upload(request): expected_filename = f"{expected_name}-{expected_version}.tar.gz" if filename != expected_filename: - request.metrics.increment( - "warehouse.upload.failed", - tags=["reason:invalid-sdist-filename-normalization"], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Filename {filename!r} is invalid, should be " - f"{expected_filename!r}.", + raise UnnormalizedFilename( + project=project.name, + normalized=project.normalized_name.replace("-", "_"), + filetype=form.filetype.data, ) filename = os.path.basename(temporary_filename) @@ -1297,17 +1399,11 @@ def file_upload(request): try: tar.getmember(target_file) except KeyError: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:missing-license-file", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"License-File {license_file} does not exist in " - f"distribution file {filename} at {target_file}", + raise MissingLicenseFile( + filename=filename, + license_file=license_file, + target_file=target_file, + filetype=form.filetype.data, ) # Check that if it's a binary wheel, it's on a supported platform @@ -1317,39 +1413,17 @@ def file_upload(request): filename ) except packaging.utils.InvalidWheelFilename as e: - request.metrics.increment( - "warehouse.upload.failed", - tags=["reason:invalid-filename", f"filetype:{form.filetype.data}"], - ) - raise _exc_with_message( - HTTPBadRequest, - str(e), - ) + raise InvalidWheelFilename(filename=filename, msg=str(e)) for tag in tags: if not _valid_platform_tag(tag.platform): - request.metrics.increment( - "warehouse.upload.failed", - tags=["reason:unsupported-platform-tag"], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Binary wheel '{filename}' has an unsupported " - f"platform tag '{tag.platform}'.", - ) + raise InvalidPlatformTag(filename=filename, tag=tag.platform) if (canonical_name := packaging.utils.canonicalize_name(meta.name)) != name: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:invalid-filename-normalized", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Start filename for {project.name!r} with " - f"{canonical_name.replace('-', '_')!r}.", + raise UnnormalizedFilename( + project=project.name, + normalized=canonical_name.replace("-", "_"), + filetype=form.filetype.data, ) # The parse_wheel_filename function does not enforce lowercasing, @@ -1363,32 +1437,17 @@ def file_upload(request): # https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode normalized_name = project.normalized_name.replace("-", "_") if name_from_filename != normalized_name: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:invalid-filename-projectname", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Filename {filename!r} should contain the normalized " - f"project name {normalized_name!r}, not " - f"{name_from_filename!r}.", + raise UnnormalizedFilename( + project=project.name, + normalized=normalized_name, + filetype=form.filetype.data, ) if meta.version != version: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:invalid-filename-version", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Version in filename should be {str(meta.version)!r} not " - f"{str(version)!r}.", + raise InvalidFilenameVersion( + expected=str(meta.version), + found=str(version), + filetype=form.filetype.data, ) filename = os.path.basename(temporary_filename) @@ -1411,36 +1470,20 @@ def file_upload(request): try: zfp.read(license_filename) except KeyError: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:missing-license-file", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"License-File {license_file} does not exist in " - f"distribution file {filename} at {license_filename}", + raise MissingLicenseFile( + filename=filename, + license_file=license_file, + target_file=license_filename, + filetype=form.filetype.data, ) try: validate_record(temporary_filename) except MissingWheelRecordError: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:missing-record-file", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - "Wheel '{filename}' does not contain the required " - "RECORD file: {record_filename}".format( - filename=filename, - record_filename=f"{name}-{version}.dist-info/RECORD", - ), + raise MissingRecordFile( + filename=filename, + record_filename=f"{name}-{version}.dist-info/RECORD", + filetype=form.filetype.data, ) except InvalidWheelRecordError: send_wheel_record_mismatch_email( @@ -1460,33 +1503,14 @@ def file_upload(request): with zipfile.ZipFile(temporary_filename) as zfp: wheel_metadata_contents = zfp.read(metadata_filename) except KeyError: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:missing-metadata-file", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Wheel '{filename}' does not contain the required " - f"METADATA file: {metadata_filename}", + raise MissingMetadataFile( + filename=filename, metadata_filename=metadata_filename ) try: with open(temporary_filename + ".metadata", "wb") as fp: fp.write(wheel_metadata_contents) except OSError: - request.metrics.increment( - "warehouse.upload.failed", - tags=[ - "reason:filename-too-long", - f"filetype:{form.filetype.data}", - ], - ) - raise _exc_with_message( - HTTPBadRequest, - f"Filename is too long: '{filename}'", - ) + raise FilenameTooLong(filename=filename, filetype=form.filetype.data) metadata_file_hashes = { "sha256": hashlib.sha256(), @@ -1514,13 +1538,7 @@ def file_upload(request): Distribution(name=filename, digest=file_hashes["sha256"]), ) except AttestationUploadError as e: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:invalid-attestations"] - ) - raise _exc_with_message( - HTTPBadRequest, - f"Invalid attestations supplied during upload: {e}", - ) + raise InvalidAttestations(msg=e) # Store the information about the file in the database. file_ = File( @@ -1726,6 +1744,8 @@ def file_upload(request): request.task(update_bigquery_release_files).delay(dist_metadata) # Log a successful upload + # TODO: This would ideally be located near the other metrics calls, but that + # happens at a layer that doesn't have the filetype data. request.metrics.increment( "warehouse.upload.ok", tags=[f"filetype:{form.filetype.data}"] ) diff --git a/warehouse/forklift/utils.py b/warehouse/forklift/utils.py deleted file mode 100644 index 52aa8a14f0c3..000000000000 --- a/warehouse/forklift/utils.py +++ /dev/null @@ -1,18 +0,0 @@ -# SPDX-License-Identifier: Apache-2.0 - -import sentry_sdk - - -def _exc_with_message(exc, message, **kwargs): - if not message: - sentry_sdk.capture_message("Attempting to _exc_with_message without a message") - - # The crappy old API that PyPI offered uses the status to pass down - # messages to the client. So this function will make that easier to do. - resp = exc(detail=message, **kwargs) - # We need to guard against characters outside of iso-8859-1 per RFC. - # Specifically here, where user-supplied text may appear in the message, - # which our WSGI server may not appropriately handle (indeed gunicorn does not). - status_message = message.encode("iso-8859-1", "replace").decode("iso-8859-1") - resp.status = f"{resp.status_code} {status_message}" - return resp From 01a85de2f61774b680efb4041c5f0eafd906365b Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sat, 25 Apr 2026 19:43:32 -0400 Subject: [PATCH 02/12] update tests --- tests/unit/forklift/test_decorators.py | 10 + tests/unit/forklift/test_legacy.py | 579 +++++-------------------- warehouse/forklift/legacy.py | 12 +- 3 files changed, 110 insertions(+), 491 deletions(-) diff --git a/tests/unit/forklift/test_decorators.py b/tests/unit/forklift/test_decorators.py index 2fcc67d45f78..21fefe3a6ffc 100644 --- a/tests/unit/forklift/test_decorators.py +++ b/tests/unit/forklift/test_decorators.py @@ -69,6 +69,16 @@ def wrapped(context, request): assert excinfo.value.values == {"field": "keywords"} + def test_fails_without_file(self): + req = DummyRequest(post=MultiDict({})) + + @decorators.sanitize + def wrapped(context, request): + pytest.fail("wrapped view should not have been called") + + with pytest.raises(decorators.NoFileUpload): + wrapped(pretend.stub(), req) + @pytest.mark.parametrize("content_type", [None, "image/foobar"]) def test_fails_invalid_content_type(self, content_type): req = DummyRequest(post=MultiDict({"content": pretend.stub(type=content_type)})) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 652458dae0a7..e874afaf03a3 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -955,7 +955,7 @@ def test_fails_with_ultranormalized_names( [("text/x-rst", ".. invalid-directive::"), (None, ".. invalid-directive::")], ) def test_fails_invalid_render( - self, pyramid_config, db_request, description_content_type, description, message + self, pyramid_config, db_request, description_content_type, description ): user = UserFactory.create() EmailFactory.create(user=user) @@ -1052,29 +1052,6 @@ def test_fails_with_stdlib_names(self, pyramid_config, db_request, name): "for more information." ) - def test_upload_fails_without_file(self, pyramid_config, db_request): - user = UserFactory.create() - EmailFactory.create(user=user) - pyramid_config.testing_securitypolicy(identity=user) - db_request.user = user - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": "example", - "version": "1.0", - "filetype": "sdist", - "md5_digest": "a fake md5 digest", - } - ) - - with pytest.raises(HTTPBadRequest) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == "400 Upload payload does not have a file." - @pytest.mark.parametrize("macaroon_in_user_context", [True, False]) @pytest.mark.parametrize( "digests", @@ -1272,7 +1249,6 @@ def storage_service_store(path, file_path, *, meta): ] assert db_request.metrics.increment.calls == [ - pretend.call("warehouse.upload.attempt"), pretend.call("warehouse.upload.ok", tags=["filetype:sdist"]), ] @@ -1344,18 +1320,9 @@ def test_upload_fails_with_legacy_ext(self, pyramid_config, db_request): } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnknownFileExtension): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Invalid file extension: Use .tar.gz, .whl or .zip " - "extension. See https://www.python.org/dev/peps/pep-0527 " - "and https://peps.python.org/pep-0715/ for more information" - ) - def test_upload_fails_for_second_sdist(self, pyramid_config, db_request): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -1556,17 +1523,9 @@ def test_upload_fails_with_invalid_digest( ) db_request.POST.update(digests) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.DigestMismatch): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 The digest supplied does not match a digest calculated " - "from the uploaded file." - ) - def test_upload_fails_with_invalid_file(self, pyramid_config, db_request): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -1669,26 +1628,10 @@ def test_upload_fails_with_too_large_file(self, pyramid_config, db_request): ), } ) - db_request.user_docs_url = pretend.call_recorder( - lambda *a, **kw: "/the/help/url/" - ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.FileTooLarge) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.user_docs_url.calls == [ - pretend.call( - "/project-management/storage-limits", - anchor="requesting-a-file-size-limit-increase", - ) - ] - assert resp.status_code == 400 - assert resp.status == ( - "400 File too large. Limit for project 'foobar' is 100 MB. " - "See /the/help/url/ for more information." - ) + assert excinfo.value.values == {"project": project.name, "limit": 100} def test_upload_fails_with_too_large_project_size_default_limit( self, pyramid_config, db_request @@ -1723,27 +1666,10 @@ def test_upload_fails_with_too_large_project_size_default_limit( ), } ) - db_request.user_docs_url = pretend.call_recorder( - lambda *a, **kw: "/the/help/url/" - ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.ProjectTooLarge) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.user_docs_url.calls == [ - pretend.call( - "/project-management/storage-limits", - anchor="requesting-a-project-size-limit-increase", - ) - ] - assert resp.status_code == 400 - assert resp.status == ( - "400 Project size too large." - " Limit for project 'foobar' total size is 10 GB. " - "See /the/help/url/" - ) + assert excinfo.value.values == {"project": project.name, "limit": 10} def test_upload_fails_with_too_large_project_size_custom_limit( self, pyramid_config, db_request @@ -1783,27 +1709,10 @@ def test_upload_fails_with_too_large_project_size_custom_limit( ), } ) - db_request.user_docs_url = pretend.call_recorder( - lambda *a, **kw: "/the/help/url/" - ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.ProjectTooLarge) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.user_docs_url.calls == [ - pretend.call( - "/project-management/storage-limits", - anchor="requesting-a-project-size-limit-increase", - ) - ] - assert resp.status_code == 400 - assert resp.status == ( - "400 Project size too large." - " Limit for project 'foobar' total size is 10 GB. " - "See /the/help/url/" - ) + assert excinfo.value.values == {"project": project.name, "limit": 10} def test_upload_succeeds_custom_project_size_limit( self, @@ -1968,21 +1877,9 @@ def mock_open(file, mode="r", *args, **kwargs): monkeypatch.setattr("builtins.open", mock_open) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.FilenameTooLong) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == f"400 Filename is too long: '{filename}'" - - assert db_request.metrics.increment.calls == [ - pretend.call("warehouse.upload.attempt"), - pretend.call( - "warehouse.upload.failed", - tags=["reason:filename-too-long", "filetype:bdist_wheel"], - ), - ] + assert excinfo.value.values == {"filename": filename, "filetype": "bdist_wheel"} def test_upload_fails_with_previously_used_filename( self, pyramid_config, db_request @@ -2014,20 +1911,10 @@ def test_upload_fails_with_previously_used_filename( ) db_request.db.add(Filename(filename=filename)) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.FilenameReused): legacy.file_upload(db_request) - resp = excinfo.value - - assert db_request.help_url.calls == [pretend.call(_anchor="file-name-reuse")] - assert resp.status_code == 400 - assert resp.status == ( - "400 This filename was previously used by a file that has since been " - "deleted. Use a different version. See /the/help/url/ for more information." - ) - def test_upload_noop_with_existing_filename_same_content( self, pyramid_config, db_request ): @@ -2169,20 +2056,13 @@ def test_upload_fails_with_diff_filename_same_blake2( path=f"source/{project.name[0]}/{project.name}/{filename}", ) ) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.FileAlreadyExists) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.help_url.calls == [pretend.call(_anchor="file-name-reuse")] - assert resp.status_code == 400 - assert resp.status == ( - f"400 File already exists ({db_request.POST['content'].filename!r}, " - f"with blake2_256 hash {blake2_256_digest!r}). " - "See /the/help/url/ for more information." - ) + assert excinfo.value.values == { + "filename": f"{project.name}-fake.tar.gz", + "blake2_256": blake2_256_digest, + } @pytest.mark.parametrize( ("filename", "filetype", "project_name"), @@ -2257,8 +2137,16 @@ def test_upload_fails_with_wrong_filename_project_name( @pytest.mark.parametrize( ("filename", "exc_type", "expected"), [ - ("wutang-6.6.6.tar.gz", legacy.UnnormalizedFilename, {}), - ("wutang-6.6.6-py3-none-any.whl", legacy.InvalidFilenameVersion, {}), + ( + "wutang-6.6.6.tar.gz", + legacy.UnnormalizedFilename, + {"project": "wutang", "normalized": "wutang", "filetype": "sdist"}, + ), + ( + "wutang-6.6.6-py3-none-any.whl", + legacy.InvalidFilenameVersion, + {"expected": "1.2.3", "filetype": "bdist_wheel", "found": "6.6.6"}, + ), ], ) def test_upload_fails_with_wrong_filename_version( @@ -2348,17 +2236,9 @@ def test_upload_fails_with_invalid_filetype( } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidFileExtension) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - f"400 Invalid file extension: Extension {extension} is invalid for " - f"filetype {filetype}. See https://www.python.org/dev/peps/pep-0527 " - "for more information." - ) + assert excinfo.value.values == {"extension": extension, "filetype": filetype} def test_upload_fails_with_invalid_extension(self, pyramid_config, db_request): user = UserFactory.create() @@ -2422,14 +2302,9 @@ def test_upload_fails_with_unsafe_filename( } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.PathlikeFilename): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == "400 Cannot upload a file with '/' or '\\' in the name." - @pytest.mark.parametrize("character", [*(chr(x) for x in range(32)), chr(127)]) def test_upload_fails_with_disallowed_in_filename( self, pyramid_config, db_request, character @@ -2494,20 +2369,12 @@ def test_upload_fails_without_user_permission(self, pyramid_config, db_request): } ) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: + with pytest.raises(legacy.UserPermissionDenied) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.help_url.calls == [pretend.call(_anchor="project-name")] - assert resp.status_code == 403 - assert resp.status == ( - f"403 The user '{user2.username}' " - f"isn't allowed to upload to project '{project.name}'. " - "See /the/help/url/ for more information." - ) + assert excinfo.value.values == { + "project": project.name, + "username": user2.username, + } def test_upload_fails_without_oidc_publisher_permission( self, pyramid_config, db_request @@ -2538,212 +2405,9 @@ def test_upload_fails_without_oidc_publisher_permission( } ) - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: + with pytest.raises(legacy.TokenPermissionDenied) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert db_request.help_url.calls == [pretend.call(_anchor="project-name")] - assert resp.status_code == 403 - assert resp.status == ( - f"403 The given token isn't allowed to upload to project '{project.name}'. " - "See /the/help/url/ for more information." - ) - - def test_upload_fails_with_unverified_email_after_permission_check( - self, pyramid_config, db_request, mocker - ): - """The email check fires from inside the view, after the permission - check passes, so a maintainer without a verified primary email gets - the email error. - - See: https://github.com/pypi/warehouse/issues/18575 - """ - user = UserFactory.create() - project = ProjectFactory.create() - release = ReleaseFactory.create(project=project, version="1.0") - RoleFactory.create(user=user, project=project) - - filename = "{}-{}.tar.gz".format( - project.normalized_name.replace("-", "_"), release.version - ) - - pyramid_config.testing_securitypolicy(identity=user) - db_request.user = user - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": project.name, - "version": release.version, - "filetype": "sdist", - "md5_digest": "nope!", - "content": SimpleNamespace( - filename=filename, - file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), - type="application/tar", - ), - } - ) - db_request.help_url = mocker.Mock(return_value="/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == ( - f"403 User {user.username!r}, associated with the API token used, " - "does not have a verified primary email address. Please add a " - "verified primary email before attempting to upload to PyPI. " - "See /the/help/url/ for more information." - ) - - def test_upload_fails_without_two_factor_after_permission_check( - self, pyramid_config, db_request, mocker - ): - """A user with permission and a verified email but no 2FA receives - the 2FA error from the view, after the permission check.""" - user = UserFactory.create(with_verified_primary_email=True) - user.totp_secret = None # Drop totp_secret so has_two_factor is False. - project = ProjectFactory.create() - release = ReleaseFactory.create(project=project, version="1.0") - RoleFactory.create(user=user, project=project) - - filename = "{}-{}.tar.gz".format( - project.normalized_name.replace("-", "_"), release.version - ) - - pyramid_config.testing_securitypolicy(identity=user) - db_request.user = user - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": project.name, - "version": release.version, - "filetype": "sdist", - "md5_digest": "nope!", - "content": SimpleNamespace( - filename=filename, - file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), - type="application/tar", - ), - } - ) - db_request.help_url = mocker.Mock(return_value="/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == ( - f"403 User {user.username!r}, associated with the API token used, " - "does not have two-factor authentication enabled. Please enable " - "two-factor authentication before attempting to upload to PyPI. " - "See /the/help/url/ for more information." - ) - - def test_upload_permission_error_surfaces_before_email_check( - self, pyramid_config, db_request, mocker - ): - """When a user lacks both project permission and a verified email, - the permission error surfaces first because it is the more useful - diagnostic for the misconfigured-token case. - - See: https://github.com/pypi/warehouse/issues/18575 - """ - owner = UserFactory.create() - EmailFactory.create(user=owner) - intruder = UserFactory.create() # No verified email, no 2FA. - project = ProjectFactory.create() - release = ReleaseFactory.create(project=project, version="1.0") - RoleFactory.create(user=owner, project=project) - - filename = "{}-{}.tar.gz".format( - project.normalized_name.replace("-", "_"), release.version - ) - - pyramid_config.testing_securitypolicy(identity=intruder, permissive=False) - db_request.user = intruder - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": project.name, - "version": release.version, - "filetype": "sdist", - "md5_digest": "nope!", - "content": SimpleNamespace( - filename=filename, - file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), - type="application/tar", - ), - } - ) - db_request.help_url = mocker.Mock(return_value="/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == ( - f"403 The user {intruder.username!r} " - f"isn't allowed to upload to project '{project.name}'. " - "See /the/help/url/ for more information." - ) - - def test_upload_new_project_fails_with_unverified_email( - self, pyramid_config, db_request, mocker - ): - """A user attempting to upload (and thereby create) a brand new - project without a verified email is rejected before the project - gets created, so we don't leave an empty project record behind.""" - user = UserFactory.create() # No verified email. - - filename = "new_project-1.0.tar.gz" - - pyramid_config.testing_securitypolicy(identity=user) - db_request.user = user - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": "new-project", - "version": "1.0", - "filetype": "sdist", - "md5_digest": "nope!", - "content": SimpleNamespace( - filename=filename, - file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), - type="application/tar", - ), - } - ) - db_request.help_url = mocker.Mock(return_value="/the/help/url/") - - with pytest.raises(HTTPForbidden) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 403 - assert resp.status == ( - f"403 User {user.username!r}, associated with the API token used, " - "does not have a verified primary email address. Please add a " - "verified primary email before attempting to upload to PyPI. " - "See /the/help/url/ for more information." - ) - # The project must not have been created. - assert ( - db_request.db.query(Project) - .filter(Project.normalized_name == "new-project") - .first() - is None - ) + assert excinfo.value.values == {"project": project.name} def test_upload_attestation_fails_without_oidc_publisher( self, @@ -2816,16 +2480,11 @@ def test_upload_attestation_fails_without_oidc_publisher( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidAttestations) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Invalid attestations supplied during upload: " - "Attestations are only supported when using Trusted Publishing" - ) + assert excinfo.value.values == { + "msg": "Attestations are only supported when using Trusted Publishing" + } @pytest.mark.parametrize( "plat", @@ -3360,29 +3019,20 @@ def storage_service_store(path, file_path, *, meta): ] @pytest.mark.parametrize( - ("filename", "expected"), + ("filename"), [ - ( - "foo-1.0.whl", - "400 Invalid wheel filename (wrong number of parts): 'foo-1.0'", - ), - ( - "foo-1.0-q-py3-none-any.whl", - "400 Invalid build number: q in 'foo-1.0-q-py3-none-any'", - ), - ( - "foo-0.0.4test1-py3-none-any.whl", - "400 Invalid wheel filename (invalid version): " - "'foo-0.0.4test1-py3-none-any'", - ), - ( - "something.tar.gz", - "400 Invalid source distribution filename: something.tar.gz", - ), + "foo-1.0.whl", + "foo-1.0-q-py3-none-any.whl", + "foo-0.0.4test1-py3-none-any.whl", + "something.tar.gz", ], ) def test_upload_fails_with_invalid_filename( - self, monkeypatch, pyramid_config, db_request, filename, expected + self, + monkeypatch, + pyramid_config, + db_request, + filename, ): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -3417,28 +3067,27 @@ def test_upload_fails_with_invalid_filename( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidFilename) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == expected + assert excinfo.value.values == { + "filename": filename, + "filetype": "bdist_wheel" if filename.endswith(".whl") else "sdist", + } @pytest.mark.parametrize( - "plat", + ("plat", "expected"), [ - "linux_x86_64", - "linux_x86_64.win32", - "macosx_9_2_x86_64", - "macosx_11_2_arm64", - "macosx_16_2_arm64", - "macosx_10_15_amd64", - "macosx_27_0_arm64", + ("linux_x86_64", "linux_x86_64"), + ("linux_x86_64.win32", "linux_x86_64"), + ("macosx_9_2_x86_64", "macosx_9_2_x86_64"), + ("macosx_11_2_arm64", "macosx_11_2_arm64"), + ("macosx_16_2_arm64", "macosx_16_2_arm64"), + ("macosx_10_15_amd64", "macosx_10_15_amd64"), + ("macosx_27_0_arm64", "macosx_27_0_arm64"), ], ) def test_upload_fails_with_unsupported_wheel_plat( - self, monkeypatch, pyramid_config, db_request, plat + self, monkeypatch, pyramid_config, db_request, plat, expected ): user = UserFactory.create() pyramid_config.testing_securitypolicy(identity=user) @@ -3472,8 +3121,7 @@ def test_upload_fails_with_unsupported_wheel_plat( with pytest.raises(legacy.InvalidPlatformTag) as excinfo: legacy.file_upload(db_request) - - assert excinfo.value.values == {"filename": filename, "tag": plat} + assert excinfo.value.values == {"filename": filename, "tag": expected} def test_upload_fails_with_missing_record_wheel( self, monkeypatch, pyramid_config, db_request @@ -3521,7 +3169,7 @@ def test_upload_fails_with_missing_record_wheel( assert excinfo.value.values == { "filename": filename, - "record_filename": f"{project.normalized_name.replace("-", "_")}-1.0.dist-info/RECORD", + "record_filename": f"{project.normalized_name.replace('-', '_')}-1.0.dist-info/RECORD", "filetype": "bdist_wheel", } @@ -3935,15 +3583,12 @@ def test_upload_fails_with_missing_metadata_wheel( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.MissingMetadataFile) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert re.match( - "400 Wheel .* does not contain the required METADATA file: .*", resp.status - ) + assert excinfo.value.values == { + "filename": filename, + "metadata_filename": f"{project_name}-{release.version}.dist-info/METADATA", + } def test_upload_updates_existing_project_name(self, pyramid_config, db_request): user = UserFactory.create() @@ -5004,21 +4649,9 @@ def test_upload_fails_nonuser_identity_cannot_create_project( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.NonUserIdentity) as excinfo: legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Non-user identities cannot create new projects. " - "This was probably caused by successfully using a pending " - "publisher but specifying the project name incorrectly (either " - "in the publisher or in your project's metadata). Please ensure " - "that both match. " - "See: https://docs.pypi.org/trusted-publishers/troubleshooting/" - ) - @pytest.mark.parametrize( ("failing_limiter", "remote_addr"), [ @@ -5441,19 +5074,18 @@ def test_upload_fails_pep427( IProjectService: project_service, }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") monkeypatch.setattr( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnnormalizedFilename) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == status + assert excinfo.value.values == { + "project": project.name, + "normalized": project.normalized_name.replace("-", "_"), + "filetype": "bdist_wheel", + } @pytest.mark.parametrize( ("filename", "version", "expected"), @@ -5509,21 +5141,18 @@ def test_upload_fails_pep625( IProjectService: project_service, }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - db_request.help_url = pretend.call_recorder(lambda **kw: "/the/help/url/") monkeypatch.setattr( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.UnnormalizedFilename) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - f"400 Filename {filename!r} is invalid, should be {expected!r}." - ) + assert excinfo.value.values == { + "project": project.name, + "normalized": project.normalized_name.replace("-", "_"), + "filetype": "sdist", + } @pytest.mark.parametrize( ("version", "expected_version", "filetype", "mimetype"), @@ -5803,16 +5432,14 @@ def test_upload_fails_missing_license_file_metadata_2_4( IFileStorage: storage_service, }.get(svc) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.MissingLicenseFile) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - f"400 License-File LICENSE does not exist " - f"in distribution file {filename} at {license_filename}" - ) + assert excinfo.value.values == { + "filename": filename, + "license_file": "LICENSE", + "target_file": license_filename, + "filetype": filetype, + } def test_upload_fails_when_license_and_license_expression_are_present( self, @@ -5970,19 +5597,9 @@ def test_upload_for_company_organization_owned_project_fails_without_subscriptio } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.OrgInactive): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 Organization account owning this project is inactive. " - "This may be due to inactive billing for Company Organizations, " - "or administrator intervention for Community Organizations. " - "Please contact support+orgs@pypi.org." - ) - def test_upload_with_organization_file_size_limit_succeeds( self, pyramid_config, db_request, monkeypatch ): @@ -6457,12 +6074,12 @@ def test_upload_fails_with_yara_match( } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidDistFile) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - assert resp.status_code == 400 - assert "PyArmor-encrypted content is not allowed" in resp.status + assert excinfo.value.values == { + "msg": "PyArmor-encrypted content is not allowed. See https://pypi.org/policy/terms-of-use/ for more information.", + "filetype": "bdist_wheel", + } def test_submit(pyramid_request): diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index e811adad6696..168cd63b1fca 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -221,14 +221,6 @@ class InvalidFilename( pass -class InvalidWheelFilename( - ForkliftError, - message="Invalid filename {filename!r}: {msg}", - tags={"reason:invalid-filename", "filetype:bdist_wheel"}, -): - pass - - class UnnormalizedFilename( ForkliftError, message="Filename for {project!r} should contain the normalized project name {normalized!r}.", @@ -1413,7 +1405,7 @@ def file_upload(request): filename ) except packaging.utils.InvalidWheelFilename as e: - raise InvalidWheelFilename(filename=filename, msg=str(e)) + raise InvalidFilename(filename=filename, filetype=form.filetype.data) for tag in tags: if not _valid_platform_tag(tag.platform): @@ -1538,7 +1530,7 @@ def file_upload(request): Distribution(name=filename, digest=file_hashes["sha256"]), ) except AttestationUploadError as e: - raise InvalidAttestations(msg=e) + raise InvalidAttestations(msg=str(e)) # Store the information about the file in the database. file_ = File( From 11bd7a925ad2620ad146aae2073dbb3361c508e4 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sat, 25 Apr 2026 20:23:43 -0400 Subject: [PATCH 03/12] more test fixes --- tests/unit/forklift/test_legacy.py | 23 +++++------ warehouse/forklift/decorators.py | 40 +++++++++++++++++++ warehouse/forklift/errors.py | 63 +++++------------------------- warehouse/forklift/legacy.py | 41 ++++++++++--------- 4 files changed, 81 insertions(+), 86 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index e874afaf03a3..88f03cb0184d 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -5,7 +5,6 @@ import hashlib import io import json -import re import tarfile import tempfile import zipfile @@ -20,7 +19,7 @@ import pytest from pypi_attestations import Attestation, Envelope, VerificationMaterial -from pyramid.httpexceptions import HTTPBadRequest, HTTPForbidden, HTTPTooManyRequests +from pyramid.httpexceptions import HTTPBadRequest, HTTPTooManyRequests from sqlalchemy import and_, event, exists from sqlalchemy.orm import joinedload from trove_classifiers import classifiers @@ -1248,10 +1247,6 @@ def storage_service_store(path, file_path, *, meta): pretend.call(uploaded_file.id), ] - assert db_request.metrics.increment.calls == [ - pretend.call("warehouse.upload.ok", tags=["filetype:sdist"]), - ] - def test_upload_fails_with_legacy_type(self, pyramid_config, db_request): user = UserFactory.create() EmailFactory.create(user=user) @@ -2670,10 +2665,6 @@ def storage_service_store(path, file_path, *, meta): ) ] - assert db_request.metrics.increment.calls == [ - pretend.call("warehouse.upload.ok", tags=["filetype:bdist_wheel"]), - ] - @pytest.mark.parametrize( ("project_name", "version"), [ @@ -3167,9 +3158,10 @@ def test_upload_fails_with_missing_record_wheel( with pytest.raises(legacy.MissingRecordFile) as excinfo: legacy.file_upload(db_request) + expected_filename = project.normalized_name.replace("-", "_") assert excinfo.value.values == { "filename": filename, - "record_filename": f"{project.normalized_name.replace('-', '_')}-1.0.dist-info/RECORD", + "record_filename": f"{expected_filename}-1.0.dist-info/RECORD", "filetype": "bdist_wheel", } @@ -3987,7 +3979,7 @@ def test_upload_fails_attestation_error( ) monkeypatch.setattr(HasEvents, "record_event", record_event) - with pytest.raises(legacy.InvalidAttestations) as excinfo: + with pytest.raises(legacy.InvalidAttestations): legacy.file_upload(db_request) @pytest.mark.parametrize( @@ -4649,7 +4641,7 @@ def test_upload_fails_nonuser_identity_cannot_create_project( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(legacy.NonUserIdentity) as excinfo: + with pytest.raises(legacy.NonUserIdentity): legacy.file_upload(db_request) @pytest.mark.parametrize( @@ -6077,7 +6069,10 @@ def test_upload_fails_with_yara_match( with pytest.raises(legacy.InvalidDistFile) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { - "msg": "PyArmor-encrypted content is not allowed. See https://pypi.org/policy/terms-of-use/ for more information.", + "msg": ( + "PyArmor-encrypted content is not allowed. See " + "https://pypi.org/policy/terms-of-use/ for more information." + ), "filetype": "bdist_wheel", } diff --git a/warehouse/forklift/decorators.py b/warehouse/forklift/decorators.py index 89870b1fb36d..5e5bc9bd1911 100644 --- a/warehouse/forklift/decorators.py +++ b/warehouse/forklift/decorators.py @@ -89,6 +89,46 @@ class MissingTwoFactor( pass +def upload_metrics(wrapped): + + def wrapper(context, request): + # We'll manually pull this out of the request because we don't want to invoke + # the forms machinery or anything. This may end up meaning we get an invalid + # value, but that's OK it's just the metrics tagging so not the end of the + # world. + # + # NOTE: We'll only use these tags for cases where we can't expect the properly + # validated value to have already been included. + tags = { + f"{k}:{v}" for (k, v) in [("filetype", request.POST.get("filetype"))] if v + } + + # Log an attempt to upload + request.metrics.increment("warehouse.upload.attempt", tags=tags) + + try: + response = wrapped(context, request) + except ForkliftError as exc: + # We've got an error that we know how to get the tags out of it, so we'll + # increment a failure with those tags. + request.metrics.increment("warehouse.upload.failed", tags=exc.tags) + raise + except Exception: + # If we get any other kind of exception, then we'll mark a failed + # upload as well, but we don't know what the error type is. + request.metrics.increment( + "warehouse.upload.failed", tags=tags | {"reason:unknown-error"} + ) + raise + else: + # Log a successful upload + request.metrics.increment("warehouse.upload.ok", tags=tags) + + return response + + return wrapper + + def sanitize(wrapped): """ Wraps a Pyramid view to sanitize the incoming request of "bad" values. diff --git a/warehouse/forklift/errors.py b/warehouse/forklift/errors.py index 8cdb289100d3..187563b702db 100644 --- a/warehouse/forklift/errors.py +++ b/warehouse/forklift/errors.py @@ -6,25 +6,26 @@ import sentry_sdk from pyramid.httpexceptions import HTTPBadRequest, HTTPException +from pyramid.view import exception_view_config # We don't subclass from a HTTP Exception here, because pyramid will automatically # turn those into a response for us, and we don't want that to happen "silently" # for these exceptions. # -# In other words, ForkliftExceptions are only intended to be raised from within -# forklift, and forklift should always have the decorator applied that does the +# In other words, a ForkliftError is only intended to be raised from within +# forklift, and forklift should always have an exception view applied that does the # translation to what Pyramid expects, so these should never reach Pyramid itself, # and if they do, then something has gone wrong and we should be alerted to that # fact. # -# All of our ForkliftExceptions have certain values that are set at the class +# All of our ForkliftErrors have certain values that are set at the class # level (like a message, tags to emit in the metrics, etc) and accept arbitrary # keyword arguments which will be formatted into those values at runtime. # # This means we can have a message (or a tag, or whatever) that says something # like `message = "this is an error with {foo}` and you can pass in foo as a -# keyword argument to the ForkliftException. +# keyword argument to the ForkliftError. class ForkliftError(Exception): message: str tags: set[str] @@ -145,53 +146,7 @@ def _exc_with_message(exc, message, **kwargs): return resp -def translate_errors(wrapped): - """ - Wraps a Pyramid view to translate errors into the format expected by the - "legacy" upload API. - - There's a general pattern that gets used wherever we have errors in the - legacy API, and while each individual location doesn't cause tons of lines - of code, collectively this ends up adding a lot of noise to that view - function. - - Essentially this decorator knows how to turn specific exceptions into the - correct HTTP responses, as well as centralizes things like emitting metrics, - etc. - """ - - def wrapper(context, request): - # Log an attempt to upload - # - # NOTE: This technically isn't part of "translating errors", but we - # emit metrics for failure in this decorator, so it makes the most - # sense to keep this here so it's co-located with that. - request.metrics.increment("warehouse.upload.attempt") - - # We're just going to call into our wrapped view and see if we get any - # errors out of it. If we do, and they're one of the kinds of errors - # that we know how to deal with, then we'll catch it and do our translation - # into the error response that the legacy upload API expects. - try: - return wrapped(context, request) - except ForkliftError as exc: - # All errors in the upload API should emit metrics, so we'll go - # ahead and do that first. - request.metrics.increment("warehouse.upload.failed", tags=exc.tags) - - # Use the registered http exception type to raise a new error, rendered - # with the mechanisms that the legacy upload API expects. - # - # We'll use `from exc` so that our traceback properly records the - # original source of the ForkliftException that caused this. - raise exc.as_http_exception(request) from exc - except Exception: - # If we get any other kind of exception, then we'll mark a failed - # upload as well before re-raising that exception to be handled up - # the stack. - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:unhandled-error"] - ) - raise - - return wrapper +@exception_view_config(ForkliftError, route_name="forklift.legacy.file_upload") +def forklift_error(exc, request): + request.metrics.increment("warehouse.upload.failed", tags=exc.tags) + return exc.as_http_exception(request) diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 168cd63b1fca..9f777cdb9752 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -43,8 +43,12 @@ ) from warehouse.events.tags import EventTag from warehouse.forklift import metadata -from warehouse.forklift.decorators import ensure_uploads_allowed, sanitize -from warehouse.forklift.errors import ForkliftError, translate_errors, _exc_with_message +from warehouse.forklift.decorators import ( + ensure_uploads_allowed, + sanitize, + upload_metrics, +) +from warehouse.forklift.errors import ForkliftError, _exc_with_message from warehouse.forklift.forms import UploadForm, _filetype_extension_mapping from warehouse.macaroons.models import Macaroon from warehouse.packaging.interfaces import IFileStorage, IProjectService @@ -223,7 +227,10 @@ class InvalidFilename( class UnnormalizedFilename( ForkliftError, - message="Filename for {project!r} should contain the normalized project name {normalized!r}.", + message=( + "Filename for {project!r} should contain the normalized project " + "name {normalized!r}." + ), tags={"reason:invalid-filename-normalized", "filetype:{filetype}"}, ): pass @@ -247,7 +254,10 @@ class InvalidPlatformTag( class MissingRecordFile( ForkliftError, - message="Wheel {filename!r} does not contain the required RECORD file: {record_filename}.", + message=( + "Wheel {filename!r} does not contain the required RECORD file: " + "{record_filename}." + ), tags={"reason:missing-record-file", "filetype:{filetype}"}, ): pass @@ -255,7 +265,10 @@ class MissingRecordFile( class MissingMetadataFile( ForkliftError, - message="Wheel {filename!r} does not contain the required METADATA file: {metadata_filename}.", + message=( + "Wheel {filename!r} does not contain the required METADATA file: " + "{metadata_filename}." + ), tags={"reason:missing-metadata-file", "filetype:bdist_wheel"}, ): pass @@ -291,7 +304,9 @@ class FilenameTooLong( class DigestMismatch( ForkliftError, - message="The digest supplied does not match a digest calculated from the uploaded file.", + message=( + "The digest supplied does not match a digest calculated from the uploaded file." + ), tags={"reason:digest-mismatch"}, ): pass @@ -816,10 +831,7 @@ def _ensure_user_can_upload(request: Request) -> None: require_methods=["POST"], has_translations=True, permit_duplicate_post_keys=True, - # NOTE: translate_errors must be first, so that it is the "outer" decorator - # and can translate errors from the other decorators, as well as the - # file_upload view itself. - decorator=[translate_errors, sanitize, ensure_uploads_allowed], + decorator=[upload_metrics, sanitize, ensure_uploads_allowed], ) def file_upload(request): # This is a list of warnings that we'll emit *IF* the request is successful. @@ -1404,7 +1416,7 @@ def file_upload(request): name, version, ___, tags = packaging.utils.parse_wheel_filename( filename ) - except packaging.utils.InvalidWheelFilename as e: + except packaging.utils.InvalidWheelFilename: raise InvalidFilename(filename=filename, filetype=form.filetype.data) for tag in tags: @@ -1735,13 +1747,6 @@ def file_upload(request): if request.registry.settings.get("warehouse.release_files_table") is not None: request.task(update_bigquery_release_files).delay(dist_metadata) - # Log a successful upload - # TODO: This would ideally be located near the other metrics calls, but that - # happens at a layer that doesn't have the filetype data. - request.metrics.increment( - "warehouse.upload.ok", tags=[f"filetype:{form.filetype.data}"] - ) - # Dispatch our task to sync this to cache as soon as possible request.task(sync_file_to_cache).delay(file_.id) From 95f437f6ad2a1fd724e50dd98ca7e471f9292af3 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sat, 25 Apr 2026 21:51:46 -0400 Subject: [PATCH 04/12] more work --- tests/unit/forklift/test_legacy.py | 38 ++++++++++++++++++++++++++++++ warehouse/forklift/errors.py | 6 ----- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 88f03cb0184d..bdbda7d474a3 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -30,6 +30,7 @@ from warehouse.accounts.utils import UserContext from warehouse.attestations.interfaces import IIntegrityService from warehouse.classifiers.models import Classifier +from warehouse.errors import WarehouseDenied from warehouse.forklift import legacy, metadata from warehouse.macaroons import IMacaroonService, caveats, security_policy from warehouse.metrics import IMetricsService @@ -2371,6 +2372,43 @@ def test_upload_fails_without_user_permission(self, pyramid_config, db_request): "username": user2.username, } + def test_upload_fails_with_permission_reason(self, pyramid_config, db_request): + user1 = UserFactory.create() + EmailFactory.create(user=user1) + user2 = UserFactory.create() + EmailFactory.create(user=user2) + project = ProjectFactory.create() + release = ReleaseFactory.create(project=project, version="1.0") + RoleFactory.create(user=user1, project=project) + + filename = "{}-{}.tar.gz".format( + project.normalized_name.replace("-", "_"), release.version + ) + + pyramid_config.testing_securitypolicy(identity=user2, permissive=False) + db_request.user = user2 + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": project.name, + "version": release.version, + "filetype": "sdist", + "md5_digest": "nope!", + "content": pretend.stub( + filename=filename, + file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), + type="application/tar", + ), + } + ) + db_request.has_permission = lambda perm, ctx: WarehouseDenied( + "bad stuff", reason="a reason for stuff" + ) + + with pytest.raises(legacy.PermissionDenied) as excinfo: + legacy.file_upload(db_request) + assert excinfo.value.values == {"msg": "bad stuff"} + def test_upload_fails_without_oidc_publisher_permission( self, pyramid_config, db_request ): diff --git a/warehouse/forklift/errors.py b/warehouse/forklift/errors.py index 187563b702db..8912fe0f560c 100644 --- a/warehouse/forklift/errors.py +++ b/warehouse/forklift/errors.py @@ -107,12 +107,6 @@ def __init__(self, *args, **kwargs): super().__init__(self.message, *args) def as_http_exception(self, request): - # TODO: Is this something we still need to worry about? - if not self.message: - sentry_sdk.capture_message( - "Attempting to _exc_with_message without a message" - ) - # Append the standard "see X for more info" message to our message if there # a help url associated with this error. # TODO: Handle _help_url, and multiples and _user_docs_anchor, _user_docs_path From a9d12228212974f6a396b1112c8aba0bcebf9539 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sat, 25 Apr 2026 22:51:16 -0400 Subject: [PATCH 05/12] rate limiting --- tests/unit/forklift/test_legacy.py | 9 ++------- warehouse/forklift/legacy.py | 15 ++++++++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index bdbda7d474a3..a8010f5420c6 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -19,7 +19,7 @@ import pytest from pypi_attestations import Attestation, Envelope, VerificationMaterial -from pyramid.httpexceptions import HTTPBadRequest, HTTPTooManyRequests +from pyramid.httpexceptions import HTTPBadRequest from sqlalchemy import and_, event, exists from sqlalchemy.orm import joinedload from trove_classifiers import classifiers @@ -4732,14 +4732,9 @@ def test_upload_new_project_fails_ratelimited( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(HTTPTooManyRequests) as excinfo: + with pytest.raises(legacy.ProjectCreationRateLimited): legacy.file_upload(db_request) - resp = excinfo.value - - assert resp.status_code == 429 - assert resp.status == ("429 Too many new projects created") - def test_upload_succeeds_creates_project( self, pyramid_config, db_request, project_service ): diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 9f777cdb9752..ece6b2453b41 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -377,6 +377,15 @@ class InvalidAttestations( pass +class ProjectCreationRateLimited( + ForkliftError, + error_type=HTTPTooManyRequests, + message="Too many new projects created", + tags={"reason:rate-limited"}, +): + pass + + # Wheel platform checking # Note: defining new platform ABI compatibility tags that don't @@ -970,11 +979,7 @@ def file_upload(request): ) raise _exc_with_message(exc.__class__, exc.detail) from None except RateLimiterException: - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:rate-limited"] - ) - msg = "Too many new projects created" - raise _exc_with_message(HTTPTooManyRequests, msg) + raise ProjectCreationRateLimited # Check that the identity has permission to do things to this project, if this # is a new project this will act as a sanity check for the role we just From 3eb9ef3667347eabe6772e8427d274222aa10378 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sat, 25 Apr 2026 23:00:33 -0400 Subject: [PATCH 06/12] make a note --- warehouse/forklift/legacy.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index ece6b2453b41..a68ea51a3008 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -973,6 +973,11 @@ def file_upload(request): project = project_service.create_project( form.name.data, request.user, request ) + # TODO: The project service raises a lot of different HTTPExceptions, ideally + # it wouldn't do that and would raise regular exceptions, and it would + # be up to the handlers to translate those. However, changing that would + # require changes to several areas of warehouse, so we'll leave that for + # a future change. except HTTPException as exc: request.metrics.increment( "warehouse.upload.failed", tags=["reason:project-creation-failed"] From f5aaad9d7aab9bdba46d0f3d1cdef85d5d11db0d Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sun, 26 Apr 2026 01:18:38 -0400 Subject: [PATCH 07/12] more work --- tests/unit/forklift/test_legacy.py | 184 ++++++++++++----------------- warehouse/forklift/legacy.py | 94 ++++++++------- warehouse/forklift/metadata.py | 6 +- 3 files changed, 132 insertions(+), 152 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index a8010f5420c6..61c85b0c991a 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -654,7 +654,7 @@ def test_fails_invalid_version(self, version): assert excinfo.value.values == {"version": version} @pytest.mark.parametrize( - ("post_data", "message"), + ("post_data", "error_type", "expected"), [ # metadata_version errors. ( @@ -665,9 +665,8 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "pyversion": "source", }, - "None is not a valid metadata version. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + legacy.InvalidCoreMetadata, + {"msg": "None is not a valid metadata version."}, ), ( { @@ -678,27 +677,25 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "pyversion": "source", }, - "'-1' is not a valid metadata version. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + legacy.InvalidCoreMetadata, + {"msg": "'-1' is not a valid metadata version."}, ), # name errors. ( - {"metadata_version": "1.2"}, - "'' is an invalid value for Name. " - "Error: This field is required. " - "See " - "https://packaging.python.org/specifications/core-metadata" - " for more information.", + {}, + legacy.InvalidCoreMetadata, + {"msg": "None is an invalid value for Name: This field is required."}, ), ( - {"metadata_version": "1.2", "name": "foo-"}, - "'foo-' is an invalid value for Name. " - "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.", + {"name": "foo-"}, + legacy.InvalidCoreMetadata, + { + "msg": ( + "'foo-' is an invalid value for Name: Start and end with a " + "letter or numeral containing only ASCII numeric and '.', '_' " + "and '-'." + ), + }, ), # version errors. ( @@ -709,9 +706,8 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - "'version' is a required field. See " - "https://packaging.python.org/specifications/core-metadata for " - "more information.", + legacy.InvalidCoreMetadata, + {"msg": "'version' is a required field."}, ), ( { @@ -721,9 +717,8 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - "'dog' is invalid for 'version'. See " - "https://packaging.python.org/specifications/core-metadata for " - "more information.", + legacy.InvalidCoreMetadata, + {"msg": "'dog' is invalid for 'version'."}, ), ( { @@ -733,74 +728,72 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - "'1.0.dev.a1' is invalid for 'version'. See " - "https://packaging.python.org/specifications/core-metadata for " - "more information.", + legacy.InvalidCoreMetadata, + {"msg": "'1.0.dev.a1' is invalid for 'version'."}, ), # filetype/pyversion errors. ( { - "metadata_version": "1.2", "name": "example", "version": "1.0", "md5_digest": "bad", }, - "Invalid value for filetype. Error: This field is required.", + legacy.InvalidUploadMetadata, + {"field": "filetype", "msg": "This field is required."}, ), ( { - "metadata_version": "1.2", "name": "example", - "version": "1.0", "filetype": "bdist_wheel", - "content": "fake binary content", }, - "Invalid value for pyversion. " - "Error: Python version is required for binary distribution uploads.", + legacy.InvalidUploadMetadata, + { + "field": "pyversion", + "msg": "Python version is required for binary distribution uploads.", + }, ), ( { - "metadata_version": "1.2", "name": "example", - "version": "1.0", "filetype": "bdist_wat", "pyversion": "1.0", "md5_digest": "bad", }, - "Invalid value for filetype. Error: Use a known file type.", + legacy.InvalidUploadMetadata, + {"field": "filetype", "msg": "Use a known file type."}, ), ( { - "metadata_version": "1.2", "name": "example", - "version": "1.0", "filetype": "sdist", "pyversion": "1.0", }, - "Invalid value for pyversion. " - "Error: Use 'source' as Python version for an sdist.", + legacy.InvalidUploadMetadata, + { + "field": "pyversion", + "msg": "Use 'source' as Python version for an sdist.", + }, ), # digest errors. ( { - "metadata_version": "1.2", "name": "example", - "version": "1.0", "filetype": "sdist", - "content": "fake binary content", }, - "Error: Include at least one message digest.", + legacy.MissingUploadMetadata, + {"msg": "Include at least one message digest."}, ), ( { - "metadata_version": "1.2", "name": "example", - "version": "1.0", "filetype": "sdist", "sha256_digest": "an invalid sha256 digest", }, - "Invalid value for sha256_digest. " - "Error: Use a valid, hex-encoded, SHA256 message digest.", + legacy.InvalidUploadMetadata, + { + "field": "sha256_digest", + "msg": "Use a valid, hex-encoded, SHA256 message digest.", + }, ), # summary errors ( @@ -812,9 +805,8 @@ def test_fails_invalid_version(self, version): "md5_digest": "a fake md5 digest", "summary": "A" * 513, }, - "'summary' field must be 512 characters or less. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + legacy.InvalidCoreMetadata, + {"msg": "'summary' field must be 512 characters or less."}, ), ( { @@ -825,9 +817,8 @@ def test_fails_invalid_version(self, version): "md5_digest": "a fake md5 digest", "summary": "A\nB", }, - "'summary' must be a single line. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + legacy.InvalidCoreMetadata, + {"msg": "'summary' must be a single line."}, ), # local version error ( @@ -838,15 +829,14 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - "The use of local versions in '1.0+local' is not allowed. " - "See https://packaging.python.org/en/latest/specifications/" - "version-specifiers/#local-version-identifiers for more information.", + legacy.InvalidLocalVersion, + {"msg": "The use of local versions in '1.0+local' is not allowed."}, ), ], ) @pytest.mark.filterwarnings("ignore:Creating a LegacyVersion.*:DeprecationWarning") def test_fails_invalid_post_data( - self, pyramid_config, db_request, post_data, message + self, pyramid_config, db_request, post_data, error_type, expected ): user = UserFactory.create() EmailFactory.create(user=user) @@ -854,13 +844,9 @@ def test_fails_invalid_post_data( db_request.user = user db_request.POST = MultiDict(post_data) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(error_type) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == f"400 {message}" + assert excinfo.value.values == expected @pytest.mark.parametrize("name", ["requirements.txt", "rrequirements.txt"]) def test_fails_with_invalid_names(self, pyramid_config, db_request, name): @@ -1277,16 +1263,12 @@ def test_upload_fails_with_legacy_type(self, pyramid_config, db_request): } ) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidUploadMetadata) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert ( - resp.status - == "400 Invalid value for filetype. Error: Use a known file type." - ) + assert excinfo.value.values == { + "field": "filetype", + "msg": "Use a known file type.", + } def test_upload_fails_with_legacy_ext(self, pyramid_config, db_request): user = UserFactory.create() @@ -1384,33 +1366,23 @@ def test_upload_fails_with_invalid_classifier(self, pyramid_config, db_request): ) db_request.POST.extend([("classifiers", "Invalid :: Classifier")]) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 'Invalid :: Classifier' is not a valid classifier. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information." - ) + assert excinfo.value.values == { + "msg": "'Invalid :: Classifier' is not a valid classifier." + } @pytest.mark.parametrize( ("deprecated_classifiers", "expected"), [ ( {"AA :: BB": ["CC :: DD"]}, - "400 The classifier 'AA :: BB' has been deprecated, use one of " - "['CC :: DD'] instead. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + "The classifier 'AA :: BB' has been deprecated, use one of " + "['CC :: DD'] instead.", ), ( {"AA :: BB": []}, - "400 The classifier 'AA :: BB' has been deprecated. See " - "https://packaging.python.org/specifications/core-metadata for more " - "information.", + "The classifier 'AA :: BB' has been deprecated.", ), ], ) @@ -1452,13 +1424,9 @@ def test_upload_fails_with_deprecated_classifier( db_request.POST.extend([("classifiers", classifier.classifier)]) db_request.route_url = pretend.call_recorder(lambda *a, **kw: "/url") - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == expected + assert excinfo.value.values == {"msg": expected} @pytest.mark.parametrize( "digests", @@ -5511,18 +5479,14 @@ def test_upload_fails_when_license_and_license_expression_are_present( IFileStorage: storage_service, }.get(svc) - with pytest.raises(HTTPBadRequest) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert resp.status == ( - "400 License is deprecated when License-Expression is present. " - "Only License-Expression should be present. " - "See https://packaging.python.org/specifications/core-metadata " - "for more information." - ) + assert excinfo.value.values == { + "msg": ( + "License is deprecated when License-Expression is present. Only " + "License-Expression should be present." + ) + } def test_upload_for_organization_owned_project_succeeds( self, pyramid_config, db_request, monkeypatch diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index a68ea51a3008..23aa5a809e77 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -386,6 +386,41 @@ class ProjectCreationRateLimited( pass +class InvalidCoreMetadata( + ForkliftError, + message="{msg}", + help_url="https://packaging.python.org/specifications/core-metadata", + tags={"reason:invalid-metadata"}, +): + pass + + +class InvalidLocalVersion( + ForkliftError, + message="{msg}", + help_url=( + "https://packaging.python.org/en/latest/specifications/" + "version-specifiers/#local-version-identifiers" + ), + tags={"reason:invalid-metadata"}, +): + pass + + +class InvalidUploadMetadata( + ForkliftError, + message="Invalid value for {field}. Error: {msg}", + tags={"reason:invalid-form-data"}, +): + pass + + +class MissingUploadMetadata( + ForkliftError, message="Error: {msg}", tags={"reason:invalid-form-data"} +): + pass + + # Wheel platform checking # Note: defining new platform ABI compatibility tags that don't @@ -855,41 +890,32 @@ def file_upload(request): form = UploadForm(request.POST) if not form.validate(): + # We'll only return a single field's worth of errors, so we'll see if our more + # important fields are involved in the errors, and if so we'll select that + # field as the one whose error we'll return. for field_name in _error_message_order: if field_name in form.errors: break + # Otherwise we'll just take the first field alphabetically as the field that + # we'll return an error from. else: field_name = sorted(form.errors.keys())[0] if field_name in form: field = form[field_name] - if field.description and isinstance(field, wtforms.StringField): - error_message = ( - "{value!r} is an invalid value for {field}. ".format( - value=( - field.data[:30] + "..." + field.data[-30:] - if field.data and len(field.data) > 60 - else field.data or "" - ), - field=field.description, - ) - + f"Error: {form.errors[field_name][0]} " - + "See " - "https://packaging.python.org/specifications/core-metadata" - + " for more information." + if field.description: + raise InvalidCoreMetadata( + msg=( + f"{field.data!r} is an invalid value for " + f"{field.description}: {form.errors[field_name][0]}" + ), ) else: - error_message = ( - f"Invalid value for {field_name}. Error: " - f"{form.errors[field_name][0]}" + raise InvalidUploadMetadata( + field=field_name, msg=form.errors[field_name][0] ) else: - error_message = f"Error: {form.errors[field_name][0]}" - - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:invalid-form-data"] - ) - raise _exc_with_message(HTTPBadRequest, error_message) + raise MissingUploadMetadata(msg=form.errors[field_name][0]) # Get a validated Metadata object from the form data. # TODO: We should eventually extract this data out of the artifact and use that, @@ -914,24 +940,10 @@ def file_upload(request): # for that field error = errors[field_name][0] error_msg = str(error) - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:invalid-metadata"] - ) - _see_url = ( - "https://packaging.python.org/en/latest/specifications/" - "version-specifiers/#local-version-identifiers" - if field_name == "version" - and any("use of local versions" in str(e) for e in errors["version"]) - else "https://packaging.python.org/specifications/core-metadata" - ) - raise _exc_with_message( - HTTPBadRequest, - " ".join( - [ - error_msg + ("." if not error_msg.endswith(".") else ""), - f"See {_see_url} for more information.", - ] - ), + if isinstance(error, metadata.InvalidLocalVersion): + raise InvalidLocalVersion(msg=error_msg) + raise InvalidCoreMetadata( + msg=error_msg + ("." if not error_msg.endswith(".") else "") ) # Set a statement timeout for this connection to prevent long-running diff --git a/warehouse/forklift/metadata.py b/warehouse/forklift/metadata.py index cdbe895e1328..2fb18822e371 100644 --- a/warehouse/forklift/metadata.py +++ b/warehouse/forklift/metadata.py @@ -70,6 +70,10 @@ class NoMetadataError(Exception): pass +class InvalidLocalVersion(InvalidMetadata): + pass + + def parse( content: bytes | None, *, form_data: MultiDict | None = None, backfill: bool = False ) -> Metadata: @@ -115,7 +119,7 @@ def _validate_metadata(metadata: Metadata, *, backfill: bool = False): # We don't allow the use of the "local version" field when releasing to PyPI if metadata.version.local: errors.append( - InvalidMetadata( + InvalidLocalVersion( "version", f"The use of local versions in '{metadata.version}' is not allowed.", ) From 018ba23f7382c3ed77422b453841be83c3154943 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sun, 26 Apr 2026 08:51:31 -0400 Subject: [PATCH 08/12] lint --- tests/unit/forklift/test_legacy.py | 4 +++- warehouse/forklift/legacy.py | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 61c85b0c991a..54008d8eefdf 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -749,7 +749,9 @@ def test_fails_invalid_version(self, version): legacy.InvalidUploadMetadata, { "field": "pyversion", - "msg": "Python version is required for binary distribution uploads.", + "msg": ( + "Python version is required for binary distribution uploads." + ), }, ), ( diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 23aa5a809e77..e535bd27ae74 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -13,12 +13,9 @@ import packaging.version import packaging_legacy.version import sentry_sdk -import wtforms -import wtforms.validators from pypi_attestations import Attestation, Distribution from pyramid.httpexceptions import ( - HTTPBadRequest, HTTPException, HTTPForbidden, HTTPGone, From 9f85b92e9c79196e3d97f842984be6163ebfca5c Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Sun, 26 Apr 2026 13:35:43 -0400 Subject: [PATCH 09/12] add more tests --- tests/unit/forklift/test_decorators.py | 65 ++++++++++++++++++++++++++ warehouse/forklift/legacy.py | 2 - 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/tests/unit/forklift/test_decorators.py b/tests/unit/forklift/test_decorators.py index 21fefe3a6ffc..46edbcad68b5 100644 --- a/tests/unit/forklift/test_decorators.py +++ b/tests/unit/forklift/test_decorators.py @@ -12,6 +12,71 @@ from warehouse.forklift import decorators +class TestUploadMetrics: + @pytest.mark.parametrize("filetype", [None, "bdist_foo"]) + def test_successful_view(self, filetype): + m = pretend.stub(increment=pretend.call_recorder(lambda n, tags=None: None)) + request = pretend.stub(metrics=m, POST={"filetype": filetype}) + response = pretend.stub() + + @decorators.upload_metrics + def wrapped(context, request): + return response + + tags = {f"filetype:{filetype}"} if filetype else set() + + assert wrapped(pretend.stub(), request) is response + assert request.metrics.increment.calls == [ + pretend.call("warehouse.upload.attempt", tags=tags), + pretend.call("warehouse.upload.ok", tags=tags), + ] + + @pytest.mark.parametrize("filetype", [None, "bdist_foo"]) + def test_forklift_error(self, filetype): + m = pretend.stub(increment=pretend.call_recorder(lambda n, tags=None: None)) + request = pretend.stub(metrics=m, POST={"filetype": filetype}) + + @decorators.upload_metrics + def wrapped(context, request): + raise decorators.InvalidTupleField(field="foo") + + with pytest.raises(decorators.InvalidTupleField): + wrapped(pretend.stub(), request) + + tags = {f"filetype:{filetype}"} if filetype else set() + + assert request.metrics.increment.calls == [ + pretend.call("warehouse.upload.attempt", tags=tags), + pretend.call( + "warehouse.upload.failed", tags={"reason:field-is-tuple", "field:foo"} + ), + ] + + @pytest.mark.parametrize("filetype", [None, "bdist_foo"]) + def test_unknown_error(self, filetype): + m = pretend.stub(increment=pretend.call_recorder(lambda n, tags=None: None)) + request = pretend.stub(metrics=m, POST={"filetype": filetype}) + + class SomeError(Exception): + pass + + @decorators.upload_metrics + def wrapped(context, request): + raise SomeError("blah") + + with pytest.raises(SomeError): + wrapped(pretend.stub(), request) + + tags = {f"filetype:{filetype}"} if filetype else set() + + assert request.metrics.increment.calls == [ + pretend.call("warehouse.upload.attempt", tags=tags), + pretend.call( + "warehouse.upload.failed", tags={"reason:unknown-error"} | tags + ), + ] + + class TestSanitizeRequest: def test_removes_unknowns(self): req = DummyRequest( diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index e535bd27ae74..afd3635282e6 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -7,8 +7,6 @@ import tempfile import zipfile -import packaging.requirements -import packaging.specifiers import packaging.utils import packaging.version import packaging_legacy.version From 831c6d4ead1c2e16ed7857cbd376a667ba818b4a Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Thu, 30 Apr 2026 00:01:13 -0400 Subject: [PATCH 10/12] lint / fmt --- tests/unit/forklift/test_decorators.py | 16 +-- tests/unit/forklift/test_legacy.py | 115 +++++++++--------- warehouse/forklift/decorators.py | 28 ++--- warehouse/forklift/errors.py | 9 +- warehouse/forklift/legacy.py | 157 +++++++++++++------------ 5 files changed, 164 insertions(+), 161 deletions(-) diff --git a/tests/unit/forklift/test_decorators.py b/tests/unit/forklift/test_decorators.py index 46edbcad68b5..0550c95f1a42 100644 --- a/tests/unit/forklift/test_decorators.py +++ b/tests/unit/forklift/test_decorators.py @@ -38,9 +38,9 @@ def test_forklift_error(self, filetype): @decorators.upload_metrics def wrapped(context, request): - raise decorators.InvalidTupleField(field="foo") + raise decorators.InvalidTupleFieldError(field="foo") - with pytest.raises(decorators.InvalidTupleField): + with pytest.raises(decorators.InvalidTupleFieldError): wrapped(pretend.stub(), request) tags = {f"filetype:{filetype}"} if filetype else set() @@ -129,7 +129,7 @@ def test_fails_with_fieldstorage(self): def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(decorators.InvalidTupleField) as excinfo: + with pytest.raises(decorators.InvalidTupleFieldError) as excinfo: wrapped(pretend.stub(), req) assert excinfo.value.values == {"field": "keywords"} @@ -141,7 +141,7 @@ def test_fails_without_file(self): def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(decorators.NoFileUpload): + with pytest.raises(decorators.NoFileUploadError): wrapped(pretend.stub(), req) @pytest.mark.parametrize("content_type", [None, "image/foobar"]) @@ -152,7 +152,7 @@ def test_fails_invalid_content_type(self, content_type): def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(decorators.InvalidContentType): + with pytest.raises(decorators.InvalidContentTypeError): wrapped(pretend.stub(), req) @@ -190,11 +190,11 @@ def wrapped(context, request): [ ( AdminFlagValue.READ_ONLY, - decorators.ReadOnlyEnabled, + decorators.ReadOnlyError, ), ( AdminFlagValue.DISALLOW_NEW_UPLOAD, - decorators.UploadsDisabled, + decorators.UploadsDisabledError, ), ], ) @@ -217,5 +217,5 @@ def test_fails_without_identity(self): def wrapped(context, request): pytest.fail("wrapped view should not have been called") - with pytest.raises(decorators.MissingIdentity): + with pytest.raises(decorators.MissingIdentityError): wrapped(pretend.stub(), req) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 54008d8eefdf..bca9f38d5763 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -11,7 +11,6 @@ from cgi import FieldStorage from textwrap import dedent -from types import SimpleNamespace from unittest import mock import pretend @@ -649,7 +648,7 @@ def test_is_duplicate_false(self, pyramid_config, db_request): class TestFileUpload: @pytest.mark.parametrize("version", ["2", "3", "-1", "0", "dog", "cat"]) def test_fails_invalid_version(self, version): - with pytest.raises(legacy.InvalidProtocolVersion) as excinfo: + with pytest.raises(legacy.InvalidProtocolVersionError) as excinfo: legacy.file_upload(pretend.stub(POST={"protocol_version": version})) assert excinfo.value.values == {"version": version} @@ -665,7 +664,7 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "pyversion": "source", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "None is not a valid metadata version."}, ), ( @@ -677,18 +676,18 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "pyversion": "source", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'-1' is not a valid metadata version."}, ), # name errors. ( {}, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "None is an invalid value for Name: This field is required."}, ), ( {"name": "foo-"}, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, { "msg": ( "'foo-' is an invalid value for Name: Start and end with a " @@ -706,7 +705,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'version' is a required field."}, ), ( @@ -717,7 +716,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'dog' is invalid for 'version'."}, ), ( @@ -728,7 +727,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'1.0.dev.a1' is invalid for 'version'."}, ), # filetype/pyversion errors. @@ -738,7 +737,7 @@ def test_fails_invalid_version(self, version): "version": "1.0", "md5_digest": "bad", }, - legacy.InvalidUploadMetadata, + legacy.InvalidUploadMetadataError, {"field": "filetype", "msg": "This field is required."}, ), ( @@ -746,7 +745,7 @@ def test_fails_invalid_version(self, version): "name": "example", "filetype": "bdist_wheel", }, - legacy.InvalidUploadMetadata, + legacy.InvalidUploadMetadataError, { "field": "pyversion", "msg": ( @@ -761,7 +760,7 @@ def test_fails_invalid_version(self, version): "pyversion": "1.0", "md5_digest": "bad", }, - legacy.InvalidUploadMetadata, + legacy.InvalidUploadMetadataError, {"field": "filetype", "msg": "Use a known file type."}, ), ( @@ -770,7 +769,7 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "pyversion": "1.0", }, - legacy.InvalidUploadMetadata, + legacy.InvalidUploadMetadataError, { "field": "pyversion", "msg": "Use 'source' as Python version for an sdist.", @@ -782,7 +781,7 @@ def test_fails_invalid_version(self, version): "name": "example", "filetype": "sdist", }, - legacy.MissingUploadMetadata, + legacy.MissingUploadMetadataError, {"msg": "Include at least one message digest."}, ), ( @@ -791,7 +790,7 @@ def test_fails_invalid_version(self, version): "filetype": "sdist", "sha256_digest": "an invalid sha256 digest", }, - legacy.InvalidUploadMetadata, + legacy.InvalidUploadMetadataError, { "field": "sha256_digest", "msg": "Use a valid, hex-encoded, SHA256 message digest.", @@ -807,7 +806,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "a fake md5 digest", "summary": "A" * 513, }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'summary' field must be 512 characters or less."}, ), ( @@ -819,7 +818,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "a fake md5 digest", "summary": "A\nB", }, - legacy.InvalidCoreMetadata, + legacy.InvalidCoreMetadataError, {"msg": "'summary' must be a single line."}, ), # local version error @@ -831,7 +830,7 @@ def test_fails_invalid_version(self, version): "md5_digest": "bad", "filetype": "sdist", }, - legacy.InvalidLocalVersion, + legacy.InvalidLocalVersionError, {"msg": "The use of local versions in '1.0+local' is not allowed."}, ), ], @@ -969,7 +968,7 @@ def test_fails_invalid_render( if description_content_type is not None: db_request.POST.add("description_content_type", description_content_type) - with pytest.raises(legacy.InvalidDescription) as excinfo: + with pytest.raises(legacy.InvalidDescriptionError) as excinfo: legacy.file_upload(db_request) excinfo.value.values == { @@ -1265,7 +1264,7 @@ def test_upload_fails_with_legacy_type(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.InvalidUploadMetadata) as excinfo: + with pytest.raises(legacy.InvalidUploadMetadataError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "field": "filetype", @@ -1300,7 +1299,7 @@ def test_upload_fails_with_legacy_ext(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.UnknownFileExtension): + with pytest.raises(legacy.UnknownFileExtensionError): legacy.file_upload(db_request) def test_upload_fails_for_second_sdist(self, pyramid_config, db_request): @@ -1336,7 +1335,7 @@ def test_upload_fails_for_second_sdist(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.DuplicateSDist): + with pytest.raises(legacy.DuplicateSDistError): legacy.file_upload(db_request) def test_upload_fails_with_invalid_classifier(self, pyramid_config, db_request): @@ -1368,7 +1367,7 @@ def test_upload_fails_with_invalid_classifier(self, pyramid_config, db_request): ) db_request.POST.extend([("classifiers", "Invalid :: Classifier")]) - with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadataError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "msg": "'Invalid :: Classifier' is not a valid classifier." @@ -1426,7 +1425,7 @@ def test_upload_fails_with_deprecated_classifier( db_request.POST.extend([("classifiers", classifier.classifier)]) db_request.route_url = pretend.call_recorder(lambda *a, **kw: "/url") - with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadataError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"msg": expected} @@ -1489,7 +1488,7 @@ def test_upload_fails_with_invalid_digest( ) db_request.POST.update(digests) - with pytest.raises(legacy.DigestMismatch): + with pytest.raises(legacy.DigestMismatchError): legacy.file_upload(db_request) def test_upload_fails_with_invalid_file(self, pyramid_config, db_request): @@ -1518,7 +1517,7 @@ def test_upload_fails_with_invalid_file(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.InvalidDistFile) as excinfo: + with pytest.raises(legacy.InvalidDistFileError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "msg": "File is not a zipfile", @@ -1562,7 +1561,7 @@ def test_upload_fails_end_of_file_error( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(legacy.InvalidDistFile) as excinfo: + with pytest.raises(legacy.InvalidDistFileError) as excinfo: legacy.file_upload(db_request) excinfo.value.values == {"msg": "File is not a tarfile"} @@ -1595,7 +1594,7 @@ def test_upload_fails_with_too_large_file(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.FileTooLarge) as excinfo: + with pytest.raises(legacy.FileTooLargeError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"project": project.name, "limit": 100} @@ -1633,7 +1632,7 @@ def test_upload_fails_with_too_large_project_size_default_limit( } ) - with pytest.raises(legacy.ProjectTooLarge) as excinfo: + with pytest.raises(legacy.ProjectTooLargeError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"project": project.name, "limit": 10} @@ -1676,7 +1675,7 @@ def test_upload_fails_with_too_large_project_size_custom_limit( } ) - with pytest.raises(legacy.ProjectTooLarge) as excinfo: + with pytest.raises(legacy.ProjectTooLargeError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"project": project.name, "limit": 10} @@ -1843,7 +1842,7 @@ def mock_open(file, mode="r", *args, **kwargs): monkeypatch.setattr("builtins.open", mock_open) - with pytest.raises(legacy.FilenameTooLong) as excinfo: + with pytest.raises(legacy.FilenameTooLongError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"filename": filename, "filetype": "bdist_wheel"} @@ -1878,7 +1877,7 @@ def test_upload_fails_with_previously_used_filename( db_request.db.add(Filename(filename=filename)) - with pytest.raises(legacy.FilenameReused): + with pytest.raises(legacy.FilenameReusedError): legacy.file_upload(db_request) def test_upload_noop_with_existing_filename_same_content( @@ -1973,7 +1972,7 @@ def test_upload_fails_with_existing_filename_diff_content( ) ) - with pytest.raises(legacy.FileAlreadyExists) as excinfo: + with pytest.raises(legacy.FileAlreadyExistsError) as excinfo: legacy.file_upload(db_request) excinfo.value.values == {"filename": filename, "blake2_256": blake2_256_digest} @@ -2023,7 +2022,7 @@ def test_upload_fails_with_diff_filename_same_blake2( ) ) - with pytest.raises(legacy.FileAlreadyExists) as excinfo: + with pytest.raises(legacy.FileAlreadyExistsError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "filename": f"{project.name}-fake.tar.gz", @@ -2092,7 +2091,7 @@ def test_upload_fails_with_wrong_filename_project_name( } ) - with pytest.raises(legacy.UnnormalizedFilename) as excinfo: + with pytest.raises(legacy.UnnormalizedFilenameError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "project": project.name, @@ -2105,12 +2104,12 @@ def test_upload_fails_with_wrong_filename_project_name( [ ( "wutang-6.6.6.tar.gz", - legacy.UnnormalizedFilename, + legacy.UnnormalizedFilenameError, {"project": "wutang", "normalized": "wutang", "filetype": "sdist"}, ), ( "wutang-6.6.6-py3-none-any.whl", - legacy.InvalidFilenameVersion, + legacy.InvalidFilenameVersionError, {"expected": "1.2.3", "filetype": "bdist_wheel", "found": "6.6.6"}, ), ], @@ -2202,7 +2201,7 @@ def test_upload_fails_with_invalid_filetype( } ) - with pytest.raises(legacy.InvalidFileExtension) as excinfo: + with pytest.raises(legacy.InvalidFileExtensionError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"extension": extension, "filetype": filetype} @@ -2234,7 +2233,7 @@ def test_upload_fails_with_invalid_extension(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.UnknownFileExtension): + with pytest.raises(legacy.UnknownFileExtensionError): legacy.file_upload(db_request) @pytest.mark.parametrize("character", ["/", "\\"]) @@ -2268,7 +2267,7 @@ def test_upload_fails_with_unsafe_filename( } ) - with pytest.raises(legacy.PathlikeFilename): + with pytest.raises(legacy.PathlikeFilenameError): legacy.file_upload(db_request) @pytest.mark.parametrize("character", [*(chr(x) for x in range(32)), chr(127)]) @@ -2302,7 +2301,7 @@ def test_upload_fails_with_disallowed_in_filename( } ) - with pytest.raises(legacy.UnprintableFilename): + with pytest.raises(legacy.UnprintableFilenameError): legacy.file_upload(db_request) def test_upload_fails_without_user_permission(self, pyramid_config, db_request): @@ -2335,7 +2334,7 @@ def test_upload_fails_without_user_permission(self, pyramid_config, db_request): } ) - with pytest.raises(legacy.UserPermissionDenied) as excinfo: + with pytest.raises(legacy.UserPermissionError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "project": project.name, @@ -2375,7 +2374,7 @@ def test_upload_fails_with_permission_reason(self, pyramid_config, db_request): "bad stuff", reason="a reason for stuff" ) - with pytest.raises(legacy.PermissionDenied) as excinfo: + with pytest.raises(legacy.PermissionError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"msg": "bad stuff"} @@ -2408,7 +2407,7 @@ def test_upload_fails_without_oidc_publisher_permission( } ) - with pytest.raises(legacy.TokenPermissionDenied) as excinfo: + with pytest.raises(legacy.TokenPermissionError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"project": project.name} @@ -2483,7 +2482,7 @@ def test_upload_attestation_fails_without_oidc_publisher( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(legacy.InvalidAttestations) as excinfo: + with pytest.raises(legacy.InvalidAttestationsError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "msg": "Attestations are only supported when using Trusted Publishing" @@ -3066,7 +3065,7 @@ def test_upload_fails_with_invalid_filename( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.InvalidFilename) as excinfo: + with pytest.raises(legacy.InvalidFilenameError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "filename": filename, @@ -3118,7 +3117,7 @@ def test_upload_fails_with_unsupported_wheel_plat( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.InvalidPlatformTag) as excinfo: + with pytest.raises(legacy.InvalidPlatformTagError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == {"filename": filename, "tag": expected} @@ -3163,7 +3162,7 @@ def test_upload_fails_with_missing_record_wheel( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.MissingRecordFile) as excinfo: + with pytest.raises(legacy.MissingRecordFileError) as excinfo: legacy.file_upload(db_request) expected_filename = project.normalized_name.replace("-", "_") @@ -3583,7 +3582,7 @@ def test_upload_fails_with_missing_metadata_wheel( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.MissingMetadataFile) as excinfo: + with pytest.raises(legacy.MissingMetadataFileError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "filename": filename, @@ -3987,7 +3986,7 @@ def test_upload_fails_attestation_error( ) monkeypatch.setattr(HasEvents, "record_event", record_event) - with pytest.raises(legacy.InvalidAttestations): + with pytest.raises(legacy.InvalidAttestationsError): legacy.file_upload(db_request) @pytest.mark.parametrize( @@ -4649,7 +4648,7 @@ def test_upload_fails_nonuser_identity_cannot_create_project( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(legacy.NonUserIdentity): + with pytest.raises(legacy.NonUserIdentityError): legacy.file_upload(db_request) @pytest.mark.parametrize( @@ -4702,7 +4701,7 @@ def test_upload_new_project_fails_ratelimited( }.get(svc) db_request.user_agent = "warehouse-tests/6.6.6" - with pytest.raises(legacy.ProjectCreationRateLimited): + with pytest.raises(legacy.ProjectCreationRateLimitedError): legacy.file_upload(db_request) def test_upload_succeeds_creates_project( @@ -5074,7 +5073,7 @@ def test_upload_fails_pep427( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.UnnormalizedFilename) as excinfo: + with pytest.raises(legacy.UnnormalizedFilenameError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "project": project.name, @@ -5141,7 +5140,7 @@ def test_upload_fails_pep625( legacy, "_is_valid_dist_file", lambda *a, **kw: (True, None) ) - with pytest.raises(legacy.UnnormalizedFilename) as excinfo: + with pytest.raises(legacy.UnnormalizedFilenameError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "project": project.name, @@ -5427,7 +5426,7 @@ def test_upload_fails_missing_license_file_metadata_2_4( IFileStorage: storage_service, }.get(svc) - with pytest.raises(legacy.MissingLicenseFile) as excinfo: + with pytest.raises(legacy.MissingLicenseFileError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "filename": filename, @@ -5481,7 +5480,7 @@ def test_upload_fails_when_license_and_license_expression_are_present( IFileStorage: storage_service, }.get(svc) - with pytest.raises(legacy.InvalidCoreMetadata) as excinfo: + with pytest.raises(legacy.InvalidCoreMetadataError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "msg": ( @@ -5588,7 +5587,7 @@ def test_upload_for_company_organization_owned_project_fails_without_subscriptio } ) - with pytest.raises(legacy.OrgInactive): + with pytest.raises(legacy.OrgInactiveError): legacy.file_upload(db_request) def test_upload_with_organization_file_size_limit_succeeds( @@ -6065,7 +6064,7 @@ def test_upload_fails_with_yara_match( } ) - with pytest.raises(legacy.InvalidDistFile) as excinfo: + with pytest.raises(legacy.InvalidDistFileError) as excinfo: legacy.file_upload(db_request) assert excinfo.value.values == { "msg": ( diff --git a/warehouse/forklift/decorators.py b/warehouse/forklift/decorators.py index 5e5bc9bd1911..838a1f482a84 100644 --- a/warehouse/forklift/decorators.py +++ b/warehouse/forklift/decorators.py @@ -8,7 +8,7 @@ from warehouse.forklift.errors import ForkliftError -class InvalidTupleField( +class InvalidTupleFieldError( ForkliftError, message="{field}: Should not be a tuple.", tags={"reason:field-is-tuple", "field:{field}"}, @@ -16,7 +16,7 @@ class InvalidTupleField( pass -class NoFileUpload( +class NoFileUploadError( ForkliftError, message="Upload payload does not have a file.", tags={"reason:no-file"}, @@ -24,7 +24,7 @@ class NoFileUpload( pass -class InvalidContentType( +class InvalidContentTypeError( ForkliftError, message="Invalid distribution file.", tags={"reason:invalid-content-type"}, @@ -32,7 +32,7 @@ class InvalidContentType( pass -class ReadOnlyEnabled( +class ReadOnlyError( ForkliftError, error_type=HTTPForbidden, message="Read-only mode: Uploads are temporarily disabled.", @@ -41,7 +41,7 @@ class ReadOnlyEnabled( pass -class UploadsDisabled( +class UploadsDisabledError( ForkliftError, error_type=HTTPForbidden, message="New uploads are temporarily disabled.", @@ -51,7 +51,7 @@ class UploadsDisabled( pass -class MissingIdentity( +class MissingIdentityError( ForkliftError, error_type=HTTPForbidden, message="Invalid or non-existent authentication information.", @@ -61,7 +61,7 @@ class MissingIdentity( pass -class UnverifiedEmail( +class UnverifiedEmailError( ForkliftError, error_type=HTTPForbidden, message=( @@ -75,7 +75,7 @@ class UnverifiedEmail( pass -class MissingTwoFactor( +class MissingTwoFactorError( ForkliftError, error_type=HTTPForbidden, message=( @@ -163,17 +163,17 @@ def wrapper(context, request): for field in set(request.POST) - {"content", "gpg_signature"}: values = request.POST.getall(field) if any(isinstance(value, cgi.FieldStorage) for value in values): - raise InvalidTupleField(field=field) + raise InvalidTupleFieldError(field=field) # Ensure that we have file data in the request. if "content" not in request.POST: - raise NoFileUpload + raise NoFileUploadError # Check the content type of what is being uploaded if not request.POST["content"].type or request.POST["content"].type.startswith( "image/" ): - raise InvalidContentType + raise InvalidContentTypeError # Otherwise, we'll just dispatch to our underlying view return wrapped(context, request) @@ -191,17 +191,17 @@ def wrapper(context, request): # The very first thing we want to check, is whether we're currently in # read only mode, because if we're in read only mode nothing else matters. if request.flags.enabled(AdminFlagValue.READ_ONLY): - raise ReadOnlyEnabled + raise ReadOnlyError # After that, we want to check if we're disallowing new uploads, which is # functionally the same as read only mode, but only for the upload endpoint. if request.flags.enabled(AdminFlagValue.DISALLOW_NEW_UPLOAD): - raise UploadsDisabled + raise UploadsDisabledError # Before we do anything else, if there isn't an authenticated identity with # this request, then we'll go ahead and bomb out. if request.identity is None: - raise MissingIdentity + raise MissingIdentityError # Otherwise, we'll just dispatch to our underlying view return wrapped(context, request) diff --git a/warehouse/forklift/errors.py b/warehouse/forklift/errors.py index 8912fe0f560c..1ae458fb9aee 100644 --- a/warehouse/forklift/errors.py +++ b/warehouse/forklift/errors.py @@ -33,7 +33,7 @@ class ForkliftError(Exception): _message_template: str _tag_templates: set[str] - _http_exception_type: HTTPException + _http_exception_type: type[HTTPException] _help_anchor: str | None _help_url: str | None _user_docs_path: str | None @@ -51,7 +51,7 @@ def __init_subclass__( *, message: str, tags: Iterable[str], - error_type=HTTPBadRequest, + error_type: type[HTTPException] = HTTPBadRequest, help_anchor=None, user_docs_path=None, user_docs_anchor=None, @@ -112,8 +112,9 @@ def as_http_exception(self, request): # TODO: Handle _help_url, and multiples and _user_docs_anchor, _user_docs_path message = self.message if self._help_anchor is not None: - message += " See {help_url} for more information.".format( - help_url=request.help_url(_anchor=self._help_anchor) + message += ( + f" See {request.help_url(_anchor=self._help_anchor)} for more " + "information." ) # Actually construct the "real" http error, using our now finalized message. diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index afd3635282e6..28659e0f8a68 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -87,7 +87,7 @@ MAX_DESCRIPTION_LENGTH_TO_BIGQUERY_IN_BYTES = 40000 -class InvalidProtocolVersion( +class InvalidProtocolVersionError( ForkliftError, message="Unknown protocol version: {version!r}", tags={"reason:unsupported-protocol-version"}, @@ -95,7 +95,7 @@ class InvalidProtocolVersion( pass -class NonUserIdentity( +class NonUserIdentityError( ForkliftError, error_type=HTTPForbidden, message=( @@ -111,7 +111,7 @@ class NonUserIdentity( pass -class UserPermissionDenied( +class UserPermissionError( ForkliftError, error_type=HTTPForbidden, message="The user {username!r} isn't allowed to upload to project {project!r}.", @@ -121,7 +121,7 @@ class UserPermissionDenied( pass -class TokenPermissionDenied( +class TokenPermissionError( ForkliftError, error_type=HTTPForbidden, message="The given token isn't allowed to upload to project {project!r}.", @@ -131,7 +131,7 @@ class TokenPermissionDenied( pass -class PermissionDenied( +class PermissionError( ForkliftError, error_type=HTTPForbidden, # NOTE: This looks wonky, but it's done so the permissions @@ -142,7 +142,7 @@ class PermissionDenied( pass -class OrgInactive( +class OrgInactiveError( ForkliftError, error_type=HTTPForbidden, message=( @@ -156,7 +156,7 @@ class OrgInactive( pass -class InvalidDescription( +class InvalidDescriptionError( ForkliftError, message="The description failed to render as {content_type!r}.", help_anchor="description-content-type", @@ -165,7 +165,7 @@ class InvalidDescription( pass -class UnprintableFilename( +class UnprintableFilenameError( ForkliftError, message=( "Cannot upload a file with non-printable " @@ -177,7 +177,7 @@ class UnprintableFilename( pass -class PathlikeFilename( +class PathlikeFilenameError( ForkliftError, message="Cannot upload a file with '/' or '\\' in the name.", tags={"reason:invalid-filename"}, @@ -185,7 +185,7 @@ class PathlikeFilename( pass -class InvalidFileExtension( +class InvalidFileExtensionError( ForkliftError, message=( "Invalid file extension: Extension {extension} is " @@ -198,7 +198,7 @@ class InvalidFileExtension( pass -class UnknownFileExtension( +class UnknownFileExtensionError( ForkliftError, message=( "Invalid file extension: Use .tar.gz, .whl or .zip " @@ -212,7 +212,7 @@ class UnknownFileExtension( pass -class InvalidFilename( +class InvalidFilenameError( ForkliftError, message="Invalid filename: {filename}", tags={"reason:invalid-filename", "filetype:{filetype}"}, @@ -220,7 +220,7 @@ class InvalidFilename( pass -class UnnormalizedFilename( +class UnnormalizedFilenameError( ForkliftError, message=( "Filename for {project!r} should contain the normalized project " @@ -231,7 +231,7 @@ class UnnormalizedFilename( pass -class InvalidFilenameVersion( +class InvalidFilenameVersionError( ForkliftError, message="Version in filename should be {expected!r} not {found!r}.", tags={"reason:invalid-filename-version", "filetype:{filetype}"}, @@ -239,7 +239,7 @@ class InvalidFilenameVersion( pass -class InvalidPlatformTag( +class InvalidPlatformTagError( ForkliftError, message="Binary wheel {filename!r} has an unsupported platform tag {tag!r}.", tags={"reason:unsupported-platform-tag"}, @@ -247,7 +247,7 @@ class InvalidPlatformTag( pass -class MissingRecordFile( +class MissingRecordFileError( ForkliftError, message=( "Wheel {filename!r} does not contain the required RECORD file: " @@ -258,7 +258,7 @@ class MissingRecordFile( pass -class MissingMetadataFile( +class MissingMetadataFileError( ForkliftError, message=( "Wheel {filename!r} does not contain the required METADATA file: " @@ -269,7 +269,7 @@ class MissingMetadataFile( pass -class FileTooLarge( +class FileTooLargeError( ForkliftError, message="File too large. Limit for project {project!r} is {limit}MB.", user_docs_path="/project-management/storage-limits", @@ -279,7 +279,7 @@ class FileTooLarge( pass -class ProjectTooLarge( +class ProjectTooLargeError( ForkliftError, message="Project size too large. Limit for project {project!r} is {limit}GB.", user_docs_path="/project-management/storage-limits", @@ -289,7 +289,7 @@ class ProjectTooLarge( pass -class FilenameTooLong( +class FilenameTooLongError( ForkliftError, message="Filename is too long: {filename!r}", tags={"reason:filename-too-long", "filetype:{filetype}"}, @@ -297,7 +297,7 @@ class FilenameTooLong( pass -class DigestMismatch( +class DigestMismatchError( ForkliftError, message=( "The digest supplied does not match a digest calculated from the uploaded file." @@ -307,7 +307,7 @@ class DigestMismatch( pass -class FileAlreadyExists( +class FileAlreadyExistsError( ForkliftError, # Note: Changing this error message to something that doesn't # start with "File already exists" will break the @@ -321,7 +321,7 @@ class FileAlreadyExists( pass -class FilenameReused( +class FilenameReusedError( ForkliftError, message=( "This filename was previously used by a file that has " @@ -333,7 +333,7 @@ class FilenameReused( pass -class DuplicateSDist( +class DuplicateSDistError( ForkliftError, message="Only one sdist may be uploaded per release.", tags={"reason:duplicate-sdist"}, @@ -341,7 +341,7 @@ class DuplicateSDist( pass -class InvalidDistFile( +class InvalidDistFileError( ForkliftError, message="Invalid distribution file: {msg}", tags={ @@ -353,7 +353,7 @@ class InvalidDistFile( pass -class MissingLicenseFile( +class MissingLicenseFileError( ForkliftError, message=( "License-File {license_file} does not exist in " @@ -364,7 +364,7 @@ class MissingLicenseFile( pass -class InvalidAttestations( +class InvalidAttestationsError( ForkliftError, message="Invalid attestations supplied during upload: {msg}", tags={"reason:invalid-attestations"}, @@ -372,7 +372,7 @@ class InvalidAttestations( pass -class ProjectCreationRateLimited( +class ProjectCreationRateLimitedError( ForkliftError, error_type=HTTPTooManyRequests, message="Too many new projects created", @@ -381,7 +381,7 @@ class ProjectCreationRateLimited( pass -class InvalidCoreMetadata( +class InvalidCoreMetadataError( ForkliftError, message="{msg}", help_url="https://packaging.python.org/specifications/core-metadata", @@ -390,7 +390,7 @@ class InvalidCoreMetadata( pass -class InvalidLocalVersion( +class InvalidLocalVersionError( ForkliftError, message="{msg}", help_url=( @@ -402,7 +402,7 @@ class InvalidLocalVersion( pass -class InvalidUploadMetadata( +class InvalidUploadMetadataError( ForkliftError, message="Invalid value for {field}. Error: {msg}", tags={"reason:invalid-form-data"}, @@ -410,7 +410,7 @@ class InvalidUploadMetadata( pass -class MissingUploadMetadata( +class MissingUploadMetadataError( ForkliftError, message="Error: {msg}", tags={"reason:invalid-form-data"} ): pass @@ -564,19 +564,19 @@ def _validate_filename(filename, filetype): # or completely by mistake. disallowed = [*(chr(x) for x in range(32)), chr(127)] if [char for char in filename if char in disallowed]: - raise UnprintableFilename + raise UnprintableFilenameError # Make sure that the filename does not contain any path separators. if "/" in filename or "\\" in filename: - raise PathlikeFilename + raise PathlikeFilenameError # Make sure the filename ends with an allowed extension. if m := _dist_file_re.match(filename): extension = m.group("extension") if extension not in _filetype_extension_mapping[filetype]: - raise InvalidFileExtension(extension=extension, filetype=filetype) + raise InvalidFileExtensionError(extension=extension, filetype=filetype) else: - raise UnknownFileExtension + raise UnknownFileExtensionError def _is_valid_dist_file(filename, filetype, metrics, *, scan=True): @@ -879,7 +879,7 @@ def file_upload(request): # We require protocol_version 1, it's the only supported version however # passing a different version should raise an error. if (version := request.POST.get("protocol_version", "1")) != "1": - raise InvalidProtocolVersion(version=version) + raise InvalidProtocolVersionError(version=version) # Validate and process the incoming file data. form = UploadForm(request.POST) @@ -899,18 +899,16 @@ def file_upload(request): if field_name in form: field = form[field_name] if field.description: - raise InvalidCoreMetadata( + raise InvalidCoreMetadataError( msg=( f"{field.data!r} is an invalid value for " f"{field.description}: {form.errors[field_name][0]}" ), ) - else: - raise InvalidUploadMetadata( - field=field_name, msg=form.errors[field_name][0] - ) - else: - raise MissingUploadMetadata(msg=form.errors[field_name][0]) + raise InvalidUploadMetadataError( + field=field_name, msg=form.errors[field_name][0] + ) + raise MissingUploadMetadataError(msg=form.errors[field_name][0]) # Get a validated Metadata object from the form data. # TODO: We should eventually extract this data out of the artifact and use that, @@ -936,8 +934,8 @@ def file_upload(request): error = errors[field_name][0] error_msg = str(error) if isinstance(error, metadata.InvalidLocalVersion): - raise InvalidLocalVersion(msg=error_msg) - raise InvalidCoreMetadata( + raise InvalidLocalVersionError(msg=error_msg) + raise InvalidCoreMetadataError( msg=error_msg + ("." if not error_msg.endswith(".") else "") ) @@ -968,7 +966,7 @@ def file_upload(request): # produce a valid API token, but the project lookup above uses (2) # and will fail because (1) != (2). if not request.user: - raise NonUserIdentity + raise NonUserIdentityError # Enforce email/2FA prerequisites before creating a brand new project, # so we don't leave an empty project record behind on rejection. @@ -991,7 +989,7 @@ def file_upload(request): ) raise _exc_with_message(exc.__class__, exc.detail) from None except RateLimiterException: - raise ProjectCreationRateLimited + raise ProjectCreationRateLimitedError # Check that the identity has permission to do things to this project, if this # is a new project this will act as a sanity check for the role we just @@ -999,20 +997,19 @@ def file_upload(request): allowed = request.has_permission(Permissions.ProjectsUpload, project) if not allowed: if getattr(allowed, "reason", None) is not None: - raise PermissionDenied(msg=allowed.msg) - elif request.user: - raise UserPermissionDenied( + raise PermissionError(msg=allowed.msg) + if request.user: + raise UserPermissionError( project=project.name, username=request.user.username ) - else: - raise TokenPermissionDenied(project=project.name) + raise TokenPermissionError(project=project.name) _ensure_user_can_upload(request) # If organization owned project, check if the organization is active. # Inactive organizations cannot upload new releases to their projects. if project.organization and not project.organization.good_standing: - raise OrgInactive + raise OrgInactiveError # If this is a user identity (i.e: API token) but there exists # a trusted publisher for this project, send an email warning that an @@ -1063,7 +1060,7 @@ def file_upload(request): # Uploading should prevent broken rendered descriptions. if rendered is None: - raise InvalidDescription(content_type=description_content_type) + raise InvalidDescriptionError(content_type=description_content_type) # Verify any verifiable URLs project_urls = ( @@ -1273,11 +1270,11 @@ def file_upload(request): for chunk in iter(lambda: request.POST["content"].file.read(8096), b""): file_size += len(chunk) if file_size > file_size_limit: - raise FileTooLarge( + raise FileTooLargeError( project=project.name, limit=file_size_limit // ONE_MIB ) if file_size + project.total_size > project_size_limit: - raise ProjectTooLarge( + raise ProjectTooLargeError( project=project.name, limit=project_size_limit // ONE_GIB ) @@ -1300,15 +1297,15 @@ def file_upload(request): for digest_name, digest_value in file_hashes.items() if getattr(form, f"{digest_name}_digest").data ): - raise DigestMismatch + raise DigestMismatchError # Check to see if the file that was uploaded exists already or not. is_duplicate = _is_duplicate_file(request.db, filename, file_hashes) if is_duplicate: request.tm.doom() return HTTPOk() - elif is_duplicate is not None: - raise FileAlreadyExists( + if is_duplicate is not None: + raise FileAlreadyExistsError( filename=filename, blake2_256=file_hashes["blake2_256"] ) @@ -1316,7 +1313,7 @@ def file_upload(request): if request.db.query( request.db.query(Filename).filter(Filename.filename == filename).exists() ).scalar(): - raise FilenameReused + raise FilenameReusedError # Check to see if uploading this file would create a duplicate sdist # for the current release. @@ -1328,7 +1325,7 @@ def file_upload(request): .exists() ).scalar() ): - raise DuplicateSDist + raise DuplicateSDistError # Check the file to make sure it is a valid distribution file. _scan = not request.flags.enabled(AdminFlagValue.DISABLE_UPLOAD_SCANNING) @@ -1343,7 +1340,7 @@ def file_upload(request): scan=_scan, ) if not _valid: - raise InvalidDistFile(filetype=form.filetype.data, msg=_msg) + raise InvalidDistFileError(filetype=form.filetype.data, msg=_msg) # Check that the sdist filename is correct if form.filetype.data == "sdist": @@ -1353,7 +1350,9 @@ def file_upload(request): packaging.utils.parse_sdist_filename(filename) ) except packaging.utils.InvalidSdistFilename: - raise InvalidFilename(filename=filename, filetype=form.filetype.data) + raise InvalidFilenameError( + filename=filename, filetype=form.filetype.data + ) # The previous function fails to accomodate the edge case where # versions may contain hyphens, so we handle that here based on @@ -1386,7 +1385,7 @@ def file_upload(request): packaging.utils.canonicalize_name(name_from_filename) != project.normalized_name ): - raise UnnormalizedFilename( + raise UnnormalizedFilenameError( project=project.name, normalized=project.normalized_name.replace("-", "_"), filetype=form.filetype.data, @@ -1399,7 +1398,7 @@ def file_upload(request): expected_filename = f"{expected_name}-{expected_version}.tar.gz" if filename != expected_filename: - raise UnnormalizedFilename( + raise UnnormalizedFilenameError( project=project.name, normalized=project.normalized_name.replace("-", "_"), filetype=form.filetype.data, @@ -1420,7 +1419,7 @@ def file_upload(request): try: tar.getmember(target_file) except KeyError: - raise MissingLicenseFile( + raise MissingLicenseFileError( filename=filename, license_file=license_file, target_file=target_file, @@ -1434,14 +1433,16 @@ def file_upload(request): filename ) except packaging.utils.InvalidWheelFilename: - raise InvalidFilename(filename=filename, filetype=form.filetype.data) + raise InvalidFilenameError( + filename=filename, filetype=form.filetype.data + ) for tag in tags: if not _valid_platform_tag(tag.platform): - raise InvalidPlatformTag(filename=filename, tag=tag.platform) + raise InvalidPlatformTagError(filename=filename, tag=tag.platform) if (canonical_name := packaging.utils.canonicalize_name(meta.name)) != name: - raise UnnormalizedFilename( + raise UnnormalizedFilenameError( project=project.name, normalized=canonical_name.replace("-", "_"), filetype=form.filetype.data, @@ -1458,14 +1459,14 @@ def file_upload(request): # https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode normalized_name = project.normalized_name.replace("-", "_") if name_from_filename != normalized_name: - raise UnnormalizedFilename( + raise UnnormalizedFilenameError( project=project.name, normalized=normalized_name, filetype=form.filetype.data, ) if meta.version != version: - raise InvalidFilenameVersion( + raise InvalidFilenameVersionError( expected=str(meta.version), found=str(version), filetype=form.filetype.data, @@ -1491,7 +1492,7 @@ def file_upload(request): try: zfp.read(license_filename) except KeyError: - raise MissingLicenseFile( + raise MissingLicenseFileError( filename=filename, license_file=license_file, target_file=license_filename, @@ -1501,7 +1502,7 @@ def file_upload(request): try: validate_record(temporary_filename) except MissingWheelRecordError: - raise MissingRecordFile( + raise MissingRecordFileError( filename=filename, record_filename=f"{name}-{version}.dist-info/RECORD", filetype=form.filetype.data, @@ -1524,14 +1525,16 @@ def file_upload(request): with zipfile.ZipFile(temporary_filename) as zfp: wheel_metadata_contents = zfp.read(metadata_filename) except KeyError: - raise MissingMetadataFile( + raise MissingMetadataFileError( filename=filename, metadata_filename=metadata_filename ) try: with open(temporary_filename + ".metadata", "wb") as fp: fp.write(wheel_metadata_contents) except OSError: - raise FilenameTooLong(filename=filename, filetype=form.filetype.data) + raise FilenameTooLongError( + filename=filename, filetype=form.filetype.data + ) metadata_file_hashes = { "sha256": hashlib.sha256(), @@ -1559,7 +1562,7 @@ def file_upload(request): Distribution(name=filename, digest=file_hashes["sha256"]), ) except AttestationUploadError as e: - raise InvalidAttestations(msg=str(e)) + raise InvalidAttestationsError(msg=str(e)) # Store the information about the file in the database. file_ = File( From f5f4224601d280515ae0cbef8d10c5fc978f337e Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Thu, 30 Apr 2026 00:06:28 -0400 Subject: [PATCH 11/12] slight tweak --- warehouse/forklift/legacy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 28659e0f8a68..a0080faa4eb8 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -1116,9 +1116,9 @@ def file_upload(request): else verify_email(email=maintainer_email, project=project) ) + is_new_release = False + canonical_version = packaging.utils.canonicalize_version(meta.version) try: - is_new_release = False - canonical_version = packaging.utils.canonicalize_version(meta.version) release = ( request.db.query(Release) .filter( From ed4c407d39c35888f5410d2121e28acdc8e00e32 Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Thu, 30 Apr 2026 23:48:43 -0400 Subject: [PATCH 12/12] restore the missing tests from a rebase --- tests/unit/forklift/test_legacy.py | 160 +++++++++++++++++++++++++++++ warehouse/forklift/decorators.py | 28 ----- warehouse/forklift/legacy.py | 58 ++++++----- 3 files changed, 190 insertions(+), 56 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index bca9f38d5763..976a59ffd1b8 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -11,6 +11,7 @@ from cgi import FieldStorage from textwrap import dedent +from types import SimpleNamespace from unittest import mock import pretend @@ -2411,6 +2412,165 @@ def test_upload_fails_without_oidc_publisher_permission( legacy.file_upload(db_request) assert excinfo.value.values == {"project": project.name} + def test_upload_fails_with_unverified_email_after_permission_check( + self, pyramid_config, db_request, mocker + ): + """The email check fires from inside the view, after the permission + check passes, so a maintainer without a verified primary email gets + the email error. + + See: https://github.com/pypi/warehouse/issues/18575 + """ + user = UserFactory.create() + project = ProjectFactory.create() + release = ReleaseFactory.create(project=project, version="1.0") + RoleFactory.create(user=user, project=project) + + filename = "{}-{}.tar.gz".format( + project.normalized_name.replace("-", "_"), release.version + ) + + pyramid_config.testing_securitypolicy(identity=user) + db_request.user = user + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": project.name, + "version": release.version, + "filetype": "sdist", + "md5_digest": "nope!", + "content": SimpleNamespace( + filename=filename, + file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), + type="application/tar", + ), + } + ) + + with pytest.raises(legacy.UnverifiedEmailError) as excinfo: + legacy.file_upload(db_request) + assert excinfo.value.values == {"username": user.username} + + def test_upload_fails_without_two_factor_after_permission_check( + self, pyramid_config, db_request, mocker + ): + """A user with permission and a verified email but no 2FA receives + the 2FA error from the view, after the permission check.""" + user = UserFactory.create(with_verified_primary_email=True) + user.totp_secret = None # Drop totp_secret so has_two_factor is False. + project = ProjectFactory.create() + release = ReleaseFactory.create(project=project, version="1.0") + RoleFactory.create(user=user, project=project) + + filename = "{}-{}.tar.gz".format( + project.normalized_name.replace("-", "_"), release.version + ) + + pyramid_config.testing_securitypolicy(identity=user) + db_request.user = user + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": project.name, + "version": release.version, + "filetype": "sdist", + "md5_digest": "nope!", + "content": SimpleNamespace( + filename=filename, + file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), + type="application/tar", + ), + } + ) + + with pytest.raises(legacy.MissingTwoFactorError) as excinfo: + legacy.file_upload(db_request) + assert excinfo.value.values == {"username": user.username} + + def test_upload_permission_error_surfaces_before_email_check( + self, pyramid_config, db_request, mocker + ): + """When a user lacks both project permission and a verified email, + the permission error surfaces first because it is the more useful + diagnostic for the misconfigured-token case. + + See: https://github.com/pypi/warehouse/issues/18575 + """ + owner = UserFactory.create() + EmailFactory.create(user=owner) + intruder = UserFactory.create() # No verified email, no 2FA. + project = ProjectFactory.create() + release = ReleaseFactory.create(project=project, version="1.0") + RoleFactory.create(user=owner, project=project) + + filename = "{}-{}.tar.gz".format( + project.normalized_name.replace("-", "_"), release.version + ) + + pyramid_config.testing_securitypolicy(identity=intruder, permissive=False) + db_request.user = intruder + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": project.name, + "version": release.version, + "filetype": "sdist", + "md5_digest": "nope!", + "content": SimpleNamespace( + filename=filename, + file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), + type="application/tar", + ), + } + ) + + with pytest.raises(legacy.UserPermissionError) as excinfo: + legacy.file_upload(db_request) + assert excinfo.value.values == { + "username": intruder.username, + "project": project.name, + } + + def test_upload_new_project_fails_with_unverified_email( + self, pyramid_config, db_request, mocker + ): + """A user attempting to upload (and thereby create) a brand new + project without a verified email is rejected before the project + gets created, so we don't leave an empty project record behind.""" + user = UserFactory.create() # No verified email. + + filename = "new_project-1.0.tar.gz" + + pyramid_config.testing_securitypolicy(identity=user) + db_request.user = user + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": "new-project", + "version": "1.0", + "filetype": "sdist", + "md5_digest": "nope!", + "content": SimpleNamespace( + filename=filename, + file=io.BytesIO(b"a" * (warehouse.constants.MAX_FILESIZE + 1)), + type="application/tar", + ), + } + ) + db_request.help_url = mocker.Mock(return_value="/the/help/url/") + + with pytest.raises(legacy.UnverifiedEmailError) as excinfo: + legacy.file_upload(db_request) + assert excinfo.value.values == {"username": user.username} + + # The project must not have been created. + assert ( + db_request.db.query(Project) + .filter(Project.normalized_name == "new-project") + .first() + is None + ) + def test_upload_attestation_fails_without_oidc_publisher( self, monkeypatch, diff --git a/warehouse/forklift/decorators.py b/warehouse/forklift/decorators.py index 838a1f482a84..f9f938df735a 100644 --- a/warehouse/forklift/decorators.py +++ b/warehouse/forklift/decorators.py @@ -61,34 +61,6 @@ class MissingIdentityError( pass -class UnverifiedEmailError( - ForkliftError, - error_type=HTTPForbidden, - message=( - "User {username!r} does not have a verified primary email address. " - "Please add a verified primary email before attempting to " - "upload to PyPI." - ), - help_anchor="verified-email", - tags={"reason:unverified-email"}, -): - pass - - -class MissingTwoFactorError( - ForkliftError, - error_type=HTTPForbidden, - message=( - "User {username!r} does not have two-factor authentication enabled. " - "Please enable two-factor authentication before attempting to " - "upload to PyPI." - ), - help_anchor="two-factor-authentication", - tags={"reason:no-2fa"}, -): - pass - - def upload_metrics(wrapped): def wrapper(context, request): diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index a0080faa4eb8..8d29049d5d17 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -142,6 +142,34 @@ class PermissionError( pass +class UnverifiedEmailError( + ForkliftError, + error_type=HTTPForbidden, + message=( + "User {username!r} does not have a verified primary email address. " + "Please add a verified primary email before attempting to " + "upload to PyPI." + ), + help_anchor="verified-email", + tags={"reason:unverified-email"}, +): + pass + + +class MissingTwoFactorError( + ForkliftError, + error_type=HTTPForbidden, + message=( + "User {username!r} does not have two-factor authentication enabled. " + "Please enable two-factor authentication before attempting to " + "upload to PyPI." + ), + help_anchor="two-factor-authentication", + tags={"reason:no-2fa"}, +): + pass + + class OrgInactiveError( ForkliftError, error_type=HTTPForbidden, @@ -831,36 +859,10 @@ def _ensure_user_can_upload(request: Request) -> None: return if not (request.user.primary_email and request.user.primary_email.verified): - request.metrics.increment( - "warehouse.upload.failed", tags=["reason:unverified-email"] - ) - raise _exc_with_message( - HTTPForbidden, - ( - "User {!r}, associated with the API token used, does not " - "have a verified primary email address. Please add a " - "verified primary email before attempting to upload to " - "PyPI. See {project_help} for more information." - ).format( - request.user.username, - project_help=request.help_url(_anchor="verified-email"), - ), - ) from None + raise UnverifiedEmailError(username=request.user.username) if not request.user.has_two_factor: - request.metrics.increment("warehouse.upload.failed", tags=["reason:no-2fa"]) - raise _exc_with_message( - HTTPForbidden, - ( - "User {!r}, associated with the API token used, does not " - "have two-factor authentication enabled. Please enable " - "two-factor authentication before attempting to upload to " - "PyPI. See {project_help} for more information." - ).format( - request.user.username, - project_help=request.help_url(_anchor="two-factor-authentication"), - ), - ) from None + raise MissingTwoFactorError(username=request.user.username) @view_config(