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

RequestFactory: fix behavior behind proxy #81

Closed
wants to merge 4 commits into from

Conversation

@grongor
Copy link
Contributor

grongor commented Jan 31, 2016

  • bugfix
  • issues - #4
  • documentation - not needed
  • BC break - may break some edge-case server proxies, most servers should stay intact
@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Jan 31, 2016

👍

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 31, 2016

👍 It just took almost 2 years to implement such simple thing. 😭

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Jan 31, 2016

Shouldn't we do something with HTTP_X_FORWARDED_PORT as well?

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Jan 31, 2016

@enumag I was thinking exactly the same thing. How will Nette\Http\Url and/or routers work now when https mode is enabled over port 80? Wouldn't this generate nonsense like https://example.com:80/?

@grongor

This comment has been minimized.

Copy link
Contributor Author

grongor commented Jan 31, 2016

What do you think about that master...grongor:fix-proxy-port ?

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 1, 2016

Has anyone considered security implication?

@milo

This comment has been minimized.

Copy link
Member

milo commented Feb 1, 2016

I was thinking about security in this way. The attacker can fake the HTTP_X_FORWARDED_PROTO header content. There are only two result states: http or https. These states affect the router behaviour (automatic redirections secured vs. no-flag) and URL (http:// vs https://).

Switch from http -> https: it can lead to application problem, but not security imho
Switch from https -> http: the header can be faked only outside of an SSL/TLS session, so somewhere after the proxy. But in that case you cannot trust the proxy and security does not matter.

Any other point of view?

@grongor

This comment has been minimized.

Copy link
Contributor Author

grongor commented Feb 1, 2016

@JanTvrdik @milo Yes, I was thinking about it the same way. If the proxy is able to mess with your headers then you can't trust it anyway - no matter what we put at server side. There might be a problem with misconfiguration of the proxy but we can't really predict that.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 1, 2016

How does Symfony and Zend handle this?

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Feb 1, 2016

Symfony checks if the proxy is trusted and if so it uses these headers instead.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 1, 2016

@enumag That's what I thought.

The current trusted proxy check should be extracted and used for HTTP_X_FORWARDED_PROTO as well.

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Feb 1, 2016

@JanTvrdik Well it seems that by default the trustedProxies array in symfony is empty. But I'm not sure if there are some added automatically elsewhere.

@Majkl578

This comment has been minimized.

Copy link
Contributor

Majkl578 commented Feb 1, 2016

@grongor

This comment has been minimized.

Copy link
Contributor Author

grongor commented Feb 1, 2016

I added the HTTP_X_FORWARDED_PORT handling and taken the $proxies into considerations when detecting scheme and port. Is that acceptable?

@grongor grongor changed the title RequestFactory: detect schema behind proxy correctly RequestFactory: fix behavior behind proxy Feb 1, 2016
}
if (isset($_SERVER['HTTP_X_FORWARDED_HOST'])) {
$remoteHost = trim(current(explode(',', $_SERVER['HTTP_X_FORWARDED_HOST'])));
}

This comment has been minimized.

Copy link
@enumag

enumag Feb 1, 2016

Contributor

I'd put these outside of the foreach.

This comment has been minimized.

Copy link
@grongor

grongor Feb 1, 2016

Author Contributor

Well, I can do that but I will have to copy the $usingTrustedProxy === true condition ... do you think that removing one indention is worth it?

This comment has been minimized.

Copy link
@enumag

enumag Feb 1, 2016

Contributor

I just think that any logic that only happens once should be outside of a loop if possible.

Also you can skip the === true part.

This comment has been minimized.

Copy link
@grongor

grongor Feb 1, 2016

Author Contributor

That makes sense, alright :) Yeah I will, I was just trying to be clear for the purpose of comment :)

if (isset($_SERVER['HTTP_X_FORWARDED_HOST'])) {
$remoteHost = trim(current(explode(',', $_SERVER['HTTP_X_FORWARDED_HOST'])));
}
}

This comment has been minimized.

Copy link
@enumag

enumag Feb 1, 2016

Contributor

Shouldn't this be grouped together with this code (it is about 20 lines below)? I don't see anything preventing it.

if ($usingTrustedProxy && !empty($_SERVER['HTTP_X_FORWARDED_PORT'])) {
    $port = (int) $_SERVER['HTTP_X_FORWARDED_PORT'];
}

This comment has been minimized.

Copy link
@grongor

grongor Feb 1, 2016

Author Contributor

Yes it can be grouped together but I thought that the separation is for better readability (1. proxy, 2. scheme, 3. port, ...). If I put it together I'm gonna somehow mix these groups together ... If you think it's better I will merge these conditions.

