Skip to content

Commit 55fb75d

Browse files
ChristophWurstChristophWurst
authored
Merge pull request #59834 from nextcloud/backport/59778/stable33
[stable33] Avoid undefined array key sharing request
2 parents d443b3d + d265606 commit 55fb75d

9 files changed

Lines changed: 240 additions & 83 deletions

File tree

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@
267267
'OCA\\DAV\\DAV\\GroupPrincipalBackend' => $baseDir . '/../lib/DAV/GroupPrincipalBackend.php',
268268
'OCA\\DAV\\DAV\\PublicAuth' => $baseDir . '/../lib/DAV/PublicAuth.php',
269269
'OCA\\DAV\\DAV\\RemoteUserPrincipalBackend' => $baseDir . '/../lib/DAV/RemoteUserPrincipalBackend.php',
270+
'OCA\\DAV\\DAV\\Security\\RateLimiting' => $baseDir . '/../lib/DAV/Security/RateLimiting.php',
270271
'OCA\\DAV\\DAV\\Sharing\\Backend' => $baseDir . '/../lib/DAV/Sharing/Backend.php',
271272
'OCA\\DAV\\DAV\\Sharing\\IShareable' => $baseDir . '/../lib/DAV/Sharing/IShareable.php',
272273
'OCA\\DAV\\DAV\\Sharing\\Plugin' => $baseDir . '/../lib/DAV/Sharing/Plugin.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@ class ComposerStaticInitDAV
282282
'OCA\\DAV\\DAV\\GroupPrincipalBackend' => __DIR__ . '/..' . '/../lib/DAV/GroupPrincipalBackend.php',
283283
'OCA\\DAV\\DAV\\PublicAuth' => __DIR__ . '/..' . '/../lib/DAV/PublicAuth.php',
284284
'OCA\\DAV\\DAV\\RemoteUserPrincipalBackend' => __DIR__ . '/..' . '/../lib/DAV/RemoteUserPrincipalBackend.php',
285+
'OCA\\DAV\\DAV\\Security\\RateLimiting' => __DIR__ . '/..' . '/../lib/DAV/Security/RateLimiting.php',
285286
'OCA\\DAV\\DAV\\Sharing\\Backend' => __DIR__ . '/..' . '/../lib/DAV/Sharing/Backend.php',
286287
'OCA\\DAV\\DAV\\Sharing\\IShareable' => __DIR__ . '/..' . '/../lib/DAV/Sharing/IShareable.php',
287288
'OCA\\DAV\\DAV\\Sharing\\Plugin' => __DIR__ . '/..' . '/../lib/DAV/Sharing/Plugin.php',

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\DAV\Security;
11+
12+
use OCA\DAV\Connector\Sabre\Exception\TooManyRequests;
13+
use OCP\IAppConfig;
14+
use OCP\IUserSession;
15+
use OCP\Security\RateLimiting\ILimiter;
16+
use OCP\Security\RateLimiting\IRateLimitExceededException;
17+
18+
class RateLimiting {
19+
20+
public function __construct(
21+
private readonly IUserSession $userSession,
22+
private readonly IAppConfig $config,
23+
private readonly ILimiter $limiter,
24+
) {
25+
}
26+
27+
/**
28+
* @throws TooManyRequests
29+
*/
30+
public function check(): void {
31+
$user = $this->userSession->getUser();
32+
if ($user === null) {
33+
return;
34+
}
35+
36+
$identifier = 'share-addressbook-or-calendar';
37+
$userLimit = $this->config->getValueInt('dav', 'rateLimitShareAddressbookOrCalendar', 20);
38+
$userPeriod = $this->config->getValueInt('dav', 'rateLimitPeriodShareAddressbookOrCalendar', 3600);
39+
40+
try {
41+
$this->limiter->registerUserRequest($identifier, $userLimit, $userPeriod, $user);
42+
} catch (IRateLimitExceededException $e) {
43+
throw new TooManyRequests('Too many addressbook or calendar share requests', 0, $e);
44+
}
45+
}
46+
}

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

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@
1010
use OCA\DAV\CalDAV\CalDavBackend;
1111
use OCA\DAV\CalDAV\CalendarHome;
1212
use OCA\DAV\Connector\Sabre\Auth;
13+
use OCA\DAV\DAV\Security\RateLimiting;
1314
use OCA\DAV\DAV\Sharing\Xml\Invite;
1415
use OCA\DAV\DAV\Sharing\Xml\ShareRequest;
1516
use OCP\AppFramework\Http;
1617
use OCP\IConfig;
1718
use OCP\IRequest;
19+
use Sabre\DAV\Exception\BadRequest;
1820
use Sabre\DAV\Exception\NotFound;
1921
use Sabre\DAV\ICollection;
2022
use Sabre\DAV\INode;
@@ -28,17 +30,11 @@ class Plugin extends ServerPlugin {
2830
public const NS_OWNCLOUD = 'http://owncloud.org/ns';
2931
public const NS_NEXTCLOUD = 'http://nextcloud.com/ns';
3032

31-
/**
32-
* Plugin constructor.
33-
*
34-
* @param Auth $auth
35-
* @param IRequest $request
36-
* @param IConfig $config
37-
*/
3833
public function __construct(
3934
private Auth $auth,
4035
private IRequest $request,
4136
private IConfig $config,
37+
private RateLimiting $rateLimiting,
4238
) {
4339
}
4440

@@ -136,6 +132,9 @@ public function httpPost(RequestInterface $request, ResponseInterface $response)
136132
// calendar.
137133
case '{' . self::NS_OWNCLOUD . '}share':
138134

135+
$this->rateLimiting->check();
136+
$this->validateShareRequest($message);
137+
139138
// We can only deal with IShareableCalendar objects
140139
if (!$node instanceof IShareable) {
141140
return;
@@ -170,6 +169,23 @@ public function httpPost(RequestInterface $request, ResponseInterface $response)
170169
}
171170
}
172171

