WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 days ago

#25239 reviewing defect (bug)

$_SERVER['SERVER_NAME'] not a reliable when generating email host names

Reported by: layotte Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.8
Component: Mail Keywords: has-patch dev-feedback needs-testing
Focuses: Cc:

Description (last modified by SergeyBiryukov)

For quite some time I have been having an issue with my comment notifications. The From address has been wordpress@_. I haven't paid much attention to this, but I had some spare time and decided to pursue the issue. Here it is...

I am running WPMS w/ domains (latest stable) with Nginx and PHP5-FPM. In WordPress the comment notifications from email addresses are being generated using $_SERVER['SERVER_NAME'] to get the current site's domain name.

e.g.

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

However, because of my environment my Nginx config for my site has the server_name set to "_" (underscore). Which is a catchall -- http://nginx.org/en/docs/http/server_names.html.

Site config in Nginx:

# Redirect everything to the main site.
server {
        listen [::]:80 default_server;
        listen [::]:443 ssl;

        ssl_certificate         /etc/nginx/ssl.crt/xxx.com.2012.crt;
        ssl_certificate_key     /etc/nginx/ssl.key/xxx.com.2012.key;

        server_name _;
        root /var/www/xxx.com;
        access_log /var/log/nginx/xxx.com.access.log;
        error_log /var/log/nginx/xxx.com.error.log;
        client_max_body_size 100m;

        if ($http_host = "www.xxx.com") {
                rewrite ^ http://xxx.com$request_uri permanent;
        }

        include global/restrictions.conf;

        # Additional rules go here.

        include global/wordpress-ms.conf;

}

The default fastcgi_params has this set:

fastcgi_param   SERVER_NAME             $server_name;

Thus, $_SERVER['SERVER_NAME'] is outputting "_" (underscore).

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain). I have implemented this change on my own site and have included the patch here.

In the meantime, anyone else facing this problem can change their fastcgi_param to $host instead of $server_name. In my opinion, not the best solution, but it works for now.

Attachments (8)

server_name.diff (4.3 KB) - added by layotte 3 years ago.
Replace $_SERVERSERVER_NAME? w/ $current_site->domain
25239.diff (3.4 KB) - added by jesin 3 years ago.
Refreshed server_name.diff and corrected minor errors
pluggable.php.diff (700 bytes) - added by mt8.biz 4 months ago.
25239.2.diff (3.6 KB) - added by jesin 4 months ago.
Refreshed 25239.diff
25239.3.patch (3.5 KB) - added by desmith 2 months ago.
25239.4.patch (3.7 KB) - added by desmith 2 months ago.
25239.5.patch (3.9 KB) - added by desmith 2 months ago.
25239.6.patch (4.1 KB) - added by desmith 8 weeks ago.

Download all attachments as: .zip

Change History (64)

@layotte
3 years ago

Replace $_SERVERSERVER_NAME? w/ $current_site->domain

#1 @SergeyBiryukov
3 years ago

  • Description modified (diff)

Related: #16805

#2 @nacin
3 years ago

nginx has an odd quirk that, given:

server_name example.com blog.example.com microsite.example.com;

And:

fastcgi_param SERVER_NAME $server_name;

When accessing microsite.example.com, SERVER_NAME will be set to example.com. This is akin to always using ServerName in Apache, rather than using ServerAlias (which is what happens).

I agree that SERVER_NAME should not be used, as depending on the setup it is not going to match HTTP_HOST. That said, if you are using a catchall like _ (it is actually meaningless, any special character will do), you should consider doing something like set $name X in your server block to ensure $server_name is what you want it to be, or actuall change the fastcgi_param call to set SERVER_NAME to $host. That will fix your issues.

So, this isn't a duplicate of #16805 — rather, we should investigate and consider a switch to HTTP_HOST here.

#3 @layotte
3 years ago

Would HTTP_HOST be better than $current_site->domain?

Last edited 3 years ago by layotte (previous) (diff)

#4 @cmmarslender
3 years ago

  • Cc cmmarslender added

