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)

trac30952.patch (3.2 KB ) - added by Carlton Gibson 4 years ago.
Patch with test case.

Download all attachments as: .zip

Change History (14)

comment:1 by Mariusz Felisiak, 5 years ago

Resolution: needsinfo
Status: newclosed
Summary: KeyError: '_password_reset_token' during password resetKeyError: '_password_reset_token' during password reset.
Type: UncategorizedBug

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.

comment:2 by Mark Gregson, 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 Andrey Shakurov, 5 years ago

Cc: Andrey Shakurov added
Resolution: needsinfo
Status: closednew

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(INTERNAL_RESET_SESSION_TOKEN, None)
Last edited 5 years ago by Andrey Shakurov (previous) (diff)

comment:4 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: newclosed

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 Peter De Wachter, 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 Peter De Wachter, 4 years ago

Resolution: needsinfo
Status: closednew

comment:7 by Carlton Gibson, 4 years ago

Resolution: needsinfo
Status: newclosed

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 Mark Gregson, 4 years ago

Resolution: needsinfo
Status: closednew
Version: 2.13.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 Mark Gregson, 4 years ago

Cc: Mark Gregson added

comment:10 by Carlton Gibson, 4 years ago

Triage Stage: UnreviewedAccepted

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.

by Carlton Gibson, 4 years ago

Attachment: trac30952.patch added

Patch with test case.

comment:11 by Carlton Gibson, 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 Mark Gregson, 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 Carlton Gibson, 2 years ago

Resolution: wontfix
Status: newclosed

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.

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