WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 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 (4)

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 2 years ago.
Refreshed server_name.diff and corrected minor errors
pluggable.php.diff (700 bytes) - added by mt8.biz 5 weeks ago.
25239.2.diff (3.6 KB) - added by jesin 4 weeks ago.
Refreshed 25239.diff

Download all attachments as: .zip

Change History (43)

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

#27811 was marked as a duplicate.

#9 @szepe.viktor
2 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
2 years ago

Refreshed server_name.diff and corrected minor errors

#10 @jesin
2 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
2 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
13 months ago

#33698 was marked as a duplicate.

#13 @SergeyBiryukov
13 months ago

  • Milestone changed from Awaiting Review to 4.4

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


13 months ago

#15 @SergeyBiryukov
13 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.


12 months ago

#17 @wonderboymusic
11 months ago

  • Milestone changed from 4.4 to Future Release

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


7 months ago

#19 @SergeyBiryukov
4 months ago

#16805 was marked as a duplicate.

#20 @ocean90
8 weeks ago

#37548 was marked as a duplicate.

#21 @mensmaximus
6 weeks 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
6 weeks 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
5 weeks 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 5 weeks ago by mt8.biz (previous) (diff)

#24 follow-up: @Grzegorz.Janoszka
5 weeks 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
5 weeks 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
5 weeks 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 5 weeks ago by Grzegorz.Janoszka (previous) (diff)

#27 in reply to: ↑ 26 ; follow-up: @mt8.biz
5 weeks 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
5 weeks 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
5 weeks 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
5 weeks ago

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

Last edited 5 weeks ago by mt8.biz (previous) (diff)

#31 @cbutlerjr
4 weeks 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 weeks ago

Refreshed 25239.diff

#32 follow-up: @dd32
4 weeks ago

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

#33 in reply to: ↑ 32 @Grzegorz.Janoszka
4 weeks 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 weeks ago

#37945 was marked as a duplicate.

#35 @BjornW
3 weeks 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 weeks ago

#37 follow-up: @neodjandre
10 days 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
7 days 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
5 days 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.

Note: See TracTickets for help on using tickets.