Fixed issue #19139: CSRF in Reset Survey menus

Tests fixed.
LimeSurvey · Nov 14, 2023 · f0ae09b · f0ae09b
1 parent b548f35
commit f0ae09b
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 33 deletions.
diff --git 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
@@ -263,6 +263,7 @@ public static function routeMatchesNoCsrfValidationRule($route, $rule)
         // 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
@@ -21,6 +21,8 @@ public function testRestRoutesSkipCsrfValidation()
         $routes = array(
             'rest/v1/actionOnItemById/15',
             'rest/v1/action',
+            '/rest/v1/actionOnItemById/25',
+            '/rest/v1/action',
         );
 
         foreach ($routes as $route) {
@@ -43,8 +45,10 @@ public function testRestRoutesSkipCsrfValidation()
     public function testRemoteControlRoutesSkipCsrfValidation()
     {
         $routes = array(
-            'remotecontrol/actionOnItemById/15',
-            'remotecontrol/action',
+            'admin/remotecontrol/actionOnItemById/15',
+            'admin/remotecontrol/action',
+            '/admin/remotecontrol/actionOnItemById/25',
+            '/admin/remotecontrol',
         );
 
         foreach ($routes as $route) {
@@ -69,6 +73,8 @@ public function testPluginsUnsecureRoutesSkipCsrfValidation()
         $routes = array(
             'plugins/unsecure/action',
             'plugins/unsecure/actionOnItemById/15',
+            '/plugins/unsecure/action',
+            '/plugins/unsecure/actionOnItemById/25',
         );
 
         foreach ($routes as $route) {
@@ -92,55 +98,70 @@ public function testRestLikeRoutesDoNotSkipCsrfValidation()
     {
         // This test is skipped since there are no rest routes on v5, hence not expected to skip CSRF validation.
         $this->markTestSkipped();
-        $route = 'admin/menus/sa/restore';
-
-        $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
-            $route,
-            self::$noCsrfValidationRoutes[0]
+        $routes = array(
+            'admin/menus/sa/restore',
+            '/admin/menus/sa/restore'
         );
 
-        $this->assertSame(
-            0,
-            $routeValidation,
-            'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.'
-        );
+        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()
     {
-        $route = 'remote/action';
-
-        $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
-            $route,
-            self::$noCsrfValidationRoutes[0]
+        $routes = array(
+            'remote/action',
+            '/remote/action'
         );
 
-        $this->assertSame(
-            0,
-            $routeValidation,
-            'CSRF validation should not be skipped since the route ' . $route . ' is not a rest route.'
-        );
+        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()
     {
-        $route = 'plugins/settings';
-
-        $routeValidation = \LSHttpRequest::routeMatchesNoCsrfValidationRule(
-            $route,
-            self::$noCsrfValidationRoutes[1]
+        $routes = array(
+            'plugins/settings',
+            '/plugins/settings'
         );
 
-        $this->assertSame(
-            0,
-            $routeValidation,
-            'CSRF validation should not be skipped since the route ' . $route . ' is not a remote control route.'
-        );
+        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.'
+            );
+        }
     }
 }