From b83a51ab4b7393e9bf13da49d53fabd11ec97326 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 13:44:35 +0000 Subject: [PATCH 1/8] Exercise duplicate POST keys in functional test --- tests/functional/forklift/test_legacy.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/tests/functional/forklift/test_legacy.py b/tests/functional/forklift/test_legacy.py index 8088b20475d7..969542e55077 100644 --- a/tests/functional/forklift/test_legacy.py +++ b/tests/functional/forklift/test_legacy.py @@ -17,6 +17,8 @@ import pymacaroons import pytest +from webob.multidict import MultiDict + from warehouse.macaroons import caveats from ...common.db.accounts import UserFactory @@ -90,10 +92,8 @@ def test_file_upload(webtest): with open("./tests/functional/_fixtures/sampleproject-3.0.0.tar.gz", "rb") as f: content = f.read() - webtest.post( - "/legacy/?:action=file_upload", - headers={"Authorization": f"Basic {credentials}"}, - params={ + params = MultiDict( + { "name": "sampleproject", "sha256_digest": ( "117ed88e5db073bb92969a7545745fd977ee85b7019706dd256a64058f70963d" @@ -101,7 +101,15 @@ def test_file_upload(webtest): "filetype": "sdist", "metadata_version": "2.1", "version": "3.0.0", - }, + } + ) + params.add("project-url", "https://example.com/foo") + params.add("project-url", "https://example.com/bar") + + webtest.post( + "/legacy/?:action=file_upload", + headers={"Authorization": f"Basic {credentials}"}, + params=params, upload_files=[("content", "sampleproject-3.0.0.tar.gz", content)], status=HTTPStatus.OK, ) From 2cb4a95fa691387ba48cedcfabe8d74b4097bdb7 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 14:24:08 +0000 Subject: [PATCH 2/8] Assert on response text too --- tests/functional/test_config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py index c69c95ff321c..64a10412c0c5 100644 --- a/tests/functional/test_config.py +++ b/tests/functional/test_config.py @@ -36,4 +36,5 @@ def test_rejects_duplicate_post_keys(webtest, socket_enabled): body.add("foo", "bar") body.add("foo", "baz") - webtest.post("/account/login", params=body, status=HTTPStatus.BAD_REQUEST) + resp = webtest.post("/account/login", params=body, status=HTTPStatus.BAD_REQUEST) + assert "POST body may not contain duplicate keys" in resp.body.decode() From 69eae76dad84eafe9010c2992eaafae86fba5f36 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 14:24:28 +0000 Subject: [PATCH 3/8] Explicitly just count keys instead of casting --- warehouse/config.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/warehouse/config.py b/warehouse/config.py index ec71b3278b27..934482c0fc17 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -274,10 +274,9 @@ def reject_duplicate_post_keys_view(view, info): @functools.wraps(view) def wrapped(context, request): if request.POST: - # Attempt to cast to a dict to catch duplicate keys - try: - dict(**request.POST) - except TypeError: + # Determine if there are any duplicate keys + keys = list(request.POST.keys()) + if len(keys) != len(set(keys)): return HTTPBadRequest("POST body may not contain duplicate keys") # Casting succeeded, so just return the regular view From 29588cdf43a7d9fd0854a306df6fa22293e46500 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 14:32:16 +0000 Subject: [PATCH 4/8] Include URL in error message --- tests/functional/test_config.py | 1 + warehouse/config.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py index 64a10412c0c5..9cc3133aa7c7 100644 --- a/tests/functional/test_config.py +++ b/tests/functional/test_config.py @@ -38,3 +38,4 @@ def test_rejects_duplicate_post_keys(webtest, socket_enabled): resp = webtest.post("/account/login", params=body, status=HTTPStatus.BAD_REQUEST) assert "POST body may not contain duplicate keys" in resp.body.decode() + assert "(URL: 'http://localhost/account/login')" in resp.body.decode() diff --git a/warehouse/config.py b/warehouse/config.py index 934482c0fc17..ea38fe586153 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -277,7 +277,10 @@ def wrapped(context, request): # Determine if there are any duplicate keys keys = list(request.POST.keys()) if len(keys) != len(set(keys)): - return HTTPBadRequest("POST body may not contain duplicate keys") + return HTTPBadRequest( + "POST body may not contain duplicate keys " + f"(URL: { request.url!r })" + ) # Casting succeeded, so just return the regular view return view(context, request) From 8d742cc520694aad2b9bb7f854632b0708e99f69 Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 14:33:39 +0000 Subject: [PATCH 5/8] Add classifiers to test too --- tests/functional/forklift/test_legacy.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/functional/forklift/test_legacy.py b/tests/functional/forklift/test_legacy.py index 969542e55077..124f475005df 100644 --- a/tests/functional/forklift/test_legacy.py +++ b/tests/functional/forklift/test_legacy.py @@ -105,6 +105,8 @@ def test_file_upload(webtest): ) params.add("project-url", "https://example.com/foo") params.add("project-url", "https://example.com/bar") + params.add("classifiers", "Programming Language :: Python :: 3.10") + params.add("classifiers", "Programming Language :: Python :: 3.11") webtest.post( "/legacy/?:action=file_upload", From f06da0340482a7ada3dc1555cf85b1fcec722e7f Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 14:44:10 +0000 Subject: [PATCH 6/8] Linting --- warehouse/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/warehouse/config.py b/warehouse/config.py index ea38fe586153..753bb9aaee76 100644 --- a/warehouse/config.py +++ b/warehouse/config.py @@ -279,7 +279,7 @@ def wrapped(context, request): if len(keys) != len(set(keys)): return HTTPBadRequest( "POST body may not contain duplicate keys " - f"(URL: { request.url!r })" + f"(URL: {request.url!r})" ) # Casting succeeded, so just return the regular view From 04a1a86dcf8944746b2071c61ee66200bd4967ab Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 15:49:39 +0000 Subject: [PATCH 7/8] Failing test --- tests/functional/forklift/test_legacy.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/functional/forklift/test_legacy.py b/tests/functional/forklift/test_legacy.py index 124f475005df..9992a23e040c 100644 --- a/tests/functional/forklift/test_legacy.py +++ b/tests/functional/forklift/test_legacy.py @@ -60,7 +60,14 @@ def test_remove_doc_upload(webtest): ) -def test_file_upload(webtest): +@pytest.mark.parametrize( + "upload_url", + [ + "/legacy/?:action=file_upload", + "/legacy/", + ], +) +def test_file_upload(webtest, upload_url): user = UserFactory.create( with_verified_primary_email=True, password=( # 'password' @@ -109,7 +116,7 @@ def test_file_upload(webtest): params.add("classifiers", "Programming Language :: Python :: 3.11") webtest.post( - "/legacy/?:action=file_upload", + upload_url, headers={"Authorization": f"Basic {credentials}"}, params=params, upload_files=[("content", "sampleproject-3.0.0.tar.gz", content)], From 9f07e2bc393abec444846203501cc495406864ea Mon Sep 17 00:00:00 2001 From: Dustin Ingram Date: Thu, 19 Sep 2024 15:56:51 +0000 Subject: [PATCH 8/8] Test both ways of providing the action --- tests/functional/forklift/test_legacy.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/functional/forklift/test_legacy.py b/tests/functional/forklift/test_legacy.py index 9992a23e040c..d520d6b09558 100644 --- a/tests/functional/forklift/test_legacy.py +++ b/tests/functional/forklift/test_legacy.py @@ -61,13 +61,13 @@ def test_remove_doc_upload(webtest): @pytest.mark.parametrize( - "upload_url", + ("upload_url", "additional_data"), [ - "/legacy/?:action=file_upload", - "/legacy/", + ("/legacy/?:action=file_upload", {}), + ("/legacy/", {":action": "file_upload", "protocol_version": "1"}), ], ) -def test_file_upload(webtest, upload_url): +def test_file_upload(webtest, upload_url, additional_data): user = UserFactory.create( with_verified_primary_email=True, password=( # 'password' @@ -110,6 +110,7 @@ def test_file_upload(webtest, upload_url): "version": "3.0.0", } ) + params.update(additional_data) params.add("project-url", "https://example.com/foo") params.add("project-url", "https://example.com/bar") params.add("classifiers", "Programming Language :: Python :: 3.10")