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

Better email address attribute #27592

Closed
galczo5 opened this issue Oct 10, 2018 · 17 comments
Closed

Better email address attribute #27592

galczo5 opened this issue Oct 10, 2018 · 17 comments

Comments

@galczo5
Copy link

@galczo5 galczo5 commented Oct 10, 2018

Hi,
Today I found out that EmailAddressAttribute is quite bad.
First of all, now email string can contain only one @ char.
RFC 2821 specifies Abc@def@example.com as valid address (https://haacked.com/archive/2007/08/21/i-knew-how-to-validate-an-email-address-until-i.aspx/), so I think that we need better validation.

Second, mail address mail@.com is not valid, but EmailAddressAttribute recognizes it as valid.

I've prepared some tests (dotnet/corefx#32717).
Can we solve it with regular expressions? I can provide new implementation in next few days.

@rmkerr
Copy link
Contributor

@rmkerr rmkerr commented Oct 10, 2018

This is a pretty difficult problem to solve, and I don't think that a (very complicated) regex is the answer. In System.Uri we implement email address parsing that isn't perfect but that is generally "good enough", and I think we should take the same approach here.

Are you actually running into issues caused by these specific cases?

@chrisaut
Copy link
Contributor

@chrisaut chrisaut commented Oct 11, 2018

I ran into issues with this too, see my my comments here: https://github.com/dotnet/corefx/issues/32688#issuecomment-428780729

@xShivan
Copy link

@xShivan xShivan commented Oct 11, 2018

@rmkerr I don't agree that regex is not the answer. Regex was removed in 2015 (details in #27569) and until that moment it worked like a charm.

@galczo5
Copy link
Author

@galczo5 galczo5 commented Oct 11, 2018

@chrisaut I totally agree. EmailAddressAttribute is useless now.

@rmkerr Our tester found out this. For him, it is unacceptable that he can register a user with an invalid email address. And for me, it is not "good enough". Even in spring they use regex for it (https://github.com/Baeldung/spring-security-registration/blob/master/src/main/java/org/baeldung/validation/EmailValidator.java)

If it is that difficult problem maybe you should remove this attribute?

@2called-chaos
Copy link

@2called-chaos 2called-chaos commented Oct 11, 2018

@galczo5 You do realise that the expression used in your linked file is not correct aka. RFC compliant?

@galczo5
Copy link
Author

@galczo5 galczo5 commented Oct 11, 2018

@2called-chaos
Yup. But it's better than current solution. I found regex in referencesource repo too: https://github.com/Microsoft/referencesource/blob/master/System.ComponentModel.DataAnnotations/DataAnnotations/EmailAddressAttribute.cs

For me, if we want to keep it regex free, we should implement additional email address domain checking and it will be great. We should make very good documentation for it.

@tomaszek92
Copy link

@tomaszek92 tomaszek92 commented Oct 11, 2018

Hi,
I think that the current implementation is not enough and it should be improved. Invalid addresses as '.@.' or 't@.com' are valid for the current validator.

@conradreuter
Copy link

@conradreuter conradreuter commented Oct 11, 2018

IMHO the safest way to validate an email address is to actually send an email and verify it came through.

@xShivan
Copy link

@xShivan xShivan commented Oct 11, 2018

@conradreuter, agreed. But in most cases you want to verify it without sending an email, that's what the old RegEx was for. Why not use it again?

@conradreuter
Copy link

@conradreuter conradreuter commented Oct 11, 2018

That highly depends on what you specify the word "valid" to mean.

If you want to make sure that the address conforms to RFC2822, then sure, a complex RegEx might be the way to go.

But for all practical purposes I can think of, "valid" means that somebody is actually able to receive an email via this address. A quick sanity check à la contains @ and something before and after might be completely sufficient.

@tomaszek92
Copy link

@tomaszek92 tomaszek92 commented Oct 11, 2018

@conradreuter I agree that simple check is ok, but current validation is not enough. A user can misspell an email address: instead of "user@user.com", the user types "user@userLcom" and it is a problem.

@conradreuter
Copy link

@conradreuter conradreuter commented Oct 11, 2018

If I'm not mistaken, according to RFC2822 this is a valid email address.

(Just like root@localhost is a valid email address)

@Clockwork-Muse
Copy link
Contributor

@Clockwork-Muse Clockwork-Muse commented Oct 11, 2018

But for all practical purposes I can think of, "valid" means that somebody is actually able to receive an email via this address. A quick sanity check à la contains @ and something before and after might be completely sufficient.

Speaking on this point, I've had multiple sites reject my primary email address as "invalid". (Including one unsubscribe box, which is a quick way to get marked as spam)

@galczo5
Copy link
Author

@galczo5 galczo5 commented Oct 12, 2018

Maybe EmailAddressAttribute should implement more than one validation algorithm.
Default one that is the same as current and second more complex that will check for domain i email.
What do you think?

@MarcoRossignoli
Copy link
Member

@MarcoRossignoli MarcoRossignoli commented Oct 12, 2018

Maybe EmailAddressAttribute should implement more than one validation algorithm.

I think we can simply have a better implementation without RegEx (for security reason dotnet/corefx#4319). On codebase there is already similar code.

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Feb 22, 2019

This is not something we plan to implement. The check is intentionally naive because doing something infallible is very hard. The email really should be validated in some other way, such as through an email confirmation flow where an email is actually sent. The validation attribute is designed only to catch egregiously wrong values such as for a U.I.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
bartoszlenar added a commit to bartoszlenar/Validot that referenced this issue Aug 3, 2020
This commit introduces modes in Email rule. Previous logic is triggered in "ComplexRegex" mode, which is still set as the default option.
"DataAnnotationsCompatible" mode validates email in the very simple way, exactly the same as in System.ComponentModel.DataAnnotations.EmailAddressAttribute
The reason behind such simple logic: dotnet/runtime#27592 (comment)
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests