Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate email casing #1763

Open
kerrizor opened this issue Aug 10, 2018 · 16 comments · May be fixed by #4200
Open

Investigate email casing #1763

kerrizor opened this issue Aug 10, 2018 · 16 comments · May be fixed by #4200

Comments

@kerrizor
Copy link
Contributor

kerrizor commented Aug 10, 2018

We had a user contact us via support to say that they weren't receiving an email confirmation. Investigating, it looks as if the system is storing their email address downcased. Fine, great.. except that the username portion of an email address (the bit before the @) technically is case sensitive.

This is likely fine, as we seldom see them, but I wonder if we should at least add a UI warning, since users /may/ not receive the email that they expect?

@sonalkr132
Copy link
Member

except that the username portion of an email address (the bit before the @) technically is case sensitive.

Yes, username part should be kept as is.

@dwradcliffe
Copy link
Member

dwradcliffe commented Aug 11, 2018 via email

@kerrizor
Copy link
Contributor Author

@dwradcliffe it doesn't appear to be set up that way, based on what I'm seeing.

irb(main):004:0> User.find_by_email("Kerrizor@kerrizor.com")
=> nil
irb(main):005:0> User.find_by_email("kerrizor@kerrizor.com")
=> #<User id: 51537, email: "kerrizor@kerrizor.com...

This /might/ be simply an edge case, as the user in question was asking for help after not receiving their confirmation email, a common enough complain that is a larger, ongoing issue. I'm dubious this is a pressing concern, but wanted to at least get it down in an issue.

@dwradcliffe
Copy link
Member

👍 If we need to make an adjustment that's fine but let's make sure we're careful to not expose a security problem by allowing case sensitive emails.

@obahareth
Copy link

I've been previously recommended to store emails as lowercase for authentication purposes, but to store them exactly as the user typed them and use that version for sending any emails.

@anuragaryan
Copy link

so which way have we decided to go in with this issue? Maintaining different email to send mails or just to add a UI warning when the user puts email in uppercase? @kerrizor @dwradcliffe
I can pick this up if we know what we want.

@kerrizor
Copy link
Contributor Author

kerrizor commented Oct 9, 2018

@anuragaryan Here's a plan.. @dwradcliffe thoughts?

  • remove case-normalization from email in the User model (IE - accept whatever the user types in)
  • add a msg to the UI about emails being case-sensitive (either static or triggered to appear when the user enters a mixed-case email) to
    • sign-up
    • login
    • password/email recovery
  • ???

@anuragaryan
Copy link

@dwradcliffe if you've nothing else to add, I'm starting on this based on @kerrizor 's plan.

@obahareth
Copy link

Based on some previous experiences, having using lowercase emails is very helpful for the majority of users; as that's what they've come to expect due to how all the major providers deal with emails. I wouldn't suggest going with removing case normalization because you'll have cases like people opening the autocorrect capitalizing the first letter, then opening it later without autocorrect and not being able to login and being very confused. I would still recommend the two-fold solution:

  1. Store email as the user typed it, send emails to this version.
  2. Store lowercase email and use it for all authentication purposes.

@simi
Copy link
Member

simi commented Oct 24, 2018

@obahareth at DB side it is not needed to be stored twice, it is enough to create "lowered" index:

CREATE UNIQUE INDEX lower_case_email ON users ((lower(email)));
SELECT username FROM users WHERE lower(email) = lower('Email@Email.com');

@anuragaryan
Copy link

@simi wouldn't email@domain.com and Email@domain.com will be considered same then?

@simi
Copy link
Member

simi commented Nov 16, 2018

@anuragaryan depends on query then (if using lower(email) or email).

@anuragaryan
Copy link

@simi That's correct, what I meant was if some mail host allow case sensitive emails, then email@domain.com and Email@domain.com can be two valid email, belonging to two different people. If we lowercase index them both, how can we differentiate between the two?

@dwradcliffe
Copy link
Member

We should store exactly what is entered at signup, and send emails using that exact casing. However we should enforce uniqueness on lower(email). This might prevent someone from signing up (they would be using a rare email provider) but it will ensure that we don't open a huge security hole for most email providers.

anuragaryan added a commit to anuragaryan/rubygems.org that referenced this issue Nov 28, 2018
* Stores the email using exact casing as input
* Index on lower(email)
* Enforce uniqueness on lower(email)
* Use Case insensitive email to login
@anuragaryan
Copy link

@dwradcliffe @simi can we get the PR reviewed?

simi pushed a commit that referenced this issue Nov 11, 2023
* Fixes #1763
* Index on lower(email)
* Enforce uniqueness on lower(email)
* Use Case insensitive email to login
@simi simi linked a pull request Nov 11, 2023 that will close this issue
@simi
Copy link
Member

simi commented Nov 11, 2023

@anuragaryan I have revived your commit and opened PR at #4200. Sorry for the massive delay in here. Feel free to review.

simi pushed a commit that referenced this issue Nov 11, 2023
* Fixes #1763
* Index on lower(email)
* Enforce uniqueness on lower(email)
* Use Case insensitive email to login
simi pushed a commit that referenced this issue Jan 9, 2024
* Fixes #1763
* Index on lower(email)
* Enforce uniqueness on lower(email)
* Use Case insensitive email to login
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants