Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[framework-bundle] Added support for the0.0.0.0/0 trusted proxy#15706
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0.0.0.0/0 trusted proxy0.0.0.0/0 trusted proxyzerkms commentedOct 8, 2015
Guys, anyone? |
zerkms commentedNov 12, 2015
A traditional monthly reminder: guys, could someone please put some attention here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would move this above the earlierif (no need to do thestrpos() check first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh that's a fair point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh from the other hand, the'0.0.0.0/0' scenario is less likely to happen. So how about promoting the more-likely-to-happen scenario over the other?
If we move this check before theif (false !== strpos($v, '/')) { then we would have 2 checks for almost every symfony user, instead of just 1 as it is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's probably true, but as theConfiguration class won't be checked at runtime it doesn't really matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@xabbuh oh, right. Completely missed that point. It does not make much difference then. Let's see if there are more concerns here from someone else.
xabbuh commentedNov 12, 2015
looks good to me |
fabpot commentedNov 13, 2015
Out of curiosity, why would you want to do that? Is it only for development purpose? |
zerkms commentedNov 13, 2015
@fabpot it's for production: I have the following configuration:
So far so good. But as you remember the application is served via HTTPS while symfony runs behind nginx http. Thankfully the x-forwarded-proto is there but for symfony to respect it the remote ip address must be trusted. What we have: all the requests to nginx come from a trusted proxy (everything is passed through haproxy) but the remote ip address is replaced with ngx_http_realip_module. Which means that for everything to work as we expected - we need to tell symfony to trust all ip's, hence to So in my implementation it's nginx that need to know who to trust to, and symfony just need to trust anyone, since everything was already constrained by haproxy and nginx. |
fabpot commentedJan 25, 2016
Thank you@zerkms. |
…ed proxy (zerkms)This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes#15706).Discussion----------[framework-bundle] Added support for the `0.0.0.0/0` trusted proxy| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MITIt is relevant to my other PR:#14690The original intention was to start accepting `0.0.0.0/0` as a trusted proxy (which is a valid CIDR notation).The prupose is to allow all requests to be treated as trusted and to eliminate using dirty `['0.0.0.0/1', '128.0.0.0/1']` workaround.Commits-------3188e1b Added support for the `0.0.0.0/0` trusted proxy
dzuelke commentedFeb 4, 2016
Can this be backported to 2.8 please,@fabpot ? |
jakzal commentedFeb 5, 2016
@dzuelke this fix was merged into 2.3, and is already present in the 2.8 branch. |
dzuelke commentedFeb 5, 2016
Thanks@jakzal |
It is relevant to my other PR:#14690
The original intention was to start accepting
0.0.0.0/0as a trusted proxy (which is a valid CIDR notation).The prupose is to allow all requests to be treated as trusted and to eliminate using dirty
['0.0.0.0/1', '128.0.0.0/1']workaround.