PILOS icon indicating copy to clipboard operation
PILOS copied to clipboard

Security: Prevent room ID enumeration

Open samuelwei opened this issue 2 months ago • 4 comments

Type

  • Bugfix
  • Feature
  • Documentation
  • Refactoring (e.g. Style updates, Test implementation, etc.)
  • Other (please describe)

Checklist

  • [x] Code updated to current develop branch head
  • [x] Passes CI checks
  • [ ] Is a part of an issue
  • [x] Tests added for the bugfix or newly implemented feature, describe below why if not
  • [x] Changelog is updated
  • [ ] Documentation of code and features exists

Changes

  • Add rate limiting to room API endpoints to mitigate room ID enumeration attacks

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

    • Rate limiting added for room-related endpoints: only counts requests that return 404, limited to 10 requests/minute per user/IP; many room management, membership, files, recordings, tokens, and streaming routes are now throttled.
  • Documentation

    • Changelog updated with an Unreleased entry referencing the new rate-limiting change.
  • Tests

    • Added tests validating 404-based rate limiting for guests and authenticated users, related routes, per-user behavior, and limit reset timing.

samuelwei avatar Oct 09 '25 12:10 samuelwei

Walkthrough

Adds a "room-enumeration" rate limiter that counts only 404 responses for room-bound routes, applies that throttle to many room-related API endpoints, updates the changelog with the PR, and adds a feature test verifying per-user/IP 404 rate limiting and reset behavior.

Changes

Cohort / File(s) Summary
Changelog update
CHANGELOG.md
Added Unreleased entry for "Rate limiting to prevent Room-ID enumeration attacks" and referenced PR [#2518].
Rate limiter configuration
app/Providers/RouteServiceProvider.php
Added room-enumeration limiter (10 requests/min per user or IP) that only increments when the response is 404 and the room route param does not resolve to a Room; added Room and Response imports.
Routing — room throttling and reorganization
routes/api.php
Wrapped a wide set of room-related endpoints in throttle:room-enumeration, reorganized route groups (added/adjusted can:*, scopeBindings, room auth middleware), and moved many room CRUD, membership, files, recordings, streaming, tokens, meetings, start/join/settings, and related OPTIONS into the throttled scope.
Tests — feature coverage
tests/Backend/Feature/api/v1/Room/RoomTest.php
Added test_room_404_rate_limit() to validate 404-only counting for guests and authenticated users, per-user/IP scoping, related-route coverage (e.g., rooms.show and files.show), and time-based reset behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • danielmachill

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "Security: Prevent room ID enumeration" directly and concisely describes the primary objective of the changeset. The changes add rate limiting specifically to prevent room ID enumeration attacks, as evidenced by the new 'room-enumeration' rate limiter in RouteServiceProvider, the throttled route groups in routes/api.php, and the test coverage validating this behavior. The title is clear, specific, and appropriately categorized with the "Security:" prefix, allowing teammates to quickly understand the main purpose of the change.
Description Check ✅ Passed The PR description follows the template structure with all major sections present: Type is clearly identified as "Feature," the Checklist includes appropriate items marked as completed (code updated, CI passes, tests added, changelog updated), Changes section provides a concise summary of the rate limiting implementation, and Other information explains the behavior. While the "Fixes #" section is empty and two checklist items are unchecked ("Is a part of an issue" and "Documentation of code and features exists"), these appear non-critical as the template acknowledges that not all points are always applicable. The description is substantially complete and conveys the PR's purpose clearly.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch sec-prevent-room-enumeration

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fdd79b8dfcd7ae786ac6b06bcf2b0a3eb3bcb2ec and e804e32e361e2f8f5ec7a5814f8050022d9322eb.

📒 Files selected for processing (1)
  • CHANGELOG.md (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
⏰ 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). (2)
  • GitHub Check: Backend
  • GitHub Check: Docker Build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 09 '25 12:10 coderabbitai[bot]

TODO

  • [x] Add tests
  • [x] Add log entries

samuelwei avatar Oct 09 '25 12:10 samuelwei

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 96.66%. Comparing base (bc9287a) to head (e804e32). :warning: Report is 1 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2518      +/-   ##
=============================================
+ Coverage      96.03%   96.66%   +0.63%     
- Complexity      1647     1649       +2     
=============================================
  Files            249      426     +177     
  Lines           5747    11999    +6252     
  Branches           0     2063    +2063     
=============================================
+ Hits            5519    11599    +6080     
- Misses           228      400     +172     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 09 '25 13:10 codecov[bot]

PILOS    Run #2652

Run Properties:  status check passed Passed #2652  •  git commit e804e32e36: Security: Prevent room ID enumeration
Project PILOS
Branch Review sec-prevent-room-enumeration
Run status status check passed Passed #2652
Run duration 07m 27s
Commit git commit e804e32e36: Security: Prevent room ID enumeration
Committer Samuel Weirich
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 599
View all changes introduced in this branch ↗︎

cypress[bot] avatar Oct 09 '25 13:10 cypress[bot]