Can you add a test case please?

@nicolas-grekas sure, added in3b3d590

I see a big issue with this change: the local IP will stay trusted even after the end of handling this sub-request

@stof you are right, local IP will be trusted for all subrequest within the master request, but I don't see any problem regarding that. Could you please explain or describe some use-case where it might be an issue?

describe some use-case where it might be an issue

instead of trying to answer this question, would it be possible to restore the list to the previous state?

@nicolas-grekas probably yes, but I think that it might be tricky.

At first, it would be necessary to change this line:https://github.com/kmadejski/symfony/blob/3b3d5903109feadbdf4b40fb9f11929cdcc617ac/src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php#L79 and storeResponse in some local variable, remove previously added local IP address from trusted proxies and finally returnResponse.

Secondly, we would have to set some flag because127.0.0.1 might have been intentionally set inTRUSTED_PROXIES and we need to know whether it should be removed after controller action execution or not.

Of course, if you think that it's a good idea then I can try to do the change as described.
However, correct me if I'm wrong, but it seems that127.0.0.1 isalways in trusted proxies when SymfonyHttpCache is enabled, right? (thanks to this piece of code:https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L479-%23L483). For what reason it is considered as an issue when not using Symfony proxy?

For what reason it is considered as an issue here?

When not using Symfony proxy (so Varnish, Fastly, Nginx cache, ..)

naming ;)

@nicolas-grekas I see that you've changed the status of this PR, should I assume that you prefer the way of trying to restore the list of trusted proxies back to the previous state?

The best would be to not alter the global state at all.
Would it make sense to set the remote address to the first trusted proxy instead of 127.0.0.1 when there is one?

The best would be to not alter the global state at all.

true.

Would it make sense to set the remote address to the first trusted proxy instead of 127.0.0.1 when there is one?

Alternatively, could we skip setting it? Tried to check blame a few rounds back for why ip is set to localhost in the first place, but seems to have been added when logic was moved in#6459. So not sure why it's done like that, but back then localhost seems to have been considered trusted:https://github.com/symfony/symfony/pull/6459/files#diff-5a6abcbb3081371c8f5e3b9434c1ec18R87

Would it make sense to set the remote address to the first trusted proxy instead of 127.0.0.1 when there is one?

@nicolas-grekas I did as you propose and it seems to be a pretty good solution which solves our issue. See changes in434a6a1.

@nicolas-grekas JFYI: I've pushed again last commit with force to restart AppVeyor build which has been failed for the unknown reason previously.

Thank you@kmadejski.

1.1.1.1 is actually a valid IP. is that a good ideea ?

@cybernet that is the test code, or are you talking about something else?

Came across issue with this fix. We specify range (xxx.xxx.xxx.xxx/8) for trusted IPs which cause this solution to fail. Might not be the best practice, but it should not cause the request to fail.

@artursvonda it seems that you are right. As a workaround, for now, you can simply set127.0.0.1 as your first trusted proxy and everything should be fine.

@nicolas-grekas I think that dealing with getting the first valid IP within given CIDR is too much forInlineFragmentRendered, therefore I would like to propose to fix it by setting fallback to127.0.0.1 in case of first trusted proxy defined in CIDR notation. WDYT about something like:

$trustedProxies = Request::getTrustedProxies();$firstTrustedProxy = reset($trustedProxies);$server['REMOTE_ADDR'] = $firstTrustedProxy && false === strpos($firstTrustedProxy, '/') ? $firstTrustedProxy : '127.0.0.1';

If you agree then I will create a new PR with the proposed change.