WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 8 years ago

#2053 closed defect (bug) (fixed)

Emails being sent with bogus "From:" header

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

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 (2)

mailfrom.patch (2.7 KB) - added by sjmurdoch 9 years ago.
Untested hack to work-around this bug
2053.diff (609 bytes) - added by rob1n 8 years ago.
Use default (wordpress@domain), but apply_filters in case someone wants to change that.

Download all attachments as: .zip

Change History (15)

comment:1 @markjaquith10 years ago

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?

comment:2 @sjmurdoch10 years ago

My case is essentially blog.domain.com and wordpress@… is not a valid email address. Stripping off 'blog' would yield a valid domain but this would obviously break in cases where wordpress@… 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.

comment:3 @sjmurdoch10 years ago

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.

comment:4 @kri9 years ago

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.

@sjmurdoch9 years ago

Untested hack to work-around this bug

comment:5 @sjmurdoch9 years ago

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.

comment:6 @rob1n8 years ago

  • Keywords dev-feedback added
  • 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?

comment:7 @ryan8 years ago

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

comment:8 @rob1n8 years ago

  • Keywords dev-feedback removed

Okay.

comment:9 @rob1n8 years ago

  • Milestone set to 2.2

@rob1n8 years ago

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

comment:10 @rob1n8 years ago

  • Keywords has-patch commit added

comment:11 @ryan8 years ago

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

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

comment:12 follow-up: @Otto428 years ago

  • Milestone changed from 2.2 to 2.3 (trunk)
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 2.0 to 2.2.1

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.

comment:13 in reply to: ↑ 12 @westi8 years ago

  • Milestone changed from 2.3 (trunk) to 2.2
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from 2.2.1 to 2.0

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

Note: See TracTickets for help on using tickets.