#5 @3flex
3 years ago

  • Cc 3flex added

This ticket was mentioned in IRC in #wordpress-dev by markoheijnen. View the logs.


3 years ago

#7 in reply to: ↑ description @sreedoap
3 years ago

I wanted to note my use case for this which caused me to run into this issue. I am calling wp_install from a command line script I made which is ran for our users who launch a WordPress site, so HTTP_HOST and SERVER_NAME were not set for me. I manually set it in my script and that fixed my issue but it would probably be wise for the wp_mail function to have a fallback if it is unable to get a valid value from HTTP_HOST or SERVER_NAME so emails don't appear to come from an "unknown sender".

Replying to layotte:

For quite some time I have been having an issue with my comment notifications. The From address has been wordpress@_. I haven't paid much attention to this, but I had some spare time and decided to pursue the issue. Here it is...

I am running WPMS w/ domains (latest stable) with Nginx and PHP5-FPM. In WordPress the comment notifications from email addresses are being generated using $_SERVER['SERVER_NAME'] to get the current site's domain name.

e.g.

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

However, because of my environment my Nginx config for my site has the server_name set to "_" (underscore). Which is a catchall -- http://nginx.org/en/docs/http/server_names.html.

Site config in Nginx:

# Redirect everything to the main site.
server {
        listen [::]:80 default_server;
        listen [::]:443 ssl;

        ssl_certificate         /etc/nginx/ssl.crt/xxx.com.2012.crt;
        ssl_certificate_key     /etc/nginx/ssl.key/xxx.com.2012.key;

        server_name _;
        root /var/www/xxx.com;
        access_log /var/log/nginx/xxx.com.access.log;
        error_log /var/log/nginx/xxx.com.error.log;
        client_max_body_size 100m;

        if ($http_host = "www.xxx.com") {
                rewrite ^ http://xxx.com$request_uri permanent;
        }

        include global/restrictions.conf;

        # Additional rules go here.

        include global/wordpress-ms.conf;

}

The default fastcgi_params has this set:

fastcgi_param   SERVER_NAME             $server_name;

Thus, $_SERVER['SERVER_NAME'] is outputting "_" (underscore).

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain). I have implemented this change on my own site and have included the patch here.

In the meantime, anyone else facing this problem can change their fastcgi_param to $host instead of $server_name. In my opinion, not the best solution, but it works for now.

#8 @SergeyBiryukov
3 years ago

#27811 was marked as a duplicate.

#9 @szepe.viktor
3 years ago

Automatic updater sends an email but SERVER_NAME is not defined when wp-cron starts as a real Linux cron job.

Please uset get_option('site_url') as a fallback to determine the hostname.

@jesin
3 years ago

Refreshed server_name.diff and corrected minor errors

#10 @jesin
3 years ago

Tested 25239.diff on a multisite Nginx environment with the following code.

add_filter( 'pre_site_option_admin_email', function() {
	return '';
});
wpmu_signup_user_notification( 'jesin', 'my@email.com', 'abcd' );
wpmu_welcome_notification( 2, 1, 'password', 'Hello World' );
wpmu_welcome_user_notification( 1, 'password' );
wpmu_signup_blog_notification( 'example.com', '/', 'Hello World', 'jesin', 'my@email.com', 'abcd' );
wp_mail( 'my@email.com', 'Subject', 'A message without From: header' );

This patch uses home_url() for single site and $current_blog->domain for multisite inside functions wp_mail() and wp_notify_postauthor() because home_url() returns the subdomain even if domain mapping is configured.

#11 in reply to: ↑ description @kitchin
3 years ago

Replying to layotte:

I propose we move away from $_SERVER['SERVER_NAME'] when generating the from email addresses and use the available $current_site global object which stores the domain variable ($current_site->domain).

I suggest also moving away from $_SERVER['HTTP_HOST']. At first glance, HTTP_HOST seems worse because it's more closely derived from user input, but actually both can be buggy depending on the web server. Apache has had bugs. See http://stackoverflow.com/questions/2297403/http-host-vs-server-name for a pretty detailed write-up of SERVER_NAME vs. HTTP_HOST.

