@baris Done!fais3000 created this issue in NodeBB/NodeBB closed Users able to signup with invalid email address (containing comma) #5902
Registration Email Case Sensitivity
Hi all - new to the community; love this product!
I'm using NodeBB as a private community and have been using the newuser-approval plugin to restrict registrants to people I invite. I sent that project a PR because it wasn't treating email addresses as case insensitive, and I was creating duplicate invitations accidentally due to capitalizations. While it is part of the RFC to treat the local part of the address as case sensitive, most large email providers do not enforce this.
So after fixing that, I tested a registration email to myself with a lowercase email address and got my email. When I went to register, I used an uppercase letter to start the email address because sometimes on iPhone and mobile devices this happens by autocorrect. When I do this, the email is rejected because the case sensitivity is enforced.
I'd like to get peoples opinion on this matter. Should emails be treated as case insensitive? If this seems like something people agree with I'd be more than happy to send a pull request to NodeBB's repository on github.
Definitely should be case insensitive in my opinion. Might need some "to lower case" in there too for insertion or some regex for seach (former is better for performance) because MongoDB is case sensitive (don't know about Redis).
Thanks for the vote of support @Fastidious ! After reviewing my initial post, I just realized I referenced the wrong plugin (I have too many tabs open!)
newuser-invitation is the plugin I sent a pull request too and it has already been merged, should be getting deployed to npm soon.
My post is in regard to NodeBB in general now, and if I should send a PR to the core project. It may be easier to explain my issue step by step.
- Invite user to my forum with their email address ([email protected]). The newuser-invitation plugin does it's thing, and now lowercases all emails added to it, so the email is now stored as [email protected]
- User gets email pointing to myforum.com/register on their mobile device and decides to register.
- When filling out the registration form, their mobile browser auto capitalizes the email address, and it is now [email protected]
- The new users hits submit and they are rejected from registering because the email address [email protected] is not in the invitation queue, but [email protected] is.
That is the issue I'm now looking to solve. My proposed solution is to lowercase all emails on the registration form prior to registration. I'm not sure where else this change will have effects if any; looking to the community to chime in on that part before I go spend time on this change if it is really a bad idea in the grand scheme of the codebase.
@drew thanks for the input.
Another concern I have here is surrounding email addresses that are already stored and validated with case sensitivity out in the wild (existing deploys).
What would happen if we auto lowercase email addresses? We'd have to do it from the user input side and out of the database before we did any comparisons / validations. I was reading the forum a bit and saw that some identities are merged (when you log in from google, facebook, etc) which use your email address as the linkage between accounts. I want to make sure we don't hurt those code paths with a change like this. What are your thoughts?
@jlank The nice thing about being a community rep is I can help until it gets tough and then say:
Sounds like a question for the admins
@jlank Thanks for the pr.
It looks like NodeBB already does toLowerCase at every occasion, so the problem was likely only on my end.
@yariplus thanks for sharing the code search; it does look like email addresses are being lower-cased at a lot of spots. I have tested the registration lower-case though and it seems like it may be lacking there. I'll see if I can pin point it in the code and get it to work as expected.
@yariplus my mistake; the lowercase did have to happen in your plugin, not the core project. Just made the changes and will be sending you a PR shortly...