[Django] #30952: KeyError: '_password_reset_token' during password reset
Django
-----------------------------------------+------------------------
Reporter: defigor | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
We are started to get the following exception, when users are trying to
reset the password:
KeyError: '_password_reset_token'
django/contrib/sessions/backends/base.py in __delitem__ at line 62
django/contrib/auth/views.py in form_valid at line 300
django/views/generic/edit.py in post at line 142
django/views/generic/base.py in dispatch at line 88
django/contrib/auth/views.py in dispatch at line 270
django/views/decorators/cache.py in _wrapped_view_func at line 44
django/utils/decorators.py in _wrapper at line 45
django/views/decorators/debug.py in sensitive_post_parameters_wrapper at
line 76
django/utils/decorators.py in _wrapper at line 45
django/views/generic/base.py in view at line 68
django/core/handlers/base.py in _get_response at line 124
django/core/handlers/base.py in _get_response at line 126
django/core/handlers/exception.py in inner at line 34
It doesn't happen every time and we were unable to reproduce locally, but
it has already happened to our customers more than 300 times on our
production system.
Have anyone seen this issue?
Django version is 2.1.10
Please let me know if I need to provide any other information?
--
Ticket URL: <https://code.djangoproject.com/ticket/30952>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* type: Uncategorized => Bug
* resolution: => needsinfo
Comment:
Thanks for this ticket, however without a reproducible scenario we're not
able to check or fix this issue. It looks like a race condition, e.g.
multiple submission (double-click?) of the same password reset form.
Moreover Django 2.1 is in Extended support so try to reproduce this issue
on the master branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:1>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mark Gregson):
I'm occasionally seeing this same behaviour in 2.2.12. Unfortunately I
haven't worked out how to reproduce it either however I spent some time
testing and analysing the code to understand how it might happen. I
haven't identified the cause but I think I have ruled out a race-
condition.
A race-condition did indeed seem likely at first simply because there is
no other obvious cause however I think it's not possible: the session is
loaded (from the DB in my case) by the session middleware before the view
function is called and _password_reset_token must be in the session at
dispatch() in order to proceed to form_valid(). A second process
modifying the stored session will not affect the first's in-memory copy of
the session while the first is between dispatch() and form_valid(), hence
no race-condition. I'm not familiar with the session code so maybe there
is some way for the session to be reloaded that would enable a race-
condition.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:2>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrey Shakurov):
* cc: Andrey Shakurov (added)
* status: closed => new
* resolution: needsinfo =>
Comment:
The same issue can be reproduced in newer versions. I've tested it in
3.0.4 with database-backed sessions and all of the standard
django.contrib.auth.urls
Steps to reproduce:
1. Open the first tab, login to your app.
2. Open the second tab on "password_reset" page. Enter the email of a user
from the first tab. Submit form.
3. Click the "password_reset_confirm" link from an email that should've
been received. Fill the form with your new password and submit it.
This will trigger this line
[https://github.com/django/django/blob/master/django/contrib/auth/views.py#L302]
against session without INTERNAL_RESET_SESSION_TOKEN which will lead to
KeyError
Way to fix the issue: use .pop() instead of del
{{{
self.request.session.pop(auth_views.INTERNAL_RESET_SESSION_TOKEN, None)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:3>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => needsinfo
Comment:
Thanks for extra details, however this scenario was reported and fixed in
#27840. I cannot reproduce `KeyError` with these steps. Can you provide a
sample project?
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:4>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Peter De Wachter):
We hit this bug as well, the mechanism is a bit convoluted though. Our
project installs a post_save receiver for the User table, for logging
purposes. This receiver accesses request.user as part of that logging (it
uses a middleware to get at the request), and that's the cause of the
failure.
What happens is this:
- The user uses a password reset link while logged in, as described by
Andrey Shakurov above.
- When PasswordResetConfirmView saves the user object with the new
password, our post_save receiver runs.
- The post_save receiver accesses request.user.
- There's nothing in the password reset flow that used request.user at an
earlier point, so there's no cached user object.
- So auth.get_user() gets called. get_user() will attempt validate the
session hash. But that will fail: even if the hash was valid before (not
necessarily the case), it will certainly be invalid after the password
change. So it flushes the session!
- Our post_save code finishes and the save completes.
- Then the view tries to delete the session field, which no longer exists,
because the session was flushed. So we get the KeyError.
I think the simplest solution is to explicitly log out the user when he
accesses a password reset link.
I've submitted a PR: https://github.com/django/django/pull/13360
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:5>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Peter De Wachter):
* status: closed => new
* resolution: needsinfo =>
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:6>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
Hi Peter.
Can I ask you to add an explicit example here?
> When PasswordResetConfirmView saves the user object with the new
password, our post_save receiver runs.
> The post_save receiver accesses request.user.
So I provide a receiver for `post_save` with the `User` model. This gets
called with `User` and the `instance` (and ...) but how are you getting
the request in there?
Let's work on the reproduce first but:
> I think the simplest solution is to explicitly log out the user when he
accesses a password reset link.
I'd need to think about it fully but, if the user is logged in would it
not make sense to ensure that the user matches that for the reset token?
(In so doing access `request.user` before processing the reset token.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:7>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mark Gregson):
* status: closed => new
* version: 2.1 => 3.1
* resolution: needsinfo =>
Comment:
Hi Carlton
With further digging, I found that my project had a similar pattern to
Peter's and the session was being flushed for the same reason. I have now
produced a simple example that reproduces the error on a fresh 2.2.16 or
3.1.2 Django project. The example reflects the use case in my project, ie,
resolving of `request.user` while logging the password change. The crux
is that `request.user` is resolved for the 1st time after the password
change and before the token is deleted from session.
{{{
#!div style="font-size: 80%"
{{{#!python
class CustomSetPasswordForm(auth_forms.SetPasswordForm):
def __init__(self, *args, request=None, **kwargs):
super().__init__(*args, **kwargs)
self.request = request
def save(self, commit=True):
user = super().save(commit)
if not self.request.user.is_anonymous: # resolves
self.request.user for the 1st time
logger.info(
"%s password changed by %s %s",
user,
self.request.user.email,
self.request.META.get("REMOTE_ADDR"),
)
return user
class PasswordResetConfirmView(auth_views.PasswordResetConfirmView):
form_class = CustomSetPasswordForm
def get_form_kwargs(self):
kwargs = super().get_form_kwargs()
kwargs["request"] = self.request
return kwargs
}}}
}}}
There are simple solutions for the above case but it's a subtle problem
that is hard to pin down so perhaps we should seek to avoid others falling
into the same trap. Perhaps the view could catch the `KeyError` and
reraise with a message that would guide dev's straight to the solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:8>
Django
------------------------------+--------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mark Gregson):
* cc: Mark Gregson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:9>
Django
------------------------------+------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
OK, thanks for the extra detail Mark. This reproduces, so I'll Accept for
now. Still not 100% sure what we should do here. I'll add a test case and
then upload the sample project later on so we can look at it more easily.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:10>
Django
------------------------------+------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* Attachment "trac30952.patch" added.
Patch with test case.
Django
------------------------------+------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
I uploaded a diff for the test suite for this.
Not sure what we should do about it. Seems that you have to jump through
hoops to opt-into it…
In particular with both the form and the signals cases, you have to ignore
the in-scope `user` in order to use `request.user` that you went to some
lengths to get hold of...
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:11>
Django
------------------------------+------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mark Gregson):
> Seems that you have to jump through hoops to opt-into it…
I agree. In the simple example it certainly looks pointless and contrived
but in my project there is a generic logging method that accepts a request
object and after a password reset information related to other objects
instantiated in the form is logged, which makes logging from the form a
reasonable option.
> Not sure what we should do about it.
Maybe having this ticket to explain the problem and solution is enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:12>
Django
------------------------------+------------------------------------
Reporter: defigor | Owner: nobody
Type: Bug | Status: closed
Component: contrib.auth | Version: 3.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
OK, given lack of follow-up, the discussed need to opt-in to this, and the
proposed ''Maybe having this ticket to explain the problem and solution is
enough'', let's close as `wontfix`.
We can always review a patch if one turns up...
Thanks all.
--
Ticket URL: <https://code.djangoproject.com/ticket/30952#comment:13>