#12 @SergeyBiryukov
15 months ago

#33698 was marked as a duplicate.

#13 @SergeyBiryukov
15 months ago

  • Milestone changed from Awaiting Review to 4.4

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


15 months ago

#15 @SergeyBiryukov
15 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


14 months ago

#17 @wonderboymusic
13 months ago

  • Milestone changed from 4.4 to Future Release

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


9 months ago

#19 @SergeyBiryukov
7 months ago

#16805 was marked as a duplicate.

#20 @ocean90
4 months ago

#37548 was marked as a duplicate.

#21 @mensmaximus
4 months ago

Can we fix this for 4.7 please?

There are several places in WordPress where we rely on $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST']. While I personally never trust in $_SERVER['SERVER_NAME'] I understand some feel uncomfortable with $_SERVER['HTTP_HOST'] either.

Given the fact that home_url and blog_id are always set and can be trusted, introducing a new function might be a solution:

function get_current_domain() {
        return parse_url( get_home_url( get_current_blog_id() ), PHP_URL_HOST );
}

The function name follows the syntax from other get_ functions in WordPress. It's name says what it does. It relies on WordPress core functions and settings that always exist. It works with PHP down to 4.0.

So instead of using $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] you could use get_current_domain().

#22 @mensmaximus
4 months ago

To make the function more flexible and to strip the subdomain for email addresses we could pass some arguments:

function get_current_domain( $strip_subdomain = true, $blog_id = '' ) {
        $blog_id = empty( $blog_id ) ? get_current_blog_id() : $blog_id;
        $domain = parse_url( get_home_url( $blog_id ), PHP_URL_HOST );
        if ( true === $strip_subdomain ) {
                $domain_parts = explode ( ".", $domain );
                array_shift( $domain_parts );
                $domain = implode( '.', $domain_parts );
        }
        return $domain;
}

#23 @mt8.biz
4 months ago

From WordPress4.6, wp_mail uses phpmailer->setfrom, ”wordpress@_” is always results error.

https://core.trac.wordpress.org/changeset/38287

error message is:

Fatal error: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@_' in /var/www/vhosts/i-XXXXXXXX/wp-includes/class-phpmailer.php:946
Stack trace:
#0 /var/www/wordpress/wp-includes/pluggable.php(352): PHPMailer->setFrom('wordpress@_', 'WordPress')
#1 /var/www/wordpress/wp-includes/pluggable.php(1726): wp_mail('hogehoge@...', '[SITE NAME]')
#2 /var/www/wordpress/wp-includes/user.php(2350): wp_new_user_notification(35, NULL, 'both')
#3 [internal function]: wp_send_new_user_notifications(35, 'both')
#4 /var/www/wordpress/wp-includes/plugin.php(524): call_user_func_array('wp_send_new_use...', Array)
#5 /var/www/wordpress/wp-admin/includes/user.php(196): do_action('edit_user_creat...', 35, 'both')
#6 /var/www/wordpress/wp-admin/user-new.php(116): edit_user()
#7 {main} thrown in /var/www/wordpress/wp-includes/class-phpmailer.php on line 946

Last edited 4 months ago by mt8.biz (previous) (diff)

#24 follow-up: @Grzegorz.Janoszka
4 months ago

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

#25 in reply to: ↑ 24 ; follow-up: @mt8.biz
4 months ago

Replying to Grzegorz.Janoszka:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

#26 in reply to: ↑ 25 ; follow-up: @Grzegorz.Janoszka
4 months ago

Replying to mt8.biz:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

Wordpress + nginx + system cron sending emails.

Not sure if nginx is required, when you use system cron the variables coming from http daemon will not be set. How do you expect to get anything from $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] when you just call the php script from cron (php wp-cron.php)?

Last edited 4 months ago by Grzegorz.Janoszka (previous) (diff)

