regression: add chat.sendMessage rate limiter - #41966
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 (3)
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. (4)
|
| Layer / File(s) | Summary |
|---|---|
Rate-limit bypass contract and helper apps/meteor/server/api/definition.ts, apps/meteor/server/api/api.ts, apps/meteor/server/api/api.helpers.ts, apps/meteor/tests/unit/server/api/canBypassRateLimit.spec.ts |
RateLimiterOptions now includes bypassPermissions. canBypassRateLimit checks global and route-specific permissions. Unit tests cover matching and non-matching permissions. |
Rate-limit verification and chat configuration apps/meteor/server/api/ApiClass.ts, apps/meteor/server/api/v1/chat.ts |
Rate-limit verification uses route-configured bypass permissions. chat.sendMessage allows five requests per second and exempts send-many-messages. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Merge Risk: 🔵 Low · up to d3c64
The endpoint now allows five messages per second, while users with the send-many-messages permission can bypass the limit. Because limiting is shared by IP address, one user can still throttle others on the same network, and permission grants should be confirmed by the owning team before merge.
Suggested labels: type: bug
Suggested reviewers: sampaiodiego
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly identifies the main change: adding a rate limiter to the chat.sendMessage endpoint to address a regression. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
Warning
Errors were encountered while retrieving linked issues.
Errors (1)
- JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.
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 @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.8.0 #41966 +/- ##
================================================
Coverage ? 69.38%
================================================
Files ? 4254
Lines ? 168903
Branches ? 30153
================================================
Hits ? 117190
Misses ? 46555
Partials ? 5158
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
6e14f80 to
7cb492c
Compare
ricardogarim
changed the title
![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
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Layne could not analyze all changed content. Review the Check Run summary before merging. |
FragaKrummenauer
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
4 participants
Proposed changes (including videos or screenshots)
POST /v1/chat.sendMessagedeclared norateLimiterOptions, so it fell back to the REST API default of 10 requests per 60 seconds — a default added in #11251 (2019) for machine-to-machine traffic.When the web client's send flow moved from the
sendMessageMeteor method to REST in #40675, it inherited that budget. The method had its own rule (5 messages per second, since 2015), so the allowance dropped from 300 to 10 messages per minute and the 11th message returned429.This declares the previous allowance on the endpoint, the same way
users.setStatusdoes:Issue(s)
Steps to test or reproduce
Requires a server started without
TEST_MODEand withAPI_Enable_Rate_Limiter_Devenabled — otherwise no rule is registered and noX-RateLimit-*headers are returned.As a regular user without
api-bypass-rate-limit:200. The 11th used to return429.200, rest429.200. The window is 1 second, not 60.botrole → no rejections.Further comments
The REST limiter buckets by IP address, not by user — the counter key is
IPAddr+ route, with no per-route way to change it. This allowance is therefore shared by everyone behind one address, unlike the Meteor rule, which was keyed byuserId. Keying onuserIdwould remove that ceiling, but it affects every route and is deliberately left out of this fix.There's a follow up task to handle per user: CORE-2637
Summary by CodeRabbit
New Features
Bug Fixes