Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 25, 2017
Merged

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

merged 1 commit into from
May 25, 2017

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented May 23, 2017 •
edited
Loading

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

@xabbuh xabbuh added this to the 3.4 milestone May 23, 2017
@xabbuh xabbuh mentioned this pull request May 23, 2017
Merged
stof
stof reviewed May 23, 2017
View reviewed changes
src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php Outdated
$currentXForwardedFor = $request->headers->get($trustedHeaderName, '');

$server['HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
} elseif (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xabbuh xabbuh force-pushed the http-foundation-forward-compatibility branch from cd52a80 to 29c9d8c Compare May 23, 2017 13:33
@xabbuh xabbuh changed the base branch from 3.4 to 3.3 May 23, 2017 13:34
@xabbuh xabbuh force-pushed the http-foundation-forward-compatibility branch from 29c9d8c to 0ae049b Compare May 23, 2017 13:34
@xabbuh xabbuh modified the milestones: 3.3, 3.4 May 23, 2017
@xabbuh xabbuh force-pushed the http-foundation-forward-compatibility branch from 0ae049b to ccf2275 Compare May 23, 2017 13:35
stof
stof reviewed May 23, 2017
View reviewed changes
src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php Outdated
@@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@xabbuh xabbuh force-pushed the http-foundation-forward-compatibility branch 2 times, most recently from 7e0c424 to b17d932 Compare May 23, 2017 14:17
nicolas-grekas
nicolas-grekas approved these changes May 23, 2017
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@xabbuh xabbuh force-pushed the http-foundation-forward-compatibility branch from b17d932 to 6350dab Compare May 23, 2017 18:23
@xabbuh xabbuh changed the title [HttpKernel] FC with HttpFoundation 4.0 [HttpKernel] don't call getTrustedHeaderName() if possible May 23, 2017
@nicolas-grekas
Copy link
Member

Thank you @xabbuh.

@nicolas-grekas nicolas-grekas merged commit 6350dab into symfony:3.3 May 25, 2017
nicolas-grekas added a commit that referenced this pull request May 25, 2017
@nicolas-grekas
bug #22873 [HttpKernel] don't call getTrustedHeaderName() if possible…
4e95aac 
… (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 http-foundation-forward-compatibility branch May 25, 2017 13:43
@fabpot fabpot mentioned this pull request May 29, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
4 participants
@xabbuh @nicolas-grekas @stof @carsonbot