Ticket #2053 (closed defect: fixed)

Opened 2 years ago

Last modified 10 months ago

Emails being sent with bogus "From:" header

Reported by: sjmurdoch Assigned to: rob1n
Priority: normal Milestone: 2.2
Component: General Version: 2.0
Severity: major Keywords: has-patch commit
Cc: sjmurdoch

Description

The function wp_mail() in wp-includes/pluggable-functions.php sets the "From:" address of emails sent to be wordpress@SERVER_NAME with 'www' stripped, unless the caller overrides this. This address is not valid on my system so email is being dropped by the local server. It will also be dropped by the receiving system if that mailserver blocks email with invalid from headers, which is a common spam prevention technique.

This behaviour is due to [3214]; previously it used get_settings('admin_email'). In #1532 markjaquith says that get_settings('admin_email') is not used "because admin email might be from off-domain and might be more likely to get flagged as spam at its destination".

A consistent "From:" header is not used. Some places use the wp_mail() default, one overrides it with the same string, another does so without stripping the 'www', and one still uses get_settings('admin_email'). None of these will be correct on all systems.

wp_mail() uses the default, invalid From header in the following cases:

  • wp-includes/pluggable-functions.php, line 332
  • wp-includes/pluggable-functions.php, line 349
  • wp-includes/pluggable-functions.php, line 358
  • wp-login.php, line 113
  • wp-login.php, line 145
  • wp-login.php, line 156

wp_mail() is overridden with an invalid From header in the following cases:

  • wp-includes/pluggable-functions.php, line 290 (wordpress@SERVER_NAME, with 'www' stripped)
  • wp-admin/install.php, line 201 (wordpress@SERVER_NAME, no 'www' stripping)

wp_mail() is overridden with potentially off-domain get_settings('admin_email') in the following case:

  • wp-content/plugins/wp-db-backup.php, line 663

The "From:" header should be set in one place, to prevent the inconsistencies above. This would allow it to be manually set in the cases where "wordpress@SERVER_NAME" is invalid, like mine. Preferably this should be displayed as an option in the web based configuration tool.

Attachments

mailfrom.patch (2.7 kB) - added by sjmurdoch on 01/24/06 22:38:28.
Untested hack to work-around this bug
2053.diff (0.6 kB) - added by rob1n on 03/18/07 01:31:20.
Use default (wordpress@domain), but apply_filters in case someone wants to change that.

Change History

12/11/05 11:16:05 changed by markjaquith

Great report... thanks.

No reason we can't both set the FROM address in one place (there should be a filter, so plugins can change it), and also make WP pick a default one more intelligently, so that fewer people like you are put in the situation of having to change it.

SERVER_NAME isn't working for you... would HTTP_HOST be more reliable, you think? ("www." stripped, of course). What about people who have blog.domain.com ... is it possible that blog.domain.com might not have a valid MX entry?

12/11/05 12:52:04 changed by sjmurdoch

My case is essentially blog.domain.com and wordpress@blog.domain.com is not a valid email address. Stripping off 'blog' would yield a valid domain but this would obviously break in cases where wordpress@blog.domain.com is the corrrect choice, which is also a plausible situation. I don't think there is any hope to reliably guess what a good "From:" header so I think it will have to be left up to the user.

Putting the defaults in one piece of code would make me happy. Then I would only need hack it in 1 place rather than 4. I don't know enough about the Wordpress internals to make a proper fix, with options and filters.

As for the default, I don't have any strong opinions. My case is fairly unusual and SERVER_NAME presumably can't be working too badly, otherwise there would be more complaints. However, my guess for the best "From:" header default woudld be use get_settings('admin_email').

This is because in my experience, the sending MTA is more likely to check that the domain is not known to be invalid than it is to check that it is local. In my case, if a non-local "From:" header is used, the MTA (exim) will simply add a "Sender:" header, but still accept the email. The receiving MTA may additionally check that mail will be accepted to the address in the "From:" header, so this requires ensuring the local part is correct too. In my case wordpress@ anything is not going to work as I cannot create arbitrary email addresses. Using get_settings('admin_email') seems like the best way to ensure that the email address has a valid MX record and will accept mail.

12/12/05 12:30:34 changed by sjmurdoch

I asked someone who knows far more about email than me and he agreed that there is no reliable way to generate a valid "From:" header. His suggestion was to ask for this during the setup process, but give a few options, for example wordpress@SERVER_NAME, get_settings('admin_email'), or custom.

As for a default, he suggested that using get_settings('admin_email') is the one most likely to get mail through, as it is the only one that is guaranteed to be a valid email address.

01/04/06 19:41:41 changed by kri

I think it would be best if things were set so that those who had different server configurations didn't have to hack into Wordpress to receive E-mail notifications. It should either be returned to get_settings('admin_email'), because that seems to work for more people (or maybe it doesn't, I don't know), or there should be a place where one has to set an E-mail for notifications to be sent to it.

01/24/06 22:38:28 changed by sjmurdoch

  • attachment mailfrom.patch added.

Untested hack to work-around this bug

01/24/06 22:43:07 changed by sjmurdoch

I didn't see any progress on this bug and I need a work around, so I have written a quick hack which does what I need. The patch defines a function wp_default_mailfrom() which returns get_settings('admin_email'). It then uses this for the From: address in the places identified in the original report.

This is not the right way to do things, and it is not properly tested, but I am posting it here in case there is anyone in a similar position to me, in need of an interim solution.

03/13/07 03:59:35 changed by rob1n

  • keywords set to dev-feedback.
  • owner changed from anonymous to rob1n.
  • status changed from new to assigned.

Patch is now invalid, and I'll gladly make a new one. Devs' opinions?

03/13/07 04:03:25 changed by ryan

Just apply_filters on the email and default to what we currently use, not admin_email. admin_email caused all kinds of pain.

03/13/07 04:05:00 changed by rob1n

  • keywords deleted.

Okay.

03/17/07 02:37:21 changed by rob1n

  • milestone set to 2.2.

03/18/07 01:31:20 changed by rob1n

  • attachment 2053.diff added.

Use default (wordpress@domain), but apply_filters in case someone wants to change that.

03/18/07 01:32:08 changed by rob1n

  • keywords set to has-patch commit.

03/18/07 23:16:16 changed by ryan

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [5061]) Add wp_mail_from filter. Props rob1n. fixes #2053

(follow-up: ↓ 13 ) 07/20/07 16:56:46 changed by Otto42

  • status changed from closed to reopened.
  • version changed from 2.0 to 2.2.1.
  • resolution deleted.
  • milestone changed from 2.2 to 2.3 (trunk).

The pluggable.php contains this code in wp_notify_postauthor():

$wp_email = 'wordpress@' . preg_replace('#^www\.#', '', strtolower($_SERVER['SERVER_NAME']));

This is done because it builds its own headers and sends them to wp_mail. The end result of this is that the wp_mail_from filter is not applied in this case, because $headers != .

Suggest changing the code above in wp_notify_postauthor() to this:

$wp_email = apply_filters('wp_mail_from', "wordpress@" . preg_replace('#^www\.#', '', strtolower($_SERVER['SERVER_NAME'])));

In order to apply the same wp_mail_from filter to these email addresses as well.

(in reply to: ↑ 12 ) 07/20/07 20:07:52 changed by westi

  • status changed from reopened to closed.
  • version changed from 2.2.1 to 2.0.
  • resolution set to fixed.
  • milestone changed from 2.3 (trunk) to 2.2.

Otto42 please don't reopen tickets when the fix has made it into a released version.

We loose the history of what was fixed when then.

I have opened a new ticket for this issue #4658