From 34d27f438e5289b7cca84e6c615194a82ae6004f Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Wed, 8 Apr 2026 15:52:42 +0200 Subject: [PATCH] fix(dav): Limit share requests AI-assisted: OpenCode (gpt-5.4) Signed-off-by: Daniel Kesselberg --- .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + apps/dav/lib/DAV/Security/RateLimiting.php | 46 +++++++++ apps/dav/lib/DAV/Sharing/Plugin.php | 30 ++++-- apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php | 6 ++ apps/dav/lib/Server.php | 5 +- .../tests/unit/CardDAV/Sharing/PluginTest.php | 62 ------------ .../unit/DAV/Security/RateLimitingTest.php | 99 +++++++++++++++++++ .../dav/tests/unit/DAV/Sharing/PluginTest.php | 73 +++++++++++--- 9 files changed, 240 insertions(+), 83 deletions(-) create mode 100644 apps/dav/lib/DAV/Security/RateLimiting.php delete mode 100644 apps/dav/tests/unit/CardDAV/Sharing/PluginTest.php create mode 100644 apps/dav/tests/unit/DAV/Security/RateLimitingTest.php diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 4e27a3f574229..b5b0c8ec728f9 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -264,6 +264,7 @@ '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', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 2f57fe9b48b73..2237d54610ba2 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -279,6 +279,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', diff --git a/apps/dav/lib/DAV/Security/RateLimiting.php b/apps/dav/lib/DAV/Security/RateLimiting.php new file mode 100644 index 0000000000000..cdee6dbd37fd8 --- /dev/null +++ b/apps/dav/lib/DAV/Security/RateLimiting.php @@ -0,0 +1,46 @@ +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); + } + } +} diff --git a/apps/dav/lib/DAV/Sharing/Plugin.php b/apps/dav/lib/DAV/Sharing/Plugin.php index 82b000bc8ce61..9c5c002b42b53 100644 --- a/apps/dav/lib/DAV/Sharing/Plugin.php +++ b/apps/dav/lib/DAV/Sharing/Plugin.php @@ -10,11 +10,13 @@ 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 @@ public function httpPost(RequestInterface $request, ResponseInterface $response) // 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 @@ public function httpPost(RequestInterface $request, ResponseInterface $response) } } + 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; diff --git a/apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php b/apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php index aefb39c5701de..fad8e52283895 100644 --- a/apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php +++ b/apps/dav/lib/DAV/Sharing/Xml/ShareRequest.php @@ -12,6 +12,8 @@ use Sabre\Xml\XmlDeserializable; class ShareRequest implements XmlDeserializable { + public const ELEMENT_SHARE = '{' . Plugin::NS_OWNCLOUD . '}share'; + /** * Constructor * @@ -33,6 +35,10 @@ public static function xmlDeserialize(Reader $reader) { $set = []; $remove = []; + if ($elements === null) { + return new self($set, $remove); + } + foreach ($elements as $elem) { switch ($elem['name']) { diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 2cf2f575596fc..51c0b3b6ccdd3 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -55,6 +55,7 @@ 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; @@ -198,7 +199,7 @@ public function __construct( // 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))); @@ -221,7 +222,7 @@ public function __construct( // 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()); diff --git a/apps/dav/tests/unit/CardDAV/Sharing/PluginTest.php b/apps/dav/tests/unit/CardDAV/Sharing/PluginTest.php deleted file mode 100644 index 1e934a69a5385..0000000000000 --- a/apps/dav/tests/unit/CardDAV/Sharing/PluginTest.php +++ /dev/null @@ -1,62 +0,0 @@ -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('principal:principals/admin mailto:wilfredo@example.com'); - $response = new Response(); - $this->plugin->httpPost($request, $response); - } -} diff --git a/apps/dav/tests/unit/DAV/Security/RateLimitingTest.php b/apps/dav/tests/unit/DAV/Security/RateLimitingTest.php new file mode 100644 index 0000000000000..69b39957319bb --- /dev/null +++ b/apps/dav/tests/unit/DAV/Security/RateLimitingTest.php @@ -0,0 +1,99 @@ +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(); + } +} diff --git a/apps/dav/tests/unit/DAV/Sharing/PluginTest.php b/apps/dav/tests/unit/DAV/Sharing/PluginTest.php index 7a88f7cc5ddde..5f4b9f00b748a 100644 --- a/apps/dav/tests/unit/DAV/Sharing/PluginTest.php +++ b/apps/dav/tests/unit/DAV/Sharing/PluginTest.php @@ -9,11 +9,13 @@ 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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { } 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 + $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 = 'principal:principals/admin mailto:wilfredo@example.com'; + + $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 = ''; + + $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 = '' + . 'principal:principals/user1' + . 'principal:principals/user2' + . 'principal:principals/user3' + . 'principal:principals/user4' + . 'principal:principals/user5' + . 'principal:principals/user6' + . 'principal:principals/user7' + . 'principal:principals/user8' + . 'principal:principals/user9' + . 'principal:principals/user10' + . 'principal:principals/user11' + . 'principal:principals/user12' + . ''; + + $this->executeRequest($body); + } + + private function executeRequest(string $body): void { $request = new Request('POST', 'addressbook1.vcf'); $request->addHeader('Content-Type', 'application/xml'); - $request->setBody('principal:principals/admin mailto:wilfredo@example.com'); + $request->setBody($body); $response = new Response(); + $this->plugin->httpPost($request, $response); } }