Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 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: master
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: UI/UX:

Description (last modified by ramiro)

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 6 years ago.
django.contrib.formtools.7828.patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 6 years ago by anonymous

  • Component changed from Uncategorized to django.contrib.formtools
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 follow-up: Changed 6 years ago by tbye@…

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 Changed 6 years ago by hanne.moa@…

  • Keywords BooleanField, hash failure added; BooleanField removed

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?

comment:4 in reply to: ↑ 2 Changed 6 years ago by hanne.moa@…

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 Changed 6 years ago by ramiro

  • Description modified (diff)

Changed 6 years ago by rishi

django.contrib.formtools.7828.patch

comment:6 Changed 6 years ago by rishi

  • 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 Changed 6 years ago by ElliottM

  • milestone set to 1.0

comment:8 Changed 6 years ago by rajeshdhawan

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 Changed 6 years ago by rajeshdhawan

  • Cc rajesh.dhawan@… added

comment:10 Changed 6 years ago by ElliottM

  • Triage Stage changed from Unreviewed to Accepted

comment:11 Changed 6 years ago by anonymous

  • Cc dev@… added

comment:12 Changed 6 years ago by adamjforster

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 Changed 6 years ago by adamjforster

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 Changed 6 years ago by mcroydon

  • Owner changed from nobody to mcroydon
  • Status changed from new to assigned

comment:15 Changed 6 years ago by mcroydon

  • Resolution set to duplicate
  • Status changed from assigned to closed

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 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.