Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 months ago

#51030 closed defect (bug) (wontfix)

Error in wp_mail function

Reported by: hichembelhadj's profile hichembelhadj Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Mail Keywords: has-patch has-unit-tests needs-testing dev-feedback 2nd-opinion
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Hello,

Currently I am developing a wordpress theme with a script to send a verification mail using wp_mail(). Since switching to wordpress version 5.5 my script no longer works. By trying PHP's mail() function it works.
By reinstalling the version of wordpress 5.4 with the wp_mail() function, it works again.
While performing the debugging, I get the following error:

[wp_mail_failed] => Array
                (
                    [0] => Invalid address:  (From): wordpress@localhost
                )

Thanks for your feedback

Change History (15)

#1 @ayeshrajans
4 years ago

Hi @hichembelhadj - Welcome to WordPress Trac.

Are you instantiating PHPMailer on your own? wp_mail function should use the PHPMailer 6 library now, and a bug with the address validation was fixed recently: #50720.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#2 @SergeyBiryukov
4 years ago

  • Component changed from General to Mail
  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.5.1

Related: [48645] / #50720.

#3 @desrosj
4 years ago

  • Keywords reporter-feedback added

#4 @khag7
4 years ago

Prior to 5.5 the wp_mail function used PHPMailer's validator function to validate emails. By default, PHPMailer::$validator variable was set to auto, and PHPMailer would then attempt to use a somewhat complex regex to validate emails.

The PHPMailer auto option was using pcre8 (if avaliable) and wordpress@localhost would be valid in that case.