#27 in reply to: ↑ 26 ; follow-up: @mt8.biz
4 months ago

Replying to Grzegorz.Janoszka:

Replying to mt8.biz:

As mentioned earlier - it completely doesn't work when system cron is used. Please fix it.

How to reproduce?

Wordpress + nginx + system cron sending emails.

Not sure if nginx is required, when you use system cron the variables coming from http daemon will not be set. How do you expect to get anything from $_SERVER['SERVER_NAME'] or $_SERVER['HTTP_HOST'] when you just call the php script from cron (php wp-cron.php)?

Should use the DB settings instead of using the $ _SEREVER I think

#28 in reply to: ↑ 27 ; follow-up: @mensmaximus
4 months ago

Replying to mt8.biz:

Should use the DB settings instead of using the $ _SEREVER I think

I already posted a suggestion to use the home_url: https://core.trac.wordpress.org/ticket/25239#comment:22

#29 in reply to: ↑ 28 @mt8.biz
4 months ago

Replying to mensmaximus:

Replying to mt8.biz:

Should use the DB settings instead of using the $ _SEREVER I think

I already posted a suggestion to use the home_url: https://core.trac.wordpress.org/ticket/25239#comment:22

yes. I khow.

#30 @mt8.biz
4 months ago

This is a fatal problem from ver4.6. I think it should be fix quick.

Last edited 4 months ago by mt8.biz (previous) (diff)

#31 @cbutlerjr
4 months ago

@jesin's patch is a little more thorough in addressing the issue - it also covers the wp_notify_postauthor() function as well as ms-functions.php. But it probably needs a refresh.
https://core.trac.wordpress.org/attachment/ticket/25239/25239.diff

@jesin
4 months ago

Refreshed 25239.diff

#32 follow-up: @dd32
3 months ago

An issue affecting 4.6 installs is fixed in 4.6.1, see #37736

#33 in reply to: ↑ 32 @Grzegorz.Janoszka
3 months ago

Replying to dd32:

An issue affecting 4.6 installs is fixed in 4.6.1, see #37736

