Drupal 5.14 & Drupal 6.8 will not load on clients that do not transmit HTTP_HOST

Works great for me, thanks. This patch also applies cleanly to Drupal 6.8.

It also fixes #345887: Drush: Bootstrap failed.

SUbscribing,greetings, Martijn

Works for me as well.
After upgrading to 5.14 anything running from pp-cli that bootstraped Drupal would die instantly without any error messages.
After applying the patch all is well in Drupal Bootstrap land again!
Thanks for the patch!

Since this is valid in Drupal 6, moving to the most recent version first.

Also, please re-roll the patch with curly braces and true capitalised - see http://drupal.org/coding-standards for more.

As an alternative solution, the final + in the regex can be changed to a *:

-  return preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)+$/', $_SERVER['HTTP_HOST']);
+  return preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)*$/', $_SERVER['HTTP_HOST']);

Also, as was pointed out, an empty HTTP_HOST isn't technically valid, so maybe drupal_valid_http_host() shouldn't worry about this, instead:

-  if (!drupal_valid_http_host()) {
+  if (!drupal_valid_http_host() && isset($_SERVER['HTTP_HOST'])) {

Sure that change is reasonable, or we could do:

return empty($_SERVER['HTTP_HOST']) || preg_match('/^\[?(?:[a-z0-9-:\]_]+\.?)+$/', $_SERVER['HTTP_HOST']);

Agreed with #8, an empty HTTP_HOST is not valid, so we should not say it is. As far as I know this code is in the 7.x codebase already, so we need to have a fix there as well.

An empty HTTP_HOST is valid HTTP/1.0. When Drupal receives a request with an empty HTTP_HOST, it means that the webserver already decided to serve it using its default virtual host. We should serve those requests using the default website.

pwolanin#9 is what makes the most sense to me, and is the only suggested solution that is E_ALL compliant.

Bumping to 7.x to get a fix there first.

Also reported at #346175: Cron does not run after upgrading 5.12->5.14 (same for 6.6->6.8) which was marked duplicate.

Added a requirement for a test on this condition in 7.x at #324875: SA-2008-067 conf_path() header injection vulnerability and reopened that one.

http://www.research.att.com/~bala/papers/h0vh1.html says "Clearly, since HTTP/1.0 clients will not send Host headers, HTTP/1.1 servers cannot simply reject all messages without them. However, the HTTP/1.1 specification requires that an HTTP/1.1 server must reject any HTTP/1.1 message that does not contain a Host header."

Not sure we should follow the letter of the spec there though, given these reports on some clients missing the Host header anyway.

Apache already does that for us. It replies

HTTP/1.1 400 Bad Request

to any requests pretending to be in HTTP/1.1 but missing the "Host:" header.

Well, reports around this issue remind us that Drupal is obviously not only run through Apache. It is run on other servers and is run from the command line as part of scripting environments. In that case, Drupal either needs to be forgiven on its own or need to enforce rules in itself.

@Gabor: I'm pretty sure that those clients that doesn't transmit the Host headers are in fact HTTP/1.0 clients. Plus, because the webserver already guarantee that HTTP/1.1 requests have an Host header (it's verified on Apache, but other webservers will probably act the same), we just have to accept an empty Host header as valid and serve the default website in that case.

I'm really not sure we need to fill in a default - one can already force the server name by setting it in settings.php as the global $base_url:
http://api.drupal.org/api/function/conf_init/7

Here's a 7.x patch with added test condition.

Just a data point that may be relevant.

I have a few sites upgraded to 6.8 on a multi site install, and cron runs fine.

I use: wget -O - -q -t 1 http://..../cron.php

The cron job runs from the same host that runs the Drupal sites, if that matter.

This is using Apache2, mod_php (PHP 5.2.4). Not CGI nor FCGI, nor do they use Lighttpd or ngnix.

On two separate servers with the same versions of Apache and PHP, but Drupal 5.14, everything runs fine as well. Cron completes its runs.

The site were upgraded by creating a patch from the 5.12 vs. 5.13 and applying it. Then from 5.13 to 5.14, (and a similar method for 6.x) but that should not matter.

@Kbahey - I think the main case for cron runs failing is when using something like drush that does PHP via CLI.

Our drupal.sh script has this:

$_SERVER['HTTP_HOST']       = 'default';

thekevinday: your patches were formatted ok. Now that the issue is in the Drupal 7.x queue, the testing is performed against that version. That might be why there were issues with the automated test.

Most often development of patches is focused on one version at a time. After a patch gets committed to the latest version, it can then be backported to D5 and 6.

Regarding cron -- that's exactly it. Some people run /usr/bin/php $DOCUMENTROOT/cron.php in their crontab.

I'm surprised by the number of exceptions on #19 though. It's possible a bad comment went in, which makes all future tests fail.

Going to send #19 for testing again.

@pwolanin: #19 was not E_ALL safe.

Here is a E_ALL safe version. Also moved the check for drupal_valid_http_host() in drupal_initialize_variables() where it makes a lot more sense (this change will probably not be backported to D6).

@Damien Tournoud - there is a typo in one of the code comments. Also, we should really start waith a 7.x patch that can be used as a direct 6.x backport I think, since this seems fairly urgent to fix in a consistent way.

Also, there is a bit of a logic error, perhaps, in that your patch will change the HTTP_HOST == '0' into HTTP_HOST == ''

I also have the problem

of running cron job manually goes okay

however when using the
crontab it does not fire

i just tested the above patch for drupal 5.14

i do get this in my cron email

PHP Warning: include_once(): Unable to access ./includes/bootstrap.inc in /var/www/vhosts/i-bloggers.com/httpdocs/cron.php on line 9

Warning: include_once(): Unable to access ./includes/bootstrap.inc in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9
PHP Warning: include_once(./includes/bootstrap.inc): failed to open stream: No such file or directory in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9

Warning: include_once(): Failed opening './includes/bootstrap.inc' for inclusion (include_path='.:') in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 9
PHP Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 10

Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/mysite.com/httpdocs/cron.php on line 10

so is it wring to use this as command

/usr/bin/php /var/www/vhosts/example.com/httpdocs/cron.php

as i do get this by email as a response to the cron

Warning: include_once(): Failed opening './includes/bootstrap.inc' for inclusion (include_path='.:') in /var/www/vhosts/example.com/httpdocs/cron.php on line 9
PHP Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/example.com/httpdocs/cron.php on line 10

Fatal error: Call to undefined function drupal_bootstrap() in /var/www/vhosts/example.com/httpdocs/cron.php on line 10

Yes, it is wrong. Please review the INSTALL.txt that comes with Drupal on how to properly set it up.

Any chance of a fix to core so that drush works normally again?

I had this in my sandbox for ages, apparently missed pushing it here.

This is still in HEAD, so needs to be applied there first.

Do we really need to check 'HTTP/1.0' if HTTP_HOST is empty? Isn't that the job of the web server? We only care that it's safe.

I agree with #36 that checking SERVER_PROTOCOL isn't necessary, though it doesn't hurt anything, since earlier bootstrap.inc will set an empty SERVER_PROTOCOL to HTTP/1.0 anyway.

BTW I'd also like to add a reminder that for a number of us, the issue isn't just old HTTP clients -- it's also making it easy to bootstrap Drupal from a script (like `php cron.php`, Drush, or a custom script like this http://www.lullabot.com/articles/quick_and_dirty_cck_imports ). I think this a really valuable use-case, and it's good DX that you don't have to do anything terribly heroic (like faking a web server CGI environment) in order to bootstrap Drupal.

Also, this check isn't consistent with the test, and causes the test to fail.

empty HTTP_HOST is valid Other bootstrap.test 85 BootstrapIPAddressTestCase->testIPAddressHost()

@kbahey, yes, the fix is likely (IMO) to get backported for D6. In the meantime you can use the first patch (in the original post, not the comments).

@grendzy

I am not the one complaining. I have not seen the problem at all on any of the sites I am involved with.

@kbahey, whoops, that was meant for tarvid. Sorry.

An update to Damien's patch:

Latest patch looks better but this comment from the previous patch would be great to carry over to the place where HTTP_HOST is set to empty if not set.

+    // Some pre-HTTP/1.1 clients will not send a Host header. Ensure the key is
+    // defined for E_ALL compliance.

Also, it would be great to accelerate the work on this patch, so that we can ensure it gets into the next Drupal 6 release.

Thanks for your review. An update with the comment retained:

Ok, we polished this issue well enough. Time to pull the trigger.

+1 for backporting this to D5.

I'm happy to pull the trigger, but ... it sounds like this patch allows us to clean-up the CLI scripts in ./scripts a bit? We can leave that for follow-up patches but I figured I'd bring it up.

@Dries, perhaps this would allow $_SERVER['HTTP_HOST'] = 'default'; to be removed from drupal.sh. This doesn't gain much, but it is one less extra thing a dev needs to worry about to bootstrap Drupal in those little one-off scripts.

Committed to CVS HEAD. I'm not sure why I did unset that tag.

Upon further review with Damien, we found out that isset($_SERVER['HTTP_HOST']) is not needed in checking the validity, since the patch already ensures that it is set. (I noticed that isset($_SERVER['HTTP_HOST']) && ... != '' would be !empty(), but looks like a better simplification is to just check for != ''). Otherwise should be good for committing to D6.

So expecting a simple patch against D7 to fix this extra cruft and a backport to D6. I'll commit the D6 patch even if the simple cleanup patch is not hitting D7 before, so let's keep on D6.

Here is a followup for D7, as #48.

A better version. drupal_valid_http_host() is now an clean helper function.

Here's a simple D6 and D5 fix.

If we clean-up D7, why not cleaning-up D6 and D5 too?

ok, same patch for D5

Same patch as #51, with fixed tests.

Committed #54 to 5.x.

Thanks, committed #53 to Drupal 6.

Tested #53 on D6 with drush, works great.

I noticed that isset($_SERVER['HTTP_HOST']) && ... != '' would be !empty()

not quite, "0" (0 as a string) is considered empty, even though it could be a valid (though admittedly strange) value for HTTP_HOST (see comment #27).

I was thinking !empty() since the proceeding preg already checked for the validity of the rest. It validated 1, 2, 3, etc. as valid host names, so why would 0 be an exception? :)

Oh yes, I see. In the current D7 version, if you switch to !empty(), "0" won't hit the regex, but the result will still be TRUE and it won't alter HTTP_HOST.

In D6 (current CVS as of a moment ago), the logic looks a little different... An empty string will pass if (isset($_SERVER['HTTP_HOST'])) {, then go to if (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {. So in D6 NULL is valid, but "" (empty string) is not?

No longer critical.

@grendzy: why would drupal_valid_http_host() allow NULL as valid, when it requires at least one valid char?

In D6 (current CVS as of a moment ago), the logic looks a little different... An empty string will pass if (isset($_SERVER['HTTP_HOST'])) {, then go to if (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {. So in D6 NULL is valid, but "" (empty string) is not?

The empty string is not a valid HTTP host, this is by design.

Hi,

my cron is still not working in 6.9. is this supposed to be fixed in that version or do we still need the patch?

tried via the admin panel and via cli.
thanks,
m

A follow-up to my post immediately above:

per this post: http://drupal.org/node/146551
i was able to get cron to run again.

it ran only after turning off the search module, and when i re-enabled that, it failed to run again.

thanks,
m

It seems very unlikely that your errors are related to this bug. You should probably reduce the number of search items to index per cron run, and if you don't have tens of thousands of nodes on your site, hit the 're-index' button to start a full index again.

As i was able to get cron to run, albeit only with the search module disabled, it probably is not relevant to this thread.
i have opened another issue http://drupal.org/node/361171 specifically for this.
thanks!
m

This went into D6, so shoudl be RTBC for 7.

Hi

I still have the issue that cron doesn't work. With 6.6 I had no problems. At the moment (6.9) I've got poormanscron installed to have regular work done. Also manually calling cron.php works.

Does anybody know a good hint how I can debug? The problem is that I can't call wget or similar tools since my provider only allows php scripts for cron tasks. I guess that the HTTP_HOST is then not set properly. So I like to add some logging mechanism in the cron.php file where I can write some environment variables to a file.

Thanks
Tobias

I probably set the status by accident to another state.

Sorry
Tobias

#57 committed to HEAD. Thanks!

subscribing