Skip to content

Commit

Permalink
fix: Adjust unit tests and protect against XSS
Browse files Browse the repository at this point in the history
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
  • Loading branch information
susnux committed Sep 6, 2024
1 parent 0a72756 commit 5fc715a
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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));
Expand All @@ -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
Expand Down
14 changes: 9 additions & 5 deletions apps/dav/lib/Connector/Sabre/BlockLegacyClientPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
namespace OCA\DAV\Connector\Sabre;

use OCA\Theming\ThemingDefaults;
use OCP\IConfig;
use OCP\IRequest;
use Sabre\DAV\Server;
Expand All @@ -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,
) {
}

/**
Expand All @@ -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 <a href="'.$customClientDesktopLink.'">version '.$minimumSupportedDesktopVersion.' or later</a>.');
$customClientDesktopLink = htmlspecialchars($this->themingDefaults->getSyncClientUrl());
$minimumSupportedDesktopVersion = htmlspecialchars($minimumSupportedDesktopVersion);

throw new \Sabre\DAV\Exception\Forbidden("This version of the client is unsupported. Upgrade to <a href=\"$customClientDesktopLink\">version $minimumSupportedDesktopVersion or later</a>.");
}
}
}
6 changes: 5 additions & 1 deletion apps/dav/lib/Connector/Sabre/ServerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 /
Expand Down
6 changes: 5 additions & 1 deletion apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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'],
Expand All @@ -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
Expand All @@ -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"><script>alter("hacked");</script>');

$this->config
->expects($this->once())
->method('getSystemValue')
->with('minimum.supported.desktop.version', '2.3.0')
->willReturn('1.7.0 <script>alert("unsafe")</script>');

$this->expectExceptionMessage('This version of the client is unsupported. Upgrade to <a href="https://example.com&quot;&gt;&lt;script&gt;alter(&quot;hacked&quot;);&lt;/script&gt;">version 1.7.0 &lt;script&gt;alert(&quot;unsafe&quot;)&lt;/script&gt; or later</a>.');

/** @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'],
Expand Down

0 comments on commit 5fc715a

Please sign in to comment.