-
Notifications
You must be signed in to change notification settings - Fork 122
[1.x] WebSocket Rate Limiting Implementation #326
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Added rate limiting configuration to `reverb.php` for WebSocket messages. - Introduced `WebSocketRateLimitManager` to handle rate limiting logic. - Updated `Server` class to utilize the rate limiter and handle rate limit exceptions. - Created `RateLimitExceededException` for managing rate limit errors. - Added unit and feature tests to ensure proper rate limiting behavior and edge cases. This enhancement improves the stability and security of WebSocket connections by preventing abuse through rate limiting.
- Updated rate limiting configuration in `reverb.php` to increase `max_attempts` to 60 and added `terminate_on_limit` option. - Replaced `WebSocketRateLimitManager` with a new `RateLimitManager` class to streamline rate limiting logic. - Modified `Server` and `ReverbServiceProvider` to utilize the new `RateLimitManager`. - Enhanced `RateLimitManager` with methods for handling rate limits, checking remaining attempts, and managing connection termination. - Updated tests to reflect changes in rate limiting behavior and added new assertions for connection termination. - Improved test coverage for the `RateLimitManager` to ensure robust functionality. This update enhances the rate limiting capabilities, providing better control over message handling and connection management.
Feature/rate limiter
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
Thanks @raphaelcangucu, drafting for now, but will pull this down and take it for a spin soon. |
Thanks @joedixon . Let me know if you need us to change anything. Basically if the user is spamming we are disconnecting him, to avoid the service to go down . |
@raphaelcangucu, I see a bunch of methods in the
In addition, this implementation is at the server level, I'm wondering if the rate limiting should be configured per application. What do you think? |
Great! The limit isn't configurable per application, right? |
Hey @joedixon , I just remember that we were using the functions on the test, to be more readable, since the attributes are protected. No, the limit isn't configured per application. |
Thanks @raphaelcangucu! I'm going to think on this a little more to decide the best route to take here. |
Hey @joedixon and @raphaelcangucu hope you're both doing well, To mention and rightfully praise, we're using Reverb and think it's amazing (Thank you @joedixon), for receiving Websocket messages we have some hacked together rate limiting logic so @raphaelcangucu a big thank you as well for furthering the push for support for this in Reverb If I may ask (and hopefully to help with this discussion and get everyones thoughts), could configuring rate limiting through the Reverb config file especially with keys such as max_attempts eventually prove restrictive and only solve one or a few types of classical rate limiting problems? i.e. "For every type of Websocket message the user can only make X number of requests in this time window" Referencing the Laravel documentation found here https://laravel.com/docs/12.x/routing#defining-rate-limiters (Sorry yes I do realize this is all about Websockets rather than HTTP requests so I will try relate this where I can, please shout back and or pick this apart) rate limiting is defined through code, with each definition having the ability to:
This is not to say to everyone "This is what Laravel does with rate limiting HTTP requests so Reverb should support all of this too" but only to suggest that defining rate limiting in the config file might prove eventually inflexible and restrictive, as well as with some assumptions made in terms of how rate limiting will be applied by developers Having said all of this, could an alternative solution be to implement a more route like way of receiving Websocket messages from users, with being able to apply the already existing rate limiting feature found in Laravel to Websocket routes? Sorry this is quite a long message and again hopefully does actually provide to the discussion, I'll add on a stripped back example of how we are handling different types of messages with the hacky rate limiting we're using, what I'm hoping with this example is to show that typically what is provided through the Laravel HTTP route and route handling is what we ended up needing here for handling different Websocket message types I'll lastly add that I'd be very happy to contribute to this discussion and or by trying to help implement this, I'm no expert by any means which must be more than obvious from my message, I'd like to think at best that this could be food for thought in keeping Reverb flexible and powerful, again big thank you to @joedixon and @raphaelcangucu Code example with rate limiting: <?php
// Hastily put together example which is likely to have errors from removing code to focus on the example
namespace App\Listeners;
use App\Models\User;
use App\Services\ChatMessageService;
use Illuminate\Support\Facades\RateLimiter;
use Laravel\Reverb\Events\MessageReceived;
use Exception;
use stdClass;
class WebsocketMessageReceived
{
private User $user;
public function __construct(
protected ChatMessageService $chatMessageService,
) {
//
}
public function handle(MessageReceived $event): void
{
$wsMessage = json_decode($event->message);
// Event name must be a string e.g client-interaction.
$wsEventType = $wsMessage->event ?? '';
$mapping = [
'client-chat-message-create' => [$this, 'handleTypeChatMessageCreate'],
'client-chat-message-delete' => [$this, 'handleTypeChatMessageDelete'],
];
if (!$classMethod = $mapping[$wsEventType] ?? null) {
throw new Exception('websocket:event:invalid');
}
$wsChannelUserId = $wsMessage->channel ?? '';
$wsChannelUserId = is_string($wsChannelUserId) ? $wsChannelUserId : '';
if (!$wsChannelUserId) {
throw new Exception('websocket:event:channel:invalid');
}
// Private channel is made up of both channel name and user ID.
[$channel, $userId] = explode('.', $wsChannelUserId, 2);
// User must be valid.
// Code removed for brevity.
$this->user = User::query()->find($userId);
// Build data.
$data = $wsMessage->data ?? null;
// Validate data.
// Code removed for brevity.
if (!($data instanceof stdClass)) {
throw new Exception('websocket:event:data:invalid');
}
// Tenant code removed for brevity.
// Must have tenant ID.
// Tenant ID must be valid.
// Tenant must exist.
// Tenant initialize.
// User must exist (Tenant aware).
[, $method] = $classMethod;
$this->$method($data);
}
private function handleTypeChatMessageCreate(stdClass $data)
{
// Rate limiting.
$rateLimitKey = 'chat-message-create:user:' . $this->user->id;
// Rate limit for now of 30 requests per minute.
$rateLimitIndividualPassed = RateLimiter::attempt(
$rateLimitKey,
30,
function () {
return;
},
);
if (!$rateLimitIndividualPassed) {
// Handle rate limiting here.
// Notify user of rate limit being exceeded.
// Code removed for brevity.
return;
}
// Create chat message.
// Code removed for brevity.
}
private function handleTypeChatMessageDelete(stdClass $data)
{
// Code removed for brevity.
// Rate limiting.
// Deletion of the chat message.
}
} |
Hi @alexuno , we tried to keep this PR as simple as possible to solve the SPAM problem. So basically we do a rate limit per connection and application, and if this limit is reached we thrown a error message or disconnect the user, to stop the SPAM. @joedixon is deciding what's the best route for this PR, since he has better knowledge about the reverb code base. In my opinion, better to solve the problem now, and after that improve and increase the scope of what it can do. Kind regards |
Hey @raphaelcangucu thank you for your reply, you're 100% right that @joedixon is the decision maker here on what's the best route for the PR That's completely fair about your opinion and I sincerely hope the opinion that I've offered in my original reply is relevant and has not served to annoy, after all we are here discussing this on a public PR that can be replied to by anyone not just the PR author and project maintainer(s) It would be nice to solve any problem really, but, 'the problem' that you mention is one that you are experiencing or that your organisation is facing, and yes trying to solve in a very appropriate way, however I would argue that the problem does not have a universal solution and might be approached and solved differently by different developers, which I don't say to be dismissive or rude but to try justify why I raised the issue of using the config file as too restrictive |
Hey @joedixon , is there any plans to merge this PR? Tks. |
I'm planning to tweak this a little bit to allow it to be configurable per app. |
WebSocket Rate Limiting Implementation
Overview
This PR implements a configurable rate limiting system for WebSocket messages in Laravel Reverb to prevent potential server overload and abuse.
Features
RateLimitManager
class that integrates with Laravel's existingRateLimiter
rate_limiting
section inconfig/reverb.php
Configuration Options
enabled
: Toggle rate limiting on/offmax_attempts
: Maximum number of messages allowed per time windowdecay_seconds
: Duration of the rate limiting window in secondsterminate_on_limit
: Whether to terminate connection when limit is exceededRelated Issue
Resolves #307