Opened 10 months ago

Closed 9 months ago

#22638 closed Bug (fixed)

Form wizard may raise unreasonable exceptions in case of SECRET_KEY change

Reported by: erikr Owned by: erikr
Component: contrib.formtools Version: 1.6
Severity: Normal Keywords:
Cc: denis.cornehl@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Our form wizard has two storage options: sessions and cookies, with SessionWizardView and CookieWizardView. To prevent manipulation, the cookies storage uses the signed cookies from django.core.signing. This creates a signature based on the SECRET_KEY. If the secret key is changed, request.get_signed_cookie will raise an exception, in which case the storage will raise WizardViewCookieModified, a subclass of SuspiciousOperation.

The cookie is loaded very early in the rendering of a the form wizard view. This means that if a user starts a form wizard, and the secret key is changed, any requests to the wizard will result in an exception and likely a 500 error. The user can only recover from this by deleting the cookie or restarting the browser (it seems to only persist in the current session).

It may appear sensible to raise a SuspiciousOperation for a possible cookie manipulation, but we currently don't do this in any other place, like sessions. Currently, user may suddenly get 500 errors for no clear reason, and the developer of the project has no ability to help them. Leaving this as is may also discourage people from rotating their secret key when needed.

I suggest that in case of an invalid wizard cookie, we simply ignore the value and thereby return the user to the first step.

Change History (11)

comment:1 Changed 10 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 10 months ago by nott

WizardViewCookieModified, a subclass of SuspiciousOperation, is raised in this situation since 1.6

comment:3 Changed 10 months ago by nott

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

comment:4 Changed 10 months ago by erikr

  • Has patch set

Apologies, forgot to assign this to myself before working on it. Wrote a PR on https://github.com/django/django/pull/2673

comment:5 Changed 10 months ago by erikr

  • Owner nott deleted
  • Status changed from assigned to new

comment:6 Changed 10 months ago by syphar

tested the behavior with a test-application, looks fine to me.

comment:7 Changed 10 months ago by syphar

  • Cc denis.cornehl@… added
  • Patch needs improvement set

comment:8 Changed 9 months ago by erikr

  • Patch needs improvement unset

comment:9 Changed 9 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 9 months ago by erikr

  • Owner set to erikr
  • Status changed from new to assigned

comment:11 Changed 9 months ago by Erik Romijn <eromijn@…>

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

In ba5ddf7aed542d25f0fdb25a04d87305de0f3972:

Fixed #22638 -- Changed CookieWizardView to ignore invalid cookies

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