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

[HttpKernel] don't call getTrustedHeaderName() if possible #22873

Merged
merged 1 commit into from May 25, 2017

Conversation

Projects
None yet
4 participants
@xabbuh
Member

xabbuh commented May 23, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets https://travis-ci.org/symfony/symfony/jobs/235008102 (failing tests of #22863)
License MIT
Doc PR
Show outdated Hide outdated src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php
$currentXForwardedFor = $request->headers->get($trustedHeaderName, '');
$server['HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
} elseif (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {

This comment has been minimized.

@stof

stof May 23, 2017

Member

Actually, the non-deprecated API should be tried first, to use it when it is available IMO

@stof

stof May 23, 2017

Member

Actually, the non-deprecated API should be tried first, to use it when it is available IMO

This comment has been minimized.

@xabbuh

xabbuh May 23, 2017

Member

In fact, I think that we can just fix this in 3.3.

@xabbuh

xabbuh May 23, 2017

Member

In fact, I think that we can just fix this in 3.3.

@xabbuh xabbuh changed the base branch from 3.4 to 3.3 May 23, 2017

@xabbuh xabbuh modified the milestones: 3.3, 3.4 May 23, 2017

Show outdated Hide outdated src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php
@@ -119,7 +119,13 @@ protected function createSubRequest($uri, Request $request)
// Sub-request object will point to localhost as client ip and real client ip
// will be included into trusted header for client ip
try {
if ($trustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP, false)) {
$hasTrustedHeaderSet = method_exists(Request::class, 'getTrustedHeaderSet');

This comment has been minimized.

@stof

stof May 23, 2017

Member

I would avoid this check by bumping the min version of HttpFoundation in HttpKernel

@stof

stof May 23, 2017

Member

I would avoid this check by bumping the min version of HttpFoundation in HttpKernel

@nicolas-grekas

👍

@xabbuh xabbuh changed the title from [HttpKernel] FC with HttpFoundation 4.0 to [HttpKernel] don't call getTrustedHeaderName() if possible May 23, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 25, 2017

Member

Thank you @xabbuh.

Member

nicolas-grekas commented May 25, 2017

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 6350dab into symfony:3.3 May 25, 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

nicolas-grekas added a commit that referenced this pull request May 25, 2017

bug #22873 [HttpKernel] don't call getTrustedHeaderName() if possible…
… (xabbuh)

This PR was merged into the 3.3 branch.

Discussion
----------

[HttpKernel] don't call getTrustedHeaderName() if possible

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | https://travis-ci.org/symfony/symfony/jobs/235008102 (failing tests of #22863)
| License       | MIT
| Doc PR        |

Commits
-------

6350dab don't call getTrustedHeaderName() if possible

@xabbuh xabbuh deleted the xabbuh:http-foundation-forward-compatibility branch May 25, 2017

@fabpot fabpot referenced this pull request May 29, 2017

Merged

Release v3.3.0 #22949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment