diff --git a/django/forms/models.py b/django/forms/models.py index 0f1f54f850f..37ef32dddbf 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -1221,8 +1221,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: @@ -1231,7 +1230,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}) @@ -1250,9 +1271,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 20aa595b771..5b8f0cdec33 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 2da43658f83..234099590bc 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 23465d2f4ea..da7ce09d299 100644 --- a/docs/spelling_wordlist +++ b/docs/spelling_wordlist @@ -134,6 +134,7 @@ dbshell de deconstruct deconstructing +deduplicates deepcopy deserialization deserialize diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 78c3bca4162..a1871bf2bfb 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -1573,6 +1573,27 @@ class ModelMultipleChoiceFieldTests(TestCase): self.assertTrue(form.is_valid()) self.assertTrue(form.has_changed()) + 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):