Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#55821 new defect (bug)

`is_email()` does not follow PHP FILTER_VALIDATE_EMAIL rules, when an email has double period (..)

Reported by: khokansardar Owned by:
Priority: normal Milestone: Awaiting Review
Component: Formatting Version: 5.9.3
Severity: normal Keywords: has-test-info has-patch has-unit-tests
Cc: Focuses:

Description (last modified by dmsnell)

If there has a typo in an email address like there has double period .. in an email address ( abc..def@… ). is_email() function return valid email when we have use like -

is_email( 'abc..def@xyz.com' )

Where it should not return as valid email address. Which is working with PHP FILTER_VALIDATE_EMAIL checking. When we are checking the same kind of email with below code it return boolean false.

filter_var( 'abc..def@xyz.com', FILTER_VALIDATE_EMAIL )

Expected behaviour should be like this. When use is_email() function it should respect like what PHP does.

  • #31992 where the change in email address parsing was introduced

Change History (13)

#1 @khokansardar
4 years ago

  • Severity normalmajor

#2 @desrosj
4 years ago

  • Severity majornormal

#3 @Boniu91
4 years ago

Testing Instructions

These steps define how to reproduce the issue, and indicate the expected behavior.

Steps to Reproduce

  1. Add the following code to the footer.php, before closing </html> https://snippi.com/s/8gji8lp
  2. Visit the page and check the footer
  3. 🐞 Bug occurs, email address is valid is displayed

Expected Results

When testing a patch to validate it works as expected:

  • 3rd point should display email address is not valid.

Additionaly

We could change the abc..def@xyz.com to other not valid emails and see the results.

#4 @Boniu91
4 years ago

  • Keywords has-testing-info added

#5 @khokansardar
3 years ago

  • Keywords needs-patch added

This ticket was mentioned in PR #4631 on WordPress/wordpress-develop by @lopo.


3 years ago
#6

  • Keywords has-patch has-unit-tests added; needs-patch removed

Added a check for double periods on the local part of the email address (taking inspiration from the same check on the domain part) + added unit test cases for both the local and the domain part (which was missing).

Trac ticket: https://core.trac.wordpress.org/ticket/55821

#7 @wordpressdotorg
16 months ago

  • Keywords has-test-info added; has-testing-info removed

This ticket was mentioned in PR #12188 on WordPress/wordpress-develop by @nimeshatxecurify.


3 months ago
#8

## Description

is_email( 'abc..def@xyz.com' ) returns the address as valid, even though the local part contains consecutive dots. PHP's filter_var( …, FILTER_VALIDATE_EMAIL ) rejects it, and so does every SMTP server, because the address is undeliverable. The same applies to a leading dot (.abc@xyz.com) and a trailing dot (abc.@xyz.com).

Since [62225]/#31992, is_email() delegates to WP_Email_Address::from_string(), whose local-part patterns are derived from the WHATWG <input type=email> character set. That set places . in the character class with no positional constraint ([…]+), so any arrangement of dots passes. The domain side already validates correctly (each label must be bookended by an alphanumeric), so this only affected the local part.

This PR restructures the local part as a proper dot-atom, mirroring the existing domain-label design already in the class: a dot may only separate non-empty atoms. Leading, trailing, and consecutive dots are now rejected, while all previously valid addresses — including the intended Unicode addresses from #31992 (grå@grå.org) — continue to validate.

## Why reject these

This change is not a departure from #31992 — it implements a decision that ticket's discussion already reached. While reviewing test cases on #31992, @agulbra noted:

to..to@couc.ou and to.@couc.ou are invalid according to RFC 5321, therefore WordPress cannot send mail to them. Rejecting these is a good idea.

and @peteresnick — the author of RFC 5322 — confirmed:

Also invalid according to 5322 section 3 (the more restrictive) syntax.

The WHATWG character set was adopted as the *basis* for the allowed characters, but the agreed intent was to be at least as strict as the syntaxes used to *generate* and *deliver* mail, which forbid empty atoms in the local part. The shipped regex used the WHATWG […]+ shorthand verbatim and did not enforce that structure; this PR closes that gap.

## Implementation

  • Add LOCAL_PART_ATOM_ASCII and LOCAL_PART_ATOM_UNICODE constants (the previous local-part character sets, minus the dot).
  • Assemble LOCAL_PART_ASCII_REGEX / LOCAL_PART_UNICODE_REGEX as atom (?:\. atom)*, the same dot-separated structure already used by DOMAIN_ASCII_REGEX / DOMAIN_UNICODE_REGEX.

## Testing instructions

  1. Apply the patch.
  2. Confirm the following now return false:
    is_email( 'abc..def@xyz.com' ); // consecutive dots
       is_email( '.abc@xyz.com' );     // leading dot
       is_email( 'abc.@xyz.com' );     // trailing dot
    
  1. Confirm normal addresses still validate:
    is_email( 'bob@example.com' );
       is_email( 'bill+ted@example.com' );
       is_email( 'a.b.c@example.com' ); 
       is_email( 'grå@grå.org' );
    
  1. Or run the test suite:
    npm run test:php -- --group 55821
       npm run test:php -- --filter 'IsEmail|EmailAddress|Antispambot|SanitizeEmail'
    

### Unit tests

  • tests/formatting/isEmail.php: Moved ..@example.com from the valid provider to the invalid provider, added the leading/trailing/consecutive-dot cases, and tagged the test with @ticket 55821.
  • tests/wp-email-address/wpEmailAddress.php: New @ticket 55821 test asserting rejection in both unicode and ascii modes.
  • tests/formatting/antispambot.php: Relabeled a now-inaccurate data-set key (antispambot() is obfuscation-only; behavior unchanged).

Trac ticket: https://core.trac.wordpress.org/ticket/55821

## Use of AI Tools

AI assistance: Yes — used to investigate the regression, cross-reference the #31992 discussion, draft the fix, and write the tests. All changes were reviewed and verified by me.

#9 @nimeshatxecurify
3 months ago

@dmsnell @agulbra @arnt May I request your input on this ticket, and the PR?

#10 @ekamran
3 months ago

I tested the newer patch from GitHub PR 12188: https://github.com/WordPress/wordpress-develop/pull/12188 locally.

I also checked current trunk before testing. The issue still exists there. For example, the current is_email() tests still include ..@example.com as a valid email case, and the current WP_Email_Address local-part regex still allows dots without checking dot-atom placement.

Environment

  • wordpress-develop PR branch: 55821-double-dot-fix
  • Docker: 29.5.3
  • PHPUnit: 9.6.34
  • Single site test run

Test results

npm run test:php -- --group 55821

Passed: 25 tests, 30 assertions.

npm run test:php -- --filter 'IsEmail|EmailAddress|Antispambot|SanitizeEmail'

Passed: 149 tests, 242 assertions.

composer lint:errors on the 4 touched files

Passed: 4 / 4 files.

What I checked

  • The PR rejects emails with invalid dots in the local part.
  • This includes leading dot, trailing dot, consecutive dots, only dots, and single dot local part.
  • The related email formatting tests are also passing, including is_email(), WP_Email_Address, antispambot(), and sanitize_email().

Based on this local automated testing, GitHub PR 12188 looks good to me.

Last edited 3 months ago by ekamran (previous) (diff)

This ticket was mentioned in Slack in #core by amykamala. View the logs.


3 months ago

#12 @dmsnell
3 months ago

thanks @nimeshatxecurify — I expected a lot of cases to arise with the change.

one note though: Google happily delivers email to localparts containing a leading or trailing dot. this lies, I think, at the heart of the complexity of email addressing. it still rejects successive dots, but allows the email addresses this change proposes to reject.

the WHATWG spec intentionally allows these addresses, meaning they should be allowed in a browser. are there any email servers or services which deliver to localparts with successive dots?

I want us to be careful about diverging from correspondence with the browsers since that is the point of integration between the WordPress backend and the interface that users see. at times, we can apply additional rules, which is fine, but there is great benefit in applying those rules after understanding the spec value first.

for example, we have an is_email() filter that could be used to further restrict the email addresses that WordPress allows, but right now a browser will accept all of these and allow a site visitor or admin to enter them.

example with dot variants


this is not me saying the request in this ticket is wrong (apart from that I do recommend we avoid using filter_var() as a standard, and at that, avoid using filter_var() anywhere). I merely want to make sure we’re thorough in the choices we make, as diversions from browser behavior introduces new opportunities to leave site visitors hanging and to exploit divergence between how a browser and how WordPress understands a given input.

#13 @dmsnell
3 months ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.