Contact Form: Revised the email validation regex to include length limit

[stevenlinx]

Fixes #3734

Changes proposed in this Pull Request:

• Revised the email validation regex to include length limit, in order to match the latest practical implementation of RFC 5322 in regular-expressions.info/email.html.

• Regex used:

PHP version: the very bottom one on regular-expressions.info/email.html. The one after the sentence "...modern regex flavors aren't truly regular, so we can add length limit checks using lookahead like we did before:" Take note, the redundant pink-colored white spaces in the middle of the regex must be removed.

\A(?=[a-z0-9@.!#$%&'*+/=?^_`{|}~-]{6,254}\z)(?=[a-z0-9.!#$%&'*+/=?^_`{|}~-]{1,64}@)[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+(?=[a-z0-9-]{1,63}\z)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\z

JavaScript version:

^(?=[a-z0-9@.!#$%&'*+/=?^_\`{|}~-]{6,254}$)(?=[a-z0-9.!#$%&'*+/=?^_\`{|}~-]{1,64}@)[a-z0-9!#$%&'*+/=?^_\`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_\`{|}~-]+)*@(?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+(?=[a-z0-9-]{1,63}$)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$

Testing instructions:

1.) Checkout the code in this PR on a test WP site.
2.) Use the Contact Form and enter some test data in each field.
3.) In the email field, test different email addresses that are expected to pass and fail.
4.) Use an email you've access to, submit the form and make sure you get the email.
5.) Revert the code back to its previous state.

Proposed changelog entry for your changes:

• Contact Form: Revised the email validation regex to include length limit.

[oskosk]

LGTM! Thank you!

[dereksmart]

This is not validating properly.

It's even checking for literal spaces right now: https://regex101.com/r/MAOFqc/4

var newRe = /(?=[a-z0-9@.!#$%&'*+/=?^_`{|}~-]{6,254}\z)  (?=[a-z0-9.!#$%&'*+/=?^_`{|}~-]{1,64}@)  [a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)* @ (?:(?=[a-z0-9-]{1,63}\.)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+   (?=[a-z0-9-]{1,63}\z)[a-z0-9](?:[a-z0-9-]*[a-z0-9])?/i;

var oldRe = /[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?/i;

newRe.test( '[email protected]' );
false
oldRe.test( '[email protected]' );
true

When I tested, I made a mistake and tested with the minified version (which doesn't include the changes proposed here)

[stevenlinx]

I think I got what was causing the issues. The regex was correct but two things must be done to apply it to javascript, due to regex flavors:

1.) The \A and \z must be either be replaced (with caret ^ and dollar $) or removed (if the subject string is the email string) since javascript does not support \A and \z, but only ^ and $.

For the previous regex on the website, there were only \A and \z at the beginning and end.

Unlike the previous regex, the latest regex version on the website (the very bottom one) not only has \A and \z in the beginning and end, but also \z in the middle (due to lookahead). In my previous commit, I only removed \A and \z in the beginning and end, but not all the \z the middle.

2.) The pink-colored white spaces in the middle of regex must also be removed, which was part of web page layout. In the previous regex, there were no white space that need to be removed.

I've push a new commit with the revised regex.

=====

Tests:

[email protected]
https://regex101.com/r/sGpumM/1

local part (64 char.):
https://regex101.com/r/AZTNg9/1
expected: pass
result: pass

local part (65 char.):
https://regex101.com/r/AZTNg9/2
expected: fail
result: fail

part of domain (63 char.):
https://regex101.com/r/AZTNg9/3
expected: pass
result: pass

part of domain (64 char.):
https://regex101.com/r/AZTNg9/4
expected: fail
result: fail

total length (254 char.):
https://regex101.com/r/AZTNg9/5
expected: pass
result: pass

https://regex101.com/r/AZTNg9/6
expected: fail
result: fail

[oskosk]

@stevenlin-x can you please clarify in this PR's description which expression is the one you're taking as appropriate from https://www.regular-expressions.info/email.html and then, right next to it, the JS version you're implementing.

Some time has passed and there's a lot of regex translation happening

[stevenlinx]

I've added the regex of both PHP and JS versions to the PR description.

Feedback was addressed

[brbrr]

I played a bit with this PR. I made a simple jsFiddle with some valid and invalid emails according to Wiki and this post

https://jsfiddle.net/f9dkotb8/1/

Here are the results:

====================
Testing valid emails
====================
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
email@[123.123.123.123] isValid: **false**
"email"@example.com isValid: false
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
[email protected] isValid: true
much."more unusual"@example.com isValid: **false**
very.unusual."@"[email protected] isValid: **false**
very."(),:;<>[]".VERY."very@\\\ "very"[email protected] isValid: **false**

====================
Testing invalid emails
====================
plainaddress isValid: false
#@%^%#$@#$@#.com isValid: false
@example.com isValid: false
Joe Smith <[email protected]> isValid: false
email.example.com isValid: false
email@[email protected] isValid: false
[email protected] isValid: false
[email protected] isValid: false
[email protected] isValid: false
あいうえお@example.com isValid: false
[email protected] (Joe Smith) isValid: false
email@example isValid: false
[email protected] isValid: false
[email protected] isValid: **true**
[email protected] isValid: **true**
[email protected] isValid: false
[email protected] isValid: false
"(),:;<>[]@example.com isValid: false
just"not"[email protected] isValid: false
this is"really"not\\[email protected] isValid: false

@stevenlin-x do you think we should cover these failing cases too?

[brbrr]

FWIW: I looked into ways to cover this change with unit-tests. Unfortunately, it's not easy(or even possible) to write tests for a specific function for this file due to its structure. We might want to refactor it to make sure every function is testable.

[stevenlinx]

@brbrr

1.)
In the code samples you provided above, if you replace the regex with the original regex (i.e. the present regex in use in production), it will also result in false positives with those valid and invalid emails samples.

In other words, you didn't read the context of the ticket in full.

2.)
You falsely thought this was a literal RFC 5322 validation exercise when it's not.

The present regex in use was derived from
regular-expressions.info/email.html

Issue #3734 pointed out the regex no longer matches the one on the updated page.

So I simply updated to reflect the character length limits.

3.)
If you think those samples should be addressed, you should ask the author of the regular-expressions.info site or you should open a separate ticket and provide the reasons on why those samples should be addressed.

Or even better, provide the regex you would use.

[stale]

This PR has been marked as stale. This happened because:

• It has been inactive in the past 3 months.
• It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello stevenlin-x! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33713-code before merging this PR. Thank you!

[kraftbj]

Apologies for the massive delay on this one. While there are other regex things to improve to ensure that all valid e-mails are accepted and all invalid are excluded, this is a step improvement and we shouldn't let perfect be the enemy of good.

Approving and open to future PRs to address the other issues (or investigate using browser-based HTML5 validation instead of the JS solution).

[matticbot]

stevenlin-x, Your synced wpcom patch D33713-code has been updated.