WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30266 closed defect (bug) (fixed)

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

Reported by: drtonyb Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.0
Component: Mail Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

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.

Attachments (4)

30266.diff (1.5 KB) - added by valendesigns 5 years ago.
30266.1.diff (4.2 KB) - added by valendesigns 5 years ago.
30266.2.diff (4.2 KB) - added by valendesigns 5 years ago.
30266.3.diff (4.3 KB) - added by valendesigns 5 years ago.

Download all attachments as: .zip

Change History (29)

@valendesigns
5 years ago

#1 @valendesigns
5 years ago

  • Keywords has-patch needs-testing added

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

#2 @valendesigns
5 years ago

  • Keywords dev-feedback added

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

#3 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.2

#4 follow-up: @valendesigns
5 years ago

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

#5 in reply to: ↑ 4 @SergeyBiryukov
5 years ago

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.

#6 @valendesigns
5 years ago

  • Keywords needs-unit-tests added; needs-testing dev-feedback removed

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

#7 @SergeyBiryukov
5 years ago

  • Owner set to valendesigns
  • Status changed from new to assigned

@valendesigns
5 years ago

#8 @valendesigns
5 years ago

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

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.

#9 @valendesigns
5 years ago

  • Keywords has-patch added

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


5 years ago

#11 @valendesigns
5 years ago

  • Keywords dev-feedback added

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

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


5 years ago

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


5 years ago

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


5 years ago

@valendesigns
5 years ago

#15 @valendesigns
5 years ago

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

#16 follow-up: @DrewAPicture
5 years ago

  • Keywords commit added; needs-testing dev-feedback removed

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

Last edited 5 years ago by DrewAPicture (previous) (diff)

#17 in reply to: ↑ 16 ; follow-up: @westi
5 years ago

  • Keywords needs-patch added; has-patch commit removed

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.

#18 in reply to: ↑ 17 @valendesigns
5 years ago

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.

#19 @valendesigns
5 years ago

@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
5 years ago

#20 @valendesigns
5 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

@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!

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


5 years ago

#22 @DrewAPicture
5 years ago

  • Keywords commit added; dev-feedback removed

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

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


5 years ago

#24 @DrewAPicture
5 years ago

  • Owner changed from valendesigns to boonebgorges
  • Status changed from assigned to reviewing

#25 @boonebgorges
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.