Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] mitigate BC break with empty trusted_proxies#23049
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
xabbuh commentedJun 3, 2017
| 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 |
syrm commentedJun 3, 2017
One step closer to respect semver :-) |
| ->arrayNode('trusted_proxies')// @deprecated in version 3.3, to be removed in 4.0 | ||
| ->beforeNormalization() | ||
| ->always() | ||
| ->ifTrue(function ($v) {returnnull ===$v ||array() ===$v; }) |
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.
if (empty($v)) ? would be aligned with if !empty below, isn't it?
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.
indeed
nicolas-grekas left a comment
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.
👍
sstok commentedJun 3, 2017
Looks good to me 👍 maybe add an exception-message expectation? |
fabpot commentedJun 3, 2017
Thank you@xabbuh. |
…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
… 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