diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index 34db783de6833..114ec3627dd75 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -24,6 +24,8 @@ * @template-implements IEventListener */ class DisplayNameCache implements IEventListener { + /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ + public const MAX_USERID_LENGTH = 64; private array $cache = []; private ICache $memCache; private IUserManager $userManager; @@ -37,6 +39,11 @@ public function getDisplayName(string $userId): ?string { if (isset($this->cache[$userId])) { return $this->cache[$userId]; } + + if (strlen($userId) > self::MAX_USERID_LENGTH) { + return null; + } + $displayName = $this->memCache->get($userId); if ($displayName) { $this->cache[$userId] = $displayName; diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 152fb08eeeb9a..03ac7c6c57066 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -53,6 +53,9 @@ * @package OC\User */ class Manager extends PublicEmitter implements IUserManager { + /** @see \OC\Config\UserConfig::USER_MAX_LENGTH */ + public const MAX_USERID_LENGTH = 64; + /** * @var \OCP\UserInterface[] $backends */ @@ -131,6 +134,10 @@ public function get($uid) { return $this->cachedUsers[$uid]; } + if (strlen($uid) > self::MAX_USERID_LENGTH) { + return null; + } + $cachedBackend = $this->cache->get(sha1($uid)); if ($cachedBackend !== null && isset($this->backends[$cachedBackend])) { // Cache has the info of the user backend already, so ask that one directly @@ -190,6 +197,10 @@ public function getUserObject($uid, $backend, $cacheUser = true) { * @return bool */ public function userExists($uid) { + if (strlen($uid) > self::MAX_USERID_LENGTH) { + return false; + } + $user = $this->get($uid); return ($user !== null); } @@ -705,14 +716,14 @@ public function getByEmail($email) { public function validateUserId(string $uid, bool $checkDataDirectory = false): void { $l = Server::get(IFactory::class)->get('lib'); - // Check the name for bad characters + // Check the ID for bad characters // Allowed are: "a-z", "A-Z", "0-9", spaces and "_.@-'" if (preg_match('/[^a-zA-Z0-9 _.@\-\']/', $uid)) { throw new \InvalidArgumentException($l->t('Only the following characters are allowed in an Login:' . ' "a-z", "A-Z", "0-9", spaces and "_.@-\'"')); } - // No empty username + // No empty user ID if (trim($uid) === '') { throw new \InvalidArgumentException($l->t('A valid Login must be provided')); } @@ -722,11 +733,16 @@ public function validateUserId(string $uid, bool $checkDataDirectory = false): v throw new \InvalidArgumentException($l->t('Login contains whitespace at the beginning or at the end')); } - // Username only consists of 1 or 2 dots (directory traversal) + // User ID only consists of 1 or 2 dots (directory traversal) if ($uid === '.' || $uid === '..') { throw new \InvalidArgumentException($l->t('Login must not consist of dots only')); } + // User ID is too long + if (strlen($uid) > self::MAX_USERID_LENGTH) { + throw new \InvalidArgumentException($l->t('Login is too long')); + } + if (!$this->verifyUid($uid, $checkDataDirectory)) { throw new \InvalidArgumentException($l->t('Login is invalid because files already exist for this user')); } diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 53f57eee086a8..4f4b761d1c914 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -79,6 +79,20 @@ public function testUserExistsSingleBackendExists(): void { $this->assertTrue($manager->userExists('foo')); } + public function testUserExistsTooLong(): void { + /** @var \Test\Util\User\Dummy|MockObject $backend */ + $backend = $this->createMock(\Test\Util\User\Dummy::class); + $backend->expects($this->never()) + ->method('userExists') + ->with($this->equalTo('foo')) + ->willReturn(true); + + $manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger); + $manager->registerBackend($backend); + + $this->assertFalse($manager->userExists('foo' . str_repeat('a', 62))); + } + public function testUserExistsSingleBackendNotExists(): void { /** * @var \Test\Util\User\Dummy | \PHPUnit\Framework\MockObject\MockObject $backend @@ -230,6 +244,20 @@ public function testGetOneBackendNotExists(): void { $this->assertEquals(null, $manager->get('foo')); } + public function testGetTooLong(): void { + /** @var \Test\Util\User\Dummy|MockObject $backend */ + $backend = $this->createMock(\Test\Util\User\Dummy::class); + $backend->expects($this->never()) + ->method('userExists') + ->with($this->equalTo('foo')) + ->willReturn(false); + + $manager = new \OC\User\Manager($this->config, $this->cacheFactory, $this->eventDispatcher, $this->logger); + $manager->registerBackend($backend); + + $this->assertEquals(null, $manager->get('foo' . str_repeat('a', 62))); + } + public function testGetOneBackendDoNotTranslateLoginNames(): void { /** * @var \Test\Util\User\Dummy | \PHPUnit\Framework\MockObject\MockObject $backend @@ -333,6 +361,7 @@ public function dataCreateUserInvalid() { ['..', 'foo', 'Username must not consist of dots only'], ['.test', '', 'A valid password must be provided'], ['test', '', 'A valid password must be provided'], + ['test' . str_repeat('a', 61), '', 'Login is too long'], ]; }