Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#7038 closed (duplicate)

FormWizard fails on forms with BooleanField when field is unselected

Reported by: anonymous Owned by: mcroydon
Component: contrib.formtools Version: dev
Severity: Keywords: FormWizard, BooleanField, hash failure
Cc: rajesh.dhawan@…, dev@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

The form wizard fails to match its security hash when processing a form containing at least one BooleanField where the BooleanField's checkbox is unselected (e.g. False). This results in (on my machine) the form containing the BooleanField being redisplayed. If *all* the BooleanFields on a form are selected, it appears to work correctly.

I was able to workaround (not fix) by overriding the security_hash method as follows:

    def security_hash(self, request, form):

        for bf in form:
            if bf.field.__class__ != BooleanField:
                data = [(bf.name, bf.data or u'')] + [settings.SECRET_KEY]

        # Use HIGHEST_PROTOCOL because it's the most efficient. It requires
        # Python 2.3, but Django requires 2.3 anyway, so that's OK.
        pickled = pickle.dumps(data, protocol=pickle.HIGHEST_PROTOCOL)
        return md5.new(pickled).hexdigest()

The problem appears (to my untrained eye) to be in the line assigning the value of data where bf.data = False and thus gets assigned .

I'm using SVN 7412.

Attachments (1)

django.contrib.formtools.7828.patch (842 bytes ) - added by rishi 16 years ago.
django.contrib.formtools.7828.patch

Download all attachments as: .zip

Change History (17)

comment:1 by anonymous, 16 years ago

Component: Uncategorizeddjango.contrib.formtools

comment:2 by tbye@…, 16 years ago

When attempting to use the sample workaround above with rev. 7545 I hit an issue. Here's my updated sample to correct the problems I ran into.

def security_hash(self, request, form):
        for bf in form:
            if bf.field != BooleanField:
                 data = [(bf.name, bf.data or '')] + [settings.SECRET_KEY]

        # Use HIGHEST_PROTOCOL because it's the most efficient. It requires 
        # Python 2.3, but Django requires 2.3 anyway, so that's OK. 
        pickled = pickle.dumps(data, protocol=pickle.HIGHEST_PROTOCOL) 
        return md5.new(pickled).hexdigest()

comment:3 by hanne.moa@…, 16 years ago

Keywords: hash failure added

I get this also... not an easy bug to detect when one has a wizard with many forms! maybe a better solution would be to explicitly set every unset BooleanField to False as soon as possible, that is: well before the hashing?

in reply to:  2 comment:4 by hanne.moa@…, 16 years ago

Replying to tbye@tbye.com:

When attempting to use the sample workaround above with rev. 7545 I hit an issue. Here's my updated sample to correct the problems I ran into.

def security_hash(self, request, form):

for bf in form:

if bf.field != BooleanField:

data = [(bf.name, bf.data or )] + [settings.SECRET_KEY]

This also means that only the last field per form, whatever that is, is hashed at all, as 'data' is replaced for almost every trip through the 'for'. Ergo, one might as well not have the hashing at all.

The real problem is that sql and web and boolean logic disagrees as to what a checkbox (CheckboxInput in django's case) is supposed to mean. Sql has three values for a bool: true, false, and unset. Web only has on/True and unset. Django has two types of boolean fields, using the same widget: BooleanField, which must be set and has two possible values: True and False, and NullboleanField, which is like sql in that it has three values: True, False and None (unset). The checkbox-widget however, obeys the web-rule of True vs. unset and ergo doesn't really fit with either of the fields =) See also #7051, #7190, #6937, #6914, #5957

comment:5 by Ramiro Morales, 16 years ago

Description: modified (diff)

by rishi, 16 years ago

django.contrib.formtools.7828.patch

comment:6 by rishi, 16 years ago

Has patch: set

I can also reproduce this issue. The problem is with the line in wizard.py:

data = [(bf.name, bf.data or '') for bf in form] + [settings.SECRET_KEY]

Where can bf.data == False when the hash is created, and bf.data == u'False' when the hash is validated. So the hashes don't match and we get a hash error. It seems like an easy fix would be to do the following:

#data = [(bf.name, unicode(bf.data) if bf.data is not None else '') for bf in form] + [settings.SECRET_KEY]

But that fails due an odd behavior with pickling where the pickled data doesn't produce the exact binary output:

>>> import cPickle as pickle
>>>
>>> u'False' == unicode(False)
True
>>> pickle1 = pickle.dumps(u'False', pickle.HIGHEST_PROTOCOL)
>>> pickle2 = pickle.dumps(unicode(False), pickle.HIGHEST_PROTOCOL)
>>> pickle1 == pickle2
False
>>> pickle1
'\x80\x02X\x05\x00\x00\x00Falseq\x01.'
>>> pickle2
'\x80\x02X\x05\x00\x00\x00False.'
>>>
>>>
>>> f1 = u'False'
>>> f2 = unicode(False)
>>> f1 == f2
True
>>> pickle1 = pickle.dumps(f1, pickle.HIGHEST_PROTOCOL)
>>> pickle2 = pickle.dumps(f2, pickle.HIGHEST_PROTOCOL)
>>> pickle1 == pickle2
True
>>> pickle1
'\x80\x02X\x05\x00\x00\x00Falseq\x01.'
>>> pickle2
'\x80\x02X\x05\x00\x00\x00Falseq\x01.'
>>>

It seems like a better approach would be to remove the pickle operation and use a different serialization method (e.g., sorted list of key values converted to a string) as the md5 input. I haven't done that in the attached patch and can leave that for someone else to choose to do. For now, the attached patch fixes it by expanding the list comprehension.

comment:7 by ElliottM, 16 years ago

milestone: 1.0

comment:8 by Rajesh Dhawan, 16 years ago

This problem is similar to the FormPreview problem of #6209. I have a fix uploaded there. Whatever fix is incorporated, it should be consistent between the hash calculations of the FormPreview and those of the FormWizard.

comment:9 by Rajesh Dhawan, 16 years ago

Cc: rajesh.dhawan@… added

comment:10 by ElliottM, 16 years ago

Triage Stage: UnreviewedAccepted

comment:11 by anonymous, 16 years ago

Cc: dev@… added

comment:12 by Adam J. Forster, 16 years ago

The patch django.contrib.formtools.7828.patch does not work for me when there are multiple blank checkboxes on a single form. It does work if there are two checkboxes where one is ticked and the other is blank, but if I leave both blank the form wizard throws me back to that step.

I am using trunk revision [8222]

After having looked at ticket #6209 I have replaced line 50 of wizard.py with the code from this patch patch-#6209.diff, so far this seems to have completely solved my problems.

I hope that this information helps.

comment:13 by Adam J. Forster, 16 years ago

Sorry the above: "I have replaced line 50 of wizard.py"

Should read: "I have replaced line 150 of wizard.py"

The link is correct.

comment:14 by mcroydon, 16 years ago

Owner: changed from nobody to mcroydon
Status: newassigned

comment:15 by mcroydon, 16 years ago

Resolution: duplicate
Status: assignedclosed

The patch to Ticket #6209 solves this using BooleanField's clean() method, which should solve your problem. Marking this as a dupe of that ticket.

comment:16 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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