With the update to the newest version of PHPMailer (#41750) the auto option is no longer available, and instead uses the php validation option. Shortly after #41750 was closed, this proved to be an issue (#50720) so WP core moved to using our own is_email function for validation.

Unfortunately, WordPress's is_email function does not consider wordpress@localhost to be a valid email, thus this ticket exists.

I think we may have been quick to jump to using our own is_email function when another better option exists. The PHPMailer class includes a validation option called html5 which I think would work.

Some quick tests show the following statements to be true:
false=== is_email( 'wordpress@localhost' )
true === PHPMailer\PHPMailer\PHPMailer::validateAddress('wordpress@localhost','html5')
false=== PHPMailer\PHPMailer\PHPMailer::validateAddress('wordpress@localhost','php')
true === PHPMailer\PHPMailer\PHPMailer::validateAddress('wordpress@localhost','pcre8')

As of 5.5, in wp_mail we assign $phpmailer::$validator to a function which wraps is_email.
Perhaps instead we should just do $phpmailer::$validator = 'html5'?

Alternatively we improve is_email to support emails with a domain that does not include a period.

#5 @khag7
4 years ago

A test to add:

function test_phpmailer_validator() {
	$phpmailer = $GLOBALS['phpmailer'];
	$this->assertTrue( $phpmailer->validateAddress( 'foo@localhost' ), 'Assert PHPMailer accepts domains without a dot' );
}

#6 @SergeyBiryukov
4 years ago

  • Keywords needs-patch needs-unit-tests added; reporter-feedback removed

#7 follow-up: @ayeshrajans
4 years ago

is_email was used because the follow up ticket to PHPMailer 6 was that certain emails that were allowed by iz_email were not accepted by PHPMailer, so using is_email() to validate the emails was sensible.

I doubt MTAs would accept root@localhost as valid address in production use cases, so I still believe using is_email provides a safe bar to validate email addresses.

In either case, is_email is mostly used for front end uses, so it should be strict to only accept real life email addresses as opposed to pure RFC addresses.

#8 in reply to: ↑ 7 @khag7
4 years ago

Replying to ayeshrajans:

is_email is mostly used for front end uses, so it should be strict to only accept real life email addresses as opposed to pure RFC addresses.

I agree that is_email is the best choice for validating input from form fields where we'd expect a "normal" email address. (i.e. "front end uses")

But "behind the scenes" it should be acceptable for PHPMailer to allow email@localhost. After all, this issue was created specifically because the updates in 5.5 broke functionality for some users.

If changing the validation for wp_mail to use PHPMailer's 'html5' validation instead of 'is_email' fixes the issue without creating any new issues, it feels like the right solution.

This ticket was mentioned in PR #490 on WordPress/wordpress-develop by khag7.


4 years ago
#9

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

Trac ticket: https://core.trac.wordpress.org/ticket/51030
Includes some tests as well

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


4 years ago

#11 @audrasjb
4 years ago

  • Milestone changed from 5.5.1 to 5.6

As per today’s bug scrub: Moving to milestone 5.6 to give it more time.

#12 @hellofromTonya
4 years ago

  • Keywords needs-testing dev-feedback 2nd-opinion added

After reviewing discussion in the scrub, this ticket needs more discussion, testing, and investigation.

Here are some of the discussions:

Per joostdevalk:

let me get this straight, all this is about emailing a domain that has no dots in it?
i see why it’s annoying for a developer but it’s not like this has a live use case, right? or am i nuts?

Per Sergey:

Seems like it

Per johnbillion:

It could affect an intranet

Marking this ticket for dev-feedback and 2nd-opinion to get more eyes on it.

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


4 years ago

#14 @helen
4 years ago

  • Milestone 5.6 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I recognize that this used to work and no longer does, but upon further reflection I think it's correct that it no longer does because that style of email address is not valid for SMTP. So I would say that by default, the current WordPress behavior is correct, and that should you be doing something more specialized like in your case, there is a "hoop" you have to jump through in the form of needing to use the is_email filter with the domain_no_periods context to get it to pass instead.

#15 @bmenant
3 months ago

I’d like to reopen this ticket since I have a different opinion and understanding of the original ticket. I keep coming back to this issue each time I set up a new development environment and notice wp_mail doesn’t fire anything because of this issue I forgot about... when I feel like it should just work out of the box.

I agree wp_mail should prevent sending emails TO unrealistic email addresses (as per is_email).

I’m not quite sure to agree wp_mail should not allow sending FROM wordpress@localhost email address.

Notably because wp_mail may infer its sender address from the hostname. On a local development instance (e.g. container), localhost is a common hostname.

And on such instance, it is fairly common to test emails using a mail catcher system (e.g. Mailpit) acting as a SMTP server (with a docker-compose configuration or with a podman pod for example).

A fix could be made over here, with a check for the lack of tld, which is the root cause of the issue IMHO:
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/pluggable.php#L368-L387

                /*
                 * If we don't have an email from the input headers, default to wordpress@$sitename
                 * Some hosts will block outgoing mail from this address if it doesn't exist,
                 * but there's no easy alternative. Defaulting to admin_email might appear to be
                 * another option, but some hosts may refuse to relay mail from an unknown domain.
                 * See https://core.trac.wordpress.org/ticket/5007.
                 */
                if ( ! isset( $from_email ) ) {
                        // Get the site domain and get rid of www.
                        $sitename   = wp_parse_url( network_home_url(), PHP_URL_HOST );
                        $from_email = 'wordpress@';

                        if ( null !== $sitename ) {
                                if ( str_starts_with( $sitename, 'www.' ) ) {
                                        $sitename = substr( $sitename, 4 );
                                }
+                               if ( ! str_contains( $sitename, '.' ) ) {
+                                       $sitename .= '.localdomain';
+                               }
                                $from_email .= $sitename;
                        }
                }

Or alternatively with a more specific test if ( 'localhost' === $sitename ) which is stricter but wouldn’t cover mutlisites development environments with custom /etc/hosts or alike setups.

Another option would be to leave it as is, but to add a test in the site-health system so users can quickly figure out emailings won’t work on a tld-less hostname.

What do you think?

Last edited 3 months ago by bmenant (previous) (diff)
Note: See TracTickets for help on using tickets.