heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24.03k stars 5.55k forks source link

Devise.email_regexp is not correct #3820

Open emn178 opened 9 years ago

emn178 commented 9 years ago

Hi,

It validates some incorrect emails as valid, like emn178@gmail..com

Change Devise.email_regexp from

/\A[^@\s]+@([^@\s]+\.)+[^@\W]+\z/

to

/\A[\w+\-.]+@[a-z\d\-]+(\.[a-z]+)*\.[a-z]+\z/i

Refer to http://stackoverflow.com/questions/22993545/ruby-email-validation-with-regex

It can validate correctly.

josevalim commented 9 years ago

Yes, you can search previous issues on the topic but the regex is not meant to be a complete validation of the e-mail because it is much more complex than the regex you posted. For example, the @ could be ip4 or ip6 addresses. So we decided to do a minimal validation. You can change it if you want.

samjewell commented 3 years ago

Ruby already has an email validation regex in URI::MailTo::EMAIL_REGEXP, and this is much stricter than the default in Devise, and also stricter than the various suggestions I've seen so far posted (on this thread, here and on #4268 and #5023)

URI::MailTo::EMAIL_REGEXP
=> /\A[a-zA-Z0-9.!\#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\z/

From the Ruby source, this Regex is taken from the WHATWG's suggested regular expression for browsers to validate an email address as part of form validation.

So we patched this issue by adding:

  # Configure the email validator to be stricter
  config.email_regexp = URI::MailTo::EMAIL_REGEXP

to our devise.rb initializer.

@josevalim / @nashby / @ghost To me this feels like a rough edge on Devise, and one which every project/developer has to encounter and fix when their project grows large enough. Would you consider smoothing this rough edge, either by:

Let me know if you'll take either of these suggestions seriously - I can make a PR if that helps.

carlosantoniodasilva commented 3 years ago

Thanks for bringing this up again @samjewell. Honestly I have changed this regex myself in some apps in the past because Devise's version became too simple to handle some common user typos.

I am hoping to work on a new major release at some point early this year, so that might be a good time to consider such a change in the Devise default. It'd also be possible to change this in the initialiser without causing any issue with existing apps; in other words, keeping the Devise default as is but changing how new apps are generated to the more strict version.

I'll reopen this for now to revisit when I have a chance to look into it in more detail / with more time.

xtagon commented 3 years ago

Before switching to URI::MailTo::EMAIL_REGEXP we saw quite a large number of invalid emails in production which resulted in unsuccesssful email delivery attempts. There's no way around it, users put in the wrong data sometimes. Sometimes a lot. We're talking about accidentally putting two dots instead of one in the domain name, accidentally typing = instead of ".", backticks, and somehow we had some with no-break-space (ASCII 160) characters which I think could be explained by users simply copying/pasting addresses.

URI::MailTo::EMAIL_REGEXP caught all of those cases.

This can all be validated on the client side as well, but when there's an API involved that's not enough, and IMO there is almost no benefit to keeping an incorrect e-mail validation just for simplicity's sake when it's no simpler than just using a valid regex. Yes it would warrant a major version bump, but is that such a bad thing? Semver exists so that breaking changes are explicit and known, not so that we can avoid making changes that would be breaking altogether.

Just my two cents (and huge thanks to everyone who provided solutions, it's appreciated!)

nfedyashev commented 2 years ago

I fully agree with @samjewell point. IMHO, stricter default email_regexp might make more sense for an average user of Devise.

In my case, "mailto:" prefix in user email was treated as a valid email and that was a bit surprising.

carlosantoniodasilva commented 1 year ago

I was playing with URI::MailTo::EMAIL_REGEXP for this and other reasons, and realized this:

>> URI::MailTo::EMAIL_REGEXP.match? 'invalid@example'
=> true

Are you guys handling this somehow as well? I don't think we can use EMAIL_REGEXP as is with that, for web app emails at least that is not a valid use case without a full domain.

samjewell commented 1 year ago

Are you guys handling this somehow as well?

I've moved on from that job now, but IIRC we don't. Personally I'd be tempted not to bother. Maybe open an issue on the Ruby core, if that's what's not working properly.

carlosantoniodasilva commented 1 year ago

Thanks @samjewell. Someone pointed out on Twitter that it's an actual valid use case for "email" as in user@localhost for example when using a local mail server. (I don't know about the RFC itself, and domain requirements, but from web app's perspective and devise, it's definitely not a valid one.)

wTheRockb commented 1 year ago

Are you guys handling this somehow as well?

I've changed my application to using /[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]+/ (for forms when using pattern attribute) and /\A[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]+/i otherwise. We felt the benefit of disallowing asdf@example was stronger than allowing user@localhost.

skreib commented 1 month ago

hey, everyone! are there any updates regarding this issue?