Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed PHP Notice: Undefined index: HTTP_HOST #17655

Closed
wants to merge 4 commits into from
Closed

Fixed PHP Notice: Undefined index: HTTP_HOST #17655

wants to merge 4 commits into from

Conversation

JannikMichel
Copy link
Contributor

@JannikMichel JannikMichel commented Aug 21, 2017

Pull Request for Issue #10973.

Summary of Changes

Fixed PHP Notice: Undefined index: HTTP_HOST

Testing Instructions

Connect to the Webserver via HTTP 1.0 to port 80.
Commandline:
telnet localhost 80

and in there :
get [...joomlaDirectory]/index.php HTTP/1.0

Expected result

No notices from PHP

Actual result

No notices from PHP

Documentation Changes Required

@Schmidie64
Copy link
Contributor

I have tested this item ✅ successfully on dc5cc1e

@icampus
I've tested this item sucessfully. A little bit complicated but everything works well.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17655.

@bembelimen
Copy link
Contributor

Is there a way to used JUri for this?

@laoneo
Copy link
Member

laoneo commented Aug 22, 2017

I mean the notices will disappear but you generate wrong urls when the HTTP_HOST variable is not set. Probably it woul be safer to throw an exception as we need th variable to generate proper urls. It can become a BC break but at least it will not generate invalid urls.

@Murat75
Copy link

Murat75 commented Aug 22, 2017

OS Darwin
PHP 7.1.4
MySQLi 5.5.5-10.1.22-MariaDB

It work!
@icampus

@ghost
Copy link

ghost commented Aug 22, 2017

@Murat75 can you please mark your Test at Issue Tracker

@JannikMichel
Copy link
Contributor Author

@bembelimen Do you mean to use JUri for the link in mod_wrapper?
@laoneo I agree, when the HTTP_HOST is empty we should throw an exception. There is no use in constructing an invalid URL. This would be a BC break so I will redo this PR for the 4.0-dev branch. What do you think?

@laoneo
Copy link
Member

laoneo commented Aug 22, 2017

@JannikMichel I think it would be a good idea to have it properly fixed in 4.

@roland-d
Copy link
Contributor

@laoneo Ok, we will make a new PR against the 4.0-dev.

@ghost
Copy link

ghost commented Aug 22, 2017

RTC after two successful tests.

@Murat75 i altered your Test.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 22, 2017
@laoneo
Copy link
Member

laoneo commented Aug 22, 2017

Would be not bad to adapt the unit tests then accordingly.

@roland-d
Copy link
Contributor

@laoneo Going to need help in that department.

@laoneo
Copy link
Member

laoneo commented Aug 22, 2017

@roland-d Ping me on glip, when you get stuck.

@bembelimen
Copy link
Contributor

@JannikMichel Yes and perhaps in the Application class, too (not sure if JUri is already available there)

@mbabker mbabker removed the RTC This Pull Request is Ready To Commit label Aug 22, 2017
@roland-d
Copy link
Contributor

@JannikMichel Please don't forget to follow up. Thanks.

@@ -973,6 +973,9 @@ protected function detectRequestUri()
$scheme = 'http://';
}

// Try to acquire the server host, if set.
$host = isset($_SERVER['HTTP_HOST']) ? $_SERVER['HTTP_HOST'] : '';
Copy link
Contributor

@photodude photodude Nov 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is for

$host = isset($_SERVER['HTTP_HOST']) ? $_SERVER['HTTP_HOST'] : "undefined";

I do not think leaving this as an empty value when HTTP_HOST is undefined is apropreate.
(note: I am not sure if it would cause problems with urls the set host to an undefined value)

