Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#14498 closed (fixed)

Forms passed to FormWizard.process_step are not guaranteed to have clean validate data

Reported by: stas Owned by: lukeplant
Component: contrib.formtools Version: 1.2
Severity: Keywords: regression
Cc: niels.busch@…, stas@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The docs for FormWizard.process_step at source:trunk/django/contrib/formtools/wizard.py (line 263) read:

Hook for modifying the FormWizard's internal state, given a fully validated Form object. The Form is guaranteed to have clean, valid data.

There is however a case in FormWizard._ _call_ _ when this will not be true.

In source:trunk/django/contrib/formtools/wizard.py (line 95), the forms for each previous step are instantiated but they are not validated, which makes them not have the cleaned_data property.

for i in range(current_step):
    form = self.get_form(i, request.POST)
    if not self._check_security_hash(request.POST.get("hash_%d" % i, ''), request, form):
        return self.render_hash_failure(request, i)
    self.process_step(request, form, i)

A solution would be to call form.is_valid in the above loop, or to edit the docs to account for this particular case.

Attachments (1)

14498_regression_fix.patch (5.7 KB) - added by stas 4 years ago.
A patch fixing the regression discovered in comment:5

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to lukeplant
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 4 years ago by lukeplant

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 4 years ago by lukeplant

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

(In [14290]) Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

comment:4 Changed 4 years ago by lukeplant

(In [14291]) [1.2.X] Fixed #14498 - Forms passed to FormWizard.process_step are not guaranteed to have cleaned_data

Thanks to stas for the report.

Backport of [14290] from trunk.

comment:5 follow-up: Changed 4 years ago by niels

  • Cc niels.busch@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I believe this changeset broke the Formwizard in other ways. It is not possible to alter the form_list in process_step anymore, because the current_step form is instantiated before previous_steps processing has been done. If I change the initial step2 form to a different form when processing step1 the current_step form is clearly not the right one anymore.

Notice I am creating the formwizard in a view, so I don't have previous state cached.

comment:6 in reply to: ↑ 5 Changed 4 years ago by stas

  • Cc stas@… added

I can confirm the same issue that niels described. I have a 3-step wizard where the step1 form is set up depending on the user's choices in step0. When I go from step0 to step1 the wizard works as expected, because form = self.get_form(next_step) is executed after the processing loop from source:django/trunk/django/contrib/formtools/wizard.py#14574#L121.

However, when I go from step1 to step2, step1 gets instantiated with form = self.get_form(current_step, request.POST) in source:django/trunk/django/contrib/formtools/wizard.py#14574#L96, before the processing loop has a chance to modify the form list. As a result, step1 form is still the one I defined in my view when instantiating my wizard.

This is broken both in 1.2.X and on trunk.

Changed 4 years ago by stas

A patch fixing the regression discovered in comment:5

comment:7 Changed 3 years ago by lrekucki

  • Has patch set
  • Keywords regression added
  • milestone set to 1.3
  • Version changed from SVN to 1.2

Updated flags to current state.

comment:8 Changed 3 years ago by lukeplant

  • milestone 1.3 deleted
  • Resolution set to fixed
  • Status changed from reopened to closed

As a technicality, this is now a different bug. Since the original bug remains fixed, it is more helpful to open a new ticket - otherwise the metadata from the original bug is lost, and the change logs referring to bugs becoming confusing. I've opened the new ticket now - #15075

comment:9 Changed 3 years ago by stas

Thanks, lukeplant, for opening #15075. I wasn't sure if I should do it or not myself. I'll know better for the next time!

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.