From 5e71b11d8d61a3edfb6d975060fb8e9471a021ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Somogyi=20M=C3=A1rton?= Date: Tue, 17 Dec 2019 19:53:55 +0100 Subject: [PATCH] #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 --- docs/guide/runtime-requests.md | 4 ++ framework/CHANGELOG.md | 2 +- framework/web/Request.php | 65 ++++++++++++++++------------- tests/framework/web/RequestTest.php | 30 ++++++++++++- 4 files changed, 71 insertions(+), 30 deletions(-) diff --git a/docs/guide/runtime-requests.md b/docs/guide/runtime-requests.md index 55fa7e7d7f..bce3c3c242 100644 --- a/docs/guide/runtime-requests.md +++ b/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 If the user's IP address is resolved before the Yii application (e.g. `ngx_http_realip_module` or similar), diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 619b6f3b8d..2e747a90cb 100644 --- a/framework/CHANGELOG.md +++ b/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) diff --git a/framework/web/Request.php b/framework/web/Request.php index 320c94b21a..83d674b058 100644 --- a/framework/web/Request.php +++ b/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,32 +1884,40 @@ class Request extends \yii\base\Request */ protected function getSecureForwardedHeaderParts() { - if (!in_array('Forwarded', $this->secureHeaders, true)) { - return []; + if ($this->_secureForwardedHeaderParts !== null) { + return $this->_secureForwardedHeaderParts; + } + if (count(preg_grep('/^forwarded$/i', $this->secureHeaders)) === 0) { + return $this->_secureForwardedHeaderParts = []; + } + /* + * First header is always correct, because proxy CAN add headers + * after last one is found. + * Keep in mind that it is NOT enforced, therefore we cannot be + * sure, that this is really a first one. + * + * 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 = []; } - if ($this->_secureForwardedHeaderParts === null) { - $this->_secureForwardedHeaderParts = []; - /* - * First one is always correct, because proxy CAN add headers - * after last one found. - * Keep in mind that it is NOT enforced, therefore we cannot be - * sure, that this is really first one - * - * 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', ''); - preg_match_all('/(?:[^",]++|"[^"]++")+/', $forwarded, $forwardedElements); + preg_match_all('/(?:[^",]++|"[^"]++")+/', $forwarded, $forwardedElements); - foreach ($forwardedElements[0] as $forwardedPairs) { - preg_match_all('/(?P\w+)\s*=\s*(?|(?P[^",;]*[^",;\s])|"(?P[^"]+)")/', $forwardedPairs, $matches, PREG_SET_ORDER); - $this->_secureForwardedHeaderParts[] = array_reduce($matches, function ($carry, $item) { - $carry[strtolower($item['key'])] = $item['value']; - return $carry; - }, []); - } + foreach ($forwardedElements[0] as $forwardedPairs) { + preg_match_all('/(?P\w+)\s*=\s*(?:(?P[^",;]*[^",;\s])|"(?P[^"]+)")/', $forwardedPairs, + $matches, PREG_SET_ORDER); + $this->_secureForwardedHeaderParts[] = array_reduce($matches, function ($carry, $item) { + $value = $item['value']; + if (isset($item['value2']) && $item['value2'] !== '') { + $value = $item['value2']; + } + $carry[strtolower($item['key'])] = $value; + return $carry; + }, []); } return $this->_secureForwardedHeaderParts; } diff --git a/tests/framework/web/RequestTest.php b/tests/framework/web/RequestTest.php index 31c2be6a6e..66255a3b32 100644 --- a/tests/framework/web/RequestTest.php +++ b/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!');