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

Require addresses to pass the built-in EMAIL_REGEXP #33

Closed
wants to merge 1 commit into from

Conversation

prognostikos
Copy link
Contributor

We had an address pass the validation of this gem but was subsequently
treated as invalid when using the mail gem in a very odd way - the
Mail::Message#to method returned a String instead of a
Mail::AddressContainer. This commit uses the built-in
URI::MailTo::EMAIL_REGEXP as part of the default validation.

However, adding this breaks another test - allowing non-ascii characters
in the local part of the address. For us this is OK as we use Amazon SES
at the moment which also does not allow non-ascii characters, but I'm
guessing this may be a problem for others given that the test exists in
the first place.

Perhaps it should be a configuration option? Or feel free to tell us that
we'll need to do that as a separate validation in our app.

@prognostikos prognostikos force-pushed the use-uri-regex branch 2 times, most recently from b144803 to 25c9f96 Compare March 27, 2024 09:16
Copy link
Member

@koppen koppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch. Just a minor idea for a possible improvement.

Comment on lines 36 to +37
return false if /(\s|["'<>,])+/.match?(address)
return false unless URI::MailTo::EMAIL_REGEXP.match?(address)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could remove the regex-check at line 36 without any regressions. I'd expect URI::MailtTo::EMAIL_REGEXP to handle those cases as well.

@koppen
Copy link
Member

koppen commented Jun 4, 2024

I've taken a closer look at this now...

We're definitely seeing email addresses with non-ASCII characters in our datasets. Are they the correct, usable email addresses or typos, who knows 🤷‍♂️ That said, everything I can find seems to indicate that bøb@example.com or josé@example.com are valid email addresses.

The guiding principle for this gem has always been

Don't reject a valid email address realistically in use by a potential user. Err on the side of accepting too much.

and given the above, bøb@example.com could be a valid email address realistically in use by a potential user so we should not reject it. Thus, if adhering to URI::MailTo::EMAIL_REGEXP means we'd reject bøb@example.com, we can't adhere to URI::MailTo::EMAIL_REGEXP.

URI::MailTo::EMAIL_REGEXP was last updated in 2014 (https://bugs.ruby-lang.org/projects/ruby-master/repository/git/revisions/e63ab5d3ad289767eab49787e4e33390b0ce74e1/diff) to "use HTML5 email regexp", seemingly based on RFC 6068.

RFC 6068 "defines the format of Uniform Resource Identifiers (URIs) to identify resources that are reached using Internet mail". It doesn't not specify a valid format for email addresses, merely how they should be represented in a mailto: URI. It even supports non-ASCII characters, although they need to be URL-escaped:

(URI.encode_uri_component("bøb") + "@example.com").match?(URI::MailTo::EMAIL_REGEXP)
 => true

In other words, verifying email adresse formats is out of scope for RFC 6068, and from that follows that URI::MailTo::EMAIL_REGEXP cannot be relied on as a generic validation of email addresses.

If you need stricter validations - ie to comply with your email service provider - you have to add a separate validation in the application, I'm afraid. But be aware that dear "Bøb" is going to be sad when he tries to sign up 😁

@koppen koppen closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants