PHPMailer's validator defaults to 'php' after the upgrade

[david.binda]

The old version of the PHPMailer was setting the validator to 'auto' resulting in a sophisticated logic for determining what email address validation should be used ( see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-phpmailer.php?rev=47198#L1082 ).

But the new version defaults to 'php', possibly leading to rejection of email addresses which were fine prior to the upgrade. For instance in environments with PCRE_VERSION constant defined (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/PHPMailer/PHPMailer.php?rev=48033#L576 ).

IMHO, the way to accommodate the backward compatibility would be to set the static validator property to a value previously determined by the auto mode. Eg.: by introducing a new function mimicking the logic, and calling it from related code; whenever a new PHPMailer object is created as well as in the wp-includes/class-phpmailer.php file, which is very likely required in case a plugin is doing something with the PHPMailer class.

But there might be some flaws I'm not seeing, or there might be a better approach.

[ayeshrajans]

Are there any specific examples that were allowed in PHPMailer 5's auto, but rejected in PHPMailer 6 php validation?

The auto mode appears to be a more forgiving work-around to deal with PHP and PCRE version differences. I highly doubt that WordPress can even work without PCRE, so most of these conditions will end up being dead code.

With WordPress's current server requirements, the only theoretical option will be pcre8.

I personally think filter_var (php validation) is the more accurate and cleaner approach. I would personally prefer to accept PHPMailer's defaults instead of fighting against it.

Tagging @Synchro of PHPMailer who might provide more insight to this.

[Synchro]

The old validator did allow/deny a few things that were different from filter_var – but it's been quite a while since 6 was released and WP is late to the party (though at least it's got there!). One thing as I recall was that filter_var allowed "dotless" domains (like "a@b") where the PCRE pattern did not, because while they're valid in RFCs, they are banned from use in email by ICANN. There might be some differences with odd things like IPv6 literals.

I moved away from the big regex mainly because there were persistent problems with several versions of PCRE, causing recursion loops, out of memory errors. The pattern that lurks behind filter_var was actually written by the same person as the one in PHPMailer 5, so it is very similar anyway.

Generally speaking though, neither pattern is likely to reject significant numbers of addresses - it's only going to be some very small edge cases that are different, and both are large supersets of the HTML5 input tag validator (which I also wrote). So it is a BC break, but not one that's likely to cause much trouble.

[david.binda]

Hey @ayeshrajans ! Thanks for the info and questions.

I've run into the issue described in this ticket with ace@204.32.222.14 email address which is used in the formatting/IsEmail.php tests.

The email address is valid accordingly to the WordPress' is_email() function, and passes the PHPMailer's validation in pcre8 mode, but fails in php mode. (please see the 50720.2.diff file with an example)

I highly doubt that WordPress can even work without PCRE, so most of these conditions will end up being dead code.
With WordPress's current server requirements, the only theoretical option will be pcre8.

That's kinda what I was trying to say. Odds that pcre8 was available and thus used before the upgrade. That's why I thought it might make sense to preserve that setting, if it's available on the server.

[ayeshrajans]

Thanks for the inputs @Synchro and @davidbinda . I think there definitely is the edge case of the example ace@1.2.3.4 not being accepted in filter_var(), which in turn means PHPMailer not accepting this email address, although it appears to make through is_email.

I think it's fairly agreed upon that we use the verbatim copy of PHPMailer 6.x versions as they are released, so patching PHPMailer wouldn't be the ideal option.

PHPMailer supports custom email validator callbacks, so I'd suggest that we simply make PHPMailer use is_email as the validator.

So to summarize:

• WordPress's is_email is more forgiving and allows a@b email addresses.
• PHPMailer by default uses the more semantically correct filter_var/FILTER_VALIDATE_EMAIL
• Because is_email allows through a@b addresses, it is technically possible to have email addresses that are rejected only at PHPMailer level, but not at is_email.
• To fix, we can make PHPMailer reuse is_email function to validate email addresses.
• We will need a few extra tests to make sure PHPMailer and is_email accept/reject same sets of email addresses, including the edge case ones.

I will submit tests if this approach is preferred, but I will attach a patch.

[ayeshrajans]

​https://travis-ci.com/github/Ayesh/wordpress-develop/builds/176764424

[Synchro]

I quite agree, injecting WP's built-in validator is a good idea for continuity.

[whyisjake]

@ayeshrajans the patch looks good, but is failing against the provided test from @davidbinda. Any thoughts on how to get them to pass?

[ayeshrajans]

@whyisjake - thanks for the review. I don't think that patch applies to the patch in #4-1, because it is validating email addresses directly, and I think it's more of a demonstration that a real test that is meant to be included in WP core.

I will upload a new patch with a new test added (to the mail test file, which I believe is more appropriate).

[ayeshrajans]

​https://travis-ci.com/github/Ayesh/wordpress-develop/builds/177174443

[david.binda]

Yeah, that's correct. The tests I provided were more a demonstration than tests :) Sorry for the confusion.

However, in case a plugin is overriding the wp_mail function (as it's a pluggable function), the is_email won't be set as a validator.

That's why I was originally proposing to set the validator outside the wp_mail function, so it would affect all other usages, even in case the function is overridden, or the PHPMailer's instance is being used directly.

[SergeyBiryukov]

The tests in 50720.4.patch​, as seen in ​https://travis-ci.com/github/Ayesh/wordpress-develop/jobs/365059445, still appear to fail, as the validator function should return a boolean value, not a string.

[ayeshrajans]

Thanks a lot, this one should work: ​https://travis-ci.com/github/Ayesh/wordpress-develop/builds/177332069

[whyisjake]

In 48645:

Mail: PHPMailer swap to use is_email for the default validator.

Prior to the PHPMailer update in 5.5, old version of the PHPMailer was setting the validator to 'auto' resulting in a sophisticated logic for determining what email address validation should be used. But the new version defaults to 'php', possibly leading to rejection of email addresses which were fine prior to the upgrade. Let's use the WordPress core function is_email() so that it can be fully pluggable.

Fixes #50720.
Props david.binda, ayeshrajans, Synchro, SergeyBiryukov, whyisjake.