Make WordPress Core

Opened 12 years ago

Closed 8 years ago

Last modified 5 years ago

#18792 closed defect (bug) (duplicate)

Wrong FROM email when using wp_mail and built in mail() function

Reported by: pigster's profile pigster Owned by: westi's profile westi
Milestone: Priority: normal
Severity: normal Version: 3.2.1
Component: External Libraries Keywords: dev-feedback needs-patch
Focuses: Cc:

Description

When using wp_mail in combination with mail() function, then From: envelope passed through -f parameter to sendmail is not set correctly.

Here is simple patch, that fixes the problem:

--- pluggable.php	2011-09-26 20:54:02.219330702 +0200
+++ pluggable.fixed.php	2011-09-27 18:19:21.283454810 +0200
@@ -394,8 +394,7 @@
 	}
 
 	// Plugin authors can override the potentially troublesome default
-	$phpmailer->From     = apply_filters( 'wp_mail_from'     , $from_email );
-	$phpmailer->FromName = apply_filters( 'wp_mail_from_name', $from_name  );
+	$phpmailer->SetFrom(apply_filters('wp_mail_from', $from_email),apply_filters('wp_mail_from_name', $from_name));
 
 	// Set destination addresses
 	if ( !is_array( $to ) )

Change History (22)

#1 @knutsp
12 years ago

  • Cc knut@… added

#2 @westi
12 years ago

  • Component changed from Mail to External Libraries
  • Owner set to westi
  • Status changed from new to accepted

#3 @KevinHamilton
12 years ago

  • Keywords has-patch added

If sending emails in a loop with a different From address for each, the above patch results in the envelope for all being set to the same as the first. Need to also reset the Sender property, as in the following patch:

===================================================================
--- pluggable.php	(revision 41797)
+++ pluggable.php	(working copy)
@@ -370,6 +370,7 @@
 	$phpmailer->ClearCCs();
 	$phpmailer->ClearCustomHeaders();
 	$phpmailer->ClearReplyTos();
+	$phpmailer->Sender='';
 
 	// From email and name
 	// If we don't have a name from the input headers

#4 @rtweedie
12 years ago

I hit a similar issue with the 'return-path' and 'reply-to' email headers not being correctly populated. Using the setFrom() method helps ensure that headers are correctly assigned.

At the moment a simple plugin helps resolve the issue, but it would be great to hear if these patches will make their way in the core in the future.

#5 @c3mdigital
11 years ago

  • Keywords needs-refresh added

#6 @gruvin
11 years ago

I just got through finding this bug, fixing it myself, searching the bug tracker here to find this issue and, well, here are my observations on the state of play ...

As of release version 3.6.1, the patch from KevinHamilton above has apparently still not been implemented, though the code on pluggable.php has had new stuff addedd ...

pluggable.php:
 309:         $phpmailer->ClearAddresses();
 310:         $phpmailer->ClearAllRecipients();
 311:         $phpmailer->ClearAttachments();
 312:         $phpmailer->ClearBCCs();
 313:         $phpmailer->ClearCCs();
 314:         $phpmailer->ClearCustomHeaders();
 315:         $phpmailer->ClearReplyTos();

I checked the ClearAddresses method, hoping it was clearing Sender. But it's not. Not sure if it should. Perhaps there needs to be a ClearFrom method added to the PHPMailer class?

I see that Sender does indeed need to be reset, because it only gets set in SetFrom if it's currently empty.

class-phpmailer.php:
 713       if (empty($this->Sender)) {
 714         $this->Sender = $address;
 715       }

Also, the original patch provided by pigster, who opened this ticket, also still remains. Specifically, PHPMailer's internal class properties are being accessed directly in the following code, inadvertently preventing property Sender from ever being set ...

pluggable.php
 341:         $phpmailer->From     = apply_filters( 'wp_mail_from'     , $from_email );
 342:         $phpmailer->FromName = apply_filters( 'wp_mail_from_name', $from_name  );

I found and fixed that problem myself, just before locating this bug ticket, unsurprisingly in the precise same way as pigster, like so ...

pluggable.php:
341:         $phpmailer->SetFrom(apply_filters( 'wp_mail_from'     , $from_email ), apply_filters( 'wp_mail_from_name', $from_name  ));

It works, albeit not accounting for the looping issue, noted above by KevinHamilton.

Did I mention that this bug has been around for at least two years and has surely caused a massive amount of trouble in terms of mail getting tagged as spam and what not, for a very, very long time? Surely that is a serious thing to the developers? Perplexed, I am. *shrug* Perhaps I'm missing something.

Has this issue been addressed already in development versions 3.7 or v3.8? How far away are those from release anyway? Where would I go to find that out myself?

Can I contribute code or a patch directly, have it verified and included in v3.6.2 somehow? I've read the docs. Whilst I know how to submit bug reports, patches and more. But not how to actually get this thing sorted. "Somewhat frustrating.", said the new kid on the block. (I'll read the docs again, lest I missed something.)

In the meantime, is there already plugin that addresses this fault specifically? I have not been able to find one -- though there are several that go around the problem this bug causes, evidently not realising where the original fault actually lies. (The Xmail plugin, for one, uses an incredibly convoluted method to get around this bug. Clever ... but oh so unneeded, if this simple error were simply fixed, already. :-/ ) Should I go to the effort of writing a plugin that fixes this, "properly"? At first glance, that doesn't seem likely possible.

--OR-- Is wp_mail() deprecated anyway -- or should it be, in favor of using PHPMailer class directly in all plugins from now on? Frankly, I'm too new to Wordpress to even guess. Any and all comments welcomed, either here or to my email (assuming you get to see that?)

Gruvin.

Last edited 11 years ago by gruvin (previous) (diff)

#7 @gruvin
11 years ago

  • Cc gruvin added
  • Severity changed from normal to major

#8 @gruvin
11 years ago

  • Keywords dev-feedback added

#9 @gruvin
11 years ago

  • Keywords needs-testing needs-unit-tests 2nd-opinion added; needs-refresh dev-feedback removed

OK. Here's a patch that takes care of all of the above.

This patch makes the original changes suggested by pigster and implements an object class friendly version of KevinHamilton's Sender= fix, as noted above.

I've tested this patch within the context that I understand the original bug and it appears to work fine. My mail from Wordpress is now being sent from the correct server IP address, for the domain being used and from the correct sender address -- via a plugin that uses wp_mail(). This results in correct SPF records being used and in the likes of Gmail not marking such messages as spam.

Index: class-phpmailer.php
===================================================================
--- class-phpmailer.php	(revision 25393)
+++ class-phpmailer.php	(working copy)
@@ -2320,6 +2320,16 @@
   }
 
   /**
+   * Clears from address and name. Also clears Sender.  Returns void.
+   * @return void
+   */
+  public function ClearFrom() {
+    $this->From='';
+    $this->FromName='';
+    $this->Sender='';
+  }
+
+  /**
    * Clears all recipients assigned in the ReplyTo array.  Returns void.
    * @return void
    */
Index: pluggable.php
===================================================================
--- pluggable.php	(revision 25393)
+++ pluggable.php	(working copy)
@@ -313,6 +313,7 @@
 	$phpmailer->ClearCCs();
 	$phpmailer->ClearCustomHeaders();
 	$phpmailer->ClearReplyTos();
+        $phpmailer->ClearFrom();
 
 	// From email and name
 	// If we don't have a name from the input headers
@@ -337,9 +338,9 @@
 	}
 
 	// Plugin authors can override the potentially troublesome default
-	$phpmailer->From     = apply_filters( 'wp_mail_from'     , $from_email );
-	$phpmailer->FromName = apply_filters( 'wp_mail_from_name', $from_name  );
 
+        $phpmailer->SetFrom(apply_filters( 'wp_mail_from'     , $from_email ), apply_filters( 'wp_mail_from_name', $from_name  ));
+
 	// Set destination addresses
 	if ( !is_array( $to ) )
 		$to = explode( ',', $to );

P.S: Revision r25393 (trunk as of just now and to which this patch applies) appears to be the exact same revision as tags/3.6.1, which I also checked out, for comparison.

Last edited 11 years ago by gruvin (previous) (diff)

#10 follow-up: @SergeyBiryukov
11 years ago

Replying to gruvin:

As of release version 3.6.1, the patch from KevinHamilton above has apparently still not been implemented, though the code on pluggable.php has had new stuff addedd ...

It's not a recent addition, see [4946] for #3862.

Has this issue been addressed already in development versions 3.7 or v3.8?

Not yet, otherwise this ticket would already be closed.

How far away are those from release anyway? Where would I go to find that out myself?

See http://make.wordpress.org/core/version-3-7-project-schedule/.

Can I contribute code or a patch directly, have it verified and included in v3.6.2 somehow?

Minor releases are for security and regression fixes only. With enough testing, the patch can still make it into 3.7 or 3.8.

--OR-- Is wp_mail() deprecated anyway -- or should it be, in favor of using PHPMailer class directly in all plugins from now on?

No, wp_mail() is not going to be deprecated.

Replying to gruvin:

OK. Here's a patch that takes care of all of the above.

  1. class-phpmailer.php is an external library, so we should avoing changing it unless it's something worth reporting upstream. Since all those properties are public, we can clear them in wp_mail() as needed.
  2. Please attach the patch as a file instead of pasting it in the comment box.

#11 @dd32
11 years ago

It looks like these patches seem to be suggesting that the Sender header would be set?

If so, See #5007 #5294 #5869 for issues where having that set causes email failures

This ticket may also be a duplicate of #14888 and finally, #22837 wants a Reply-To header (in addition to the Sender header)

#12 @SergeyBiryukov
11 years ago

#25354 was marked as a duplicate.

#13 @SergeyBiryukov
10 years ago

#25651 was marked as a duplicate.

#14 in reply to: ↑ 10 @MaximumResults
10 years ago

Replying to SergeyBiryukov:

Minor releases are for security and regression fixes only. With enough testing, the patch can still make it into 3.7 or 3.8.

I would suggest that this is a security issue in many instances.

If you host your WordPress site on a cPanel hosting service, any mail sent with wp_mail() that doesn't set the "Sender:" and "Reply-To:" headers with $phpmailer->SetFrom() as shown in the patch will include the cPanel login name and the hosting server hostname in the email headers.

Some of those emails (registration confirmation, forgotten passwords, confirmation emails from form mailer plugins) could go to external entities including bots that might be playing with forms on the site.

With the details from the email, the recipient of the email has almost everything needed to log in to cPanel on the hosting server and create problems. The only thing missing is the cPanel password.

Replying to SergeyBiryukov:

Replying to gruvin:

OK. Here's a patch that takes care of all of the above.

  1. class-phpmailer.php is an external library, so we should avoing changing it unless it's something worth reporting upstream. Since all those properties are public, we can clear them in wp_mail() as needed.
  2. Please attach the patch as a file instead of pasting it in the comment box.

I would also suggest using this for the new code:

	$phpmailer->SetFrom(apply_filters('wp_mail_from', $from_email),apply_filters('wp_mail_from_name', $from_name),true);

The current version of class-phpmailer.php does default to 1 for the third parameter ($auto within the SetFrom() function), but $this->Sender and $this->ReplyTo are set inside a block with "if ($auto) {/*...Set Reply-to and Sender...*/}" Specifying an explicit true on that parameter might be wise, just in case defaults change in a future version of class-phpmailer.php.

#15 follow-up: @atimmer
10 years ago

Maybe we can set Sender only when the user passes a custom mail address. When a user passes a mail address we then assume their intend is to make sure te receiver receives the message from that mail address.

We should add the check after the filter. If the $from_email doesn't contain Wordpress@ we set the Sender.

#16 in reply to: ↑ 15 ; follow-up: @MaximumResults
10 years ago

Replying to atimmer:

Maybe we can set Sender only when the user passes a custom mail address. When a user passes a mail address we then assume their intend is to make sure te receiver receives the message from that mail address.

We should add the check after the filter. If the $from_email doesn't contain Wordpress@ we set the Sender.

So, you're saying, if I have "anyone can register" turned on, and my site is hosted on a cPanel hosting service, WordPress should send the cPanel hosting login details (less password) to anyone who registers on the site? That doesn't sound like a good idea at all. The "WordPress@" emails are the ones that most need to have the Sender set, and that needs to apply to the "Envelope-to:" header, the "Sender:"/"X-Sender:" header and especially for the "-f{user}@{domain}" string passed to the mail() PHP function.

Last edited 10 years ago by MaximumResults (previous) (diff)

#17 in reply to: ↑ 16 @bpetty
10 years ago

Replying to MaximumResults:

So, you're saying, if I have "anyone can register" turned on, and my site is hosted on a cPanel hosting service, WordPress should send the cPanel hosting login details (less password) to anyone who registers on the site? That doesn't sound like a good idea at all. The "WordPress@" emails are the ones that most need to have the Sender set, and that needs to apply to the "Envelope-to:" header, the "Sender:"/"X-Sender:" header and especially for the "-f{user}@{domain}" string passed to the mail() PHP function.

The fact that you might see email from username@box### has absolutely nothing to do with any headers WordPress does or does not configure when sending email from shared hosting providers.

Speaking for Bluehost (other shared hosts do this as well though), shared accounts don't have any valid default email addresses configured until you specifically configure one, and absolutely require email to be sent from a valid address in order to prevent abuse and spam. Even with WordPress configured to use a default wordpress@$sitename address, the "From" header is rewritten by default (to the username@box### address) by shared hosting providers since that still isn't a valid email address. The remaining headers won't make any difference here regardless. See this help article for more information. In the case of WordPress, you could alternatively just create a "wordpress" email account, and it would just work too, making this rather simple to fix if you're concerned about security.

By the way, WordPress has turned off user registration by default as well, so it's not the best argument for modifying the default behavior of email since it's not a default option itself.

#18 @soulseekah
10 years ago

  • Cc gennady@… added

#19 @kitchin
10 years ago

  • Cc kitchin@… added

#20 @chriscct7
8 years ago

  • Keywords dev-feedback needs-patch added; has-patch needs-testing needs-unit-tests 2nd-opinion removed
  • Severity changed from major to normal

The existing code needs to be made into a proper patch, and the filter would need to be documented. Also a filter inside of a filter just seems a bit weird.

#21 @MattyRob
8 years ago

  • Resolution set to duplicate
  • Status changed from accepted to closed

Duplicate of #37736.

This appears to have been fixed in [38058]

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

#22 @netweb
8 years ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.