I'm also not sure if there is a better way to handle the situation of HTTP_HOST being undefined.
(the exceptions approach for J4 is likely the direction things need to go)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about resolving to $_SERVER['SERVER_NAME'], like here: https://github.com/slimphp/Slim/blob/3.9.0/Slim/Http/Uri.php#L176-L181

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both $_SERVER['HTTP_HOST'] and $_SERVER['SERVER_NAME'] can both be empty, then what?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$_SERVER['SERVER_NAME'] would work in my case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@photodude ATM we are relying only on $_SERVER['HTTP_HOST'] so it's a step forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that adding $_SERVER['SERVER_NAME'] could be a step in the right direction. But we need to properly handle the case of both $_SERVER['HTTP_HOST'] and $_SERVER['SERVER_NAME'] being empty otherwise we are just introducing a new known problem.

We also need to check more than just Apache. What happens on Nginx or on IIS?

@csthomas
Copy link
Contributor

csthomas commented Nov 4, 2017

HTTP_HOST is not set when we run CLI script or the Apache or NginX do not have such variable set up.

If we do not know the name of domain we can not create any absolute URL with schema.
This means that $theURI or $uri should be relative without schema and host.

There is a global configuration variable live_site, I do not know if this is deprecated.

$live_site = ($uri->isSsl()) ? str_replace('http://', 'https://', $config->get('live_site')) : $config->get('live_site');

and IMO it should be used if HTTP_HOST is not available.

@Quy
Copy link
Contributor

Quy commented Nov 4, 2017

I checked 2 sites and live_site is blank.
I get this error when the client is using HTTP/1.0.

error log:

PHP Notice: Undefined index: HTTP_HOST in /httpdocs/libraries/src/Application/WebApplication.php on line 988
PHP Notice: Undefined index: HTTP_HOST in /httpdocs/libraries/src/Uri/Uri.php on line 90

access log:

176.58.114.41 - - [03/Nov/2017:10:45:21 -0700] "GET / HTTP/1.0" 400 0 "-" "-"
176.58.114.41 - - [03/Nov/2017:10:45:24 -0700] "GET / HTTP/1.0" 200 75748 "-" "-"

@piotr-cz
Copy link
Contributor

piotr-cz commented Nov 8, 2017

AFAIK the live_site setting is meant for edge cases when it's not possible to resolve URL, like this one

@csthomas
Copy link
Contributor

csthomas commented Nov 8, 2017

Maybe this way?

# HTTP/1.1
if $_SERVER['HTTP_HOST'] then 
...

# HTTP/1.0 and we have server variable
elseif $_SERVER['SERVER_NAME'] then 
...

# CLI request and joomla has set up $live_site
elseif $live_site then 
...

# At least do not raise PHP notice
else 'use empty schema and host'

@photodude
Copy link
Contributor

photodude commented Nov 8, 2017

I think @csthomas's suggestion is getting the closest to a proper way to address this issue.
As I noted when I opened #10203 the solution to this is going to be complex.

Looking at the definition of the $live_site variable in most cases we are getting text something like http://www.yourdomainname.com where $_SERVER['HTTP_HOST'] or $_SERVER['SERVER_NAME'] would only give text like www.yourdomainname.com or yourdomainname.com
So additional processing is going to be needed to strip off the http:// from the $live_site variable

@photodude
Copy link
Contributor

photodude commented Nov 8, 2017

possible example based on @csthomas's suggestion

if (isset($_SERVER['HTTP_HOST']))
{
    $host =  $_SERVER['HTTP_HOST'];
}
elseif (isset($_SERVER['SERVER_NAME']))
{
    $host = $_SERVER['SERVER_NAME'];
}
elseif (!empty(JFactory::getConfig()->get('live_site')))
{
    $liveSite = str_replace("http://", "https://", JFactory::getConfig()->get('live_site'));
    $host = $liveSite;
}
elseif ( !empty($_SERVER['SERVER_ADDR']))
{
    $host = $_SERVER['SERVER_ADDR'];
}
else
{
    $host = '';
}

@csthomas
Copy link
Contributor

csthomas commented Nov 9, 2017

Why would you want to add$_SERVER['SERVER_ADDR']?

Take a look at https://stackoverflow.com/questions/5705082/is-serverserver-addr-safe-to-rely-on/5705170

$_SERVER['HTTP_HOST'] and $live_site - may contain schema and port, ex http://www.example.com:8080
$_SERVER['SERVER_NAME'] - usually does not contains schema or port, only www.example.com

More info at:

@photodude
Copy link
Contributor

$_SERVER['SERVER_ADDR'] is the IP address
On IIS $_SERVER['SERVER_NAME'] might be "The server's host name, DNS alias, or IP address"

I considered $_SERVER['SERVER_ADDR'] as an alternative to possible IIS output for $_SERVER['SERVER_NAME']

based on some things I was reading for CLI we may want to also consider

elseif (isset($_ENV["HOSTNAME"]))
{
    $host = $_ENV["HOSTNAME"];
}
elseif (isset($_ENV["COMPUTERNAME"]))
{
    $host = $$_ENV["COMPUTERNAME"];
}

@photodude
Copy link
Contributor

@csthomas I was looking into the $_SERVER['SERVER_NAME'] issue with the port. From other things I was reading with Apache 2 "if your httpd UseCanonicalName directive is set to "On" then port gets parsed out of the ServerName directive."
So $_SERVER['SERVER_NAME'] may or may not include the port depending on apache server configuration.

Not sure about behavior on Nginx or IIS.

@Quy
Copy link
Contributor

Quy commented Nov 21, 2017

@photodude I would like it simple HTTP_HOST > SERVER_NAME > live_site that you proposed earlier without having to consider all possible cases/setup.

@photodude
Copy link
Contributor

@Quy sounds good to me. It's much better than what's currently in place.

I still question the last case. Is $host = ''; or $host = "undefined"; more correct. I generally am against leaving things as empty strings, but I'm not sure what the most appropriate solution here is.

If possible, someone should try to put some unit tests in for this issue too.

@mbabker
Copy link
Contributor

mbabker commented Nov 21, 2017

You shouldn't shove arbitrary data into something if you can't resolve a correct value.

@photodude
Copy link
Contributor

@mbabker I somewhat agree. There is definitely a balance between not shoving data into something when a value cannot be resolved; and specifying a default/fallback value for something when a value cannot be resolved because the variable should not be empty.

This is a case where I continue to be unsure of the appropriate course of action.

@Quy
Copy link
Contributor

Quy commented Nov 21, 2017

I get this warning now and then, but not frequent enough. In my case, SERVER_NAME would have resolved this issue. I don't mind updating live_site as the last resort/fallback. If left empty it will be the scenario now, but at least I have control of this value if required.

@rotech1
Copy link

rotech1 commented Feb 15, 2018

Joomla 5.8.5, PHP 7.1.13 on Apache still get this...

[Thu Feb 15 01:18:48 2018] [notice] [client xxx.xxx.xxx.xxx] PHP Notice: Undefined index: HTTP_HOST in [redacted]/libraries/src/Application/WebApplication.php on line 986
[Thu Feb 15 01:18:48 2018] [notice] [client xxx.xxx.xxx.xxx] PHP Notice: Undefined index: HTTP_HOST in [redacted]/libraries/src/Uri/Uri.php on line 90

@ghost ghost added the J3 Issue label Apr 5, 2019
@ghost ghost removed the J3 Issue label Apr 19, 2019
@zero-24 zero-24 deleted the branch joomla:staging August 8, 2021 20:08
@zero-24 zero-24 closed this Aug 8, 2021
@zero-24
Copy link
Contributor

zero-24 commented Aug 8, 2021

Dear @JannikMichel

in preperation of the upcomming release of Joomla 3.10 we have used GitHubs rename feature to rename the staging branch into 3.10-dev. Usually GitHub moves all existing PRs towards the new branch just fine, but here it didnt work. The reason seems to be that the fork of the CMS that was used as base for this PR has been deleted so GitHub does no longer have a base to rebase the PR against the new branch and we are also not able to reopen the PR. For that reason GitHub closed this PR in my name, when this issue is still valid It would require a new PR against the new 3.10-dev or 4.0-dev branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet