RequestFactory: fix behavior behind proxy

[grongor]

• bugfix
• issues - https + nginx proxy #4
• documentation - not needed
• BC break - may break some edge-case server proxies, most servers should stay intact

[hrach]

👍

[Majkl578]

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

[enumag]

Shouldn't we do something with HTTP_X_FORWARDED_PORT as well?

[Majkl578]

@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]

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

[JanTvrdik]

Has anyone considered security implication?

[milo]

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]

@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]

How does Symfony and Zend handle this?

[enumag]

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

[JanTvrdik]

@enumag That's what I thought.

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

[enumag]

@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]

Symfony configures trusted proxies using kernel parameter: https://github.com/symfony/symfony/blob/83b53f4a89fa2f436ba0f8d88b0714aa534b2cd2/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php#L53

[grongor]

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

[enumag]

I'd put these outside of the foreach.

[grongor]

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?

[enumag]

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.

[grongor]

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

[JanTvrdik]

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

[grongor]

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

[JanTvrdik]

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

[grongor]

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

[Majkl578]

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

[JanTvrdik]

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]

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