Merge pull request #59778 from nextcloud/bug/noid/avoid-undefined-array-key-sharing-request

Avoid undefined array key sharing request

apps/dav/composer/composer/autoload_classmap.php

@@ -268,6 +268,7 @@ return array(
     'OCA\\DAV\\DAV\\GroupPrincipalBackend' => $baseDir . '/../lib/DAV/GroupPrincipalBackend.php',
     'OCA\\DAV\\DAV\\PublicAuth' => $baseDir . '/../lib/DAV/PublicAuth.php',
     'OCA\\DAV\\DAV\\RemoteUserPrincipalBackend' => $baseDir . '/../lib/DAV/RemoteUserPrincipalBackend.php',
+    'OCA\\DAV\\DAV\\Security\\RateLimiting' => $baseDir . '/../lib/DAV/Security/RateLimiting.php',
     'OCA\\DAV\\DAV\\Sharing\\Backend' => $baseDir . '/../lib/DAV/Sharing/Backend.php',
     'OCA\\DAV\\DAV\\Sharing\\IShareable' => $baseDir . '/../lib/DAV/Sharing/IShareable.php',
     'OCA\\DAV\\DAV\\Sharing\\Plugin' => $baseDir . '/../lib/DAV/Sharing/Plugin.php',

apps/dav/composer/composer/autoload_static.php

@@ -283,6 +283,7 @@ class ComposerStaticInitDAV
         'OCA\\DAV\\DAV\\GroupPrincipalBackend' => __DIR__ . '/..' . '/../lib/DAV/GroupPrincipalBackend.php',
         'OCA\\DAV\\DAV\\PublicAuth' => __DIR__ . '/..' . '/../lib/DAV/PublicAuth.php',
         'OCA\\DAV\\DAV\\RemoteUserPrincipalBackend' => __DIR__ . '/..' . '/../lib/DAV/RemoteUserPrincipalBackend.php',
+        'OCA\\DAV\\DAV\\Security\\RateLimiting' => __DIR__ . '/..' . '/../lib/DAV/Security/RateLimiting.php',
         'OCA\\DAV\\DAV\\Sharing\\Backend' => __DIR__ . '/..' . '/../lib/DAV/Sharing/Backend.php',
         'OCA\\DAV\\DAV\\Sharing\\IShareable' => __DIR__ . '/..' . '/../lib/DAV/Sharing/IShareable.php',
         'OCA\\DAV\\DAV\\Sharing\\Plugin' => __DIR__ . '/..' . '/../lib/DAV/Sharing/Plugin.php',

apps/dav/lib/DAV/Security/RateLimiting.php

@@ -0,0 +1,46 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\DAV\Security;
+
+use OCA\DAV\Connector\Sabre\Exception\TooManyRequests;
+use OCP\IAppConfig;
+use OCP\IUserSession;
+use OCP\Security\RateLimiting\ILimiter;
+use OCP\Security\RateLimiting\IRateLimitExceededException;
+
+class RateLimiting {
+
+	public function __construct(
+		private readonly IUserSession $userSession,
+		private readonly IAppConfig $config,
+		private readonly ILimiter $limiter,
+	) {
+	}
+
+	/**
+	 * @throws TooManyRequests
+	 */
+	public function check(): void {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return;
+		}
+
+		$identifier = 'share-addressbook-or-calendar';
+		$userLimit = $this->config->getValueInt('dav', 'rateLimitShareAddressbookOrCalendar', 20);
+		$userPeriod = $this->config->getValueInt('dav', 'rateLimitPeriodShareAddressbookOrCalendar', 3600);
+
+		try {
+			$this->limiter->registerUserRequest($identifier, $userLimit, $userPeriod, $user);
+		} catch (IRateLimitExceededException $e) {
+			throw new TooManyRequests('Too many addressbook or calendar share requests', 0, $e);
+		}
+	}
+}

apps/dav/lib/DAV/Sharing/Plugin.php

@@ -10,11 +10,13 @@ namespace OCA\DAV\DAV\Sharing;
 use OCA\DAV\CalDAV\CalDavBackend;
 use OCA\DAV\CalDAV\CalendarHome;
 use OCA\DAV\Connector\Sabre\Auth;
+use OCA\DAV\DAV\Security\RateLimiting;
 use OCA\DAV\DAV\Sharing\Xml\Invite;
 use OCA\DAV\DAV\Sharing\Xml\ShareRequest;
 use OCP\AppFramework\Http;
 use OCP\IConfig;
 use OCP\IRequest;
+use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\ICollection;
 use Sabre\DAV\INode;
