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

Add gmail validation to isEmail #832

Merged
merged 5 commits into from
May 16, 2018
Merged

Add gmail validation to isEmail #832

merged 5 commits into from
May 16, 2018

Conversation

tux-tn
Copy link
Member

@tux-tn tux-tn commented May 15, 2018

This is a partial fix to #825 concerning Gmail

  • Gmail username has to be between 6 and 30 letters
  • The only allowed characters are alphanumeric and dots
  • Consecutive dots are not allowed
  • Both first and last characters have to be alphanumeric

I also edited isEmail tests since some addresses marked valid are not a valid Gmail address.

Not related but i also fixed a deprecated use of new Buffer DEP0005

lib/isEmail.js Outdated
@@ -34,6 +34,7 @@ var default_email_options = {
/* eslint-disable no-control-regex */
var displayName = /^[a-z\d!#\$%&'\*\+\-\/=\?\^_`{\|}~\.\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF]+[a-z\d!#\$%&'\*\+\-\/=\?\^_`{\|}~\,\.\u00A0-\uD7FF\uF900-\uFDCF\uFDF0-\uFFEF\s]*<(.+)>$/i;
var emailUserPart = /^[a-z\d!#\$%&'\*\+\-\/=\?\^_`{\|}~]+$/i;
var gmailUserPart = /^[a-z\d](\.?[a-z\d])+$/;
Copy link
Collaborator

@chriso chriso May 16, 2018

Choose a reason for hiding this comment

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

This is probably vulnerable to ReDoS. To sidestep this, we split the user part by . and then test each part individually. Could you set the pattern we use to /^[a-z\d]+$/i when it's a gmail domain?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, i did put length checking before and not inside the regex to try to avoid ReDoS but testing each part individually is a better idea since i have found that dots don't count in length checking

// Removing sub-address from username before gmail validation
const username = user.split('+')[0];

if (!isByteLength(username, { min: 6, max: 30 })) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a source for the [6, 30] length restriction? and the alphanumeric char restriction?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find a link in google/gmail support except for G-suite users but both length and characters restrictions can be found in Gmail sign-up page

Copy link
Collaborator

Choose a reason for hiding this comment

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

K, confirmed:

screen shot 2018-05-16 at 8 37 52 pm

screen shot 2018-05-16 at 8 37 42 pm

lib/isEmail.js Outdated
return false;
}
}
return true;
Copy link
Collaborator

@chriso chriso May 16, 2018

Choose a reason for hiding this comment

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

Remove this line, or else everything after + isn't validated:

> validator.isEmail('foobar+ probably not <valid> @gmail.com')
true

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad.., forgot about gmail sub addresses..

@chriso
Copy link
Collaborator

chriso commented May 16, 2018

Thanks 😄

@chriso chriso merged commit 447981f into validatorjs:master May 16, 2018
@tux-tn
Copy link
Member Author

tux-tn commented May 16, 2018

Thank you for your review 😄

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