From 330c3e2317f59a0b55fd632e992bf5656d7617a8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Tue, 12 May 2026 16:28:09 +0200
Subject: [PATCH 1/6] fix(security): validate federation server against
 trusted-servers list
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replaces the always-true isServerAllowed() stub with a real check against
ownCloud's federation trusted-servers list (OCA\Federation\TrustedServers).

When TrustedServers is null (federation app not installed), all remote
federation requests are denied. URL comparison strips trailing slashes and
checks both http/https scheme variants to tolerate minor admin-input
differences.

Fixes OC10-100 (SSRF via federated endpoint, CVSS 7.1).

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 lib/FederationService.php            |  69 +++++++++------
 tests/unit/FederationServiceTest.php | 121 +++++++++++++++++++--------
 2 files changed, 131 insertions(+), 59 deletions(-)

diff --git a/lib/FederationService.php b/lib/FederationService.php
index 3f042f0bf..3074721cc 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -21,34 +21,34 @@
  */
 namespace OCA\Richdocuments;
 
+use OCA\Federation\TrustedServers;
 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 TrustedServers|null */
+	private $trustedServers;
+
 	public function __construct(
 		ILogger $logger,
 		IURLGenerator $urlGenerator,
-		IClientService $httpClient
+		IClientService $httpClient,
+		?TrustedServers $trustedServers
 	) {
-		$this->logger = $logger;
-		$this->urlGenerator = $urlGenerator;
-		$this->httpClient = $httpClient;
+		$this->logger         = $logger;
+		$this->urlGenerator   = $urlGenerator;
+		$this->httpClient     = $httpClient;
+		$this->trustedServers = $trustedServers;
 	}
 
 	/**
@@ -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,37 @@ public function getWopiForToken($server, $accessToken) {
 	}
 
 	/**
-	 * Check if server is allowed
+	 * Check if the given server URL is in ownCloud's trusted-servers list.
+	 *
+	 * Returns false when the federation app is not installed ($trustedServers is null)
+	 * or when the server is not in the trusted list. Checks both the URL as-is (after
+	 * stripping trailing slashes) and the http/https scheme variant to tolerate minor
+	 * mismatches between how the admin stored the URL and how the request arrives.
 	 *
 	 * @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 {
+		if ($this->trustedServers === null) {
+			return false;
+		}
+
+		$normalized = \rtrim($remote, '/');
+
+		if ($this->trustedServers->isTrustedServer($normalized)) {
+			return true;
+		}
+
+		// swap scheme and try again
+		if (\strpos($normalized, 'https://') === 0) {
+			$swapped = 'http://' . \substr($normalized, 8);
+		} elseif (\strpos($normalized, 'http://') === 0) {
+			$swapped = 'https://' . \substr($normalized, 7);
+		} else {
+			return false;
+		}
 
-		return true;
+		return $this->trustedServers->isTrustedServer($swapped);
 	}
 
 	/**
diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index 7efb59459..682d9e454 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -21,6 +21,7 @@
  */
 namespace OCA\Richdocuments\Tests;
 
+use OCA\Federation\TrustedServers;
 use OCA\Richdocuments\FederationService;
 use OCP\Http\Client\IClientService;
 use OCP\ILogger;
@@ -29,57 +30,44 @@
 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 TrustedServers|MockObject */
+	private $trustedServers;
+
+	/** @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->urlGenerator   = $this->createMock(IURLGenerator::class);
+		$this->logger         = $this->createMock(ILogger::class);
+		$this->httpClient     = $this->createMock(IClientService::class);
+		$this->trustedServers = $this->createMock(TrustedServers::class);
 
 		$this->federationService = new FederationService(
 			$this->logger,
 			$this->urlGenerator,
-			$this->httpClient
+			$this->httpClient,
+			$this->trustedServers
 		);
 	}
 
+	// -------------------------------------------------------------------------
+	// 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,70 @@ public function testSplitUserRemote($userId, $remote) {
 
 		$this->assertSame("{$userId}@{$remote}", $federatedCloudID);
 	}
+
+	// -------------------------------------------------------------------------
+	// isServerAllowed() tests
+	// -------------------------------------------------------------------------
+
+	public function testIsServerAllowedReturnsFalseWhenTrustedServersIsNull(): void {
+		$service = new FederationService(
+			$this->logger,
+			$this->urlGenerator,
+			$this->httpClient,
+			null
+		);
+
+		$this->assertFalse($service->isServerAllowed('https://remote.example.com'));
+	}
+
+	public function testIsServerAllowedReturnsTrueForExactMatch(): void {
+		$this->trustedServers->method('isTrustedServer')
+			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedStripsTrailingSlash(): void {
+		$this->trustedServers->method('isTrustedServer')
+			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/'));
+	}
+
+	public function testIsServerAllowedStripsMultipleTrailingSlashes(): void {
+		$this->trustedServers->method('isTrustedServer')
+			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
+	}
+
+	public function testIsServerAllowedSwapsHttpToHttps(): void {
+		// Admin stored https://, request arrives as http://
+		$this->trustedServers->method('isTrustedServer')
+			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+
+		$this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedSwapsHttpsToHttp(): void {
+		// Admin stored http://, request arrives as https://
+		$this->trustedServers->method('isTrustedServer')
+			->willReturnCallback(fn($url) => $url === 'http://trusted.example.com');
+
+		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
+	}
+
+	public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
+		$this->trustedServers->method('isTrustedServer')
+			->willReturn(false);
+
+		$this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
+	}
+
+	public function testIsServerAllowedReturnsFalseWhenListIsEmpty(): void {
+		$this->trustedServers->method('isTrustedServer')
+			->willReturn(false);
+
+		$this->assertFalse($this->federationService->isServerAllowed('https://any.server.com'));
+	}
 }

From 9ed6f1902a8612d4056f3418f953c96c639a8371 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Tue, 12 May 2026 17:10:46 +0200
Subject: [PATCH 2/6] fix(security): wire TrustedServers injection for
 FederationService DI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Explicitly register FederationService in the app container so that
TrustedServers (from the federation app) is injected when available,
and null is passed when the federation app is not installed.

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 lib/AppInfo/Application.php | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 6c37c964e..19b10f3f2 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -14,6 +14,7 @@
 namespace OCA\Richdocuments\AppInfo;
 
 use OCA\Richdocuments\AppConfig;
+use OCA\Richdocuments\FederationService;
 use OCP\AppFramework\App;
 use OC\AppFramework\Utility\SimpleContainer;
 use OCP\Share;
@@ -36,6 +37,23 @@ private function registerServices() {
 		$container->registerService('L10N', function (SimpleContainer $c) use ($server) {
 			return $server->getL10N($c->query('AppName'));
 		});
+
+		/**
+		 * FederationService — inject TrustedServers only when the federation app is installed.
+		 * When null, FederationService::isServerAllowed() denies all remote servers (secure default).
+		 */
+		$container->registerService(FederationService::class, function () use ($server) {
+			$trustedServers = null;
+			if ($server->getAppManager()->isInstalled('federation')) {
+				$trustedServers = \OC::$server->query(\OCA\Federation\TrustedServers::class);
+			}
+			return new FederationService(
+				$server->getLogger(),
+				$server->getURLGenerator(),
+				$server->getHTTPClientService(),
+				$trustedServers
+			);
+		});
 	}
 
 	public function registerScripts() {

From da83c50c1d4a163099266704abf465400ea30473 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Wed, 13 May 2026 10:25:41 +0200
Subject: [PATCH 3/6] fix(review): use injected \$server over \OC::\$server,
 drop redundant test
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Application.php: replace \OC::$server->query() with the already-captured
  $server variable for consistency and testability
- FederationServiceTest: remove testIsServerAllowedReturnsFalseWhenListIsEmpty
  which was identical to testIsServerAllowedReturnsFalseForUntrustedServer

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 lib/AppInfo/Application.php          | 2 +-
 tests/unit/FederationServiceTest.php | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 19b10f3f2..5be87dd22 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -45,7 +45,7 @@ private function registerServices() {
 		$container->registerService(FederationService::class, function () use ($server) {
 			$trustedServers = null;
 			if ($server->getAppManager()->isInstalled('federation')) {
-				$trustedServers = \OC::$server->query(\OCA\Federation\TrustedServers::class);
+				$trustedServers = $server->query(\OCA\Federation\TrustedServers::class);
 			}
 			return new FederationService(
 				$server->getLogger(),
diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index 682d9e454..7db3c7ace 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -150,10 +150,4 @@ public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
 		$this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
 	}
 
-	public function testIsServerAllowedReturnsFalseWhenListIsEmpty(): void {
-		$this->trustedServers->method('isTrustedServer')
-			->willReturn(false);
-
-		$this->assertFalse($this->federationService->isServerAllowed('https://any.server.com'));
-	}
 }

From d76b4013f630a8adcd9243d76bbc9ec015f130ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Wed, 13 May 2026 10:55:20 +0200
Subject: [PATCH 4/6] fix(style): add space after fn keyword in arrow functions
 (phpcs)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 tests/unit/FederationServiceTest.php | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index 7db3c7ace..b0e536d3f 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -108,21 +108,21 @@ public function testIsServerAllowedReturnsFalseWhenTrustedServersIsNull(): void
 
 	public function testIsServerAllowedReturnsTrueForExactMatch(): void {
 		$this->trustedServers->method('isTrustedServer')
-			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+			->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com');
 
 		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
 	}
 
 	public function testIsServerAllowedStripsTrailingSlash(): void {
 		$this->trustedServers->method('isTrustedServer')
-			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+			->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com');
 
 		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/'));
 	}
 
 	public function testIsServerAllowedStripsMultipleTrailingSlashes(): void {
 		$this->trustedServers->method('isTrustedServer')
-			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+			->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com');
 
 		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
 	}
@@ -130,7 +130,7 @@ public function testIsServerAllowedStripsMultipleTrailingSlashes(): void {
 	public function testIsServerAllowedSwapsHttpToHttps(): void {
 		// Admin stored https://, request arrives as http://
 		$this->trustedServers->method('isTrustedServer')
-			->willReturnCallback(fn($url) => $url === 'https://trusted.example.com');
+			->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com');
 
 		$this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
 	}
@@ -138,7 +138,7 @@ public function testIsServerAllowedSwapsHttpToHttps(): void {
 	public function testIsServerAllowedSwapsHttpsToHttp(): void {
 		// Admin stored http://, request arrives as https://
 		$this->trustedServers->method('isTrustedServer')
-			->willReturnCallback(fn($url) => $url === 'http://trusted.example.com');
+			->willReturnCallback(fn ($url) => $url === 'http://trusted.example.com');
 
 		$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
 	}

From 97ff361437adcb40fe8aa488077071cac0a7aa66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Wed, 13 May 2026 11:09:04 +0200
Subject: [PATCH 5/6] fix(phan): suppress undeclared-class warnings for
 OCA\Federation\TrustedServers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The federation app is an optional runtime dependency not present in
Phan's analysis path. Suppress PhanUndeclaredClass* at the five sites
that reference TrustedServers, matching the project's established
inline-suppression pattern.

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 lib/AppInfo/Application.php | 1 +
 lib/FederationService.php   | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 5be87dd22..738468d02 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -45,6 +45,7 @@ private function registerServices() {
 		$container->registerService(FederationService::class, function () use ($server) {
 			$trustedServers = null;
 			if ($server->getAppManager()->isInstalled('federation')) {
+				/* @phan-suppress-next-line PhanUndeclaredClassReference */
 				$trustedServers = $server->query(\OCA\Federation\TrustedServers::class);
 			}
 			return new FederationService(
diff --git a/lib/FederationService.php b/lib/FederationService.php
index 3074721cc..3bef1c5ad 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -36,6 +36,7 @@ class FederationService {
 	/** @var IClientService */
 	private $httpClient;
 
+	/* @phan-suppress-next-line PhanUndeclaredTypeProperty */
 	/** @var TrustedServers|null */
 	private $trustedServers;
 
@@ -43,6 +44,7 @@ public function __construct(
 		ILogger $logger,
 		IURLGenerator $urlGenerator,
 		IClientService $httpClient,
+		/* @phan-suppress-next-line PhanUndeclaredTypeParameter */
 		?TrustedServers $trustedServers
 	) {
 		$this->logger         = $logger;
@@ -130,6 +132,7 @@ public function isServerAllowed(string $remote): bool {
 
 		$normalized = \rtrim($remote, '/');
 
+		/* @phan-suppress-next-line PhanUndeclaredClassMethod */
 		if ($this->trustedServers->isTrustedServer($normalized)) {
 			return true;
 		}
@@ -143,6 +146,7 @@ public function isServerAllowed(string $remote): bool {
 			return false;
 		}
 
+		/* @phan-suppress-next-line PhanUndeclaredClassMethod */
 		return $this->trustedServers->isTrustedServer($swapped);
 	}
 

From b67fa7f25bc4434bc49c437070ccef18cc0b10ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
 <1005065+DeepDiver1975@users.noreply.github.com>
Date: Wed, 13 May 2026 11:27:09 +0200
Subject: [PATCH 6/6] fix(phan): move PhanUndeclaredTypeProperty suppress to
 property declaration line
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
 lib/FederationService.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/FederationService.php b/lib/FederationService.php
index 3bef1c5ad..b2c94bb3c 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -36,8 +36,8 @@ class FederationService {
 	/** @var IClientService */
 	private $httpClient;
 
-	/* @phan-suppress-next-line PhanUndeclaredTypeProperty */
 	/** @var TrustedServers|null */
+	/* @phan-suppress-next-line PhanUndeclaredTypeProperty */
 	private $trustedServers;
 
 	public function __construct(