#17733: Additional fixes for #17665, Forwarded header parsing in Request

- Remove header from secure headers
- Regexp and return null fix
- Fix tests, fix in array case sensitivity, rx duplicated group name
- Simplify code
- Add phpdoc

Co-Authored-By: Alexander Makarov <sam@rmcreative.ru>

docs/guide/runtime-requests.md

@@ -201,6 +201,10 @@ except the `X-ProxyUser-Ip` and `Front-End-Https` headers in case the request is
 In that case the former is used to retrieve the user IP as configured in `ipHeaders` and the latter
 will be used to determine the result of [[yii\web\Request::getIsSecureConnection()]].
 
+Since 2.0.31 [RFC 7239](https://tools.ietf.org/html/rfc7239) `Forwarded` header is supported. In order to enable
+it you need to add header name to `secureHeaders`. Make sure your proxy is setting it, otherwise end user would be
+able to spoof IP and protocol.
+
 ### Already resolved user IP <span id="already-respolved-user-ip"></span>
 
 If the user's IP address is resolved before the Yii application (e.g. `ngx_http_realip_module` or similar),

framework/CHANGELOG.md

@@ -5,7 +5,7 @@ Yii Framework 2 Change Log
 ------------------------
 
 - Bug #17661: Fix query builder incorrect IN/NOT IN condition handling for null values (strychu)
-- Enh #17665: Implement RFC 7239 `Forwarded` header parsing in Request (mikk150)
+- Enh #17665: Implement RFC 7239 `Forwarded` header parsing in Request (mikk150, kamarton)
 - Bug #17687: `Query::indexBy` can now include a table alias (brandonkelly)
 - Bug #17685: Fix invalid db component in `m180523_151638_rbac_updates_indexes_without_prefix` (rvkulikov)
 - Bug #17694: Fixed Error Handler to clear registered view tags, scripts, and files when rendering error view through action view (bizley)

framework/web/Request.php

@@ -212,8 +212,10 @@ class Request extends \yii\base\Request
     /**
      * @var array lists of headers that are, by default, subject to the trusted host configuration.
      * These headers will be filtered unless explicitly allowed in [[trustedHosts]].
+     * If the list contains the `Forwarded` header, processing will be done according to RFC 7239.
      * The match of header names is case-insensitive.
      * @see https://en.wikipedia.org/wiki/List_of_HTTP_header_fields
+     * @see https://tools.ietf.org/html/rfc7239
      * @see $trustedHosts
      * @since 2.0.13
      */
@@ -226,13 +228,11 @@ class Request extends \yii\base\Request
         // Microsoft:
         'Front-End-Https',
         'X-Rewrite-Url',
-
-        // RFC 7239:
-        'Forwarded',
     ];
     /**
      * @var string[] List of headers where proxies store the real client IP.
      * It's not advisable to put insecure headers here.
+     * To use the `Forwarded` header according to RFC 7239, the header must be added to [[secureHeaders]] list.
      * The match of header names is case-insensitive.
      * @see $trustedHosts
      * @see $secureHeaders
@@ -728,7 +728,7 @@ class Request extends \yii\base\Request
             $secure = $this->getIsSecureConnection();
             $http = $secure ? 'https' : 'http';
 
-            if ($this->getSecureForwardedHeaderTrustedPart('host')) {
+            if ($this->getSecureForwardedHeaderTrustedPart('host') !== null) {
                 $this->_hostInfo = $http . '://' . $this->getSecureForwardedHeaderTrustedPart('host');
             } elseif ($this->headers->has('X-Forwarded-Host')) {
                 $this->_hostInfo = $http . '://' . trim(explode(',', $this->headers->get('X-Forwarded-Host'))[0]);
@@ -1839,7 +1839,7 @@ class Request extends \yii\base\Request
      *
      * @param string $token Header token
      *
-     * @return string
+     * @return string|null
      *
      * @since 2.0.31
      */
@@ -1853,6 +1853,7 @@ class Request extends \yii\base\Request
                 return $lastElement[$token];
             }
         }
