Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it#21830
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
ba36fe8 to306cc67Comparenicolas-grekas commentedMar 10, 2017
PR scope now focused on adding the second argument only. Ready. Status: needs review |
306cc67 toa2bb870Compare| * @throws \InvalidArgumentException When $trustedHeader is invalid | ||
| */ | ||
| publicstaticfunctionsetTrustedProxies(array$proxies) | ||
| publicstaticfunctionsetTrustedProxies(array$proxies/*, string $trustedHeader*/) |
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 guess the comment is a mistake ?
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.
it's a reminder, for it to be uncommented in 4.0, as done in such cases usually already
a2bb870 tod89b2acComparenicolas-grekas commentedMar 11, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
724fb7a to1539245Comparenicolas-grekas commentedMar 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Status: needs review PR ready, with more deprecations, namely:
|
1539245 to072fe39Compare| or to`Request::HEADER_X_FORWARDED_ALL` if it is using`X-Forwarded-*` headers instead. | ||
| * The`Request::setTrustedHeaderName()` and`Request::getTrustedHeaderName()` methods are deprecated, | ||
| use the RFC7239`Forwarded` header, or the`X-Forwarded-*` headers instead. |
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.
Not sure, but IIRC, it was introduced because some proxies/setups use non-standard headers.
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.
Looking at Express, the values are hardcoded:
http://expressjs.com/en/guide/behind-proxies.html
https://github.com/expressjs/express/blob/master/lib/request.js
I guess we're safe not supporting any fancy names.
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.
The default names are non-standard, but widely used by popular reverse proxies (like Apache mod_proxy or Amazon EC2).. So, I think we should check if this is still the case or not.
nicolas-grekasMar 19, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
As far as I looked correctly, I'd rephrase this to "The default names are non-standardized" - because they are the defacto standard in fact. Can anyone find any other name in use in place ofX-Forwarded-*, with the same semantic?
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.
Apparently, Apache mod_proxy and Amazon EC2 use different names.
| * | ||
| * @param array $proxies A list of trusted proxies | ||
| * @param array $proxies A list of trusted proxies | ||
| * @param int $trustedHeaderSet A bit field of Request::HEADER_*, usually either Request::HEADER_FORWARDED or Request::HEADER_X_FORWARDED_ALL, to set which headers to trust from your proxies |
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 means that you can trust theForwarded and theX-Forwarded-For headers at the same time. I don't think that this is desirable.
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.
Yes, I think it's still legitimate to support that, at least for BC.
This would be really opt-in anyway, low chance anyone will do it be mistake to me.
072fe39 to309d76dCompareUPGRADE-3.3.md Outdated
| * The`cache:clear` command should always be called with the`--no-warmup` option. | ||
| Warmup should be done via the`cache:warmup` command. | ||
| * The "framework.trusted_proxies configuration option and the corresponding "kernel.trusted_proxies" parameter have been deprecated and will be removed in 4.0. Use the Request::setTrustedProxies() method in your front controller instead. |
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.
missing '"' after 'framework.trusted_proxies'
309d76d to0f83fd9Comparefabpot commentedMar 22, 2017
👍 |
…ies() - deprecate not setting it
0f83fd9 tod3c9604Comparefabpot commentedMar 22, 2017
Thank you@nicolas-grekas. |
…:setTrustedProxies() - deprecate not setting it (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | no tests yet| Fixed tickets | -| License | MIT| Doc PR | -Follow up of#18688PR adds a second `$trustedHeaderSet` argument to `Request::setTrustedProxies()`, can be either `Request::HEADER_FORWARDED` or `Request::HEADER_X_FORWARDED_ALL` to set which header to trust from your proxies - the idea being that without this info, one will get some `ConflictingHeadersException`, but those may be lost in the logs.Commits-------d3c9604 [HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it
weaverryan commentedMar 23, 2017
Don't forget about the docs please! I just opened an issue on the docs about this - our system of always making sure at least a docs issue is opened for changes is really important! It doesn't need to be a PR - we can take care of it :) |
dzuelke commentedMar 30, 2017
Bit late to the game, but I'd like to point out#20178, which is related, and should maybe be addressed together with this. TL;DR: out of the box, Symfony should not trust any headers, even when setting a trusted proxy; the list should always be opt-in, as people are vulnerable to header forgery otherwise. AWS ELBsdo not set an Poking@fabpot and@nicolas-grekas :) |
dzuelke commentedMar 30, 2017
Also seesymfony/symfony-docs#7045 |
…) takes a new required $trustedHeaderSet argument (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | yes| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#20178| License | MIT| Doc PR | -As discussed in linked issue, and already deprecated by#21830Commits-------72e2895 [BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument
As deprecated insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)
This PR was merged into the master branch.Discussion----------Remove framework.trusted_proxies optionAs deprecated and "removed" insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)Commits-------2507e41 Remove framework.trusted_proxies option
As deprecated insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)
This PR was merged into the master branch.Discussion----------Remove framework.trusted_proxies optionAs deprecated and "removed" insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)Commits-------2507e41 Remove framework.trusted_proxies option
As deprecated insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)
This PR was merged into the master branch.Discussion----------Remove framework.trusted_proxies optionAs deprecated and "removed" insymfony/symfony#21830 andsymfony/symfony#22238 (BC break)Commits-------2507e41 Remove framework.trusted_proxies option
Uh oh!
There was an error while loading.Please reload this page.
Follow up of#18688
PR adds a second
$trustedHeaderSetargument toRequest::setTrustedProxies(), can be eitherRequest::HEADER_FORWARDEDorRequest::HEADER_X_FORWARDED_ALLto set which header to trust from your proxies - the idea being that without this info, one will get someConflictingHeadersException, but those may be lost in the logs.