Opened 9 years ago
Closed 9 years ago
#22638 closed Bug (fixed)
Form wizard may raise unreasonable exceptions in case of SECRET_KEY change
Reported by: | Sasha Romijn | Owned by: | Sasha Romijn |
---|---|---|---|
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 9 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
Owner: | changed from nobody to nott |
---|---|
Status: | new → assigned |
comment:4 Changed 9 years ago by
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 9 years ago by
Owner: | nott deleted |
---|---|
Status: | assigned → new |
comment:7 Changed 9 years ago by
Cc: | denis.cornehl@… added |
---|---|
Patch needs improvement: | set |
comment:8 Changed 9 years ago by
Patch needs improvement: | unset |
---|
Existing PR updated: https://github.com/django/django/pull/2673
comment:9 Changed 9 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:10 Changed 9 years ago by
Owner: | set to Sasha Romijn |
---|---|
Status: | new → assigned |
comment:11 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
WizardViewCookieModified, a subclass of SuspiciousOperation, is raised in this situation since 1.6