Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Update InlineFragmentRenderer.php #7554

Closed
wants to merge 1 commit into from

4 participants

@kinncj

this is wrong:
"// the sub-request is internal
 $server['REMOTE_ADDR'] = '127.0.0.1';"

I always want to know the client IP address...
If I was behind a proxy/varnish, I should be able to get the reverse instead of localhost

@kinncj kinncj Update InlineFragmentRenderer.php
this is wrong:
"// the sub-request is internal
 $server['REMOTE_ADDR'] = '127.0.0.1';"

I always want to know the client IP address...
If I was behind a proxy/varnish, I should be able to get the reverse instead of localhost
9c4eb20
@stof
Collaborator

subrequest are meant to be internal by design, even when doing them inline rather than doing them from Varnish.

@guilhermeblanco

@stof then InlineFragmentRenderer should add the X-Forwarded-For of real client IP, just like EsiFragmentRenderer expects.
Otherwise, there's no real way of grabbing the real client IP using HttpFoundation\Request component.
This line could be then modified to a server inclusion of an X-Forwarded-For header with real client IP.

Makes sense?

@kinncj

#7557
This is the PR with the fix.

I totally agree with blanco... the getClientIp should always return the client(in this case, browser) IP address...

@fabpot
Owner

closing in favor of #7559

@fabpot fabpot closed this
@fabpot fabpot referenced this pull request from a commit
@fabpot fabpot merged branch guilhermeblanco/client_ip_fix (PR #7559)
This PR was squashed before being merged into the master branch (closes #7559).

Discussion
----------

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

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.

Commits
-------

773e109 [HttpFoundation] [HttpKernel] Internal sub-requests should have X-Forwarded-For header providing real client IP
2f3b33a
@hackzilla hackzilla referenced this pull request from a commit
@fabpot fabpot merged branch guilhermeblanco/client_ip_fix (PR #7559)
This PR was squashed before being merged into the master branch (closes #7559).

Discussion
----------

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

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.

Commits
-------

773e109 [HttpFoundation] [HttpKernel] Internal sub-requests should have X-Forwarded-For header providing real client IP
4d7d4bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2013
  1. @kinncj

    Update InlineFragmentRenderer.php

    kinncj authored
    this is wrong:
    "// the sub-request is internal
     $server['REMOTE_ADDR'] = '127.0.0.1';"
    
    I always want to know the client IP address...
    If I was behind a proxy/varnish, I should be able to get the reverse instead of localhost
This page is out of date. Refresh to see the latest.
View
3  src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php
@@ -86,9 +86,6 @@ protected function createSubRequest($uri, Request $request)
$cookies = $request->cookies->all();
$server = $request->server->all();
- // the sub-request is internal
- $server['REMOTE_ADDR'] = '127.0.0.1';
-
$subRequest = $request::create($uri, 'get', array(), $cookies, array(), $server);
if ($session = $request->getSession()) {
$subRequest->setSession($session);
Something went wrong with that request. Please try again.