diff --git a/ietf/api/__init__.py b/ietf/api/__init__.py index f4bfe8330e..30254221d1 100644 --- a/ietf/api/__init__.py +++ b/ietf/api/__init__.py @@ -1,28 +1,26 @@ # 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"] @@ -30,16 +28,17 @@ # 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\d+d)?\s?(?P\d+h)?\s?(?P\d+m)?\s?(?P\d+s?)$') +TIMEDELTA_REGEX = re.compile( + r"^(?P\d+d)?\s?(?P\d+h)?\s?(?P\d+m)?\s?(?P\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,33 +165,50 @@ 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." @@ -145,7 +216,7 @@ 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( diff --git a/ietf/api/tests.py b/ietf/api/tests.py index b8b69f8345..41a7f65d59 100644 --- a/ietf/api/tests.py +++ b/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")