Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

email validation regexp update with tests #6026

Closed
wants to merge 4 commits into from
Closed

email validation regexp update with tests #6026

wants to merge 4 commits into from

Conversation

KevinBrogan
Copy link
Contributor

Added some improved testing and regexp matching to be a closer match to chromium and to rfc1035

Previously, domain parts which began with a dash or a number, or ended with a dash, would be accepted as valid.
@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@KevinBrogan
Copy link
Contributor Author

CLA is now signed.

On Tue, Jan 28, 2014 at 3:15 PM, Mary Poppins notifications@github.comwrote:

I'm sorry, but I wasn't able to verify your Contributor License Agreement
(CLA) signature. CLA signature is required for any code contributions to
AngularJS.

Please sign our CLAhttps://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#signing-the-claand ensure
that the CLA signature email address and the email address in this PR's
commits match
.

If you signed the CLA as a corporation, please let us know the company's
name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses
don't match. Please sign the CLA again or update the email address in the
commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA
verification process.

Reply to this email directly or view it on GitHubhttps://github.com//pull/6026#issuecomment-33538863
.

@ghost ghost assigned caitp Jan 29, 2014
@caitp
Copy link
Contributor

caitp commented Jan 31, 2014

The fix was merged into chrome, https://src.chromium.org/viewvc/blink?revision=165978&view=revision, probably won't get released until like v34 or something. But, it might not be the worst thing in the world to merge a similar fix into angular. I'll mark this for 1.3

@@ -9,7 +9,7 @@
*/

var URL_REGEXP = /^(ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?$/;
var EMAIL_REGEXP = /^[a-z0-9!#$%&'*+/=?^_`{|}~.-]+@[a-z0-9-]+(\.[a-z0-9-]+)*$/i;
var EMAIL_REGEXP = /^[a-z0-9!#$%&'*+/=?^_`{|}~.-]+@[a-z]([a-z0-9-]*[a-z0-9])?(\.[a-z]([a-z0-9-]*[a-z0-9])?)*$/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

[a-z]([a-z0-9-]*[a-z0-9])

The first character of a domain label can be a digit (moot@4chan.org, for example).

In Chrome, we're using a maximum of 63 digits for each domain label, because the RFC states that the domain can only be so long. It might be good to use that strategy here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would appear that the rfc I referenced was outdated.

From rfc 1123.

Limiting domain labels to 63 characters would also appear to not be correct.

2.1 Host Names and Numbers

  The syntax of a legal Internet host name was specified in

RFC-952 http://www.faqs.org/rfcs/rfc952.html
[DNS:4]. One aspect of host name syntax is hereby changed: the
restriction on the first character is relaxed to allow either a
letter or a digit. Host software MUST support this more liberal
syntax.

  Host software MUST handle host names of up to 63 characters and
  SHOULD handle host names of up to 255 characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

domain label and host name are different things. A domain label is the section of text between . periods (and after the @). An email address is technically not supposed to exceed 256 characters, but the domain label in Chromium is limited to 63 characters each, which is way more than anybody really uses.

For instance, moot@4chan.org, both 4chan and org are domain labels. 4chan.org is the full domain name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is a character limit necessary though? The consequences of not implementing
a character limit are the same as a typo or spelling mistake, are they not?

On Wed, Feb 19, 2014 at 1:53 PM, Caitlin Potter notifications@github.comwrote:

In src/ng/directive/input.js:

@@ -9,7 +9,7 @@
*/

var URL_REGEXP = /^(ftp|http|https)://(\w+:{0,1}\w_@)?(\S+)(:[0-9]+)?(/|/([\w#!:.?+=&%@!-/]))?$/;
-var EMAIL_REGEXP = /^[a-z0-9!#$%&'+/=?^{|}~.-]+@[a-z0-9-]+(.[a-z0-9-]+)_$/i; +var EMAIL_REGEXP = /^[a-z0-9!#$%&'_+/=?^_{|}~.-]+@a-z?(.a-z?)*$/i;

domain label and host name are different things. A domain label is the
section of text between . periods (and after the @). An email address is
technically not supposed to exceed 256 characters, but the domain label in
Chromium is limited to 63 characters each, which is way more than anybody
really uses.

Reply to this email directly or view it on GitHubhttps://github.com//pull/6026/files#r9885266
.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to use the same character limit rules, but I'm not super worried about these, I would like to hear the opinion of another developer for that.

changed the regular expression again as domain validation was modified by rfc1123 to allow numbers in the first character of a domain label.
modified test for domain labels beginning with numebrs to match rfc 1123
@caitp
Copy link
Contributor

caitp commented Jul 7, 2014

So, Chrome stable has the correct email validation now, I think we can merge this, it's a bit later than it should have been =)

@caitp caitp closed this in e24fb17 Jul 7, 2014
@caitp
Copy link
Contributor

caitp commented Jul 7, 2014

agh oh god didn't commit message it >_<

caitp pushed a commit that referenced this pull request Jul 7, 2014
Previously, domain parts which began with or ended with a dash, would be accepted as valid. This CL matches Angular's email validation with that of Chromium and Firefox.

Closes #6026
caitp pushed a commit that referenced this pull request Jul 7, 2014
Previously, domain parts which began with or ended with a dash, would be accepted as valid. This CL matches Angular's email validation with that of Chromium and Firefox.

Closes #6026
ckknight pushed a commit to ckknight/angular.js that referenced this pull request Jul 16, 2014
Previously, domain parts which began with or ended with a dash, would be accepted as valid. This CL matches Angular's email validation with that of Chromium and Firefox.

Closes angular#6026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants