Add type checking to our XMLRPC API (pypi#4575)

Author: dstufft

requirements/main.in

@@ -49,6 +49,7 @@ sqlalchemy-citext
 stdlib-list
 structlog
 transaction
+typeguard
 whitenoise
 WTForms>=2.0.0
 zope.sqlalchemy

requirements/main.txt

@@ -522,6 +522,9 @@ transaction==2.2.1 \
 translationstring==1.3 \
     --hash=sha256:4ee44cfa58c52ade8910ea0ebc3d2d84bdcad9fa0422405b1801ec9b9a65b72d \
     --hash=sha256:e26c7bf383413234ed442e0980a2ebe192b95e3745288a8fd2805156d27515b4
+typeguard==2.2.2 \
+    --hash=sha256:13ecd6ca9cd4a8b7965138c5782e3ab3711096f573c91b3ce120ac4764a6a233 \
+    --hash=sha256:b8ddc6e2e60bd64b7003f9a685a09ba387b74adf2f6bea7534a76d61892f573e
 urllib3==1.22 \
     --hash=sha256:06330f386d6e4b195fbfc736b297f58c5a892e4440e54d294d7004e3a9bbea1b \
     --hash=sha256:cc44da8e1145637334317feebd728bd869a35285b93cbb4cca2577da7e62db4f

tests/functional/legacy_api/test_xmlrpc.py

@@ -33,6 +33,15 @@ def test_xmlrpc_succeeds(app_config, webtest, metrics):
 
 def test_invalid_arguments(app_config, webtest):
     with pytest.raises(
-        xmlrpc.client.Fault, match="server error; invalid method params"
+        xmlrpc.client.Fault,
+        match="client error; missing a required argument: 'package_name'",
     ):
         webtest.xmlrpc("/pypi", "package_releases")
+
+
+def test_arguments_with_wrong_type(app_config, webtest):
+    with pytest.raises(
+        xmlrpc.client.Fault,
+        match='client error; type of argument "serial" must be int; got str instead',
+    ):
+        webtest.xmlrpc("/pypi", "changelog_since_serial", "wrong!")

tests/unit/legacy/api/xmlrpc/test_xmlrpc.py

@@ -40,16 +40,6 @@ def test_fails_with_invalid_operator(self, pyramid_request, metrics):
         )
         assert metrics.histogram.calls == []
 
-    def test_fails_if_spec_not_mapping(self, pyramid_request, metrics):
-        with pytest.raises(xmlrpc.XMLRPCWrappedError) as exc:
-            xmlrpc.search(pyramid_request, "a string")
-
-        assert (
-            exc.value.faultString
-            == "TypeError: Invalid spec, must be a mapping/dictionary."
-        )
-        assert metrics.histogram.calls == []
-
     def test_default_search_operator(self, pyramid_request, metrics):
         class FakeQuery:
             def __init__(self, type, must):

warehouse/legacy/api/xmlrpc/views.py

@@ -10,19 +10,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import collections.abc
 import datetime
 import functools
 import xmlrpc.client
 import xmlrpc.server
 
+from typing import List, Mapping, Union
+
 import elasticsearch
+import typeguard
 
 from elasticsearch_dsl import Q
 from packaging.utils import canonicalize_name
 from pyramid.view import view_config
+from pyramid_rpc.mapper import MapplyViewMapper
 from pyramid_rpc.xmlrpc import (
     XmlRpcError,
+    XmlRpcInvalidMethodParams,
     exception_view as _exception_view,
     xmlrpc_method as _xmlrpc_method,
 )
@@ -72,6 +76,7 @@ def xmlrpc_method(**kwargs):
         require_csrf=False,
         require_methods=["POST"],
         decorator=(submit_xmlrpc_metrics(method=kwargs["method"]),),
+        mapper=TypedMapplyViewMapper,
     )
 
     def decorator(f):
@@ -106,6 +111,15 @@ class XMLRPCServiceUnavailable(XmlRpcError):
     faultString = "server error; service unavailable"
 
 
+class XMLRPCInvalidParamTypes(XmlRpcInvalidMethodParams):
+    def __init__(self, exc):
+        self.exc = exc
+
+    @property
+    def faultString(self):  # noqa
+        return f"client error; {self.exc}"
+
+
 class XMLRPCWrappedError(xmlrpc.server.Fault):
     def __init__(self, exc):
         self.faultCode = -32500
@@ -116,18 +130,25 @@ def faultString(self):  # noqa
         return "{exc.__class__.__name__}: {exc}".format(exc=self.wrapped_exception)
 
 
+class TypedMapplyViewMapper(MapplyViewMapper):
+    def mapply(self, fn, args, kwargs):
+        try:
+            memo = typeguard._CallMemo(fn, args=args, kwargs=kwargs)
+            typeguard.check_argument_types(memo)
+        except TypeError as exc:
+            print(exc)
+            raise XMLRPCInvalidParamTypes(exc)
+
+        return super().mapply(fn, args, kwargs)
+
+
 @view_config(route_name="pypi", context=Exception, renderer="xmlrpc")
 def exception_view(exc, request):
     return _exception_view(exc, request)
 
 
 @xmlrpc_method(method="search")
-def search(request, spec, operator="and"):
-    if not isinstance(spec, collections.abc.Mapping):
-        raise XMLRPCWrappedError(
-            TypeError("Invalid spec, must be a mapping/dictionary.")
-        )
-
+def search(request, spec: Mapping[str, Union[str, List[str]]], operator: str = "and"):
     if operator not in {"and", "or"}:
         raise XMLRPCWrappedError(
             ValueError("Invalid operator, must be one of 'and' or 'or'.")
@@ -220,7 +241,7 @@ def list_packages_with_serial(request):
 
 
 @xmlrpc_method(method="package_hosting_mode")
-def package_hosting_mode(request, package_name):
+def package_hosting_mode(request, package_name: str):
     try:
         project = (
             request.db.query(Project)
@@ -234,7 +255,7 @@ def package_hosting_mode(request, package_name):
 
 
 @xmlrpc_method(method="user_packages")
-def user_packages(request, username):
+def user_packages(request, username: str):
     roles = (
         request.db.query(Role)
         .join(User, Project)
@@ -253,7 +274,7 @@ def top_packages(request, num=None):
 
 
 @xmlrpc_cache_by_project(method="package_releases")
-def package_releases(request, package_name, show_hidden=False):
+def package_releases(request, package_name: str, show_hidden: bool = False):
     try:
         project = (
             request.db.query(Project)
@@ -293,7 +314,7 @@ def package_data(request, package_name, version):
 
 
 @xmlrpc_cache_by_project(method="release_data")
-def release_data(request, package_name, version):
+def release_data(request, package_name: str, version: str):
     try:
         release = (
             request.db.query(Release)
@@ -367,7 +388,7 @@ def package_urls(request, package_name, version):
 
 
 @xmlrpc_cache_by_project(method="release_urls")
-def release_urls(request, package_name, version):
+def release_urls(request, package_name: str, version: str):
     files = (
         request.db.query(File)
         .join(Release, Project)
@@ -401,7 +422,7 @@ def release_urls(request, package_name, version):
 
 
 @xmlrpc_cache_by_project(method="package_roles")
-def package_roles(request, package_name):
+def package_roles(request, package_name: str):
     roles = (
         request.db.query(Role)
         .join(User, Project)
@@ -418,7 +439,7 @@ def changelog_last_serial(request):
 
 
 @xmlrpc_method(method="changelog_since_serial")
-def changelog_since_serial(request, serial):
+def changelog_since_serial(request, serial: int):
     entries = (
         request.db.query(JournalEntry)
         .filter(JournalEntry.id > serial)
@@ -439,7 +460,7 @@ def changelog_since_serial(request, serial):
 
 
 @xmlrpc_method(method="changelog")
-def changelog(request, since, with_ids=False):
+def changelog(request, since: int, with_ids: bool = False):
     since = datetime.datetime.utcfromtimestamp(since)
     entries = (
         request.db.query(JournalEntry)
@@ -466,7 +487,7 @@ def changelog(request, since, with_ids=False):
 
 
 @xmlrpc_method(method="browse")
-def browse(request, classifiers):
+def browse(request, classifiers: List[str]):
     classifiers_q = (
         request.db.query(Classifier)
         .filter(Classifier.classifier.in_(classifiers))