bug #23067 [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break (nicolas-grekas)

This PR was merged into the 3.3 branch. Discussion ---------- [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Basically reverts #22238 + cleanups some comments + adds missing syncing logic in setTrustedHeaderName. The reason for this proposal is that the BC break can go un-noticed until prod, *even if you have proper CI*. That's because your CI may not replicate exactly what your prod have (ie a reverse proxy), so that maybe only prod has a trusted-proxies configuration. I realized this while thinking about #23049: it made this situation even more likely, by removing an opportunity for you to notice the break before prod. The reasons for the BC break are still valid and all of this is security-related. But the core security issue is already fixed. The remaining issue still exists (an heisenbug related to some people having both Forwarded and X-Forwarded-* set for some reason), but deprecating might still be enough. WDYT? (I'm sure everyone is going to be happy with the BC break reversal, but I'm asking for feedback from people who actually could take the time to *understand* and *balance* the rationales here, thanks :) ) Commits ------- 2132333059 [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break
2017-06-05 10:06:12 -07:00 · 2017-06-05 10:06:12 -07:00 · 085d8fec5d
commit 085d8fec5d
parent 58f03a734f 2132333059
7 changed files with 123 additions and 24 deletions
--- a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
+++ b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@ -7,7 +7,7 @@ CHANGELOG
 * Not defining the `type` option of the `framework.workflows.*` configuration entries is deprecated.
   The default value will be `state_machine` in Symfony 4.0.
 * Deprecated the `CompilerDebugDumpPass` class
- * [BC BREAK] Removed the "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter
+ * Deprecated the "framework.trusted_proxies" configuration option and the corresponding "kernel.trusted_proxies" parameter
 * Added a new new version strategy option called json_manifest_path
   that allows you to use the `JsonManifestVersionStrategy`.
 * Added `Symfony\Bundle\FrameworkBundle\Controller\AbstractController`. It provides
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@ -65,14 +65,38 @@ class Configuration implements ConfigurationInterface
                    ->info("Set true to enable support for the '_method' request parameter to determine the intended HTTP method on POST requests. Note: When using the HttpCache, you need to call the method in your front controller instead")
                    ->defaultTrue()
                ->end()
-                ->arrayNode('trusted_proxies') // @deprecated in version 3.3, to be removed in 4.0
+                ->arrayNode('trusted_proxies')
                    ->beforeNormalization()
-                        ->ifTrue(function ($v) { return empty($v); })
-                        ->then(function () { @trigger_error('The "framework.trusted_proxies" configuration key has been removed in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.', E_USER_DEPRECATED); })
+                        ->ifTrue(function ($v) {
+                            @trigger_error('The "framework.trusted_proxies" configuration key has been deprecated in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.', E_USER_DEPRECATED);
+
+                            return !is_array($v) && null !== $v;
+                        })
+                        ->then(function ($v) { return is_bool($v) ? array() : preg_split('/\s*,\s*/', $v); })
                    ->end()
-                    ->beforeNormalization()
-                        ->ifTrue(function ($v) { return !empty($v); })
-                        ->thenInvalid('The "framework.trusted_proxies" configuration key has been removed in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.')
+                    ->prototype('scalar')
+                        ->validate()
+                            ->ifTrue(function ($v) {
+                                if (empty($v)) {
+                                    return false;
+                                }
+
+                                if (false !== strpos($v, '/')) {
+                                    if ('0.0.0.0/0' === $v) {
+                                        return false;
+                                    }
+
+                                    list($v, $mask) = explode('/', $v, 2);
+
+                                    if (strcmp($mask, (int) $mask) || $mask < 1 || $mask > (false !== strpos($v, ':') ? 128 : 32)) {
+                                        return true;
+                                    }
+                                }
+
+                                return !filter_var($v, FILTER_VALIDATE_IP);
+                            })
+                            ->thenInvalid('Invalid proxy IP "%s"')
+                        ->end()
                    ->end()
                ->end()
                ->scalarNode('ide')->defaultNull()->end()
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@ -146,6 +146,9 @@ class FrameworkExtension extends Extension

        $container->setParameter('kernel.http_method_override', $config['http_method_override']);
        $container->setParameter('kernel.trusted_hosts', $config['trusted_hosts']);
+        if ($config['trusted_proxies']) {
+            $container->setParameter('kernel.trusted_proxies', $config['trusted_proxies']);
+        }
        $container->setParameter('kernel.default_locale', $config['default_locale']);

        if (!$container->hasParameter('debug.file_link_format')) {
--- a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
+++ b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
@ -45,7 +45,7 @@ class ConfigurationTest extends TestCase

    /**
     * @group legacy
-     * @expectedDeprecation The "framework.trusted_proxies" configuration key has been removed in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.
+     * @expectedDeprecation The "framework.trusted_proxies" configuration key has been deprecated in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.
     */
    public function testTrustedProxiesSetToNullIsDeprecated()
    {
@ -56,7 +56,7 @@ class ConfigurationTest extends TestCase

    /**
     * @group legacy
-     * @expectedDeprecation The "framework.trusted_proxies" configuration key has been removed in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.
+     * @expectedDeprecation The "framework.trusted_proxies" configuration key has been deprecated in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.
     */
    public function testTrustedProxiesSetToEmptyArrayIsDeprecated()
    {
@ -66,7 +66,8 @@ class ConfigurationTest extends TestCase
    }

    /**
-     * @expectedException \InvalidArgumentException
+     * @group legacy
+     * @expectedDeprecation The "framework.trusted_proxies" configuration key has been deprecated in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead.
     */
    public function testTrustedProxiesSetToNonEmptyArrayIsInvalid()
    {
@ -75,6 +76,70 @@ class ConfigurationTest extends TestCase
        $processor->processConfiguration($configuration, array(array('trusted_proxies' => array('127.0.0.1'))));
    }

+    /**
+     * @group legacy
+     * @dataProvider getTestValidTrustedProxiesData
+     */
+    public function testValidTrustedProxies($trustedProxies, $processedProxies)
+    {
+        $processor = new Processor();
+        $configuration = new Configuration(true);
+        $config = $processor->processConfiguration($configuration, array(array(
+            'secret' => 's3cr3t',
+            'trusted_proxies' => $trustedProxies,
+        )));
+
+        $this->assertEquals($processedProxies, $config['trusted_proxies']);
+    }
+
+    public function getTestValidTrustedProxiesData()
+    {
+        return array(
+            array(array('127.0.0.1'), array('127.0.0.1')),
+            array(array('::1'), array('::1')),
+            array(array('127.0.0.1', '::1'), array('127.0.0.1', '::1')),
+            array(null, array()),
+            array(false, array()),
+            array(array(), array()),
+            array(array('10.0.0.0/8'), array('10.0.0.0/8')),
+            array(array('::ffff:0:0/96'), array('::ffff:0:0/96')),
+            array(array('0.0.0.0/0'), array('0.0.0.0/0')),
+        );
+    }
+
+    /**
+     * @group legacy
+     * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
+     */
+    public function testInvalidTypeTrustedProxies()
+    {
+        $processor = new Processor();
+        $configuration = new Configuration(true);
+        $processor->processConfiguration($configuration, array(
+            array(
+                'secret' => 's3cr3t',
+                'trusted_proxies' => 'Not an IP address',
+            ),
+        ));
+    }
+
+    /**
+     * @group legacy
+     * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException
+     */
+    public function testInvalidValueTrustedProxies()
+    {
+        $processor = new Processor();
+        $configuration = new Configuration(true);
+
+        $processor->processConfiguration($configuration, array(
+            array(
+                'secret' => 's3cr3t',
+                'trusted_proxies' => array('Not an IP address'),
+            ),
+        ));
+    }
+
    public function testAssetsCanBeEnabled()
    {
        $processor = new Processor();
@ -156,6 +221,7 @@ class ConfigurationTest extends TestCase
    {
        return array(
            'http_method_override' => true,
+            'trusted_proxies' => array(),
            'ide' => null,
            'default_locale' => 'en',
            'csrf_protection' => array(
--- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md
+++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md
@ -4,7 +4,7 @@ CHANGELOG
 3.3.0
 -----

- * [BC BREAK] the `Request::setTrustedProxies()` method takes a new `$trustedHeaderSet` argument,
+ * the `Request::setTrustedProxies()` method takes a new `$trustedHeaderSet` argument,
   see http://symfony.com/doc/current/components/http_foundation/trusting_proxies.html for more info,
 * deprecated the `Request::setTrustedHeaderName()` and `Request::getTrustedHeaderName()` methods,
 * added `File\Stream`, to be passed to `BinaryFileResponse` when the size of the served file is unknown,
--- a/src/Symfony/Component/HttpFoundation/Request.php
+++ b/src/Symfony/Component/HttpFoundation/Request.php
@ -581,7 +581,7 @@ class Request
     * You should only list the reverse proxies that you manage directly.
     *
     * @param array $proxies          A list of trusted proxies
-     * @param int   $trustedHeaderSet A bit field of Request::HEADER_*, usually either Request::HEADER_FORWARDED or Request::HEADER_X_FORWARDED_ALL, to set which headers to trust from your proxies
+     * @param int   $trustedHeaderSet A bit field of Request::HEADER_*, to set which headers to trust from your proxies
     *
     * @throws \InvalidArgumentException When $trustedHeaderSet is invalid
     */
@ -590,10 +590,11 @@ class Request
        self::$trustedProxies = $proxies;

        if (2 > func_num_args()) {
-            // @deprecated code path in 3.3, to be replaced by mandatory argument in 4.0.
-            throw new \InvalidArgumentException(sprintf('The %s() method expects a bit field of Request::HEADER_* as second argument. Defining it is required since version 3.3. See http://symfony.com/doc/current/components/http_foundation/trusting_proxies.html for more info.', __METHOD__));
+            @trigger_error(sprintf('The %s() method expects a bit field of Request::HEADER_* as second argument since version 3.3. Defining it will be required in 4.0. ', __METHOD__), E_USER_DEPRECATED);
+
+            return;
        }
-        $trustedHeaderSet = func_get_arg(1);
+        $trustedHeaderSet = (int) func_get_arg(1);

        foreach (self::$trustedHeaderNames as $header => $name) {
            self::$trustedHeaders[$header] = $header & $trustedHeaderSet ? $name : null;
@ -665,11 +666,11 @@ class Request
     *
     * @throws \InvalidArgumentException
     *
-     * @deprecated since version 3.3, to be removed in 4.0. Use "X-Forwarded-*" headers or the "Forwarded" header defined in RFC7239, and the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.
+     * @deprecated since version 3.3, to be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.
     */
    public static function setTrustedHeaderName($key, $value)
    {
-        @trigger_error(sprintf('The "%s()" method is deprecated since version 3.3 and will be removed in 4.0. Use "X-Forwarded-*" headers or the "Forwarded" header defined in RFC7239, and the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.', __METHOD__), E_USER_DEPRECATED);
+        @trigger_error(sprintf('The "%s()" method is deprecated since version 3.3 and will be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.', __METHOD__), E_USER_DEPRECATED);

        if (!array_key_exists($key, self::$trustedHeaders)) {
            throw new \InvalidArgumentException(sprintf('Unable to set the trusted header name for key "%s".', $key));
@ -679,6 +680,9 @@ class Request

        if (null !== $value) {
            self::$trustedHeaderNames[$key] = $value;
+            self::$trustedHeaderSet |= $key;
+        } else {
+            self::$trustedHeaderSet &= ~$key;
        }
    }

@ -886,8 +890,8 @@ class Request
     * adding the IP address where it received the request from.
     *
     * If your reverse proxy uses a different header name than "X-Forwarded-For",
-     * ("Client-Ip" for instance), configure it via "setTrustedHeaderName()" with
-     * the "client-ip" key.
+     * ("Client-Ip" for instance), configure it via the $trustedHeaderSet
+     * argument of the Request::setTrustedProxies() method instead.
     *
     * @return string|null The client IP address
     *
@ -993,7 +997,8 @@ class Request
     * The "X-Forwarded-Port" header must contain the client port.
     *
     * If your reverse proxy uses a different header name than "X-Forwarded-Port",
-     * configure it via "setTrustedHeaderName()" with the "client-port" key.
+     * configure it via via the $trustedHeaderSet argument of the
+     * Request::setTrustedProxies() method instead.
     *
     * @return int|string can be a string if fetched from the server bag
     */
@ -1210,8 +1215,8 @@ class Request
     * The "X-Forwarded-Proto" header must contain the protocol: "https" or "http".
     *
     * If your reverse proxy uses a different header name than "X-Forwarded-Proto"
-     * ("SSL_HTTPS" for instance), configure it via "setTrustedHeaderName()" with
-     * the "client-proto" key.
+     * ("SSL_HTTPS" for instance), configure it via the $trustedHeaderSet
+     * argument of the Request::setTrustedProxies() method instead.
     *
     * @return bool
     */
@ -1235,7 +1240,8 @@ class Request
     * The "X-Forwarded-Host" header must contain the client host name.
     *
     * If your reverse proxy uses a different header name than "X-Forwarded-Host",
-     * configure it via "setTrustedHeaderName()" with the "client-host" key.
+     * configure it via the $trustedHeaderSet argument of the
+     * Request::setTrustedProxies() method instead.
     *
     * @return string
     *
--- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
+++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
@ -1729,7 +1729,7 @@ class RequestTest extends TestCase

    /**
     * @group legacy
-     * @expectedDeprecation The "Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()" method is deprecated since version 3.3 and will be removed in 4.0. Use "X-Forwarded-*" headers or the "Forwarded" header defined in RFC7239, and the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.
+     * @expectedDeprecation The "Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()" method is deprecated since version 3.3 and will be removed in 4.0. Use the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.
     */
    public function testLegacyTrustedProxies()
    {