Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
@@ -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

6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
##-------------
Expand Down
2 changes: 1 addition & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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.]]></description>
<summary>Edit office documents directly in your browser.</summary>
<licence>AGPL</licence>
<version>4.2.2</version>
<version>4.2.3</version>
<author>Collabora Productivity based on work of Frank Karlitschek, Victor Dubiniuk</author>
<bugs>https://github.com/owncloud/richdocuments/issues</bugs>
<repository type="git">https://github.com/owncloud/richdocuments.git</repository>
Expand Down
64 changes: 41 additions & 23 deletions lib/FederationService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -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;

Expand Down Expand Up @@ -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;
}

/**
Expand Down
121 changes: 87 additions & 34 deletions tests/unit/FederationServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -92,9 +80,6 @@ public function dataGenerateFederatedCloudID() {

/**
* @dataProvider dataGenerateFederatedCloudID
*
* @param string $userId
* @param string $expectedFederatedCloudID
*/
public function testSplitUserRemote($userId, $remote) {
$this->urlGenerator->method('getAbsoluteUrl')
Expand All @@ -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'));
}
}
Loading