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 01/15] 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 02/15] 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 03/15] 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 04/15] 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 05/15] 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 06/15] 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(
From 3653951c64d9eab583c39baa8df745d99dc40222 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 08:45:52 +0200
Subject: [PATCH 07/15] docs: add design spec for federation allowlist via
system config
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>
---
...oc10-federation-allowlist-config-design.md | 115 ++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
diff --git a/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md b/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
new file mode 100644
index 000000000..cbb918494
--- /dev/null
+++ b/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
@@ -0,0 +1,115 @@
+# Federation Allowlist via System Config — Design
+
+**Date:** 2026-05-15
+**Scope:** Replace `OCA\Federation\TrustedServers` with a `config.php` system config allowlist as the sole authority for permitted federation servers in `FederationService`.
+
+---
+
+## Problem
+
+The previous PR (OC10-100) fixed an SSRF by wiring `TrustedServers` from the federation app into `FederationService`. However, this creates a hard runtime dependency on the federation app being installed. Admins who don't use the federation app but do want to permit specific Collabora federation endpoints have no way to configure this.
+
+This PR replaces `TrustedServers` entirely with a flat system config key, giving admins a direct, federation-app-independent allowlist.
+
+---
+
+## Config Key
+
+```php
+// config/config.php
+'richdocuments.federation_allowlist' => [
+ 'https://collab1.example.com',
+ 'https://collab2.example.com',
+],
+```
+
+- Type: `array` of URL strings
+- Location: `config/config.php` (system config), read via `IConfig::getSystemValue()`
+- Default: absent (treated as `[]`) → deny all
+- If set to a non-array value: treated as `[]` → deny all
+
+---
+
+## Architecture
+
+### `FederationService`
+
+**Dependencies change:**
+- Remove: `?TrustedServers $trustedServers`
+- Add: `IConfig $config`
+- Remove: all four Phan suppression comments (no longer needed)
+- Remove: `use OCA\Federation\TrustedServers`
+- Add: `use OCP\IConfig`
+
+**`isServerAllowed(string $remote): bool`**
+
+1. `$allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', [])`
+2. If `!is_array($allowlist) || empty($allowlist)` → return `false`
+3. `$normalized = rtrim($remote, '/')`
+4. For each `$entry` in `$allowlist`:
+ - `$e = rtrim($entry, '/')`
+ - If `$normalized === $e` → return `true`
+ - Compute scheme-swapped variant of `$normalized` (http↔https)
+ - If scheme-swapped variant `=== $e` → return `true`
+5. Return `false`
+
+### `Application.php`
+
+DI registration simplifies — no `isInstalled('federation')` check:
+
+```php
+$container->registerService(FederationService::class, function () use ($server) {
+ return new FederationService(
+ $server->getLogger(),
+ $server->getURLGenerator(),
+ $server->getHTTPClientService(),
+ $server->getConfig()
+ );
+});
+```
+
+The docblock comment about null-as-secure-default is removed (no longer applicable).
+
+---
+
+## Test Cases (`FederationServiceTest`)
+
+Replace `TrustedServers` mock with `IConfig` mock. All `isTrustedServer` setups replaced with `getSystemValue('richdocuments.federation_allowlist', [])` stubs.
+
+| Test | Input | Config value | Expected |
+|---|---|---|---|
+| `testIsServerAllowedReturnsFalseWhenKeyIsAbsent` | any URL | `[]` (default) | `false` |
+| `testIsServerAllowedReturnsFalseWhenListIsEmpty` | any URL | `[]` | `false` |
+| `testIsServerAllowedReturnsFalseForNonArrayConfig` | any URL | `'not-an-array'` | `false` |
+| `testIsServerAllowedReturnsTrueForExactMatch` | `https://trusted.example.com` | `['https://trusted.example.com']` | `true` |
+| `testIsServerAllowedStripsTrailingSlash` | `https://trusted.example.com/` | `['https://trusted.example.com']` | `true` |
+| `testIsServerAllowedStripsMultipleTrailingSlashes` | `https://trusted.example.com///` | `['https://trusted.example.com']` | `true` |
+| `testIsServerAllowedSwapsHttpToHttps` | `http://trusted.example.com` | `['https://trusted.example.com']` | `true` |
+| `testIsServerAllowedSwapsHttpsToHttp` | `https://trusted.example.com` | `['http://trusted.example.com']` | `true` |
+| `testIsServerAllowedReturnsFalseForUntrustedServer` | `https://evil.attacker.com` | `['https://trusted.example.com']` | `false` |
+
+---
+
+## Error Handling
+
+- **Key absent:** `getSystemValue` returns `[]` default → deny all. No exception.
+- **Non-array value:** `is_array()` guard → deny all. No exception.
+- **Malformed entries:** Empty strings or non-URLs in the list never match → harmlessly ignored.
+- **Logging:** Unchanged. Existing `"Server {server} is not allowed."` log lines in `getWopiForToken()` and `getRemoteWopiSrc()` cover the denial case.
+
+---
+
+## Known Limitations
+
+- Path-prefixed installs (e.g. `https://cloud.example.com/owncloud`) are not specially handled — trailing-slash stripping only. Carried forward from previous PR.
+- No admin UI. Config is file-only (`config.php`).
+
+---
+
+## Files Changed
+
+| File | Change |
+|---|---|
+| `lib/FederationService.php` | Replace `TrustedServers` → `IConfig`; rewrite `isServerAllowed()` |
+| `lib/AppInfo/Application.php` | Simplify DI registration |
+| `tests/unit/FederationServiceTest.php` | Replace mock type; update/add test cases |
From cc28073dbde44bcaa78d3d2ade10e8a69c5f02ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 08:50:09 +0200
Subject: [PATCH 08/15] docs: add implementation plan for federation allowlist
via system config
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>
---
...-05-15-oc10-federation-allowlist-config.md | 521 ++++++++++++++++++
1 file changed, 521 insertions(+)
create mode 100644 docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
diff --git a/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md b/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
new file mode 100644
index 000000000..fc2841a6c
--- /dev/null
+++ b/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
@@ -0,0 +1,521 @@
+# Federation Allowlist via System Config Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Replace `OCA\Federation\TrustedServers` in `FederationService` with a `richdocuments.federation_allowlist` system config key as the sole authority for permitted federation servers.
+
+**Architecture:** Inject `IConfig` (ownCloud's core config interface) into `FederationService` in place of `TrustedServers`. `isServerAllowed()` reads the allowlist from `config.php` via `getSystemValue()`, applies trailing-slash normalisation and http/https scheme-swap matching per entry, and returns false by default when the key is absent or non-array. `Application.php` DI registration simplifies — no federation-app check needed.
+
+**Tech Stack:** PHP 8.3, ownCloud app framework, `OCP\IConfig`, PHPUnit 9/10.
+
+---
+
+## File Map
+
+| File | Action | Responsibility |
+|---|---|---|
+| `lib/FederationService.php` | Modify | Replace `TrustedServers` with `IConfig`; rewrite `isServerAllowed()` |
+| `lib/AppInfo/Application.php` | Modify | Simplify DI registration for `FederationService` |
+| `tests/unit/FederationServiceTest.php` | Modify | Replace mock type and all `isServerAllowed` test cases |
+
+---
+
+## Task 1: Replace tests for `isServerAllowed()` with IConfig-based versions
+
+**Files:**
+- Modify: `tests/unit/FederationServiceTest.php`
+
+The existing `isServerAllowed` tests use a `TrustedServers` mock. Replace them wholesale with `IConfig`-based tests. The `testSplitUserRemote` test and its data provider are unchanged except for updating the constructor call in `setUp()` to pass an `IConfig` mock instead of a `TrustedServers` mock.
+
+- [ ] **Step 1: Replace the entire file with the new test class**
+
+```php
+
+ *
+ * @copyright Copyright (c) 2023, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see
+ *
+ */
+namespace OCA\Richdocuments\Tests;
+
+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 {
+ /** @var ILogger|MockObject */
+ private $logger;
+
+ /** @var IClientService|MockObject */
+ private $httpClient;
+
+ /** @var IURLGenerator|MockObject */
+ private $urlGenerator;
+
+ /** @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->config = $this->createMock(IConfig::class);
+
+ $this->federationService = new FederationService(
+ $this->logger,
+ $this->urlGenerator,
+ $this->httpClient,
+ $this->config
+ );
+ }
+
+ // -------------------------------------------------------------------------
+ // Existing tests
+ // -------------------------------------------------------------------------
+
+ public function dataGenerateFederatedCloudID() {
+ $userPrefix = ['username', '1234'];
+ $remotes = ['localhost', 'local.host', 'dev.local.host', '127.0.0.1'];
+
+ $testCases = [];
+ foreach ($userPrefix as $user) {
+ foreach ($remotes as $remote) {
+ $testCases[] = [$user, $remote];
+ }
+ }
+ return $testCases;
+ }
+
+ /**
+ * @dataProvider dataGenerateFederatedCloudID
+ */
+ public function testSplitUserRemote($userId, $remote) {
+ $this->urlGenerator->method('getAbsoluteUrl')
+ ->with('/')
+ ->willReturn("https://{$remote}/");
+
+ $federatedCloudID = $this->federationService->generateFederatedCloudID($userId);
+
+ $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 testIsServerAllowedReturnsFalseWhenListIsEmpty(): 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(['https://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(['https://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(['https://trusted.example.com']);
+
+ $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
+ }
+
+ public function testIsServerAllowedSwapsHttpToHttps(): void {
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['https://trusted.example.com']);
+
+ $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
+ }
+
+ public function testIsServerAllowedSwapsHttpsToHttp(): void {
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['http://trusted.example.com']);
+
+ $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
+ }
+
+ public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['https://trusted.example.com']);
+
+ $this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
+ }
+}
+```
+
+- [ ] **Step 2: Verify the test count**
+
+```bash
+grep -c "public function test" tests/unit/FederationServiceTest.php
+```
+
+Expected output: `11`
+
+---
+
+## Task 2: Rewrite `FederationService` to use `IConfig`
+
+**Files:**
+- Modify: `lib/FederationService.php`
+
+Replace the `TrustedServers` dependency with `IConfig`. The new `isServerAllowed()` iterates the allowlist array, normalises each entry, and checks for exact or scheme-swapped match.
+
+- [ ] **Step 1: Replace the entire file**
+
+```php
+
+ * @author Szymon Kłos
+ *
+ * @copyright Copyright (c) 2023, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see
+ *
+ */
+namespace OCA\Richdocuments;
+
+use OCP\IConfig;
+use OCP\ILogger;
+use OCP\Http\Client\IClientService;
+use OCP\IURLGenerator;
+
+class FederationService {
+ /** @var ILogger */
+ private $logger;
+
+ /** @var IURLGenerator */
+ private $urlGenerator;
+
+ /** @var IClientService */
+ private $httpClient;
+
+ /** @var IConfig */
+ private $config;
+
+ public function __construct(
+ ILogger $logger,
+ IURLGenerator $urlGenerator,
+ IClientService $httpClient,
+ IConfig $config
+ ) {
+ $this->logger = $logger;
+ $this->urlGenerator = $urlGenerator;
+ $this->httpClient = $httpClient;
+ $this->config = $config;
+ }
+
+ /**
+ * Get the Url of the collabora document on a federated server.
+ *
+ * @param string $shareToken
+ * @param string $shareRelativePath
+ * @param string $server
+ * @param string $accessToken
+ * @return string with the Url to the given resource
+ */
+ public function getRemoteFileUrl(string $shareToken, string $shareRelativePath, string $server, string $accessToken) : string {
+ $serverHost = $this->urlGenerator->getAbsoluteURL('/');
+ $remoteFileUrl = \rtrim($server, '/') . '/index.php/apps/richdocuments/documents.php/federated' .
+ '?shareToken=' . $shareToken .
+ '&shareRelativePath=' . $shareRelativePath .
+ '&server=' . \rtrim($serverHost, '/') .
+ '&accessToken=' . $accessToken;
+ return $remoteFileUrl;
+ }
+
+ /**
+ * @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;
+
+ if (!$this->isServerAllowed($remote)) {
+ $this->logger->info("Server {server} is not allowed.", ["server" => $remote]);
+ return null;
+ }
+
+ try {
+ $client = $this->httpClient->newClient();
+ $url = $remote . '/ocs/v2.php/apps/richdocuments/api/v1/federation';
+
+ $response = $client->post(
+ $url,
+ [
+ 'form_params' => [
+ 'token' => $accessToken,
+ 'format' => 'json'
+ ],
+ 'timeout' => 3,
+ 'connect_timeout' => 3,
+ ]
+ );
+
+ $responseBody = $response->getBody();
+ $data = \json_decode($responseBody, true, 512);
+ if (\is_array($data)) {
+ return $data['ocs']['data'];
+ }
+ return null;
+ } catch (\Throwable $e) {
+ $this->logger->error('Cannot get the wopi info from remote server: ' . $remote, ['exception' => $e]);
+ }
+
+ return null;
+ }
+
+ /**
+ * Check if the given server URL is in the richdocuments.federation_allowlist system config.
+ *
+ * Returns false when the key is absent, empty, or not an array. Each entry is
+ * normalised (trailing slashes stripped) and checked against the incoming URL
+ * both as-is and with the http/https scheme swapped, to tolerate minor
+ * mismatches between how the admin stored the URL and how the request arrives.
+ *
+ * @param string $remote a remote url
+ * @return bool
+ */
+ public function isServerAllowed(string $remote): bool {
+ $allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []);
+
+ if (!\is_array($allowlist) || empty($allowlist)) {
+ return false;
+ }
+
+ $normalized = \rtrim($remote, '/');
+
+ if (\strpos($normalized, 'https://') === 0) {
+ $swapped = 'http://' . \substr($normalized, 8);
+ } elseif (\strpos($normalized, 'http://') === 0) {
+ $swapped = 'https://' . \substr($normalized, 7);
+ } else {
+ $swapped = null;
+ }
+
+ foreach ($allowlist as $entry) {
+ $e = \rtrim($entry, '/');
+ if ($normalized === $e) {
+ return true;
+ }
+ if ($swapped !== null && $swapped === $e) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ /**
+ * Get the wopiSrc Url from a remote server.
+ *
+ * @param string $server a remote
+ * @return string with the wopi src Url
+ */
+ public function getRemoteWopiSrc($server) {
+ if (!$this->isServerAllowed($server)) {
+ $this->logger->info("Server {server} is not allowed.", ["server" => $server]);
+ return '';
+ }
+
+ try {
+ $getWopiSrcUrl = $server . '/ocs/v2.php/apps/richdocuments/api/v1/federation?format=json';
+ $client = $this->httpClient->newClient();
+ $response = $client->get($getWopiSrcUrl, ['timeout' => 5]);
+ $data = \json_decode($response->getBody(), true);
+
+ if (\is_array($data)) {
+ return $data['ocs']['data']['wopi_url'];
+ }
+ } catch (\Throwable $e) {
+ $this->logger->error('Cannot get the wopiSrc of remote server: ' . $server, ['exception' => $e]);
+ }
+
+ return '';
+ }
+
+ /**
+ * Given local userId return federated cloud id
+ *
+ * @param string $userId user id
+ * @return string
+ */
+ public function generateFederatedCloudID(string $userId) : string {
+ $remote = \preg_replace('|^(.*?://)|', '', \rtrim($this->urlGenerator->getAbsoluteURL('/'), '/'));
+ return "{$userId}@{$remote}";
+ }
+}
+```
+
+- [ ] **Step 2: Commit**
+
+```bash
+git add lib/FederationService.php tests/unit/FederationServiceTest.php
+git commit -s -m "feat(security): replace TrustedServers with richdocuments.federation_allowlist system config
+
+isServerAllowed() now reads a PHP array from config.php via
+richdocuments.federation_allowlist. Absent/empty/non-array values deny all.
+Each entry is checked with trailing-slash stripping and http/https scheme
+swap to tolerate minor URL mismatches.
+
+Removes the optional federation-app dependency entirely."
+```
+
+---
+
+## Task 3: Simplify DI registration in `Application.php`
+
+**Files:**
+- Modify: `lib/AppInfo/Application.php`
+
+Remove the federation-app `isInstalled()` check and Phan suppression. Pass `$server->getConfig()` as the fourth argument.
+
+- [ ] **Step 1: Update `registerServices()` in `lib/AppInfo/Application.php`**
+
+Replace the entire `FederationService` registration block (lines 41–56):
+
+```php
+ $container->registerService(FederationService::class, function () use ($server) {
+ return new FederationService(
+ $server->getLogger(),
+ $server->getURLGenerator(),
+ $server->getHTTPClientService(),
+ $server->getConfig()
+ );
+ });
+```
+
+The block being replaced is:
+
+```php
+ /**
+ * 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')) {
+ /* @phan-suppress-next-line PhanUndeclaredClassReference */
+ $trustedServers = $server->query(\OCA\Federation\TrustedServers::class);
+ }
+ return new FederationService(
+ $server->getLogger(),
+ $server->getURLGenerator(),
+ $server->getHTTPClientService(),
+ $trustedServers
+ );
+ });
+```
+
+Also remove the `use OCA\Richdocuments\FederationService;` import only if it is no longer referenced elsewhere in the file — check first. It IS still used in `registerService(FederationService::class, ...)` so keep it.
+
+- [ ] **Step 2: Run the code style check locally**
+
+```bash
+make test-php-style
+```
+
+Expected: no output errors, exit 0.
+
+- [ ] **Step 3: Commit**
+
+```bash
+git add lib/AppInfo/Application.php
+git commit -s -m "fix: simplify FederationService DI — inject IConfig directly
+
+No longer needs the federation app isInstalled() check or Phan
+suppression. IConfig is a core ownCloud interface, always available."
+```
+
+---
+
+## Self-Review
+
+**Spec coverage:**
+- ✅ `richdocuments.federation_allowlist` PHP array in system config — Task 2
+- ✅ Absent key → deny all — Task 2 (`getSystemValue` default `[]`) + test in Task 1
+- ✅ Non-array value → deny all — Task 2 (`is_array` guard) + test in Task 1
+- ✅ Empty array → deny all — Task 2 + test in Task 1
+- ✅ Exact match — Task 2 + test in Task 1
+- ✅ Trailing slash stripping — Task 2 + tests in Task 1
+- ✅ http/https scheme swap — Task 2 + tests in Task 1
+- ✅ Untrusted server → false — Task 2 + test in Task 1
+- ✅ DI registration simplified in Application.php — Task 3
+- ✅ All Phan suppressions removed — Task 2 (FederationService.php) + Task 3 (Application.php)
+
+**Placeholder scan:** None found.
+
+**Type consistency:**
+- `IConfig $config` — constructor param in Task 2, mock in Task 1, DI in Task 3. Consistent.
+- `isServerAllowed(string $remote): bool` — signature unchanged, used in `getWopiForToken()` and `getRemoteWopiSrc()` in Task 2, tested in Task 1. Consistent.
+- `getSystemValue('richdocuments.federation_allowlist', [])` — called in Task 2, mocked with same signature in Task 1. Consistent.
From 57d77a9562a726807c75ef0d9476ec904bdeb719 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 08:55:14 +0200
Subject: [PATCH 09/15] docs: add pre-PR code style gate task to federation
allowlist plan
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>
---
...-05-15-oc10-federation-allowlist-config.md | 72 +++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md b/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
index fc2841a6c..10bd656e2 100644
--- a/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
+++ b/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
@@ -499,6 +499,77 @@ suppression. IConfig is a core ownCloud interface, always available."
---
+## Task 4: Pre-PR code style gate
+
+**Files:** none (verification only)
+
+All three make targets must pass before opening the PR. These run in CI, so a local pass is required first. Note: `test-php-phan` and `test-php-phpstan` require ownCloud core to be checked out at `../../` — if running in an isolated environment without core, skip those two and rely on CI to catch them (confirmed passing on previous PR). `test-php-style` can always be run locally.
+
+- [ ] **Step 1: Run PHP code style**
+
+```bash
+make test-php-style
+```
+
+Expected: exit 0, no files listed as needing fixes. If it fails, the error output will show a unified diff of what needs changing — apply it and re-run before continuing.
+
+- [ ] **Step 2: Run Phan (requires ownCloud core at `../../`)**
+
+```bash
+make test-php-phan 2>&1 | grep -E "^lib/|^appinfo/" | grep -v "^lib/Controller\|^lib/Db\|^lib/Document\|^lib/File\|^lib/Http\|^lib/Back\|^lib/Wopi" | head -20
+```
+
+Expected: no lines output (no new errors in `lib/FederationService.php` or `lib/AppInfo/Application.php`). If errors appear for our files specifically, fix them before continuing. Pre-existing errors in other files can be ignored.
+
+If core is not available locally, skip this step — CI will catch it.
+
+- [ ] **Step 3: Run PHPStan (requires ownCloud core at `../../`)**
+
+```bash
+make test-php-phpstan 2>&1 | tail -20
+```
+
+Expected: `[OK] No errors` or exit 0. If core is not available locally, skip — CI will catch it.
+
+- [ ] **Step 4: Open the PR in draft mode**
+
+```bash
+git push -u origin
+gh pr create --draft \
+ --title "feat(security): use richdocuments.federation_allowlist system config for federation server validation" \
+ --body "$(cat <<'EOF'
+## Summary
+
+- Replaces \`OCA\Federation\TrustedServers\` with a plain PHP array in \`config.php\` as the sole authority for permitted federation servers
+- Admins configure the allowlist via \`richdocuments.federation_allowlist\` system config key — no federation app dependency required
+- Absent, empty, or non-array values deny all federation requests (secure default)
+- URL matching applies trailing-slash stripping and http/https scheme swap, consistent with previous PR
+
+## Config
+
+\`\`\`php
+// config/config.php
+'richdocuments.federation_allowlist' => [
+ 'https://collab1.example.com',
+ 'https://collab2.example.com',
+],
+\`\`\`
+
+## Test plan
+
+- [ ] Unit tests cover: absent key, empty list, non-array value, exact match, trailing slash (single/multiple), http→https swap, https→http swap, untrusted server
+- [ ] \`make test-php-style\` passes
+- [ ] CI: PHP Code Style, Phan, PHPStan all green
+- [ ] Integration: add entry to \`config.php\`, verify trusted remote works; verify unlisted remote is denied with log entry
+- [ ] Integration: omit key entirely, verify all federation requests denied
+
+🤖 Generated with [Claude Code](https://claude.com/claude-code)
+EOF
+)"
+```
+
+---
+
## Self-Review
**Spec coverage:**
@@ -512,6 +583,7 @@ suppression. IConfig is a core ownCloud interface, always available."
- ✅ Untrusted server → false — Task 2 + test in Task 1
- ✅ DI registration simplified in Application.php — Task 3
- ✅ All Phan suppressions removed — Task 2 (FederationService.php) + Task 3 (Application.php)
+- ✅ Pre-PR code style gate (test-php-style, test-php-phan, test-php-phpstan) — Task 4
**Placeholder scan:** None found.
From 9baaca6e855473100304ec8beb0c0180f179f296 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 09:09:58 +0200
Subject: [PATCH 10/15] feat(security): replace TrustedServers with
richdocuments.federation_allowlist system config
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
isServerAllowed() now reads a PHP array from config.php via
richdocuments.federation_allowlist. Absent/empty/non-array values deny all.
Each entry is checked with trailing-slash stripping and http/https scheme
swap to tolerate minor URL mismatches.
Removes the optional federation-app dependency entirely.
Co-Authored-By: Claude Sonnet 4.6
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
lib/FederationService.php | 51 +++++++++---------
tests/unit/FederationServiceTest.php | 78 +++++++++++++++++-----------
2 files changed, 74 insertions(+), 55 deletions(-)
diff --git a/lib/FederationService.php b/lib/FederationService.php
index b2c94bb3c..103f8b044 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -21,7 +21,7 @@
*/
namespace OCA\Richdocuments;
-use OCA\Federation\TrustedServers;
+use OCP\IConfig;
use OCP\ILogger;
use OCP\Http\Client\IClientService;
use OCP\IURLGenerator;
@@ -36,21 +36,19 @@ class FederationService {
/** @var IClientService */
private $httpClient;
- /** @var TrustedServers|null */
- /* @phan-suppress-next-line PhanUndeclaredTypeProperty */
- private $trustedServers;
+ /** @var IConfig */
+ private $config;
public function __construct(
ILogger $logger,
IURLGenerator $urlGenerator,
IClientService $httpClient,
- /* @phan-suppress-next-line PhanUndeclaredTypeParameter */
- ?TrustedServers $trustedServers
+ IConfig $config
) {
- $this->logger = $logger;
- $this->urlGenerator = $urlGenerator;
- $this->httpClient = $httpClient;
- $this->trustedServers = $trustedServers;
+ $this->logger = $logger;
+ $this->urlGenerator = $urlGenerator;
+ $this->httpClient = $httpClient;
+ $this->config = $config;
}
/**
@@ -115,39 +113,44 @@ public function getWopiForToken($server, $accessToken) {
}
/**
- * Check if the given server URL is in ownCloud's trusted-servers list.
+ * Check if the given server URL is in the richdocuments.federation_allowlist system config.
*
- * 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
+ * Returns false when the key is absent, empty, or not an array. Each entry is
+ * normalised (trailing slashes stripped) and checked against the incoming URL
+ * both as-is and with the http/https scheme swapped, to tolerate minor
* mismatches between how the admin stored the URL and how the request arrives.
*
* @param string $remote a remote url
* @return bool
*/
public function isServerAllowed(string $remote): bool {
- if ($this->trustedServers === null) {
+ $allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []);
+
+ if (!\is_array($allowlist) || empty($allowlist)) {
return false;
}
$normalized = \rtrim($remote, '/');
- /* @phan-suppress-next-line PhanUndeclaredClassMethod */
- 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;
+ $swapped = null;
+ }
+
+ foreach ($allowlist as $entry) {
+ $e = \rtrim($entry, '/');
+ if ($normalized === $e) {
+ return true;
+ }
+ if ($swapped !== null && $swapped === $e) {
+ return true;
+ }
}
- /* @phan-suppress-next-line PhanUndeclaredClassMethod */
- return $this->trustedServers->isTrustedServer($swapped);
+ return false;
}
/**
diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index b0e536d3f..469b20aeb 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -21,9 +21,9 @@
*/
namespace OCA\Richdocuments\Tests;
-use OCA\Federation\TrustedServers;
use OCA\Richdocuments\FederationService;
use OCP\Http\Client\IClientService;
+use OCP\IConfig;
use OCP\ILogger;
use OCP\IURLGenerator;
use PHPUnit\Framework\MockObject\MockObject;
@@ -39,8 +39,8 @@ class FederationServiceTest extends TestCase {
/** @var IURLGenerator|MockObject */
private $urlGenerator;
- /** @var TrustedServers|MockObject */
- private $trustedServers;
+ /** @var IConfig|MockObject */
+ private $config;
/** @var FederationService */
private $federationService;
@@ -48,16 +48,16 @@ class FederationServiceTest extends TestCase {
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->trustedServers = $this->createMock(TrustedServers::class);
+ $this->urlGenerator = $this->createMock(IURLGenerator::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->trustedServers
+ $this->config
);
}
@@ -95,59 +95,75 @@ public function testSplitUserRemote($userId, $remote) {
// isServerAllowed() tests
// -------------------------------------------------------------------------
- public function testIsServerAllowedReturnsFalseWhenTrustedServersIsNull(): void {
- $service = new FederationService(
- $this->logger,
- $this->urlGenerator,
- $this->httpClient,
- null
- );
+ public function testIsServerAllowedReturnsFalseWhenKeyIsAbsent(): void {
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn([]);
- $this->assertFalse($service->isServerAllowed('https://remote.example.com'));
+ $this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com'));
+ }
+
+ public function testIsServerAllowedReturnsFalseWhenListIsEmpty(): 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->trustedServers->method('isTrustedServer')
- ->willReturnCallback(fn ($url) => $url === 'https://trusted.example.com');
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['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->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['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->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['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->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['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->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['http://trusted.example.com']);
$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
}
public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
- $this->trustedServers->method('isTrustedServer')
- ->willReturn(false);
+ $this->config->method('getSystemValue')
+ ->with('richdocuments.federation_allowlist', [])
+ ->willReturn(['https://trusted.example.com']);
$this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
}
-
}
From 45d01bd8ae845a2cabd82e387879934d0956ca28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 09:16:29 +0200
Subject: [PATCH 11/15] =?UTF-8?q?fix:=20simplify=20FederationService=20DI?=
=?UTF-8?q?=20=E2=80=94=20inject=20IConfig=20directly?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
No longer needs the federation app isInstalled() check or Phan
suppression. IConfig is a core ownCloud interface, always available.
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
lib/AppInfo/Application.php | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 738468d02..8191a497c 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -38,21 +38,12 @@ private function registerServices() {
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')) {
- /* @phan-suppress-next-line PhanUndeclaredClassReference */
- $trustedServers = $server->query(\OCA\Federation\TrustedServers::class);
- }
return new FederationService(
$server->getLogger(),
$server->getURLGenerator(),
$server->getHTTPClientService(),
- $trustedServers
+ $server->getConfig()
);
});
}
From dfc7d6a8bd9e86dbf7d86686712bff49519ff861 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 09:50:21 +0200
Subject: [PATCH 12/15] chore: ignore docs/superpowers planning artifacts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-Authored-By: Claude Sonnet 4.6
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
.gitignore | 3 +
...-05-15-oc10-federation-allowlist-config.md | 593 ------------------
...oc10-federation-allowlist-config-design.md | 115 ----
3 files changed, 3 insertions(+), 708 deletions(-)
delete mode 100644 docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
delete mode 100644 docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
diff --git a/.gitignore b/.gitignore
index 014e07349..108b313d8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,3 +23,6 @@ node_modules
.idea/
.pnpm-store/
+
+# Superpowers planning artifacts
+docs/superpowers/
diff --git a/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md b/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
deleted file mode 100644
index 10bd656e2..000000000
--- a/docs/superpowers/plans/2026-05-15-oc10-federation-allowlist-config.md
+++ /dev/null
@@ -1,593 +0,0 @@
-# Federation Allowlist via System Config Implementation Plan
-
-> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
-
-**Goal:** Replace `OCA\Federation\TrustedServers` in `FederationService` with a `richdocuments.federation_allowlist` system config key as the sole authority for permitted federation servers.
-
-**Architecture:** Inject `IConfig` (ownCloud's core config interface) into `FederationService` in place of `TrustedServers`. `isServerAllowed()` reads the allowlist from `config.php` via `getSystemValue()`, applies trailing-slash normalisation and http/https scheme-swap matching per entry, and returns false by default when the key is absent or non-array. `Application.php` DI registration simplifies — no federation-app check needed.
-
-**Tech Stack:** PHP 8.3, ownCloud app framework, `OCP\IConfig`, PHPUnit 9/10.
-
----
-
-## File Map
-
-| File | Action | Responsibility |
-|---|---|---|
-| `lib/FederationService.php` | Modify | Replace `TrustedServers` with `IConfig`; rewrite `isServerAllowed()` |
-| `lib/AppInfo/Application.php` | Modify | Simplify DI registration for `FederationService` |
-| `tests/unit/FederationServiceTest.php` | Modify | Replace mock type and all `isServerAllowed` test cases |
-
----
-
-## Task 1: Replace tests for `isServerAllowed()` with IConfig-based versions
-
-**Files:**
-- Modify: `tests/unit/FederationServiceTest.php`
-
-The existing `isServerAllowed` tests use a `TrustedServers` mock. Replace them wholesale with `IConfig`-based tests. The `testSplitUserRemote` test and its data provider are unchanged except for updating the constructor call in `setUp()` to pass an `IConfig` mock instead of a `TrustedServers` mock.
-
-- [ ] **Step 1: Replace the entire file with the new test class**
-
-```php
-
- *
- * @copyright Copyright (c) 2023, ownCloud GmbH
- * @license AGPL-3.0
- *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see
- *
- */
-namespace OCA\Richdocuments\Tests;
-
-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 {
- /** @var ILogger|MockObject */
- private $logger;
-
- /** @var IClientService|MockObject */
- private $httpClient;
-
- /** @var IURLGenerator|MockObject */
- private $urlGenerator;
-
- /** @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->config = $this->createMock(IConfig::class);
-
- $this->federationService = new FederationService(
- $this->logger,
- $this->urlGenerator,
- $this->httpClient,
- $this->config
- );
- }
-
- // -------------------------------------------------------------------------
- // Existing tests
- // -------------------------------------------------------------------------
-
- public function dataGenerateFederatedCloudID() {
- $userPrefix = ['username', '1234'];
- $remotes = ['localhost', 'local.host', 'dev.local.host', '127.0.0.1'];
-
- $testCases = [];
- foreach ($userPrefix as $user) {
- foreach ($remotes as $remote) {
- $testCases[] = [$user, $remote];
- }
- }
- return $testCases;
- }
-
- /**
- * @dataProvider dataGenerateFederatedCloudID
- */
- public function testSplitUserRemote($userId, $remote) {
- $this->urlGenerator->method('getAbsoluteUrl')
- ->with('/')
- ->willReturn("https://{$remote}/");
-
- $federatedCloudID = $this->federationService->generateFederatedCloudID($userId);
-
- $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 testIsServerAllowedReturnsFalseWhenListIsEmpty(): 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(['https://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(['https://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(['https://trusted.example.com']);
-
- $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
- }
-
- public function testIsServerAllowedSwapsHttpToHttps(): void {
- $this->config->method('getSystemValue')
- ->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
-
- $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
- }
-
- public function testIsServerAllowedSwapsHttpsToHttp(): void {
- $this->config->method('getSystemValue')
- ->with('richdocuments.federation_allowlist', [])
- ->willReturn(['http://trusted.example.com']);
-
- $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
- }
-
- public function testIsServerAllowedReturnsFalseForUntrustedServer(): void {
- $this->config->method('getSystemValue')
- ->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
-
- $this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
- }
-}
-```
-
-- [ ] **Step 2: Verify the test count**
-
-```bash
-grep -c "public function test" tests/unit/FederationServiceTest.php
-```
-
-Expected output: `11`
-
----
-
-## Task 2: Rewrite `FederationService` to use `IConfig`
-
-**Files:**
-- Modify: `lib/FederationService.php`
-
-Replace the `TrustedServers` dependency with `IConfig`. The new `isServerAllowed()` iterates the allowlist array, normalises each entry, and checks for exact or scheme-swapped match.
-
-- [ ] **Step 1: Replace the entire file**
-
-```php
-
- * @author Szymon Kłos
- *
- * @copyright Copyright (c) 2023, ownCloud GmbH
- * @license AGPL-3.0
- *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see
- *
- */
-namespace OCA\Richdocuments;
-
-use OCP\IConfig;
-use OCP\ILogger;
-use OCP\Http\Client\IClientService;
-use OCP\IURLGenerator;
-
-class FederationService {
- /** @var ILogger */
- private $logger;
-
- /** @var IURLGenerator */
- private $urlGenerator;
-
- /** @var IClientService */
- private $httpClient;
-
- /** @var IConfig */
- private $config;
-
- public function __construct(
- ILogger $logger,
- IURLGenerator $urlGenerator,
- IClientService $httpClient,
- IConfig $config
- ) {
- $this->logger = $logger;
- $this->urlGenerator = $urlGenerator;
- $this->httpClient = $httpClient;
- $this->config = $config;
- }
-
- /**
- * Get the Url of the collabora document on a federated server.
- *
- * @param string $shareToken
- * @param string $shareRelativePath
- * @param string $server
- * @param string $accessToken
- * @return string with the Url to the given resource
- */
- public function getRemoteFileUrl(string $shareToken, string $shareRelativePath, string $server, string $accessToken) : string {
- $serverHost = $this->urlGenerator->getAbsoluteURL('/');
- $remoteFileUrl = \rtrim($server, '/') . '/index.php/apps/richdocuments/documents.php/federated' .
- '?shareToken=' . $shareToken .
- '&shareRelativePath=' . $shareRelativePath .
- '&server=' . \rtrim($serverHost, '/') .
- '&accessToken=' . $accessToken;
- return $remoteFileUrl;
- }
-
- /**
- * @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;
-
- if (!$this->isServerAllowed($remote)) {
- $this->logger->info("Server {server} is not allowed.", ["server" => $remote]);
- return null;
- }
-
- try {
- $client = $this->httpClient->newClient();
- $url = $remote . '/ocs/v2.php/apps/richdocuments/api/v1/federation';
-
- $response = $client->post(
- $url,
- [
- 'form_params' => [
- 'token' => $accessToken,
- 'format' => 'json'
- ],
- 'timeout' => 3,
- 'connect_timeout' => 3,
- ]
- );
-
- $responseBody = $response->getBody();
- $data = \json_decode($responseBody, true, 512);
- if (\is_array($data)) {
- return $data['ocs']['data'];
- }
- return null;
- } catch (\Throwable $e) {
- $this->logger->error('Cannot get the wopi info from remote server: ' . $remote, ['exception' => $e]);
- }
-
- return null;
- }
-
- /**
- * Check if the given server URL is in the richdocuments.federation_allowlist system config.
- *
- * Returns false when the key is absent, empty, or not an array. Each entry is
- * normalised (trailing slashes stripped) and checked against the incoming URL
- * both as-is and with the http/https scheme swapped, to tolerate minor
- * mismatches between how the admin stored the URL and how the request arrives.
- *
- * @param string $remote a remote url
- * @return bool
- */
- public function isServerAllowed(string $remote): bool {
- $allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []);
-
- if (!\is_array($allowlist) || empty($allowlist)) {
- return false;
- }
-
- $normalized = \rtrim($remote, '/');
-
- if (\strpos($normalized, 'https://') === 0) {
- $swapped = 'http://' . \substr($normalized, 8);
- } elseif (\strpos($normalized, 'http://') === 0) {
- $swapped = 'https://' . \substr($normalized, 7);
- } else {
- $swapped = null;
- }
-
- foreach ($allowlist as $entry) {
- $e = \rtrim($entry, '/');
- if ($normalized === $e) {
- return true;
- }
- if ($swapped !== null && $swapped === $e) {
- return true;
- }
- }
-
- return false;
- }
-
- /**
- * Get the wopiSrc Url from a remote server.
- *
- * @param string $server a remote
- * @return string with the wopi src Url
- */
- public function getRemoteWopiSrc($server) {
- if (!$this->isServerAllowed($server)) {
- $this->logger->info("Server {server} is not allowed.", ["server" => $server]);
- return '';
- }
-
- try {
- $getWopiSrcUrl = $server . '/ocs/v2.php/apps/richdocuments/api/v1/federation?format=json';
- $client = $this->httpClient->newClient();
- $response = $client->get($getWopiSrcUrl, ['timeout' => 5]);
- $data = \json_decode($response->getBody(), true);
-
- if (\is_array($data)) {
- return $data['ocs']['data']['wopi_url'];
- }
- } catch (\Throwable $e) {
- $this->logger->error('Cannot get the wopiSrc of remote server: ' . $server, ['exception' => $e]);
- }
-
- return '';
- }
-
- /**
- * Given local userId return federated cloud id
- *
- * @param string $userId user id
- * @return string
- */
- public function generateFederatedCloudID(string $userId) : string {
- $remote = \preg_replace('|^(.*?://)|', '', \rtrim($this->urlGenerator->getAbsoluteURL('/'), '/'));
- return "{$userId}@{$remote}";
- }
-}
-```
-
-- [ ] **Step 2: Commit**
-
-```bash
-git add lib/FederationService.php tests/unit/FederationServiceTest.php
-git commit -s -m "feat(security): replace TrustedServers with richdocuments.federation_allowlist system config
-
-isServerAllowed() now reads a PHP array from config.php via
-richdocuments.federation_allowlist. Absent/empty/non-array values deny all.
-Each entry is checked with trailing-slash stripping and http/https scheme
-swap to tolerate minor URL mismatches.
-
-Removes the optional federation-app dependency entirely."
-```
-
----
-
-## Task 3: Simplify DI registration in `Application.php`
-
-**Files:**
-- Modify: `lib/AppInfo/Application.php`
-
-Remove the federation-app `isInstalled()` check and Phan suppression. Pass `$server->getConfig()` as the fourth argument.
-
-- [ ] **Step 1: Update `registerServices()` in `lib/AppInfo/Application.php`**
-
-Replace the entire `FederationService` registration block (lines 41–56):
-
-```php
- $container->registerService(FederationService::class, function () use ($server) {
- return new FederationService(
- $server->getLogger(),
- $server->getURLGenerator(),
- $server->getHTTPClientService(),
- $server->getConfig()
- );
- });
-```
-
-The block being replaced is:
-
-```php
- /**
- * 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')) {
- /* @phan-suppress-next-line PhanUndeclaredClassReference */
- $trustedServers = $server->query(\OCA\Federation\TrustedServers::class);
- }
- return new FederationService(
- $server->getLogger(),
- $server->getURLGenerator(),
- $server->getHTTPClientService(),
- $trustedServers
- );
- });
-```
-
-Also remove the `use OCA\Richdocuments\FederationService;` import only if it is no longer referenced elsewhere in the file — check first. It IS still used in `registerService(FederationService::class, ...)` so keep it.
-
-- [ ] **Step 2: Run the code style check locally**
-
-```bash
-make test-php-style
-```
-
-Expected: no output errors, exit 0.
-
-- [ ] **Step 3: Commit**
-
-```bash
-git add lib/AppInfo/Application.php
-git commit -s -m "fix: simplify FederationService DI — inject IConfig directly
-
-No longer needs the federation app isInstalled() check or Phan
-suppression. IConfig is a core ownCloud interface, always available."
-```
-
----
-
-## Task 4: Pre-PR code style gate
-
-**Files:** none (verification only)
-
-All three make targets must pass before opening the PR. These run in CI, so a local pass is required first. Note: `test-php-phan` and `test-php-phpstan` require ownCloud core to be checked out at `../../` — if running in an isolated environment without core, skip those two and rely on CI to catch them (confirmed passing on previous PR). `test-php-style` can always be run locally.
-
-- [ ] **Step 1: Run PHP code style**
-
-```bash
-make test-php-style
-```
-
-Expected: exit 0, no files listed as needing fixes. If it fails, the error output will show a unified diff of what needs changing — apply it and re-run before continuing.
-
-- [ ] **Step 2: Run Phan (requires ownCloud core at `../../`)**
-
-```bash
-make test-php-phan 2>&1 | grep -E "^lib/|^appinfo/" | grep -v "^lib/Controller\|^lib/Db\|^lib/Document\|^lib/File\|^lib/Http\|^lib/Back\|^lib/Wopi" | head -20
-```
-
-Expected: no lines output (no new errors in `lib/FederationService.php` or `lib/AppInfo/Application.php`). If errors appear for our files specifically, fix them before continuing. Pre-existing errors in other files can be ignored.
-
-If core is not available locally, skip this step — CI will catch it.
-
-- [ ] **Step 3: Run PHPStan (requires ownCloud core at `../../`)**
-
-```bash
-make test-php-phpstan 2>&1 | tail -20
-```
-
-Expected: `[OK] No errors` or exit 0. If core is not available locally, skip — CI will catch it.
-
-- [ ] **Step 4: Open the PR in draft mode**
-
-```bash
-git push -u origin
-gh pr create --draft \
- --title "feat(security): use richdocuments.federation_allowlist system config for federation server validation" \
- --body "$(cat <<'EOF'
-## Summary
-
-- Replaces \`OCA\Federation\TrustedServers\` with a plain PHP array in \`config.php\` as the sole authority for permitted federation servers
-- Admins configure the allowlist via \`richdocuments.federation_allowlist\` system config key — no federation app dependency required
-- Absent, empty, or non-array values deny all federation requests (secure default)
-- URL matching applies trailing-slash stripping and http/https scheme swap, consistent with previous PR
-
-## Config
-
-\`\`\`php
-// config/config.php
-'richdocuments.federation_allowlist' => [
- 'https://collab1.example.com',
- 'https://collab2.example.com',
-],
-\`\`\`
-
-## Test plan
-
-- [ ] Unit tests cover: absent key, empty list, non-array value, exact match, trailing slash (single/multiple), http→https swap, https→http swap, untrusted server
-- [ ] \`make test-php-style\` passes
-- [ ] CI: PHP Code Style, Phan, PHPStan all green
-- [ ] Integration: add entry to \`config.php\`, verify trusted remote works; verify unlisted remote is denied with log entry
-- [ ] Integration: omit key entirely, verify all federation requests denied
-
-🤖 Generated with [Claude Code](https://claude.com/claude-code)
-EOF
-)"
-```
-
----
-
-## Self-Review
-
-**Spec coverage:**
-- ✅ `richdocuments.federation_allowlist` PHP array in system config — Task 2
-- ✅ Absent key → deny all — Task 2 (`getSystemValue` default `[]`) + test in Task 1
-- ✅ Non-array value → deny all — Task 2 (`is_array` guard) + test in Task 1
-- ✅ Empty array → deny all — Task 2 + test in Task 1
-- ✅ Exact match — Task 2 + test in Task 1
-- ✅ Trailing slash stripping — Task 2 + tests in Task 1
-- ✅ http/https scheme swap — Task 2 + tests in Task 1
-- ✅ Untrusted server → false — Task 2 + test in Task 1
-- ✅ DI registration simplified in Application.php — Task 3
-- ✅ All Phan suppressions removed — Task 2 (FederationService.php) + Task 3 (Application.php)
-- ✅ Pre-PR code style gate (test-php-style, test-php-phan, test-php-phpstan) — Task 4
-
-**Placeholder scan:** None found.
-
-**Type consistency:**
-- `IConfig $config` — constructor param in Task 2, mock in Task 1, DI in Task 3. Consistent.
-- `isServerAllowed(string $remote): bool` — signature unchanged, used in `getWopiForToken()` and `getRemoteWopiSrc()` in Task 2, tested in Task 1. Consistent.
-- `getSystemValue('richdocuments.federation_allowlist', [])` — called in Task 2, mocked with same signature in Task 1. Consistent.
diff --git a/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md b/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
deleted file mode 100644
index cbb918494..000000000
--- a/docs/superpowers/specs/2026-05-15-oc10-federation-allowlist-config-design.md
+++ /dev/null
@@ -1,115 +0,0 @@
-# Federation Allowlist via System Config — Design
-
-**Date:** 2026-05-15
-**Scope:** Replace `OCA\Federation\TrustedServers` with a `config.php` system config allowlist as the sole authority for permitted federation servers in `FederationService`.
-
----
-
-## Problem
-
-The previous PR (OC10-100) fixed an SSRF by wiring `TrustedServers` from the federation app into `FederationService`. However, this creates a hard runtime dependency on the federation app being installed. Admins who don't use the federation app but do want to permit specific Collabora federation endpoints have no way to configure this.
-
-This PR replaces `TrustedServers` entirely with a flat system config key, giving admins a direct, federation-app-independent allowlist.
-
----
-
-## Config Key
-
-```php
-// config/config.php
-'richdocuments.federation_allowlist' => [
- 'https://collab1.example.com',
- 'https://collab2.example.com',
-],
-```
-
-- Type: `array` of URL strings
-- Location: `config/config.php` (system config), read via `IConfig::getSystemValue()`
-- Default: absent (treated as `[]`) → deny all
-- If set to a non-array value: treated as `[]` → deny all
-
----
-
-## Architecture
-
-### `FederationService`
-
-**Dependencies change:**
-- Remove: `?TrustedServers $trustedServers`
-- Add: `IConfig $config`
-- Remove: all four Phan suppression comments (no longer needed)
-- Remove: `use OCA\Federation\TrustedServers`
-- Add: `use OCP\IConfig`
-
-**`isServerAllowed(string $remote): bool`**
-
-1. `$allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', [])`
-2. If `!is_array($allowlist) || empty($allowlist)` → return `false`
-3. `$normalized = rtrim($remote, '/')`
-4. For each `$entry` in `$allowlist`:
- - `$e = rtrim($entry, '/')`
- - If `$normalized === $e` → return `true`
- - Compute scheme-swapped variant of `$normalized` (http↔https)
- - If scheme-swapped variant `=== $e` → return `true`
-5. Return `false`
-
-### `Application.php`
-
-DI registration simplifies — no `isInstalled('federation')` check:
-
-```php
-$container->registerService(FederationService::class, function () use ($server) {
- return new FederationService(
- $server->getLogger(),
- $server->getURLGenerator(),
- $server->getHTTPClientService(),
- $server->getConfig()
- );
-});
-```
-
-The docblock comment about null-as-secure-default is removed (no longer applicable).
-
----
-
-## Test Cases (`FederationServiceTest`)
-
-Replace `TrustedServers` mock with `IConfig` mock. All `isTrustedServer` setups replaced with `getSystemValue('richdocuments.federation_allowlist', [])` stubs.
-
-| Test | Input | Config value | Expected |
-|---|---|---|---|
-| `testIsServerAllowedReturnsFalseWhenKeyIsAbsent` | any URL | `[]` (default) | `false` |
-| `testIsServerAllowedReturnsFalseWhenListIsEmpty` | any URL | `[]` | `false` |
-| `testIsServerAllowedReturnsFalseForNonArrayConfig` | any URL | `'not-an-array'` | `false` |
-| `testIsServerAllowedReturnsTrueForExactMatch` | `https://trusted.example.com` | `['https://trusted.example.com']` | `true` |
-| `testIsServerAllowedStripsTrailingSlash` | `https://trusted.example.com/` | `['https://trusted.example.com']` | `true` |
-| `testIsServerAllowedStripsMultipleTrailingSlashes` | `https://trusted.example.com///` | `['https://trusted.example.com']` | `true` |
-| `testIsServerAllowedSwapsHttpToHttps` | `http://trusted.example.com` | `['https://trusted.example.com']` | `true` |
-| `testIsServerAllowedSwapsHttpsToHttp` | `https://trusted.example.com` | `['http://trusted.example.com']` | `true` |
-| `testIsServerAllowedReturnsFalseForUntrustedServer` | `https://evil.attacker.com` | `['https://trusted.example.com']` | `false` |
-
----
-
-## Error Handling
-
-- **Key absent:** `getSystemValue` returns `[]` default → deny all. No exception.
-- **Non-array value:** `is_array()` guard → deny all. No exception.
-- **Malformed entries:** Empty strings or non-URLs in the list never match → harmlessly ignored.
-- **Logging:** Unchanged. Existing `"Server {server} is not allowed."` log lines in `getWopiForToken()` and `getRemoteWopiSrc()` cover the denial case.
-
----
-
-## Known Limitations
-
-- Path-prefixed installs (e.g. `https://cloud.example.com/owncloud`) are not specially handled — trailing-slash stripping only. Carried forward from previous PR.
-- No admin UI. Config is file-only (`config.php`).
-
----
-
-## Files Changed
-
-| File | Change |
-|---|---|
-| `lib/FederationService.php` | Replace `TrustedServers` → `IConfig`; rewrite `isServerAllowed()` |
-| `lib/AppInfo/Application.php` | Simplify DI registration |
-| `tests/unit/FederationServiceTest.php` | Replace mock type; update/add test cases |
From 610323093a01e40a332d99b5bb379441795ced1f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 09:53:13 +0200
Subject: [PATCH 13/15] feat(security): use domain-only entries in
federation_allowlist config
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Allowlist entries are now plain domain names (e.g. 'cloud.example.com')
without a scheme. isServerAllowed() strips the http/https scheme from
the incoming URL before matching, so both http and https requests are
accepted for any listed domain.
Co-Authored-By: Claude Sonnet 4.6
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
lib/FederationService.php | 24 +++++-------------------
tests/unit/FederationServiceTest.php | 20 ++++++++++----------
2 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/lib/FederationService.php b/lib/FederationService.php
index 103f8b044..d52880e61 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -113,12 +113,10 @@ public function getWopiForToken($server, $accessToken) {
}
/**
- * Check if the given server URL is in the richdocuments.federation_allowlist system config.
+ * 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. Each entry is
- * normalised (trailing slashes stripped) and checked against the incoming URL
- * both as-is and with the http/https scheme swapped, to tolerate minor
- * mismatches between how the admin stored the URL and how the request arrives.
+ * Returns false when the key is absent, empty, or not an array.
*
* @param string $remote a remote url
* @return bool
@@ -130,22 +128,10 @@ public function isServerAllowed(string $remote): bool {
return false;
}
- $normalized = \rtrim($remote, '/');
-
- if (\strpos($normalized, 'https://') === 0) {
- $swapped = 'http://' . \substr($normalized, 8);
- } elseif (\strpos($normalized, 'http://') === 0) {
- $swapped = 'https://' . \substr($normalized, 7);
- } else {
- $swapped = null;
- }
+ $domain = \rtrim(\preg_replace('|^https?://|', '', $remote), '/');
foreach ($allowlist as $entry) {
- $e = \rtrim($entry, '/');
- if ($normalized === $e) {
- return true;
- }
- if ($swapped !== null && $swapped === $e) {
+ if ($domain === \rtrim((string)$entry, '/')) {
return true;
}
}
diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index 469b20aeb..7db87249d 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -122,7 +122,7 @@ public function testIsServerAllowedReturnsFalseForNonArrayConfig(): void {
public function testIsServerAllowedReturnsTrueForExactMatch(): void {
$this->config->method('getSystemValue')
->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
}
@@ -130,7 +130,7 @@ public function testIsServerAllowedReturnsTrueForExactMatch(): void {
public function testIsServerAllowedStripsTrailingSlash(): void {
$this->config->method('getSystemValue')
->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/'));
}
@@ -138,31 +138,31 @@ public function testIsServerAllowedStripsTrailingSlash(): void {
public function testIsServerAllowedStripsMultipleTrailingSlashes(): void {
$this->config->method('getSystemValue')
->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
$this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///'));
}
- public function testIsServerAllowedSwapsHttpToHttps(): void {
+ public function testIsServerAllowedMatchesHttps(): void {
$this->config->method('getSystemValue')
->with('richdocuments.federation_allowlist', [])
- ->willReturn(['https://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
- $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com'));
+ $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com'));
}
- public function testIsServerAllowedSwapsHttpsToHttp(): void {
+ public function testIsServerAllowedMatchesHttp(): void {
$this->config->method('getSystemValue')
->with('richdocuments.federation_allowlist', [])
- ->willReturn(['http://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
- $this->assertTrue($this->federationService->isServerAllowed('https://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(['https://trusted.example.com']);
+ ->willReturn(['trusted.example.com']);
$this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com'));
}
From 42d8f7440e6f946b2aa4e8b172255ff686523968 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 10:07:26 +0200
Subject: [PATCH 14/15] fix: add (string) cast on preg_replace, drop redundant
test, note path-prefix limitation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-Authored-By: Claude Sonnet 4.6
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
lib/FederationService.php | 5 ++++-
tests/unit/FederationServiceTest.php | 8 --------
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/lib/FederationService.php b/lib/FederationService.php
index d52880e61..82880da0f 100644
--- a/lib/FederationService.php
+++ b/lib/FederationService.php
@@ -118,6 +118,9 @@ public function getWopiForToken($server, $accessToken) {
*
* 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
*/
@@ -128,7 +131,7 @@ public function isServerAllowed(string $remote): bool {
return false;
}
- $domain = \rtrim(\preg_replace('|^https?://|', '', $remote), '/');
+ $domain = \rtrim((string)\preg_replace('|^https?://|', '', $remote), '/');
foreach ($allowlist as $entry) {
if ($domain === \rtrim((string)$entry, '/')) {
diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php
index 7db87249d..cb7a4ba98 100644
--- a/tests/unit/FederationServiceTest.php
+++ b/tests/unit/FederationServiceTest.php
@@ -103,14 +103,6 @@ public function testIsServerAllowedReturnsFalseWhenKeyIsAbsent(): void {
$this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com'));
}
- public function testIsServerAllowedReturnsFalseWhenListIsEmpty(): 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', [])
From fddcf61ddc6b0c15747aefd72ba598f86b4f7c75 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?=
<1005065+DeepDiver1975@users.noreply.github.com>
Date: Fri, 15 May 2026 12:55:49 +0200
Subject: [PATCH 15/15] refactor: remove explicit FederationService DI
registration
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
ownCloud's SimpleContainer autowires constructor dependencies by type
via reflection, so the manual registerService is redundant.
Co-Authored-By: Claude Sonnet 4.6
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
---
lib/AppInfo/Application.php | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
index 8191a497c..83a105e26 100644
--- a/lib/AppInfo/Application.php
+++ b/lib/AppInfo/Application.php
@@ -14,7 +14,6 @@
namespace OCA\Richdocuments\AppInfo;
use OCA\Richdocuments\AppConfig;
-use OCA\Richdocuments\FederationService;
use OCP\AppFramework\App;
use OC\AppFramework\Utility\SimpleContainer;
use OCP\Share;
@@ -24,28 +23,12 @@ 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'));
});
-
- $container->registerService(FederationService::class, function () use ($server) {
- return new FederationService(
- $server->getLogger(),
- $server->getURLGenerator(),
- $server->getHTTPClientService(),
- $server->getConfig()
- );
- });
}
public function registerScripts() {