WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 29 hours 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 6 days ago.
25239.2.diff (3.6 KB) - added by jesin 40 hours ago.
Refreshed 25239.diff

Download all attachments as: .zip

Change History (37)

@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
12 months ago

#33698 was marked as a duplicate.

#13 @SergeyBiryukov
12 months ago

  • Milestone changed from Awaiting Review to 4.4

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


12 months ago

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


11 months ago

#17 @wonderboymusic
10 months ago

  • Milestone changed from 4.4 to Future Release

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


6 months ago

#19 @SergeyBiryukov
3 months ago

#16805 was marked as a duplicate.

#20 @ocean90
4 weeks ago

#37548 was marked as a duplicate.

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

@mt8.biz
6 days ago

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

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

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

Last edited 6 days ago by mt8.biz (previous) (diff)

#31 @cbutlerjr
42 hours 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
40 hours ago

Refreshed 25239.diff

#32 follow-up: @dd32
34 hours ago

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

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

Note: See TracTickets for help on using tickets.