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() {