feat: implement command rate limiter #1524
Conversation
|
We don't have a means of updating config files properly, and the nature of this will induce weird protocol state if somebody does trigger this; I would generally mostly prefer that we address the bottleneck rather than introducing mechanisms like this which, with the current state of the protocol, creates a whole bunch of other issues if people start configuring this for anything but kicking bad clients |
You will need some sort of rate limiting mechanism to prevent this, can send thousands of packets a second on any velocity command |
|
The defaults should also be fine to the extent where most shouldn't need to configure it |
|
Could also add a hard minimum which isn't configurable so users can't break it |
|
Is there a reason why literal kick messages are used instead of translatables? |
|
Could add, figured it wouldn't be needed as literal messages are used pretty often for kicks (i.e VelocityServer L521) |
|
You should probably use translatables and change the English message to "You are tab completing too fast." rather than the message you have right now |
I don't know why that is, but you should be abel to just add a key to the .properties file(s) and use that in the translatable, we can translate at this point because the player was already connected. |
* Validate uncompressed packet size * Fix debug using incorrect value
There was a problem hiding this comment.
The biggest issue with this is that we have no means to update peoples config files properly, will probably have to figure out some quick mitigation for that in this case, I know that we've been able to handle this somewhat manually but it's far from ideal; I don't really wanna add a bunch of new config options, especially which have side effects, which don't show up in peoples existing config files
|
This pr currently has an issue with signed commands e.g. the built-in Quick demo: |
My latest commit should resolve this |
Could make the defaults extra tame |
|
Additionally, now that it forwards the packet to the server upon hitting rate limit, I could disable kicks by default |
|
Then would you rename |
|
Is it intentional that tab completes don't reset to 0 when not failing, if a player fails once it never resets so over time it will eventually kick them, the limits currently set are good, but without it resetting to 0 you will get players that are kicked if not adjusted |
Wasn't intentional no, fixed :D |
|
Any update on this? |
|
Generally needs testing/additional feedback, been too preoccupied to deal with this myself |
Okay, thanks for the clarification. |
|
Our server and many others medium / large servers have been running this in production for weeks now, without any issues |
|
I would generally want 3rd party confirmation of that, but I'll look at merging this tomorrow morning unless somebody else gets to it |
|
tested aswell since a months with no issues and default values |
Velocity proxies can still be crashed with any other commands besides
/velocity info. A proper fix for this is to rate limit the frequency of which a client can send commands to the server. Currently there is none.