[BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument #22238
Conversation
|
👍 |
| @trigger_error('The "kernel.trusted_proxies" parameter is deprecated since version 3.3 and will be removed in 4.0. Use the Request::setTrustedProxies() method in your front controller instead.', E_USER_DEPRECATED); | ||
|
|
||
| Request::setTrustedProxies($trustedProxies, Request::getTrustedHeaderSet()); | ||
| if ($this->container->hasParameter('kernel.trusted_proxies')) { |
There was a problem hiding this comment.
this will break the cache:clear command in non-debug environment when upgrading, as the existing cache (used when booting the kernel) will have this parameter defined.
5d846ad to
564cd68
Compare
…equired $trustedHeaderSet argument
564cd68 to
72e2895
Compare
|
Thank you @nicolas-grekas. |
…) 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
|
Very very bad decision to put a BC Break and not respecting semver. |
|
You made a BC Break, so you made a major version right ? According to the semver standard ?
and of course not a minor version because minor is for backwards-compatible bug fixes. According to the semver standard. |
|
You have many solution to avoid BC Break. Not respecting semver is a bad thing. |
|
@syrm the security-checker warns about using an insecure version of a library. But it won't help about building a secure version of the API |
|
It can evolve. |
|
Thank you for breaking my production, I think such I huge framework as Symfony should stick to the semver rules. |
|
But the worst thing is I get an official answer about that they gonna do that again in futur. |
|
The rules have always been very clear and have always been part of the documentation. For security issues, we can break backward compatibility. As for breaking BC in general, I like to think that we are doing a great job. Of course, from time to time, we might break BC by error. When it happens and when it's reported before the stable release, we fix it. Then, there are also cases where the difference between breaking BC and fixing a bug is not that clear. It's not black and white. It's difficult. The core team spends most of its time on this topic. Happy if you can help. |
|
As I said, even for security reason you should NEVER do a BC break. For example this security problem is only on AWS. And you break the code for everyone one using proxy. You should have put a warning on dev bar about this security problem and let each one should do what they want. |
|
I think you misunderstood the security issue, it's not AWS specific at all but can affect anyone using trusted proxies really. |
|
Only if you have bad server config |
|
Sorry for joining the party a bit late. I understand Symfony policy about security issues and I second them. However when you have: framework:
trusted_proxies: ~Which is the default in a fair number of applications, you get a hard BC break still even though there is no security threats. IMO this is being downright annoying for absolutely no reason. |
|
@theofidry I a gree, when default (null) is set it should be simple ignored. Otherwise you can throw an exception as then it shouldn't go left unnoticed. |
|
Anyone who would like to give Theo's idea a try? (The config option should then be deprecated) |
|
I just tried quickly with something like: ->arrayNode('trusted_proxies') // @deprecated in version 3.3, to be removed in 4.0
->beforeNormalization()
->always(function ($values) {
if (null === $values || array() == $values) {
return $this;
}
return $this->thenInvalid('The "framework.trusted_proxies" configuration key has been removed in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.')
})
->end()
->end()But it didn't seem to work very well. I'm not very familiar with the config and I didn't see anything on how to create a custom validation rule here. Unfortunately I cannot continue on this right now, might give it another try later during the week if I find some time. |
|
Can you give #23049 a try? |
|
|
|
My main complain is because the issue simply broke with a recommended solution that could have come as a default argument. It took me a couple of hours to first identify that the issue wasn't on my side and that after chain of dependencies it was SF's update that did it. I wonder if there is a proper way to warn people of an incoming BC, as @syrm recommended. |
…xies (xabbuh) This PR was merged into the 3.3 branch. Discussion ---------- [FrameworkBundle] mitigate BC break with empty trusted_proxies | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | kind of | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22238 (comment) | License | MIT | Doc PR | Commits ------- ff055ef mitigate BC break with empty trusted_proxies
…xies (xabbuh) This PR was merged into the 3.3 branch. Discussion ---------- [FrameworkBundle] mitigate BC break with empty trusted_proxies | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | kind of | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#22238 (comment) | License | MIT | Doc PR | Commits ------- ff055ef7f3 mitigate BC break with empty trusted_proxies
… BC break (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Basically reverts #22238 + cleanups some comments + adds missing syncing logic in setTrustedHeaderName. The reason for this proposal is that the BC break can go un-noticed until prod, *even if you have proper CI*. That's because your CI may not replicate exactly what your prod have (ie a reverse proxy), so that maybe only prod has a trusted-proxies configuration. I realized this while thinking about #23049: it made this situation even more likely, by removing an opportunity for you to notice the break before prod. The reasons for the BC break are still valid and all of this is security-related. But the core security issue is already fixed. The remaining issue still exists (an heisenbug related to some people having both Forwarded and X-Forwarded-* set for some reason), but deprecating might still be enough. WDYT? (I'm sure everyone is going to be happy with the BC break reversal, but I'm asking for feedback from people who actually could take the time to *understand* and *balance* the rationales here, thanks :) ) Commits ------- 2132333 [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break
|
For the record, this has been reverted (at least the BC break) in #23067 and will be part of the upcoming 3.3.1 release. |
|
Hello! i have a same error on a web server to install shopware.de error: it was the same error as this I think or how could I to an work around? thx |
|
@petereth it's in the issue description: https://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-symfony-3-3, or alternatively you can just update (Sf 3.1.1 and 3.1.2 have already been released) |
|
Hi @theofidry thank you really for that help, is it possible for you that you could please help for that case to fix that I do not know where to find this on https://github.com/shopware/shopware/tree/5.2/vendor - there is a symphony tree ! thank you |
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
As discussed in linked issue, and already deprecated by #21830
See http://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-symfony-3-3