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

IsEmail validates non RFC compliant #908

Closed
DerKnerd opened this issue Oct 11, 2018 · 14 comments
Closed

IsEmail validates non RFC compliant #908

DerKnerd opened this issue Oct 11, 2018 · 14 comments

Comments

@DerKnerd
Copy link

We are using sequelize which uses this library for email validation. Currently we experiencing the issue, that the valid email addresses we provide are taken as invalid. A few examples:

And probably more. Can you please fix that, it causes major issues in business use cases, cause local domain addresses with no tld, a number in the tld or just one character in the tld are very common.

@DerKnerd DerKnerd changed the title IsEmail validates against the RFC IsEmail validates non RFC compliant7 Oct 11, 2018
@DerKnerd DerKnerd changed the title IsEmail validates non RFC compliant7 IsEmail validates non RFC compliant Oct 11, 2018
@justinkalland
Copy link

You can use the option require_tld and each of your cases passes.

Here is an example:

'use strict';

const validator = require('validator');

const validate = [
  'admin@admin',
  'admin@admin.x',
  'admin@admin.de2'
];

validate.forEach(email => {
  const isEmail = validator.isEmail(email, {
    require_tld: false
  });

  console.log(email + ': ' + isEmail); // Each email passes
});

Output:

admin@admin: true
admin@admin.x: true
admin@admin.de2: true

@DerKnerd
Copy link
Author

Ok, it actually doesn't solve the problem, but that lies in sequelize. Apart from that, I think it would be better that the default case is RFC compliant and not non compliant.

@eudson
Copy link

eudson commented Oct 12, 2018

You can use the option require_tld and each of your cases passes.

Here is an example:

'use strict';

const validator = require('validator');

const validate = [
  'admin@admin',
  'admin@admin.x',
  'admin@admin.de2'
];

validate.forEach(email => {
  const isEmail = validator.isEmail(email, {
    require_tld: false
  });

  console.log(email + ': ' + isEmail); // Each email passes
});

Output:

admin@admin: true
admin@admin.x: true
admin@admin.de2: true

This solution only works for the e-mails provided by @DerKnerd but e-mails from google with username part that has a length less than six don't work. And the bug is here

if (!isByteLength(username.replace('.', ''), { min: 6, max: 30 })) { return false; }
https://github.com/chriso/validator.js/blob/master/src/lib/isEmail.js#L65

would be good if you could fix this or I can submit a Pull Request

@justinkalland
Copy link

justinkalland commented Oct 12, 2018

Can a gmail address be shorter than 6 characters? Do you have an example? I just tried:
screen shot 2018-10-12 at 11 29 16 am

(note that the screenshot is wrong, I originally tried 5 characters, and then I changed it to 6 and took the screenshot before I submitted again)

@DerKnerd
Copy link
Author

But is still a valid email address, the name of the function is misleading, cause it does vendor specific checks.

@justinkalland
Copy link

It doesn't do vendor specific checks unless you specify the option domain_specific_validation when calling isEmail()

I don't feel the function is misleading at all, it is well documented and works great.

@DerKnerd
Copy link
Author

Ok, I didn't see that part in the code. But still it doesn't work great, it is wrong in the default case. The RFCs very clearly defines how a valid email address should look like. Here is an article explaining it very thourgly https://haacked.com/archive/2007/08/21/i-knew-how-to-validate-an-email-address-until-i.aspx/

Here are all relevnt RFCs:

So no,it doesn't work great. It works against the standards at least in the default case and this case should never represent something different than the standards.

@eudson
Copy link

eudson commented Oct 12, 2018

@justinkalland you right google username minimum length is 6 but as @DerKnerd said you are doing vendor specific specif checks (https://github.com/chriso/validator.js/blob/master/src/lib/isEmail.js#L45) I tested without setting the option domain_specific_validation and it validates google rules. Also the only domain is google, so it should be something like apply_google_validations.
Anyways, this is a great library and we are just helping improving it.

@chriso
Copy link
Collaborator

chriso commented Oct 21, 2018

The latest version of the library does not do domain-specific validation by default (it did at one point, but that was removed; see #873).

As for requiring a TLD, the validator tries to balance strict RFC validation with the fact that users expect emails to be a certain way in almost all cases. We have the same issue with isURL(). Most users would expect google.com to be a valid URL despite the missing protocol and path. We require a TLD here too, because if we were to drop the TLD requirement too then x would be considered a valid URL. Similarly, without the TLD requirement we would consider a@b to be a valid email address. This is tough to get right – there are hostnames without a TLD that are commonly used (localhost, for example).

The TLD validator, enabled by default, requires at least two characters in the TLD, and does not allow numbers. Your example email hostnames admin.x and admin.de2 are not valid (they also don't exist/resolve). If you consider these to be valid email addresses, I'd like to hear why. We could potentially relax those requirements, or have an option to disable them.

@profnandaa
Copy link
Member

Sidenote:
As much as GMail does not accept usernames less than 6 chars anymore, I believe we have valid GMail emails out there that are less than 6 chars. I tried sending an email to rob@gmail.com and didn't get a fail like what I got with this one
screen shot 2018-10-25 at 11 38 16 am

We might need to revisit this; that's minor though.

@profnandaa
Copy link
Member

profnandaa commented Oct 25, 2018

@DerKnerd - I think the reason why the lib went for this default behavior is because RFC is broader than the general use-case.

I think it would be better that the default case is RFC compliant and not non compliant.

UPDATE: will need to fact check, based on this

@DerKnerd
Copy link
Author

@chriso for me this answer is fairly simple. The default case is to comply with the RFC because the internet is build on standards. It would probably be good idea to allow developers to also ask for a tld, but that shouldn't be the default case. I think it is not hard at all, just implement the RFC as the default case, and allow more restrictions by the developer.

My "home" is business development, and the Exchange Server by Microsoft allows nearly everything RFC compliant as an email address so the use cases are there out in the business world.

@profnandaa I know that even the w3c said the requirements by the RFC are to strict and browser implement a way simpler RegEx. But that doesn't change the fact, that it is more allowing than the validation of validator.js. This behavior causes problems in node.js environments, cause the browser says it is valid, but the server says it is not even though it is.

@chriso
Copy link
Collaborator

chriso commented Oct 25, 2018

Thanks for the perspective, but I think we'll stick with the current behavior. Strict RFC validation is a non-goal of the library, and relaxing the current defaults would cause more issues than it would solve.

@hiteshjoshi1
Copy link

My gripe with this is - foo@bar.com is valid but foo@gmail.com is not.
IMHO - This is vendor-specific behavior which should not be get invoked OOTB. It should be a configuration option.
And if it does, it should give a more specific message: Gmail usernames cannot be less than 6 digits.

@DerKnerd DerKnerd closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants