fix: handle exception for API filter values that fail below tastypie

ietf/api/__init__.py

@@ -1,45 +1,44 @@
 # Copyright The IETF Trust 2014-2020, All Rights Reserved
-# -*- coding: utf-8 -*-
 
 
 import datetime
 import re
 import sys
-
 from urllib.parse import urlencode
 
+import tastypie.resources
+import tastypie.serializers
 from django.apps import apps as django_apps
 from django.core.exceptions import ObjectDoesNotExist
+from django.db import DataError, transaction
 from django.http import HttpResponseNotAllowed
 from django.utils.module_loading import autodiscover_modules
-
-
-import debug                            # pyflakes:ignore
-
-import tastypie.resources
-import tastypie.serializers
 from tastypie.api import Api
 from tastypie.bundle import Bundle
-from tastypie.exceptions import ApiFieldError
+from tastypie.exceptions import ApiFieldError, BadRequest, InvalidFilterError
 from tastypie.fields import ApiField
 
+import debug  # noqa: F401  (pyflakes:ignore)
+from ietf.utils.log import log
+
 _api_list = []
 
 OMITTED_APPS_APIS = ["ietf.status"]
 
 # Pre-py3.11, fromisoformat() does not handle Z or +HH tz offsets
 HAVE_BROKEN_FROMISOFORMAT = sys.version_info < (3, 11, 0, "", 0)
 
+
 def populate_api_list():
     _module_dict = globals()
     for app_config in django_apps.get_app_configs():
-        if '.' in app_config.name and app_config.name not in OMITTED_APPS_APIS:
-            _root, _name = app_config.name.split('.', 1)
-            if _root == 'ietf':
-                if not '.' in _name:
-                    _api = Api(api_name=_name)
-                    _module_dict[_name] = _api
-                    _api_list.append((_name, _api))
+        if "." in app_config.name and app_config.name not in OMITTED_APPS_APIS:
+            _root, _name = app_config.name.split(".", 1)
+            if _root == "ietf" and "." not in _name:
+                _api = Api(api_name=_name)
+                _module_dict[_name] = _api
+                _api_list.append((_name, _api))
+
 
 def autodiscover():
     """
@@ -51,6 +50,40 @@ def autodiscover():
 
 
 class ModelResource(tastypie.resources.ModelResource):
+    def dispatch(self, request_type, request, **kwargs):
+        """Turn a database error caused by request data into a bad request
+
+        Filter values reach the database with very little validation, and some of
+        them only fail once the query actually runs - below tastypie, and long
+        after build_filters() had any chance to reject them. Left alone those
+        surface as unhandled exceptions.
+
+        Only DataError is treated this way: it is the DBAPI error for a problem
+        with the data in the query, so it is the client's to fix. The other
+        DatabaseError subclasses (OperationalError, ProgrammingError,
+        InternalError) indicate a broken database or a bug of ours, and are left
+        alone so they still raise and report.
+
+        The database's message is logged rather than returned - it can quote the
+        offending value, and this response body is not escaped.
+        """
+        try:
+            return super().dispatch(request_type, request, **kwargs)
+        except DataError as err:
+            # The failed statement has aborted the transaction if there is one, so
+            # nothing more can be done with the connection until it is rolled back.
+            # Requests normally run in autocommit, where there is no transaction to
+            # roll back and this is a no-op, but without it the guard would quietly
+            # stop working if ATOMIC_REQUESTS were ever turned on: the 400 would be
+            # built and then lost when the atomic block failed to commit.
+            if not transaction.get_autocommit():
+                transaction.set_rollback(True)
+            log(f"DataError handling {request.method} {request.get_full_path()}: {err}")
+            raise BadRequest(
+                "The database could not process this request. This is usually a "
+                "malformed filter value."
+            )
+
     def post_detail(self, request, **kwargs):
         return HttpResponseNotAllowed(["GET"])
 
@@ -60,11 +93,11 @@ def generate_cache_key(self, *args, **kwargs):
 
         This is based off the current api_name/resource_name/args/kwargs.
         """
-        #smooshed = ["%s=%s" % (key, value) for key, value in kwargs.items()]
+        # smooshed = ["%s=%s" % (key, value) for key, value in kwargs.items()]
         smooshed = urlencode(kwargs)
 
         # Use a list plus a ``.join()`` because it's faster than concatenation.