This comment has been minimized.

Copy link
@enumag

enumag Feb 1, 2016

Contributor

Let's wait for other opinions. :-)

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

I vote for less nesting -- merging the conditions.

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

well in that case it will add one indention:

if ($usingTrustedProxy) {
    if (isset($_SERVER['HTTP_X_FORWARDED_FOR'])) {
        $remoteAddr = trim(current(explode(',', $_SERVER['HTTP_X_FORWARDED_FOR'])));
    }

    if (isset($_SERVER['HTTP_X_FORWARDED_HOST'])) {
        $remoteHost = trim(current(explode(',', $_SERVER['HTTP_X_FORWARDED_HOST'])));
    }

    if (!empty($_SERVER['HTTP_X_FORWARDED_PORT'])) {
        $port = (int) $_SERVER['HTTP_X_FORWARDED_PORT'];
    }
}

Or am I missing something?

// use real client address and host if trusted proxy is used
$usingTrustedProxy = FALSE;
foreach ($this->proxies as $proxy) {
if (Helpers::ipMatch($remoteAddr, $proxy)) {

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

What if $remoteAddr is NULL?

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

Well, it work's, but I guess I should test the null value explicitly.

$usingTrustedProxy = TRUE;
break;
}
}

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

This foreach could be rewritten (to avoid variable default + if + break) as such:

$trustedProxies = count(array_filter($this->proxies, function ($proxy) use ($remoteAddr) {
    return Helpers::ipMatch($remoteAddr, $proxy);
});

and the check below as:

if ($trustedProxies) {

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

Good point, I'll change foreach to array_filter

$port = (int) $_SERVER['HTTP_X_FORWARDED_PORT'];
}

$isInvalidCombination = $scheme === 'http' && $port === 443 || $scheme === 'https' && $port === 80;

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

Missing brackets.

Anyhow I think this should not be tested at all.

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

I omitted them intentionally - it works well. Anyway if we don't test it there then we can endup with strange urls like https://localhost:80. Of course we can decide that this is ok and it's up to the developer to take care of these invalid links. The server would have to be badly missconfigured to produce such a strange combination of scheme and port.

$isInvalidCombination = $scheme === 'http' && $port === 443 || $scheme === 'https' && $port === 80;
if ($port !== null && !$isInvalidCombination) {
$url->setPort($port);
}

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

How is that invalid? I always though that protocol is port independent, i.e. it should be possible to run HTTPS over port 80 – https://example.com:80.

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

That is true, however in that case it doesn't really makes sense. I think that a lot of browsers would choose to do that anyway. But if you think that this shouldn't be there I can remove it - I just thought that it would be for the better.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

Yes I think it should be removed. Its too smart.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 2, 2016

Should I point out all the issues or will @dg rewrite it anyway?

@grongor

This comment has been minimized.

Copy link
Contributor Author

grongor commented Feb 2, 2016

Please point out the issues, otherwise I won't learn what's wrong with this contribution :)

$remoteHost = !empty($_SERVER['REMOTE_HOST']) ? $_SERVER['REMOTE_HOST'] : NULL;

// use real client address and host if trusted proxy is used
$usingTrustedProxy = (bool)count(array_filter($this->proxies, function ($proxy) use ($remoteAddr) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

There should be space after (bool), the count call should be removed.

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

****, you are right, I again blindly copied the code from @Majkl578 :D

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

Sorry, count was a leftover, originally I had it like count(...) !== 0; but then I changed my mind. :)

@@ -218,4 +231,38 @@ public function createHttpRequest()
return new Request($url, NULL, $post, $files, $cookies, $headers, $method, $remoteAddr, $remoteHost, $rawBodyCallback);
}

/**
* @param bool $usingTrustedProxy

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

There should be two spaces between @param and bool (to align with return). The name of the parameter ($usingTrustedProxy) should be removed.

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

Where did you get these information? I was following the coding standards. Omitting parameter name breaks PhpStorm and is not recommended by phpDocumentator - are you really sure that we want that?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

Where did you get these information?

By looking at Nette code.

I was following the coding standards.

Nope. Citation „The @param annotation is followed by two spaces, type and optionally by space and description.“

Omitting parameter name breaks PhpStorm and is not recommended by phpDocumentator

This is well known but consistency is a lot more important. I even wrote Nette RFC to change that but there is unresolved issue with alignment.

{
if ($usingTrustedProxy) {
if (!empty($_SERVER['HTTP_X_FORWARDED_PROTO'])) {
if (strcasecmp($_SERVER['HTTP_X_FORWARDED_PROTO'], 'https') === 0) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

Those two condition should be merged, possible rewritten as ternary expression.

} elseif ($port === 80) {
return 'http';
}
}

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

AFAIK you cannot derive schema from port. How does Symfony and Zend handle this?

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

I think it's the same case as with the conflicting scheme and ports. If we decide that we don't want the code to be that clever than we can remove this as well. It only handles badly configured proxies anyway.

return 'https';
}

return 'http';

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

Possibly rewrite to ternary expression.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 2, 2016

Some more thoughts – I would try to remove the getSchema method entirely (inline in as it was before) and move the proxy-specific logic to where it was before.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Feb 2, 2016

One of reasons to keep the proxy logic together is #20

@@ -218,4 +231,38 @@ public function createHttpRequest()
return new Request($url, NULL, $post, $files, $cookies, $headers, $method, $remoteAddr, $remoteHost, $rawBodyCallback);
}

/**

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 2, 2016

Contributor

There should be two empty lines between two methods.

Jakub Chabek
Correctly detect scheme and port if the
server is behind a trusted proxy.
@grongor

This comment has been minimized.

Copy link
Contributor Author

grongor commented Feb 2, 2016

Thanks @JanTvrdik - I updated the code according to you suggestions and it's much easier :-) I also got rid of the "smart" scheme/port alignment. I hope I didn't miss something again ;-)

$remoteAddr = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : NULL;
$remoteHost = isset($_SERVER['REMOTE_HOST']) ? $_SERVER['REMOTE_HOST'] : NULL;
// use real client address and host if trusted proxy is used
$usingTrustedProxy = (bool)array_filter($this->proxies, function ($proxy) use ($remoteAddr) {

This comment has been minimized.

Copy link
@hrach

hrach Feb 2, 2016

Contributor

do a space after the casting

This comment has been minimized.

Copy link
@grongor

grongor Feb 2, 2016

Author Contributor

****, I tried so hard ... it's hard to notice these things when all other projects I'm working on use exactly opposite standards ...

This comment has been minimized.

Copy link
@hrach

hrach Feb 2, 2016

Contributor

still, you did an awesome work! thank you!

$remoteHost = isset($_SERVER['REMOTE_HOST']) ? $_SERVER['REMOTE_HOST'] : NULL;
// use real client address and host if trusted proxy is used
$usingTrustedProxy = (bool) array_filter($this->proxies, function ($proxy) use ($remoteAddr) {
return $remoteAddr !== NULL && Helpers::ipMatch($remoteAddr, $proxy);

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 2, 2016

Contributor

This $remoteAddr !== NULL is redundant, it should go outside the callback.

@@ -185,24 +187,34 @@ public function createHttpRequest()
}
}

$remoteAddr = !empty($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : NULL;
$remoteHost = !empty($_SERVER['REMOTE_HOST']) ? $_SERVER['REMOTE_HOST'] : NULL;

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 3, 2016

Contributor

Why change isset to !empty? Isset worked OK for years, is faster to execute, can be in PHP 7 rewritten with ?? and does not consider 0 as empty.

This comment has been minimized.

Copy link
@grongor

grongor Feb 3, 2016

Author Contributor

Because I don't think that having an empty host or address makes sense. Is that wrong assumption?

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Feb 3, 2016

Contributor

Having invalid value like '0' or '1' here makes no sense either and it's not checked anyway, maybe it could be.

This comment has been minimized.

Copy link
@grongor

grongor Feb 3, 2016

Author Contributor

That's true but I guess that we don't want to check webserver settings there. I just though that this is a little bit better (to ignore empty header rather the somehow try to use it).

@@ -76,9 +76,11 @@ public function createHttpRequest()
} elseif (isset($_SERVER['SERVER_PORT'])) {
$url->setPort((int) $_SERVER['SERVER_PORT']);
}
} else if (!empty($_SERVER['SERVER_PORT'])) {
$url->setPort((int) $_SERVER['SERVER_PORT']);

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Feb 3, 2016

Contributor

What's the point of having port without host? Does the code solve a real world issue? If not, it should be removed.

This comment has been minimized.

Copy link
@grongor

grongor Feb 3, 2016

Author Contributor

Yeah I guess not, you are right, thanks.

@dg dg closed this in 805ff25 Feb 7, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 7, 2016

@grongor thanks, merged dadd667

dg added a commit that referenced this pull request Feb 8, 2016
…hind a trusted proxy [Closes #81][Closes #4]
dg added a commit that referenced this pull request Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.