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 ------- 2132333 [HttpFoundation][FrameworkBundle] Revert "trusted proxies" BC break
symfony · Jun 5, 2017 · 085d8fe · 085d8fe
2 parents 58f03a7 + 2132333
commit 085d8fe
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 24 deletions.
diff --git 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

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -65,14 +65,38 @@ public function getConfigTreeBuilder()
                     ->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()

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
@@ -146,6 +146,9 @@ public function load(array $configs, ContainerBuilder $container)
 
         $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')) {

diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php
@@ -45,7 +45,7 @@ public function testDoNoDuplicateDefaultFormResources()
 
     /**
      * @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 @@ public function testTrustedProxiesSetToNullIsDeprecated()
 
     /**
      * @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 @@ public function testTrustedProxiesSetToEmptyArrayIsDeprecated()
     }
 
     /**
-     * @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 @@ public function testTrustedProxiesSetToNonEmptyArrayIsInvalid()
         $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 @@ protected static function getBundleDefaultConfig()
     {
         return array(
             'http_method_override' => true,
+            'trusted_proxies' => array(),
             'ide' => null,
             'default_locale' => 'en',
             'csrf_protection' => array(

diff --git 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,

diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php
@@ -581,7 +581,7 @@ public function overrideGlobals()
      * 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 @@ public static function setTrustedProxies(array $proxies/*, int $trustedHeaderSet
         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 @@ public static function getTrustedHosts()
      *
      * @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 @@ public static function setTrustedHeaderName($key, $value)
 
         if (null !== $value) {
             self::$trustedHeaderNames[$key] = $value;
+            self::$trustedHeaderSet |= $key;
+        } else {
+            self::$trustedHeaderSet &= ~$key;
         }
     }
 
@@ -886,8 +890,8 @@ public function getClientIps()
      * 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 @@ public function getScheme()
      * 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 @@ public function getQueryString()
      * 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 @@ public function isSecure()
      * 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
      *

diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
@@ -1729,7 +1729,7 @@ public function testTrustedProxiesXForwardedFor()
 
     /**
      * @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()
     {