Fixed issue #19139: [security] CSRF in Reset Survey menus (#3603)

Co-authored-by: Lapiu Dev <devgit@lapiu.biz>

application/config/internal.php

@@ -156,7 +156,7 @@
             'noCsrfValidationParams' => array(),
             'noCsrfValidationRoutes' => array(
                 'rest',
-                'remotecontrol',
+                'admin/remotecontrol',
                 'plugins/unsecure',
             ),
             'csrfCookie' => array(

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);
+    }
 }

tests/unit/CsrfHttpRequestTest.php

@@ -0,0 +1,142 @@
+<?php
+
+namespace ls\tests;
+
+class CsrfHttpRequestTest extends TestBaseClass
+{
+    public static $noCsrfValidationRoutes;
+
+    public static function setUpBeforeClass(): void
+    {
+        self::$noCsrfValidationRoutes = \Yii::app()->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.'
+        );
+    }
+}