Grav vulnerable to Path traversal / arbitrary YAML write via user creation leading to Account Takeover / System Corruption

@@ -583,10 +583,19 @@ public function save()
     {
         // TODO: We may want to handle this in the storage layer in the future.
         $key = $this->getStorageKey();
-        if (!$key || strpos($key, '@@')) {
+        $isNewUser = !$key || strpos($key, '@@');
+
+        if ($isNewUser) {
             $storage = $this->getFlexDirectory()->getStorage();
             if ($storage instanceof FileStorage) {
-                $this->setStorageKey($this->getKey());
+                $newKey = $this->getKey();
+
+                // Check if a user with this username already exists (prevent overwriting)
+                if ($storage->hasKey($newKey)) {
+                    throw new RuntimeException('User account with this username already exists');
+                }
+
+                $this->setStorageKey($newKey);
             }
         }
 

@@ -128,8 +128,20 @@ public function save()
         if ($file) {
             $username = $this->filterUsername((string)$this->get('username'));
 
+            // Validate username to prevent path traversal attacks
+            if (!self::isValidUsername($username)) {
+                throw new \RuntimeException('Invalid username: contains invalid characters or sequences');
+            }
+
             if (!$file->filename()) {
                 $locator = Grav::instance()['locator'];
+
+                // Check if a user with this username already exists (prevent overwriting)
+                $existingFile = $locator->findResource('account://' . $username . YAML_EXT);
+                if ($existingFile) {
+                    throw new \RuntimeException('User account with this username already exists');
+                }
+
                 $file->filename($locator->findResource('account://' . $username . YAML_EXT, true, true));
             }
 
@@ -313,6 +325,37 @@ protected function filterUsername(string $username): string
         return mb_strtolower($username);
     }
 
+    /**
+     * Validates a username to prevent path traversal and other attacks.
+     *
+     * @param string $username
+     * @return bool
+     */
+    public static function isValidUsername(string $username): bool
+    {
+        // Username must not be empty
+        if (!$username) {
+            return false;
+        }
+
+        // Username must not contain filesystem-dangerous characters: \ / ? * : ; { } or newlines
+        if (!preg_match('/^[^\\/?*:;{}\\\\\\n]+$/u', $username)) {
+            return false;
+        }
+
+        // Username must not contain path traversal sequences (..)
+        if (str_contains($username, '..')) {
+            return false;
+        }
+
+        // Username must not start with a dot (hidden files)
+        if (str_starts_with($username, '.')) {
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * @return string|null
      */

@@ -220,6 +220,39 @@ public function normalizeKey(string $key): string
      */
     protected function validateKey(string $key): bool
     {
-        return $key && (bool) preg_match('/^[^\\/?*:;{}\\\\\\n]+$/u', $key);
+        // Key must not be empty
+        if (!$key) {
+            return false;
+        }
+
+        // Key must not contain filesystem-dangerous characters: \ / ? * : ; { } or newlines
+        if (!preg_match('/^[^\\/?*:;{}\\\\\\n]+$/u', $key)) {
+            return false;
+        }
+
+        // Key must not contain path traversal sequences (..)
+        if (str_contains($key, '..')) {
+            return false;
+        }
+
+        // Key must not start with a dot (hidden files)
+        if (str_starts_with($key, '.')) {
+            return false;
+        }
+
+        return true;
+    }
+
+    /**
+     * Validates a key and throws an exception if invalid.
+     *
+     * @param string $key
+     * @throws \InvalidArgumentException
+     */
+    public function assertValidKey(string $key): void
+    {
+        if (!$this->validateKey($key)) {
+            throw new \InvalidArgumentException(sprintf('Invalid storage key: "%s"', $key));
+        }
     }
 }

@@ -419,6 +419,9 @@ protected function saveRow(string $key, array $row): array
 
             $key = $this->normalizeKey($key);
 
+            // Validate the key to prevent path traversal and other attacks
+            $this->assertValidKey($key);
+
             // Check if the row already exists and if the key has been changed.
             $oldKey = $row['__META']['storage_key'] ?? null;
             if (is_string($oldKey) && $oldKey !== $key) {

@@ -0,0 +1,185 @@
+<?php
+
+use Grav\Common\User\DataUser\User;
+
+/**
+ * Class UsernameValidationTest
+ *
+ * Tests for username validation security fixes.
+ * Covers: GHSA-h756-wh59-hhjv (path traversal), GHSA-cjcp-qxvg-4rjm (uniqueness)
+ *
+ * Naming convention: test{Method}_{GHSA_ID}_{description}
+ */
+class UsernameValidationTest extends \PHPUnit\Framework\TestCase
+{
+    // =========================================================================
+    // GHSA-h756-wh59-hhjv: Path Traversal in Username Creation
+    // =========================================================================
+
+    /**
+     * @dataProvider providerGHSAh756_PathTraversalUsernames
+     */
+    public function testIsValidUsername_GHSAh756_BlocksPathTraversal(string $username, string $description): void
+    {
+        $result = User::isValidUsername($username);
+        self::assertFalse($result, "Should block path traversal: $description");
+    }
+
+    public static function providerGHSAh756_PathTraversalUsernames(): array
+    {
+        return [
+            // Basic path traversal attempts
+            ['../admin', 'Unix path traversal to parent'],
+            ['..\\admin', 'Windows path traversal to parent'],
+            ['../../etc/passwd', 'Multiple level traversal'],
+            ['..\\..\\windows\\system32', 'Windows multi-level traversal'],
+
+            // Path traversal in middle of username
+            ['foo/../bar', 'Traversal in middle'],
+            ['foo\\..\\bar', 'Windows traversal in middle'],
+
+            // Encoded and variant attempts
+            ['..', 'Just double dots'],
+            ['...', 'Triple dots containing double'],
+
+            // Attempts to escape accounts directory
+            ['../accounts/admin', 'Escape to accounts directory'],
+            ['..\\accounts\\admin', 'Windows escape to accounts'],
+            ['../config/system', 'Escape to config directory'],
+        ];
+    }
+
+    // =========================================================================
+    // GHSA-h756-wh59-hhjv: Dangerous Characters in Username
+    // =========================================================================
+
+    /**
+     * @dataProvider providerGHSAh756_DangerousCharacters
+     */
+    public function testIsValidUsername_GHSAh756_BlocksDangerousCharacters(string $username, string $description): void
+    {
+        $result = User::isValidUsername($username);
+        self::assertFalse($result, "Should block dangerous character: $description");
+    }
+
+    public static function providerGHSAh756_DangerousCharacters(): array
+    {
+        return [
+            // Filesystem dangerous characters
+            ['user/name', 'Forward slash'],
+            ['user\\name', 'Backslash'],
+            ['user?name', 'Question mark'],
+            ['user*name', 'Asterisk wildcard'],
+            ['user:name', 'Colon'],
+            ['user;name', 'Semicolon'],
+            ['user{name', 'Opening brace'],
+            ['user}name', 'Closing brace'],
+            ["user\nname", 'Newline character'],
+
+            // Hidden files (starting with dot)
+            ['.htaccess', 'Hidden file .htaccess'],
+            ['.env', 'Hidden file .env'],
+            ['.gitignore', 'Hidden file .gitignore'],
+            ['.hidden', 'Generic hidden file'],
+        ];
+    }
+
+    // =========================================================================
+    // GHSA-cjcp-qxvg-4rjm: Username Uniqueness (Empty Username)
+    // =========================================================================
+
+    public function testIsValidUsername_GHSAcjcp_BlocksEmptyUsername(): void
+    {
+        self::assertFalse(User::isValidUsername(''), 'Empty username should be invalid');
+    }
+
+    // =========================================================================
+    // Valid Usernames (Should Pass Validation)
+    // =========================================================================
+
+    /**
+     * @dataProvider providerValidUsernames
+     */
+    public function testIsValidUsername_AllowsValidUsernames(string $username, string $description): void
+    {
+        $result = User::isValidUsername($username);
+        self::assertTrue($result, "Should allow valid username: $description");
+    }
+
+    public static function providerValidUsernames(): array
+    {
+        return [
+            // Standard usernames
+            ['admin', 'Simple admin username'],
+            ['john_doe', 'Username with underscore'],
+            ['john-doe', 'Username with hyphen'],
+            ['john.doe', 'Username with single dot (not at start)'],
+            ['user123', 'Username with numbers'],
+            ['JohnDoe', 'Mixed case username'],
+
+            // Unicode usernames
+            ['用户名', 'Chinese characters'],
+            ['пользователь', 'Cyrillic characters'],
+            ['ユーザー', 'Japanese characters'],
+            ['müller', 'German umlaut'],
+            ['josé', 'Spanish accent'],
+
+            // Edge cases that should be valid
+            ['a', 'Single character'],
+            ['ab', 'Two characters'],
+            ['user.name.here', 'Multiple dots (not traversal)'],
+            ['123456', 'All numbers'],
+            ['user_name_with_many_underscores', 'Many underscores'],
+        ];
+    }
+
+    // =========================================================================
+    // Boundary Tests
+    // =========================================================================
+
+    public function testIsValidUsername_BoundaryDotPosition(): void
+    {
+        // Dot at start is invalid (hidden file)
+        self::assertFalse(User::isValidUsername('.user'), 'Dot at start should be invalid');
+
+        // Dot in middle is valid
+        self::assertTrue(User::isValidUsername('user.name'), 'Dot in middle should be valid');
+
+        // Dot at end is valid
+        self::assertTrue(User::isValidUsername('user.'), 'Dot at end should be valid');
+    }
+
+    public function testIsValidUsername_BoundaryDoubleDotsPosition(): void
+    {
+        // Double dots anywhere should be invalid (path traversal)
+        self::assertFalse(User::isValidUsername('..user'), 'Double dots at start');
+        self::assertFalse(User::isValidUsername('user..name'), 'Double dots in middle');
+        self::assertFalse(User::isValidUsername('user..'), 'Double dots at end');
+    }
+
+    // =========================================================================
+    // Combined Attack Vectors
+    // =========================================================================
+
+    /**
+     * @dataProvider providerCombinedAttacks
+     */
+    public function testIsValidUsername_BlocksCombinedAttacks(string $username, string $description): void
+    {
+        $result = User::isValidUsername($username);
+        self::assertFalse($result, "Should block combined attack: $description");
+    }
+
+    public static function providerCombinedAttacks(): array
+    {
+        return [
+            ['../../../etc/passwd', 'Deep path traversal'],
+            ['..\\..\\..\\windows\\system32\\config\\sam', 'Windows deep traversal'],
+            ['./../admin', 'Hidden file + traversal'],
+            ['admin/../../../root', 'Valid prefix + deep traversal'],
+            ["admin\n../etc/passwd", 'Newline injection + traversal'],
+            ['admin;rm -rf /', 'Semicolon command separator'],
+            ['admin/etc/passwd', 'Slash in username'],
+        ];
+    }
+}