Skip to content

Commit 1fcafb0

Browse files
skjnldsvskjnldsv
committed
fix(sharing): rate limit the API for how the dialog uses it
Every write endpoint allowed one call per one to five seconds, which the dialog exceeds on its own: it creates a draft when it opens, adds a recipient per pick, and fans a preset change out over one request per permission. Size the limits per minute from those flows instead, keep the tighter one on regenerating a token, and add the missing limit on the per-recipient permission endpoint. The end-to-end tests no longer switch the limiter off, so they double as the check that the limits are livable. Signed-off-by: Barthelemy Briand <barthelemy.briand@nextcloud.com> Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
1 parent a0f656c commit 1fcafb0

2 files changed

Lines changed: 23 additions & 26 deletions

File tree

‎apps/sharing/lib/Controller/ApiV1Controller.php‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ public function __construct(
9292
*/
9393
#[NoAdminRequired]
9494
#[ApiRoute(verb: 'GET', url: '/api/v1/recipients')]
95-
#[UserRateLimit(limit: 5, period: 1)]
95+
#[UserRateLimit(limit: 60, period: 60)]
9696
public function searchRecipients(?array $filterRecipientTypeClasses, string $query, int $limit = 10, int $offset = 0, ?string $id = null): DataResponse {
9797
/** @psalm-suppress DocblockTypeContradiction */
9898
if ($limit < 1) {
@@ -147,7 +147,7 @@ public function generateSecret(): DataResponse {
147147
*/
148148
#[NoAdminRequired]
149149
#[ApiRoute(verb: 'POST', url: '/api/v1/share')]
150-
#[UserRateLimit(limit: 1, period: 5)]
150+
#[UserRateLimit(limit: 30, period: 60)]
151151
public function createShare(): DataResponse {
152152
try {
153153
try {
@@ -180,7 +180,7 @@ public function createShare(): DataResponse {
180180
*/
181181
#[NoAdminRequired]
182182
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/state')]
183-
#[UserRateLimit(limit: 1, period: 5)]
183+
#[UserRateLimit(limit: 60, period: 60)]
184184
public function updateShareState(string $id, string $state): DataResponse {
185185
try {
186186
$shareState = ShareState::from($state);
@@ -220,7 +220,7 @@ public function updateShareState(string $id, string $state): DataResponse {
220220
*/
221221
#[NoAdminRequired]
222222
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/user-status')]
223-
#[UserRateLimit(limit: 1, period: 5)]
223+
#[UserRateLimit(limit: 60, period: 60)]
224224
public function updateShareUserStatus(string $id, string $userStatus): DataResponse {
225225
try {
226226
$shareUserStatus = ShareUserStatus::from($userStatus);
@@ -260,7 +260,7 @@ public function updateShareUserStatus(string $id, string $userStatus): DataRespo
260260
*/
261261
#[NoAdminRequired]
262262
#[ApiRoute(verb: 'POST', url: '/api/v1/share/{id}/source')]
263-
#[UserRateLimit(limit: 1, period: 1)]
263+
#[UserRateLimit(limit: 60, period: 60)]
264264
public function addShareSource(string $id, string $class, string $value): DataResponse {
265265
try {
266266
try {
@@ -297,7 +297,7 @@ public function addShareSource(string $id, string $class, string $value): DataRe
297297
*/
298298
#[NoAdminRequired]
299299
#[ApiRoute(verb: 'DELETE', url: '/api/v1/share/{id}/source')]
300-
#[UserRateLimit(limit: 1, period: 1)]
300+
#[UserRateLimit(limit: 60, period: 60)]
301301
public function removeShareSource(string $id, string $class, string $value): DataResponse {
302302
try {
303303
try {
@@ -334,7 +334,7 @@ public function removeShareSource(string $id, string $class, string $value): Dat
334334
*/
335335
#[NoAdminRequired]
336336
#[ApiRoute(verb: 'POST', url: '/api/v1/share/{id}/recipient')]
337-
#[UserRateLimit(limit: 1, period: 1)]
337+
#[UserRateLimit(limit: 120, period: 60)]
338338
public function addShareRecipient(string $id, string $class, string $value, ?string $instance): DataResponse {
339339
try {
340340
try {
@@ -372,7 +372,7 @@ public function addShareRecipient(string $id, string $class, string $value, ?str
372372
*/
373373
#[NoAdminRequired]
374374
#[ApiRoute(verb: 'DELETE', url: '/api/v1/share/{id}/recipient')]
375-
#[UserRateLimit(limit: 1, period: 1)]
375+
#[UserRateLimit(limit: 120, period: 60)]
376376
public function removeShareRecipient(string $id, string $class, string $value, ?string $instance): DataResponse {
377377
try {
378378
try {
@@ -410,7 +410,7 @@ public function removeShareRecipient(string $id, string $class, string $value, ?
410410
*/
411411
#[NoAdminRequired]
412412
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/recipient/secret')]
413-
#[UserRateLimit(limit: 1, period: 5)]
413+
#[UserRateLimit(limit: 20, period: 60)]
414414
public function updateShareRecipientSecret(string $id, string $class, string $value, ?string $instance, string $secret): DataResponse {
415415
try {
416416
try {
@@ -448,7 +448,7 @@ public function updateShareRecipientSecret(string $id, string $class, string $va
448448
*/
449449
#[NoAdminRequired]
450450
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/property')]
451-
#[UserRateLimit(limit: 1, period: 1)]
451+
#[UserRateLimit(limit: 120, period: 60)]
452452
public function updateShareProperty(string $id, string $class, ?string $value): DataResponse {
453453
try {
454454
try {
@@ -486,7 +486,7 @@ public function updateShareProperty(string $id, string $class, ?string $value):
486486
*/
487487
#[NoAdminRequired]
488488
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/permission')]
489-
#[UserRateLimit(limit: 1, period: 1)]
489+
#[UserRateLimit(limit: 240, period: 60)]
490490
public function updateSharePermission(string $id, string $class, bool $enabled): DataResponse {
491491
try {
492492
try {
@@ -526,6 +526,7 @@ public function updateSharePermission(string $id, string $class, bool $enabled):
526526
*/
527527
#[NoAdminRequired]
528528
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/recipient/permission')]
529+
#[UserRateLimit(limit: 240, period: 60)]
529530
public function updateShareRecipientPermission(string $id, string $recipientClass, string $recipientValue, ?string $recipientInstance, string $permissionClass, bool $enabled): DataResponse {
530531
try {
531532
try {
@@ -560,7 +561,7 @@ public function updateShareRecipientPermission(string $id, string $recipientClas
560561
*/
561562
#[NoAdminRequired]
562563
#[ApiRoute(verb: 'PUT', url: '/api/v1/share/{id}/permission/preset')]
563-
#[UserRateLimit(limit: 1, period: 1)]
564+
#[UserRateLimit(limit: 60, period: 60)]
564565
public function selectSharePermissionPreset(string $id, string $permissionPresetClass): DataResponse {
565566
try {
566567
try {
@@ -593,7 +594,7 @@ public function selectSharePermissionPreset(string $id, string $permissionPreset
593594
*/
594595
#[NoAdminRequired]
595596
#[ApiRoute(verb: 'DELETE', url: '/api/v1/share/{id}')]
596-
#[UserRateLimit(limit: 1, period: 5)]
597+
#[UserRateLimit(limit: 60, period: 60)]
597598
public function deleteShare(string $id): DataResponse {
598599
try {
599600
try {
@@ -628,7 +629,7 @@ public function deleteShare(string $id): DataResponse {
628629
#[PublicPage]
629630
// This should be a GET, but GET doesn't allow a request body which is required for the $arguments.
630631
#[ApiRoute(verb: 'POST', url: '/api/v1/share/{id}')]
631-
#[UserRateLimit(limit: 1, period: 1)]
632+
#[UserRateLimit(limit: 120, period: 60)]
632633
#[AnonRateLimit(limit: 1, period: 5)]
633634
#[BruteForceProtection(action: 'getShare')]
634635
public function getShare(string $id, ?string $secret = null, array $arguments = []): DataResponse {
@@ -667,7 +668,7 @@ public function getShare(string $id, ?string $secret = null, array $arguments =
667668
*/
668669
#[NoAdminRequired]
669670
#[ApiRoute(verb: 'GET', url: '/api/v1/shares')]
670-
#[UserRateLimit(limit: 1, period: 1)]
671+
#[UserRateLimit(limit: 240, period: 60)]
671672
public function getShares(?string $filterSourceTypeClass, ?string $filterSourceTypeValue, ?string $filterState, ?string $filterUserStatus, ?string $lastShareID, int $limit = 100): DataResponse {
672673
/** @psalm-suppress DocblockTypeContradiction */
673674
if ($limit < 1) {

‎tests/playwright/support/utils/unifiedSharing.ts‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -64,24 +64,20 @@ async function ocs<T>(
6464
}
6565

6666
/**
67-
* Turn the unified sharing API on for the whole instance and lift its rate
68-
* limits, then return a restore function.
67+
* Turn the unified sharing API on for the whole instance and return a restore
68+
* function. It ships enabled, so this only matters after a spec turned it off.
6969
*
70-
* The API is off by default (`sharing.unified_api_enable`), which is what makes
71-
* the sidebar render the legacy sections. Its write endpoints are also rate
72-
* limited to one call per 1–5 seconds per user, which no realistic test (or
73-
* user) can stay under — a dialog that creates a draft on open and adds two
74-
* recipients would already trip it — so the limiter goes off with it.
70+
* The rate limiter is deliberately left on: these specs drive the dialog the
71+
* way a person does, so they are also the check that the API's limits are
72+
* livable.
7573
*
76-
* Both are instance-wide, so specs using this must not run next to specs that
77-
* expect the legacy sidebar. See the `unified-sharing` Playwright project.
74+
* The switch is instance-wide, so specs using this must not run next to specs
75+
* that expect the legacy sidebar. See the serial `sharing` Playwright project.
7876
*/
7977
export async function enableUnifiedSharing(): Promise<() => Promise<void>> {
8078
await runOcc(['config:system:set', 'sharing.unified_api_enable', '--value', 'true', '--type', 'boolean'])
81-
await runOcc(['config:system:set', 'ratelimit.protection.enabled', '--value', 'false', '--type', 'boolean'])
8279
return async () => {
8380
await runOcc(['config:system:delete', 'sharing.unified_api_enable'], { failOnError: false })
84-
await runOcc(['config:system:delete', 'ratelimit.protection.enabled'], { failOnError: false })
8581
}
8682
}
8783

0 commit comments

Comments
 (0)