From 66f258c7d27d01767e9efce78b1e489f8aa17d6a Mon Sep 17 00:00:00 2001 From: tyrathalis Date: Wed, 29 Jul 2026 14:57:27 -0700 Subject: [PATCH] Rate limit and bound chat from remote clients Chat is accepted from a remote client before it has logged in and is rebroadcast to every peer, so a client sending it in a loop costs the host's CPU, everyone's bandwidth and everyone's scrollback at once. Nothing bounds the rate, and the length bound is fixed. Adds a per-client token bucket sized for a person typing (10 message burst, one refilled per second). Over-rate messages are dropped rather than queued, since queuing is what the sender wants. The existing LogSafe length bound on the rebroadcast becomes configurable at the same time. Both are policy rather than correctness, so both are tunable and both can be switched off outright: forge.net.chatBurst, forge.net.chatRefillMillis and forge.net.maxChatLength, alongside the existing forge.net.heartbeatTimeout. Zero or less on the burst disables rate limiting; zero or less on the length disables truncation. The defaults are a guess at "generous for a human" -- if they are wrong for how people actually use the lobby, they are one property away rather than a rebuild. Note that switching the cap off still strips control characters, since that is a correctness property of the rebroadcast rather than an anti-spam measure -- only the truncation is optional. Values are read per call rather than into static final fields, because Integer.getInteger in a static initialiser is fixed at class-load and surefire shares one JVM, so a test could not otherwise vary them. Deliberately not included: a general per-message rate limit covering all inbound traffic. That risks throttling legitimate high-rate game traffic, and there is no measured baseline here for what a busy turn sends -- guessing a number and stalling a real game is how this kind of change gets reverted. Portions authored with an AI assistant (Claude), reviewed by a human. Co-Authored-By: Claude Fable 5 --- .../java/forge/net/ChatRateLimitTest.java | 53 +++++++++++++++++++ .../gamemodes/net/server/FServerManager.java | 20 ++++++- .../gamemodes/net/server/RemoteClient.java | 40 ++++++++++++++ 3 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 forge-gui-desktop/src/test/java/forge/net/ChatRateLimitTest.java diff --git a/forge-gui-desktop/src/test/java/forge/net/ChatRateLimitTest.java b/forge-gui-desktop/src/test/java/forge/net/ChatRateLimitTest.java new file mode 100644 index 000000000000..f2cdc097e288 --- /dev/null +++ b/forge-gui-desktop/src/test/java/forge/net/ChatRateLimitTest.java @@ -0,0 +1,53 @@ +package forge.net; + +import forge.gamemodes.net.server.RemoteClient; +import org.testng.Assert; +import org.testng.annotations.Test; + +/** + * Chat is accepted before login and rebroadcast to every peer, so a client + * that sends it in a loop costs the host and everyone else. The limit is a + * token bucket sized for a person typing, and switchable off. + */ +public class ChatRateLimitTest { + + /** Far above any legitimate burst, low enough to fail fast. */ + private static final int DRAIN_CEILING = 10_000; + + @Test(timeOut = 30_000) + public void testChatIsRateLimitedAndRefills() throws Exception { + final RemoteClient client = new RemoteClient(null); + + // Bounded, not "while (allowChatMessage())". An unbounded drain loop + // terminates only if the limiter works, so with the limiter removed -- + // the case this exists to detect -- it spins forever and the run hangs + // instead of failing. A hang reads as "still running", which is worse. + int drained = 0; + while (drained < DRAIN_CEILING && client.allowChatMessage()) { + drained++; + } + Assert.assertTrue(drained > 0, "A burst should be allowed - people do type"); + Assert.assertTrue(drained < DRAIN_CEILING, + "Bucket never emptied after " + DRAIN_CEILING + " messages; not rate limiting"); + + // One token per refill interval, so this cannot be shortened much. + Thread.sleep(1100); + Assert.assertTrue(client.allowChatMessage(), + "Allowance must come back - a rate limit that never refills is a mute"); + } + + @Test(timeOut = 30_000) + public void testLimitCanBeSwitchedOff() { + System.setProperty("forge.net.chatBurst", "0"); + try { + final RemoteClient client = new RemoteClient(null); + for (int i = 0; i < DRAIN_CEILING; i++) { + Assert.assertTrue(client.allowChatMessage(), + "Disabled limit must never refuse (refused at " + i + ")"); + } + } finally { + System.clearProperty("forge.net.chatBurst"); + } + } + +} diff --git a/forge-gui/src/main/java/forge/gamemodes/net/server/FServerManager.java b/forge-gui/src/main/java/forge/gamemodes/net/server/FServerManager.java index 3642e6c9ee9e..224eaad16b47 100644 --- a/forge-gui/src/main/java/forge/gamemodes/net/server/FServerManager.java +++ b/forge-gui/src/main/java/forge/gamemodes/net/server/FServerManager.java @@ -71,6 +71,15 @@ public final class FServerManager implements IHasForgeLog { static final int HEARTBEAT_TIMEOUT_SECONDS = Integer.getInteger("forge.net.heartbeatTimeout", 45); + /** + * Chat is rebroadcast to every peer, so one very long message costs + * everyone. Read per call for the same reason as the burst in + * {@link RemoteClient}; zero or less switches truncation off. + */ + private static int maxChatLength() { + return Integer.getInteger("forge.net.maxChatLength", 512); + } + private static final int OUTBOUND_BUFFER_LOW_WATER = 64 * 1024; private static final int OUTBOUND_BUFFER_HIGH_WATER = 1024 * 1024; private static final int RECONNECT_TIMEOUT_SECONDS = 300; @@ -1008,10 +1017,17 @@ public final void channelRead(final ChannelHandlerContext ctx, final Object msg) if (client == null) { return; } + if (!client.allowChatMessage()) { + return; // Over its rate; dropped rather than queued + } // Strip control characters before echoing to other players: a // carriage return lets one player paint fake system lines in - // everyone else's chat pane. - final String text = LogSafe.forDisplay(raw); + // everyone else's chat pane. Stripping is unconditional; only + // the length cap is policy, so zero or less strips without + // truncating rather than skipping the scrub. + final String text = maxChatLength() > 0 + ? LogSafe.forDisplay(raw, maxChatLength()) + : LogSafe.forDisplay(raw, Integer.MAX_VALUE); String username = client.getUsername(); // Append (Host) indicator for the host player if (client.getIndex() == 0) { diff --git a/forge-gui/src/main/java/forge/gamemodes/net/server/RemoteClient.java b/forge-gui/src/main/java/forge/gamemodes/net/server/RemoteClient.java index afc860a9457d..87e7315ea857 100644 --- a/forge-gui/src/main/java/forge/gamemodes/net/server/RemoteClient.java +++ b/forge-gui/src/main/java/forge/gamemodes/net/server/RemoteClient.java @@ -18,8 +18,25 @@ public final class RemoteClient implements IToClient, IHasForgeLog { /** Special value indicating the client hasn't been assigned a slot yet. */ public static final int UNASSIGNED_SLOT = -1; + /** + * Token-bucket allowance for chat, sized for a person typing rather than a + * script holding the key down. Read per call rather than into static final + * fields, because Integer.getInteger in a static initialiser is fixed at + * class-load and surefire shares one JVM, so a test could not vary them. + * A burst of zero or less switches the limit off. + */ + private static int chatBurst() { + return Integer.getInteger("forge.net.chatBurst", 10); + } + + private static int chatRefillMillis() { + return Integer.getInteger("forge.net.chatRefillMillis", 1000); + } + private volatile Channel channel; private String username; + private double chatTokens = chatBurst(); + private long chatLastRefill = System.currentTimeMillis(); private int index = UNASSIGNED_SLOT; private boolean libgdx; private volatile ReplyPool replies = new ReplyPool(); @@ -67,6 +84,29 @@ public SocketAddress getRemoteAddress() { return ch == null ? null : ch.remoteAddress(); } + /** + * Consume one chat allowance; false when the peer is over its rate. + * Always true when the limit is switched off. + */ + public synchronized boolean allowChatMessage() { + final int burst = chatBurst(); + if (burst <= 0) { + return true; + } + final long refill = Math.max(1, chatRefillMillis()); + final long now = System.currentTimeMillis(); + final long elapsed = now - chatLastRefill; + if (elapsed > 0) { + chatTokens = Math.min(burst, chatTokens + (double) elapsed / refill); + chatLastRefill = now; + } + if (chatTokens < 1) { + return false; + } + chatTokens -= 1; + return true; + } + /** Encodes synchronously on the caller's thread. Returns null on failure (logged). */ private ByteBuf encodeOnCallingThread(final NetEvent event) { final Channel ch = channel;