diff --git a/django/forms/models.py b/django/forms/models.py index 34ff3c478c..1ec1398147 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -1232,8 +1232,7 @@ class ModelMultipleChoiceField(ModelChoiceField): def to_python(self, value): if not value: return [] - to_py = super(ModelMultipleChoiceField, self).to_python - return [to_py(val) for val in value] + return list(self._check_values(value)) def clean(self, value): if self.required and not value: @@ -1242,7 +1241,29 @@ class ModelMultipleChoiceField(ModelChoiceField): return self.queryset.none() if not isinstance(value, (list, tuple)): raise ValidationError(self.error_messages['list'], code='list') + qs = self._check_values(value) + # Since this overrides the inherited ModelChoiceField.clean + # we run custom validators here + self.run_validators(value) + return qs + + def _check_values(self, value): + """ + Given a list of possible PK values, returns a QuerySet of the + corresponding objects. Raises a ValidationError if a given value is + invalid (not a valid PK, not in the queryset, etc.) + """ key = self.to_field_name or 'pk' + # deduplicate given values to avoid creating many querysets or + # requiring the database backend deduplicate efficiently. + try: + value = frozenset(value) + except TypeError: + # list of lists isn't hashable, for example + raise ValidationError( + self.error_messages['list'], + code='list', + ) for pk in value: try: self.queryset.filter(**{key: pk}) @@ -1261,9 +1282,6 @@ class ModelMultipleChoiceField(ModelChoiceField): code='invalid_choice', params={'value': val}, ) - # Since this overrides the inherited ModelChoiceField.clean - # we run custom validators here - self.run_validators(value) return qs def prepare_value(self, value): diff --git a/docs/releases/1.6.10.txt b/docs/releases/1.6.10.txt index 20aa595b77..5b8f0cdec3 100644 --- a/docs/releases/1.6.10.txt +++ b/docs/releases/1.6.10.txt @@ -58,3 +58,12 @@ Note, however, that this view has always carried a warning that it is not hardened for production use and should be used only as a development aid. Now may be a good time to audit your project and serve your files in production using a real front-end web server if you are not doing so. + +Database denial-of-service with ``ModelMultipleChoiceField`` +============================================================ + +Given a form that uses ``ModelMultipleChoiceField`` and +``show_hidden_initial=True`` (not a documented API), it was possible for a user +to cause an unreasonable number of SQL queries by submitting duplicate values +for the field's data. The validation logic in ``ModelMultipleChoiceField`` now +deduplicates submitted values to address this issue. diff --git a/docs/releases/1.7.3.txt b/docs/releases/1.7.3.txt index 2da43658f8..234099590b 100644 --- a/docs/releases/1.7.3.txt +++ b/docs/releases/1.7.3.txt @@ -59,6 +59,15 @@ hardened for production use and should be used only as a development aid. Now may be a good time to audit your project and serve your files in production using a real front-end web server if you are not doing so. +Database denial-of-service with ``ModelMultipleChoiceField`` +============================================================ + +Given a form that uses ``ModelMultipleChoiceField`` and +``show_hidden_initial=True`` (not a documented API), it was possible for a user +to cause an unreasonable number of SQL queries by submitting duplicate values +for the field's data. The validation logic in ``ModelMultipleChoiceField`` now +deduplicates submitted values to address this issue. + Bugfixes ======== diff --git a/docs/spelling_wordlist b/docs/spelling_wordlist index 008da9fe93..7efc687e17 100644 --- a/docs/spelling_wordlist +++ b/docs/spelling_wordlist @@ -138,6 +138,7 @@ de Debian deconstruct deconstructing +deduplicates deepcopy deserialization deserialize diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 4ff900e1b2..4bb7c47eb2 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -1635,6 +1635,27 @@ class ModelMultipleChoiceFieldTests(TestCase): with self.assertNumQueries(1): template.render(Context({'field': field})) + def test_show_hidden_initial_changed_queries_efficiently(self): + class WriterForm(forms.Form): + persons = forms.ModelMultipleChoiceField( + show_hidden_initial=True, queryset=Writer.objects.all()) + + writers = (Writer.objects.create(name=str(x)) for x in range(0, 50)) + writer_pks = tuple(x.pk for x in writers) + form = WriterForm(data={'initial-persons': writer_pks}) + with self.assertNumQueries(1): + self.assertTrue(form.has_changed()) + + def test_clean_does_deduplicate_values(self): + class WriterForm(forms.Form): + persons = forms.ModelMultipleChoiceField(queryset=Writer.objects.all()) + + person1 = Writer.objects.create(name="Person 1") + form = WriterForm(data={}) + queryset = form.fields['persons'].clean([str(person1.pk)] * 50) + sql, params = queryset.query.sql_with_params() + self.assertEqual(len(params), 1) + class ModelOneToOneFieldTests(TestCase): def test_modelform_onetoonefield(self):