Improve validation of Theme directory names

Author: LukeTowers

modules/cms/classes/Theme.php

@@ -93,9 +93,14 @@ public function getPath(?string $dirName = null): string
 
     /**
      * Sets the theme directory name.
+     * @throws ApplicationException if the directory name is invalid.
      */
     public function setDirName(string $dirName): void
     {
+        if (!static::isValidDirName($dirName)) {
+            throw new ApplicationException(Lang::get('cms::lang.theme.dir_name_invalid'));
+        }
+
         $this->dirName = $dirName;
     }
 
@@ -107,6 +112,14 @@ public function getDirName(): string
         return $this->dirName;
     }
 
+    /**
+     * Determines if the given directory name is valid.
+     */
+    public static function isValidDirName(string $dirName): bool
+    {
+        return (bool) preg_match('/^[a-z0-9\_\-]+$/i', $dirName);
+    }
+
     /**
      * Helper for {{ theme.id }} twig vars
      * Returns a unique string for this theme.
@@ -237,9 +250,14 @@ public static function getActiveTheme(): self
     /**
      * Sets the active theme in the database.
      * The active theme code is stored in the database and overrides the configuration cms.activeTheme parameter.
+     * @throws ApplicationException if the directory name is invalid.
      */
     public static function setActiveTheme(string $code): void
     {
+        if (!static::isValidDirName($code)) {
+            throw new ApplicationException(Lang::get('cms::lang.theme.dir_name_invalid'));
+        }
+
         self::resetCache();
 
         Parameter::set(self::ACTIVE_KEY, $code);

modules/cms/console/ThemeInstall.php

@@ -50,7 +50,7 @@ public function handle()
         }
 
         if ($argDirName) {
-            if (!preg_match('/^[a-z0-9\_\-]+$/i', $argDirName)) {
+            if (!Theme::isValidDirName($argDirName)) {
                 return $this->error('Invalid destination directory name.');
             }
 

modules/cms/controllers/Themes.php

@@ -149,7 +149,7 @@ public function index_onCreate()
             throw new ValidationException(['name' => Lang::get('cms::lang.theme.create_theme_required_name')]);
         }
 
-        if (!preg_match('/^[a-z0-9\_\-]+$/i', $newDirName)) {
+        if (!CmsTheme::isValidDirName($newDirName)) {
             throw new ValidationException(['dir_name' => Lang::get('cms::lang.theme.dir_name_invalid')]);
         }
 
@@ -233,7 +233,7 @@ public function index_onDuplicateTheme()
         $sourcePath = $theme->getPath();
         $destinationPath = themes_path().'/'.$newDirName;
 
-        if (!preg_match('/^[a-z0-9\_\-]+$/i', $newDirName)) {
+        if (!CmsTheme::isValidDirName($newDirName)) {
             throw new ValidationException(['new_dir_name' => Lang::get('cms::lang.theme.dir_name_invalid')]);
         }
 

modules/cms/tests/classes/ThemeTest.php

@@ -6,6 +6,7 @@
 use Cms\Classes\Theme;
 use Config;
 use Event;
+use Winter\Storm\Exception\ApplicationException;
 
 class ThemeTest extends TestCase
 {
@@ -122,4 +123,44 @@ public function testChildThemeAssetUrl()
             $theme->assetUrl('assets/css/style2.css')
         );
     }
+
+    public function dirNameValidityProvider(): array
+    {
+        return [
+            // Valid names
+            'lowercase'        => ['mytheme', true],
+            'uppercase'        => ['MYTHEME', true],
+            'mixed case'       => ['MyTheme', true],
+            'with digits'      => ['theme123', true],
+            'with hyphens'     => ['my-theme', true],
+            'with underscores' => ['my_theme', true],
+            'all allowed'      => ['My-Theme_123', true],
+            'single char'      => ['a', true],
+
+            // Invalid names
+            'empty string'     => ['', false],
+            'dot'              => ['.', false],
+            'dot dot'          => ['..', false],
+            'path traversal'   => ['../etc', false],
+            'forward slash'    => ['foo/bar', false],
+            'backslash'        => ['foo\\bar', false],
+            'spaces'           => ['my theme', false],
+            'special chars'    => ['theme!@#', false],
+        ];
+    }
+
+    /**
+     * @dataProvider dirNameValidityProvider
+     */
+    public function testIsValidDirName(string $dirName, bool $expected): void
+    {
+        $this->assertSame($expected, Theme::isValidDirName($dirName));
+    }
+
+    public function testLoadRejectsPathTraversal()
+    {
+        $this->expectException(ApplicationException::class);
+
+        Theme::load('../../etc');
+    }
 }