Skip to content

[HttpFoundation] add support for X_FORWARDED_PREFIX header #37734

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
Aug 22, 2020
Merged

[HttpFoundation] add support for X_FORWARDED_PREFIX header #37734

merged 1 commit into from
Aug 22, 2020

Conversation

jeff1985
Copy link
Contributor

@jeff1985 jeff1985 commented Aug 3, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #36809
License MIT

Add support for X-Forwarded-Prefix header added by the popular Traefik HTTP LoadBalancer and Reverse Proxy. This ensures that the links rendered by symfony application deployed behind LB are valid even if the application is deployed via prefix URL.

Example routing setup:
route /admin/(.*) => symfony backend /$1
in this case links rendered by symfony backend must start with /admin/

To accept traefik prefix in your symfony app, you must modify index.php to allow accepting this header:

Request::setTrustedProxies(explode(',', $trustedProxies), Request::HEADER_X_FORWARDED_TRAEFIK ^ Request::HEADER_X_FORWARDED_HOST );`

stof
stof requested changes Aug 3, 2020
View reviewed changes
@jeff1985 jeff1985 mentioned this pull request Aug 3, 2020
Closed
@stof
Copy link
Member

stof commented Aug 3, 2020

btw, HEADER_X_FORWARDED_ALL should not be updated to whitelist the new traefik-only header, as that would make it trusted by all projects using that constant today while being behind proxies that don't know support that header, which would be a potential security issue for them (the bitmask about which headers are trusted is precisely meant to avoid a security issue).

A new HEADER_X_FORWARDED_TRAEFIK constant might be added with the bitmask of headers support by Traefik though for easy config.

@jeff1985
Copy link
Contributor Author

jeff1985 commented Aug 4, 2020

btw, HEADER_X_FORWARDED_ALL should not be updated to whitelist the new traefik-only header, as that would make it trusted by all projects using that constant today while being behind proxies that don't know support that header, which would be a potential security issue for them (the bitmask about which headers are trusted is precisely meant to avoid a security issue).

A new HEADER_X_FORWARDED_TRAEFIK constant might be added with the bitmask of headers support by Traefik though for easy config.

Agree! I added the required midifications.

@stof
Copy link
Member

stof commented Aug 4, 2020

getBasePath also needs to account for the proxy prefix

@jeff1985
Copy link
Contributor Author

jeff1985 commented Aug 4, 2020

Hi @stof,

do you have any idea, why the integration tests are failing? The error does not seem to be related to my changes.

@jeff1985
Copy link
Contributor Author

jeff1985 commented Aug 4, 2020

getBasePath also needs to account for the proxy prefix

Do you have any special condition in mind that we should take care of? I checked the code, and actually getBasePath uses getBaseUrl internally, so it should work without modifications. I added a check in the tests to make sure it works as expected.

@stof
Copy link
Member

stof commented Aug 4, 2020

hmm, I forgot that prepareBasePath uses getBaseUrl. So this might be a matter of adding an assertion in the tests to avoid regressions there, but there might indeed not need any change in the code.

@jeff1985
Copy link
Contributor Author

jeff1985 commented Aug 4, 2020

hmm, I forgot that prepareBasePath uses getBaseUrl. So this might be a matter of adding an assertion in the tests to avoid regressions there, but there might indeed not need any change in the code.

Yep, ok. The assertion in tests is already done.

@jeff1985
Copy link
Contributor Author

jeff1985 commented Aug 5, 2020

Hi @stof

as far as I see all issues are now resolved, right? Are there any todos to get this merged?

@jeff1985 jeff1985 requested a review from stof August 18, 2020 13:43
@fabpot fabpot force-pushed the header-x-forwarded-prefix branch from aea776d to 109e0a9 Compare August 22, 2020 06:37
@fabpot
Copy link
Member

fabpot commented Aug 22, 2020

Thank you @jeff1985.

@fabpot fabpot mentioned this pull request Aug 22, 2020
Closed
@fabpot fabpot merged commit c281867 into symfony:master Aug 22, 2020
maxhelias
maxhelias reviewed Aug 24, 2020
View reviewed changes
src/Symfony/Component/HttpFoundation/CHANGELOG.md
@@ -1,6 +1,11 @@
CHANGELOG
=========

5.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it rather 5.2?

@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@drupol drupol mentioned this pull request Feb 23, 2021
Merged
nicolas-grekas added a commit that referenced this pull request Feb 23, 2021
@nicolas-grekas
bug #40281 [FrameworkBundle] Allow x-forwarded-prefix trusted header …
5028aaf 
…in config (drupol)

This PR was submitted for the 5.x branch but it was merged into the 5.2 branch instead.

Discussion
----------

[FrameworkBundle] Allow x-forwarded-prefix trusted header in config

| Q             | A
| ------------- | ---
| Branch?       | 5.2 (as requested by @nicolas-grekas)
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

Support for `X_FORWARDED_PREFIX` has been added in PR #37734.

However, it is impossible to use it because the configuration doesn't allow the `x-forwarded-prefix` value in `framework.yaml`.

Commits
-------

95fdd90 Allow x-forwarded-prefix trusted header.
@Foxprodev Foxprodev mentioned this pull request Apr 5, 2021
Closed
@JohJohan JohJohan mentioned this pull request Oct 10, 2021
Merged
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 14, 2021
@javiereguiluz
minor #15910 [HttpFoundation] 14114 header option 'X-Forwarded-Prefix…
31be485 
…' (JohJohan)

This PR was submitted for the 5.2 branch but it was merged into the 5.3 branch instead.

Discussion
----------

[HttpFoundation] 14114  header option 'X-Forwarded-Prefix'

Fixes #14114.

Should i also document something about using it with traefik? `@jeff1985` you might be able to anwser that as you added the code with symfony/symfony#37734

Could a label `hacktoberfest-accepted` be added to this pull request? I am participating https://hacktoberfest.digitalocean.com/details

Commits
-------

c670141 14114 [HttpFoundation] header option 'X-Forwarded-Prefix'
LukeTowers added a commit to wintercms/storm that referenced this pull request Dec 9, 2021
@LukeTowers
Update supported proxy headers for Symfony 6.
fcecefd 
Refs:
- symfony/symfony#37734
- symfony/symfony#38954

This upgrade causes a breaking change since newly generated config files created from v1.1.4 to v1.1.8 include a default reference to `Illuminate\Http\Request::HTTP_X_FORWARDED_ALL` which no longer exists as of Laravel 9 / Symfony 6 and there is no way for us to replace that class to add it back ourselves without copying the entirety of the class into our project and class_alias()ing it, which would be a bad idea for lots of reasons.
@gguichard gguichard mentioned this pull request Jan 5, 2022
Closed
@GromNaN GromNaN mentioned this pull request Jun 14, 2022
Closed
@raskyld raskyld mentioned this pull request Oct 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxhelias maxhelias maxhelias left review comments

@fabpot fabpot fabpot approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature HttpFoundation Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support X_FORWARDED_PREFIX HTTP request header
5 participants
@jeff1985 @stof @fabpot @maxhelias @carsonbot