Yes, you're absolutely right. The real bug was sending to the "wrong" matching email. But this is what makes this bug so hard to find. You're looking at "equal" strings, so why should it make a difference if you pick A or A if A === A ? :)
username = request.post_params('username')
evil_email = request.post_params('email_address')
user = get_user_by_name(username)
good_email = user.email_address
if good_email != evil_email:
# Hackers!
else:
reset_password(user.id, evil_email) # it's fine; it's the same as user.email_address
Then you start to wonder if that line shouldn't look more like
reset_password(user)
If it makes no difference which email address you pick, why not pick the one that represents doing something safe instead of the one that represents doing something dangerous?
``` if evil_email not in good_email_addresses: # Hackers! else: # just reset with provided email. If I thought there was a potential security issue I would have already addressed it. ```
So easy to be lazy at this point, especially under time pressure.
Tripple backticks do nothing, and will be shown on the same line as the next line even if put on a separate line, because they are considered as regular text and part of one paragraph of text.
If the code were instead written:
username = request.post_params('username')
email = request.post_params('email_address')
user = get_user_by_name(username)
if emails_are_equal(email, user.email_address)
reset_password(user.id, email)
My whole point is that changing reset_password(user.id, email) to reset_password(user.id, user.email_address) neutralizes that risk. Then it doesn't matter whether emails_are_equal is risky or not, because a failure in emails_are_equal can only cause you to do a safe thing. What's less embarrassing -- "we accidentally sent a password reset for your account to you", or "we accidentally sent a password reset for your account to someone else"?
So you might not feel compelled to rewrite reset_password(user.id, email) as reset_password(user.id, user.email_address) -- but you should.
At least the first two (removing dots and suffixes), along with conversion to lowercase, are strictly invalid transformations which may result in an email address referring to a different account. Sure, most email providers treat the account name as case-insensitive, and the use of '+' as a label/subaccount separator is a common convention, but neither of these is required. The RFCs say that the account name is case-sensitive (unlike the domain name) and '+' is just an ordinary part of the name; any special significance is assigned by the server. Ignoring '.' characters is something specific to Gmail.
In the context of a search it's not unreasonable to ignore some of these differences, but at that point the matching names aren't "equal", just "similar". Certainly it should never be assumed that it's safe to send email to the transformed version of the address.