Prevent duplicate POST keys in requests (#16732)

* Add form IDs and update functional test

* Reformat comment

* Remove unnecessary cast to MultiDict

* Add a failing test

* Add view deriver to prohibit duplicate POST keys

* Permit duplicate POST keys for file_upload view

* Update translations

Author: di

tests/functional/manage/test_views.py

@@ -64,16 +64,15 @@ def test_changing_password_succeeds(self, webtest, socket_enabled):
         login_page = webtest.get("/account/login/", status=HTTPStatus.OK)
 
         # Fill & submit the login form
-        login_form = login_page.forms[2]  # TODO: form should have an ID, doesn't yet
+        login_form = login_page.forms["login-form"]
         anonymous_csrf_token = login_form["csrf_token"].value
         login_form["username"] = user.username
         login_form["password"] = "password"
         login_form["csrf_token"] = anonymous_csrf_token
 
         two_factor_page = login_form.submit().follow(status=HTTPStatus.OK)
 
-        # TODO: form doesn't have an ID yet
-        two_factor_form = two_factor_page.forms[2]
+        two_factor_form = two_factor_page.forms["totp-auth-form"]
         two_factor_form["csrf_token"] = anonymous_csrf_token
 
         # Generate the correct TOTP value from the known secret
@@ -94,9 +93,8 @@ def test_changing_password_succeeds(self, webtest, socket_enabled):
         assert anonymous_csrf_token != logged_in_csrf_token
 
         # Fill & submit the change password form
-        # TODO: form doesn't have an ID yet
         new_password = faker.Faker().password()  # a secure-enough password for testing
-        change_password_form = change_password_page.forms[3]
+        change_password_form = change_password_page.forms["change-password-form"]
         change_password_form["csrf_token"] = logged_in_csrf_token
         change_password_form["password"] = "password"
         change_password_form["new_password"] = new_password

tests/functional/test_config.py

@@ -0,0 +1,39 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from http import HTTPStatus
+
+from webob.multidict import MultiDict
+
+from ..common.db.accounts import UserFactory
+
+
+def test_rejects_duplicate_post_keys(webtest, socket_enabled):
+    # create a User
+    user = UserFactory.create(with_verified_primary_email=True, clear_pwd="password")
+
+    # /login is an unauthenticated endpoint that accepts a POST
+    login_page = webtest.get("/account/login/", status=HTTPStatus.OK)
+
+    # Get the CSRF token from the form
+    login_form = login_page.forms["login-form"]
+    anonymous_csrf_token = login_form["csrf_token"].value
+
+    body = MultiDict()
+    body.add("username", user.username)
+    body.add("password", "password")
+    body.add("csrf_token", anonymous_csrf_token)
+    # Add multiple duplicate keys to the POST body, doesn't matter what they are
+    body.add("foo", "bar")
+    body.add("foo", "baz")
+
+    webtest.post("/account/login", params=body, status=HTTPStatus.BAD_REQUEST)

tests/unit/test_config.py

@@ -290,6 +290,7 @@ def __init__(self):
         whitenoise_add_manifest=pretend.call_recorder(lambda *a, **kw: None),
         scan=pretend.call_recorder(lambda categories, ignore: None),
         commit=pretend.call_recorder(lambda: None),
+        add_view_deriver=pretend.call_recorder(lambda *a, **kw: None),
     )
     configurator_cls = pretend.call_recorder(lambda settings: configurator_obj)
     monkeypatch.setattr(config, "Configurator", configurator_cls)
@@ -531,6 +532,11 @@ def __init__(self):
         pretend.call("json", json_renderer_obj),
         pretend.call("xmlrpc", xmlrpc_renderer_obj),
     ]
