[HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it #21830
Conversation
ba36fe8 to
306cc67
Compare
|
PR scope now focused on adding the second argument only. Ready. Status: needs review |
306cc67 to
a2bb870
Compare
| */ | ||
| public static function setTrustedProxies(array $proxies) | ||
| public static function setTrustedProxies(array $proxies/*, string $trustedHeader*/) |
There was a problem hiding this comment.
I guess the comment is a mistake ?
There was a problem hiding this comment.
it's a reminder, for it to be uncommented in 4.0, as done in such cases usually already
a2bb870 to
d89b2ac
Compare
|
|
724fb7a to
1539245
Compare
|
Status: needs review PR ready, with more deprecations, namely:
|
1539245 to
072fe39
Compare
| 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.
Not sure, but IIRC, it was introduced because some proxies/setups use non-standard headers.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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 of X-Forwarded-*, with the same semantic?
There was a problem hiding this comment.
Apparently, Apache mod_proxy and Amazon EC2 use different names.
| @@ -548,11 +571,26 @@ public function overrideGlobals() | |||
| * | |||
| * You should only list the reverse proxies that you manage directly. | |||
| * | |||
| * @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.
I means that you can trust the Forwarded and the X-Forwarded-For headers at the same time. I don't think that this is desirable.
There was a problem hiding this comment.
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 to
309d76d
Compare
UPGRADE-3.3.md
Outdated
| @@ -126,6 +126,9 @@ FrameworkBundle | |||
| * 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.
missing '"' after 'framework.trusted_proxies'
309d76d to
0f83fd9
Compare
|
👍 |
…ies() - deprecate not setting it
0f83fd9 to
d3c9604
Compare
|
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 #18688 PR 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
|
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 :) |
|
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 ELBs do not set an Poking @fabpot and @nicolas-grekas :) |
|
Also see symfony/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 #21830 Commits ------- 72e2895 [BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument
As deprecated in symfony/symfony#21830 and symfony/symfony#22238 (BC break)
This PR was merged into the master branch. Discussion ---------- Remove framework.trusted_proxies option As deprecated and "removed" in symfony/symfony#21830 and symfony/symfony#22238 (BC break) Commits ------- 2507e41 Remove framework.trusted_proxies option
As deprecated in symfony/symfony#21830 and symfony/symfony#22238 (BC break)
This PR was merged into the master branch. Discussion ---------- Remove framework.trusted_proxies option As deprecated and "removed" in symfony/symfony#21830 and symfony/symfony#22238 (BC break) Commits ------- 2507e41 Remove framework.trusted_proxies option
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.