Opened 4 years ago

Closed 3 years ago

#16820 closed Bug (fixed)

CheckboxInput.value_from_datadict with '0'

Reported by: danfairs Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

CheckboxInput's value_from_datadict returns the literal value if the value's lowercased form is not in 'true' or 'false'. This looks like it's based on the idea that if any value is present in the data dictionary for a checkbox, then the value of the checkbox is True.

This is fine as far as it goes. However, a problem arises if the value being posted happens to be the string '0' (single character string, character is 0). This *should* be interpreted as a True value (ie. checkbox checked). When the string '0' is passed into BooleanField.to_python() (as would happen if a CheckboxInput is used on a ModelForm's BooleanField), False will be returned (presumably for RadioSelect compatibility). So, it is False that will be present in the cleaned_data dictionary that is used in form clean_* methods.

(Normally, a browser wouldn't provide '0' as a value, but badly-behaving clients may do - particularly if Django forms are used as part of data validation an API, which is where we discovered this problem.)

A fix for this is for CheckboxInput.value_from_datadict to always bool() its return value. This will result in True being returned for the case of '0' being in the data dictionary, which of course won't be special-cased in BooleanField.to_python.

I'll attach a patch and test shortly.

Attachments (2)

checkbox.patch (2.8 KB) - added by danfairs 4 years ago.
CheckboxInput.value_to_datadict patch with tests
16820-2.diff (2.0 KB) - added by claudep 3 years ago.
Updated patch and simplified tests

Download all attachments as: .zip

Change History (9)

Changed 4 years ago by danfairs

CheckboxInput.value_to_datadict patch with tests

comment:1 Changed 4 years ago by danfairs

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by Alex

This method is supposed to handle stuff that browsers send, is there some browser that sends this value?

comment:3 Changed 4 years ago by danfairs

To my knowledge, no browser sends '0'. We discovered this while testing our
API against various (custom) clients, some of which didn't behave quite as
we'd asked, and sent '0'.

We're reusing our web forms for the API to do data validation, wherever
possible.

The current behaviour led to a surprising - to our eyes - situation where
you could have code like this (abridged from our internal test suite):

class FlagModel(models.Model):
    flag = models.BooleanField()


class DefaultModelForm(forms.ModelForm):
    # This form will get an auto-generated BooleanField, which uses
    # the CheckboxInput widget.
    class Meta:
        model = FlagModel

    def clean(self):
        # Now, because BooleanField.to_python knows about '0',
        # cleaned_data actually contains False. '0' and '1' can
        # be posted by RadioSelect widgets.
        cleaned_data = super(DefaultModelForm, self).clean()
        if cleaned_data.get('flag') != False:
            raise forms.ValidationError(u'Flag must be false!')
        return cleaned_data


class CheckTestCase(TestCase):

    def setUp(self):
        self.m = FlagModel.objects.create(flag=True)
 
    def test_checkbox(self):
        formset_class = forms.models.modelformset_factory(
            FlagModel,
            max_num=1,
            form=DefaultModelForm
        )
        formset = formset_class({
            'form-INITIAL_FORMS': '1',
            'form-TOTAL_FORMS': '1',
            'form-0-id': str(self.m.pk),
            'form-0-flag': '0',
        }, queryset=FlagModel.objects.all())

        # Before the patch is applied, formset.is_valid() will return
        # True - with the implication that the flag field has been
        # checked and has been set to False
        #self.assertFalse(formset.is_valid())

        # Since the form validates, flag should be false... right?
        formset.save()
        m = FlagModel.objects.get(pk=self.m.pk)

        # To our surprise, m.flag actually turns out to be True at this point,
        # so this assertion fails.
        self.assertFalse(m.flag)

What made this more confusing was that other fields on the real model actually
did save.

comment:4 Changed 4 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

Per the HTML spec, the browser sends:

  • <name>=<value> for checked checkboxes,
  • nothing for unchecked checkboxes.

See http://www.w3.org/TR/html4/interact/forms.html#checkbox

Put this in a file called "test.html", load it in a browser, and see for yourself:

<form action="test.html" method="GET">
    <input type="submit" value="Go">
    <input type="checkbox" name="nuts" value="0">
    <input type="checkbox" name="foo" value="true">
    <input type="checkbox" name="bar" value="false">
</form>

Per the spec, the proper implementation would be simply:

    def value_from_datadict(self, data, files, name):
        return name in data

comment:5 Changed 4 years ago by aaugustin

#17114 is related.

Changed 3 years ago by claudep

Updated patch and simplified tests

comment:6 Changed 3 years ago by claudep

Updated patch and reused existing test cases instead of creating new ones.

comment:7 Changed 3 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In fbb664066f8970279af020cf163cb364156290c3:

[1.5.x] Fixed #16820 -- Treated '0' value as True for checkbox inputs

Thanks Dan Fairs for the report and the initial patch.

Backport of be29329cc from master.

Note: See TracTickets for help on using tickets.
Back to Top