Skip to content

[HttpFundation][FrameworkBundle] Deprecate the HEADER_X_FORWARDED_ALL constant #38954

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

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Nov 1, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? yes
Tickets -
License MIT
Doc PR TODO

The HEADER_X_FORWARDED_ALL implicitly trust the x-forwarded-host header, leading to possible host header attack (as warned in the documentation.)

Moreover, this HEADER_X_FORWARDED_ALL does not really fowards all headers, as ti does not supports X-Forwarded-Prefix headers.

This PR deprecate the constant and the new framework bundle configuration. It will be removed in 6.0. People have to use: either:

  • Request::setTrustedProxies(['1.2.3.4'], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO);
  • Request::setTrustedProxies(['1.2.3.4'], Request::HEADER_X_FORWARDED_TRAEFIK);
  • framework.trusted_headers: [x-forwarded-for, x-forwarded-host, x-forwarded-port, x-forwarded-proto]

@jderusse jderusse added this to the 5.x milestone Nov 1, 2020
@jderusse jderusse force-pushed the deprecat-xforwardedall branch 2 times, most recently from 8be58b8 to 8a4eb65 Compare November 1, 2020 21:33
@jderusse jderusse force-pushed the deprecat-xforwardedall branch from 1ae32f9 to a96c0bb Compare November 2, 2020 09:57
@jderusse jderusse force-pushed the deprecat-xforwardedall branch from a96c0bb to fc921b1 Compare November 2, 2020 12:39
@jderusse jderusse changed the title Deprecate the HEADER_X_FORWARDED_ALL constant [HttpFundation] Deprecate the HEADER_X_FORWARDED_ALL constant Nov 2, 2020
@jderusse jderusse changed the title [HttpFundation] Deprecate the HEADER_X_FORWARDED_ALL constant [HttpFundation][FrameworkBundle] Deprecate the HEADER_X_FORWARDED_ALL constant Nov 2, 2020
@jderusse jderusse force-pushed the deprecat-xforwardedall branch from fc921b1 to 5baba3d Compare November 2, 2020 12:46
@derrabus derrabus modified the milestones: 5.x, 5.2 Nov 2, 2020
@jderusse jderusse force-pushed the deprecat-xforwardedall branch from 5baba3d to fbea094 Compare November 2, 2020 15:11
@jderusse jderusse force-pushed the deprecat-xforwardedall branch 4 times, most recently from ea9ff44 to fe344eb Compare November 2, 2020 15:57
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.

(with some remaining nitpicking)

@jderusse jderusse force-pushed the deprecat-xforwardedall branch from fe344eb to 7cf4dd6 Compare November 2, 2020 16:16
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 2, 2020

Can you please submit recipe PR to not use the constant in any version?

@fabpot
Copy link
Member

fabpot commented Nov 4, 2020

Thank you @jderusse.

@fabpot fabpot merged commit 6251c4e into symfony:5.x Nov 4, 2020
@jderusse jderusse deleted the deprecat-xforwardedall branch November 4, 2020 17:44
@fabpot fabpot mentioned this pull request Nov 10, 2020
LukeTowers added a commit to wintercms/storm that referenced this pull request Dec 9, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants