From ee493a661fd2e697f1c15681f7c6c42253c4f146 Mon Sep 17 00:00:00 2001 From: Matt Layman Date: Mon, 14 May 2018 17:55:37 -0400 Subject: [PATCH 1/3] Fail to upload package when description rendering is invalid. This branch prevents a package upload if the rendered content for the description_content_type is invalid. If no description_content_type is set, 'text/x-rst' is assumed as the type. Fixes #3966 --- tests/unit/forklift/test_legacy.py | 57 ++++++++++++++++++++++++++++++ warehouse/forklift/legacy.py | 22 +++++++++++- warehouse/utils/readme.py | 5 +-- 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 3b6978267701..f7d0f9e8970f 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -1003,6 +1003,62 @@ def test_fails_with_invalid_names(self, pyramid_config, db_request, name): "See /the/help/url/ " "for more information.").format(name) + @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." + ), + ( + "", + ".. invalid-directive::", + "400 The description failed to render for 'text/x-rst'. " + "See /the/help/url/ for more information." + ), + ], + ) + def test_fails_invalid_render(self, pyramid_config, db_request, + description_content_type, description, + message): + pyramid_config.testing_securitypolicy(userid=1) + user = UserFactory.create() + EmailFactory.create(user=user) + db_request.user = user + db_request.remote_addr = "10.10.10.30" + + db_request.POST = MultiDict({ + "metadata_version": "1.2", + "name": "example", + "version": "1.0", + "filetype": "sdist", + "md5_digest": "a fake md5 digest", + "content": pretend.stub( + filename="example-1.0.tar.gz", + file=io.BytesIO(b"A fake file."), + type="application/tar", + ), + "description_content_type": description_content_type, + "description": description, + }) + + db_request.help_url = pretend.call_recorder( + lambda **kw: "/the/help/url/" + ) + + with pytest.raises(HTTPBadRequest) 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 == 400 + assert resp.status == message + @pytest.mark.parametrize("name", ["xml", "XML", "pickle", "PiCKle", "main", "future", "al", "uU", "test", "encodings.utf_8_sig", @@ -1215,6 +1271,7 @@ def test_successful_upload(self, tmpdir, monkeypatch, pyramid_config, "filetype": "sdist", "pyversion": "source", "content": content, + "description": "an example description", }) db_request.POST.extend([ ("classifiers", "Environment :: Other Environment"), diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index ac0103e6571c..1f9867f4852c 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -45,7 +45,7 @@ Project, Release, Dependency, DependencyKind, Role, File, Filename, JournalEntry, BlacklistedProject, ) -from warehouse.utils import http +from warehouse.utils import http, readme MAX_FILESIZE = 60 * 1024 * 1024 # 60M @@ -940,6 +940,26 @@ def file_upload(request): ) ) + # Uploading should prevent broken rendered descriptions. + if form.description.data: + description_content_type = form.description_content_type.data + if not description_content_type: + description_content_type = 'text/x-rst' + rendered = readme.render( + form.description.data, description_content_type, + use_fallback=False) + if rendered is None: + raise _exc_with_message( + HTTPBadRequest, + ("The description failed to render " + "for '{description_content_type}'. " + "See {projecthelp} " + "for more information.").format( + description_content_type=description_content_type, + projecthelp=request.help_url(_anchor='project-name'), + ), + ) from None + try: canonical_version = packaging.utils.canonicalize_version( form.version.data diff --git a/warehouse/utils/readme.py b/warehouse/utils/readme.py index a6dd147eccc5..7b90a9c39f50 100644 --- a/warehouse/utils/readme.py +++ b/warehouse/utils/readme.py @@ -29,7 +29,7 @@ } -def render(value, content_type=None): +def render(value, content_type=None, use_fallback=True): if value is None: return value @@ -45,7 +45,8 @@ def render(value, content_type=None): # If the content was not rendered, we'll render as plaintext instead. The # reason it's necessary to do this instead of just accepting plaintext is # that readme_renderer will deal with sanitizing the content. - if rendered is None: + # Skip the fallback option when validating that rendered output is ok. + if use_fallback and rendered is None: rendered = readme_renderer.txt.render(value) return rendered From cd34e96f535a7851eff57cd393cb1a7dcd9369ce Mon Sep 17 00:00:00 2001 From: Matt Layman Date: Tue, 15 May 2018 21:41:11 -0400 Subject: [PATCH 2/3] Add documentation and improve error message. --- .gitignore | 1 + tests/unit/forklift/test_legacy.py | 5 +++-- warehouse/forklift/legacy.py | 19 +++++++++++++------ warehouse/templates/pages/help.html | 23 +++++++++++++++++++++++ 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 45d95a43ff86..6f7dd4e351d2 100644 --- a/.gitignore +++ b/.gitignore @@ -29,3 +29,4 @@ warehouse/static/dist warehouse/admin/static/dist tags +*.sw* diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index f7d0f9e8970f..571743104e86 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -1014,7 +1014,8 @@ def test_fails_with_invalid_names(self, pyramid_config, db_request, name): ( "", ".. invalid-directive::", - "400 The description failed to render for 'text/x-rst'. " + "400 The description failed to render in the default format " + "of reStructuredText. " "See /the/help/url/ for more information." ), ], @@ -1053,7 +1054,7 @@ def test_fails_invalid_render(self, pyramid_config, db_request, resp = excinfo.value assert db_request.help_url.calls == [ - pretend.call(_anchor='project-name') + pretend.call(_anchor='description-content-type') ] assert resp.status_code == 400 diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 1f9867f4852c..1e1fbe97bd33 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -949,14 +949,21 @@ def file_upload(request): form.description.data, description_content_type, use_fallback=False) if rendered is None: + if form.description_content_type.data: + message = ( + "The description failed to render " + "for '{description_content_type}'.").format( + description_content_type=description_content_type) + else: + message = ( + "The description failed to render " + "in the default format of reStructuredText.") raise _exc_with_message( HTTPBadRequest, - ("The description failed to render " - "for '{description_content_type}'. " - "See {projecthelp} " - "for more information.").format( - description_content_type=description_content_type, - projecthelp=request.help_url(_anchor='project-name'), + "{message} See {projecthelp} for more information.".format( + message=message, + projecthelp=request.help_url( + _anchor='description-content-type'), ), ) from None diff --git a/warehouse/templates/pages/help.html b/warehouse/templates/pages/help.html index 918f2e4a8d13..ad29c1d0dfee 100644 --- a/warehouse/templates/pages/help.html +++ b/warehouse/templates/pages/help.html @@ -39,6 +39,7 @@ {% macro file_name_reuse() %}Why am I getting a "Filename or contents already exists" or "Filename has been previously used" error?{% endmacro %} {% macro project_name() %}Why isn't my desired project name available?{% endmacro %} {% macro project_name_claim() %}How do I claim an abandoned or previously registered project name?{% endmacro %} +{% macro description_content_type() %}How can I upload a description in a different format?{% endmacro %} {% macro new_classifier() %}How do I request a new trove classifier?{% endmacro %} {% macro feedback() %}Where can I report a bug or provide feedback?{% endmacro %} {% macro maintainers() %}Who maintains PyPI?{% endmacro %} @@ -84,6 +85,7 @@

