diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml new file mode 100644 index 000000000..e0d86600f --- /dev/null +++ b/.github/workflows/main.yml @@ -0,0 +1,66 @@ +name: CI + +on: + push: + branches: + - release-4.2.3 + pull_request: + types: + - opened + - synchronize + - reopened + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +env: + APP_NAME: richdocuments + PHP_VERSIONS: '["7.4"]' + +jobs: + get-vars: + runs-on: ubuntu-latest + outputs: + app-name: ${{ env.APP_NAME }} + php-versions: ${{ env.PHP_VERSIONS }} + steps: + - name: Set variables + run: | + echo "App name $APP_NAME" + echo "PHP versions string: $PHP_VERSIONS" + + semantic-git-messages: + name: Commits + uses: owncloud/reusable-workflows/.github/workflows/semantic-git-message.yml@main + + php-code-style: + name: PHP Code Style + needs: + - get-vars + uses: owncloud/reusable-workflows/.github/workflows/php-codestyle.yml@main + with: + app-name: ${{ needs.get-vars.outputs.app-name }} + php-versions: ${{ needs.get-vars.outputs.php-versions }} + core-ref-php74: '10.16' + + php-unit: + name: PHP Unit + needs: + - get-vars + uses: owncloud/reusable-workflows/.github/workflows/php-unit.yml@main + with: + app-name: ${{ needs.get-vars.outputs.app-name }} + php-versions: ${{ needs.get-vars.outputs.php-versions }} + core-ref-php74: '10.16' + + build: + name: Build + needs: + - php-code-style + - php-unit + uses: owncloud/reusable-workflows/.github/workflows/build.yml@main + diff --git a/CHANGELOG.md b/CHANGELOG.md index cd466549d..a8339df85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). +## [4.2.3] - 2026-05-18 + +### Security + +- [599](https://github.com/owncloud/richdocuments/pull/599) - Fix SSRF in federation endpoint: validate remote server against richdocuments.federation_allowlist system config + ## [4.2.2] - 2025-11-19 ### Fixed diff --git a/Makefile b/Makefile index 7a353ce6b..8184cfb3f 100644 --- a/Makefile +++ b/Makefile @@ -132,6 +132,13 @@ clean-deps: .PHONY: clean clean: clean-deps clean-dist clean-build +##------------- +## CI +##------------- + +.PHONY: ci +ci: vendor ## Install dependencies for CI + ##------------- ## Tests ##------------- diff --git a/appinfo/info.xml b/appinfo/info.xml index afc1a4bce..096282378 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -9,7 +9,7 @@ This application can connect to a Collabora Online (or other) server (WOPI-like You can also edit your documents off-line with the Collabora Office app from the **[Android](https://play.google.com/store/apps/details?id=com.collabora.libreoffice)** and **[iOS](https://apps.apple.com/us/app/collabora-office/id1440482071)** store.]]> Edit office documents directly in your browser. AGPL - 4.2.2 + 4.2.3 Collabora Productivity based on work of Frank Karlitschek, Victor Dubiniuk https://github.com/owncloud/richdocuments/issues https://github.com/owncloud/richdocuments.git diff --git a/lib/FederationService.php b/lib/FederationService.php index 3f042f0bf..d37cedf2f 100644 --- a/lib/FederationService.php +++ b/lib/FederationService.php @@ -21,34 +21,34 @@ */ namespace OCA\Richdocuments; +use OCP\IConfig; use OCP\ILogger; use OCP\Http\Client\IClientService; use OCP\IURLGenerator; class FederationService { - /** - * @var ILogger - */ + /** @var ILogger */ private $logger; - /** - * @var IURLGenerator - */ + /** @var IURLGenerator */ private $urlGenerator; - /** - * @var IClientService - */ + /** @var IClientService */ private $httpClient; + /** @var IConfig */ + private $config; + public function __construct( ILogger $logger, IURLGenerator $urlGenerator, - IClientService $httpClient + IClientService $httpClient, + IConfig $config ) { - $this->logger = $logger; + $this->logger = $logger; $this->urlGenerator = $urlGenerator; - $this->httpClient = $httpClient; + $this->httpClient = $httpClient; + $this->config = $config; } /** @@ -69,13 +69,12 @@ public function getRemoteFileUrl(string $shareToken, string $shareRelativePath, '&accessToken=' . $accessToken; return $remoteFileUrl; } - + /** - * - * @param string $server addres of a remote server - * @param string $accessToken wopi access token from a remote server - * @return array|null with additional wopi information - */ + * @param string $server address of a remote server + * @param string $accessToken wopi access token from a remote server + * @return array|null with additional wopi information + */ public function getWopiForToken($server, $accessToken) { $remote = $server; @@ -114,15 +113,34 @@ public function getWopiForToken($server, $accessToken) { } /** - * Check if server is allowed + * Check if the given server URL matches an entry in the richdocuments.federation_allowlist + * system config. Allowlist entries are plain domain names (no scheme). + * + * Returns false when the key is absent, empty, or not an array. + * + * Known limitation: path-prefixed installs (e.g. cloud.example.com/owncloud) are not + * supported — only the host component is extracted and matched against allowlist entries. * * @param string $remote a remote url - * @return bool indicating if given remote is allowed server + * @return bool */ - public function isServerAllowed($remote) { - // TODO: implement check for trusted server, for a moment all trusted + public function isServerAllowed(string $remote): bool { + $allowlist = $this->config->getSystemValue('richdocuments.federation_allowlist', []); + + if (!\is_array($allowlist) || empty($allowlist)) { + return false; + } + + $parsed = \parse_url($remote); + $domain = isset($parsed['host']) ? \rtrim((string)$parsed['host'], '/') : ''; + + foreach ($allowlist as $entry) { + if ($domain === \rtrim((string)$entry, '/')) { + return true; + } + } - return true; + return false; } /** diff --git a/tests/unit/FederationServiceTest.php b/tests/unit/FederationServiceTest.php index 7efb59459..cb7a4ba98 100644 --- a/tests/unit/FederationServiceTest.php +++ b/tests/unit/FederationServiceTest.php @@ -23,63 +23,51 @@ use OCA\Richdocuments\FederationService; use OCP\Http\Client\IClientService; +use OCP\IConfig; use OCP\ILogger; use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class FederationServiceTest extends TestCase { - /** - * The ILogger instance. - * - * @var ILogger - */ + /** @var ILogger|MockObject */ private $logger; - /** - * The IClientService instance. - * - * @var IClientService - */ + /** @var IClientService|MockObject */ private $httpClient; - /** - * The IURLGenerator instance. - * - * @var IURLGenerator - */ + /** @var IURLGenerator|MockObject */ private $urlGenerator; - /** - * @var FederationService|MockObject The discovery service mock object. - */ + /** @var IConfig|MockObject */ + private $config; + + /** @var FederationService */ private $federationService; protected function setUp(): void { parent::setUp(); $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->logger = $this->createMock(ILogger::class); - $this->httpClient = $this->createMock(IClientService::class); + $this->logger = $this->createMock(ILogger::class); + $this->httpClient = $this->createMock(IClientService::class); + $this->config = $this->createMock(IConfig::class); $this->federationService = new FederationService( $this->logger, $this->urlGenerator, - $this->httpClient + $this->httpClient, + $this->config ); } + // ------------------------------------------------------------------------- + // Existing tests + // ------------------------------------------------------------------------- + public function dataGenerateFederatedCloudID() { - $userPrefix = [ - 'username', - '1234' - ]; - $remotes = [ - 'localhost', - 'local.host', - 'dev.local.host', - '127.0.0.1', - ]; + $userPrefix = ['username', '1234']; + $remotes = ['localhost', 'local.host', 'dev.local.host', '127.0.0.1']; $testCases = []; foreach ($userPrefix as $user) { @@ -92,9 +80,6 @@ public function dataGenerateFederatedCloudID() { /** * @dataProvider dataGenerateFederatedCloudID - * - * @param string $userId - * @param string $expectedFederatedCloudID */ public function testSplitUserRemote($userId, $remote) { $this->urlGenerator->method('getAbsoluteUrl') @@ -105,4 +90,72 @@ public function testSplitUserRemote($userId, $remote) { $this->assertSame("{$userId}@{$remote}", $federatedCloudID); } + + // ------------------------------------------------------------------------- + // isServerAllowed() tests + // ------------------------------------------------------------------------- + + public function testIsServerAllowedReturnsFalseWhenKeyIsAbsent(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn([]); + + $this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com')); + } + + public function testIsServerAllowedReturnsFalseForNonArrayConfig(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn('not-an-array'); + + $this->assertFalse($this->federationService->isServerAllowed('https://remote.example.com')); + } + + public function testIsServerAllowedReturnsTrueForExactMatch(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedStripsTrailingSlash(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com/')); + } + + public function testIsServerAllowedStripsMultipleTrailingSlashes(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com///')); + } + + public function testIsServerAllowedMatchesHttps(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('https://trusted.example.com')); + } + + public function testIsServerAllowedMatchesHttp(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertTrue($this->federationService->isServerAllowed('http://trusted.example.com')); + } + + public function testIsServerAllowedReturnsFalseForUntrustedServer(): void { + $this->config->method('getSystemValue') + ->with('richdocuments.federation_allowlist', []) + ->willReturn(['trusted.example.com']); + + $this->assertFalse($this->federationService->isServerAllowed('https://evil.attacker.com')); + } }