-        return "%s:%s:%s:%s" % (self._meta.api_name, self._meta.resource_name, ':'.join(args), smooshed)
+        return f"{self._meta.api_name}:{self._meta.resource_name}:{':'.join(args)}:{smooshed}"
 
     def _z_aware_fromisoformat(self, value: str) -> datetime.datetime:
         """datetime.datetime.fromisoformat replacement that works with python < 3.11"""
@@ -78,9 +111,27 @@ def _z_aware_fromisoformat(self, value: str) -> datetime.datetime:
     def filter_value_to_python(
         self, value, field_name, filters, filter_expr, filter_type
     ):
-        py_value = super().filter_value_to_python(
-            value, field_name, filters, filter_expr, filter_type
-        )
+        try:
+            py_value = super().filter_value_to_python(
+                value, field_name, filters, filter_expr, filter_type
+            )
+        except TypeError:
+            # For "in" and "range" filters tastypie calls len() on the value, but
+            # string_to_python() has already mapped "true"/"false"/"nil"/"none" to a
+            # bool or None, which have no len().
+            raise InvalidFilterError(
+                f"Invalid value for the '{filter_type}' filter on '{field_name}'"
+            )
+        if filter_type == "range" and len(py_value) != 2:
+            # Django renders a range lookup as "BETWEEN %s AND %s" and indexes the
+            # value without checking its length, so anything other than exactly two
+            # values raises IndexError (or ValueError) when the query is compiled -
+            # long after this method has returned, where it can only become a 500.
+            # Reject it here, while it can still be reported as a bad request.
+            raise InvalidFilterError(
+                f"The '{filter_type}' filter on '{field_name}' requires exactly two "
+                f"comma-separated values"
+            )
         if isinstance(
             self.fields[field_name], tastypie.fields.DateTimeField
         ) and isinstance(py_value, str):
@@ -96,10 +147,13 @@ def filter_value_to_python(
         return py_value
 
 
-TIMEDELTA_REGEX = re.compile(r'^(?P<days>\d+d)?\s?(?P<hours>\d+h)?\s?(?P<minutes>\d+m)?\s?(?P<seconds>\d+s?)$')
+TIMEDELTA_REGEX = re.compile(
+    r"^(?P<days>\d+d)?\s?(?P<hours>\d+h)?\s?(?P<minutes>\d+m)?\s?(?P<seconds>\d+s?)$"
+)
+
 
 class TimedeltaField(ApiField):
-    dehydrated_type = 'timedelta'
+    dehydrated_type = "timedelta"
     help_text = "A timedelta field, with duration expressed in seconds. Ex: 132"
 
     def convert(self, value):
@@ -111,41 +165,58 @@ def convert(self, value):
 
             if match:
                 data = match.groupdict()
-                return datetime.timedelta(int(data['days']), int(data['hours']), int(data['minutes']), int(data['seconds']))
+                return datetime.timedelta(
+                    int(data["days"]),
+                    int(data["hours"]),
+                    int(data["minutes"]),
+                    int(data["seconds"]),
+                )
             else:
-                raise ApiFieldError("Timedelta provided to '%s' field doesn't appear to be a valid timedelta string: '%s'" % (self.instance_name, value))
+                raise ApiFieldError(
+                    f"Timedelta provided to '{self.instance_name}' field doesn't appear to be a valid timedelta string: '{value}'"
+                )
 
         return value
 
     def hydrate(self, bundle):
-        value = super(TimedeltaField, self).hydrate(bundle)
+        value = super().hydrate(bundle)
 
-        if value and not hasattr(value, 'seconds'):
+        if value and not hasattr(value, "seconds"):
             if isinstance(value, str):
                 try:
                     match = TIMEDELTA_REGEX.search(value)
 
                     if match:
                         data = match.groupdict()
-                        value = datetime.timedelta(int(data['days']), int(data['hours']), int(data['minutes']), int(data['seconds']))
+                        value = datetime.timedelta(
+                            int(data["days"]),
+                            int(data["hours"]),
+                            int(data["minutes"]),
+                            int(data["seconds"]),
+                        )
                     else:
                         raise ValueError()
                 except (ValueError, TypeError):
-                    raise ApiFieldError("Timedelta provided to '%s' field doesn't appear to be a valid datetime string: '%s'" % (self.instance_name, value))
+                    raise ApiFieldError(
+                        f"Timedelta provided to '{self.instance_name}' field doesn't appear to be a valid datetime string: '{value}'"
+                    )
 
             else:
-                raise ApiFieldError("Datetime provided to '%s' field must be a string: %s" % (self.instance_name, value))
+                raise ApiFieldError(
+                    f"Datetime provided to '{self.instance_name}' field must be a string: {value}"
+                )
 
         return value
 
+
 class ToOneField(tastypie.fields.ToOneField):
     "Subclass of tastypie.fields.ToOneField which adds caching in the dehydrate method."
 
     def dehydrate(self, bundle, for_list=True):
         foreign_obj = None
         previous_obj = None
         attrib = None
-        
+
         if callable(self.attribute):
             previous_obj = bundle.obj
             foreign_obj = self.attribute(bundle)
@@ -163,25 +234,34 @@ def dehydrate(self, bundle, for_list=True):
         if not foreign_obj:
             if not self.null:
                 if callable(self.attribute):
-                    raise ApiFieldError("The related resource for resource %s could not be found." % (previous_obj))
+                    raise ApiFieldError(
+                        f"The related resource for resource {previous_obj} could not be found."
+                    )
                 else:
-                    raise ApiFieldError("The model '%r' has an empty attribute '%s' and doesn't allow a null value." % (previous_obj, attrib))
+                    raise ApiFieldError(
+                        f"The model '{previous_obj!r}' has an empty attribute '{attrib}' and doesn't allow a null value."
+                    )
             return None
 
         fk_resource = self.get_related_resource(foreign_obj)
 
         # Up to this point we've copied the code from tastypie 0.13.1.  Now
         # we add caching.
-        cache_key = fk_resource.generate_cache_key('related', pk=foreign_obj.pk, for_list=for_list, )
+        cache_key = fk_resource.generate_cache_key(
+            "related",
+            pk=foreign_obj.pk,
+            for_list=for_list,
+        )
         dehydrated = fk_resource._meta.cache.get(cache_key)
         if dehydrated is None:
             fk_bundle = Bundle(obj=foreign_obj, request=bundle.request)
-            dehydrated = self.dehydrate_related(fk_bundle, fk_resource, for_list=for_list)
+            dehydrated = self.dehydrate_related(
+                fk_bundle, fk_resource, for_list=for_list
+            )
             fk_resource._meta.cache.set(cache_key, dehydrated)
         return dehydrated
 
 
-
 # XML 1.0 forbids all control characters except tab (#x9), LF (#xA), and CR (#xD).
 # Replace each with its Unicode control picture (U+2400 + codepoint) so the
 # substitution is lossless and the result is valid XML.
@@ -192,12 +272,19 @@ class Serializer(tastypie.serializers.Serializer):
     OPTION_ESCAPE_XML_INVALID = "datatracker-escape-xml-invalid"
 
     def format_datetime(self, data):
-        return data.astimezone(datetime.UTC).replace(tzinfo=None).isoformat(timespec="seconds") + "Z"
+        return (
+            data.astimezone(datetime.UTC)
+            .replace(tzinfo=None)
+            .isoformat(timespec="seconds")
+            + "Z"
+        )
 
     def to_simple(self, data, options):
         options = options or {}
         simple_data = super().to_simple(data, options)
-        if options.get(self.OPTION_ESCAPE_XML_INVALID, False) and isinstance(simple_data, str):
+        if options.get(self.OPTION_ESCAPE_XML_INVALID, False) and isinstance(
+            simple_data, str
+        ):
             # Replace control chars invalid in XML 1.0 with their Unicode
             # control pictures (U+2400-U+241F) so lxml won't reject the string.
             simple_data = _XML_INVALID_CTRL_RE.sub(

ietf/api/tests.py

@@ -12,7 +12,7 @@
 from importlib import import_module
 from pathlib import Path
 from random import randrange
-from urllib.parse import urljoin, urlencode
+from urllib.parse import quote, urlencode, urljoin
 
 from django.apps import apps
 from django.conf import settings
@@ -1666,6 +1666,81 @@ def test_api_top_level(self):
             self.assertIn(name, resource_list,
                         "Expected a REST API resource for %s, but didn't find one" % name)
 
+    def _assert_filter_is_bad_request(self, querystring, leaked):
+        """Assert a filter the database rejects gives a 400 that leaks nothing
+
+        Only one such request per test method: this class runs each test inside a
+        transaction, and the failed statement aborts it, so a second query in the
+        same test raises InternalError. Production requests run in autocommit and
+        are unaffected - each is a single request that returns immediately.
+        """
+        r = self.client.get("/api/v1/doc/document/?format=json&limit=1&" + querystring)
+        self.assertEqual(r.status_code, 400, "Expected 400 for %s" % querystring)
+        body = r.content.decode("utf-8")
+        # the database quotes its own diagnostics - none of that should come back
+        self.assertNotIn("invalid regular expression", body)
+        self.assertNotIn(leaked, body)
+
+    def test_database_error_unbalanced_bracket(self):
+        """A filter value the database rejects is a bad request, not a 500
+
+        An invalid regex is only rejected once the query runs, below anything
+        tastypie can validate. See ietf.api.ModelResource.dispatch.
+        """
+        self._assert_filter_is_bad_request("name__regex=%5B", "brackets")
+
+    def test_database_error_unbalanced_paren(self):
+        self._assert_filter_is_bad_request("name__regex=%28", "parentheses")
+
+    def test_database_error_bad_quantifier(self):
+        self._assert_filter_is_bad_request("name__iregex=" + quote("a{2,1}"), "quantifier")
+
+    def test_valid_regex_filter_still_works(self):
+        for q in ("name__regex=^draft-", "name__iregex=^DRAFT-", "name__regex=(quic|tls)"):
+            r = self.client.get("/api/v1/doc/document/?format=json&limit=1&" + quote(q, safe="=&"))
+            self.assertEqual(r.status_code, 200, "Expected 200 for %s" % q)
+
+    def test_malformed_range_filter(self):
+        """A range filter without exactly two values is a bad request, not a 500
+
+        Django renders a range lookup as "BETWEEN %s AND %s" and indexes the value
+        without checking its length, so a wrong number of values raises IndexError
+        when the query is compiled - too late for tastypie to report it as anything
+        but a 500. See ietf.api.ModelResource.filter_value_to_python.
+
+        Note the test client sets SERVER_NAME to "testserver", which makes tastypie
+        re-raise unhandled exceptions rather than converting them to a 500, so a
+        regression here surfaces as an error rather than a wrong status code.
+        """
+        # The double-encoded comma from the request that broke in production: %252C
+        # decodes to the literal text "%2C", so the value never splits into two.
+        r = self.client.get(
+            "/api/v1/doc/document/?format=json&limit=1&rev__range=02%252C99&type__slug=draft"
+        )
+        self.assertEqual(r.status_code, 400)
+
+        # note "%252C" not "%2C" - the latter is just a comma once the URL is decoded
+        for bad in ("", "02", "02%252C99", "02,99,77", "true", "nil"):
+            r = self.client.get("/api/v1/doc/document/?format=json&limit=1&rev__range=" + bad)
+            self.assertEqual(r.status_code, 400, "Expected 400 for rev__range=%s" % bad)
+
+        # a well-formed range filter still works
+        r = self.client.get("/api/v1/doc/document/?format=json&limit=1&rev__range=00,99")
+        self.assertEqual(r.status_code, 200)
+        # ... on a datetime field too, and via repeated parameters
+        r = self.client.get(
+            "/api/v1/doc/document/?format=json&limit=1"
+            "&time__range=2020-01-01T00:00:00Z&time__range=2030-01-01T00:00:00Z"
+        )
+        self.assertEqual(r.status_code, 200)
+
+        # "in" filters accept any number of values, including one
+        r = self.client.get("/api/v1/doc/document/?format=json&limit=1&rev__in=00")
+        self.assertEqual(r.status_code, 200)
+        # but not a value that string_to_python() turns into a bool
+        r = self.client.get("/api/v1/doc/document/?format=json&limit=1&rev__in=true")
+        self.assertEqual(r.status_code, 400)
+
     def test_all_model_resources_exist(self):
         client = Client(Accept='application/json')
         r = client.get("/api/v1")