[HttpFoundation] [HttpKernel] Internal sub-requests should have X-Forwarded-For header providing real client IP #7559

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

guilhermeblanco commented Apr 4, 2013

This is a better alternative to fix issue highlighted in #7554 and #7557.

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #7554, #7557
License MIT

When dealing with inline fragment renderer, it emulates an internal request by overriding the REMOTE_ADDR on Request. This is true, since conceptually request came from local server.
The problem that this introduces is that overriding the server value, it turns into an impossible state to retrieve the real client ip, only returning the local server IP (which is hardcoded to 127.0.0.1).

This patch takes the same approach as a Varnish call (it behaves the exact same way, reusing all code built for handling client ip handling on sub-requests), populating the X-Forwarded-For header and also making getClientIp smarter by removing possible local IP addresses from being considered as the client IP address.

@lazyhammer lazyhammer and 1 other commented on an outdated diff Apr 4, 2013

...ponent/HttpKernel/Fragment/InlineFragmentRenderer.php
@@ -87,9 +87,15 @@ protected function createSubRequest($uri, Request $request)
$server = $request->server->all();
// the sub-request is internal
+ $originalXForwardedFor = isset($server['HTTP_X_FORWARDED_FOR'])
@lazyhammer

lazyhammer Apr 4, 2013

Contributor

This header name is configurable in Request, so you should get it from there.

@guilhermeblanco

guilhermeblanco Apr 4, 2013

Contributor

Makes sense, will update shortly

@guilhermeblanco

guilhermeblanco Apr 4, 2013

Contributor

@lazyhammer According to codebase, Symfony\Component\HttpFoundation\Request contains the constants which are keys to a protected static array of actual header names.
What would be your suggested approach to handle it?

  1. Make the static array public ($trustedHeaders)
  2. Add a static getter in Request (something like getTrustedHeaderName($key))
    How it should behave if no key is found? Should I throw an InvalidArgumentException or should I return a blank string?
@lazyhammer

lazyhammer Apr 4, 2013

Contributor

I would go the second way, but I'm not the one who makes the final decision :)
I think you should replicate the setter's behavior - throwing an exception if the key does not exist.

@lazyhammer lazyhammer and 1 other commented on an outdated diff Apr 4, 2013

src/Symfony/Component/HttpFoundation/Request.php
@@ -696,7 +696,7 @@ public function getClientIp()
$clientIps[] = $ip;
$trustedProxies = self::$trustProxy && !self::$trustedProxies ? array($ip) : self::$trustedProxies;
- $clientIps = array_diff($clientIps, $trustedProxies);
+ $clientIps = array_diff($clientIps, array_merge($trustedProxies, $this->getLocalIpAddresses()));
@lazyhammer

lazyhammer Apr 4, 2013

Contributor

Maybe, it would be better to document that InlineFragmentRenderer acts like a proxy and user have to add 127.0.0.1 to the list of trusted proxies if he wants to get the real client IP for this subrequest?
BTW, since 2.3 you can't trust proxies and have an empty trusted proxy list, so the user will have to explicitly set some IP to make this line reachable.

@guilhermeblanco

guilhermeblanco Apr 4, 2013

Contributor

Trusted proxies should always contain the local IP address IMHO...
Of course that here is not the correct place to fix this, but I think this should be considered when adding the trusted proxies.
I'll remove from here.

Contributor

guilhermeblanco commented Apr 5, 2013

@lazyhammer updated code as per your suggestions.

@fabpot could you please review and possibly merge this into master and 2.2 branch?
This is a bugfix to make inline fragment renderer behaves the exact same way as Symfony handles requested from reverse proxies, such as Varnish. Also updated the tests to handle this concept.

@stof stof commented on the diff Apr 5, 2013

...ponent/HttpKernel/Fragment/InlineFragmentRenderer.php
@@ -86,10 +86,25 @@ protected function createSubRequest($uri, Request $request)
$cookies = $request->cookies->all();
$server = $request->server->all();
- // the sub-request is internal
+ // Override the arguments to emulate a sub-request.
+ // Sub-request object will point to localhost as client ip and real client ip
+ // will be included into trusted header for client ip
+ try {
+ $headerClientIp = 'HTTP_' . Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP);
+
+ $originalXForwardedFor = isset($server[$headerClientIp])
@stof

stof Apr 5, 2013

Member

