diff --git a/application/config/internal.php b/application/config/internal.php index cec288cc7b7..b290e856976 100644 --- a/application/config/internal.php +++ b/application/config/internal.php @@ -156,7 +156,7 @@ 'noCsrfValidationParams' => array(), 'noCsrfValidationRoutes' => array( 'rest', - 'remotecontrol', + 'admin/remotecontrol', 'plugins/unsecure', ), 'csrfCookie' => array( diff --git a/application/core/LSHttpRequest.php b/application/core/LSHttpRequest.php index 81a334c9fbe..b1ac8d095a3 100644 --- a/application/core/LSHttpRequest.php +++ b/application/core/LSHttpRequest.php @@ -164,7 +164,7 @@ protected function normalizeRequest() // $validationParams['request'] = 'acs'; foreach ($validationRoutes as $cr) { - if (preg_match('#' . $cr . '#', (string) $route)) { + if (self::routeMatchesNoCsrfValidationRule($route, $cr)) { Yii::app()->detachEventHandler( 'onBeginRequest', array($this, 'validateCsrfToken') @@ -251,4 +251,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/unit/CsrfHttpRequestTest.php b/tests/unit/CsrfHttpRequestTest.php new file mode 100644 index 00000000000..ca2e2b9178b --- /dev/null +++ b/tests/unit/CsrfHttpRequestTest.php @@ -0,0 +1,142 @@ +request->noCsrfValidationRoutes; + } + + /** + * Testing that rest routes skip CSRF validation. + */ + public function testRestRoutesSkipCsrfValidation() + { + $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[1] + ); + + $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[2] + ); + + $this->assertSame( + 1, + $skipValidation, + 'CSRF validation should be skipped since the route ' . $route . ' is a plugins/unsecure route.' + ); + } + } + + /** + * Testing that similar routes don't skip CSRF validation. + */ + public function testSimilarRoutesDoNotSkipCsrfValidation() + { + $routes = array( + 'admin/menus/sa/restore', + 'admin/remote/action', + 'plugins/settings', + '/admin/menus/sa/restore', + '/admin/remote/action', + '/plugins/settings', + ); + + // Asserting that a restlike route doesn't skip validation. + $restRouteValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $routes[0], + self::$noCsrfValidationRoutes[0] + ); + + $this->assertSame( + 0, + $restRouteValidation, + 'CSRF validation should not be skipped since the route ' . $routes[0] . ' is not a rest route.' + ); + + // Asserting that a remotecontrol-like route doesn't skip validation. + $remoteControlRouteValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $routes[1], + self::$noCsrfValidationRoutes[1] + ); + + $this->assertSame( + 0, + $remoteControlRouteValidation, + 'CSRF validation should not be skipped since the route ' . $routes[1] . ' is not a remote control route.' + ); + + // Asserting that a plugins/unsecure-like route doesn't skip validation. + $pluginUnsecureRouteValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule( + $routes[2], + self::$noCsrfValidationRoutes[2] + ); + + $this->assertSame( + 0, + $pluginUnsecureRouteValidation, + 'CSRF validation should not be skipped since the route ' . $routes[2] . ' is not a remote control route.' + ); + } +}