Skip to content
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

[FrameworkBundle] mitigate BC break with empty trusted_proxies #23049

Merged
merged 1 commit into from Jun 3, 2017

Conversation

@xabbuh
Copy link
Member

commented Jun 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

This comment has been minimized.

Copy link

commented Jun 3, 2017

One step closer to respect semver :-)

@@ -67,7 +67,11 @@ public function getConfigTreeBuilder()
->end()
->arrayNode('trusted_proxies') // @deprecated in version 3.3, to be removed in 4.0
->beforeNormalization()
->always()
->ifTrue(function ($v) { return null === $v || array() === $v; })

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 3, 2017

Member

if (empty($v)) ? would be aligned with if !empty below, isn't it?

This comment has been minimized.

Copy link
@xabbuh

xabbuh Jun 3, 2017

Author Member

indeed

@xabbuh xabbuh force-pushed the xabbuh:issue-22238 branch from 714569e to ff055ef Jun 3, 2017

@nicolas-grekas
Copy link
Member

left a comment

👍

@sstok

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2017

Looks good to me 👍 maybe add an exception-message expectation?

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 3, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit ff055ef into symfony:3.3 Jun 3, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
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 3, 2017
bug #23049 [FrameworkBundle] mitigate BC break with empty trusted_pro…
…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

@xabbuh xabbuh deleted the xabbuh:issue-22238 branch Jun 3, 2017

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
@fabpot fabpot referenced this pull request Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.