diff --git a/src/client/java/dev/creesch/WebInterface.java b/src/client/java/dev/creesch/WebInterface.java index 21a1793..35329f0 100644 --- a/src/client/java/dev/creesch/WebInterface.java +++ b/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,6 +175,10 @@ 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 {}", @@ -178,10 +186,6 @@ private void handleReceivedMessages(WsMessageContext ctx) { ); 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); diff --git a/src/client/java/dev/creesch/model/WebsocketMessageBuilder.java b/src/client/java/dev/creesch/model/WebsocketMessageBuilder.java index 64ef76f..a81615d 100644 --- a/src/client/java/dev/creesch/model/WebsocketMessageBuilder.java +++ b/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>() {}.getType(); Map translations = gson.fromJson( translationsJson, diff --git a/src/client/resources/web/js/managers/modal_manager.mjs b/src/client/resources/web/js/managers/modal_manager.mjs index e7f32df..6dfd8a6 100644 --- a/src/client/resources/web/js/managers/modal_manager.mjs +++ b/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); diff --git a/src/client/resources/web/js/managers/tab_list_manager.mjs b/src/client/resources/web/js/managers/tab_list_manager.mjs index e5ce81b..073d816 100644 --- a/src/client/resources/web/js/managers/tab_list_manager.mjs +++ b/src/client/resources/web/js/managers/tab_list_manager.mjs @@ -154,7 +154,17 @@ 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) { @@ -162,37 +172,30 @@ class TabListManager { } 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);