+        return null;
     }
 
     /**
@@ -1883,33 +1884,41 @@ class Request extends \yii\base\Request
      */
     protected function getSecureForwardedHeaderParts()
     {
-        if (!in_array('Forwarded', $this->secureHeaders, true)) {
+        if ($this->_secureForwardedHeaderParts !== null) {
-            return [];
+            return $this->_secureForwardedHeaderParts;
+        }
+        if (count(preg_grep('/^forwarded$/i', $this->secureHeaders)) === 0) {
+            return $this->_secureForwardedHeaderParts = [];
         }
-        if ($this->_secureForwardedHeaderParts === null) {
-            $this->_secureForwardedHeaderParts = [];
         /*
-             * First one is always correct, because proxy CAN add headers
+         * First header is always correct, because proxy CAN add headers
-             * after last one found.
+         * after last one is found.
          * Keep in mind that it is NOT enforced, therefore we cannot be
-             * sure, that this is really first one
+         * sure, that this is really a first one.
          *
-             * FPM keeps last header sent which is a bug, you need to merge
+         * FPM keeps last header sent which is a bug. You need to merge
          * headers together on your web server before letting FPM handle it
          * @see https://bugs.php.net/bug.php?id=78844
          */
         $forwarded = $this->headers->get('Forwarded', '');
+        if ($forwarded === '') {
+            return $this->_secureForwardedHeaderParts = [];
+        }
 
         preg_match_all('/(?:[^",]++|"[^"]++")+/', $forwarded, $forwardedElements);
 
         foreach ($forwardedElements[0] as $forwardedPairs) {
-                preg_match_all('/(?P<key>\w+)\s*=\s*(?|(?P<value>[^",;]*[^",;\s])|"(?P<value>[^"]+)")/', $forwardedPairs, $matches, PREG_SET_ORDER);
+            preg_match_all('/(?P<key>\w+)\s*=\s*(?:(?P<value>[^",;]*[^",;\s])|"(?P<value2>[^"]+)")/', $forwardedPairs,
+                $matches, PREG_SET_ORDER);
             $this->_secureForwardedHeaderParts[] = array_reduce($matches, function ($carry, $item) {
-                    $carry[strtolower($item['key'])] = $item['value'];
+                $value = $item['value'];
+                if (isset($item['value2']) && $item['value2'] !== '') {
+                    $value = $item['value2'];
+                }
+                $carry[strtolower($item['key'])] = $value;
                 return $carry;
             }, []);
         }
-        }
         return $this->_secureForwardedHeaderParts;
     }
 }

tests/framework/web/RequestTest.php

@@ -385,6 +385,12 @@ class RequestTest extends TestCase
             'trustedHosts' => [
                 '192.168.0.0/24',
             ],
+            'secureHeaders' => [
+                'X-Forwarded-For',
+                'X-Forwarded-Host',
+                'X-Forwarded-Proto',
+                'forwarded',
+            ],
         ]);
 
         $this->assertEquals($expected[0], $request->getHostInfo());
@@ -547,6 +553,14 @@ class RequestTest extends TestCase
             'trustedHosts' => [
                 '192.168.0.0/24',
             ],
+            'secureHeaders' => [
+                'Front-End-Https',
+                'X-Rewrite-Url',
+                'X-Forwarded-For',
+                'X-Forwarded-Host',
+                'X-Forwarded-Proto',
+                'forwarded',
+            ],
         ]);
         $_SERVER = $server;
 
@@ -703,6 +717,14 @@ class RequestTest extends TestCase
             'trustedHosts' => [
                 '192.168.0.0/24',
             ],
+            'secureHeaders' => [
+                'Front-End-Https',
+                'X-Rewrite-Url',
+                'X-Forwarded-For',
+                'X-Forwarded-Host',
+                'X-Forwarded-Proto',
+                'forwarded',
+            ],
         ]);
 
         $this->assertEquals($expected, $request->getUserIP());
@@ -1052,6 +1074,12 @@ class RequestTest extends TestCase
                 '192.168.10.0/24',
                 '192.168.20.0/24'
             ],
+            'secureHeaders' => [
+                'X-Forwarded-For',
+                'X-Forwarded-Host',
+                'X-Forwarded-Proto',
+                'forwarded',
+            ],
         ]);
 
         $this->assertSame($expectedUserIp, $request->userIP, 'User IP fail!');
@@ -1076,7 +1104,7 @@ class RequestTest extends TestCase
                 'X-Forwarded-For',
                 'X-Forwarded-Host',
                 'X-Forwarded-Proto',
-            ]
+            ],
         ]);
 
         $this->assertSame('10.0.0.1', $request->userIP, 'User IP fail!');