Warn users that underscores in (sub)domain names are not valid.

core-require_no_underscores_in_hostname.patch queued for re-testing.

Fixing typo in spelling.

Just come across this myself. That patch would have been really useful if it had been merged in.

IE has problems accepting cookies from subdomain's that dont follow the URI RFC. (http://www.ietf.org/rfc/rfc2396.txt)

+++ b/modules/system/system.install
@@ -486,6 +486,20 @@ function system_requirements($phase) {
+      'description' => $t('Host names should not include an underscore. Whilst most browsers will still function, Internet Explorer will appear to serve the web site, but will silently drop all cookies for the domain, preventing Internet Explorer users from logging in.'),

Can we shorten the text to say just don't support it in Drupal at all without explain in detail.

Please find the patch.

Hi! I found some minor issues. Others can check the logic thoroughly too.

1. +++ b/core/modules/system/system.install
   @@ -23,6 +23,20 @@ function system_requirements($phase) {
   +    // underscore character is not permitted.  Whilst most browsers will still
   ...
   +      'title' => t('Host name'),
   +      'value' => t('Fails'),
   +      'severity' => REQUIREMENT_WARNING,
   +      'description' => t('Host names should not include an underscore. Whilst most browsers will still function, Internet Explorer will appear to serve the web site, but will silently drop all cookies for the domain, preventing Internet Explorer users from logging in.'),
   

   Please replace the double spaces before "Whilst".

2. +++ b/core/modules/system/system.install
   @@ -23,6 +23,20 @@ function system_requirements($phase) {
   +   $requirements['hostname'] = array(
   

   Please fix indentions and its array value declarations.

Thanks!

Re-rolled the #10 patch as per #12. Thank you!

This could use a review, the patch is straightforward just needs to make sure it is correct english and does what is suggested in the issue summary accurately.

Reviewed. It does what is suggested.

Applied patch is working fine

Does filter_var($url, FILTER_VALIDATE_URL) handle underscores in domain names? If so should we use it here?

Also the message should show the hostname I think. In the middle of a long list of requirement notices you don't necessarily want to look up to the address bar and back.

Updated the patch to use the new core array syntax, added the domain name to the message text.

Not sure about the filter_var() thing. While it returns FALSE for domain names containing underlines, what's the advantage here? It also performs checks not related to this issue.

Does filter_var($url, FILTER_VALIDATE_URL) handle underscores in domain names? If so should we use it here?

It does check over underscores. But if we use that here, it might also return false because of an other error in the domain name. That means if we still warn the user about underscores only in the error message when using filter_var(), we might lead the user to a false direction.

+++ b/core/modules/system/system.install
@@ -29,6 +29,20 @@ function system_requirements($phase) {
+    $requirements['hostname'] = array(
+    (...)
+    );

it should be updated to the short array notation [].

Just overlooked this. Changed.

Seems OK.

$_SERVER['HTTP_HOST'] is not assumed in core, and also could be on $_SERVER['SERVER_NAME'] depending on proxy.

Consider checking both values and at least doing an isset()?

This suggests $_SERVER['SERVER_NAME'] being more reliable, but neither being always available.
https://stackoverflow.com/questions/2297403/what-is-the-difference-betwe...

I'm having a look at this one at DC Vienna.

Taking a look with @rakesh_verma at Vienna - edit: can review the patch once rakesh_verma has uploaded it

Considering both $_SERVER['HTTP_HOST'] and $_SERVER['SERVER_NAME'] with an isset. Just as mentioned in comment #35.

Patch fails to apply:

1444718-39.patch:12: tab in indent.
	  (isset($_SERVER['SERVER_NAME']) && strpos($_SERVER['SERVER_NAME'], '_') !== FALSE)) {
error: corrupt patch at line 27

Suggestion: change tabs to spaces for indentation

Fixing the test cases.

Tried applying patch from #41, error:

error: corrupt patch at line 27

Probably some tab/tabs still left, so rerolling the patch for someone else to review.

Also: this issue should probably be tested by a Windows / IE user to verify the fix works.

Patch 43 only aims at removing last remaining tabs, no other changes made.

A bit unsure about the if statement vs Drupal coding standards: as the statement is > 80 characters, it should probably be divided to 2 lines, however might need restructuring to separate statements according to https://www.drupal.org/docs/develop/standards/coding-standards#linelength. I used my best judgement so left it as is for now - hoping at least the patch can now be applied.

patch is working fine.

1. +++ b/core/modules/system/system.install
   @@ -32,6 +32,22 @@ function system_requirements($phase) {
   +  // $_SERVER['HTTP_HOST'] is not assumed in core and could be on $_SERVER['SERVER_NAME']. Henceforth checking both of them.
   

   Comment needs to wrap at 80 chars.

2. +++ b/core/modules/system/system.install
   @@ -32,6 +32,22 @@ function system_requirements($phase) {
   +  if ((isset($_SERVER['HTTP_HOST']) && strpos($_SERVER['HTTP_HOST'], '_') !== FALSE) || (isset($_SERVER['SERVER_NAME']) && strpos($_SERVER['SERVER_NAME'], '_') !== FALSE)) {
   ...
   +      'description' => t('Host names should not include an underscore, like in %domain. Whilst most browsers will still function, Internet Explorer will appear to serve the web site, but will silently drop all cookies for the domain, preventing Internet Explorer users from logging in.', ['%domain' => $_SERVER['HTTP_HOST']]),
   

   These should use the \Drupal::request()->getHttpHost() which handles all this juggling as well as validation.

I agree around the usage of wrapper function getHttpHost on Request object provided by HttpFoundation rather than relying on $_SERVER. Attaching an updated patch.

Was making some tweaks to drupalci to use hostnames, and the hostnames I created arbitrarily had underscores in them. (things like http://php_apache_local_41f5fbd85887179e9e38d7234c3ce954).

Anyhow, took me about half a day to figure out what the problem was, so I look forward to something like this patch being louder about invalid configurations.

1. +++ b/core/modules/system/system.install
   @@ -32,6 +32,24 @@ function system_requirements($phase) {
   +  $http_host = \Drupal::request()->getHttpHost();
   

   Is there a reason to use getHttpHost() over getHost()? I don't think there is a reason for having the port appended in this instance.

2. +++ b/core/modules/system/system.install
   @@ -32,6 +32,24 @@ function system_requirements($phase) {
   +  if (strpos($http_host, '_') !== FALSE || (isset($_SERVER['SERVER_NAME']) && strpos($_SERVER['SERVER_NAME'], '_') !== FALSE)) {
   

   There is no need to handle $_SERVER['SERVER_NAME'] here. All you need is: if (strpos($http_host, '_') !== FALSE) {

   See \Symfony\Component\HttpFoundation\Request::getHost().

One thing that is interesting is that the Symfony code allows underscores see...

        // as the host can come from the user (HTTP_HOST and depending on the configuration, SERVER_NAME too can come from the user)
        // check that it does not contain forbidden characters (see RFC 952 and RFC 2181)
        // use preg_replace() instead of preg_match() to prevent DoS attacks with long host names
        if ($host && '' !== preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)) {
            throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host));
        }

Psy Shell v0.8.0 (PHP 7.2.0RC2 — cli) by Justin Hileman
>>> $host = 'blah_ddgfd';
=> "blah_ddgfd"
>>> preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)
=> ""

There is an interesting debate on https://github.com/symfony/symfony/issues/10467 where it is proposed to allows underscores in URIs... so what's super fun is that whilst getHost() allows underscores... \Symfony\Component\Validator\Constraints\UrlValidator::validate() doesn't. Seems like everyone including M$ are confused on whether underscores should or should not be supported.

Closing this as outdated since the issue seems to be centered around IE which is a no longer supported browser