Bugfix/review findings

src/client/java/dev/creesch/WebInterface.java

@@ -10,6 +10,8 @@
 import dev.creesch.util.LocalNetworkAddressResolver;
 import dev.creesch.util.NamedLogger;
 import io.javalin.Javalin;
+import io.javalin.http.BadRequestResponse;
+import io.javalin.http.UnauthorizedResponse;
 import io.javalin.http.staticfiles.Location;
 import io.javalin.websocket.WsCloseStatus;
 import io.javalin.websocket.WsContext;
@@ -49,6 +51,11 @@ public class WebInterface {
         "[\\n\\r§\u00A7\\u0000-\\u001F\\u200B-\\u200F\\u2028-\\u202F]"
     );
     private static final Pattern MULTIPLE_SPACES = Pattern.compile("\\s{2,}");
+    // Hosts a browser can reach a loopback-bound server through: "localhost" or an IP literal.
+    private static final Pattern LOOPBACK_HOST = Pattern.compile(
+        "^(localhost|127\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}|\\[::1\\])(:\\d+)?$",
+        Pattern.CASE_INSENSITIVE
+    );
     private static final Pattern SUPPORTED_COMMANDS = Pattern.compile(
         "^/(msg|tell|w|me)(\\s.*|$)",
         Pattern.CASE_INSENSITIVE
@@ -121,16 +128,14 @@ private Javalin createServer() {
                     LOGGER.warn(
                         "Unauthorized attempt to access subdirectory: " + uri
                     );
-                    ctx.status(401).result("Unauthorized access");
-                    return;
+                    throw new UnauthorizedResponse("Unauthorized access");
                 }
 
                 // Reject requests containing `..` (path traversal attack)
                 // Javelin also does this, this is just to be extra secure
                 if (uri.contains("..")) {
                     LOGGER.warn("Invalid path detected: " + uri);
-                    ctx.status(400).result("Invalid path");
-                    return;
+                    throw new BadRequestResponse("Invalid path");
                 }
 
                 // Security headers
@@ -158,7 +163,6 @@ private Javalin createServer() {
     }
 
     private void handleReceivedMessages(WsMessageContext ctx) {
-        LOGGER.info(ctx.message());
         // Parse received message from json
         IncomingWebsocketJsonMessage receivedMessage = gson.fromJson(
             ctx.message(),
@@ -171,17 +175,17 @@ private void handleReceivedMessages(WsMessageContext ctx) {
                     receivedMessage.getPayload(),
                     String.class
                 );
+                LOGGER.info("Received WebSocket message: {}", message);
+
+                // Sanitize the message
+                message = sanitizeMessage(message);
                 if (message.trim().isEmpty()) {
                     LOGGER.warn(
                         "Received an empty message from {}",
                         ctx.session.getRemoteSocketAddress()
                     );
                     return;
                 }
-                LOGGER.info("Received WebSocket message: {}", message);
-
-                // Sanitize the message
-                message = sanitizeMessage(message);
 
                 // Send the sanitized message to Minecraft chat
                 sendMinecraftMessage(message);
@@ -192,6 +196,14 @@ private void handleReceivedMessages(WsMessageContext ctx) {
                     HistoryPayload.class
                 );
                 int requestedLimit = historyPayload.getLimit();
+                if (requestedLimit <= 0) {
+                    LOGGER.warn(
+                        "Ignoring history request with invalid limit {} from {}",
+                        requestedLimit,
+                        ctx.session.getRemoteSocketAddress()
+                    );
+                    return;
+                }
                 int moreHistoryRequestedLimit = requestedLimit + 1; // Used further down to determine if there are more messages available in history.
                 LOGGER.info(
                     "Received history request: {}",
@@ -303,11 +315,16 @@ private void setupWebSocket() {
                 // So on connect make sure the list is send immediatly.
                 WebsocketJsonMessage playerListMessage =
                     WebsocketMessageBuilder.createPlayerList(client);
-                String jsonPlayerListMessage = gson.toJson(playerListMessage);
+                String jsonPlayerListMessage =
+                    playerListMessage == null
+                        ? null
+                        : gson.toJson(playerListMessage);
 
                 try {
                     ctx.send(jsonJoinMessage);
-                    ctx.send(jsonPlayerListMessage);
+                    if (jsonPlayerListMessage != null) {
+                        ctx.send(jsonPlayerListMessage);
+                    }
                 } catch (Exception e) {
                     LOGGER.info(jsonJoinMessage);
                     LOGGER.info(jsonPlayerListMessage);
@@ -329,7 +346,18 @@ private void setupWebSocket() {
                 removeConnection(ctx);
             });
 
-            ws.onMessage((ctx) -> handleReceivedMessages(ctx));
+            ws.onMessage((ctx) -> {
+                // Client input is untrusted. An uncaught exception here makes Javalin close the session.
+                try {
+                    handleReceivedMessages(ctx);
+                } catch (Exception e) {
+                    LOGGER.warn(
+                        "Ignoring malformed WebSocket message from {}",
+                        ctx.session.getRemoteSocketAddress(),
+                        e
+                    );
+                }
+            });
 
             ws.onError((ctx) -> {
                 // If a shutdown is Initiated it is expected that there will be jetty related errors.
@@ -355,12 +383,16 @@ private void setupWebSocket() {
      * any website open in the user's browser could connect and chat as the player
      * (cross-site WebSocket hijacking). The chat page is served by this same server,
      * so a legitimate browser origin always matches the Host header of the upgrade request.
+     * When bound to loopback the Host must also be localhost itself: a DNS name that resolves
+     * to 127.0.0.1 (DNS rebinding) would otherwise make a foreign origin match its own Host.
+     * With LAN access enabled users may reach the server by any hostname, so only the
+     * Origin/Host comparison applies there.
      * Requests without an Origin header (non-browser clients) are allowed.
      *
      * @param ctx The WebSocket context of the new connection.
      * @return True if the connection is allowed.
      */
-    private static boolean isAllowedOrigin(WsContext ctx) {
+    private boolean isAllowedOrigin(WsContext ctx) {
         String origin = ctx.header("Origin");
         if (origin == null || origin.isEmpty()) {
             return true;
@@ -370,6 +402,9 @@ private static boolean isAllowedOrigin(WsContext ctx) {
         if (host == null || host.isEmpty()) {
             return false;
         }
+        if (!lanEnabled && !LOOPBACK_HOST.matcher(host).matches()) {
+            return false;
+        }
 
         return (
             origin.equalsIgnoreCase("http://" + host) ||
@@ -483,7 +518,8 @@ private void sendMinecraftMessage(String message) {
     }
 
     public void broadcastMessage(WebsocketJsonMessage message) {
-        if (server == null || connections == null || connections.isEmpty()) {
+        // Builders return null when there is nothing to send (e.g. no connection yet).
+        if (message == null || server == null || connections.isEmpty()) {
             return;
         }
         String jsonMessage = gson.toJson(message);

src/client/java/dev/creesch/model/WebsocketMessageBuilder.java

@@ -202,7 +202,6 @@ public static WebsocketJsonMessage createHistoricChatMessage(
         String minecraftVersion
     ) {
         // Back to objects we go
-        LOGGER.info(translationsJson);
         Type type = new TypeToken<Map<String, String>>() {}.getType();
         Map<String, String> translations = gson.fromJson(
             translationsJson,

src/client/resources/web/js/managers/modal_manager.mjs

@@ -25,20 +25,24 @@ class ModalManager {
 
     /**
      * @param {HTMLDivElement} container - Full-screen backdrop element.
-     * @param {HTMLDivElement} content - Centered box that holds the contents.
      * @param {HTMLButtonElement} closeButton - The "X" button in the top right.
      * @param {HTMLDivElement} body - Element the caller's contents are placed in.
      */
-    constructor(container, content, closeButton, body) {
+    constructor(container, closeButton, body) {
         this.#container = container;
         this.#body = body;
         this.#closeButton = closeButton;
 
-        // Close on the "X", a backdrop click, or Escape. Clicks inside the
-        // content box must not bubble up to the backdrop and close the modal.
+        // Close on the "X", a backdrop click, or Escape. Only a click that
+        // targets the backdrop itself counts: a drag that starts inside the
+        // content box (e.g. selecting text) and ends over the backdrop
+        // dispatches its click on the container, and must not close it.
         closeButton.addEventListener('click', () => this.close());
-        this.#container.addEventListener('click', () => this.close());
-        content.addEventListener('click', (event) => event.stopPropagation());
+        this.#container.addEventListener('click', (event) => {
+            if (event.target === this.#container) {
+                this.close();
+            }
+        });
         document.addEventListener('keydown', (event) => {
             if (this.#isOpen && event.key === 'Escape') {
                 this.close();
@@ -73,9 +77,6 @@ class ModalManager {
 const container = /** @type {HTMLDivElement} */ (
     querySelectorWithAssertion('#modal-container')
 );
-const content = /** @type {HTMLDivElement} */ (
-    querySelectorWithAssertion('#modal-content')
-);
 const closeButton = /** @type {HTMLButtonElement} */ (
     querySelectorWithAssertion('#modal-close')
 );
@@ -84,9 +85,4 @@ const body = /** @type {HTMLDivElement} */ (
 );
 
 // Export a singleton instance since only one modal can be shown at a time.
-export const modalManager = new ModalManager(
-    container,
-    content,
-    closeButton,
-    body,
-);
+export const modalManager = new ModalManager(container, closeButton, body);

src/client/resources/web/js/managers/tab_list_manager.mjs

@@ -154,45 +154,48 @@ class TabListManager {
      * @param {PlayerInfo[]} matches
      */
     #populateList(matches) {
-        this.#players = matches;
+        // Keep only what is rendered so keyboard navigation cannot select
+        // a player that is not shown.
+        this.#players = matches
+            // Show names in alphabetical order.
+            .sort((a, b) =>
+                formatComponentToString(a.playerDisplayName).localeCompare(
+                    formatComponentToString(b.playerDisplayName),
+                ),
+            )
+            // Show only first 5 matches.
+            .slice(0, 5);
 
         const ul = tabListElement.querySelector('ul');
         if (!ul) {
             return;
         }
 
         ul.replaceChildren(
-            ...matches
-                // Show names in alphabetical order.
-                .sort((a, b) =>
-                    formatComponentToString(a.playerDisplayName).localeCompare(
-                        formatComponentToString(b.playerDisplayName),
-                    ),
-                )
-                // Show only first 5 matches.
-                .slice(0, 5)
-                .map((match, index) => {
-                    const li = document.createElement('li');
-                    // Using mousedown because clicking causes blur event on chat input hiding the selection.
-                    li.addEventListener('mousedown', () => {
-                        this.#insertPlayerName();
-                    });
-                    li.addEventListener('mouseenter', () => {
-                        this.#updateSelection(index);
-                    });
-
-                    const displayName = formatComponentToString(
-                        match.playerDisplayName,
-                    );
-                    const displayNameUnchanged =
-                        displayName.toLocaleLowerCase() ===
-                        match.playerName.toLocaleLowerCase();
-                    li.textContent = displayNameUnchanged
-                        ? displayName
-                        : `${displayName} (${match.playerName})`;
-
-                    return li;
-                }),
+            ...this.#players.map((match, index) => {
+                const li = document.createElement('li');
+                // Using mousedown because clicking causes blur event on chat input hiding the selection.
+                // preventDefault keeps focus in the chat input after the name is inserted.
+                li.addEventListener('mousedown', (e) => {
+                    e.preventDefault();
+                    this.#insertPlayerName();
+                });
+                li.addEventListener('mouseenter', () => {
+                    this.#updateSelection(index);
+                });
+
+                const displayName = formatComponentToString(
+                    match.playerDisplayName,
+                );
+                const displayNameUnchanged =
+                    displayName.toLocaleLowerCase() ===
+                    match.playerName.toLocaleLowerCase();
+                li.textContent = displayNameUnchanged
+                    ? displayName
+                    : `${displayName} (${match.playerName})`;
+
+                return li;
+            }),
         );
 
         this.#updateSelection(0);