From 7cf4dd69171555c59d3d0fe85e6734cd5106a630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Sun, 1 Nov 2020 22:31:33 +0100 Subject: [PATCH] Deprecate HEADER_X_FORWARDED_ALL constant --- UPGRADE-5.2.md | 1 + UPGRADE-6.0.md | 1 + .../Tests/Processor/WebProcessorTest.php | 2 +- .../DependencyInjection/Configuration.php | 3 +- .../FrameworkExtension.php | 7 ---- .../DependencyInjection/ConfigurationTest.php | 6 +-- .../Component/HttpFoundation/CHANGELOG.md | 2 + .../Component/HttpFoundation/Request.php | 10 +++-- .../HttpFoundation/Tests/RequestTest.php | 42 ++++++++++++++----- .../Tests/HttpCache/HttpCacheTest.php | 2 +- 10 files changed, 48 insertions(+), 28 deletions(-) diff --git a/UPGRADE-5.2.md b/UPGRADE-5.2.md index 6ce95580c3..bc6c4b69bf 100644 --- a/UPGRADE-5.2.md +++ b/UPGRADE-5.2.md @@ -43,6 +43,7 @@ HttpFoundation -------------- * Deprecated not passing a `Closure` together with `FILTER_CALLBACK` to `ParameterBag::filter()`; wrap your filter in a closure instead. + * Deprecated the `Request::HEADER_X_FORWARDED_ALL` constant, use either `Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO` or `Request::HEADER_X_FORWARDED_AWS_ELB` or `Request::HEADER_X_FORWARDED_TRAEFIK`constants instead. Lock ---- diff --git a/UPGRADE-6.0.md b/UPGRADE-6.0.md index 5e14f6c9ab..abeb497d9c 100644 --- a/UPGRADE-6.0.md +++ b/UPGRADE-6.0.md @@ -67,6 +67,7 @@ HttpFoundation `RedirectResponse::create()`, and `StreamedResponse::create()` methods (use `__construct()` instead) * Not passing a `Closure` together with `FILTER_CALLBACK` to `ParameterBag::filter()` throws an `InvalidArgumentException`; wrap your filter in a closure instead. + * Removed the `Request::HEADER_X_FORWARDED_ALL` constant, use either `Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO` or `Request::HEADER_X_FORWARDED_AWS_ELB` or `Request::HEADER_X_FORWARDED_TRAEFIK`constants instead. HttpKernel ---------- diff --git a/src/Symfony/Bridge/Monolog/Tests/Processor/WebProcessorTest.php b/src/Symfony/Bridge/Monolog/Tests/Processor/WebProcessorTest.php index 24aed73095..628facbb21 100644 --- a/src/Symfony/Bridge/Monolog/Tests/Processor/WebProcessorTest.php +++ b/src/Symfony/Bridge/Monolog/Tests/Processor/WebProcessorTest.php @@ -38,7 +38,7 @@ class WebProcessorTest extends TestCase public function testUseRequestClientIp() { - Request::setTrustedProxies(['192.168.0.1'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['192.168.0.1'], Request::HEADER_X_FORWARDED_FOR); [$event, $server] = $this->createRequestEvent(['X_FORWARDED_FOR' => '192.168.0.2']); $processor = new WebProcessor(); diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index 717495d053..3460470622 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php @@ -92,13 +92,12 @@ class Configuration implements ConfigurationInterface ->arrayNode('trusted_headers') ->fixXmlConfig('trusted_header') ->performNoDeepMerging() - ->defaultValue(['x-forwarded-all', '!x-forwarded-host', '!x-forwarded-prefix']) + ->defaultValue(['x-forwarded-for', 'x-forwarded-port', 'x-forwarded-proto']) ->beforeNormalization()->ifString()->then(function ($v) { return $v ? array_map('trim', explode(',', $v)) : []; })->end() ->enumPrototype() ->values([ 'forwarded', 'x-forwarded-for', 'x-forwarded-host', 'x-forwarded-proto', 'x-forwarded-port', - 'x-forwarded-all', '!x-forwarded-host', '!x-forwarded-prefix', ]) ->end() ->end() diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index c15c5d97e7..dd5b759879 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -2294,13 +2294,6 @@ class FrameworkExtension extends Extension case 'x-forwarded-host': $trustedHeaders |= Request::HEADER_X_FORWARDED_HOST; break; case 'x-forwarded-proto': $trustedHeaders |= Request::HEADER_X_FORWARDED_PROTO; break; case 'x-forwarded-port': $trustedHeaders |= Request::HEADER_X_FORWARDED_PORT; break; - case '!x-forwarded-host': $trustedHeaders &= ~Request::HEADER_X_FORWARDED_HOST; break; - case 'x-forwarded-all': - if (!\in_array('!x-forwarded-prefix', $headers)) { - throw new LogicException('When using "x-forwarded-all" in "framework.trusted_headers", "!x-forwarded-prefix" must be explicitly listed until support for X-Forwarded-Prefix is implemented.'); - } - $trustedHeaders |= Request::HEADER_X_FORWARDED_ALL; - break; } } diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php index 6bc6d958b7..8dd31fb7d0 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php +++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php @@ -341,9 +341,9 @@ class ConfigurationTest extends TestCase 'secret' => 's3cr3t', 'trusted_hosts' => [], 'trusted_headers' => [ - 'x-forwarded-all', - '!x-forwarded-host', - '!x-forwarded-prefix', + 'x-forwarded-for', + 'x-forwarded-port', + 'x-forwarded-proto', ], 'csrf_protection' => [ 'enabled' => false, diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md index 49220fb318..fef21b8723 100644 --- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md +++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md @@ -11,6 +11,8 @@ CHANGELOG * added `Request::toArray()` to parse a JSON request body to an array * added `RateLimiter\RequestRateLimiterInterface` and `RateLimiter\AbstractRequestRateLimiter` * deprecated not passing a `Closure` together with `FILTER_CALLBACK` to `ParameterBag::filter()`; wrap your filter in a closure instead. + * Deprecated the `Request::HEADER_X_FORWARDED_ALL` constant, use either `HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PORT | HEADER_X_FORWARDED_PROTO` or `HEADER_X_FORWARDED_AWS_ELB` or `HEADER_X_FORWARDED_TRAEFIK` constants instead. + 5.1.0 ----- diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 4b0696a4e6..34cd492465 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -47,9 +47,10 @@ class Request const HEADER_X_FORWARDED_PORT = 0b010000; const HEADER_X_FORWARDED_PREFIX = 0b100000; - const HEADER_X_FORWARDED_ALL = 0b011110; // All "X-Forwarded-*" headers sent by "usual" reverse proxy - const HEADER_X_FORWARDED_AWS_ELB = 0b011010; // AWS ELB doesn't send X-Forwarded-Host - const HEADER_X_FORWARDED_TRAEFIK = 0b111110; // All "X-Forwarded-*" headers sent by Traefik reverse proxy + /** @deprecated since Symfony 5.2, use either "HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PORT | HEADER_X_FORWARDED_PROTO" or "HEADER_X_FORWARDED_AWS_ELB" or "HEADER_X_FORWARDED_TRAEFIK" constants instead. */ + const HEADER_X_FORWARDED_ALL = 0b1011110; // All "X-Forwarded-*" headers sent by "usual" reverse proxy + const HEADER_X_FORWARDED_AWS_ELB = 0b0011010; // AWS ELB doesn't send X-Forwarded-Host + const HEADER_X_FORWARDED_TRAEFIK = 0b0111110; // All "X-Forwarded-*" headers sent by Traefik reverse proxy const METHOD_HEAD = 'HEAD'; const METHOD_GET = 'GET'; @@ -593,6 +594,9 @@ class Request */ public static function setTrustedProxies(array $proxies, int $trustedHeaderSet) { + if (self::HEADER_X_FORWARDED_ALL === $trustedHeaderSet) { + trigger_deprecation('symfony/http-fundation', '5.2', 'The "HEADER_X_FORWARDED_ALL" constant is deprecated, use either "HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PORT | HEADER_X_FORWARDED_PROTO" or "HEADER_X_FORWARDED_AWS_ELB" or "HEADER_X_FORWARDED_TRAEFIK" constants instead.'); + } self::$trustedProxies = array_reduce($proxies, function ($proxies, $proxy) { if ('REMOTE_ADDR' !== $proxy) { $proxies[] = $proxy; diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php index 5d466afe25..6b01446c2f 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php @@ -12,6 +12,7 @@ namespace Symfony\Component\HttpFoundation\Tests; use PHPUnit\Framework\TestCase; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\HttpFoundation\Exception\JsonException; use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException; use Symfony\Component\HttpFoundation\InputBag; @@ -22,6 +23,8 @@ use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage; class RequestTest extends TestCase { + use ExpectDeprecationTrait; + protected function tearDown(): void { Request::setTrustedProxies([], -1); @@ -867,7 +870,7 @@ class RequestTest extends TestCase $this->assertEquals(80, $port, 'Without trusted proxies FORWARDED_PROTO and FORWARDED_PORT are ignored.'); - Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_PROTO | Request::HEADER_X_FORWARDED_PORT); $request = Request::create('http://example.com', 'GET', [], [], [], [ 'HTTP_X_FORWARDED_PROTO' => 'https', 'HTTP_X_FORWARDED_PORT' => '8443', @@ -1091,7 +1094,7 @@ class RequestTest extends TestCase 'HTTP_X_FORWARDED_FOR' => $httpXForwardedFor, ]; - Request::setTrustedProxies(['88.88.88.88'], Request::HEADER_X_FORWARDED_ALL | Request::HEADER_FORWARDED); + Request::setTrustedProxies(['88.88.88.88'], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_FORWARDED); $request->initialize([], [], [], [], [], $server); @@ -1349,7 +1352,7 @@ class RequestTest extends TestCase $request->headers->set('X_FORWARDED_PROTO', 'https'); - Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_PROTO); $this->assertFalse($request->isSecure()); $request->server->set('REMOTE_ADDR', '1.1.1.1'); $this->assertTrue($request->isSecure()); @@ -1830,7 +1833,7 @@ class RequestTest extends TestCase } if ($trustedProxies) { - Request::setTrustedProxies($trustedProxies, Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies($trustedProxies, Request::HEADER_X_FORWARDED_FOR); } $request->initialize([], [], [], [], [], $server); @@ -1873,35 +1876,35 @@ class RequestTest extends TestCase $this->assertFalse($request->isSecure()); // disabling proxy trusting - Request::setTrustedProxies([], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies([], Request::HEADER_X_FORWARDED_FOR); $this->assertEquals('3.3.3.3', $request->getClientIp()); $this->assertEquals('example.com', $request->getHost()); $this->assertEquals(80, $request->getPort()); $this->assertFalse($request->isSecure()); // request is forwarded by a non-trusted proxy - Request::setTrustedProxies(['2.2.2.2'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['2.2.2.2'], Request::HEADER_X_FORWARDED_FOR); $this->assertEquals('3.3.3.3', $request->getClientIp()); $this->assertEquals('example.com', $request->getHost()); $this->assertEquals(80, $request->getPort()); $this->assertFalse($request->isSecure()); // trusted proxy via setTrustedProxies() - Request::setTrustedProxies(['3.3.3.3', '2.2.2.2'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['3.3.3.3', '2.2.2.2'], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO); $this->assertEquals('1.1.1.1', $request->getClientIp()); $this->assertEquals('foo.example.com', $request->getHost()); $this->assertEquals(443, $request->getPort()); $this->assertTrue($request->isSecure()); // trusted proxy via setTrustedProxies() - Request::setTrustedProxies(['3.3.3.4', '2.2.2.2'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['3.3.3.4', '2.2.2.2'], Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_HOST | Request::HEADER_X_FORWARDED_PORT | Request::HEADER_X_FORWARDED_PROTO); $this->assertEquals('3.3.3.3', $request->getClientIp()); $this->assertEquals('example.com', $request->getHost()); $this->assertEquals(80, $request->getPort()); $this->assertFalse($request->isSecure()); // check various X_FORWARDED_PROTO header values - Request::setTrustedProxies(['3.3.3.3', '2.2.2.2'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['3.3.3.3', '2.2.2.2'], Request::HEADER_X_FORWARDED_PROTO); $request->headers->set('X_FORWARDED_PROTO', 'ssl'); $this->assertTrue($request->isSecure()); @@ -2377,7 +2380,7 @@ class RequestTest extends TestCase public function testTrustedPortDoesNotDefaultToZero() { - Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies(['1.1.1.1'], Request::HEADER_X_FORWARDED_FOR); $request = Request::create('/'); $request->server->set('REMOTE_ADDR', '1.1.1.1'); @@ -2393,7 +2396,7 @@ class RequestTest extends TestCase public function testTrustedProxiesRemoteAddr($serverRemoteAddr, $trustedProxies, $result) { $_SERVER['REMOTE_ADDR'] = $serverRemoteAddr; - Request::setTrustedProxies($trustedProxies, Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies($trustedProxies, Request::HEADER_X_FORWARDED_FOR); $this->assertSame($result, Request::getTrustedProxies()); } @@ -2464,6 +2467,23 @@ class RequestTest extends TestCase ], ]; } + + /** + * @group legacy + */ + public function testXForwarededAllConstantDeprecated() + { + $this->expectDeprecation('Since symfony/http-fundation 5.2: The "HEADER_X_FORWARDED_ALL" constant is deprecated, use either "HEADER_X_FORWARDED_FOR | HEADER_X_FORWARDED_HOST | HEADER_X_FORWARDED_PORT | HEADER_X_FORWARDED_PROTO" or "HEADER_X_FORWARDED_AWS_ELB" or "HEADER_X_FORWARDED_TRAEFIK" constants instead.'); + + Request::setTrustedProxies([], Request::HEADER_X_FORWARDED_ALL); + } + + public function testReservedFlags() + { + foreach ((new \ReflectionClass(Request::class))->getConstants() as $constant => $value) { + $this->assertNotSame(0b10000000, $value, sprintf('The constant "%s" should not use the reserved value "0b10000000".', $constant)); + } + } } class RequestContentProxy extends Request diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php index 6ad64e4791..0f0a25ef1c 100644 --- a/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php @@ -1361,7 +1361,7 @@ class HttpCacheTest extends HttpCacheTestCase */ public function testHttpCacheIsSetAsATrustedProxy(array $existing) { - Request::setTrustedProxies($existing, Request::HEADER_X_FORWARDED_ALL); + Request::setTrustedProxies($existing, Request::HEADER_X_FORWARDED_FOR); $this->setNextResponse(); $this->request('GET', '/', ['REMOTE_ADDR' => '10.0.0.1']);