+    assert configurator_obj.add_view_deriver.calls == [
+        pretend.call(
+            config.reject_duplicate_post_keys_view, under=config.viewderivers.INGRESS
+        )
+    ]
 
     assert json_renderer_cls.calls == [
         pretend.call(

warehouse/config.py

@@ -12,6 +12,7 @@
 
 import base64
 import enum
+import functools
 import json
 import os
 import secrets
@@ -24,10 +25,11 @@
 import platformdirs
 import transaction
 
-from pyramid import renderers
+from pyramid import renderers, viewderivers
 from pyramid.authorization import Allow, Authenticated
 from pyramid.config import Configurator as _Configurator
 from pyramid.exceptions import HTTPForbidden
+from pyramid.httpexceptions import HTTPBadRequest
 from pyramid.settings import asbool
 from pyramid.tweens import EXCVIEW
 from pyramid_rpc.xmlrpc import XMLRPCRenderer
@@ -266,6 +268,29 @@ def from_base64_encoded_json(configuration):
     return json.loads(base64.urlsafe_b64decode(configuration.encode("ascii")))
 
 
+def reject_duplicate_post_keys_view(view, info):
+    if not info.options.get("permit_duplicate_post_keys"):
+
+        @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:
+                    return HTTPBadRequest("POST body may not contain duplicate keys")
+
+            # Casting succeeded, so just return the regular view
+            return view(context, request)
+
+        return wrapped
+
+    return view
+
+
+reject_duplicate_post_keys_view.options = {"permit_duplicate_post_keys"}  # type: ignore
+
+
 def configure(settings=None):
     # Sanity check: regardless of what we're configuring, some of Warehouse's
     # application state depends on a handful of XDG directories existing.
@@ -817,6 +842,9 @@ def configure(settings=None):
         ],
     )
 
+    # Reject requests with duplicate POST keys
+    config.add_view_deriver(reject_duplicate_post_keys_view, under=viewderivers.INGRESS)
+
     # Enable Warehouse to serve our static files
     prevent_http_cache = config.get_settings().get("pyramid.prevent_http_cache", False)
     config.add_static_view(
@@ -893,8 +921,8 @@ def configure(settings=None):
     )
 
     # Sanity check our request and responses.
-    # Note: It is very important that this go last. We need everything else that might
-    #       have added a tween to be registered prior to this.
+    # Note: It is very important that this go last. We need everything else
+    # that might have added a tween to be registered prior to this.
     config.include(".sanity")
 
     # Finally, commit all of our changes

warehouse/forklift/legacy.py

@@ -487,6 +487,7 @@ def _sort_releases(request: Request, project: Project):
     require_csrf=False,
     require_methods=["POST"],
     has_translations=True,
+    permit_duplicate_post_keys=True,
 )
 def file_upload(request):
     # This is a list of warnings that we'll emit *IF* the request is successful.

warehouse/locale/messages.pot

@@ -151,7 +151,7 @@ msgstr ""
 msgid "Successful WebAuthn assertion"
 msgstr ""
 
-#: warehouse/accounts/views.py:569 warehouse/manage/views/__init__.py:870
+#: warehouse/accounts/views.py:569 warehouse/manage/views/__init__.py:865
 msgid "Recovery code accepted. The supplied code cannot be used again."
 msgstr ""
 
@@ -285,7 +285,7 @@ msgid "You are now ${role} of the '${project_name}' project."
 msgstr ""
 
 #: warehouse/accounts/views.py:1548 warehouse/accounts/views.py:1791
-#: warehouse/manage/views/__init__.py:1249
+#: warehouse/manage/views/__init__.py:1242
 msgid ""
 "Trusted publishing is temporarily disabled. See https://pypi.org/help"
 "#admin-intervention for details."
@@ -305,19 +305,19 @@ msgstr ""
 msgid "You can't register more than 3 pending trusted publishers at once."
 msgstr ""
 
-#: warehouse/accounts/views.py:1614 warehouse/manage/views/__init__.py:1304
-#: warehouse/manage/views/__init__.py:1417
-#: warehouse/manage/views/__init__.py:1529
-#: warehouse/manage/views/__init__.py:1639
+#: warehouse/accounts/views.py:1614 warehouse/manage/views/__init__.py:1297
+#: warehouse/manage/views/__init__.py:1410
+#: warehouse/manage/views/__init__.py:1522
+#: warehouse/manage/views/__init__.py:1632
 msgid ""
 "There have been too many attempted trusted publisher registrations. Try "
 "again later."
 msgstr ""
 
-#: warehouse/accounts/views.py:1625 warehouse/manage/views/__init__.py:1318
-#: warehouse/manage/views/__init__.py:1431
-#: warehouse/manage/views/__init__.py:1543
-#: warehouse/manage/views/__init__.py:1653
+#: warehouse/accounts/views.py:1625 warehouse/manage/views/__init__.py:1311
+#: warehouse/manage/views/__init__.py:1424
+#: warehouse/manage/views/__init__.py:1536
+#: warehouse/manage/views/__init__.py:1646
 msgid "The trusted publisher could not be registered"
 msgstr ""
 
