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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break #23067

Merged
merged 1 commit into from Jun 5, 2017

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Member

nicolas-grekas commented Jun 5, 2017

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 :) )

@fabpot

fabpot approved these changes Jun 5, 2017

@nicolas-grekas nicolas-grekas added the Ready label Jun 5, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Jun 5, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Jun 5, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 2132333 into symfony:3.3 Jun 5, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 5, 2017

bug #23067 [HttpFoundation][FrameworkBundle] Revert "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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:trusted-proxies branch Jun 5, 2017

@fabpot fabpot referenced this pull request Jun 5, 2017

Merged

Release v3.3.1 #23068

@syrm

This comment has been minimized.

Show comment
Hide comment
@syrm

syrm Jun 6, 2017

Well, i was one of the most criticism guy about this BC break.
This revert is good, BUT you have no information there is a possible security flow problem.
So i think the trigger_error message should notify about that.

syrm commented Jun 6, 2017

Well, i was one of the most criticism guy about this BC break.
This revert is good, BUT you have no information there is a possible security flow problem.
So i think the trigger_error message should notify about that.

@andig

This comment has been minimized.

Show comment
Hide comment
@andig

andig Jun 6, 2017

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?

andig commented Jun 6, 2017

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?

@syrm

This comment has been minimized.

Show comment
Hide comment
@syrm

syrm Jun 6, 2017

No break again in 3.3.1 if you already applied the change for 3.3

syrm commented Jun 6, 2017

No break again in 3.3.1 if you already applied the change for 3.3

@syrm

This comment has been minimized.

Show comment
Hide comment
@syrm

syrm Jun 9, 2017

@nicolas-grekas no answer about lack of information in trigger_error about security problem ?

syrm commented Jun 9, 2017

@nicolas-grekas no answer about lack of information in trigger_error about security problem ?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jun 9, 2017

Member

I don't have anything specific to say about your suggestion. Please open a PR if you want.
Please also note that I prefer not commenting on closed PRs/issues, it's a nightmare to track.
Thanks.

Member

nicolas-grekas commented Jun 9, 2017

I don't have anything specific to say about your suggestion. Please open a PR if you want.
Please also note that I prefer not commenting on closed PRs/issues, it's a nightmare to track.
Thanks.

fabpot added a commit that referenced this pull request Jun 14, 2017

bug #23074 [HttpFoundation] add back support for legacy constant valu…
…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