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

[HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer. #26973

Merged
merged 1 commit into from May 27, 2018

Conversation

Projects
None yet
8 participants
@kmadejski
Contributor

kmadejski commented Apr 18, 2018

Q A
Branch? 2.7 and up
Bug fix? improvement
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ---
License MIT
Doc PR ---

SubRequest used in InlineFragmentRendered explicitly sets $server['REMOTE_ADDR'] to 127.0.0.1. Therefore, it's required to configure 127.0.0.1 address in TRUSTED_PROXIES environment variable. Without that, Request::isFromTrustedProxy() will return false.
The current behavior might be a little bit problematic, for instance, in case where images are rendered through subrequests. These might end-up with an incorrect schema in URL (http instead of https).

@andrerom

andrerom approved these changes Apr 18, 2018 edited

👍 Seems to be same logic as in HttpCache::forward()

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 18, 2018

Can you add a test case please?

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented Apr 18, 2018

@nicolas-grekas sure, added in 3b3d590

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 18, 2018

@stof

This comment has been minimized.

Member

stof commented Apr 18, 2018

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

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented Apr 18, 2018

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 20, 2018

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?

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented Apr 20, 2018

@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 store Response in some local variable, remove previously added local IP address from trusted proxies and finally return Response.

Secondly, we would have to set some flag because 127.0.0.1 might have been intentionally set in TRUSTED_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 that 127.0.0.1 is always in trusted proxies when Symfony HttpCache 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?

@andrerom

This comment has been minimized.

Contributor

andrerom commented Apr 20, 2018

For what reason it is considered as an issue here?

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

naming ;)

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented Apr 26, 2018

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

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 29, 2018

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?

@andrerom

This comment has been minimized.

Contributor

andrerom commented Apr 29, 2018

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

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented May 14, 2018

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 in 434a6a1.

@kmadejski kmadejski changed the title from [HttpKernel] Explicitly set 127.0.0.1 as a trusted proxy to [HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer. May 14, 2018

@@ -122,7 +122,9 @@ protected function createSubRequest($uri, Request $request)
// Do nothing
}
$server['REMOTE_ADDR'] = '127.0.0.1';
$trustedProxies = Request::getTrustedProxies();
$server['REMOTE_ADDR'] = isset($trustedProxies[0]) ? $trustedProxies[0] : '127.0.0.1';

This comment has been minimized.

@andrerom

andrerom May 14, 2018

Contributor

Should this rather pick the last/closest one? (unsure if it makes a difference tough)

This comment has been minimized.

@kmadejski

kmadejski May 14, 2018

Contributor

I'm not sure if there is any significant difference, @nicolas-grekas what do you think?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 15, 2018

Member

just to be bullet proof:
$trustedProxies ? reset($trustedProxies) : '127.0.0.1';
then the first LGTM

This comment has been minimized.

@kmadejski

kmadejski May 15, 2018

Contributor

@nicolas-grekas changed in 4fb6e03.

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented May 15, 2018

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

@fabpot fabpot changed the base branch from 2.7 to 2.8 May 27, 2018

@fabpot

fabpot approved these changes May 27, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented May 27, 2018

Thank you @kmadejski.

@fabpot fabpot merged commit 18f55fe into symfony:2.8 May 27, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 27, 2018

bug #26973 [HttpKernel] Set first trusted proxy as REMOTE_ADDR in Inl…
…ineFragmentRenderer. (kmadejski)

This PR was squashed before being merged into the 2.8 branch (closes #26973).

Discussion
----------

[HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer.

| Q             | A
| ------------- | ---
| Branch?       | 2.7 and up
| Bug fix?      | improvement
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ---
| License       | MIT
| Doc PR        | ---

SubRequest used in `InlineFragmentRendered` explicitly sets `$server['REMOTE_ADDR']` to `127.0.0.1`. Therefore, it's required to configure `127.0.0.1` address in TRUSTED_PROXIES environment variable. Without that, `Request::isFromTrustedProxy()` will return false.
The current behavior might be a little bit problematic, for instance, in case where images are rendered through subrequests. These might end-up with an incorrect schema in URL (`http` instead of `https`).

Commits
-------

18f55fe [HttpKernel] Set first trusted proxy as REMOTE_ADDR in InlineFragmentRenderer.
@cybernet

This comment has been minimized.

cybernet commented Jun 25, 2018

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

@andrerom

This comment has been minimized.

Contributor

andrerom commented Jun 25, 2018

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

@artursvonda

This comment has been minimized.

Contributor

artursvonda commented Jul 12, 2018

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.

@kmadejski

This comment has been minimized.

Contributor

kmadejski commented Jul 13, 2018

@artursvonda it seems that you are right. As a workaround, for now, you can simply set 127.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 for InlineFragmentRendered, therefore I would like to propose to fix it by setting fallback to 127.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.

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