dd32: the one we are discussing here is completely different and unrelated. It is about using HTTP variables when they are not available (like when calling wp-cron script from system cron.

#34 @ocean90
3 months ago

#37945 was marked as a duplicate.

#35 @BjornW
3 months ago

@ocean90 & @dd32 is there a reason for not accepting @jesin's patch?

This ticket was mentioned in Slack in #cli by danielbachhuber. View the logs.


3 months ago

#37 follow-up: @neodjandre
3 months ago

I am facing the same problem using:

Nginx + Postfix + Real Cron Jobs

I am receiving these errors:

Error message
phpmailerException: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@' in /var/www/html/wp-includes/class-phpmailer.php:946

Stack trace
in PHPMailer::setFrom called at /var/www/html/wp-includes/pluggable.php (352)
in wp_mail called at /var/www/html/wp-includes/user.php (1841)
in wp_update_user called at /var/www/html/wp-admin/includes/user.php (182)
in edit_user called at /var/www/html/wp-admin/user-edit.php (147)

#38 in reply to: ↑ 37 @BjornW
3 months ago

In my case I've fixed this with this small plugin. It uses the admin user's email address as a From address thus enforcing a working email address. For now, this is a workable solution for me at least until @jesin's patch or someone else's is accepted in WordPress. Hope this helps.

Replying to neodjandre:

I am facing the same problem using:

Nginx + Postfix + Real Cron Jobs

I am receiving these errors:

Error message
phpmailerException: Uncaught exception 'phpmailerException' with message 'Invalid address: wordpress@' in /var/www/html/wp-includes/class-phpmailer.php:946

Stack trace
in PHPMailer::setFrom called at /var/www/html/wp-includes/pluggable.php (352)
in wp_mail called at /var/www/html/wp-includes/user.php (1841)
in wp_update_user called at /var/www/html/wp-admin/includes/user.php (182)
in edit_user called at /var/www/html/wp-admin/user-edit.php (147)

#39 @Ipstenu
3 months ago

FYI, since we changed how phpMailer validates emails, this has caused a previously silent error in wp-cli to bomb out.

https://github.com/wp-cli/wp-cli/issues/3374

the tl;dr version is that because the email is being passed as wordpress@ without a domain (since php CLI can't grab that variable), and phpmailer now flags that as an error, no one can reset passwords on command line unless they use the --url parameter to force the domain in.

The change in 4.6 (and 4.6.1) highlights a problem that had been flying under the radar for a long time, I think.

#40 @ocean90
3 months ago

#38161 was marked as a duplicate.

#41 @danielbachhuber
3 months ago

At the very least, could we get a try... catch around the thrown Exception?

#42 @Ipstenu
3 months ago

From #38161

Using something like substr( home_url( '', 'http'),7) might be a suitable substitute when $_SERVER is unavailable. (If there's a better way to get a site's hostname than slicing up home_url() that of course would work too.)

We'd want to use parse_url() I think, since https is everywhere. Also some people use WWW and some don't :/

@desmith
2 months ago

#43 @desmith
2 months ago

This patch combines @jesin's "replace it in a bunch of spots" and @mensmaximus's "add a new function for this" techniques. I omitted the parameters for get_current_domain() because there's only one current domain at any given time. First patch, probably did everything wrong, et cetera.

#44 @desmith
2 months ago

Please ignore the above patch (which doesn't fix all the cases of this that I've encountered), in lieu of another one coming shortly. Apologies.

#45 @desmith
2 months ago

Okay, let's try this again.

This patch adds a new function in pluggable.php, get_current_domain(). Does what it says on the tin, returns the current blog domain name, with leading 'www.' stripped. It also modifies a few places where emails are sent, to use that function instead of $_SERVER['SERVER_NAME'], notably in the password reset email-send, which fixes the issue with wp-cli password reset emails.

There is a possible side effect. Since I replaced $_SERVER['SERVER_NAME'] in several email-related places, anyone that's depending on their emails' From: headers looking like support@… instead of support@theirsite will be disappointed. I could un-patch those instances, of course, but I think this behavior is cleaner overall.

@desmith
2 months ago

#46 follow-up: @joemcgill
2 months ago

Replacing $_SERVER['SERVER_NAME'] with output from a helper function in these instances seems like a good idea to me and could be useful outside of instances where we're building email addresses. I question whether this should be a pluggable function, though. This function feels more like get_site_url() or get_home_url() which live in wp-includes/link-template.php.

I also wonder if it was an oversight that none of the multisite functions were stripping www subdomain prefixes or if that was intentionally allowed? Either way, 25239.4.patch has a redundant preg_replace() call if that method is used inside the helper function.

#47 in reply to: ↑ 46 @jdgrimes
2 months ago

Replying to joemcgill:

I question whether this should be a pluggable function, though.

Yeah, I think that the consensus is that pluggable functions were a mistake, and filter and action hooks should be used to provide overridability/customizability. So no new pluggable functions are being added, as far as I know.

So the function should be moved to a different file as you've suggested, and we should consider the merits of adding a filter on the value that it returns.

@desmith
2 months ago

#48 follow-up: @desmith
2 months ago

As suggested, I've moved the new function to a more-appropriate place, and added an apply_filters call.

@joemcgill , you were right in that I overlooked a preg_replace call; thanks for the catch.

The question of whether we should remove leading 'www.' in names here is still open. It is a change, and while I'd argue it's a change for the better, it's a pretty weak argument.

#49 in reply to: ↑ 48 @mensmaximus
2 months ago

Replying to desmith:

The question of whether we should remove leading 'www.' in names here is still open. It is a change, and while I'd argue it's a change for the better, it's a pretty weak argument.

Please ounce again take a look at my suggestion: https://core.trac.wordpress.org/ticket/25239#comment:22

As you can see I have addressed the subdomain issue by passing a parameter to the function. I also passed a blog id parameter to make the function more universal.

#50 @Ipstenu
2 months ago

@joemcgill - Probably an oversight or an 'It didn't break, leave it alone.' which actually applies to this change as a whole ;)

Leading www's should be removed when we're talking about _email_ however. You don't email support@… after all. :) (Hopefully a better argument since part of the point of this ticket is making VALID email address)

#51 @joemcgill
2 months ago

@Ipstenu thanks for pointing out that those are for the From: emails. Even so, your point about "leave it alone" stands, so I wonder if we should only use home_url() here as a fallback whenever $_SERVER['SERVER_NAME'] isn't set—which is the reason for this ticket in the first place.

@mensmaximus Thanks for the reminder about your approach. If I understand your thinking, making this function more flexible would also allow it to be used instead of $_SERVER['HTTP_HOST'] as well, correct? If so, I worry about there becoming overlap/confusion between this function and already existing functions, like get_site_url(), get_home_url(), etc.

Is there a needed use case for a function that does any more than checks to see if $_SERVER['SERVER_NAME'] is set, and if not, returns home_url() (with a www subdomain stripped, if applicable)? If not, it may be better to keep the scope of this narrowly defined as a wrapper around $_SERVER['SERVER_NAME'] and to reinforce its purpose in the function name—so maybe something like wp_server_name() or get_server_name() instead.

@desmith
8 weeks ago

#52 @desmith
8 weeks ago

Let's reduce the scope a bit, then. Latest version just has a function called get_email_domain() that returns the domain name, leading www. stripped, and that's it. Function name is a bit more clear, at least. All the places it's being used are related to email, so the function name and its use make a bit more sense.

If we want to add an if{} to only act if $_SERVER['SERVER_NAME'] is unset, easy enough to do, but I'm not sure it's necessary. Thoughts/opinions?

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


3 weeks ago

#54 follow-up: @riasat
2 weeks ago

WP v4.6.1

My nginx conf server block:

server {
	listen         80;
	server_name    ~^(?<subdomain>.+?)\.(?<domain>.+)$;
	root  /var/www/$subdomain;
	include conf.d/server_block_common.cnf;
}

This is what I always get:

Fatal error:  Uncaught phpmailerException: Invalid address: wordpress@~^(?&lt;subdomain&gt;.+?)\\.(?&lt;domain&gt;.+)$ in 

I know I can fix it by using "wp_mail_from" but I still don't get it why you can not simply default to "wordpress@…" or even just give an OPTION in settings to set the SEND FROM field. It is ridiculous that people have to resort to code if they want to change wordpress portion of the SEND FROM field.

#55 in reply to: ↑ 54 @dd32
2 weeks ago

Replying to riasat:

server {
	listen         80;
	server_name    ~^(?<subdomain>.+?)\.(?<domain>.+)$;
	root  /var/www/$subdomain;
	include conf.d/server_block_common.cnf;
}

This is what I always get:

Fatal error:  Uncaught phpmailerException: Invalid address: wordpress@~^(?&lt;subdomain&gt;.+?)\\.(?&lt;domain&gt;.+)$ in 

@riasat Just focusing on your error here, rather than the ticket - Looks like you've got a error in your nginx config there, although that should work for matching (and appears to be), the matched name isn't be passed to PHP, instead it's passing the literal regex through.
You need to correct the value being passed to PHP through fastcgi_param SERVER_NAME ... in your nginx config, something like http://serverfault.com/questions/650560/nginx-regex-vhost-pattern-ends-up-as-php-server-name should set your on your way.

#56 @derekakelly
8 days ago

I am also getting this bug, although it's only happening on my "add-on domain". My main website also uses the same configuration but does not have this error. I am on a Cloud VPS hosting account. just thought it was odd so I'd point it out. This is my cronjob command

/usr/local/php56/bin/php-cli /home/xxxxxx/public_html/xxxxxxxx.com/wp-cron.php

This is also a new error, up until a week or two ago, i'd never seen it before.

Thanks!

Note: See TracTickets for help on using tickets.