From 038ce9f77e1ab0bb21037c927e6bf741374e3549 Mon Sep 17 00:00:00 2001 From: Ather Shu Date: Wed, 15 Jan 2020 20:51:57 +0800 Subject: [PATCH] Fix #17755: Fix a bug for web request with `trustedHosts` set to format `['10.0.0.1' => ['X-Forwarded-For']]` --- framework/CHANGELOG.md | 1 + framework/web/Request.php | 9 +- tests/framework/web/RequestTest.php | 131 +++++++++++++++++++++++++++- 3 files changed, 139 insertions(+), 2 deletions(-) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 6e27532736..6db7a93c30 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -7,6 +7,7 @@ Yii Framework 2 Change Log - Bug #17037: Fix uploaded file saving method in case generated by `MultipartFormDataParser` (sup-ham) - Bug #17803: Fixed ErrorHandler unregister and register only change global state when applicable (SamMousa) - Bug #17744: Fix a bug with setting incorrect `defaultValue` to AR column with `CURRENT_TIMESTAMP(x)` as default expression (MySQL >= 5.6.4) (bizley) +- Bug #17755: Fix a bug for web request with `trustedHosts` set to format `['10.0.0.1' => ['X-Forwarded-For']]` (shushenghong) - Enh #17729: Path alias support was added to `yii\web\UploadFile::saveAs()` (sup-ham) - Bug #17749: Dispatcher fix if target crashed in PHP 7.0+ (kamarton) - Bug #17762: PHP 7.4: Remove special condition for converting PHP errors to exceptions if they occurred inside of `__toString()` call (rob006) diff --git a/framework/web/Request.php b/framework/web/Request.php index 83d674b058..615fa3f515 100644 --- a/framework/web/Request.php +++ b/framework/web/Request.php @@ -1866,8 +1866,15 @@ class Request extends \yii\base\Request protected function getSecureForwardedHeaderTrustedParts() { $validator = $this->getIpValidator(); + $trustedHosts = []; + foreach ($this->trustedHosts as $trustedCidr => $trustedCidrOrHeaders) { + if (!is_array($trustedCidrOrHeaders)) { + $trustedCidr = $trustedCidrOrHeaders; + } + $trustedHosts[] = $trustedCidr; + } + $validator->setRanges($trustedHosts); - $validator->setRanges($this->trustedHosts); return array_filter($this->getSecureForwardedHeaderParts(), function ($headerPart) use ($validator) { return isset($headerPart['for']) ? !$validator->validate($headerPart['for']) : true; }); diff --git a/tests/framework/web/RequestTest.php b/tests/framework/web/RequestTest.php index 66255a3b32..934148b198 100644 --- a/tests/framework/web/RequestTest.php +++ b/tests/framework/web/RequestTest.php @@ -393,6 +393,23 @@ class RequestTest extends TestCase ], ]); + + $this->assertEquals($expected[0], $request->getHostInfo()); + $this->assertEquals($expected[1], $request->getHostName()); + + $request = new Request([ + 'trustedHosts' => [ + '192.168.0.0/24' => ['X-Forwarded-Host', 'forwarded'], + ], + 'secureHeaders' => [ + 'X-Forwarded-For', + 'X-Forwarded-Host', + 'X-Forwarded-Proto', + 'forwarded', + ], + ]); + + $this->assertEquals($expected[0], $request->getHostInfo()); $this->assertEquals($expected[1], $request->getHostName()); $_SERVER = $original; @@ -549,6 +566,8 @@ class RequestTest extends TestCase public function testGetIsSecureConnection($server, $expected) { $original = $_SERVER; + $_SERVER = $server; + $request = new Request([ 'trustedHosts' => [ '192.168.0.0/24', @@ -562,9 +581,62 @@ class RequestTest extends TestCase 'forwarded', ], ]); + $this->assertEquals($expected, $request->getIsSecureConnection()); + + $request = new Request([ + 'trustedHosts' => [ + '192.168.0.0/24' => ['Front-End-Https', 'X-Forwarded-Proto', 'forwarded'], + ], + 'secureHeaders' => [ + 'Front-End-Https', + 'X-Rewrite-Url', + 'X-Forwarded-For', + 'X-Forwarded-Host', + 'X-Forwarded-Proto', + 'forwarded', + ], + ]); + $this->assertEquals($expected, $request->getIsSecureConnection()); + + $_SERVER = $original; + } + + public function isSecureServerWithoutTrustedHostDataProvider() + { + return [ + // RFC 7239 forwarded header is not enabled + [[ + 'HTTP_FORWARDED' => 'proto=https', + 'REMOTE_ADDR' => '192.168.0.1', + ], false], + ]; + } + + /** + * @dataProvider isSecureServerWithoutTrustedHostDataProvider + * @param array $server + * @param bool $expected + */ + public function testGetIsSecureConnectionWithoutTrustedHost($server, $expected) + { + $original = $_SERVER; $_SERVER = $server; + $request = new Request([ + 'trustedHosts' => [ + '192.168.0.0/24' => ['Front-End-Https', 'X-Forwarded-Proto'], + ], + 'secureHeaders' => [ + 'Front-End-Https', + 'X-Rewrite-Url', + 'X-Forwarded-For', + 'X-Forwarded-Host', + 'X-Forwarded-Proto', + 'forwarded', + ], + ]); $this->assertEquals($expected, $request->getIsSecureConnection()); + $_SERVER = $original; } @@ -726,11 +798,68 @@ class RequestTest extends TestCase 'forwarded', ], ]); - $this->assertEquals($expected, $request->getUserIP()); + + $request = new Request([ + 'trustedHosts' => [ + '192.168.0.0/24' => ['X-Forwarded-For', 'forwarded'], + ], + 'secureHeaders' => [ + 'Front-End-Https', + 'X-Rewrite-Url', + 'X-Forwarded-For', + 'X-Forwarded-Host', + 'X-Forwarded-Proto', + 'forwarded', + ], + ]); + $this->assertEquals($expected, $request->getUserIP()); + $_SERVER = $original; } + public function getUserIPWithoutTruestHostDataProvider() + { + return [ + // RFC 7239 forwarded is not enabled + [ + [ + 'HTTP_FORWARDED' => 'for=123.123.123.123', + 'REMOTE_ADDR' => '192.168.0.1', + ], + '192.168.0.1', + ], + ]; + } + + /** + * @dataProvider getUserIPWithoutTruestHostDataProvider + * @param array $server + * @param string $expected + */ + public function testGetUserIPWithoutTrustedHost($server, $expected) + { + $original = $_SERVER; + $_SERVER = $server; + + $request = new Request([ + 'trustedHosts' => [ + '192.168.0.0/24' => ['X-Forwarded-For'], + ], + 'secureHeaders' => [ + 'Front-End-Https', + 'X-Rewrite-Url', + 'X-Forwarded-For', + 'X-Forwarded-Host', + 'X-Forwarded-Proto', + 'forwarded', + ], + ]); + $this->assertEquals($expected, $request->getUserIP()); + + $_SERVER = $original; + } + public function getMethodDataProvider() { return [