From 619b6da872f60a47d414bd8954b775f33af77caf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 19 Oct 2023 16:25:06 +0200 Subject: [PATCH] fix: Add proper feature policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- composer/composer/autoload_classmap.php | 3 +- composer/composer/autoload_static.php | 3 +- lib/AppConfig.php | 70 +++++++++- lib/AppInfo/Application.php | 7 +- .../AddContentSecurityPolicyListener.php | 69 ++++++++++ lib/Listener/AddFeaturePolicyListener.php | 64 +++++++++ lib/Listener/CSPListener.php | 130 ------------------ .../Listener/AddFeaturePolicyListenerTest.php | 84 +++++++++++ ... AddContentSecurityPolicyListenerTest.php} | 32 +++-- 9 files changed, 311 insertions(+), 151 deletions(-) create mode 100644 lib/Listener/AddContentSecurityPolicyListener.php create mode 100644 lib/Listener/AddFeaturePolicyListener.php delete mode 100644 lib/Listener/CSPListener.php create mode 100644 tests/OCA/Richdocuments/Listener/AddFeaturePolicyListenerTest.php rename tests/lib/Listener/{CSPListenerTest.php => AddContentSecurityPolicyListenerTest.php} (91%) diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index be62418aeb..2aa64bd592 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -37,8 +37,9 @@ 'OCA\\Richdocuments\\Exceptions\\ExpiredTokenException' => $baseDir . '/../lib/Exceptions/ExpiredTokenException.php', 'OCA\\Richdocuments\\Exceptions\\UnknownTokenException' => $baseDir . '/../lib/Exceptions/UnknownTokenException.php', 'OCA\\Richdocuments\\Helper' => $baseDir . '/../lib/Helper.php', + 'OCA\\Richdocuments\\Listener\\AddContentSecurityPolicyListener' => $baseDir . '/../lib/Listener/AddContentSecurityPolicyListener.php', + 'OCA\\Richdocuments\\Listener\\AddFeaturePolicyListener' => $baseDir . '/../lib/Listener/AddFeaturePolicyListener.php', 'OCA\\Richdocuments\\Listener\\BeforeFetchPreviewListener' => $baseDir . '/../lib/Listener/BeforeFetchPreviewListener.php', - 'OCA\\Richdocuments\\Listener\\CSPListener' => $baseDir . '/../lib/Listener/CSPListener.php', 'OCA\\Richdocuments\\Listener\\FileCreatedFromTemplateListener' => $baseDir . '/../lib/Listener/FileCreatedFromTemplateListener.php', 'OCA\\Richdocuments\\Listener\\LoadViewerListener' => $baseDir . '/../lib/Listener/LoadViewerListener.php', 'OCA\\Richdocuments\\Listener\\ReferenceListener' => $baseDir . '/../lib/Listener/ReferenceListener.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index 0ca83e59be..7a0f9877b8 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -52,8 +52,9 @@ class ComposerStaticInitRichdocuments 'OCA\\Richdocuments\\Exceptions\\ExpiredTokenException' => __DIR__ . '/..' . '/../lib/Exceptions/ExpiredTokenException.php', 'OCA\\Richdocuments\\Exceptions\\UnknownTokenException' => __DIR__ . '/..' . '/../lib/Exceptions/UnknownTokenException.php', 'OCA\\Richdocuments\\Helper' => __DIR__ . '/..' . '/../lib/Helper.php', + 'OCA\\Richdocuments\\Listener\\AddContentSecurityPolicyListener' => __DIR__ . '/..' . '/../lib/Listener/AddContentSecurityPolicyListener.php', + 'OCA\\Richdocuments\\Listener\\AddFeaturePolicyListener' => __DIR__ . '/..' . '/../lib/Listener/AddFeaturePolicyListener.php', 'OCA\\Richdocuments\\Listener\\BeforeFetchPreviewListener' => __DIR__ . '/..' . '/../lib/Listener/BeforeFetchPreviewListener.php', - 'OCA\\Richdocuments\\Listener\\CSPListener' => __DIR__ . '/..' . '/../lib/Listener/CSPListener.php', 'OCA\\Richdocuments\\Listener\\FileCreatedFromTemplateListener' => __DIR__ . '/..' . '/../lib/Listener/FileCreatedFromTemplateListener.php', 'OCA\\Richdocuments\\Listener\\LoadViewerListener' => __DIR__ . '/..' . '/../lib/Listener/LoadViewerListener.php', 'OCA\\Richdocuments\\Listener\\ReferenceListener' => __DIR__ . '/..' . '/../lib/Listener/ReferenceListener.php', diff --git a/lib/AppConfig.php b/lib/AppConfig.php index 6168ddf71a..6e98e10e06 100644 --- a/lib/AppConfig.php +++ b/lib/AppConfig.php @@ -13,6 +13,9 @@ use \OCP\IConfig; use OCA\Richdocuments\AppInfo\Application; +use OCA\Richdocuments\Service\FederationService; +use OCP\App\IAppManager; +use OCP\GlobalScale\IConfig as GlobalScaleConfig; class AppConfig { public const WOPI_URL = 'wopi_url'; @@ -28,7 +31,7 @@ class AppConfig { // Default: 'no', set to 'yes' to enable public const USE_SECURE_VIEW_ADDITIONAL_MIMES = 'use_secure_view_additional_mimes'; - private $defaults = [ + private array $defaults = [ 'wopi_url' => '', 'timeout' => 15, 'watermark_text' => '{userId}', @@ -46,15 +49,15 @@ class AppConfig { 'watermark_linkTagsList' => 'array' ]; - /** @var IConfig */ - private $config; - - public function __construct(IConfig $config) { - $this->config = $config; + public function __construct( + private IConfig $config, + private IAppManager $appManager, + private GlobalScaleConfig $globalScaleConfig, + ) { } public function getAppNamespace($key) { - if (strpos($key, 'watermark_') === 0) { + if (str_starts_with($key, 'watermark_')) { return self::WATERMARK_APP_NAMESPACE; } return Application::APPNAME; @@ -186,4 +189,57 @@ public function getWopiOverride(): array { public function useSecureViewAdditionalMimes(): bool { return $this->config->getAppValue(Application::APPNAME, self::USE_SECURE_VIEW_ADDITIONAL_MIMES, 'no') === 'yes'; } + + public function getDomainList(): array { + $urls = array_merge( + [ $this->domainOnly($this->getCollaboraUrlPublic()) ], + $this->getFederationDomains(), + $this->getGSDomains() + ); + + return array_filter($urls); + } + + private function getFederationDomains(): array { + if (!$this->appManager->isEnabledForUser('federation')) { + return []; + } + + $federationService = \OCP\Server::get(FederationService::class); + $trustedNextcloudDomains = array_filter(array_map(function ($server) use ($federationService) { + return $federationService->isTrustedRemote($server) ? $server : null; + }, $federationService->getTrustedServers())); + + $trustedCollaboraDomains = array_filter(array_map(function ($server) use ($federationService) { + try { + return $federationService->getRemoteCollaboraURL($server); + } catch (\Exception $e) { + // If there is no remote collabora server we can just skip that + return null; + } + }, $trustedNextcloudDomains)); + + return array_map(function ($url) { + return $this->domainOnly($url); + }, array_merge($trustedNextcloudDomains, $trustedCollaboraDomains)); + } + + private function getGSDomains(): array { + if (!$this->globalScaleConfig->isGlobalScaleEnabled()) { + return []; + } + + return $this->getGlobalScaleTrustedHosts(); + } + + /** + * Strips the path and query parameters from the URL. + */ + private function domainOnly(string $url): string { + $parsedUrl = parse_url($url); + $scheme = isset($parsedUrl['scheme']) ? $parsedUrl['scheme'] . '://' : ''; + $host = $parsedUrl['host'] ?? ''; + $port = isset($parsedUrl['port']) ? ':' . $parsedUrl['port'] : ''; + return "$scheme$host$port"; + } } diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 17ab2c9948..9e31e7a50e 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -28,8 +28,9 @@ use OCA\Richdocuments\AppConfig; use OCA\Richdocuments\Capabilities; use OCA\Richdocuments\Db\WopiMapper; +use OCA\Richdocuments\Listener\AddContentSecurityPolicyListener; +use OCA\Richdocuments\Listener\AddFeaturePolicyListener; use OCA\Richdocuments\Listener\BeforeFetchPreviewListener; -use OCA\Richdocuments\Listener\CSPListener; use OCA\Richdocuments\Listener\FileCreatedFromTemplateListener; use OCA\Richdocuments\Listener\LoadViewerListener; use OCA\Richdocuments\Listener\ReferenceListener; @@ -60,6 +61,7 @@ use OCP\IPreview; use OCP\Preview\BeforePreviewFetchedEvent; use OCP\Security\CSP\AddContentSecurityPolicyEvent; +use OCP\Security\FeaturePolicy\AddFeaturePolicyEvent; use OCP\Server; class Application extends App implements IBootstrap { @@ -75,7 +77,8 @@ public function register(IRegistrationContext $context): void { $context->registerCapability(Capabilities::class); $context->registerMiddleWare(WOPIMiddleware::class); $context->registerEventListener(FileCreatedFromTemplateEvent::class, FileCreatedFromTemplateListener::class); - $context->registerEventListener(AddContentSecurityPolicyEvent::class, CSPListener::class); + $context->registerEventListener(AddContentSecurityPolicyEvent::class, AddContentSecurityPolicyListener::class); + $context->registerEventListener(AddFeaturePolicyEvent::class, AddFeaturePolicyListener::class); $context->registerEventListener(LoadViewer::class, LoadViewerListener::class); $context->registerEventListener(ShareLinkAccessedEvent::class, ShareLinkListener::class); $context->registerEventListener(BeforePreviewFetchedEvent::class, BeforeFetchPreviewListener::class); diff --git a/lib/Listener/AddContentSecurityPolicyListener.php b/lib/Listener/AddContentSecurityPolicyListener.php new file mode 100644 index 0000000000..a0fbbacfc7 --- /dev/null +++ b/lib/Listener/AddContentSecurityPolicyListener.php @@ -0,0 +1,69 @@ + + * + * @author Julius Härtl + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ +namespace OCA\Richdocuments\Listener; + +use OCA\Richdocuments\AppConfig; +use OCP\AppFramework\Http\EmptyContentSecurityPolicy; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\IRequest; +use OCP\Security\CSP\AddContentSecurityPolicyEvent; + +/** @template-implements IEventListener */ +class AddContentSecurityPolicyListener implements IEventListener { + public function __construct( + private IRequest $request, + private AppConfig $config, + ) { + } + + public function handle(Event $event): void { + if (!$event instanceof AddContentSecurityPolicyEvent) { + return; + } + + if (!$this->isPageLoad()) { + return; + } + + $policy = new EmptyContentSecurityPolicy(); + $policy->addAllowedFrameDomain("'self'"); + $policy->addAllowedFrameDomain("nc:"); + + foreach ($this->config->getDomainList() as $url) { + $policy->addAllowedFrameDomain($url); + $policy->addAllowedFormActionDomain($url); + $policy->addAllowedFrameAncestorDomain($url); + $policy->addAllowedImageDomain($url); + } + + $event->addPolicy($policy); + } + + private function isPageLoad(): bool { + $scriptNameParts = explode('/', $this->request->getScriptName()); + return end($scriptNameParts) === 'index.php'; + } +} diff --git a/lib/Listener/AddFeaturePolicyListener.php b/lib/Listener/AddFeaturePolicyListener.php new file mode 100644 index 0000000000..4e5c702a11 --- /dev/null +++ b/lib/Listener/AddFeaturePolicyListener.php @@ -0,0 +1,64 @@ + + * + * @author Julius Härtl + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + * + */ +namespace OCA\Richdocuments\Listener; + +use OCA\Richdocuments\AppConfig; +use OCP\AppFramework\Http\FeaturePolicy; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\IRequest; +use OCP\Security\FeaturePolicy\AddFeaturePolicyEvent; + +/** @template-implements IEventListener */ +class AddFeaturePolicyListener implements IEventListener { + public function __construct( + private IRequest $request, + private AppConfig $config, + ) { + } + + public function handle(Event $event): void { + if (!$event instanceof AddFeaturePolicyEvent) { + return; + } + + if (!$this->isPageLoad()) { + return; + } + + $policy = new FeaturePolicy(); + + foreach ($this->config->getDomainList() as $url) { + $policy->addAllowedFullScreenDomain($url); + } + + $event->addPolicy($policy); + } + + private function isPageLoad(): bool { + $scriptNameParts = explode('/', $this->request->getScriptName()); + return end($scriptNameParts) === 'index.php'; + } +} diff --git a/lib/Listener/CSPListener.php b/lib/Listener/CSPListener.php deleted file mode 100644 index 04fa01a9ba..0000000000 --- a/lib/Listener/CSPListener.php +++ /dev/null @@ -1,130 +0,0 @@ - - * - * @author Julius Härtl - * - * @license GNU AGPL version 3 or any later version - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as - * published by the Free Software Foundation, either version 3 of the - * License, or (at your option) any later version. - * - * 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 - * along with this program. If not, see . - * - */ -namespace OCA\Richdocuments\Listener; - -use OCA\Richdocuments\AppConfig; -use OCA\Richdocuments\Service\FederationService; -use OCP\App\IAppManager; -use OCP\AppFramework\Http\EmptyContentSecurityPolicy; -use OCP\EventDispatcher\Event; -use OCP\EventDispatcher\IEventListener; -use OCP\GlobalScale\IConfig as GlobalScaleConfig; -use OCP\IRequest; -use OCP\Security\CSP\AddContentSecurityPolicyEvent; - -/** @template-implements IEventListener */ -class CSPListener implements IEventListener { - private IRequest $request; - private AppConfig $config; - private IAppManager $appManager; - private FederationService $federationService; - private GlobalScaleConfig $globalScaleConfig; - - public function __construct(IRequest $request, AppConfig $config, IAppManager $appManager, FederationService $federationService, GlobalScaleConfig $globalScaleConfig) { - $this->request = $request; - $this->config = $config; - $this->appManager = $appManager; - $this->federationService = $federationService; - $this->globalScaleConfig = $globalScaleConfig; - } - - public function handle(Event $event): void { - if (!$event instanceof AddContentSecurityPolicyEvent) { - return; - } - - if (!$this->isPageLoad()) { - return; - } - - $urls = array_merge( - [ $this->domainOnly($this->config->getCollaboraUrlPublic()) ], - $this->getFederationDomains(), - $this->getGSDomains() - ); - - $urls = array_filter($urls); - - $policy = new EmptyContentSecurityPolicy(); - $policy->addAllowedFrameDomain("'self'"); - $policy->addAllowedFrameDomain("nc:"); - - foreach ($urls as $url) { - $policy->addAllowedFrameDomain($url); - $policy->addAllowedFormActionDomain($url); - $policy->addAllowedFrameAncestorDomain($url); - $policy->addAllowedImageDomain($url); - } - - $event->addPolicy($policy); - } - - private function isPageLoad(): bool { - $scriptNameParts = explode('/', $this->request->getScriptName()); - return end($scriptNameParts) === 'index.php'; - } - - private function getFederationDomains(): array { - if (!$this->appManager->isEnabledForUser('federation')) { - return []; - } - - $trustedNextcloudDomains = array_filter(array_map(function ($server) { - return $this->federationService->isTrustedRemote($server) ? $server : null; - }, $this->federationService->getTrustedServers())); - - $trustedCollaboraDomains = array_filter(array_map(function ($server) { - try { - return $this->federationService->getRemoteCollaboraURL($server); - } catch (\Exception $e) { - // If there is no remote collabora server we can just skip that - return null; - } - }, $trustedNextcloudDomains)); - - return array_map(function ($url) { - return $this->domainOnly($url); - }, array_merge($trustedNextcloudDomains, $trustedCollaboraDomains)); - } - - private function getGSDomains(): array { - if (!$this->globalScaleConfig->isGlobalScaleEnabled()) { - return []; - } - - return $this->config->getGlobalScaleTrustedHosts(); - } - - /** - * Strips the path and query parameters from the URL. - */ - private function domainOnly(string $url): string { - $parsedUrl = parse_url($url); - $scheme = isset($parsedUrl['scheme']) ? $parsedUrl['scheme'] . '://' : ''; - $host = $parsedUrl['host'] ?? ''; - $port = isset($parsedUrl['port']) ? ':' . $parsedUrl['port'] : ''; - return "$scheme$host$port"; - } -} diff --git a/tests/OCA/Richdocuments/Listener/AddFeaturePolicyListenerTest.php b/tests/OCA/Richdocuments/Listener/AddFeaturePolicyListenerTest.php new file mode 100644 index 0000000000..edab68c331 --- /dev/null +++ b/tests/OCA/Richdocuments/Listener/AddFeaturePolicyListenerTest.php @@ -0,0 +1,84 @@ + + * + * @author Julius Härtl + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * 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 + * along with this program. If not, see . + */ + +namespace OCA\Richdocuments\Listener; + +use OC\Security\FeaturePolicy\FeaturePolicyManager; +use OCA\Richdocuments\AppConfig; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IRequest; +use PHPUnit\Framework\TestCase; + +class AddFeaturePolicyListenerTest extends TestCase { + private AddFeaturePolicyListener $featurePolicyListener; + + public function setUp(): void { + + $this->request = $this->createMock(IRequest::class); + $this->config = $this->createMock(AppConfig::class); + parent::setUp(); + + $this->featurePolicyListener = new AddFeaturePolicyListener( + $this->request, + $this->config, + ); + } + + public function testEmpty() { + $this->expectPageLoad(); + $this->config->expects(self::any()) + ->method('getDomainList') + ->willReturn([]); + + $policy = $this->getMergedPolicy(); + self::assertEquals(["'self'"], $policy->getFullscreenDomains()); + } + + public function testDomains() { + $this->expectPageLoad(); + $this->config->expects(self::any()) + ->method('getDomainList') + ->willReturn(['https://collabora.local']); + + $policy = $this->getMergedPolicy(); + self::assertEquals(["'self'", 'https://collabora.local'], $policy->getFullscreenDomains()); + } + + private function getMergedPolicy(): \OC\Security\FeaturePolicy\FeaturePolicy { + $eventDispatcher = $this->createMock(IEventDispatcher::class); + $eventDispatcher->expects(self::once()) + ->method('dispatchTyped') + ->willReturnCallback(function ($event) { + $this->featurePolicyListener->handle($event); + }); + $manager = new FeaturePolicyManager($eventDispatcher); + + return $manager->getDefaultPolicy(); + } + + private function expectPageLoad(): void { + $this->request->expects(self::once()) + ->method('getScriptName') + ->willReturn('index.php'); + } + +} diff --git a/tests/lib/Listener/CSPListenerTest.php b/tests/lib/Listener/AddContentSecurityPolicyListenerTest.php similarity index 91% rename from tests/lib/Listener/CSPListenerTest.php rename to tests/lib/Listener/AddContentSecurityPolicyListenerTest.php index f50c3791d0..fe97e01bfa 100644 --- a/tests/lib/Listener/CSPListenerTest.php +++ b/tests/lib/Listener/AddContentSecurityPolicyListenerTest.php @@ -26,19 +26,23 @@ use OC\Security\CSP\ContentSecurityPolicyManager; use OCA\Richdocuments\AppConfig; -use OCA\Richdocuments\Listener\CSPListener; +use OCA\Richdocuments\Listener\AddContentSecurityPolicyListener; use OCA\Richdocuments\Service\FederationService; use OCP\App\IAppManager; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\EmptyContentSecurityPolicy; use OCP\EventDispatcher\IEventDispatcher; use OCP\GlobalScale\IConfig as GlobalScaleConfig; +use OCP\IConfig; use OCP\IRequest; use OCP\Security\CSP\AddContentSecurityPolicyEvent; use PHPUnit\Framework\MockObject\MockObject; -use PHPUnit\Framework\TestCase; +use Test\TestCase; -class CSPListenerTest extends TestCase { +/** + * @group DB + */ +class AddContentSecurityPolicyListenerTest extends TestCase { /** @var IRequest|MockObject */ private $request; /** @var AppConfig|MockObject */ @@ -49,23 +53,31 @@ class CSPListenerTest extends TestCase { private $gsConfig; /** @var FederationService|MockObject */ private $federationService; - private CSPListener $listener; + private AddContentSecurityPolicyListener $listener; public function setUp(): void { parent::setUp(); - $this->request = $this->createMock(IRequest::class); - $this->config = $this->createMock(AppConfig::class); $this->appManager = $this->createMock(IAppManager::class); $this->gsConfig = $this->createMock(GlobalScaleConfig::class); $this->federationService = $this->createMock(FederationService::class); - $this->listener = new CSPListener( + $this->overwriteService(FederationService::class, $this->federationService); + + $this->request = $this->createMock(IRequest::class); + $this->config = $this->getMockBuilder(AppConfig::class) + ->setConstructorArgs([ + $this->createMock(IConfig::class), + $this->appManager, + $this->gsConfig, + ]) + ->onlyMethods(['getCollaboraUrlPublic', 'getGlobalScaleTrustedHosts']) + ->getMock(); + + + $this->listener = new AddContentSecurityPolicyListener( $this->request, $this->config, - $this->appManager, - $this->federationService, - $this->gsConfig ); }