fix(AccountManager): Sanitize social media handles

Ensure to only accept valid X and fediverse handles.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

1 files changed, 200 insertions, 50 deletions

diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php
index 4462c6c0722..fab3aaf5fdd 100644
--- a/tests/lib/Accounts/AccountManagerTest.php
+++ b/tests/lib/Accounts/AccountManagerTest.php
@@ -17,6 +17,9 @@ use OCP\Accounts\UserUpdatedEvent;
 use OCP\BackgroundJob\IJobList;
 use OCP\Defaults;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Http\Client\IClient;
+use OCP\Http\Client\IClientService;
+use OCP\Http\Client\IResponse;
 use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IPhoneNumberUtil;
@@ -37,45 +40,31 @@ use Test\TestCase;
  * @package Test\Accounts
  */
 class AccountManagerTest extends TestCase {
-	/** @var IVerificationToken|MockObject */
-	protected $verificationToken;
-	/** @var IMailer|MockObject */
-	protected $mailer;
-	/** @var ICrypto|MockObject */
-	protected $crypto;
-	/** @var IURLGenerator|MockObject */
-	protected $urlGenerator;
-	/** @var Defaults|MockObject */
-	protected $defaults;
-	/** @var IFactory|MockObject */
-	protected $l10nFactory;
-
-	/** @var IDBConnection */
-	private $connection;
-
-	/** @var IConfig|MockObject */
-	private $config;
-
-	/** @var IEventDispatcher|MockObject */
-	private $eventDispatcher;
-
-	/** @var IJobList|MockObject */
-	private $jobList;
-	/** @var IPhoneNumberUtil */
-	private $phoneNumberUtil;
 
 	/** accounts table name */
 	private string $table = 'accounts';
-
-	/** @var LoggerInterface|MockObject */
-	private $logger;
-
 	private AccountManager $accountManager;
+	private IDBConnection $connection;
+	private IPhoneNumberUtil $phoneNumberUtil;
+
+	protected IVerificationToken&MockObject $verificationToken;
+	protected IMailer&MockObject $mailer;
+	protected ICrypto&MockObject $crypto;
+	protected IURLGenerator&MockObject $urlGenerator;
+	protected Defaults&MockObject $defaults;
+	protected IFactory&MockObject $l10nFactory;
+	protected IConfig&MockObject $config;
+	protected IEventDispatcher&MockObject $eventDispatcher;
+	protected IJobList&MockObject $jobList;
+	private LoggerInterface&MockObject $logger;
+	private IClientService&MockObject $clientService;
 
 	protected function setUp(): void {
 		parent::setUp();
+		$this->connection = \OCP\Server::get(IDBConnection::class);
+		$this->phoneNumberUtil = new PhoneNumberUtil();
+
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
-		$this->connection = \OC::$server->get(IDBConnection::class);
 		$this->config = $this->createMock(IConfig::class);
 		$this->jobList = $this->createMock(IJobList::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
@@ -85,7 +74,7 @@ class AccountManagerTest extends TestCase {
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->crypto = $this->createMock(ICrypto::class);
-		$this->phoneNumberUtil = new PhoneNumberUtil();
+		$this->clientService = $this->createMock(IClientService::class);
 
 		$this->accountManager = new AccountManager(
 			$this->connection,
@@ -100,6 +89,7 @@ class AccountManagerTest extends TestCase {
 			$this->urlGenerator,
 			$this->crypto,
 			$this->phoneNumberUtil,
+			$this->clientService,
 		);
 	}
 
@@ -465,6 +455,7 @@ class AccountManagerTest extends TestCase {
 				$this->urlGenerator,
 				$this->crypto,
 				$this->phoneNumberUtil,
+				$this->clientService,
 			])
 			->onlyMethods($mockedMethods)
 			->getMock();
@@ -684,7 +675,7 @@ class AccountManagerTest extends TestCase {
 		$this->assertEquals($expected, $accountManager->getAccount($user));
 	}
 
-	public function dataParsePhoneNumber(): array {
+	public static function dataParsePhoneNumber(): array {
 		return [
 			['0711 / 25 24 28-90', 'DE', '+4971125242890'],
 			['0711 / 25 24 28-90', '', null],
@@ -695,39 +686,198 @@ class AccountManagerTest extends TestCase {
 	/**
 	 * @dataProvider dataParsePhoneNumber
 	 */
-	public function testParsePhoneNumber(string $phoneInput, string $defaultRegion, ?string $phoneNumber): void {
+	public function testSanitizePhoneNumberOnUpdateAccount(string $phoneInput, string $defaultRegion, ?string $phoneNumber): void {
 		$this->config->method('getSystemValueString')
 			->willReturn($defaultRegion);
 
+		$user = $this->createMock(IUser::class);
+		$account = new Account($user);
+		$account->setProperty(IAccountManager::PROPERTY_PHONE, $phoneInput, IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED);
+		$manager = $this->getInstance(['getUser', 'updateUser']);
+		$manager->method('getUser')
+			->with($user, false)
+			->willReturn([]);
+		$manager->expects($phoneNumber === null ? self::never() : self::once())
+			->method('updateUser');
+
 		if ($phoneNumber === null) {
 			$this->expectException(\InvalidArgumentException::class);
-			self::invokePrivate($this->accountManager, 'parsePhoneNumber', [$phoneInput]);
-		} else {
-			self::assertEquals($phoneNumber, self::invokePrivate($this->accountManager, 'parsePhoneNumber', [$phoneInput]));
+		}
+
+		$manager->updateAccount($account);
+
+		if ($phoneNumber !== null) {
+			self::assertEquals($phoneNumber, $account->getProperty(IAccountManager::PROPERTY_PHONE)->getValue());
 		}
 	}
 
-	public function dataParseWebsite(): array {
+	public static function dataSanitizeOnUpdate(): array {
 		return [
-			['https://nextcloud.com', 'https://nextcloud.com'],
-			['http://nextcloud.com', 'http://nextcloud.com'],
-			['ftp://nextcloud.com', null],
-			['//nextcloud.com/', null],
-			['https:///?query', null],
+			[IAccountManager::PROPERTY_WEBSITE, 'https://nextcloud.com', 'https://nextcloud.com'],
+			[IAccountManager::PROPERTY_WEBSITE, 'http://nextcloud.com', 'http://nextcloud.com'],
+			[IAccountManager::PROPERTY_WEBSITE, 'ftp://nextcloud.com', null],
+			[IAccountManager::PROPERTY_WEBSITE, '//nextcloud.com/', null],
+			[IAccountManager::PROPERTY_WEBSITE, 'https:///?query', null],
+
+			[IAccountManager::PROPERTY_TWITTER, '@nextcloud', 'nextcloud'],
+			[IAccountManager::PROPERTY_TWITTER, '_nextcloud', '_nextcloud'],
+			[IAccountManager::PROPERTY_TWITTER, 'FooB4r', 'FooB4r'],
+			[IAccountManager::PROPERTY_TWITTER, 'X', null],
+			[IAccountManager::PROPERTY_TWITTER, 'next.cloud', null],
+			[IAccountManager::PROPERTY_TWITTER, 'ab/cd.zip', null],
+			[IAccountManager::PROPERTY_TWITTER, 'tooLongForTwitterAndX', null],
+
+			[IAccountManager::PROPERTY_FEDIVERSE, 'nextcloud@mastodon.social', 'nextcloud@mastodon.social'],
+			[IAccountManager::PROPERTY_FEDIVERSE, '@nextcloud@mastodon.xyz', 'nextcloud@mastodon.xyz'],
+			[IAccountManager::PROPERTY_FEDIVERSE, 'l33t.h4x0r@sub.localhost.local', 'l33t.h4x0r@sub.localhost.local'],
+			[IAccountManager::PROPERTY_FEDIVERSE, 'invalid/name@mastodon.social', null],
+			[IAccountManager::PROPERTY_FEDIVERSE, 'name@evil.host/malware.exe', null],
+			[IAccountManager::PROPERTY_FEDIVERSE, '@is-it-a-host-or-name', null],
+			[IAccountManager::PROPERTY_FEDIVERSE, 'only-a-name', null],
 		];
 	}
 
 	/**
-	 * @dataProvider dataParseWebsite
-	 * @param string $websiteInput
-	 * @param string|null $websiteOutput
+	 * @dataProvider dataSanitizeOnUpdate
 	 */
-	public function testParseWebsite(string $websiteInput, ?string $websiteOutput): void {
-		if ($websiteOutput === null) {
+	public function testSanitizingOnUpdateAccount(string $property, string $input, ?string $output): void {
+
+		if ($property === IAccountManager::PROPERTY_FEDIVERSE) {
+			// We do not test the server response here we do this in the `testSanitizingFediverseServer`
+			$this->config
+				->method('getSystemValueBool')
+				->with('has_internet_connection', true)
+				->willReturn(false);
+		}
+
+		$user = $this->createMock(IUser::class);
+
+		$account = new Account($user);
+		$account->setProperty($property, $input, IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED);
+
+		$manager = $this->getInstance(['getUser', 'updateUser']);
+		$manager->method('getUser')
+			->with($user, false)
+			->willReturn([]);
+		$manager->expects($output === null ? self::never() : self::once())
+			->method('updateUser');
+
+		if ($output === null) {
 			$this->expectException(\InvalidArgumentException::class);
-			self::invokePrivate($this->accountManager, 'parseWebsite', [$websiteInput]);
+			$this->expectExceptionMessage($property);
+		}
+
+		$manager->updateAccount($account);
+
+		if ($output !== null) {
+			self::assertEquals($output, $account->getProperty($property)->getValue());
+		}
+	}
+
+	public static function dataSanitizeFediverseServer(): array {
+		return [
+			'no internet' => [
+				'@foo@example.com',
+				'foo@example.com',
+				false,
+				null,
+			],
+			'no internet - no at' => [
+				'foo@example.com',
+				'foo@example.com',
+				false,
+				null,
+			],
+			'valid response' => [
+				'@foo@example.com',
+				'foo@example.com',
+				true,
+				json_encode(['username' => 'foo']),
+			],
+			'valid response - no at' => [
+				'foo@example.com',
+				'foo@example.com',
+				true,
+				json_encode(['username' => 'foo']),
+			],
+			// failures
+			'invalid response' => [
+				'@foo@example.com',
+				null,
+				true,
+				json_encode(['not found']),
+			],
+			'no response' => [
+				'@foo@example.com',
+				null,
+				true,
+				null,
+			],
+			'wrong user' => [
+				'@foo@example.com',
+				null,
+				true,
+				json_encode(['username' => 'foo@other.example.com']),
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataSanitizeFediverseServer
+	 */
+	public function testSanitizingFediverseServer(string $input, ?string $output, bool $hasInternet, ?string $serverResponse): void {
+		$this->config->expects(self::once())
+			->method('getSystemValueBool')
+			->with('has_internet_connection', true)
+			->willReturn($hasInternet);
+
+		if ($hasInternet) {
+			$client = $this->createMock(IClient::class);
+			if ($serverResponse !== null) {
+				$response = $this->createMock(IResponse::class);
+				$response->method('getBody')
+					->willReturn($serverResponse);
+				$client->expects(self::once())
+					->method('get')
+					->with('https://example.com/api/v1/accounts/lookup?acct=foo@example.com')
+					->willReturn($response);
+			} else {
+				$client->expects(self::once())
+					->method('get')
+					->with('https://example.com/api/v1/accounts/lookup?acct=foo@example.com')
+					->willThrowException(new \Exception('404'));
+			}
+
+			$this->clientService
+				->expects(self::once())
+				->method('newClient')
+				->willReturn($client);
 		} else {
-			self::assertEquals($websiteOutput, self::invokePrivate($this->accountManager, 'parseWebsite', [$websiteInput]));
+			$this->clientService
+				->expects(self::never())
+				->method('newClient');
+		}
+
+		$user = $this->createMock(IUser::class);
+		$account = new Account($user);
+		$account->setProperty(IAccountManager::PROPERTY_FEDIVERSE, $input, IAccountManager::SCOPE_LOCAL, IAccountManager::NOT_VERIFIED);
+
+		$manager = $this->getInstance(['getUser', 'updateUser']);
+		$manager->method('getUser')
+			->with($user, false)
+			->willReturn([]);
+		$manager->expects($output === null ? self::never() : self::once())
+			->method('updateUser');
+
+		if ($output === null) {
+			$this->expectException(\InvalidArgumentException::class);
+			$this->expectExceptionMessage(IAccountManager::PROPERTY_FEDIVERSE);
+		}
+
+		$manager->updateAccount($account);
+
+		if ($output !== null) {
+			self::assertEquals($output, $account->getProperty(IAccountManager::PROPERTY_FEDIVERSE)->getValue());
 		}
 	}