@@ -427,130 +427,130 @@ msgstr ""
 msgid "This team name has already been used. Choose a different team name."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:282
+#: warehouse/manage/views/__init__.py:281
 msgid "Account details updated"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:312
+#: warehouse/manage/views/__init__.py:311
 msgid "Email ${email_address} added - check your email for a verification link"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:818
+#: warehouse/manage/views/__init__.py:813
 msgid "Recovery codes already generated"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:819
+#: warehouse/manage/views/__init__.py:814
 msgid "Generating new recovery codes will invalidate your existing codes."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:928
+#: warehouse/manage/views/__init__.py:923
 msgid "Verify your email to create an API token."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1028
+#: warehouse/manage/views/__init__.py:1021
 msgid "API Token does not exist."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1060
+#: warehouse/manage/views/__init__.py:1053
 msgid "Invalid credentials. Try again"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1285
+#: warehouse/manage/views/__init__.py:1278
 msgid ""
 "GitHub-based trusted publishing is temporarily disabled. See "
 "https://pypi.org/help#admin-intervention for details."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1398
+#: warehouse/manage/views/__init__.py:1391
 msgid ""
 "GitLab-based trusted publishing is temporarily disabled. See "
 "https://pypi.org/help#admin-intervention for details."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1510
+#: warehouse/manage/views/__init__.py:1503
 msgid ""
 "Google-based trusted publishing is temporarily disabled. See "
 "https://pypi.org/help#admin-intervention for details."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1619
+#: warehouse/manage/views/__init__.py:1612
 msgid ""
 "ActiveState-based trusted publishing is temporarily disabled. See "
 "https://pypi.org/help#admin-intervention for details."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1854
-#: warehouse/manage/views/__init__.py:2155
-#: warehouse/manage/views/__init__.py:2263
+#: warehouse/manage/views/__init__.py:1847
+#: warehouse/manage/views/__init__.py:2148
+#: warehouse/manage/views/__init__.py:2256
 msgid ""
 "Project deletion temporarily disabled. See https://pypi.org/help#admin-"
 "intervention for details."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1986
-#: warehouse/manage/views/__init__.py:2071
-#: warehouse/manage/views/__init__.py:2172
-#: warehouse/manage/views/__init__.py:2272
+#: warehouse/manage/views/__init__.py:1979
+#: warehouse/manage/views/__init__.py:2064
+#: warehouse/manage/views/__init__.py:2165
+#: warehouse/manage/views/__init__.py:2265
 msgid "Confirm the request"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:1998
+#: warehouse/manage/views/__init__.py:1991
 msgid "Could not yank release - "
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2083
+#: warehouse/manage/views/__init__.py:2076
 msgid "Could not un-yank release - "
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2184
+#: warehouse/manage/views/__init__.py:2177
 msgid "Could not delete release - "
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2284
+#: warehouse/manage/views/__init__.py:2277
 msgid "Could not find file"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2288
+#: warehouse/manage/views/__init__.py:2281
 msgid "Could not delete file - "
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2438
+#: warehouse/manage/views/__init__.py:2431
 msgid "Team '${team_name}' already has ${role_name} role for project"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2545
+#: warehouse/manage/views/__init__.py:2538
 msgid "User '${username}' already has ${role_name} role for project"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2612
+#: warehouse/manage/views/__init__.py:2605
 msgid "${username} is now ${role} of the '${project_name}' project."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2644
+#: warehouse/manage/views/__init__.py:2637
 msgid ""
 "User '${username}' does not have a verified primary email address and "
 "cannot be added as a ${role_name} for project"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2657
+#: warehouse/manage/views/__init__.py:2650
 #: warehouse/manage/views/organizations.py:878
 msgid "User '${username}' already has an active invite. Please try again later."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2722
+#: warehouse/manage/views/__init__.py:2715
 #: warehouse/manage/views/organizations.py:943
 msgid "Invitation sent to '${username}'"
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2755
+#: warehouse/manage/views/__init__.py:2748
 msgid "Could not find role invitation."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2766
+#: warehouse/manage/views/__init__.py:2759
 msgid "Invitation already expired."
 msgstr ""
 
-#: warehouse/manage/views/__init__.py:2798
+#: warehouse/manage/views/__init__.py:2791
 #: warehouse/manage/views/organizations.py:1130
 msgid "Invitation revoked from '${username}'."
 msgstr ""