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

Register router treat email as case-sensitive #245

Closed
MariusMez opened this issue Jul 6, 2020 · 8 comments
Closed

Register router treat email as case-sensitive #245

MariusMez opened this issue Jul 6, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@MariusMez
Copy link

MariusMez commented Jul 6, 2020

Hi,
I've just noticed that the default provided router for /register POST endpoint treat email-addresses as case-sensitive.
As a result two "identicals" email-addresses with different cases writing are treated as different users.

eg: during registration John.Doe@company.com is verified as a different user than john.doe@company.com

It may be misleading :-)

@frankie567
Copy link
Member

Hi @MariusMez!

Yes, I noticed that some days ago. Actually, this is compliant with RFC 5321:

The local-part of a mailbox MUST BE treated as case sensitive.

Pydantic respects that in their implementation. Django also does it like this.

I've read some StackOverflow posts out there with people saying that they have seen email servers with two different users having an email address differing only by the case.

So, I would say that we'll keep the standard way of thinking, even if it may sound a bit counter-intuitive in the real life. Now, I think you could modify your user model so that you treat e-mail as case-insensitive. Something like:

from fastapi_users import models
from pydantic import validator

class UserCreate(models.BaseUserCreate):
    @validator('email')
    def lowercase_email(cls, v):
        return v.lower()

I've not tested it, but you get the idea 🙂

@frankie567 frankie567 added the question Further information is requested label Jul 8, 2020
@MariusMez
Copy link
Author

Yes I agree with you @frankie567 , just wanted to notice it :-)

Because I've some users who type their email in login form starting with an uppercase (but they registered it in lowercase...) and as a result the verification fail. But what make me notice it, it's that I have some users who registered twice using the same email but with cases variations...

In fact I think for most cases we should saved the provided user email as case sensitive to ensure email acceptance by email server.

But for most app at registration when we check for previous registered users (when email is treated as the unique field) we must treat it as case-insensitive for searching in database (and the same logic apply at login to retrieve the user).

In any-case (jeu de mot inside) thank for your answer!

@frankie567
Copy link
Member

frankie567 commented Jul 8, 2020

Yes, I get the nuance there: keep the case in the DB but ignore it while looking for it. I definitely agree.

This is something that should be done!

@frankie567 frankie567 added enhancement New feature or request and removed question Further information is requested labels Jul 8, 2020
@euri10
Copy link

euri10 commented Jul 8, 2020

I can be totally off on this but it seems to me it's a bad idea, or put it differently this immediately rings a bell "Unicode hack".

Let's say francois@voron.com is in the db already, someone could register a different email with a Turkish dotless i and login against the lowercased email, both will be the same.

@frankie567
Copy link
Member

I'm not very versed into Unicode hacks, but from what I understand, lowercasing a Turkish dotless i won't turn in into a regular i ? Isn't it?

@euri10
Copy link

euri10 commented Jul 8, 2020 via email

@frankie567
Copy link
Member

frankie567 commented Jul 9, 2020

I guess you are refering to this article : https://eng.getwisdom.io/hacking-github-with-unicode-dotless-i/

The way I see it, the vulnerability only occurs when we check only by email ; which is the case with forgot password feature. However, as they say in the article, the fix is quite simple : ensure you send the email to the address in the database, not the one in user input.

For login, you still have to match the password. If an attacker has the password, it doesn't really matter if they Unicode-hack the email (again, I'm not a security expert ; so I may be really mistaken there).

I've tested on very big web services (Google, Stack Overflow and GitHub), and they all treat emails as case-insensitive. I believe we can trust their security teams for that choice.

EDIT: However, as @MariusMez said, it's indeed important to keep the case the user has input in the database.

@frankie567
Copy link
Member

Released in v3.0.0 🎉 https://frankie567.github.io/fastapi-users/migration/2x_to_3x/

JeffreyThijs added a commit to JeffreyThijs/fastapi-users that referenced this issue Nov 29, 2020
…ving user in DB (fastapi-users#250)

* Add unit tests to enforce email case insensitivity

* Handle email as case insentitive while retrieving user in DB

* Apply isort/black

* Add migration doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants