diff --git a/lib/private/Avatar/AvatarManager.php b/lib/private/Avatar/AvatarManager.php index 2f5008a7b21fb..e37b6fef4ee68 100644 --- a/lib/private/Avatar/AvatarManager.php +++ b/lib/private/Avatar/AvatarManager.php @@ -57,7 +57,8 @@ public function __construct( public function getAvatar(string $userId): IAvatar { $user = $this->userManager->get($userId); if ($user === null) { - if ($this->cloudIdManager->isValidCloudId($userId)) { + // Remote avatars are only fetched for logged-in users, e.g. to show the owner of a federated share + if ($this->userSession->getUser() !== null && $this->cloudIdManager->isValidCloudId($userId)) { return $this->getRemoteAvatar($userId); } diff --git a/lib/private/Avatar/RemoteAvatar.php b/lib/private/Avatar/RemoteAvatar.php index 50eb5cf8f6747..2d4d5fc154def 100644 --- a/lib/private/Avatar/RemoteAvatar.php +++ b/lib/private/Avatar/RemoteAvatar.php @@ -20,6 +20,7 @@ class RemoteAvatar extends Avatar { private const int IMAGE_CACHE_AGE = 60 * 60 * 24; // One day + private const int MAX_AVATAR_SIZE = 2 * 1024 * 1024; private ICloudId $cloudId; @@ -92,7 +93,9 @@ public function getFile(int $size, bool $darkTheme = false): ISimpleFile { $clientService = Server::get(IClientService::class); $client = $clientService->newClient(); $response = $client->get($url, [ - 'verify' => !$this->config->getSystemValueBool('sharing.federation.allowSelfSignedCertificates', false) + 'verify' => !$this->config->getSystemValueBool('sharing.federation.allowSelfSignedCertificates', false), + 'timeout' => 5, + 'stream' => true, ]); $contentType = $response->getHeader('Content-Type'); @@ -100,18 +103,31 @@ public function getFile(int $size, bool $darkTheme = false): ISimpleFile { throw new \Exception('Unknown filetype'); } + // The header only has the encoded size of compressed responses, so the body is limited while reading it too + $contentLength = $response->getHeader('Content-Length'); + if (!ctype_digit($contentLength)) { + throw new \Exception('Invalid remote avatar size'); + } + if ((int)$contentLength > self::MAX_AVATAR_SIZE) { + throw new \Exception('Remote avatar too large'); + } + $avatar = $response->getBody(); if ($avatar === null) { throw new \Exception('Failed to fetch remote avatar'); } if (is_resource($avatar)) { - $avatar = stream_get_contents($avatar); + $avatar = stream_get_contents($avatar, self::MAX_AVATAR_SIZE + 1); if ($avatar === false) { throw new \Exception('Failed to fetch remote avatar'); } } + if (strlen($avatar) > self::MAX_AVATAR_SIZE) { + throw new \Exception('Remote avatar too large'); + } + $ext = match ($contentType) { 'image/png' => 'png', 'image/jpg', 'image/jpeg' => 'jpg', diff --git a/tests/lib/Avatar/AvatarManagerTest.php b/tests/lib/Avatar/AvatarManagerTest.php index 63011e40bd15d..ee833d8ab7f19 100644 --- a/tests/lib/Avatar/AvatarManagerTest.php +++ b/tests/lib/Avatar/AvatarManagerTest.php @@ -290,6 +290,9 @@ public function testGetAvatarForRemoteUser(): void { ->expects($this->once()) ->method('get') ->willReturn(null); + $this->userSession + ->method('getUser') + ->willReturn($this->createMock(IUser::class)); $resolvedCloudId = $this->createMock(ICloudId::class); $resolvedCloudId->method('getUser')->willReturn('user'); @@ -316,6 +319,25 @@ public function testGetAvatarForRemoteUser(): void { $this->assertSame('user@remote.example.com', $avatar->getDisplayName()); } + public function testGetAvatarForRemoteUserWithoutSession(): void { + $this->expectException(\Exception::class); + $this->expectExceptionMessage('user does not exist'); + + $this->userManager + ->method('get') + ->willReturn(null); + $this->userSession + ->method('getUser') + ->willReturn(null); + $this->cloudIdManager + ->method('isValidCloudId') + ->willReturn(true); + $this->appData->expects($this->never())->method('getFolder'); + $this->appData->expects($this->never())->method('newFolder'); + + $this->avatarManager->getAvatar('user@https://remote.example.com'); + } + public function testGetAvatarThrowsForUnknownUserThatIsNotACloudId(): void { $this->expectException(\Exception::class); $this->expectExceptionMessage('user does not exist'); @@ -325,6 +347,9 @@ public function testGetAvatarThrowsForUnknownUserThatIsNotACloudId(): void { ->method('get') ->with('invalidUser') ->willReturn(null); + $this->userSession + ->method('getUser') + ->willReturn($this->createMock(IUser::class)); $this->cloudIdManager->expects($this->once()) ->method('isValidCloudId') diff --git a/tests/lib/Avatar/RemoteAvatarTest.php b/tests/lib/Avatar/RemoteAvatarTest.php index fb008746fb183..34d93d8e8bb07 100644 --- a/tests/lib/Avatar/RemoteAvatarTest.php +++ b/tests/lib/Avatar/RemoteAvatarTest.php @@ -63,9 +63,12 @@ protected function setUp(): void { * * @param string|resource|false $body */ - private function mockRemoteClient(string $contentType, string $body, ?string $expectedUrl = null, ?array $expectedOptions = null): void { + private function mockRemoteClient(string $contentType, mixed $body, ?string $expectedUrl = null, ?array $expectedOptions = null, string $contentLength = ''): void { $response = $this->createMock(IResponse::class); - $response->method('getHeader')->with('Content-Type')->willReturn($contentType); + $response->method('getHeader')->willReturnMap([ + ['Content-Type', $contentType], + ['Content-Length', $contentLength], + ]); $response->method('getBody')->willReturn($body); $client = $this->createMock(IClient::class); @@ -97,7 +100,8 @@ public function testGetFileFetchesAvatarFromRemoteInstance(): void { 'image/png', $fileContents, 'https://remote.example.com/index.php/avatar/user/64', - ['verify' => true], + ['verify' => true, 'timeout' => 5, 'stream' => true], + (string)strlen($fileContents), ); $expectedFile = $this->createMock(ISimpleFile::class); @@ -139,6 +143,56 @@ public function testGetFileThrowsOnUnexpectedContentType(): void { $this->avatar->getFile(64); } + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestGetFileRejectsInvalidContentLength')] + public function testGetFileRejectsInvalidContentLength(string $contentLength, string $message): void { + $this->mockRemoteClient('image/png', 'png-bytes', contentLength: $contentLength); + $this->folder->expects($this->never())->method('newFile'); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage($message); + + $this->avatar->getFile(64); + } + + public static function dataTestGetFileRejectsInvalidContentLength(): array { + return [ + 'too large' => [(string)(3 * 1024 * 1024), 'Remote avatar too large'], + 'not a number' => ['abc', 'Invalid remote avatar size'], + 'exponent' => ['1e3', 'Invalid remote avatar size'], + 'negative' => ['-1', 'Invalid remote avatar size'], + 'missing' => ['', 'Invalid remote avatar size'], + ]; + } + + public function testGetFileRejectsTooLargeBody(): void { + // a compressed response only announces its encoded size, but inflates to more than the limit + $body = fopen('php://memory', 'r+'); + fwrite($body, str_repeat('a', 2 * 1024 * 1024 + 1)); + rewind($body); + $this->mockRemoteClient('image/png', $body, contentLength: '1024'); + $this->folder->expects($this->never())->method('newFile'); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Remote avatar too large'); + + $this->avatar->getFile(64); + } + + public function testGetFileReadsStreamedBody(): void { + $body = fopen('php://memory', 'r+'); + fwrite($body, 'png-bytes'); + rewind($body); + $this->mockRemoteClient('image/png', $body, contentLength: '9'); + + $file = $this->createMock(ISimpleFile::class); + $this->folder->expects($this->once()) + ->method('newFile') + ->with('avatar.64.png', 'png-bytes') + ->willReturn($file); + + $this->assertSame($file, $this->avatar->getFile(64)); + } + public function testIsCustomAvatar(): void { $this->assertTrue($this->avatar->isCustomAvatar()); }