172+
private function validateShareRequest($shareRequest): void {
173+
if (!$shareRequest instanceof ShareRequest) {
174+
// @FIXME: Replace switch-case in httpPost with instanceof ShareRequest
175+
throw new BadRequest('The given request is not valid');
176+
}
177+
178+
$elements = (count($shareRequest->set) + count($shareRequest->remove));
179+
180+
if ($elements === 0) {
181+
throw new BadRequest(ShareRequest::ELEMENT_SHARE . ' needs at least one set or remove element');
182+
}
183+
184+
if ($elements > 10) {
185+
throw new BadRequest(ShareRequest::ELEMENT_SHARE . ' is limited to 10 set or remove elements');
186+
}
187+
}
188+
173189
private function preloadCollection(PropFind $propFind, ICollection $collection): void {
174190
if (!$collection instanceof CalendarHome || $propFind->getDepth() !== 1) {
175191
return;

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
use Sabre\Xml\XmlDeserializable;
1313

1414
class ShareRequest implements XmlDeserializable {
15+
public const ELEMENT_SHARE = '{' . Plugin::NS_OWNCLOUD . '}share';
16+
1517
/**
1618
* Constructor
1719
*
@@ -33,6 +35,10 @@ public static function xmlDeserialize(Reader $reader) {
3335
$set = [];
3436
$remove = [];
3537

38+
if ($elements === null) {
39+
return new self($set, $remove);
40+
}
41+
3642
foreach ($elements as $elem) {
3743
switch ($elem['name']) {
3844

‎apps/dav/lib/Server.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
use OCA\DAV\Connector\Sabre\ZipFolderPlugin;
5656
use OCA\DAV\DAV\CustomPropertiesBackend;
5757
use OCA\DAV\DAV\PublicAuth;
58+
use OCA\DAV\DAV\Security\RateLimiting;
5859
use OCA\DAV\DAV\ViewOnlyPlugin;
5960
use OCA\DAV\Db\PropertyMapper;
6061
use OCA\DAV\Events\SabrePluginAddEvent;
@@ -199,7 +200,7 @@ public function __construct(
199200

200201
// calendar plugins
201202
if ($this->requestIsForSubtree(['calendars', 'public-calendars', 'system-calendars', 'principals'])) {
202-
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class)));
203+
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class), \OCP\Server::get(RateLimiting::class)));
203204
$this->server->addPlugin(new \OCA\DAV\CalDAV\Plugin());
204205
$this->server->addPlugin(new ICSExportPlugin(\OCP\Server::get(IConfig::class), $logger));
205206
$this->server->addPlugin(new \OCA\DAV\CalDAV\Schedule\Plugin(\OCP\Server::get(IConfig::class), \OCP\Server::get(LoggerInterface::class), \OCP\Server::get(DefaultCalendarValidator::class)));
@@ -222,7 +223,7 @@ public function __construct(
222223

223224
// addressbook plugins
224225
if ($this->requestIsForSubtree(['addressbooks', 'principals'])) {
225-
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class)));
226+
$this->server->addPlugin(new DAV\Sharing\Plugin($authBackend, \OCP\Server::get(IRequest::class), \OCP\Server::get(IConfig::class), \OCP\Server::get(RateLimiting::class)));
226227
$this->server->addPlugin(new \OCA\DAV\CardDAV\Plugin());
227228
$this->server->addPlugin(new VCFExportPlugin());
228229
$this->server->addPlugin(new MultiGetExportPlugin());

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

Lines changed: 0 additions & 62 deletions
This file was deleted.

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Tests\unit\DAV\Security;
11+
12+
use OCA\DAV\Connector\Sabre\Exception\TooManyRequests;
13+
use OCA\DAV\DAV\Security\RateLimiting;
14+
use OCP\IAppConfig;
15+
use OCP\IUser;
16+
use OCP\IUserSession;
17+
use OCP\Security\RateLimiting\ILimiter;
18+
use OCP\Security\RateLimiting\IRateLimitExceededException;
19+
use PHPUnit\Framework\MockObject\MockObject;
20+
use Test\TestCase;
21+
22+
class RateLimitingTest extends TestCase {
23+
private IUserSession $userSession;
24+
private IAppConfig&MockObject $config;
25+
private ILimiter&MockObject $limiter;
26+
private RateLimiting $rateLimiting;
27+
private string $userId = 'user123';
28+
29+
protected function setUp(): void {
30+
parent::setUp();
31+
32+
$this->userSession = $this->createMock(IUserSession::class);
33+
$this->config = $this->createMock(IAppConfig::class);
34+
$this->limiter = $this->createMock(ILimiter::class);
35+
36+
$this->rateLimiting = new RateLimiting(
37+
$this->userSession,
38+
$this->config,
39+
$this->limiter,
40+
);
41+
}
42+
43+
public function testNoUserObject(): void {
44+
$this->userSession->expects($this->once())
45+
->method('getUser')
46+
->willReturn(null);
47+
$this->limiter->expects($this->never())
48+
->method('registerUserRequest');
49+
50+
$this->rateLimiting->check();
51+
}
52+
53+
public function testRegisterShareRequest(): void {
54+
$user = $this->createMock(IUser::class);
55+
$this->userSession->expects($this->once())
56+
->method('getUser')
57+
->willReturn($user);
58+
$this->config->method('getValueInt')
59+
->willReturnCallback(static function (string $app, string $key, int $default): int {
60+
return match ($key) {
61+
'rateLimitShareAddressbookOrCalendar' => 7,
62+
'rateLimitPeriodShareAddressbookOrCalendar' => 600,
63+
default => $default,
64+
};
65+
});
66+
$this->limiter->expects($this->once())
67+
->method('registerUserRequest')
68+
->with(
69+
'share-addressbook-or-calendar',
70+
7,
71+
600,
72+
$user,
73+
);
74+
75+
$this->rateLimiting->check();
76+
}
77+
78+
public function testShareRequestRateLimitExceeded(): void {
79+
$user = $this->createMock(IUser::class);
80+
$this->userSession->expects($this->once())
81+
->method('getUser')
82+
->willReturn($user);
83+
$this->config->method('getValueInt')
84+
->willReturnArgument(2);
85+
$this->limiter->expects($this->once())
86+
->method('registerUserRequest')
87+
->with(
88+
'share-addressbook-or-calendar',
89+
20,
90+
3600,
91+
$user,
92+
)
93+
->willThrowException($this->createMock(IRateLimitExceededException::class));
94+
95+
$this->expectException(TooManyRequests::class);
96+
97+
$this->rateLimiting->check();
98+
}
99+
}

0 commit comments

Comments
 (0)