Incorrect implementation of Bukkit-based chat events and their cancellation status
Hopefuls opened this issue · 10 comments
Hello! Supporter from AdvancedBan here!
It seems like you have redirected a user within your server due to the assumption of an issue being related to us. After inspecting the source code on how your plugin handles cancelled chat events, I have found the issue that is related to your plugin, not AdvancedBan or any other Plugin used within the bukkit-environment:
This Bukkit-based Chat Listener never respects the cancellation status and will still send moderative messages from your plugin despite the event being cancelled. This is not an issue with other plugins that may apply cancelling on events, but how your plugin handles it.
My suggested change is to first of all check the cancellation status before processing the event. If the event has been cancelled already, it can simply be ignored and does not need further processing. Likely how it was done on your bungeecord-version: https://github.com/2lstudios-mc/ChatSentinel/blob/2c1577f850b371b9af04920ca8c8566ab2b5850d/src/main/java/dev/_2lstudios/chatsentinel/bungee/listeners/ChatListener.java#L131
Kind regards
Cancellation check is already done. AdvancedBans probably runs after this plugin and doesn't check cancellation.
Both AdvancedBan and your Plugin messed up the EventPriorities. AdvancedBan is set to HIGHEST yours to LOWEST.
But Event Priorities work the other way around:
Listeners are called in following order: LOWEST -> LOW -> NORMAL -> HIGH -> HIGHEST -> MONITOR
(https://hub.spigotmc.org/javadocs/spigot/org/bukkit/event/EventPriority.html)
So your Plugin @linsaftw always calls first (obviously event isnt cancelled at that point of time)
Then the Ban Plugin is called.
So ChatSentinels Priority should be set to HIGHEST. To work with any Mute Plugin
The described issue of the user occurs with every Ban Plugin i tested (EssentialsX, AdvancedBan, LibertyBans)
AdvancedBan also needs a Change so the priority isnt HIGHEST but something lower or maybe even better a configuration option (could be useful for your plugin aswell)
Aswell as the above mentioned by alfi, this mostly depends on what the user prefers:
- Should the user still get your plugins message that you cant say a swear word?
- Should the user get the muted message instead? (this would be a good use-case seeing from a usage-perspective)
To also note: ignoreCancelled is essentially worthless if your Plugin Listener runs first.
Solution: AdvancedBans to Lowest and ChatSentinel to Low. Is that good for you guys?
ChatSentinel can't be set to normal or higher because it filters flooding to prevent exploits. Otherwise Normal priority plugins will trigger and be exploited.
That will work for Advancedban (The fix for Advancedban will probably take some time because there are other issues which need fixing before releasing a new version).
Keep in mind that other Mute-Plugins which have set the event priority to normal or higher will keep causing issues with your Plugin.
That will work for Advancedban (The fix for Advancedban will probably take some time because there are other issues which need fixing before releasing a new version).
Keep in mind that other Mute-Plugins which have set the event priority to normal or higher will keep causing issues with your Plugin.
Bad coded plugins. Not my concern. Plugins that cancel must be set to LOW or LOWEST.
Will be fixed now. Thanks.
That will work for Advancedban (The fix for Advancedban will probably take some time because there are other issues which need fixing before releasing a new version).
Keep in mind that other Mute-Plugins which have set the event priority to normal or higher will keep causing issues with your Plugin.Bad coded plugins. Not my concern. Plugins that cancel must be set to LOW or LOWEST.
Didn't we have to point out your mistake with mixing up even priorities? There was a reason why advancedban had to be set to highesr for compatibility with another plugin. So I wouldnt call those "bad coded Plugins" (just because they aren't doing it the way you want them) . As you need to set yours to LOW. Maybe they have a reason to set it to something Higher aswell. The best option is probably to let the user decide which one to use. If they need your flooding then they need to go low if they don't care about that part they can set it higher so it works with other mute plugins. That's how advancedban is going to implement it in the future. (Letting the user choose with an info to be careful)
Bro, cancelling should be done on LOW or LOWEST. The reason is NORMAL priority will be most of plugins including chat ones, opening doors to high risk exploits.
So, if you cancel on NORMAL or higher for the mute system you will basically get the server fricked up because you are allowing clients to spam NORMAL or higher priority plugins.
Because ChatSentinel depends on a mute plugin, it is okay for me to change from LOWEST to LOW, but the layer of the MUTE plugin should be first priority (LOWEST) to prevent exploits.