EssentialsX

EssentialsX

2M Downloads

EssentialsChat "reverts" the modification of chat messages that were previously modified by other plugins

Akiranya opened this issue ยท 12 comments

commented

Type of bug

Compatibility issue, Other unexpected behaviour

/ess dump all output

https://essentialsx.net/dump.html?id=5189ff1529f64e51b045f39d742616ea

Error log (if applicable)

No response

Bug description

I have ShowItem installed on my server, which is a plugin that lets player send clickable item of their own to the chat. There is a feature of ShowItem: player can type a placeholder such as #h and sent it out. The placeholder is then replaced with actually clickable item in the player's hand.

Ideally, the placeholder should be replaced correctly - the placeholder becomes a actually clickable item so that other players can click it and view it on the chat. But with EssentialsChat installed, this feature of ShowItem doesn't work anymore. The placeholder is sent as it is - nothing is replaced.

Steps to reproduce

  1. Have ShowItem and EssentialsChat installed.
  2. Send a placeholder to the chat.
  3. The placeholder is as it is.

Expected behaviour

The placeholder of ShowItem should be replaced correctly with EssentialsChat installed.

Actual behaviour

The placeholder of ShowItem is not replaced correctly with EssentialsChat installed.

I also have reported this issue on the ShowItem issue tracker. Please see here for more information. I have post a full log of the modifications on the AsyncPlayerChatEvent using EventDebugger plugin. That should provide deeper information about this bug.

commented

EssentialsX Chat only currently supports modifications to chat messages within Bukkit's AsyncPlayerChatEvent. We're working on supporting Paper's component-based chat event, but this is dependent on #4717.

commented

Thank you for the reply! I will try to tweak ShowItem to see if I can let it handle the modifications on AsyncPlayerChatEvent

commented

It can't - it relies on chat components, which don't exist on Bukkit. You'll need to wait for EssentialsX Chat to support components first.

commented

will be in a follow up pr of mine after #4717

commented

Hi, I'd like to add to this - it's not only affecting chat components. My plugin (ChatColor2) uses the AsyncPlayerChatEvent - the default event priority for my plugin is LOWEST, however, even with all essentials.chat. permissions, all colour applied by my plugin to chat messages is erased.

In the past, assigning the essentials.chat.color permission allowed for the colours to appear (regardless of ChatColor's listener priority), but this no longer works - I have to set the event priority of my plugin to a higher priority than EssentialsChat for it to work (I have tested HIGHEST and it works).

If you need any additional info I'll be happy to provide it :)

commented

Yes! I'm seeing the same thing with both Spigot and Paper. I have a language filter plugin which uses AsyncPlayerChatEvent#setMessage() to replace naughty words. If my listener is at priority LOWEST, any changes my plugin makes with setMessage() get lost, and the original message is output. If I set my listener to LOW, it works correctly.

If I remove EssentialsXChat, my plugin works properly.

commented

I found another case of EssentialsXChat tampering with the text of chat messages.

I have a language filter plugin which can change words in chat messages. For this test, I have it changing "kys" to "ily" in both the preview event and the regular chat event. My event handler is at priority LOW.

As you can see below, AsyncPlayerChatEvent#getMessage() should return the actual string typed by the player ("kys"). But with EssentialsXChat installed, it's returning the modified string from the Preview Event.

This is without EssentialsXChat:

AsyncPlayerChatPreviewEvent: k
AsyncPlayerChatPreviewEvent: ky
AsyncPlayerChatPreviewEvent: kys
AsyncPlayerChatEvent: kys <<<----- This is what it should be

This is with EssentialsXChat:

AsyncPlayerChatPreviewEvent: k
AsyncPlayerChatPreviewEvent: ky
AsyncPlayerChatPreviewEvent: kys
AsyncPlayerChatEvent: ily <<<----- This is what EssentialsXChat corrupts

Why is EssentialsXChat changing the behavior of AsyncPlayerChatEvent? This is important because it affects what plugins see and how they handle the event.

EssentialsChat v2.19.7 "Secure signed chat and previews are enabled."
git-Paper-215 (MC: 1.19.2)

commented

It doesn't apply changes from other plugins unless they also modified the message during the preview event (in which case, they would need to be applied to match the message signature).

Yes, but that modification should be done by the other plugin, not by Essentials, because the other plugin may have additional processing to do during the chat event that it couldn't do during the preview event.

We will revisit the current behaviour when we have a clearer picture of how chat signing will work in the future.

Maybe it will become moot, but here's a use case: Plugin wants to replace profanity with other text and set the player on fire. The plugin can change the text in the preview event, but setting the player on fire there would be bad, so that has to wait for the actual chat event. Essentials' current processing makes that impossible because the string with profanity is replaced by Essentials during the chat first, so the plugin doesn't know that profanity was used.

And then there's the whole thing about Essentials putting the profanity back in the chat!

But we'll see how this all falls out in 1.19.3.

commented

This line is the root of all evil. It is causing both problems. 1) It can throw away changes made by another plugin. 2) It can take changes from another plugin's preview event processing and pass them off as the actual typed message, depriving the other plugin from the opportunity to see the original message in the chat event.

Following is the case of chat previews being turned off in server.properties and not used by the other plugin which listens at priority LOWEST. You can see the other plugin changes the string to "ily". handleChatApplyPreview gets this modified string ("ily"), but then changes it back to "kys" by calling event.setMessage()! Bad EssentialsChat, bad!

2022-10-19 08:38:48 [INFO] [PwnFilter] Activated PlayerListener with Priority Setting: LOWEST
2022-10-19 08:39:46 [INFO] <S:[Member]Bobcat00> Chat previews are off in server properties and PwnFilter config. On in the client.

2022-10-19 08:40:20 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatFormat: kys
2022-10-19 08:40:20 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatPostFormat: kys
2022-10-19 08:40:20 [INFO] [PwnFilter] AsyncPlayerChatEvent: kys
2022-10-19 08:40:20 [INFO] [PwnFilter] |CHAT| MATCH <InfectedCatBite> kys
2022-10-19 08:40:20 [INFO] [PwnFilter] <Abort> Not processing more rules.
2022-10-19 08:40:20 [INFO] [PwnFilter] |CHAT| SENT <InfectedCatBite> ily
2022-10-19 08:40:20 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatApplyPreview: ily
2022-10-19 08:40:20 [INFO] [EssentialsChat] event.setMessage: kys
2022-10-19 08:40:20 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatRecipients: kys
2022-10-19 08:40:20 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatSubmit: kys
2022-10-19 08:40:20 [INFO] <S:[Member]InfectedCatBite> kys

In the following case, previews are fully on (client is set to 'While Typing') and the other plugin listens at priority LOW. handleChatApplyPreview gets the original string and calls event.setMessage() to change it to the old preview string. If the other plugin has additional processing in its chat event listener, it doesn't see the original string, only what EssentialsChat has set it to, as if that's what the player typed.

2022-10-19 08:09:14 [INFO] [PwnFilter] Activated PlayerListener with Priority Setting: LOW

2022-10-19 08:10:19 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatFormat: k
2022-10-19 08:10:19 [INFO] [PwnFilter] AsyncPlayerChatPreviewEvent: k
2022-10-19 08:10:19 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatPostFormat: k

2022-10-19 08:10:23 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatFormat: ky
2022-10-19 08:10:23 [INFO] [PwnFilter] AsyncPlayerChatPreviewEvent: ky
2022-10-19 08:10:23 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatPostFormat: ky

2022-10-19 08:10:27 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatFormat: kys
2022-10-19 08:10:27 [INFO] [PwnFilter] AsyncPlayerChatPreviewEvent: kys
2022-10-19 08:10:27 [INFO] [EssentialsChat] AsyncPlayerChatPreviewEvent handleChatPostFormat: ily

2022-10-19 08:10:33 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatApplyPreview: kys
2022-10-19 08:10:33 [INFO] [EssentialsChat] event.setMessage: ily
2022-10-19 08:10:33 [INFO] [PwnFilter] AsyncPlayerChatEvent: ily
2022-10-19 08:10:33 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatRecipients: ily
2022-10-19 08:10:33 [INFO] [EssentialsChat] AsyncPlayerChatEvent handleChatSubmit: ily

I don't know how to fix this, because I don't know what all this cache handling is trying to accomplish, or why 6 event handlers running at 3 different priorities are necessary. But EssentialsChat shouldn't be tampering with messages in this manner.

commented

On 1.19.1+, EssentialsX Chat performs chat formatting during the preview event so that the client can see and sign the formatted chat message. When the player presses Enter to send the message, it applies the cached modifications at LOWEST priority (as you can see by reading the call to that method), and any plugins using a higher priority will apply their own modifications afterwards. The other event handlers take care of other parts of the formatting and recipient handling and have always been present.

commented

EssentialsXChat shouldn't be applying cached modifications from other plugins. Let those plugins take care of their own changes.

But it probably doesn't matter, because after spending hours on this, Mojang says:

CHANGES IN 22W42A [1.19.3]
Removed Chat Preview
commented

It doesn't apply changes from other plugins unless they also modified the message during the preview event (in which case, they would need to be applied to match the message signature).

We're waiting to see what exactly happens with chat previews both in the next MC release and in Spigot. We will revisit the current behaviour when we have a clearer picture of how chat signing will work in the future.