chore: align REST rate limits with the DDP methods they replaced - #42104
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 8770314 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
WalkthroughThe change adds explicit rate limits to four REST endpoints: ChangesREST rate limit alignment
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested labels: Merge Risk: ⚪ Minimal · up to The reviewed rate-limit alignment introduces no substantiated merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Errors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #42104 +/- ##
===========================================
+ Coverage 69.41% 69.43% +0.02%
===========================================
Files 4310 4310
Lines 177068 177426 +358
Branches 31500 31517 +17
===========================================
+ Hits 122910 123199 +289
- Misses 49045 49109 +64
- Partials 5113 5118 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
18228a2 to
3fcab23
Compare
|
@cubic-dev-ai review |
@ricardogarim I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
sampaiodiego
left a comment
sampaiodiego
left a comment
There was a problem hiding this comment.
can you please check the ones that were using DDPRateLimiter.addRule as well?
|
we may want to increate the default rate limiter values at some point as well, they are currently too restrictive.. 10 requests per minute is far from an abusive limit. |
ricardogarim
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
removed
the
stat: ready to merge
dionisio-bot
Bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
Aligns the rate limits of REST endpoints with the DDP methods whose traffic they now carry.
A DDP method keeps its manual rule even when called over HTTP, because
method.call/:methodappliesDDPRateLimiteritself. The rule only goes dormant when the client switches to a dedicated endpoint, and that endpoint then gets the generic REST default: 10 / 60s per IP (API_Enable_Rate_Limiter_Limit_Calls_Default). CORE-2629 was the first case noticed, onchat.sendMessage. This PR fixes four more and documents the ones we looked at and left alone.↑more permissive than today,=unchanged.spotlightspotlight— 100 / 100sdirectorybrowseChannels— 100 / 100schat.followMessagefollowMessage— 5 / 5schat.unfollowMessageunfollowMessage— 5 / 5susers.setStatussetUserStatus— 1 / 1susers.setAvatarsetAvatarFromService— 1 / 5srooms.infogetRoomById— 10 / 60sim.create/dm.createcreateDirectMessage— 10 / 60susers.forgotPasswordsendForgotPasswordEmail— 10 / 60sNo endpoint ends up stricter than it is today. Why the
=rows were left alone:users.setStatusandusers.setAvatar: their DDP rules only work per user. On a per-IP bucket an allowance of 1 rejects the second call in the window from anyone behind one address. The 5 / 60s onsetStatusis also deliberate (chore: remove rate limiter for functions #38354), andsetAvatarFromServicenever covered uploads or URLs, which the endpoint also carries.rooms.info,im.create/dm.create,users.forgotPassword: the DDP value equals the REST default, so declaring it would only detach the route fromAPI_Enable_Rate_Limiter_Limit_Calls_Default. Deployments behind a shared address rely on that setting forrooms.info, which the client hits on every discussion or omnichannel room open. Thesend-many-messagesbypass was not carried over either;api-bypass-rate-limitcovers the same roles.users.resetAvatar(DDP 1 / 60s) andautotranslate.getSupportedLanguages(DDP 5 / 60s): the method is tighter than the REST default, andresetAvataralso serves the moderation console. Lowering limits to match is a separate call.setEmailandsetRealName(DDP 1 / 1s each) have no endpoint of their own: the profile form sends everything throughusers.updateOwnBasicInfo, which stays at 1 / 60s. That endpoint also changes the password, and the limit is brute-force protection from Chore: Convert users endpoints #25635; matching the methods would loosen it sixty-fold.Already aligned:
chat.sendMessage,users.register,push.test,audit.auditions,audit.messages.sendSMTPTestEmailis still called as a method (admin action button), so its DDP rule still applies.userSetUtcOffsetis no longer called at all; the client writes the offset throughusers.setPreferences.That accounts for all 21 manual DDP rules in the repo (
DDPRateLimiter.addRuleandRateLimiter.limitMethod, 20 distinct methods): 4 fixed here, 5 already aligned, 9 documented above as left alone, 1 still on DDP, 1 superseded.Issue(s)
Steps to test or reproduce
Run without
TEST_MODE(env -u TEST_MODE yarn dev) and enableAPI_Enable_Rate_Limiter_Dev. Ondevelopthe eleventh navbar search inside a minute returns429; here a hundred pass and the hundred-and-first is rejected.Further comments
We considered keying the limiter by user instead of by address (#41970 added a
per: 'user'option for that) and decided to adjust the route values for now and close that PR.So these buckets are still keyed by IP, shared by everyone behind one address. With a wide window that dilutes well enough;
spotlightis the one to watch, since the composer autocomplete calls it per keystroke. If it hurts in practice, per-user keying is the follow-up.Endpoints that never had a manual DDP rule also lost the 20 / 10s per-user floor and sit on 10 / 60s per IP. That is a much larger set, and a conversation rather than a patch.