regression: add rate limiters to the thread REST endpoints - #41984
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (17)
🧰 Additional context used📓 Path-based instructions (2)The main Rocket.Chat Meteor application resides in `apps/meteor/`; place its application code there rather than in other monorepo areas.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc) Files:
🔇 Additional comments (1)
WalkthroughThree chat REST endpoints now limit requests to 20 per 10 seconds through ChangesChat endpoint rate limiting
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change is localized to restoring rate limits for the thread endpoints, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. 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 @@
## release-8.8.0 #41984 +/- ##
================================================
Coverage ? 69.38%
================================================
Files ? 4254
Lines ? 168911
Branches ? 30113
================================================
Hits ? 117191
Misses ? 46570
Partials ? 5150
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
FragaKrummenauer
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Two of the thread pane's data sources moved off DDP during the 8.8.0 cycle and landed on REST endpoints that never declared
rateLimiterOptions, so they inherited the default REST rule — 10 calls per 60 seconds per IP, a ceiling designed for machine-to-machine traffic. No call site changed, only the budget.Neither DDP method had a dedicated rule, so both ran on the generic per-method limits in
apps/meteor/server/startup/rateLimiter.js:Connection_By_Method(10 / 10s per connection) andUser_By_Method(20 / 10s per user), both applied at once. This PR restores the per-user ceiling on all three thread routes.POST /v1/chat.readThreadreadThreadsDDP)GET /v1/chat.getThreadMessagesgetThreadMessagesDDP)GET /v1/chat.getThreadsListWhy the per-user rule and not the per-connection one. The REST limiter keys on IP, a coarser bucket than a DDP connection — one tab was one connection, and a user with two tabs had two 10-call buckets under a shared 20-call ceiling. Copying the per-connection number onto an IP key would be stricter than DDP ever was, and would fail this ticket's own reproduction: 11 replies arriving as a burst is 11 calls in one window, which 10/10s rejects exactly like 10/60s does.
chat.getThreadsListis not part of the regression — it has been REST since 2022 and has no earlier value to restore. It is included because it is the third leg of the same pane with the same paginated shape, so leaving it on 10/60s would just move the failure one panel over. It can be dropped without affecting the reported bug.Issue(s)
CORE-2649
Steps to test or reproduce
chat.readThread(the reported case):API_Enable_Rate_Limiter; leave the defaults at 10 calls / 60000 ms.POST /api/v1/chat.readThreadreturns 429; after it, all succeed. A'stunreadshould not retain the thread'stmid.chat.getThreadMessages— open 11 different threads within 60 seconds (each open costs one page request). Before this change the 11th thread fails to load its replies; after it, all open normally. Same result by dragging the scrollbar of a single thread past ~550 replies.Further comments
1. Per-user keying (follow-up: CORE-2637). This restores the DDP budget, but keyed on IP, not userId — all the REST limiter can do today. A single user is now at or above what DDP gave them; users sharing a NAT still share one bucket. These three routes should switch over once per-user keying lands.
2. The migration guide needs updating in the meantime.
docs/api-endpoint-migration.mdnever mentions carrying the method's rate limit over, which is what produced both this regression and CORE-2629. It needs a step: copy the method's dedicatedRateLimiter.limitMethodrule, or if it had none, declarerateLimiterOptionsexplicitly — the REST default is a machine-to-machine ceiling, not a UI one.Summary by CodeRabbit