Security: Prevent room ID enumeration#2518
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "room-enumeration" rate limiter that counts only 404 responses for unresolved room parameters, applies that throttle to many room-related API and web routes, updates the changelog, and adds a feature test validating per-user/IP 404 rate limiting and reset behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
TODO
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2518 +/- ##
==========================================
Coverage 96.77% 96.77%
- Complexity 1839 1841 +2
==========================================
Files 444 444
Lines 12570 12579 +9
Branches 2063 2063
==========================================
+ Hits 12164 12173 +9
Misses 406 406 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PILOS
|
||||||||||||||||||||||||||||
| Project |
PILOS
|
| Branch Review |
sec-prevent-room-enumeration
|
| Run status |
|
| Run duration | 06m 56s |
| Commit |
|
| Committer | Samuel Weirich |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
615
|
| View all changes introduced in this branch ↗︎ | |
2d96ac3 to
61c5d8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Providers/RouteServiceProvider.php (1)
76-89: Limiter design is sound; make limits configurable and verify 'after' semantics
- Counts only 404s when the room param didn’t bind to a Room model. Good approach to avoid false positives.
- Suggest reading limits from config (e.g., perMinutes/attempts), not hard-coded 10/min.
Proposed change:
- RateLimiter::for('room-enumeration', function (Request $request) { - return Limit::perMinute(10) + RateLimiter::for('room-enumeration', function (Request $request) { + $max = config('rate_limits.room_enumeration.max_attempts', 10); + $decay = config('rate_limits.room_enumeration.decay_minutes', 1); + return Limit::perMinutes($decay, $max) ->by($request->user()?->id ?: $request->ip()) ->after(function (\Symfony\Component\HttpFoundation\Response $response) use ($request) { // If the response is not a 404, do not count this request if ($response->getStatusCode() !== 404) { return false; } // Only count the request if the route parameter 'room' was not resolved to a Room model // Prevent counting requests that are valid and return a 404 for other reasons return ! ($request->route('room') instanceof Room); }); });Please confirm the project’s Laravel version supports Limit::after with a boolean return to control counting. If not, we’ll switch to an alternative (e.g., custom middleware with RateLimiter::hit only on 404). Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(2 hunks)app/Providers/RouteServiceProvider.php(2 hunks)routes/api.php(2 hunks)tests/Backend/Feature/api/v1/Room/RoomTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/Providers/RouteServiceProvider.php (2)
app/Models/User.php (1)
User(20-349)_ide_helper.php (2)
for(9551-9555)ip(10222-10226)
tests/Backend/Feature/api/v1/Room/RoomTest.php (1)
app/Models/Room.php (2)
Room(19-464)owner(214-217)
routes/api.php (7)
app/Http/Controllers/api/v1/RoomController.php (1)
RoomController(32-426)app/Http/Controllers/api/v1/RoomMemberController.php (1)
RoomMemberController(20-215)app/Http/Controllers/api/v1/RecordingController.php (1)
RecordingController(16-103)app/Http/Controllers/api/v1/RoomStreamingController.php (1)
RoomStreamingController(16-135)app/Http/Controllers/api/v1/RoomTokenController.php (1)
RoomTokenController(17-139)app/Http/Controllers/api/v1/RoomFileController.php (1)
RoomFileController(16-163)app/Http/Controllers/api/v1/RecordingFormatController.php (1)
RecordingFormatController(10-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker Build
🔇 Additional comments (6)
CHANGELOG.md (2)
13-13: Changelog entry LGTMAccurately reflects the new security feature.
561-561: Reference link added correctlyFootnote for [#2518] is consistent and valid.
tests/Backend/Feature/api/v1/Room/RoomTest.php (1)
419-470: Strong coverage for 404-based throttlingCovers guest vs auth, cross-route blocking, window reset, and excluding valid-room 404s. Nicely done.
routes/api.php (3)
77-137: Throttle scope alignment looks good; confirm intent to block writes after thresholdWrapping all per-room endpoints means once the 404 threshold is exceeded, even valid writes (update/delete, streaming actions, membership ops, etc.) are blocked for the window. This matches the PR note (“all room API calls are blocked”). If that’s intended, LGTM.
164-165: Meetings endpoint throttled consistentlyBrings meetings under the same limiter. Consistent with the threat model.
171-187: Public room routes throttled with auth middleware where neededGood use of room.authenticate and scopeBindings alongside the throttle to keep behavior consistent for guests and users.
88ee280 to
bc9287a
Compare
…numeration # Conflicts: # CHANGELOG.md # routes/api.php
Type
Checklist
Changes
Other information
All API requests resulting in a 404 response due to an invalid room ID are counted. If the limit exceeds the threshold all room api calls are blocked.
Summary by CodeRabbit
New Features
Documentation
Tests