Skip to content

Commit

Permalink
fix request URI with multiple leading slashes, #203
Browse files Browse the repository at this point in the history
  • Loading branch information
@ikkez
ikkez committed Mar 30, 2017
1 parent 224fc9f commit 575cfda
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion base.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2244,7 +2244,8 @@ function($level,$text,$file,$line) {
if (!$cli)
$base=rtrim($this->fixslashes(
dirname($_SERVER['SCRIPT_NAME'])),'/');
$uri=parse_url($_SERVER['REQUEST_URI']);
$uri=parse_url((preg_match('/^\w+:\/\//',$_SERVER['REQUEST_URI'])?'':
'//'.$_SERVER['SERVER_NAME']).$_SERVER['REQUEST_URI']);
$path=preg_replace('/^'.preg_quote($base,'/').'/','',$uri['path']);
session_cache_limiter('');
call_user_func_array('session_set_cookie_params',
Expand Down

9 comments on commit 575cfda

@xfra35
Copy link
Member

@xfra35 xfra35 commented on 575cfda Mar 31, 2017

Choose a reason for hiding this comment

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

Why the preg_match? $_SERVER['REQUEST_URI'] should always be scheme-less.

@ikkez
Copy link
Member Author

@ikkez ikkez commented on 575cfda Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I read that in case a proxy was used, REQUEST_URI can also be a full absolute URI. In that case we don't need to adjust this thing, or it would break.

@xfra35
Copy link
Member

@xfra35 xfra35 commented on 575cfda Mar 31, 2017

Choose a reason for hiding this comment

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

On which server? If that's the case, maybe we should tweak REQUEST_URI or URI so that other pieces don't break (REALM for example).

@ikkez
Copy link
Member Author

@ikkez ikkez commented on 575cfda Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

not tested by myself yet, just read that in comments on http://php.net/manual/en/reserved.variables.server.php but maybe this is not valid. I was just believing that, due to the fact that parse_url is used at this point, which already handles this occasion. So I thought it's already prepared for that in the core?!?

@xfra35
Copy link
Member

@xfra35 xfra35 commented on 575cfda Mar 31, 2017

Choose a reason for hiding this comment

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

Justed tested it, following these instructions, with the following .pac file:

function FindProxyForURL(url, host) {
    if (dnsDomainIs(host, ".foo.local"))
        return "PROXY 192.168.33.10";  
    return "DIRECT";
}

Here are the variables which differ:

http://192.168.33.10/foo/barhttp://www.foo.local/foo/bar
HTTP_HOST192.168.33.10www.foo.local
SERVER_NAME192.168.33.10www.foo.local
REQUEST_URI/foo/barhttp://www.foo.local/foo/bar

But now: isn't that a misuse of the proxy autoconfig ? We're sending a proxy command to a web server, instead of a proxy server. And if I look at the Apache access.log, I can see this:

192.168.33.1 - - [31/Mar/2017:14:18:30 +0000] "GET /foo/bar/ HTTP/1.1"
192.168.33.1 - - [31/Mar/2017:14:21:01 +0000] "GET http://www.foo.local/foo/bar/ HTTP/1.1"

which shows that the HTTP request generated by the .pac file is intended for a proxy, not a web server.

https://tools.ietf.org/html/rfc2616#section-5.1.2 says:

The absoluteURI form is REQUIRED when the request is being made to a proxy.

and

The most common form of Request-URI is that used to identify a resource on an origin server or gateway. In this case the absolute path of the URI MUST be transmitted as the Request-URI, and the network location of the URI (authority) MUST be transmitted in a Host header field.

@ikkez
Copy link
Member Author

@ikkez ikkez commented on 575cfda Mar 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thx for the research @xfra35. so what does this mean? It's valid to patch it for the framework, as we don't know if the application will ever be used as a proxy service directly?! (actually I really wrote such a thing on F3 already)

@xfra35
Copy link
Member

@xfra35 xfra35 commented on 575cfda Apr 4, 2017

Choose a reason for hiding this comment

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

Well it looks to me that:

  • the absoluteURI form is meant for web proxies
  • the framework is used on web servers

So the framework should never receive an absoluteURI form.

But the fact is that we may, in very rare cases, receive an absolute URI. So your patch makes sense.

The next question is: what to do with REQUEST_URI? Should we patch it as well, so that we can keep on relying on it as a relative URI in all circumstances? That would fix REALM at the same time.

@bcosca
Copy link
Collaborator

@bcosca bcosca commented on 575cfda Apr 5, 2017

Choose a reason for hiding this comment

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

I am for patching REQUEST_URIif both of you agree.

@xfra35
Copy link
Member

@xfra35 xfra35 commented on 575cfda Apr 5, 2017

Choose a reason for hiding this comment

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

Ok done here 4d70bb4

Please sign in to comment.