Merge branch 'master' into fix/terminate-deleted-user-sessions

packages/controller/src/ControlConnection.ts

@@ -48,9 +48,13 @@ export default class ControlConnection extends BaseConnection {
 
 		this._version = registerData.version;
 
-		this.connector.on("connect", () => {
-			this.connector._socket!.clusterio_ignore_dump = Boolean(this.ws_dumper);
-		});
+		// A resume installs a fresh socket, so re-apply the ignore flag or the
+		// dumper's own frames get echoed back into it and recurse forever.
+		for (let event of ["connect", "resume"] as const) {
+			this.connector.on(event, () => {
+				this.connector._socket!.clusterio_ignore_dump = Boolean(this.ws_dumper);
+			});
+		}
 		this.connector.on("close", () => {
 			if (this.logTransport) {
 				this._controller.clusterLogger.remove(this.logTransport);

packages/host/src/Instance.ts

@@ -306,7 +306,12 @@ export default class Instance extends lib.Link {
 				return;
 			}
 
-			let name = /^([^ ]+)/.exec(parsed.message)![1];
+			// Guard against lines that classify as actions but lack a name
+			let nameMatch = /^([^ ]+)/.exec(parsed.message);
+			if (!nameMatch) {
+				return;
+			}
+			let name = nameMatch[1];
 
 			// Detect player leave and join
 			if (parsed.action === "JOIN") {
@@ -322,7 +327,9 @@ export default class Instance extends lib.Link {
 
 			// Detect banned and admin state change, whitelist changes are not logged
 			if (parsed.action === "BAN") {
-				const reason = /\. Reason: (.+)\.$/.exec(parsed.message)![1];
+				// Ban lines without a ". Reason: X." tail default to no reason
+				const reasonMatch = /\. Reason: (.+)\.$/.exec(parsed.message);
+				const reason = reasonMatch ? reasonMatch[1] : "";
 				this._recordUserUpdate(parsed.action, name, reason !== "unspecified" ? reason : "");
 			} else if (["UNBANNED", "PROMOTE", "DEMOTE"].includes(parsed.action)) {
 				this._recordUserUpdate(parsed.action as "UNBANNED" | "PROMOTE" | "DEMOTE", name);
@@ -348,8 +355,11 @@ export default class Instance extends lib.Link {
 				return;
 			}
 
-			const name = /^([^ ]+)/.exec(parsed.message)![1];
-			this._recordUserUpdate(parsed.action as "PROMOTE" | "DEMOTE", name);
+			const nameMatch = /^([^ ]+)/.exec(parsed.message);
+			if (!nameMatch) {
+				return;
+			}
+			this._recordUserUpdate(parsed.action as "PROMOTE" | "DEMOTE", nameMatch[1]);
 		});
 	}
 

packages/lib/src/data/messages_user.ts

@@ -2,6 +2,7 @@ import { Type, Static } from "@sinclair/typebox";
 import UserDetails, { IUser } from "./UserDetails.js";
 import { StringEnum, jsonArray, plainJson } from "./composites.js";
 import { MessageRequest } from "./messages_core.js";
+import { PermissionError } from "../errors.js";
 
 export class UserGetRequest {
 	declare ["constructor"]: typeof UserGetRequest;
@@ -276,8 +277,9 @@ export class UserBulkImportRequest {
 					user.checkPermission("core.user.set_whitelisted");
 					break;
 				default:
+					// Unvalidated input during permission check: deny, don't throw a bare Error
 					// @ts-expect-error Unreachable
-					throw new Error(`Unknown import / restore type: ${data.importType}`);
+					throw new PermissionError(`Unknown import / restore type: ${data.importType}`);
 			}
 		}
 	}

packages/lib/src/link/link.ts

@@ -196,16 +196,20 @@ export class Link {
 
 		let entry = this._validateMessage(message);
 		if (entry && this.connector.dst.type === libData.Address.control) {
+			let permissionMessage = message as libData.MessageRequest | libData.MessageEvent;
 			try {
-				this.validatePermission(
-					message as libData.MessageRequest | libData.MessageEvent,
-					entry
-				); // Somewhat hacky, defined in ControlConnection
+				this.validatePermission(permissionMessage, entry); // Somewhat hacky, defined in ControlConnection
 			} catch (err) {
-				if (err instanceof libErrors.PermissionError) {
-					return;
+				// A non-PermissionError means the permission check itself is
+				// broken.  validatePermission (control connections only) already
+				// sent a response error, so log and drop instead of crashing.
+				if (!(err instanceof libErrors.PermissionError)) {
+					logger.error(
+						`Unexpected error checking permission for ${permissionMessage.name}:\n` +
+						`${(err as Error).stack ?? (err as Error).message}`
+					);
 				}
-				throw err;
+				return;
 			}
 		}
 

packages/web_ui/src/components/LoadScenarioModal.tsx

@@ -44,7 +44,7 @@ export default function LoadScenarioModal(props: LoadScenarioModalProps) {
 		}
 		if (values.mapSettings && values.mapSettings.trim()) {
 			try {
-				mapSettings = JSON.parse(values.mapGenSettings);
+				mapSettings = JSON.parse(values.mapSettings);
 			} catch (err: any) {
 				form.setFields([{ name: "mapSettings", errors: [err.message] }]);
 				return;

test/controller/ControlConnection.test.js

@@ -1,12 +1,44 @@
 import assert from "node:assert/strict";
 import events from "node:events";
 import * as lib from "@clusterio/lib";
-import { ControlConnection, ControllerHooks } from "@clusterio/controller";
+import { Controller, ControlConnection, ControllerHooks } from "@clusterio/controller";
 import { MockConnector, MockController, MockLogger } from "../mock.js";
 
 const addr = lib.Address.fromShorthand;
 
 describe("controller/src/ControlConnection", function() {
+	describe(".handleDebugDumpWsRequest()", function() {
+		function makeConnection() {
+			const controllerConfig = new lib.ControllerConfig("controller");
+			const connector = new lib.VirtualConnector(
+				lib.Address.fromShorthand("controller"),
+				lib.Address.fromShorthand({ controlId: 1 }),
+			);
+			connector._socket = {};
+			const controller = new Controller(lib.logger, [], controllerConfig);
+			const user = controller.users.getOrCreateUser("test");
+			return new ControlConnection({ version: "2.0.0" }, connector, controller, user, 1);
+		}
+
+		it("re-applies clusterio_ignore_dump to the socket on resume", async function() {
+			const connection = makeConnection();
+			await connection.handleDebugDumpWsRequest(new lib.DebugDumpWsRequest());
+			// A resume installs a fresh socket without the flag.
+			const resumedSocket = {};
+			connection.connector._socket = resumedSocket;
+			connection.connector.emit("resume");
+			assert.equal(resumedSocket.clusterio_ignore_dump, true);
+		});
+
+		it("leaves the flag off on resume without a dumper", function() {
+			const connection = makeConnection();
+			const resumedSocket = {};
+			connection.connector._socket = resumedSocket;
+			connection.connector.emit("resume");
+			assert.equal(resumedSocket.clusterio_ignore_dump, false);
+		});
+	});
+
 	describe(".handleControllerRestartRequest()", function() {
 		let mockController;
 

test/host/Instance.js

@@ -328,4 +328,37 @@ describe("class Instance", function() {
 			}
 		});
 	});
+
+	describe("_watchServerLogActions()", function() {
+		beforeEach(function() {
+			instance.config.set("factorio.sync_banlist", "bidirectional");
+			instance._watchServerLogActions();
+		});
+		afterEach(function() {
+			clearInterval(instance._playerCheckInterval);
+		});
+		function emitAction(action, message) {
+			instance.server.emit("output", { type: "action", action, message }, "");
+		}
+		it("should record a normal JOIN action", function() {
+			emitAction("JOIN", "player joined the game");
+			assert(instance.playersOnline.has("player"), "player was not added");
+		});
+		it("should parse the reason of a normal BAN action", function() {
+			emitAction("BAN", "player was banned by <server>. Reason: griefing.");
+			assert.deepEqual(
+				connector.sentMessages[0],
+				new lib.MessageEvent(
+					1, src, addr("allInstances"), "InstanceBanlistUpdateEvent",
+					new lib.InstanceBanlistUpdateEvent("player", true, "griefing"),
+				),
+			);
+		});
+		it("should not throw on a BAN action without a reason tail", function() {
+			assert.doesNotThrow(() => emitAction("BAN", "evil"));
+		});
+		it("should not throw on an action whose message starts with a space", function() {
+			assert.doesNotThrow(() => emitAction("JOIN", " "));
+		});
+	});
 });

test/lib/data/messages_user.test.js

@@ -0,0 +1,27 @@
+import assert from "node:assert/strict";
+import * as lib from "@clusterio/lib";
+
+describe("lib/data/messages_user", function() {
+	describe("UserBulkImportRequest", function() {
+		// A user that grants every permission, so only the importType switch
+		// can reject the request.
+		function grantAllUser() {
+			const checked = [];
+			return { checked, checkPermission(permission) { checked.push(permission); } };
+		}
+
+		it("should deny an unknown importType with a PermissionError", function() {
+			const user = grantAllUser();
+			assert.throws(
+				() => lib.UserBulkImportRequest.permission(user, { data: { importType: "foo", users: [] } }),
+				lib.PermissionError,
+			);
+		});
+
+		it("should check permissions for a known importType", function() {
+			const user = grantAllUser();
+			lib.UserBulkImportRequest.permission(user, { data: { importType: "admins", users: [] } });
+			assert.deepEqual(user.checked, ["core.user.bulk_import", "core.user.set_admin"]);
+		});
+	});
+});

test/lib/link/link.js

@@ -539,6 +539,51 @@ describe("lib/link/link", function() {
 				);
 			});
 
+			describe("permission checking", function() {
+				// A control connection has connector.dst.type === control, which is
+				// what gates permission checking, see link.js _processMessage.
+				function controlSetup() {
+					const connector = new mock.MockConnector(dst, src);
+					const link = new lib.Link(connector);
+					const message = new lib.MessageRequest(1, src, dst, "SimpleRequest");
+					return { connector, link, message };
+				}
+
+				it("should silently deny on PermissionError from the permission check", function() {
+					const { connector, link, message } = controlSetup();
+					let handled = false;
+					let errored = false;
+					connector.on("error", () => { errored = true; });
+					link.handle(SimpleRequest, async () => { handled = true; });
+					link.validatePermission = () => { throw new lib.PermissionError("Denied"); };
+					connector.emit("message", message);
+					assert(!handled, "handler ran despite denial");
+					assert(!errored, "connector emitted error on denial");
+					assert.deepEqual(connector.sentMessages, [], "denial sent a message");
+				});
+
+				it("should not crash when the permission check throws a non-PermissionError", function() {
+					const { connector, link, message } = controlSetup();
+					let handled = false;
+					let errored = false;
+					const logged = [];
+					const originalError = lib.logger.error;
+					lib.logger.error = msg => { logged.push(msg); };
+					connector.on("error", () => { errored = true; });
+					try {
+						link.handle(SimpleRequest, async () => { handled = true; });
+						link.validatePermission = () => { throwSimple("check is broken"); };
+						connector.emit("message", message);
+					} finally {
+						lib.logger.error = originalError;
+					}
+					assert(!errored, "non-PermissionError escaped to the connector error path");
+					assert(!handled, "handler ran after the permission check threw");
+					assert.equal(logged.length, 1);
+					assert(logged[0].startsWith("Unexpected error checking permission for SimpleRequest:\n"));
+				});
+			});
+
 			describe("Broadcast handling", function() {
 				// A broadcast is addressed to every link of the type it targets,
 				// so a link it reaches has to pass it on as well as handle it,