Opened 5 years ago
Closed 2 years ago
#30952 closed Bug (wontfix)
KeyError: '_password_reset_token' during password reset.
Reported by: | defigor | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | 3.1 |
Severity: | Normal | Keywords: | |
Cc: | Andrey Shakurov, Mark Gregson | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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?
Attachments (1)
Change History (14)
comment:1 by , 5 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
Summary: | KeyError: '_password_reset_token' during password reset → KeyError: '_password_reset_token' during password reset. |
Type: | Uncategorized → Bug |
comment:2 by , 5 years ago
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.
comment:3 by , 5 years ago
Cc: | added |
---|---|
Resolution: | needsinfo |
Status: | closed → new |
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:
- Open the first tab, login to your app.
- Open the second tab on "password_reset" page. Enter the email of a user from the first tab. Submit form.
- 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)
comment:4 by , 4 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
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?
comment:5 by , 4 years ago
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
comment:6 by , 4 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
comment:7 by , 4 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
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.)
comment:8 by , 4 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Version: | 2.1 → 3.1 |
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.
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.
comment:9 by , 4 years ago
Cc: | added |
---|
comment:10 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:11 by , 4 years ago
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...
comment:12 by , 4 years ago
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.
comment:13 by , 2 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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.
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.