[FrameworkBundle] used the new method for trusted proxies #6154
Conversation
| @@ -62,6 +62,9 @@ public function load(array $configs, ContainerBuilder $container) | |||
| } | |||
| $container->setParameter('kernel.secret', $config['secret']); | |||
|
|
|||
| $container->setParameter('kernel.trusted_proxies', $config['trusted_proxies']); | |||
|
|
|||
| // for BC, to be removed in 2.3 | |||
There was a problem hiding this comment.
best would be to use a @deprecated annotation
There was a problem hiding this comment.
@lsmith77 : you're right, should it be /** @deprecated ... */ (as it's a phpdoc annotation) or is it ok in a standard comment?
|
As this is a sensitive issue, can you add some tests? Thanks. |
| @@ -62,6 +62,9 @@ public function load(array $configs, ContainerBuilder $container) | |||
| } | |||
| $container->setParameter('kernel.secret', $config['secret']); | |||
|
|
|||
| $container->setParameter('kernel.trusted_proxies', $config['trusted_proxies']); | |||
|
|
|||
| // @deprecated, to be removed in 2.3 | |||
| $container->setParameter('kernel.trust_proxy_headers', $config['trust_proxy_headers']); | |||
There was a problem hiding this comment.
Shouldn't an exception be thrown instead as this is related to security ?
There was a problem hiding this comment.
There is no security issue when using this configuration setting. The only thing is that it is not as flexible as the new way.
There was a problem hiding this comment.
It might be a security vulnerability if the webserver socket access isn't restricted to the proxy IP, as any client would then be able to fake a client IP, wasn't this case the reason of the method change?
There was a problem hiding this comment.
I don't understand your last message, but there are no security issues. When calling trustProxyData(), we now just assume that are trusting the last proxy, and as such we return the last value of the x-forwarded-for header.
There was a problem hiding this comment.
For example if one read the cookbook entry about varnish and decides to
install it, he will typically end up with apache listening to port 8080
for eg. and varnish to 80. But if he didn't put any concern about
security at system level, apache socket could be publicly accessible,
thus making any client being able to fake user's IP by requesting
"host:8080" with the forwarded-for header, this wouldn't be possible
with the new method.
There was a problem hiding this comment.
Why are you talking about sockets here? I don't get it. But anyways, the old method is safe as this is just a shortcut for setTrustedProxies(array($request->server->get('REMOTE_ADDR')).
|
Well I don't know why it fails on travis, I can't run the full test suite locally because of a segfault but |
|
But it looks like the failing tests come from what you've changed. |
|
Yes, I'm not saying it's not my fault but I can't reproduce this as locally it tells me they pass, I'll try to fix this this evening. |
|
Apparently it fails only when running the whole testsuite, looking at other travis builds I can see this one on 2.0 : https://travis-ci.org/symfony/symfony/jobs/3495511 which fails in a similar way than here (https://travis-ci.org/symfony/symfony/jobs/3530928). Because of a place trying to access an undefined $_SERVER key : I'd be happy if someone could give me some pointers in here as I don't have any clue about how to fix this.. |
|
As a consulsion I'd say I can't run the whole testsuite locally (it fails even when I revert my commit), so there is no reliable way for me to fix this, if anyone is up for continuing this feel free. |
|
@bamarni Can you just update this PR with the code change and no tests at all? I will then finish the PR. Thanks. |
|
@fabpot: thanks for helping me out on this, hope you won't run into the same issue! |
[FrameworkBundle] Added tests for trusted_proxies configuration.
This PR was merged into the 2.0 branch. Commits ------- f0743b1 Merge pull request #1 from pylebecq/2.0 555e777 [FrameworkBundle] Added tests for trusted_proxies configuration. a0e2391 [FrameworkBundle] used the new method for trusted proxies Discussion ---------- [FrameworkBundle] used the new method for trusted proxies This makes the framework bundle using the new method from the request class. --------------------------------------------------------------------------- by fabpot at 2012-12-05T10:38:20Z As this is a sensitive issue, can you add some tests? Thanks. --------------------------------------------------------------------------- by bamarni at 2012-12-06T13:00:24Z Well I don't know why it fails on travis, I can't run the full test suite locally because of a segfault but ```phpunit src/Symfony/Bundle/``` marks all the tests as passing. --------------------------------------------------------------------------- by fabpot at 2012-12-06T13:08:11Z But it looks like the failing tests come from what you've changed. --------------------------------------------------------------------------- by bamarni at 2012-12-06T13:29:33Z Yes, I'm not saying it's not my fault but I can't reproduce this as locally it tells me they pass, I'll try to fix this this evening. --------------------------------------------------------------------------- by bamarni at 2012-12-06T17:49:28Z Apparently it fails only when running the whole testsuite, looking at other travis builds I can see this one on 2.0 : https://travis-ci.org/symfony/symfony/jobs/3495511 which fails in a similar way than here (https://travis-ci.org/symfony/symfony/jobs/3530928). Because of a place trying to access an undefined $_SERVER key : ```PHP Notice: Undefined index: SCRIPT_NAME ...``` but I can't find where, and the stack trace references some phpunit classes. I'd be happy if someone could give me some pointers in here as I don't have any clue about how to fix this.. --------------------------------------------------------------------------- by bamarni at 2012-12-06T18:00:57Z As a consulsion I'd say I can't run the whole testsuite locally (it fails even when I revert my commit), so there is no reliable way for me to fix this, if anyone is up for continuing this feel free. --------------------------------------------------------------------------- by fabpot at 2012-12-11T09:47:48Z @bamarni Can you just update this PR with the code change and no tests at all? I will then finish the PR. Thanks. --------------------------------------------------------------------------- by bamarni at 2012-12-11T16:58:17Z @fabpot: thanks for helping me out on this, hope you won't run into the same issue!
This makes the framework bundle using the new method from the request class.