you should use $request->header-get(Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP), ''); instead to get the header. This way, it will handle the header properly to keep the name case-insensitive, whereas your code does not.

@guilhermeblanco

guilhermeblanco Apr 7, 2013

Contributor

@stof possible yes, but it wouldn't solve the problem. It needs to be overwritten later and AFAIR there's no update support for Bags. =(

@stof

stof Apr 7, 2013

Member

@guilhermeblanco currently, it would work if the case used when sending the header is different, as you are using the non-normalized array.

@guilhermeblanco

guilhermeblanco Apr 9, 2013

Contributor

@stof I updated the code (haven't pushed yet), but wanna some feedback from you. Here it is the method:

    protected function createSubRequest($uri, Request $request)
    {
        $cookies = $request->cookies->all();
        $server = $request->server->all();

        // Override the arguments to emulate a sub-request.
        // Sub-request object will point to localhost as client ip and real client ip
        // will be included into trusted header for client ip
        try {
            $trustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP);
            $oldXForwardedFor = $request->headers->get($trustedHeaderName, '');
            $newXForwardedFor = ($oldXForwardedFor ? $oldXForwardedFor . ', ' : '') . $request->getClientIp();

            $server['HTTP_' . $trustedHeaderName] = $newXForwardedFor;
        } catch (\InvalidArgumentException $e) {
            // Do nothing
        }

        $server['REMOTE_ADDR'] = '127.0.0.1';

        $subRequest = $request::create($uri, 'get', array(), $cookies, array(), $server);

        if ($session = $request->getSession()) {
            $subRequest->setSession($session);
        }

        return $subRequest;
    }
@fabpot

fabpot Apr 20, 2013

Owner

Can you push your changes?

@Ocramius Ocramius commented on the diff Apr 15, 2013

...ponent/HttpKernel/Fragment/InlineFragmentRenderer.php
@@ -86,10 +86,25 @@ protected function createSubRequest($uri, Request $request)
$cookies = $request->cookies->all();
$server = $request->server->all();
- // the sub-request is internal
+ // Override the arguments to emulate a sub-request.
+ // Sub-request object will point to localhost as client ip and real client ip
+ // will be included into trusted header for client ip
+ try {
+ $headerClientIp = 'HTTP_' . Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP);
+
+ $originalXForwardedFor = isset($server[$headerClientIp])
+ ? $server[$headerClientIp] . ', '
+ : '';
+
+ $server[$headerClientIp] = $originalXForwardedFor . $server['REMOTE_ADDR'];
+ } catch (\InvalidArgumentException $e) {
+ // Do nothing
@Ocramius

Ocramius Apr 15, 2013

Contributor

Uhmmm... any better approach? Is there any way to check getTrustedHeaderName upfront instead?

@guilhermeblanco

guilhermeblanco Apr 15, 2013

Contributor

@Ocramius it's the exact method that either exists and it gets returned or it throws InvalidArgumentException.

@Ocramius

Ocramius Apr 15, 2013

Contributor

method_exists then

@guilhermeblanco

guilhermeblanco Apr 15, 2013

Contributor

No... you haven't got me correctly. Method is gonna always exist on Request object.
The problem is that if the argument exists, it returns it. Otherwise, it throws an InvalidArgumentException.
Look at the implementation of method Request::getTrustedHeaderName:

    public static function getTrustedHeaderName($key)
    {
        if (!array_key_exists($key, self::$trustedHeaders)) {
            throw new \InvalidArgumentException(sprintf('Unable to get the trusted header name for key "%s".', $key));
        }

        return self::$trustedHeaders[$key];
    }
@Ocramius

Ocramius Apr 15, 2013

Contributor

Ah, I see. It may not be in the scope of this PR, but can the exception type be specialized a bit more?

@guilhermeblanco

guilhermeblanco Apr 15, 2013

Contributor

@Ocramius it is out of this scope. I used the same concept of setTrustedHeaderName here, which throws the exact same exception.

fabpot closed this in 2f3b33a Apr 21, 2013

Contributor

guilhermeblanco commented Apr 22, 2013

@fabpot do you still want me to push my changes?
It seems you already merged this one. Maybe as a new PR? Please provide clarification here...

Owner

fabpot commented Apr 22, 2013

@guilhermeblanco I've done the needed changes. So, everything should be fine now.

checat commented Jun 3, 2013

Hi! Will this be backported to 2.2? I see this fix was not merged into 2.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment