From ed49efbf149b20c49f4551d21f96ab86a93ba3bd Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Fri, 31 Jul 2026 14:46:45 +0000 Subject: [PATCH 1/3] chore: ruff ruff --- ietf/api/__init__.py | 101 ++++++++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 34 deletions(-) diff --git a/ietf/api/__init__.py b/ietf/api/__init__.py index f4bfe8330e..84ef0058e4 100644 --- a/ietf/api/__init__.py +++ b/ietf/api/__init__.py @@ -1,28 +1,24 @@ # 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.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.fields import ApiField +import debug # noqa: F401 (pyflakes:ignore) + _api_list = [] OMITTED_APPS_APIS = ["ietf.status"] @@ -30,16 +26,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(): """ @@ -60,11 +57,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""" @@ -96,10 +93,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 +111,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 +162,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 +180,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 +218,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( From a99ecaba5cd691ecfbaaff9cfaed9792e73a9bc0 Mon Sep 17 00:00:00 2001 From: Robert Sparks Date: Fri, 31 Jul 2026 14:47:46 +0000 Subject: [PATCH 2/3] fix: handle exception for API filter values that fail below tastypie Filter values reach the ORM with very little validation, and some of them only fail after tastypie has stopped looking at them - either while the query is being built or once it runs. Those surfaced as unhandled exceptions. Add guards at both layers, in the ModelResource that every datatracker resource subclasses: dispatch() converts DataError into a bad request, covering values that are only rejected once the query reaches the database. Only DataError is treated this way, as the DBAPI error for a problem with the data in the query. OperationalError, ProgrammingError and InternalError indicate a broken database or a bug of ours, and still raise and report. The database's message is logged rather than returned, since it can quote the offending value into a response body that is not escaped. --- ietf/api/__init__.py | 62 ++++++++++++++++++++++++++++++++--- ietf/api/tests.py | 77 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 134 insertions(+), 5 deletions(-) diff --git a/ietf/api/__init__.py b/ietf/api/__init__.py index 84ef0058e4..30254221d1 100644 --- a/ietf/api/__init__.py +++ b/ietf/api/__init__.py @@ -10,14 +10,16 @@ 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 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 = [] @@ -48,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"]) @@ -75,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): diff --git a/ietf/api/tests.py b/ietf/api/tests.py index 87f7d684d3..9a7264bf40 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 +from urllib.parse import quote, urljoin from django.apps import apps from django.conf import settings @@ -1524,6 +1524,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") From d7e5cd2e42adc4ce4812dbe63bc96f0a127c2e86 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Tue, 4 Aug 2026 20:36:18 -0300 Subject: [PATCH 3/3] test: fix merge cruft --- ietf/api/tests.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ietf/api/tests.py b/ietf/api/tests.py index a2eed4fda2..41a7f65d59 100644 --- a/ietf/api/tests.py +++ b/ietf/api/tests.py @@ -58,8 +58,7 @@ class CustomApiTests(TestCase): settings_temp_path_overrides = TestCase.settings_temp_path_overrides + ['AGENDA_PATH'] def test_api_help_page(self): - url = urlreverse(' - .api_help') + url = urlreverse('ietf.api.views.api_help') r = self.client.get(url) self.assertContains(r, 'The datatracker API', status_code=200)