feat(security): use richdocuments.federation_allowlist system config for federation server validation

.gitignore

@@ -23,3 +23,6 @@ node_modules
 
 .idea/
 .pnpm-store/
+
+# Superpowers planning artifacts
+docs/superpowers/

lib/AppInfo/Application.php

@@ -23,16 +23,9 @@ class Application extends App {
 	public function __construct(array $urlParams = []) {
 		parent::__construct('richdocuments', $urlParams);
 
-		$this->registerServices();
-	}
-
-	private function registerServices() {
 		$container = $this->getContainer();
 		$server = $container->getServer();
 
-		/**
-		 * Core
-		 */
 		$container->registerService('L10N', function (SimpleContainer $c) use ($server) {
 			return $server->getL10N($c->query('AppName'));
 		});

lib/FederationService.php

@@ -21,34 +21,34 @@
  */
 namespace OCA\Richdocuments;
 
+use OCP\IConfig;
 use OCP\ILogger;
 use OCP\Http\Client\IClientService;
 use OCP\IURLGenerator;
 
 class FederationService {
-	/**
-	 * @var ILogger
-	 */
+	/** @var ILogger */
 	private $logger;
 
-	/**
-	 * @var IURLGenerator
-	 */
+	/** @var IURLGenerator */
 	private $urlGenerator;
 
-	/**
-	 * @var IClientService
-	 */
+	/** @var IClientService */
 	private $httpClient;
 
+	/** @var IConfig */
+	private $config;
+
 	public function __construct(
 		ILogger $logger,
 		IURLGenerator $urlGenerator,
-		IClientService $httpClient
+		IClientService $httpClient,
+		IConfig $config
 	) {
-		$this->logger = $logger;
+		$this->logger       = $logger;
 		$this->urlGenerator = $urlGenerator;
-		$this->httpClient = $httpClient;
+		$this->httpClient   = $httpClient;
+		$this->config       = $config;
 	}
 
 	/**
@@ -69,13 +69,12 @@ public function getRemoteFileUrl(string $shareToken, string $shareRelativePath,
 			'&accessToken=' . $accessToken;
 		return $remoteFileUrl;
 	}
-	
+
 	/**
-	*
-	* @param string $server addres of a remote server
-	* @param string $accessToken wopi access token from a remote server
-	* @return array|null with additional wopi information
-	*/
+	 * @param string $server address of a remote server
+	 * @param string $accessToken wopi access token from a remote server
+	 * @return array|null with additional wopi information
+	 */
 	public function getWopiForToken($server, $accessToken) {
 		$remote = $server;
 
@@ -114,15 +113,33 @@ public function getWopiForToken($server, $accessToken) {
 	}
 
 	/**
-	 * Check if server is allowed
+	 * Check if the given server URL matches an entry in the richdocuments.federation_allowlist
+	 * system config. Allowlist entries are plain domain names (no scheme).
+	 *
+	 * Returns false when the key is absent, empty, or not an array.
+	 *
+	 * Known limitation: path-prefixed installs (e.g. cloud.example.com/owncloud) are not
+	 * supported — the path component will prevent matching a bare domain entry.
 	 *
 	 * @param string $remote a remote url
-	 * @return bool indicating if given remote is allowed server
+	 * @return bool
 	 */
-	public function isServerAllowed($remote) {
-		// TODO: implement check for trusted server, for a moment all trusted
+	public function isServerAllowed(string $remote): bool {
+		$allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []);
+
+		if (!\is_array($allowlist) || empty($allowlist)) {
+			return false;
+		}
+
+		$domain = \rtrim((string)\preg_replace('|^https?://|', '', $remote), '/');
+
+		foreach ($allowlist as $entry) {
+			if ($domain === \rtrim((string)$entry, '/')) {
+				return true;
+			}
+		}
 
-		return true;
+		return false;
 	}
 
 	/**

tests/unit/FederationServiceTest.php

@@ -23,63 +23,51 @@
 
 use OCA\Richdocuments\FederationService;
 use OCP\Http\Client\IClientService;
+use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IURLGenerator;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class FederationServiceTest extends TestCase {
-	/**
-	 * The ILogger instance.
-	 *
-	 * @var ILogger
-	 */
+	/** @var ILogger|MockObject */
 	private $logger;
 
-	/**
-	 * The IClientService instance.
-	 *
-	 * @var IClientService
-	 */
+	/** @var IClientService|MockObject */
 	private $httpClient;
 
-	/**
-	 * The IURLGenerator instance.
-	 *
-	 * @var IURLGenerator
-	 */
+	/** @var IURLGenerator|MockObject */
 	private $urlGenerator;
 
-	/**
-	 * @var FederationService|MockObject The discovery service mock object.
-	 */
+	/** @var IConfig|MockObject */
+	private $config;
+
+	/** @var FederationService */
 	private $federationService;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
-		$this->logger = $this->createMock(ILogger::class);
-		$this->httpClient = $this->createMock(IClientService::class);
+		$this->logger       = $this->createMock(ILogger::class);
+		$this->httpClient   = $this->createMock(IClientService::class);
+		$this->config       = $this->createMock(IConfig::class);
 
 		$this->federationService = new FederationService(
 			$this->logger,
 			$this->urlGenerator,
-			$this->httpClient
+			$this->httpClient,
+			$this->config
 		);
 	}
 
+	// -------------------------------------------------------------------------
+	// Existing tests
+	// -------------------------------------------------------------------------
+
 	public function dataGenerateFederatedCloudID() {
-		$userPrefix = [
-			'username',
-			'1234'
-		];
-		$remotes = [
-			'localhost',
-			'local.host',
-			'dev.local.host',
-			'127.0.0.1',
-		];
+		$userPrefix = ['username', '1234'];
+		$remotes    = ['localhost', 'local.host', 'dev.local.host', '127.0.0.1'];
 
 		$testCases = [];
 		foreach ($userPrefix as $user) {
@@ -92,9 +80,6 @@ public function dataGenerateFederatedCloudID() {
 
 	/**
 	 * @dataProvider dataGenerateFederatedCloudID
-	 *
-	 * @param string $userId
-	 * @param string $expectedFederatedCloudID
 	 */
 	public function testSplitUserRemote($userId, $remote) {
 		$this->urlGenerator->method('getAbsoluteUrl')
@@ -105,4 +90,72 @@ public function testSplitUserRemote($userId, $remote) {
 
 		$this->assertSame("{$userId}@{$remote}", $federatedCloudID);
 	}
+
+	// -------------------------------------------------------------------------
+	// isServerAllowed() tests
+	// -------------------------------------------------------------------------
+
+	public function testIsServerAllowedReturnsFalseWhenKeyIsAbsent(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn([]);
+
+		$this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com'));
+	}
+
+	public function testIsServerAllowedReturnsFalseForNonArrayConfig(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn('not-an-array');
+
+		$this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com'));
+	}
+
+	public function testIsServerAllowedReturnsTrueForExactMatch(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedStripsTrailingSlash(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/'));
+	}
+
+	public function testIsServerAllowedStripsMultipleTrailingSlashes(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
+	}
+
+	public function testIsServerAllowedMatchesHttps(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedMatchesHttp(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
+		$this->config->method('getSystemValue')
+			->with('richdocuments.federation_allowlist', [])
+			->willReturn(['trusted.example.com']);
+
+		$this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
+	}
 }