[Symfony 4] Drupal\Core\StackMiddleware\ReverseProxyMiddleware calls Symfony\Component\HttpFoundation\Request::setTrustedHeaderName() which does not exist [#3030501]

Problem/Motivation

Found in #2976394: Allow Symfony 4.4 to be installed in Drupal 8, although Drupal\Tests\Core\StackMiddleware\ReverseProxyMiddlewareTest has an @expectedDeprecation The "Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()" method is deprecated since Symfony 3.3 and will be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead., that does not help once updated to Symfony 4 since the method is actually, effectively, absolutely not there anymore.

Proposed resolution

Do a proper deprecation of how custom header settings and replace using a new reverse_proxy_trusted_headers setting instead.

Support for the following settings in deprecated in Drupal 8:

reverse_proxy_header
reverse_proxy_proto_header
reverse_proxy_host_header
reverse_proxy_port_heade
reverse_proxy_forwarded_header

In Drupal 8 these could be set to custom header names if used by your reverse proxy. Support for this will be dropped in Drupal 9.

Additionally you could set each value to NULL to indicate that the corresponding header was not to be trusted. In Drupal 8.7.0 a new setting has been added to control this reverse_proxy_trusted_headers.

Before

$settings['reverse_proxy_host_header'] = NULL
$settings['reverse_proxy_forwarded_header'] = NULL;

After

$settings['reverse_proxy_trusted_headers'] = \Symfony\Component\HttpFoundation\Request::HEADER_X_FORWARDED_FOR | 
\Symfony\Component\HttpFoundation\Request::HEADER_X_FORWARDED_PROTO | \Symfony\Component\HttpFoundation\Request::HEADER_X_FORWARDED_PORT;

$settings['reverse_proxy_trusted_headers'] = \Symfony\Component\HttpFoundation\Request::HEADER_X_FORWARDED_AWS_ELB

Note the default value for $settings['reverse_proxy_trusted_headers'] is \Symfony\Component\HttpFoundation\Request::HEADER_X_FORWARDED_ALL | \Symfony\Component\HttpFoundation\Request::HEADER_FORWARDED. This value is the most permissive and does not make much sense as reverse proxies do not use both "X-Forwarded-*" headers and "Forwarded" header. Therefore it is recommended to set a value.

Remaining tasks

Do it.

User interface changes

None.

API changes

Don't know yet. Hopefully none.

Data model changes

None.

Release notes snippet

N/A.

Comment	File	Size	Author
#11	3030501-11.patch	13.62 KB	alexpott
#11	8.7.x: PHP 7.2 & MySQL 5.5 24,998 pass
#11	7-11-interdiff.txt	3.01 KB	alexpott
#7	3030501-7.patch	13.41 KB	alexpott
#7	8.7.x: PHP 7.2 & MySQL 5.5 24,945 pass
#7	6-7-interdiff.txt	7.83 KB	alexpott
#6	3030501-6.patch	7.37 KB	alexpott
#6	8.7.x: PHP 7.2 & MySQL 5.5 24,941 pass
#6	3-6-interdiff.txt	2.25 KB	alexpott
#4	3030501-3.patch	6.11 KB	alexpott
#4	8.7.x: PHP 7.2 & MySQL 5.5 24,941 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

4 February 2019 at 14:11

Gábor Hojtsy created an issue. See original summary.

Comment #2

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 14:22

I had a quick look at this one. Can't immediately see how to set custom trusted header names in SF4.

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 4 February 2019 at 14:36

@alexpott: According to https://symfony.com/blog/fixing-the-trusted-proxies-configuration-for-sy..., it is combined with setTrustedProxies() now, second argument?

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 14:41

Status:

Active

» Needs review

File	Size
3030501-3.patch	6.11 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,941 pass

This deprecation happened in https://github.com/symfony/symfony/pull/25623 but it was a result of https://github.com/symfony/symfony/pull/21830

It seems the ability to have custom headers has been completely removed which means this change becomes something like the patch attached.

@Berdir yeah but the custom header thing is completely gone.

Comment #5

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 14:55

So what's a bit fun is that our implement here isn't really what we want and has never been. Ideally we'd have a setting that tells Symfony\Request what to trust. Atm we do $request::setTrustedProxies($proxies, Request::HEADER_X_FORWARDED_ALL | Request::HEADER_FORWARDED); which doesn't make a great deal of sense. But at least before you could disable certain headings by doing
$settings['reverse_proxy_host_header'] = NULL in settings.php. This is important because AWS ELB doesn't send X-Forwarded-Host. So the patch in #4 is not right. We need a new setting.

Comment #6

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 15:12

Issue tags:

+Needs change record

File	Size
3-6-interdiff.txt	2.25 KB
3030501-6.patch	7.37 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,941 pass

Here's step 1 - the new setting. And then we need BC in place for the old ability to set to a NULL to not trust the header.

Comment #7

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 16:59

File	Size
6-7-interdiff.txt	7.83 KB
3030501-7.patch	13.41 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,945 pass

And here's a BC layer which supports the current behaviour and tells people to move to the new reverse_proxy_trusted_headers setting instead.

Comment #8

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 17:00

This issue is a bit tricky because we didn't really handle the deprecation correctly in #2927746: Update Symfony components to 3.4.*

Comment #9

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 17:15

Issue summary:

View changes

Comment #10

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 13 February 2019 at 10:10

Status:	Needs review	» Needs work
Issue tags:	-Needs change record

So the concept basically is that the deprecated setting is not to be used after updating to Symfony 4 on an individual site. That sounds fine. Added that to the change notice draft and fixed up some wording, especially the title: https://www.drupal.org/node/3030558/revisions/view/11285892/11298269

Found these super-minor things:

+++ b/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php
@@ -58,24 +58,35 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
+        // Set the default value. note that this is the most relaxed setting
+        // possible and not recommended.

"Note" should be uppercase.

+++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
@@ -47,30 +49,83 @@ public function testNoProxy() {
+        // For AWS configuration forwarded and x_forwarded_host headers are not
+        // trusted

Period missing at the end of sentence.

+++ b/sites/default/default.settings.php
@@ -365,34 +364,31 @@
+ * Specify which headers the reserve proxy uses.

This sounds odd to me. Maybe:

Specify which headers are used by the reserve proxy.

Comment #11

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 13 February 2019 at 14:25

Status:

Needs work

» Needs review

File	Size
7-11-interdiff.txt	3.01 KB
3030501-11.patch	13.62 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,998 pass

Addressed #10. I went a little bit further on .3 and made it more like what is there for other settings.

Comment #12

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 13 February 2019 at 15:00

Status:

Needs review

» Reviewed & tested by the community

Looks good to me now.

Comment #13

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 14 February 2019 at 09:03

Status:

Reviewed & tested by the community

» Fixed

Committed 2aa7c3a and pushed to 8.7.x. Thanks!

Comment #14

14 February 2019 at 09:03

larowlan committed 2aa7c3a on 8.7.x

Issue #3030501 by alexpott, Gábor Hojtsy, Berdir: [Symfony 4] Drupal\...

Comment #15

28 February 2019 at 09:04

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Comment #16

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 6 March 2019 at 17:32

Hm, #2976394-144: Allow Symfony 4.4 to be installed in Drupal 8 still fails with

1) Drupal\Tests\Core\StackMiddleware\ReverseProxyMiddlewareTest::testReverseProxyEnabledLegacy with data set "Proxy with deprecated custom headers" (array(array('127.0.0.2', '127.0.0.3'), null, null), 26, array('The 'reverse_proxy_host_heade...030558', 'The 'reverse_proxy_forwarded_...030558', 'The "Symfony\Component\HttpFo...stead.'))
Error: Call to undefined method Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()

/var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:78
/var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:46
/var/www/html/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php:151
/var/www/html/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php:95

2) Drupal\Tests\Core\StackMiddleware\ReverseProxyMiddlewareTest::testReverseProxyEnabledLegacy with data set "Proxy with deprecated custom header" (array(array('127.0.0.2', '127.0.0.3'), null), 30, array('The 'reverse_proxy_forwarded_...030558', 'The "Symfony\Component\HttpFo...stead.'))
Error: Call to undefined method Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()

/var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:78
/var/www/html/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:46
/var/www/html/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php:151
/var/www/html/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php:95

Can we figure out a way to conditionally test it so it does not fail with Symfony 4.