Page MenuHomePhabricator

Sometimes receive more than one password reset email within the $wgPasswordReminderResendTime
Open, Needs TriagePublicBUG REPORT

Description

What is the problem?

When I submit the same password reset request multiple times in a short period, I can receive more than one password reset email.

I have reproduced this by submitting email and username, just email or just username, and with users who have PRU enabled and disabled.

So far, I have only seen two password reset emails being sent at a time. The second email has the working temporary password (I assume the first temporary password has been overwritten).

Steps to reproduce problem

Probably easiest to do this with some sort of script.

For example, for <username> who has not had a password reset in the last 24 hours (and from an IP that isn't throttled):

for x in {1..5}; do curl -s 'https://<wiki>/wiki/Special:PasswordReset' --data 'wpUsername=<username>&wpEmail=<email>&wpEditToken=%2B%5C' > /dev/null; done

(N.B. I have reproduced this with as little as 3 iterations, but no less)

Expected behavior: <username> receives one password reset email
Observed behavior: <username> (sometimes) receives two password reset emails

Environment

Wiki(s): Reproduced on:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 29 2020, 1:59 PM
dom_walden updated the task description. (Show Details)Apr 29 2020, 2:00 PM
dom_walden updated the task description. (Show Details)Apr 29 2020, 3:46 PM
Reedy added a subscriber: Reedy.Apr 29 2020, 4:21 PM

This feels race-condition-y

Like user_newpass_time isn't updated (though, I note in code it does a DB_MASTER read https://github.com/wikimedia/mediawiki/blob/fb4d145a337de257d93e9a234ed0a93f82e23947/includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php#L244-L249 ) before the second request fires

To help narrow it down, if you put in an artificial sleep between requests (5s?) does that prevent it happening?

This feels race-condition-y

Yeah, this seems likely. It doesn't happen all the time (I don't think I made that very clear in the description).

To help narrow it down, if you put in an artificial sleep between requests (5s?) does that prevent it happening?

I have not seen happen so far when I do.

ifried added a subscriber: ifried.EditedApr 29 2020, 5:33 PM

We've discussed this ticket as a team. This is not something that is easily reproducible in the UI. Furthermore, the issue of repeat emails does not impede the user's ability to reset the password, and it does not create security violations. Because this requires a non-trivial amount of technical work, and because it is not a high priority product fix, we will leave the current behavior. However, if another team would like to change this in the future, they can certainly do so.

Reedy added a comment.Apr 29 2020, 5:42 PM

I wonder if this basically the server is still dealing with it on the backend on one request, when the second comes in, it gets past the check (because the DB update hasn't happened?)...

Certainly, we could put a lock in place (keyed on the user)... But I'm not sure if that's overkill