Problems

  • {{ private_indices() }}
  • {{ project_name() }}
  • {{ project_name_claim() }}
    • +
  • {{ description_content_type() }}
  • {{ file_size_limit() }}
  • {{ admin_intervention() }}
  • {{ file_name_reuse() }}
    • @@ -241,6 +243,27 @@

    {{ project_name_claim() }}

    PEP 541 has been accepted, and PyPI is creating a workflow which will be documented here.

    +

    {{ description_content_type() }}

    +

    + By default, + an upload's description will render + with reStructuredText. + If the description is in an alternate format + like Markdown, + a package may set the long_description_content_type + in setup.py + to the alternate format. + Refer to the Python Packaging User Guide + for details on the available formats. +

    +

    + PyPI will reject uploads + if the description fails to render. + To check a description locally for validity, + you may use readme_renderer, + which is the same description renderer used by PyPI. +

    +

    {{ file_size_limit() }}

    If you can't upload your project's release to PyPI because you're hitting the upload file size limit, we can sometimes increase your limit. Please make sure you've uploaded at least one release for the project that's under the limit (a developmental release version number is fine). Then, please file an issue and tell us:

    From dc8e7676e9f2c53f3e8f37600bf0ef672547071c Mon Sep 17 00:00:00 2001 From: Donald Stufft Date: Thu, 24 May 2018 16:23:06 -0400 Subject: [PATCH 3/3] fix typo --- tests/unit/forklift/test_legacy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 350e7b08d9a5..a567f0161f8b 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -914,7 +914,7 @@ def test_fails_with_invalid_names(self, pyramid_config, db_request, name): assert resp.status_code == 400 assert resp.status == ( - "400 The name {!r} is not allowed. " + "400 The name {!r} isn't allowed. " "See /the/help/url/ " "for more information." ).format(name)