fix(avatar): add timeout and size limit when fetching remote avatars

lib/private/Avatar/AvatarManager.php

@@ -60,7 +60,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);
 			}
 

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,26 +93,41 @@ 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');
 		if (str_starts_with($contentType, 'image/') === false) {
 			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',

tests/lib/Avatar/AvatarManagerTest.php

@@ -285,6 +285,9 @@ public function testGetAvatarForRemoteUser(): void {
 			->expects($this->once())
 			->method('get')
 			->willReturn(null);
+		$this->getAutoMock(IUserSession::class)
+			->method('getUser')
+			->willReturn($this->createMock(IUser::class));
 
 		$resolvedCloudId = $this->createMock(ICloudId::class);
 		$resolvedCloudId->method('getUser')->willReturn('user');
@@ -311,6 +314,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->getAutoMock(Manager::class)
+			->method('get')
+			->willReturn(null);
+		$this->getAutoMock(IUserSession::class)
+			->method('getUser')
+			->willReturn(null);
+		$this->getAutoMock(ICloudIdManager::class)
+			->method('isValidCloudId')
+			->willReturn(true);
+		$this->getAutoMock(IAppData::class)->expects($this->never())->method('getFolder');
+		$this->getAutoMock(IAppData::class)->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');
@@ -320,6 +342,9 @@ public function testGetAvatarThrowsForUnknownUserThatIsNotACloudId(): void {
 			->method('get')
 			->with('invalidUser')
 			->willReturn(null);
+		$this->getAutoMock(IUserSession::class)
+			->method('getUser')
+			->willReturn($this->createMock(IUser::class));
 
 		$this->getAutoMock(ICloudIdManager::class)->expects($this->once())
 			->method('isValidCloudId')

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());
 	}