server: improve parsing of websocket connection URL, and add tests (#1288)

* add unit tests for parsing websocket connection url

* add e2e test for websocket reconnects

* account for base_url and make parsing more robust

Author: dyc3

server/clientmanager.ts

@@ -18,7 +18,7 @@ import {
 import { ClientNotFoundInRoomException, MissingToken } from "./exceptions";
 import { MySession, OttWebsocketError, AuthToken, ClientId } from "../common/models/types";
 import roommanager from "./roommanager";
-import { ANNOUNCEMENT_CHANNEL } from "../common/constants";
+import { ANNOUNCEMENT_CHANNEL, ROOM_NAME_REGEX } from "../common/constants";
 import tokens, { SessionInfo } from "./auth/tokens";
 import { RoomStateSyncable } from "./room";
 import { Gauge } from "prom-client";
@@ -77,8 +77,12 @@ export function shutdown() {
  */
 async function onDirectConnect(socket: WebSocket, req: express.Request) {
 	try {
-		const connectUrl = new URL(`ws://${req.headers.host}${req.url}`);
-		const roomName = connectUrl.pathname.split("/").slice(-1)[0];
+		const roomName = parseWebsocketConnectionUrl(req);
+		if (!ROOM_NAME_REGEX.test(roomName)) {
+			log.warn("Rejecting connection because the room name was invalid");
+			socket.close(OttWebsocketError.INVALID_CONNECTION_URL, "Invalid room name");
+			return;
+		}
 		log.debug(`connection received: ${roomName}, waiting for auth token...`);
 		const client = new DirectClient(roomName, socket);
 		addClient(client);
@@ -88,6 +92,18 @@ async function onDirectConnect(socket: WebSocket, req: express.Request) {
 	}
 }
 
+/**
+ * Extract the room name from the websocket connection url.
+ * @returns Room name
+ */
+export function parseWebsocketConnectionUrl(req: express.Request): string {
+	const connectUrl = new URL(req.url, `ws://${req.headers.host ?? "localhost"}`);
+	const base_url = conf.get("base_url");
+	const adjustedPath = base_url ? connectUrl.pathname.replace(base_url, "") : connectUrl.pathname;
+	const roomName = adjustedPath.split("/").slice(3)[0];
+	return roomName;
+}
+
 export function addClient(client: Client) {
 	connections.push(client);
 	client.on("auth", onClientAuth);

server/tests/unit/clientmanager.spec.ts

@@ -1,4 +1,4 @@
-import clientmanager from "../../clientmanager";
+import clientmanager, { parseWebsocketConnectionUrl } from "../../clientmanager";
 import {
 	BalancerConnection,
 	BalancerConnectionEventHandlers,
@@ -13,6 +13,8 @@ import { buildClients } from "../../redisclient";
 import { Result, ok } from "../../../common/result";
 import roommanager from "../../roommanager";
 import { loadModels } from "../../models";
+import type { Request } from "express";
+import { loadConfigFile, conf } from "../../ott-config";
 
 class TestClient extends Client {
 	sendRawMock = jest.fn();
@@ -50,6 +52,7 @@ class BalancerConnectionMock extends BalancerConnection {
 
 describe("ClientManager", () => {
 	beforeAll(async () => {
+		loadConfigFile();
 		loadModels();
 		await buildClients();
 		await clientmanager.setup();
@@ -66,6 +69,32 @@ describe("ClientManager", () => {
 		await roommanager.unloadRoom("foo");
 	});
 
+	it.each([
+		["/api/room/foo", "foo"],
+		["/api/room/foo/", "foo"],
+		["/api/room/foo/bar", "foo"],
+		["/api/room/foo?reconnect=true", "foo"],
+	])(`should parse room name from %s`, (path, roomName) => {
+		const got = parseWebsocketConnectionUrl({ url: path, headers: {} } as Request);
+		expect(got).toEqual(roomName);
+	});
+
+	it.each([
+		["/base", "/api/room/foo", "foo"],
+		["/base", "/api/room/foo/", "foo"],
+		["/base", "/api/room/foo/bar", "foo"],
+		["/base", "/api/room/foo?reconnect=true", "foo"],
+		["/base/base2", "/api/room/foo", "foo"],
+		["/base/base2", "/api/room/foo/", "foo"],
+		["/base/base2", "/api/room/foo/bar", "foo"],
+		["/base/base2", "/api/room/foo?reconnect=true", "foo"],
+	])(`should parse room name when base url is %s from %s`, (baseurl, path, roomName) => {
+		conf.set("base_url", baseurl);
+		const got = parseWebsocketConnectionUrl({ url: baseurl + path, headers: {} } as Request);
+		expect(got).toEqual(roomName);
+		conf.set("base_url", "/");
+	});
+
 	it("should add clients", () => {
 		const client = new TestClient("foo");
 		clientmanager.addClient(client);

tests/e2e/integration/ws-connection.spec.ts

@@ -0,0 +1,24 @@
+describe("Websocket connection", () => {
+	beforeEach(() => {
+		cy.ottEnsureToken();
+		cy.ottResetRateLimit();
+		cy.ottRequest({ method: "POST", url: "/api/room/generate" }).then(resp => {
+			// @ts-expect-error Cypress doesn't know how to respect this return type
+			cy.visit(`/room/${resp.body.room}`);
+		});
+	});
+
+	it("should connect to the websocket", () => {
+		cy.get("#connectStatus").should("contain", "Connected");
+	});
+
+	it("should connect to the websocket on reconnect", () => {
+		cy.get("#connectStatus").should("contain", "Connected");
+		cy.get("button").eq(0).focus(); // focus something so keyboard shortcuts work
+		cy.realPress(["Control", "Shift", "F12"]);
+		cy.get("button").contains("Disconnect Me").click();
+		cy.scrollTo("top");
+		cy.get("#connectStatus").should("contain", "Connecting");
+		cy.get("#connectStatus").should("contain", "Connected");
+	});
+})