wp_mail() can send emails with a blank 'From' address

[drtonyb]

If wp_mail() is used to send an email with a blank 'From' address set, it is not detected and replaced with the default wordpress@mydomainname...

Problem as I see it is in wp-includes/pluggable.php, where line 366 contains the test

	if ( !isset( $from_email ) ) {

The test using isset does not detect an empty string, allowing a blank 'from' address to be sent. Wouldn't it be better to test using empty?

	if ( empty( $from_email ) ) {

The same issue affects the 'from name' test on line 356.

[valendesigns]

I've change the !isset to empty in the wp_mail() function for the $from_name, $from_email, $content_type, and $charset conditionals.

[valendesigns]

Just tested, and the 30266.diff​ patch is still applying cleanly. Any core devs want to jump in and give this ticket a review?

[valendesigns]

Thanks @SergeyBiryukov, do you think this needs unit tests or is the proposed change simple enough?

[SergeyBiryukov]

Replying to valendesigns:

Thanks @SergeyBiryukov, do you think this needs unit tests or is the proposed change simple enough?

Seems simple, but having unit tests would still be great.

[valendesigns]

@SergeyBiryukov I'll refresh the patch with unit tests. Could you make me the owner? Cheers!

[valendesigns]

My latest patch 30266.1.diff​ doesn't make the same assumptions as the first one. I added 4 unit tests and found that the real issue was in the switch statement. Basically, we have four bugs.

• First, the $from_email was being set to an empty string and was remedied by changing else to else if ( '' !== trim( $content ) ) so it doesn't get set.

• Second, we have an issue where one is not like the other. If you pass in an email as the from header like From: wordpress@example.com you get back From: WordPress <wordpress@example.com>, though if you pass in the email like From: <wordpress@example.com> you get back "<wordpress@example.com" <wordpress@example.com>, which I fixed by checking if the less than symbol is not in the first position before setting the $from_name.

• Third, there was an issue where $charset was coming back empty because of list( $type, $charset ) setting the variable. I changed that to list( $type, $charset_content ) and updated the associated code so $charset is not set to an empty value.

• Last, $content_type was also being set to an empty value so I replaced the else with else if ( '' !== trim( $content ) ) like the first issue.

I did not touch the isset checks as they are now being correctly handled in the switch statement. Please test and let me know if there's something that still needs work.

[valendesigns]

Anyone core devs willing to test and give some feedback on the latest patch?

[valendesigns]

Patch has been refreshed to follow elseif coding standards and remove the angle brackets used in the $to variable from the tests.

[DrewAPicture]

30266.2.diff​ looks good to me, and sends emails in the expected format. Unit tests pass. Moving for commit consideration.

[westi]

Replying to DrewAPicture:

30266.2.diff​ looks good to me, and sends emails in the expected format. Unit tests pass. Moving for commit consideration.

I'm unsure about a few things here:

• I think the unit tests would be better without the preg_matches and testing the actually generated headers - using the regular expressions looks strange in a set of tests to me
• I think that the change to add } elseif ( '' !== trim( $content ) ) { should be commented so it is obvious why this is being done.

[valendesigns]

Replying to westi:

• I think the unit tests would be better without the preg_matches and testing the actually generated headers - using the regular expressions looks strange in a set of tests to me

I'll rethink that approach and update.

• I think that the change to add } elseif ( '' !== trim( $content ) ) { should be commented so it is obvious why this is being done.

Will add inline comments to the next iteration.

[valendesigns]

@westi There isn't a way to grab the headers other than in that string, AFAIK. What did you have in mind when you said the "actually generated headers"?

[valendesigns]

@westi I've cleaned up the tests and added some inline comments as you suggested. Please have a look when you get a moment. Cheers!

[DrewAPicture]

30266.3.diff​ applies and unit tests pass. Moving for commit consideration.

[boonebgorges]

In 32070:

Improve handling of incomplete From and Content-Type headers in wp_mail().

When an incomplete header is provided (eg, 'From' with an email address but no
name), ensure that the WP defaults are filled in properly.

Props valendesigns.
Fixes #30266.