diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 6c202c8e613c..2de07fde7e33 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -2050,11 +2050,6 @@ def changelist_view(self, request, extra_context=None): # me back" button on the action confirmation page. return HttpResponseRedirect(request.get_full_path()) - # If we're allowing changelist editing, we need to construct a formset - # for the changelist given all the fields to be edited. Then we'll - # use the formset to validate/process POSTed data. - formset = cl.formset = None - # Handle POSTed bulk-edit data. if request.method == "POST" and cl.list_editable and "_save" in request.POST: if not self.has_change_permission(request): @@ -2063,13 +2058,11 @@ def changelist_view(self, request, extra_context=None): modified_objects = self._get_list_editable_queryset( request, FormSet.get_default_prefix() ) - formset = cl.formset = FormSet( - request.POST, request.FILES, queryset=modified_objects - ) - if formset.is_valid(): + cl.formset = FormSet(request.POST, request.FILES, queryset=modified_objects) + if cl.formset.is_valid(): changecount = 0 with transaction.atomic(using=router.db_for_write(self.model)): - for form in formset.forms: + for form in cl.formset.forms: if form.has_changed(): obj = self.save_form(request, form, change=True) self.save_model(request, obj, form, change=True) @@ -2095,11 +2088,11 @@ def changelist_view(self, request, extra_context=None): # Handle GET -- construct a formset for display. elif cl.list_editable and self.has_change_permission(request): FormSet = self.get_changelist_formset(request) - formset = cl.formset = FormSet(queryset=cl.result_list) + cl.formset = FormSet(queryset=cl.result_list) # Build the list of media to be used by the formset. - if formset: - media = self.media + formset.media + if cl.formset: + media = self.media + cl.formset.media else: media = self.media diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 8c9118808e8e..40a6b3bf3a86 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -101,7 +101,7 @@ def __init__( self.preserved_filters = model_admin.get_preserved_filters(request) self.sortable_by = sortable_by self.search_help_text = search_help_text - + self.formset = None # Get search parameters from the query string. _search_form = self.search_form_class(request.GET) if not _search_form.is_valid(): diff --git a/django/core/serializers/xml_serializer.py b/django/core/serializers/xml_serializer.py index d43865c25743..d8ffbdf00a98 100644 --- a/django/core/serializers/xml_serializer.py +++ b/django/core/serializers/xml_serializer.py @@ -10,7 +10,7 @@ from django.apps import apps from django.conf import settings -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, SuspiciousOperation from django.core.serializers import base from django.db import DEFAULT_DB_ALIAS, models from django.utils.xmlutils import SimplerXMLGenerator, UnserializableContentError @@ -411,6 +411,8 @@ def m2m_convert(n): try: for c in node.getElementsByTagName("object"): values.append(m2m_convert(c)) + except SuspiciousOperation: + raise except Exception as e: if isinstance(e, ObjectDoesNotExist) and self.handle_forward_references: return base.DEFER_FIELD @@ -439,17 +441,15 @@ def _get_model_from_node(self, node, attr): ) +def check_element_type(element): + if element.childNodes: + raise SuspiciousOperation(f"Unexpected element: {element.tagName!r}") + return element.nodeType in (element.TEXT_NODE, element.CDATA_SECTION_NODE) + + def getInnerText(node): - """Get the inner text of a DOM node and any children one level deep.""" - # inspired by - # https://mail.python.org/pipermail/xml-sig/2005-March/011022.html return "".join( - [ - element.data - for child in node.childNodes - for element in (child, *child.childNodes) - if element.nodeType in (element.TEXT_NODE, element.CDATA_SECTION_NODE) - ] + [child.data for child in node.childNodes if check_element_type(child)] ) diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 831315ba6a92..feb0441bab80 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -1032,9 +1032,9 @@ def _get_default(self): if self.has_db_default(): from django.db.models.expressions import DatabaseDefault - return lambda: DatabaseDefault( - self._db_default_expression, output_field=self - ) + default = DatabaseDefault(self._db_default_expression, output_field=self) + + return lambda: default if ( not self.empty_strings_allowed diff --git a/docs/releases/6.1.txt b/docs/releases/6.1.txt index b5b47a11d836..57129bfb666f 100644 --- a/docs/releases/6.1.txt +++ b/docs/releases/6.1.txt @@ -303,6 +303,10 @@ Serialization ``()``. This ensures primary keys are serialized when using :option:`dumpdata --natural-primary`. +* The XML deserializer now raises + :exc:`~django.core.exceptions.SuspiciousOperation` when it encounters + unexpected nested tags. + Signals ~~~~~~~ diff --git a/docs/topics/serialization.txt b/docs/topics/serialization.txt index f801df05617d..1a129072e09c 100644 --- a/docs/topics/serialization.txt +++ b/docs/topics/serialization.txt @@ -198,7 +198,8 @@ lowercase name of the model ("session") separated by a dot. Each field of the object is serialized as a ````-element sporting the fields "type" and "name". The text content of the element represents the value -that should be stored. +that should be stored. (If the element contains child tags, +:exc:`~django.core.exceptions.SuspiciousOperation` is raised.) Foreign keys and other relational fields are treated a little bit differently: @@ -237,6 +238,11 @@ This example links the given user with the permission models with PKs 46 and XHTML, XML and Control Codes `_. +.. versionchanged:: 6.1 + + :exc:`~django.core.exceptions.SuspiciousOperation` is raised when + unexpected nested tags are found. + .. _serialization-formats-json: JSON diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 68f6ca453eaf..319d6259f6a9 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -240,7 +240,6 @@ def test_result_list_empty_changelist_value(self): request.user = self.superuser m = ChildAdmin(Child, custom_site) cl = m.get_changelist_instance(request) - cl.formset = None template = Template( "{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}" ) @@ -285,7 +284,6 @@ def test_result_list_set_empty_value_display_on_admin_site(self): admin.site.empty_value_display = "???" m = ChildAdmin(Child, admin.site) cl = m.get_changelist_instance(request) - cl.formset = None template = Template( "{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}" ) @@ -310,7 +308,6 @@ def test_result_list_set_empty_value_display_in_model_admin(self): request.user = self.superuser m = EmptyValueChildAdmin(Child, admin.site) cl = m.get_changelist_instance(request) - cl.formset = None template = Template( "{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}" ) @@ -341,7 +338,6 @@ def test_result_list_html(self): request.user = self.superuser m = ChildAdmin(Child, custom_site) cl = m.get_changelist_instance(request) - cl.formset = None template = Template( "{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}" ) @@ -370,7 +366,6 @@ def test_action_checkbox_for_model_with_dunder_html(self): request = self._mocked_authenticated_request("/grandchild/", self.superuser) m = GrandChildAdmin(GrandChild, custom_site) cl = m.get_changelist_instance(request) - cl.formset = None template = Template( "{% load admin_list %}{% spaceless %}{% result_list cl %}{% endspaceless %}" ) diff --git a/tests/fixtures/fixtures/fixture9.xml b/tests/fixtures/fixtures/fixture9.xml index 100f63d10661..51003093f376 100644 --- a/tests/fixtures/fixtures/fixture9.xml +++ b/tests/fixtures/fixtures/fixture9.xml @@ -1,7 +1,7 @@ - + Stephane Grappelli @@ -19,11 +19,11 @@ - Artist formerly known as "Prince" + Artist formerly known as "Prince" - + Artist formerly known as "Prince" diff --git a/tests/fixtures/fixtures/invalid_deeply_nested_elements.xml b/tests/fixtures/fixtures/invalid_deeply_nested_elements.xml index cd164d3d2601..e3ed67f32061 100644 --- a/tests/fixtures/fixtures/invalid_deeply_nested_elements.xml +++ b/tests/fixtures/fixtures/invalid_deeply_nested_elements.xml @@ -3,7 +3,8 @@ - Django pony + + Django pony diff --git a/tests/fixtures/fixtures/invalid_deeply_nested_elements_natural_key.xml b/tests/fixtures/fixtures/invalid_deeply_nested_elements_natural_key.xml new file mode 100644 index 000000000000..7433fad6c7f6 --- /dev/null +++ b/tests/fixtures/fixtures/invalid_deeply_nested_elements_natural_key.xml @@ -0,0 +1,14 @@ + + + + + Music for all ages + + + + Artist formerly known as Prince + + + + + diff --git a/tests/fixtures/tests.py b/tests/fixtures/tests.py index 7511569d2102..dd03343027f3 100644 --- a/tests/fixtures/tests.py +++ b/tests/fixtures/tests.py @@ -10,6 +10,7 @@ from django.apps import apps from django.contrib.sites.models import Site from django.core import management +from django.core.exceptions import SuspiciousOperation from django.core.files.temp import NamedTemporaryFile from django.core.management import CommandError from django.core.management.commands.dumpdata import ProxyModelWarning @@ -23,7 +24,6 @@ CircularA, CircularB, NaturalKeyThing, - Person, PrimaryKeyUUIDModel, ProxySpy, Spy, @@ -522,12 +522,13 @@ def test_loading_and_dumping(self): ) def test_deeply_nested_elements(self): - """Text inside deeply-nested tags is skipped.""" - management.call_command( - "loaddata", "invalid_deeply_nested_elements.xml", verbosity=0 - ) - person = Person.objects.get(pk=1) - self.assertEqual(person.name, "Django") # not "Django pony" + """Text inside deeply-nested tags raises SuspiciousOperation.""" + for file in [ + "invalid_deeply_nested_elements.xml", + "invalid_deeply_nested_elements_natural_key.xml", + ]: + with self.subTest(file=file), self.assertRaises(SuspiciousOperation): + management.call_command("loaddata", file, verbosity=0) def test_dumpdata_with_excludes(self): # Load fixture1 which has a site, two articles, and a category diff --git a/tests/serializers/test_deserialization.py b/tests/serializers/test_deserialization.py index a718a990385a..b6a0818c427a 100644 --- a/tests/serializers/test_deserialization.py +++ b/tests/serializers/test_deserialization.py @@ -1,15 +1,14 @@ import json -import time +import textwrap import unittest +from django.core.exceptions import SuspiciousOperation from django.core.serializers.base import DeserializationError, DeserializedObject from django.core.serializers.json import Deserializer as JsonDeserializer from django.core.serializers.jsonl import Deserializer as JsonlDeserializer from django.core.serializers.python import Deserializer from django.core.serializers.xml_serializer import Deserializer as XMLDeserializer -from django.db import models from django.test import SimpleTestCase -from django.test.utils import garbage_collect from .models import Author @@ -138,52 +137,23 @@ def test_yaml_bytes_input(self): self.assertEqual(first_item.object, self.jane) self.assertEqual(second_item.object, self.joe) - def test_crafted_xml_performance(self): - """The time to process invalid inputs is not quadratic.""" - - def build_crafted_xml(depth, leaf_text_len): - nested_open = "" * depth - nested_close = "" * depth - leaf = "x" * leaf_text_len - field_content = f"{nested_open}{leaf}{nested_close}" - return f""" - - - {field_content} - m - - - """ - - def deserialize(crafted_xml): - iterator = XMLDeserializer(crafted_xml) - garbage_collect() - - start_time = time.perf_counter() - result = list(iterator) - end_time = time.perf_counter() - - self.assertEqual(len(result), 1) - self.assertIsInstance(result[0].object, models.Model) - return end_time - start_time - - def assertFactor(label, params, factor=2): - factors = [] - prev_time = None - for depth, length in params: - crafted_xml = build_crafted_xml(depth, length) - elapsed = deserialize(crafted_xml) - if prev_time is not None: - factors.append(elapsed / prev_time) - prev_time = elapsed - - with self.subTest(label): - # Assert based on the average factor to reduce test flakiness. - self.assertLessEqual(sum(factors) / len(factors), factor) - - assertFactor( - "varying depth, varying length", - [(50, 2000), (100, 4000), (200, 8000), (400, 16000), (800, 32000)], - 2, + def test_crafted_xml_rejected(self): + depth = 100 + leaf_text_len = 1000 + nested_open = "" * depth + nested_close = "" * depth + leaf = "x" * leaf_text_len + field_content = f"{nested_open}{leaf}{nested_close}" + crafted_xml = textwrap.dedent( + f""" + + + {field_content} + m + + """ ) - assertFactor("constant depth, varying length", [(100, 1), (100, 1000)], 2) + + msg = "Unexpected element: 'nested'" + with self.assertRaisesMessage(SuspiciousOperation, msg): + list(XMLDeserializer(crafted_xml))