From 0a72756d96cb1d85b6c0fd26189c341b75ec4f0d Mon Sep 17 00:00:00 2001 From: Camila Date: Fri, 2 Feb 2024 10:41:43 +0100 Subject: [PATCH 1/2] fix(dav): Update 403 error message * The user should get a more friendly warning when their desktop client version is not supported anymore by the server. See #nextcloud/desktop/issues/6273 * Update BlockLegacyClientPluginTest to reflect the new 403 error message. Signed-off-by: Camila Ayres --- .../Sabre/BlockLegacyClientPlugin.php | 3 ++- .../Sabre/BlockLegacyClientPluginTest.php | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php b/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php index c4e579bef0bd4..e59e3329cec2a 100644 --- a/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php +++ b/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php @@ -51,7 +51,8 @@ public function beforeHandler(RequestInterface $request) { preg_match(IRequest::USER_AGENT_CLIENT_DESKTOP, $userAgent, $versionMatches); if (isset($versionMatches[1]) && version_compare($versionMatches[1], $minimumSupportedDesktopVersion) === -1) { - throw new \Sabre\DAV\Exception\Forbidden('Unsupported client version.'); + $customClientDesktopLink = $this->config->getSystemValue('customclient_desktop', 'https://nextcloud.com/install/#install-clients'); + throw new \Sabre\DAV\Exception\Forbidden('This version of the client is unsupported. Upgrade to version '.$minimumSupportedDesktopVersion.' or later.'); } } } diff --git a/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php index 607ad71b11d3a..ff928d46a351a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php @@ -45,7 +45,20 @@ public function oldDesktopClientProvider(): array { */ public function testBeforeHandlerException(string $userAgent): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); - $this->expectExceptionMessage('Unsupported client version.'); + + $this->config + ->expects($this->once()) + ->method('getSystemValue') + ->with('customclient_desktop', 'https://nextcloud.com/install/#install-clients') + ->willReturn('https://nextcloud.com/install/#install-clients'); + + $this->config + ->expects($this->once()) + ->method('getSystemValue') + ->with('minimum.supported.desktop.version', '2.3.0') + ->willReturn('1.7.0'); + + $this->expectExceptionMessage('This version of the client is unsupported. Upgrade to version 1.7.0 or later.'); /** @var RequestInterface|MockObject $request */ $request = $this->createMock('\Sabre\HTTP\RequestInterface'); @@ -55,11 +68,6 @@ public function testBeforeHandlerException(string $userAgent): void { ->with('User-Agent') ->willReturn($userAgent); - $this->config - ->expects($this->once()) - ->method('getSystemValue') - ->with('minimum.supported.desktop.version', '2.3.0') - ->willReturn('1.7.0'); $this->blockLegacyClientVersionPlugin->beforeHandler($request); } From 5fc715a9e2d2284751b46a928ab402ec28c7ca08 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 6 Sep 2024 14:39:32 +0200 Subject: [PATCH 2/2] fix: Adjust unit tests and protect against XSS Signed-off-by: Ferdinand Thiessen --- .../InvitationResponseServer.php | 12 +++-- .../Sabre/BlockLegacyClientPlugin.php | 14 +++-- .../dav/lib/Connector/Sabre/ServerFactory.php | 6 ++- apps/dav/lib/Server.php | 6 ++- .../Sabre/BlockLegacyClientPluginTest.php | 54 +++++++++++++++---- 5 files changed, 72 insertions(+), 20 deletions(-) diff --git a/apps/dav/lib/CalDAV/InvitationResponse/InvitationResponseServer.php b/apps/dav/lib/CalDAV/InvitationResponse/InvitationResponseServer.php index e076b2cd1e234..6fa94eebc91ea 100644 --- a/apps/dav/lib/CalDAV/InvitationResponse/InvitationResponseServer.php +++ b/apps/dav/lib/CalDAV/InvitationResponse/InvitationResponseServer.php @@ -15,7 +15,9 @@ use OCA\DAV\Connector\Sabre\DavAclPlugin; use OCA\DAV\Events\SabrePluginAuthInitEvent; use OCA\DAV\RootCollection; +use OCA\Theming\ThemingDefaults; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IConfig; use Psr\Log\LoggerInterface; use Sabre\VObject\ITip\Message; @@ -28,9 +30,8 @@ class InvitationResponseServer { */ public function __construct(bool $public = true) { $baseUri = \OC::$WEBROOT . '/remote.php/dav/'; - $logger = \OC::$server->get(LoggerInterface::class); - /** @var IEventDispatcher $dispatcher */ - $dispatcher = \OC::$server->query(IEventDispatcher::class); + $logger = \OCP\Server::get(LoggerInterface::class); + $dispatcher = \OCP\Server::get(IEventDispatcher::class); $root = new RootCollection(); $this->server = new \OCA\DAV\Connector\Sabre\Server(new CachingTree($root)); @@ -42,7 +43,10 @@ public function __construct(bool $public = true) { $this->server->httpRequest->setUrl($baseUri); $this->server->setBaseUri($baseUri); - $this->server->addPlugin(new BlockLegacyClientPlugin(\OC::$server->getConfig())); + $this->server->addPlugin(new BlockLegacyClientPlugin( + \OCP\Server::get(IConfig::class), + \OCP\Server::get(ThemingDefaults::class), + )); $this->server->addPlugin(new AnonymousOptionsPlugin()); // allow custom principal uri option diff --git a/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php b/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php index e59e3329cec2a..cff60b9e0f888 100644 --- a/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php +++ b/apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php @@ -7,6 +7,7 @@ */ namespace OCA\DAV\Connector\Sabre; +use OCA\Theming\ThemingDefaults; use OCP\IConfig; use OCP\IRequest; use Sabre\DAV\Server; @@ -21,10 +22,11 @@ */ class BlockLegacyClientPlugin extends ServerPlugin { protected ?Server $server = null; - protected IConfig $config; - public function __construct(IConfig $config) { - $this->config = $config; + public function __construct( + private IConfig $config, + private ThemingDefaults $themingDefaults, + ) { } /** @@ -51,8 +53,10 @@ public function beforeHandler(RequestInterface $request) { preg_match(IRequest::USER_AGENT_CLIENT_DESKTOP, $userAgent, $versionMatches); if (isset($versionMatches[1]) && version_compare($versionMatches[1], $minimumSupportedDesktopVersion) === -1) { - $customClientDesktopLink = $this->config->getSystemValue('customclient_desktop', 'https://nextcloud.com/install/#install-clients'); - throw new \Sabre\DAV\Exception\Forbidden('This version of the client is unsupported. Upgrade to version '.$minimumSupportedDesktopVersion.' or later.'); + $customClientDesktopLink = htmlspecialchars($this->themingDefaults->getSyncClientUrl()); + $minimumSupportedDesktopVersion = htmlspecialchars($minimumSupportedDesktopVersion); + + throw new \Sabre\DAV\Exception\Forbidden("This version of the client is unsupported. Upgrade to version $minimumSupportedDesktopVersion or later."); } } } diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 2d804aad842fe..42d3ce1818ace 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -11,6 +11,7 @@ use OCA\DAV\CalDAV\DefaultCalendarValidator; use OCA\DAV\DAV\ViewOnlyPlugin; use OCA\DAV\Files\ErrorPagePlugin; +use OCA\Theming\ThemingDefaults; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IFilenameValidator; @@ -78,7 +79,10 @@ public function createServer(string $baseUri, // Load plugins $server->addPlugin(new \OCA\DAV\Connector\Sabre\MaintenancePlugin($this->config, $this->l10n)); - $server->addPlugin(new \OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin($this->config)); + $server->addPlugin(new BlockLegacyClientPlugin( + \OCP\Server::get(IConfig::class), + \OCP\Server::get(ThemingDefaults::class), + )); $server->addPlugin(new \OCA\DAV\Connector\Sabre\AnonymousOptionsPlugin()); $server->addPlugin($authPlugin); // FIXME: The following line is a workaround for legacy components relying on being able to send a GET to / diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 55c8a5afec1f2..2784e99b5f73b 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -50,6 +50,7 @@ use OCA\DAV\SystemTag\SystemTagPlugin; use OCA\DAV\Upload\ChunkingPlugin; use OCA\DAV\Upload\ChunkingV2Plugin; +use OCA\Theming\ThemingDefaults; use OCP\AppFramework\Http\Response; use OCP\Diagnostics\IEventLogger; use OCP\EventDispatcher\IEventDispatcher; @@ -110,7 +111,10 @@ public function __construct(IRequest $request, string $baseUri) { $this->server->setBaseUri($this->baseUri); $this->server->addPlugin(new ProfilerPlugin($this->request)); - $this->server->addPlugin(new BlockLegacyClientPlugin(\OC::$server->getConfig())); + $this->server->addPlugin(new BlockLegacyClientPlugin( + \OCP\Server::get(IConfig::class), + \OCP\Server::get(ThemingDefaults::class), + )); $this->server->addPlugin(new AnonymousOptionsPlugin()); $authPlugin = new Plugin(); $authPlugin->addBackend(new PublicAuth()); diff --git a/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php index ff928d46a351a..c44f52ec7136a 100644 --- a/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/BlockLegacyClientPluginTest.php @@ -10,6 +10,7 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OCA\DAV\Connector\Sabre\BlockLegacyClientPlugin; +use OCA\Theming\ThemingDefaults; use OCP\IConfig; use PHPUnit\Framework\MockObject\MockObject; use Sabre\HTTP\RequestInterface; @@ -21,19 +22,23 @@ * @package OCA\DAV\Tests\unit\Connector\Sabre */ class BlockLegacyClientPluginTest extends TestCase { - /** @var IConfig|MockObject */ - private $config; - /** @var BlockLegacyClientPlugin */ - private $blockLegacyClientVersionPlugin; + + private IConfig&MockObject $config; + private ThemingDefaults&MockObject $themingDefaults; + private BlockLegacyClientPlugin $blockLegacyClientVersionPlugin; protected function setUp(): void { parent::setUp(); $this->config = $this->createMock(IConfig::class); - $this->blockLegacyClientVersionPlugin = new BlockLegacyClientPlugin($this->config); + $this->themingDefaults = $this->createMock(ThemingDefaults::class); + $this->blockLegacyClientVersionPlugin = new BlockLegacyClientPlugin( + $this->config, + $this->themingDefaults, + ); } - public function oldDesktopClientProvider(): array { + public static function oldDesktopClientProvider(): array { return [ ['Mozilla/5.0 (Windows) mirall/1.5.0'], ['Mozilla/5.0 (Bogus Text) mirall/1.6.9'], @@ -46,10 +51,9 @@ public function oldDesktopClientProvider(): array { public function testBeforeHandlerException(string $userAgent): void { $this->expectException(\Sabre\DAV\Exception\Forbidden::class); - $this->config + $this->themingDefaults ->expects($this->once()) - ->method('getSystemValue') - ->with('customclient_desktop', 'https://nextcloud.com/install/#install-clients') + ->method('getSyncClientUrl') ->willReturn('https://nextcloud.com/install/#install-clients'); $this->config @@ -72,6 +76,38 @@ public function testBeforeHandlerException(string $userAgent): void { $this->blockLegacyClientVersionPlugin->beforeHandler($request); } + /** + * Ensure that there is no room for XSS attack through configured URL / version + * @dataProvider oldDesktopClientProvider + */ + public function testBeforeHandlerExceptionPreventXSSAttack(string $userAgent): void { + $this->expectException(\Sabre\DAV\Exception\Forbidden::class); + + $this->themingDefaults + ->expects($this->once()) + ->method('getSyncClientUrl') + ->willReturn('https://example.com">'); + + $this->config + ->expects($this->once()) + ->method('getSystemValue') + ->with('minimum.supported.desktop.version', '2.3.0') + ->willReturn('1.7.0 '); + + $this->expectExceptionMessage('This version of the client is unsupported. Upgrade to version 1.7.0 <script>alert("unsafe")</script> or later.'); + + /** @var RequestInterface|MockObject $request */ + $request = $this->createMock('\Sabre\HTTP\RequestInterface'); + $request + ->expects($this->once()) + ->method('getHeader') + ->with('User-Agent') + ->willReturn($userAgent); + + + $this->blockLegacyClientVersionPlugin->beforeHandler($request); + } + public function newAndAlternateDesktopClientProvider(): array { return [ ['Mozilla/5.0 (Windows) mirall/1.7.0'],