-
Notifications
You must be signed in to change notification settings - Fork 12
Add smart chat resend/clear detection and intercept <actionbar> & <sound> tags #25
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||||||||||||||||
| import to.itsme.itsmyconfig.util.IMCSerializer; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.Strings; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.Utilities; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.ChatResendDetector; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||
| import java.util.Optional; | ||||||||||||||||||||
|
|
@@ -52,14 +53,21 @@ public void onPacketSend(final PacketSendEvent event) { | |||||||||||||||||||
| if (!(type instanceof PacketType.Play.Server server)) { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Record EVERY packet for burst detection (including blank lines) | ||||||||||||||||||||
| final Player player = event.getPlayer(); | ||||||||||||||||||||
| final String playerIdentifier = player.getUniqueId().toString(); | ||||||||||||||||||||
| ChatResendDetector.recordPacket(playerIdentifier); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final boolean isInBurst = ChatResendDetector.isInBurst(playerIdentifier); | ||||||||||||||||||||
| Utilities.debug(() -> "################# CHAT PACKET #################\nProcessing packet " + server.name() + | ||||||||||||||||||||
| (isInBurst ? " (RESEND DETECTED)" : "")); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final PacketProcessor<?> processor = packetTypeMap.get(server); | ||||||||||||||||||||
| if (processor == null) { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Utilities.debug(() -> "################# CHAT PACKET #################\nProcessing packet " + server.name()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Convert to wrapped packet only once | ||||||||||||||||||||
| Object wrappedPacket = switch (server) { | ||||||||||||||||||||
| case CHAT_MESSAGE -> new WrapperPlayServerChatMessage(event); | ||||||||||||||||||||
|
|
@@ -89,7 +97,12 @@ public void onPacketSend(final PacketSendEvent event) { | |||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final String message = packet.message(); | ||||||||||||||||||||
| Utilities.debug(() -> "Found message: " + message); | ||||||||||||||||||||
| final boolean hasInvisibleUnicode = ChatResendDetector.containsInvisibleUnicode(message); | ||||||||||||||||||||
| final boolean isChatClear = isInBurst || hasInvisibleUnicode; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Utilities.debug(() -> "Found message: " + message + | ||||||||||||||||||||
| (hasInvisibleUnicode ? " [INVISIBLE UNICODE]" : "") + | ||||||||||||||||||||
| (isChatClear ? " [CHAT CLEAR DETECTED]" : "")); | ||||||||||||||||||||
|
Comment on lines
+101
to
+105
|
||||||||||||||||||||
| final boolean isChatClear = isInBurst || hasInvisibleUnicode; | |
| Utilities.debug(() -> "Found message: " + message + | |
| (hasInvisibleUnicode ? " [INVISIBLE UNICODE]" : "") + | |
| (isChatClear ? " [CHAT CLEAR DETECTED]" : "")); | |
| Utilities.debug(() -> "Found message: " + message + | |
| (hasInvisibleUnicode ? " [INVISIBLE UNICODE]" : "") + | |
| ((isInBurst || hasInvisibleUnicode) ? " [CHAT CLEAR DETECTED]" : "")); |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation: The comment mentions checking for placeholders "like console does" but there's no reference to where this console behavior is implemented or why this change is needed for chat clear detection. This makes it unclear if this change is related to the PR's purpose or is an unrelated feature addition.
| // Also check if message contains ItsMyConfig placeholders even without prefix (like console does) | |
| // Also check if the message contains ItsMyConfig placeholders even without the symbol-prefix. | |
| // This mirrors the behavior of the server console, which allows sending messages with placeholders | |
| // (e.g., "<p:...>") directly, without requiring the prefix. See ConsoleCommandSender handling in | |
| // PacketProcessor#processConsoleMessage for details. This ensures that placeholder messages sent | |
| // from the console are processed correctly, and is also relevant for chat clear detection logic. |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder check using message.contains("<p:") is fragile and inconsistent with the tag detection pattern used elsewhere in the codebase. This hard-coded string check doesn't account for escaped tags or tags within quotes, and doesn't match the ARG_TAG_PATTERN regex used in TagManager. Consider using a consistent approach for detecting placeholders across the codebase.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||||
| import to.itsme.itsmyconfig.util.IMCSerializer; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.Strings; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.Utilities; | ||||||||||||||||||||
| import to.itsme.itsmyconfig.util.ChatResendDetector; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||
|
|
@@ -53,24 +54,35 @@ public void load() { | |||||||||||||||||||
| public void onPacketSending(final PacketEvent event) { | ||||||||||||||||||||
| final PacketContainer container = event.getPacket(); | ||||||||||||||||||||
| final PacketType type = container.getType(); | ||||||||||||||||||||
| Utilities.debug(() -> "################# CHAT PACKET #################\nProccessing packet " + type.name()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Record EVERY packet for burst detection (including blank lines) | ||||||||||||||||||||
| final String playerIdentifier = event.getPlayer().getUniqueId().toString(); | ||||||||||||||||||||
| ChatResendDetector.recordPacket(playerIdentifier); | ||||||||||||||||||||
|
Comment on lines
+58
to
+60
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| final boolean isInBurst = ChatResendDetector.isInBurst(playerIdentifier); | ||||||||||||||||||||
| Utilities.debug(() -> "################# CHAT PACKET #################\nProccessing packet " + type.name() + | ||||||||||||||||||||
|
||||||||||||||||||||
| Utilities.debug(() -> "################# CHAT PACKET #################\nProccessing packet " + type.name() + | |
| Utilities.debug(() -> "################# CHAT PACKET #################\nProcessing packet " + type.name() + |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable: The variable 'isChatClear' is computed but never used. It appears to be calculated for debugging purposes but isn't referenced anywhere in the method logic. Either use it for some purpose or remove it to avoid confusion.
| final boolean isChatClear = isInBurst || hasInvisibleUnicode; | |
| Utilities.debug(() -> "Found message: " + message + | |
| (hasInvisibleUnicode ? " [INVISIBLE UNICODE]" : "") + | |
| (isChatClear ? " [CHAT CLEAR DETECTED]" : "")); | |
| Utilities.debug(() -> "Found message: " + message + | |
| (hasInvisibleUnicode ? " [INVISIBLE UNICODE]" : "") + | |
| ((isInBurst || hasInvisibleUnicode) ? " [CHAT CLEAR DETECTED]" : "")); |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic error: The code now processes ALL messages (even those without the symbol prefix) by falling back to the original message when parsed.isEmpty(). This changes the original behavior which would return early if the message didn't have the prefix. Messages without the prefix should not be processed for placeholder replacement unless there's a specific reason documented in the PR description. This could lead to unintended processing of regular chat messages.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,122 @@ | ||||||||||||||||||||||
| package to.itsme.itsmyconfig.util; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||||||||||||
| import java.util.concurrent.atomic.AtomicInteger; | ||||||||||||||||||||||
| import java.util.concurrent.atomic.AtomicLong; | ||||||||||||||||||||||
| import java.util.regex.Pattern; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Detects rapid chat resend patterns used by chat deletion/moderation plugins. | ||||||||||||||||||||||
| * When plugins delete messages from chat, they typically flood blank lines to clear | ||||||||||||||||||||||
| * the chat window, then resend the chat history. This detector identifies that pattern | ||||||||||||||||||||||
| * to prevent actionbar/sound effects from firing during resends. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| public final class ChatResendDetector { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private static final long BURST_WINDOW_MS = 100; // 100ms window for detecting rapid packet bursts | ||||||||||||||||||||||
| private static final int BURST_THRESHOLD = 6; // 6+ packets in 100ms = resend pattern detected | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private static final Map<String, BurstTracker> trackers = new ConcurrentHashMap<>(); | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Invisible unicode characters commonly used to prevent chat condensing | ||||||||||||||||||||||
| // Includes: Zero Width Space, Zero Width Non-Joiner, Zero Width Joiner, | ||||||||||||||||||||||
| // Word Joiner, Zero Width No-Break Space (BOM), Mongolian Vowel Separator | ||||||||||||||||||||||
|
Comment on lines
+23
to
+24
|
||||||||||||||||||||||
| // Includes: Zero Width Space, Zero Width Non-Joiner, Zero Width Joiner, | |
| // Word Joiner, Zero Width No-Break Space (BOM), Mongolian Vowel Separator | |
| // Includes (but is not limited to): Zero Width Space, Zero Width Non-Joiner, Zero Width Joiner, | |
| // Word Joiner, Zero Width No-Break Space (BOM), Mongolian Vowel Separator, and other invisible or formatting Unicode characters. |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete documentation: The Javadoc comment for endBurst mentions "testing or manual intervention" but doesn't warn about the memory leak implications if this method is not called in production. Given that this is the only way to clean up tracker entries, the documentation should clarify when/how this should be called in production environments (e.g., on player disconnect).
| * Forcefully ends burst tracking for testing or manual intervention. | |
| * Forcefully ends burst tracking for the given identifier. | |
| * <p> | |
| * <b>Warning:</b> If this method is not called when a player (or other identifier) disconnects or is no longer being tracked, | |
| * the internal tracker map will retain entries indefinitely, leading to a memory leak. | |
| * <p> | |
| * This method <b>must</b> be called in production environments (e.g., on player disconnect) to clean up tracker entries. | |
| * While it can also be used for testing or manual intervention, proper cleanup is required to prevent unbounded memory growth. | |
| * | |
| * @param identifier Unique identifier (usually player UUID) |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition: The window expiration check and state reset are not atomic. Between checking if the window has expired (line 97) and resetting the state (lines 98-100), another thread could call isInBurst and read stale values. This could cause inconsistent behavior where a burst is reported as active when it should have expired. Consider synchronizing both addPacket and isInBurst methods.
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition: The window expiration check and state reset are not atomic. Between checking the window expiration (line 115) and resetting the state (lines 116-117), another thread could call addPacket and modify the count or windowStart. This could lead to inconsistent state where inBurst is reset but count remains non-zero, or vice versa. Consider using synchronized methods or AtomicReference to wrap the BurstTracker state.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,88 @@ | ||||||
| package to.itsme.itsmyconfig.util; | ||||||
|
|
||||||
| import org.junit.jupiter.api.Test; | ||||||
| import static org.junit.jupiter.api.Assertions.*; | ||||||
|
|
||||||
| public class ChatResendDetectorTest { | ||||||
|
|
||||||
| @Test | ||||||
| public void testNoBurstOnSinglePacket() { | ||||||
| final String testId = "test-player-single"; | ||||||
|
|
||||||
| // Clean up any existing state | ||||||
| ChatResendDetector.endBurst(testId); | ||||||
|
|
||||||
| // Single packet should not trigger burst | ||||||
| ChatResendDetector.recordPacket(testId); | ||||||
| assertFalse(ChatResendDetector.isInBurst(testId)); | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| public void testNoBurstOnFewPackets() { | ||||||
| final String testId = "test-player-few"; | ||||||
|
|
||||||
| // Clean up any existing state | ||||||
| ChatResendDetector.endBurst(testId); | ||||||
|
|
||||||
| // Few packets (under threshold of 6) should not trigger burst | ||||||
| for (int i = 0; i < 4; i++) { | ||||||
| ChatResendDetector.recordPacket(testId); | ||||||
| assertFalse(ChatResendDetector.isInBurst(testId)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| public void testBurstOnManyRapidPackets() { | ||||||
| final String testId = "test-player-burst"; | ||||||
|
|
||||||
| // Clean up any existing state | ||||||
| ChatResendDetector.endBurst(testId); | ||||||
|
|
||||||
| // 6+ packets in rapid succession should trigger burst | ||||||
| for (int i = 0; i < 5; i++) { | ||||||
| ChatResendDetector.recordPacket(testId); | ||||||
| assertFalse(ChatResendDetector.isInBurst(testId)); // 1-5 | ||||||
| } | ||||||
| ChatResendDetector.recordPacket(testId); // 6th | ||||||
| assertTrue(ChatResendDetector.isInBurst(testId)); // Should now be in burst | ||||||
| } | ||||||
|
|
||||||
| @Test | ||||||
| public void testBurstResetAfterDelay() throws InterruptedException { | ||||||
| final String testId = "test-player-reset"; | ||||||
|
|
||||||
| // Clean up any existing state | ||||||
| ChatResendDetector.endBurst(testId); | ||||||
|
|
||||||
| // Send rapid packets to trigger burst | ||||||
| for (int i = 0; i < 6; i++) { | ||||||
| ChatResendDetector.recordPacket(testId); | ||||||
| } | ||||||
| assertTrue(ChatResendDetector.isInBurst(testId)); | ||||||
|
|
||||||
| // Wait for window to expire | ||||||
| Thread.sleep(120); // Longer than 100ms threshold | ||||||
|
||||||
| Thread.sleep(120); // Longer than 100ms threshold | |
| Thread.sleep(200); // Increased buffer to avoid flakiness (threshold is 100ms) |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for containsInvisibleUnicode method: The tests only cover the timing-based burst detection logic but don't test the invisible unicode detection feature, which is a key part of the chat clear detection mechanism mentioned in the PR description. Add tests that verify containsInvisibleUnicode returns true for messages with characters like Zero Width Space (U+200B), Zero Width Non-Joiner (U+200C), etc., and false for normal messages.
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for isChatClearPacket method: This public method combines burst detection with invisible unicode detection and is used by the listeners, but there are no tests covering it. Add tests to verify it correctly returns true when either condition is met (burst OR invisible unicode), and false when neither is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect packet recording: The detector records packets before checking if the processor exists (line 66-68) and before verifying the packet type is actually a chat packet. This means ALL server packets (not just chat-related ones) are recorded, and DISCONNECT packets (line 34) are also recorded. Disconnect packets should not contribute to chat resend detection. Move the recordPacket call after confirming it's a chat-related packet.