diff --git a/application/config/internal.php b/application/config/internal.php index b6e87a84bb0..a637202a050 100644 --- a/application/config/internal.php +++ b/application/config/internal.php @@ -149,7 +149,7 @@ 'enableCookieValidation' => false, // Enable to activate cookie protection 'noCsrfValidationParams' => array(), 'noCsrfValidationRoutes' => array( - 'remotecontrol', + 'admin/remotecontrol', 'plugins/unsecure', ), 'csrfCookie' => array( diff --git a/application/core/LSHttpRequest.php b/application/core/LSHttpRequest.php index 66df55688b0..b87cab75dc3 100644 --- a/application/core/LSHttpRequest.php +++ b/application/core/LSHttpRequest.php @@ -163,7 +163,7 @@ protected function normalizeRequest() // $validationParams['request'] = 'acs'; foreach ($validationRoutes as $cr) { - if (preg_match('#' . $cr . '#', $route)) { + if (self::routeMatchesNoCsrfValidationRule($route, $cr)) { Yii::app()->detachEventHandler( 'onBeginRequest', array($this, 'validateCsrfToken') @@ -250,4 +250,20 @@ public function setQueryParams($values) { $this->queryParams = $values; } + + /** + * Returns true if the route matches the given validation rule. + * @param string $route the route to be checked + * @param string $rule the validation rule + * @return bool true if the route matches the given validation rule + */ + public static function routeMatchesNoCsrfValidationRule($route, $rule) + { + // The rule should either match the whole route, or the start of the route followed by a slash. + // For example the routes "rest" (in the case of "index.php/rest?...") or "rest/..." (in the case of + // "index.php/rest/...") should be matched by the rule "rest", but the route "admin/menus/sa/restore" + // should not. + $route = ltrim($route, '/'); + return preg_match('#^' . $rule . '$|^' . $rule . '/#', (string) $route); + } } diff --git a/tests/functional/backend/RemoteControlTest.php b/tests/functional/backend/RemoteControlTest.php new file mode 100644 index 00000000000..80f8058206a --- /dev/null +++ b/tests/functional/backend/RemoteControlTest.php @@ -0,0 +1,62 @@ +urlManager; + self::$tmpBaseUrl = $urlMan->getBaseUrl(); + $urlMan->setBaseUrl('http://' . self::$domain . '/index.php'); + //$serverUrl = App()->createAbsoluteUrl('/admin/remotecontrol'); + $serverUrl = $urlMan->createUrl('/admin/remotecontrol'); + + self::$tmpRPCType = Yii::app()->getConfig('RPCInterface'); + + if (self::$tmpRPCType === 'off') { + \SettingGlobal::setSetting('RPCInterface', 'json'); + $RPCType = 'json'; + } else { + $RPCType = self::$tmpRPCType; + } + + if ($RPCType == 'xml') { + $cur_path = get_include_path(); + set_include_path($cur_path . PATH_SEPARATOR . APPPATH . 'helpers'); + require_once('Zend/XmlRpc/Client.php'); + + self::$client = new \Zend_XmlRpc_Client($serverUrl); + } elseif ($RPCType == 'json') { + Yii::app()->loadLibrary('jsonRPCClient'); + self::$client = new \jsonRPCClient($serverUrl); + } + } + + public static function tearDownAfterClass(): void + { + parent::tearDownAfterClass(); + + $urlMan = Yii::app()->urlManager; + $urlMan->setBaseUrl(self::$tmpBaseUrl); + + \SettingGlobal::setSetting('RPCInterface', self::$tmpRPCType); + } + + public function testGetSessionKey() + { + $sessionKey = self::$client->call('get_session_key', ['admin', 'password']); + $this->assertIsNotArray($sessionKey); + + self::$client->call('release_session_key', [$sessionKey]); + } +} diff --git a/tests/unit/CsrfHttpRequestTest.php b/tests/unit/CsrfHttpRequestTest.php new file mode 100644 index 00000000000..1949fe7478d --- /dev/null +++ b/tests/unit/CsrfHttpRequestTest.php @@ -0,0 +1,167 @@ +request->noCsrfValidationRoutes; + } + + /** + * Testing that rest routes skip CSRF validation. + */ + public function testRestRoutesSkipCsrfValidation() + { + // This test is skipped since there are no rest routes on v5, hence not expected to skip CSRF validation. + $this->markTestSkipped(); + $routes = array( + 'rest/v1/actionOnItemById/15', + 'rest/v1/action', + '/rest/v1/actionOnItemById/25', + '/rest/v1/action', + ); + + foreach ($routes as $route) { + $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[0] + ); + + $this->assertSame( + 1, + $skipValidation, + 'CSRF validation should be skipped since the route ' . $route . ' is a rest route.' + ); + } + } + + /** + * Testing that remotecontrol routes skip CSRF validation. + */ + public function testRemoteControlRoutesSkipCsrfValidation() + { + $routes = array( + 'admin/remotecontrol/actionOnItemById/15', + 'admin/remotecontrol/action', + '/admin/remotecontrol/actionOnItemById/25', + '/admin/remotecontrol', + ); + + foreach ($routes as $route) { + $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[0] + ); + + $this->assertSame( + 1, + $skipValidation, + 'CSRF validation should be skipped since the route ' . $route . ' is a remote control route.' + ); + } + } + + /** + * Testing that plugins/unsecure routes skip CSRF validation. + */ + public function testPluginsUnsecureRoutesSkipCsrfValidation() + { + $routes = array( + 'plugins/unsecure/action', + 'plugins/unsecure/actionOnItemById/15', + '/plugins/unsecure/action', + '/plugins/unsecure/actionOnItemById/25', + ); + + foreach ($routes as $route) { + $skipValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[1] + ); + + $this->assertSame( + 1, + $skipValidation, + 'CSRF validation should be skipped since the route ' . $route . ' is a plugins/unsecure route.' + ); + } + } + + /** + * Testing that rest-like routes don't skip CSRF validation. + */ + public function testRestLikeRoutesDoNotSkipCsrfValidation() + { + // This test is skipped since there are no rest routes on v5, hence not expected to skip CSRF validation. + $this->markTestSkipped(); + $routes = array( + 'admin/menus/sa/restore', + '/admin/menus/sa/restore' + ); + + foreach ($routes as $route) { + $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[0] + ); + + $this->assertSame( + 0, + $routeValidation, + 'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.' + ); + } + } + + /** + * Testing that remote-like routes don't skip CSRF validation. + */ + public function testRemoteLikeRoutesDoNotSkipCsrfValidation() + { + $routes = array( + 'remote/action', + '/remote/action' + ); + + foreach ($routes as $route) { + $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[0] + ); + + $this->assertSame( + 0, + $routeValidation, + 'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.' + ); + } + } + + /** + * Testing that plugins/unsecure-like routes don't skip CSRF validation. + */ + public function testPluginUnsecureLikeRoutesDoNotSkipCsrfValidation() + { + $routes = array( + 'plugins/settings', + '/plugins/settings' + ); + + foreach ($routes as $route) { + $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $route, + self::$noCsrfValidationRoutes[1] + ); + + $this->assertSame( + 0, + $routeValidation, + 'CSRF validation should not be skipped since the route ' . $route . ' is not a remote control route.' + ); + } + } +}