Rate limit websocket joins. #2165
Conversation
crates/websocket/src/routes.rs
Outdated
| } | ||
| actix::fut::ready(()) | ||
| }) | ||
| .wait(ctx); |
There was a problem hiding this comment.
don't wait on the check future like this, check doesn't need to be a async
There was a problem hiding this comment.
you should also put rate limiting in the StreamHandler. Only limiting connect doesn't get you much, since connect is called once per session, and not on every message
crates/websocket/src/routes.rs
Outdated
| @@ -98,6 +105,8 @@ impl Handler<WsMessage> for WsSession { | |||
| /// WebSocket message handler | |||
| impl StreamHandler<Result<ws::Message, ws::ProtocolError>> for WsSession { | |||
| fn handle(&mut self, result: Result<ws::Message, ws::ProtocolError>, ctx: &mut Self::Context) { | |||
| self.rate_limit_check(ctx); | |||
There was a problem hiding this comment.
if the check fails you should return early
crates/websocket/src/routes.rs
Outdated
| @@ -57,6 +61,9 @@ impl Actor for WsSession { | |||
| // before processing any other events. | |||
| // across all routes within application | |||
| let addr = ctx.address(); | |||
|
|
|||
| self.rate_limit_check(ctx); | |||
There was a problem hiding this comment.
if the check fails you should return early
| UserOperation::GetCaptcha => rate_limiter.post().check(ip).await, | ||
| _ => rate_limiter.message().check(ip).await, | ||
| UserOperation::GetCaptcha => rate_limiter.post().check(ip), | ||
| _ => rate_limiter.message().check(ip), |
There was a problem hiding this comment.
This means that we are checking the message rate limit twice in a row. Should be removed (also in line 488), so that only specific rate limits (register, post etc) are checked here, in addition to message rate limit.
There was a problem hiding this comment.
I made a new PR to address this.
No description provided.