[HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break #23067
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabpot
approved these changes
Jun 5, 2017
|
Thank you @nicolas-grekas. |
fabpot
added a commit
that referenced
this pull request
Jun 5, 2017
… 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
Merged
|
Well, i was one of the most criticism guy about this BC break. |
|
I'm a little confused how an intentional bc break csn be ubdone by a patch release. I've just updated requirements to 3.3 and fixed my code. 3.3.1 would essentially break that again? |
|
No break again in 3.3.1 if you already applied the change for 3.3 |
|
@nicolas-grekas no answer about lack of information in trigger_error about security problem ? |
|
I don't have anything specific to say about your suggestion. Please open a PR if you want. |
fabpot
added a commit
that referenced
this pull request
Jun 14, 2017
…es (xabbuh) This PR was merged into the 3.3 branch. Discussion ---------- [HttpFoundation] add back support for legacy constant values | 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 | Due to the data type change of the `Request::HEADER_` constants in Symfony 3.3 #23067 introduced a small BC break if someone used the old constant values statically instead of referring to the constants themselves. Commits ------- fddd754 add back support for legacy constant values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 :) )