@@ -28,17 +30,11 @@ class Plugin extends ServerPlugin {
 	public const NS_OWNCLOUD = 'http://owncloud.org/ns';
 	public const NS_NEXTCLOUD = 'http://nextcloud.com/ns';
 
-	/**
-	 * Plugin constructor.
-	 *
-	 * @param Auth $auth
-	 * @param IRequest $request
-	 * @param IConfig $config
-	 */
 	public function __construct(
 		private Auth $auth,
 		private IRequest $request,
 		private IConfig $config,
+		private RateLimiting $rateLimiting,
 	) {
 	}
 
@@ -136,6 +132,9 @@ class Plugin extends ServerPlugin {
 			// calendar.
 			case '{' . self::NS_OWNCLOUD . '}share':
 
+				$this->rateLimiting->check();
+				$this->validateShareRequest($message);
+
 				// We can only deal with IShareableCalendar objects
 				if (!$node instanceof IShareable) {
 					return;
@@ -170,6 +169,23 @@ class Plugin extends ServerPlugin {
 		}
 	}
 
+	private function validateShareRequest($shareRequest): void {
+		if (!$shareRequest instanceof ShareRequest) {
+			// @FIXME: Replace switch-case in httpPost with instanceof ShareRequest
+			throw new BadRequest('The given request is not valid');
+		}
+
+		$elements = (count($shareRequest->set) + count($shareRequest->remove));
+
+		if ($elements === 0) {
+			throw new BadRequest(ShareRequest::ELEMENT_SHARE . ' needs at least one set or remove element');
+		}
+
+		if ($elements > 10) {
+			throw new BadRequest(ShareRequest::ELEMENT_SHARE . ' is limited to 10 set or remove elements');
+		}
+	}
+
 	private function preloadCollection(PropFind $propFind, ICollection $collection): void {
 		if (!$collection instanceof CalendarHome || $propFind->getDepth() !== 1) {
 			return;

apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php

@@ -12,6 +12,8 @@ use Sabre\Xml\Reader;
 use Sabre\Xml\XmlDeserializable;
 
 class ShareRequest implements XmlDeserializable {
+	public const ELEMENT_SHARE = '{' . Plugin::NS_OWNCLOUD . '}share';
+
 	/**
 	 * Constructor
 	 *
@@ -33,6 +35,10 @@ class ShareRequest implements XmlDeserializable {
 		$set = [];
 		$remove = [];
 
+		if ($elements === null) {
+			return new self($set, $remove);
+		}
+
 		foreach ($elements as $elem) {
 			switch ($elem['name']) {
 

apps/dav/lib/Server.php

@@ -56,6 +56,7 @@ use OCA\DAV\Connector\Sabre\UserIdHeaderPlugin;
 use OCA\DAV\Connector\Sabre\ZipFolderPlugin;
 use OCA\DAV\DAV\CustomPropertiesBackend;
 use OCA\DAV\DAV\PublicAuth;
+use OCA\DAV\DAV\Security\RateLimiting;
 use OCA\DAV\DAV\ViewOnlyPlugin;
 use OCA\DAV\Db\PropertyMapper;
 use OCA\DAV\Events\SabrePluginAddEvent;
@@ -200,7 +201,7 @@ class Server {
 
 		// calendar plugins
 		if ($this->requestIsForSubtree(['calendars', 'public-calendars', 'system-calendars', 'principals'])) {
-			$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class)));
+			$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class), \OCP\Server::get(RateLimiting::class)));
 			$this->server->addPlugin(new \OCA\DAV\CalDAV\Plugin());
 			$this->server->addPlugin(new ICSExportPlugin(\OCP\Server::get(IConfig::class), $logger));
 			$this->server->addPlugin(new \OCA\DAV\CalDAV\Schedule\Plugin(\OCP\Server::get(IConfig::class), \OCP\Server::get(LoggerInterface::class), \OCP\Server::get(DefaultCalendarValidator::class)));
@@ -223,7 +224,7 @@ class Server {
 
 		// addressbook plugins
 		if ($this->requestIsForSubtree(['addressbooks', 'principals'])) {
-			$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class)));
+			$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class), \OCP\Server::get(RateLimiting::class)));
 			$this->server->addPlugin(new \OCA\DAV\CardDAV\Plugin());
 			$this->server->addPlugin(new VCFExportPlugin());
 			$this->server->addPlugin(new MultiGetExportPlugin());

apps/dav/tests/unit/CardDAV/Sharing/PluginTest.php

@@ -1,62 +0,0 @@
-<?php
-
-declare(strict_types=1);
-/**
- * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
- * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
- * SPDX-License-Identifier: AGPL-3.0-only
- */
-namespace OCA\DAV\Tests\unit\CardDAV\Sharing;
-
-use OCA\DAV\Connector\Sabre\Auth;
-use OCA\DAV\DAV\Sharing\IShareable;
-use OCA\DAV\DAV\Sharing\Plugin;
-use OCP\IConfig;
-use OCP\IRequest;
-use PHPUnit\Framework\MockObject\MockObject;
-use Sabre\DAV\Server;
-use Sabre\DAV\SimpleCollection;
-use Sabre\HTTP\Request;
-use Sabre\HTTP\Response;
-use Test\TestCase;
-
-class PluginTest extends TestCase {
-	private Plugin $plugin;
-	private Server $server;
-	private IShareable&MockObject $book;
-
-	protected function setUp(): void {
-		parent::setUp();
-
-		$authBackend = $this->createMock(Auth::class);
-		$authBackend->method('isDavAuthenticated')
-			->willReturn(true);
-		$request = $this->createMock(IRequest::class);
-		$config = $this->createMock(IConfig::class);
-		$this->plugin = new Plugin($authBackend, $request, $config);
-
-		$root = new SimpleCollection('root');
-		$this->server = new \Sabre\DAV\Server($root);
-		$this->book = $this->createMock(IShareable::class);
-		$this->book->method('getName')
-			->willReturn('addressbook1.vcf');
-		$root->addChild($this->book);
-		$this->plugin->initialize($this->server);
-	}
-
-	public function testSharing(): void {
-		$this->book->expects($this->once())->method('updateShares')->with([[
-			'href' => 'principal:principals/admin',
-			'commonName' => null,
-			'summary' => null,
-			'readOnly' => false
-		]], ['mailto:wilfredo@example.com']);
-
-		// setup request
-		$request = new Request('POST', 'addressbook1.vcf');
-		$request->addHeader('Content-Type', 'application/xml');
-		$request->setBody('<?xml version="1.0" encoding="utf-8" ?><CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns"><CS:set><D:href>principal:principals/admin</D:href><CS:read-write/></CS:set> <CS:remove><D:href>mailto:wilfredo@example.com</D:href></CS:remove></CS:share>');
-		$response = new Response();
-		$this->plugin->httpPost($request, $response);
-	}
-}

apps/dav/tests/unit/DAV/Security/RateLimitingTest.php

@@ -0,0 +1,99 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\Tests\unit\DAV\Security;
+
+use OCA\DAV\Connector\Sabre\Exception\TooManyRequests;
+use OCA\DAV\DAV\Security\RateLimiting;
+use OCP\IAppConfig;
+use OCP\IUser;
+use OCP\IUserSession;
+use OCP\Security\RateLimiting\ILimiter;
+use OCP\Security\RateLimiting\IRateLimitExceededException;
+use PHPUnit\Framework\MockObject\MockObject;
+use Test\TestCase;
+
+class RateLimitingTest extends TestCase {
+	private IUserSession $userSession;
+	private IAppConfig&MockObject $config;
+	private ILimiter&MockObject $limiter;
+	private RateLimiting $rateLimiting;
+	private string $userId = 'user123';
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->config = $this->createMock(IAppConfig::class);
+		$this->limiter = $this->createMock(ILimiter::class);
+
+		$this->rateLimiting = new RateLimiting(
+			$this->userSession,
+			$this->config,
+			$this->limiter,
+		);
+	}
+
+	public function testNoUserObject(): void {
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->willReturn(null);
+		$this->limiter->expects($this->never())
+			->method('registerUserRequest');
+
+		$this->rateLimiting->check();
+	}
+
+	public function testRegisterShareRequest(): void {
+		$user = $this->createMock(IUser::class);
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->willReturn($user);
+		$this->config->method('getValueInt')
+			->willReturnCallback(static function (string $app, string $key, int $default): int {
+				return match ($key) {
+					'rateLimitShareAddressbookOrCalendar' => 7,
+					'rateLimitPeriodShareAddressbookOrCalendar' => 600,
+					default => $default,
+				};
+			});
+		$this->limiter->expects($this->once())
+			->method('registerUserRequest')
+			->with(
+				'share-addressbook-or-calendar',
+				7,
+				600,
+				$user,
+			);
+
+		$this->rateLimiting->check();
+	}
+
+	public function testShareRequestRateLimitExceeded(): void {
+		$user = $this->createMock(IUser::class);
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->willReturn($user);
+		$this->config->method('getValueInt')
+			->willReturnArgument(2);
+		$this->limiter->expects($this->once())
+			->method('registerUserRequest')
+			->with(
+				'share-addressbook-or-calendar',
+				20,
+				3600,
+				$user,
+			)
+			->willThrowException($this->createMock(IRateLimitExceededException::class));
+
+		$this->expectException(TooManyRequests::class);
+
+		$this->rateLimiting->check();
+	}
+}

apps/dav/tests/unit/DAV/Sharing/PluginTest.php

@@ -9,11 +9,13 @@ declare(strict_types=1);
 namespace OCA\DAV\Tests\unit\DAV\Sharing;
 
 use OCA\DAV\Connector\Sabre\Auth;
+use OCA\DAV\DAV\Security\RateLimiting;
 use OCA\DAV\DAV\Sharing\IShareable;
 use OCA\DAV\DAV\Sharing\Plugin;
 use OCP\IConfig;
 use OCP\IRequest;
 use PHPUnit\Framework\MockObject\MockObject;
+use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Server;
 use Sabre\DAV\SimpleCollection;
 use Sabre\HTTP\Request;
@@ -24,6 +26,7 @@ class PluginTest extends TestCase {
 	private Plugin $plugin;
 	private Server $server;
 	private IShareable&MockObject $book;
+	private RateLimiting&MockObject $rateLimiting;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -33,11 +36,11 @@ class PluginTest extends TestCase {
 
 		$request = $this->createMock(IRequest::class);
 		$config = $this->createMock(IConfig::class);
-		$this->plugin = new Plugin($authBackend, $request, $config);
+		$this->rateLimiting = $this->createMock(RateLimiting::class);
+		$this->plugin = new Plugin($authBackend, $request, $config, $this->rateLimiting);
 
 		$root = new SimpleCollection('root');
-		$this->server = new \Sabre\DAV\Server($root);
-		/** @var SimpleCollection $node */
+		$this->server = new Server($root);
 		$this->book = $this->createMock(IShareable::class);
 		$this->book->method('getName')->willReturn('addressbook1.vcf');
 		$root->addChild($this->book);
@@ -45,18 +48,64 @@ class PluginTest extends TestCase {
 	}
 
 	public function testSharing(): void {
-		$this->book->expects($this->once())->method('updateShares')->with([[
-			'href' => 'principal:principals/admin',
-			'commonName' => null,
-			'summary' => null,
-			'readOnly' => false
-		]], ['mailto:wilfredo@example.com']);
+		$this->rateLimiting->expects(self::once())
+			->method('check');
+		$this->book->expects(self::once())
+			->method('updateShares')
+			->with([[
+				'href' => 'principal:principals/admin',
+				'commonName' => null,
+				'summary' => null,
+				'readOnly' => false,
+			]], ['mailto:wilfredo@example.com']);
+		$body = '<?xml version="1.0" encoding="utf-8" ?><CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns"><CS:set><D:href>principal:principals/admin</D:href><CS:read-write/></CS:set> <CS:remove><D:href>mailto:wilfredo@example.com</D:href></CS:remove></CS:share>';
 
-		// setup request
+		$this->executeRequest($body);
+	}
+
+	public function testEmptyShareRequestIsRejected(): void {
+		$this->rateLimiting->expects(self::once())
+			->method('check');
+		$this->book->expects(self::never())
+			->method('updateShares');
+		$this->expectException(BadRequest::class);
+		$this->expectExceptionMessage('{http://owncloud.org/ns}share needs at least one set or remove element');
+		$body = '<?xml version="1.0" encoding="utf-8" ?><CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns"></CS:share>';
+
+		$this->executeRequest($body);
+	}
+
+	public function testShareRequestWithTooManyElementsIsRejected(): void {
+		$this->rateLimiting->expects(self::once())
+			->method('check');
+		$this->book->expects(self::never())
+			->method('updateShares');
+		$this->expectException(BadRequest::class);
+		$this->expectExceptionMessage('{http://owncloud.org/ns}share is limited to 10 set or remove elements');
+		$body = '<?xml version="1.0" encoding="utf-8" ?><CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns">'
+			. '<CS:set><D:href>principal:principals/user1</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user2</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user3</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user4</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user5</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user6</D:href></CS:set>'
+			. '<CS:set><D:href>principal:principals/user7</D:href></CS:set>'
+			. '<CS:remove><D:href>principal:principals/user8</D:href></CS:remove>'
+			. '<CS:remove><D:href>principal:principals/user9</D:href></CS:remove>'
+			. '<CS:remove><D:href>principal:principals/user10</D:href></CS:remove>'
+			. '<CS:remove><D:href>principal:principals/user11</D:href></CS:remove>'
+			. '<CS:remove><D:href>principal:principals/user12</D:href></CS:remove>'
+			. '</CS:share>';
+
+		$this->executeRequest($body);
+	}
+
+	private function executeRequest(string $body): void {
 		$request = new Request('POST', 'addressbook1.vcf');
 		$request->addHeader('Content-Type', 'application/xml');
-		$request->setBody('<?xml version="1.0" encoding="utf-8" ?><CS:share xmlns:D="DAV:" xmlns:CS="http://owncloud.org/ns"><CS:set><D:href>principal:principals/admin</D:href><CS:read-write/></CS:set> <CS:remove><D:href>mailto:wilfredo@example.com</D:href></CS:remove></CS:share>');
+		$request->setBody($body);
 		$response = new Response();
+
 		$this->plugin->httpPost($request, $response);
 	}
 }