From fbdd647168c86031d64db4eb1ebb7b5ee494afb4 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 01:36:54 +0000 Subject: [PATCH 1/9] Keep instances assignable after a failed assign or create assignInstance sets instance.assigned_host to the new host before sending InstanceAssignInternalRequest. If the host fails that request the controller kept the new host id while the host never added the instance. Assigning to the same host again then returned early as a no-op. Repro: make the host's instances directory read-only, assign instance 3001 to host 10 (EACCES on mkdir), make it writable, assign again (ok, nothing happens), start (instance does not exist on host). Now a failed send leaves the instance unassigned and rethrows. InstanceCreateRequest applied instance.assigned_host from the request config, creating an instance assigned to a host that never heard of it. Repro: InstanceCreateRequest({"instance.id": 4001, "instance.name": "c1", "instance.assigned_host": 10}). Reject a non-null value with the same error the config set requests use. Co-Authored-By: Claude Opus 5 (1M context) --- packages/controller/src/ControlConnection.ts | 3 ++ packages/controller/src/InstanceManager.ts | 14 +++++-- test/controller/ControlConnection.test.js | 39 +++++++++++++++++++- test/controller/InstanceManager.test.js | 39 ++++++++++++++++++++ 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index b502429b..f07eda68 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -288,6 +288,9 @@ export default class ControlConnection extends BaseConnection { } async handleInstanceCreateRequest(request: lib.InstanceCreateRequest) { + if ((request.config["instance.assigned_host"] ?? null) !== null) { + throw new lib.RequestError("instance.assigned_host must be set through the assign-host interface"); + } const instanceConfig = new lib.InstanceConfig("controller"); if (request.cloneFromId) { const baseInstance = this._controller.instances.get(request.cloneFromId); diff --git a/packages/controller/src/InstanceManager.ts b/packages/controller/src/InstanceManager.ts index 0736c493..adc3d7d2 100644 --- a/packages/controller/src/InstanceManager.ts +++ b/packages/controller/src/InstanceManager.ts @@ -208,9 +208,17 @@ export default class InstanceManager { instance.config.set("instance.assigned_host", hostId); if (newHostConnection) { - await newHostConnection.send( - new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")), - ); + try { + await newHostConnection.send( + new lib.InstanceAssignInternalRequest(instanceId, instance.config.toRemote("host")), + ); + } catch (err) { + // Leave unassigned so the assign can be retried + instance.config.set("instance.assigned_host", null); + instance.status = "unassigned"; + this.records.set(instance); + throw err; + } } else { instance.status = "unassigned"; } diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 2ad0db9d..3026e17e 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -1,5 +1,7 @@ import assert from "node:assert/strict"; -import { ControlConnection } from "@clusterio/controller"; +import * as lib from "@clusterio/lib"; +import { ControlConnection, InstanceManager } from "@clusterio/controller"; +import { MockController } from "../mock.js"; describe("controller/src/ControlConnection", function() { describe(".handleControllerRestartRequest()", function() { @@ -38,4 +40,39 @@ describe("controller/src/ControlConnection", function() { assert.equal(mockController.stopped, false); }); }); + + describe(".handleInstanceCreateRequest()", function() { + let mockController; + + beforeEach(function() { + mockController = new MockController(); + mockController.mockConfigEntries.set("controller.name", "Test"); + mockController.instances = new InstanceManager(new lib.SubscribableDatastore(), mockController); + }); + + async function create(config, cloneFromId) { + await ControlConnection.prototype.handleInstanceCreateRequest.call( + { _controller: mockController }, + new lib.InstanceCreateRequest(config, cloneFromId), + ); + } + + it("rejects instance.assigned_host in the config", async function() { + await assert.rejects( + create({ "instance.id": 4001, "instance.name": "c1", "instance.assigned_host": 10 }), + new lib.RequestError("instance.assigned_host must be set through the assign-host interface"), + ); + assert.equal(mockController.instances.has(4001), false); + }); + + it("creates an unassigned clone of an assigned instance", async function() { + await create({ "instance.id": 4001, "instance.name": "base" }); + mockController.instances.getMutable(4001).config.set("instance.assigned_host", 10); + + await create({ "instance.id": 4002, "instance.name": "clone", "instance.assigned_host": null }, 4001); + const clone = mockController.instances.get(4002); + assert.equal(clone.config.get("instance.name"), "clone"); + assert.equal(clone.config.get("instance.assigned_host"), null); + }); + }); }); diff --git a/test/controller/InstanceManager.test.js b/test/controller/InstanceManager.test.js index 59e05063..237b3dca 100644 --- a/test/controller/InstanceManager.test.js +++ b/test/controller/InstanceManager.test.js @@ -298,6 +298,45 @@ describe("controller/InstanceManager", function () { assert(assignRequest instanceof lib.InstanceAssignInternalRequest); assert.equal(assignRequest.instanceId, 1); }); + + it("should leave instance unassigned if the new host fails the assign", async function () { + let unassignRequest; + controller.wsServer.hostConnections.set(5, { + send: async (request) => { + unassignRequest = request; + }, + connector: { closing: false }, + }); + let failAssign = true; + let assignRequest; + controller.wsServer.hostConnections.set(6, { + send: async (request) => { + if (failAssign) { + throw new lib.RequestError("EACCES: permission denied"); + } + assignRequest = request; + }, + connector: { closing: false }, + }); + + instance.config.set("instance.assigned_host", 5); + instance.status = "stopped"; + await assert.rejects( + () => instances.assignInstance(1, 6), + { message: "EACCES: permission denied" } + ); + + assert(unassignRequest instanceof lib.InstanceUnassignInternalRequest); + const stored = instances.records.get(1); + assert.equal(stored.config.get("instance.assigned_host"), null); + assert.equal(stored.status, "unassigned"); + + failAssign = false; + await instances.assignInstance(1, 6); + assert.equal(instance.config.get("instance.assigned_host"), 6); + assert(assignRequest instanceof lib.InstanceAssignInternalRequest); + assert.equal(assignRequest.instanceId, 1); + }); }); describe(".unassignInstance()", function () { From 5b63fd27fea8f8d1e3ec359a1912107ab956b5d2 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 14:44:18 +0200 Subject: [PATCH 2/9] Fix listener leaks when a control connection closes (#1027) The close handler removed the log subscription transport from the global lib logger, but updateLogSubscriptions adds it to the controller's clusterLogger. Every control connection that subscribed to logs stayed attached to clusterLogger after closing, keeping the connection alive and running every later cluster log line through its filter. handleDebugDumpWsRequest added a new debugEvents listener on each call and overwrote ws_dumper, so the close handler only removed the last one. Repeated requests stacked listeners, triggered MaxListenersExceededWarning after 11 calls, and left the earlier listeners behind. Ignore the request if the connection is already dumping. Co-authored-by: Claude Opus 5 (1M context) --- packages/controller/src/ControlConnection.ts | 5 +- test/controller/ControlConnection.test.js | 55 +++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index b502429b..60f3e263 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -53,7 +53,7 @@ export default class ControlConnection extends BaseConnection { }); this.connector.on("close", () => { if (this.logTransport) { - logger.remove(this.logTransport); + this._controller.clusterLogger.remove(this.logTransport); this.logTransport = null; } if (this.ws_dumper) { @@ -1325,6 +1325,9 @@ export default class ControlConnection extends BaseConnection { } async handleDebugDumpWsRequest(request: lib.DebugDumpWsRequest) { + if (this.ws_dumper) { + return; // Already dumping + } this.ws_dumper = data => { if (this.connector.connected) { this.send(new lib.DebugWsMessageEvent(data.direction, data.content)); diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 2ad0db9d..b97c5527 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -1,5 +1,10 @@ import assert from "node:assert/strict"; -import { ControlConnection } from "@clusterio/controller"; +import events from "node:events"; +import * as lib from "@clusterio/lib"; +import { ControlConnection, ControllerHooks } from "@clusterio/controller"; +import { MockConnector, MockLogger } from "../mock.js"; + +const addr = lib.Address.fromShorthand; describe("controller/src/ControlConnection", function() { describe(".handleControllerRestartRequest()", function() { @@ -38,4 +43,52 @@ describe("controller/src/ControlConnection", function() { assert.equal(mockController.stopped, false); }); }); + + describe("close cleanup", function() { + let connector; + let mockController; + let connection; + + beforeEach(function() { + connector = new MockConnector(addr("controller"), addr({ controlId: 1 })); + connector._socket = {}; + const transports = new Set(); + mockController = { + hooks: new ControllerHooks(new MockLogger()), + router: null, + _registeredRequests: new Map(), + _fallbackedRequests: new Map(), + _registeredEvents: new Map(), + _snoopedEvents: new Map(), + subscriptions: { unsubscribeLink() {} }, + clusterLogger: { + transports, + add(transport) { transports.add(transport); }, + remove(transport) { transports.delete(transport); }, + }, + debugEvents: new events.EventEmitter(), + sendRequestToHostByInstanceId() {}, + }; + connection = new ControlConnection({ version: "test" }, connector, mockController, {}, 1); + }); + + it("removes the log transport from clusterLogger", async function() { + await connection.handleLogSetSubscriptionsRequest( + new lib.LogSetSubscriptionsRequest(true, false, [], []) + ); + assert.equal(mockController.clusterLogger.transports.size, 1); + connector.emit("close"); + assert.equal(mockController.clusterLogger.transports.size, 0); + assert.equal(connection.logTransport, null); + }); + + it("removes the debug dump listener after repeated requests", async function() { + await connection.handleDebugDumpWsRequest(new lib.DebugDumpWsRequest()); + await connection.handleDebugDumpWsRequest(new lib.DebugDumpWsRequest()); + assert.equal(mockController.debugEvents.listenerCount("message"), 1); + assert.equal(connector._socket.clusterio_ignore_dump, true); + connector.emit("close"); + assert.equal(mockController.debugEvents.listenerCount("message"), 0); + }); + }); }); From fb71ee72826443d3f58f33207268dc1294a41fd9 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 01:49:23 +0000 Subject: [PATCH 3/9] Terminate control connections of deleted users ControlConnection keeps the User object it authenticated with and checks permissions against it. Deleting the user removed the record but left its sessions open, so a deleted user kept all its permissions until the WebSocket closed. Repro: create "victim" with role 0 and connect as it. Another admin sends UserDeleteRequest("victim"). Victim can still send UserCreateRequest("mallory") and UserUpdateRolesRequest("mallory", [0]). Terminate the deleted user's connections like revoke-token does. When a user deletes itself, defer terminating the requesting connection so the response is sent first. Co-Authored-By: Claude Opus 5 (1M context) --- packages/controller/src/ControlConnection.ts | 11 ++++ test/controller/ControlConnection.test.js | 55 +++++++++++++++++++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 60f3e263..79669e8d 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -1070,6 +1070,17 @@ export default class ControlConnection extends BaseConnection { } this._controller.users.deleteUser(user); + for (let controlConnection of this._controller.wsServer.controlConnections.values()) { + if (controlConnection.user.id !== user.id) { + continue; + } + if (controlConnection === this) { + // Let the response go out before dropping our own session + setImmediate(() => controlConnection.connector.terminate()); + } else { + controlConnection.connector.terminate(); + } + } if (user.isAdmin) { this._controller.sendTo("allInstances", new lib.InstanceAdminlistUpdateEvent(name, false)); diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index b97c5527..383eca96 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -2,7 +2,7 @@ import assert from "node:assert/strict"; import events from "node:events"; import * as lib from "@clusterio/lib"; import { ControlConnection, ControllerHooks } from "@clusterio/controller"; -import { MockConnector, MockLogger } from "../mock.js"; +import { MockConnector, MockController, MockLogger } from "../mock.js"; const addr = lib.Address.fromShorthand; @@ -91,4 +91,57 @@ describe("controller/src/ControlConnection", function() { assert.equal(mockController.debugEvents.listenerCount("message"), 0); }); }); + + describe(".handleUserDeleteRequest()", function() { + let mockController; + let nextId; + + beforeEach(function() { + mockController = new MockController(); + mockController.sendTo = () => {}; + mockController.wsServer.controlConnections = new Map(); + nextId = 1; + }); + + function connect(name) { + const connection = { + _controller: mockController, + user: mockController.users.getByName(name), + connector: { + terminated: false, + terminate() { this.terminated = true; }, + }, + }; + mockController.wsServer.controlConnections.set(nextId, connection); + nextId += 1; + return connection; + } + + async function deleteUser(connection, name) { + await ControlConnection.prototype.handleUserDeleteRequest.call( + connection, new lib.UserDeleteRequest(name) + ); + } + + it("terminates connections of the deleted user", async function() { + const admin = connect("test"); + const player1 = connect("player"); + const player2 = connect("player"); + + await deleteUser(admin, "player"); + assert.equal(mockController.users.getByName("player"), undefined); + assert.equal(player1.connector.terminated, true); + assert.equal(player2.connector.terminated, true); + assert.equal(admin.connector.terminated, false); + }); + + it("terminates its own connection after the handler returns", async function() { + const player = connect("player"); + + await deleteUser(player, "player"); + assert.equal(player.connector.terminated, false); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(player.connector.terminated, true); + }); + }); }); From 1da7c1da672cf739171e39a7a2c538a5da5a86d5 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 13:31:09 +0000 Subject: [PATCH 4/9] Terminate deleted user sessions immediately Drop the special case that deferred terminating the connection issuing the delete, so deleting yourself behaves the same as revoke-token and every session of the deleted user is closed before the handler returns. Set tokenValidAfter to the current time in createUser instead of 0. Deleting a user and recreating it under the same name left tokens issued before the deletion valid, as the check is iat < tokenValidAfter. Doing it in createUser covers every path that makes a user. A token signed in the same second as creation is still accepted, so create-admin and the other production paths sign after the user exists and are unaffected. The integration harness signed the control token at import time, before bootstrap create-admin created the user. Sign it in the root before hook instead and reuse it in link.js. Co-Authored-By: Claude Opus 5 (1M context) --- packages/controller/src/ControlConnection.ts | 8 +------- packages/controller/src/UserManager.ts | 4 +++- test/controller/ControlConnection.test.js | 4 +--- test/controller/UserManager.test.js | 17 +++++++++++++++++ test/integration/index.js | 4 +++- test/integration/link.js | 9 +++++---- 6 files changed, 30 insertions(+), 16 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 79669e8d..b37aa37c 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/packages/controller/src/ControlConnection.ts @@ -1071,13 +1071,7 @@ export default class ControlConnection extends BaseConnection { this._controller.users.deleteUser(user); for (let controlConnection of this._controller.wsServer.controlConnections.values()) { - if (controlConnection.user.id !== user.id) { - continue; - } - if (controlConnection === this) { - // Let the response go out before dropping our own session - setImmediate(() => controlConnection.connector.terminate()); - } else { + if (controlConnection.user.id === user.id) { controlConnection.connector.terminate(); } } diff --git a/packages/controller/src/UserManager.ts b/packages/controller/src/UserManager.ts index 60649e5f..9ef56d7c 100644 --- a/packages/controller/src/UserManager.ts +++ b/packages/controller/src/UserManager.ts @@ -119,7 +119,9 @@ export default class UserManager { roles.add(defaultRoleId); } - const user = new User(this.records, this._controllerRoles, 0, name, roles); + // Reject tokens issued before the user existed, e.g. for a previous user by the same name + const tokenValidAfter = Math.floor(Date.now() / 1000); + const user = new User(this.records, this._controllerRoles, tokenValidAfter, name, roles); this.records.set(user); return user; } diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index 383eca96..75832ca1 100644 --- a/test/controller/ControlConnection.test.js +++ b/test/controller/ControlConnection.test.js @@ -135,12 +135,10 @@ describe("controller/src/ControlConnection", function() { assert.equal(admin.connector.terminated, false); }); - it("terminates its own connection after the handler returns", async function() { + it("terminates its own connection when deleting itself", async function() { const player = connect("player"); await deleteUser(player, "player"); - assert.equal(player.connector.terminated, false); - await new Promise(resolve => setImmediate(resolve)); assert.equal(player.connector.terminated, true); }); }); diff --git a/test/controller/UserManager.test.js b/test/controller/UserManager.test.js index 5005875e..62ca5209 100644 --- a/test/controller/UserManager.test.js +++ b/test/controller/UserManager.test.js @@ -79,6 +79,23 @@ describe("controller/UserManager", function () { const user = userManager.createUser("Eve"); assert(user.roleIds.has(1)); }); + + it("should set tokenValidAfter to the current time", function () { + const before = Math.floor(Date.now() / 1000); + const user = userManager.createUser("Mallory"); + assert(user.tokenValidAfter >= before, "tokenValidAfter predates the call"); + assert(user.tokenValidAfter <= Math.floor(Date.now() / 1000), "tokenValidAfter is in the future"); + }); + + it("should invalidate tokens issued before a user of the same name was recreated", function () { + const user = userManager.createUser("Niaj"); + const oldTokenIat = user.tokenValidAfter - 60; + userManager.deleteUser(user); + + // WsServer.registerControl rejects the token when iat < tokenValidAfter + const recreated = userManager.createUser("Niaj"); + assert(oldTokenIat < recreated.tokenValidAfter, "token from before the deletion is still valid"); + }); }); describe(".getOrCreateUser()", function () { diff --git a/test/integration/index.js b/test/integration/index.js index c2816412..a1730c22 100644 --- a/test/integration/index.js +++ b/test/integration/index.js @@ -191,7 +191,8 @@ let haveFactorioInstall; const baseHostConfig = loadJSON("config-host.json"); export let url = "https://localhost:4443/"; -export let controlToken = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64")); +// Signed in the root before hook, as the user has to exist before the token is issued +export let controlToken; export let instancesDir = path.join("temp", "test", "instances"); export let modsDir = path.join("temp", "test", "mods"); export let databaseDir = path.join("temp", "test", "database"); @@ -373,6 +374,7 @@ before(async function() { console.log("Bootstrapping"); await execController("bootstrap create-admin test"); await execController("bootstrap create-ctl-config test"); + controlToken = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64")); controllerProcess = await spawnNode("controller:", "../../packages/controller run", /Started controller/); diff --git a/test/integration/link.js b/test/integration/link.js index d45a28ca..53b3c76b 100644 --- a/test/integration/link.js +++ b/test/integration/link.js @@ -2,10 +2,11 @@ import assert from "node:assert/strict"; import events from "node:events"; import jwt from "jsonwebtoken"; -import { TestControlConnector, TestHostConnector, TestControl, get, exec, url, slowTest } from "./index.js"; +import { + TestControlConnector, TestHostConnector, TestControl, get, exec, url, controlToken, slowTest, +} from "./index.js"; import { ConnectionClosed, ProtocolError, PolicyViolation, AuthenticationFailed } from "@clusterio/lib"; -let token = jwt.sign({ aud: "user", user: "test" }, Buffer.from("TestSecretDoNotUse", "base64")); let tokenHost = jwt.sign({ aud: "host", host: 0 }, Buffer.from("TestSecretDoNotUse", "base64")); describe("Integration of lib/link", function() { @@ -14,7 +15,7 @@ describe("Integration of lib/link", function() { let hostConnector; beforeEach(async function() { controlConnector = new TestControlConnector(url, 0.2); - controlConnector.token = token; + controlConnector.token = controlToken; control = new TestControl(controlConnector, false); hostConnector = new TestHostConnector(url, 0.2); hostConnector.token = tokenHost; @@ -100,7 +101,7 @@ describe("Integration of lib/link", function() { }); it("should refuse connection with incorrect audience", async function() { - hostConnector.token = token; + hostConnector.token = controlToken; await assert.rejects( hostConnector.connect(), new Error("Authentication failed: jwt audience invalid. expected: host or slave") From c9e8aeee5cd5f673d67da3c09607fa4d56ec7369 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 13:34:58 +0000 Subject: [PATCH 5/9] Handle SessionLost when deleting your own account Deleting your own account now terminates the session immediately, so the web UI received a SessionLost rejection and reported "Error deleting user" for a delete that had succeeded. Use the same branch revoke does. Co-Authored-By: Claude Opus 5 (1M context) --- packages/web_ui/src/components/UserViewPage.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/web_ui/src/components/UserViewPage.tsx b/packages/web_ui/src/components/UserViewPage.tsx index fd6b2848..4a4bd97f 100644 --- a/packages/web_ui/src/components/UserViewPage.tsx +++ b/packages/web_ui/src/components/UserViewPage.tsx @@ -146,6 +146,13 @@ export default function UserViewPage() { new lib.UserDeleteRequest(userName) ).then(() => { navigate("/users"); + }, err => { + if (err instanceof lib.SessionLost && userName === account.name) { + // Got kicked out after deleting our own account + navigate("/users"); + return; + } + throw err; }).catch(notifyErrorHandler("Error deleting user")); }} > From 558999faa82c6ce12db45295e10cf464c332e075 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 15:45:50 +0200 Subject: [PATCH 6/9] Parse the Map Settings field in the load scenario modal (#1032) The Map Settings block parsed values.mapGenSettings, so filling in only Map Settings failed with "undefined" is not valid JSON and the modal would not submit, while filling in both sent the map gen settings JSON to Factorio as --map-settings. Co-authored-by: Claude Opus 5 (1M context) --- packages/web_ui/src/components/LoadScenarioModal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web_ui/src/components/LoadScenarioModal.tsx b/packages/web_ui/src/components/LoadScenarioModal.tsx index b83edb6c..135d38e3 100644 --- a/packages/web_ui/src/components/LoadScenarioModal.tsx +++ b/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; From 8239a9650909b2c655d3d3fb81fa83263b35eb33 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 15:51:16 +0200 Subject: [PATCH 7/9] Deny instead of crashing when a control permission check throws (#1033) A permission function runs during ingress permission checking, before the request body is validated. UserBulkImportRequest.permission read the raw, unvalidated message.data and threw a bare Error for an unknown importType. That error is not a PermissionError, so _processMessage rethrew it to the connector "message" listener, which emitted "error" on the connector. Controller-side connectors have no "error" listener, so Node rethrew and handleUnhandledErrors exited the process. Repro: a control client with client-side validateSent disabled sends UserBulkImportRequest with importType "foo"; the client gets a normal RequestError response and the controller then logs a fatal uncaught exception and exits. Fix both layers: _processMessage now logs and drops an unexpected error from the permission check (control connections only, where a response was already sent) instead of rethrowing, and UserBulkImportRequest throws a PermissionError for an unknown importType so it is denied cleanly. A genuine PermissionError still denies silently as before. Co-authored-by: Claude Opus 4.8 --- packages/lib/src/data/messages_user.ts | 4 ++- packages/lib/src/link/link.ts | 18 +++++++---- test/lib/data/messages_user.test.js | 27 ++++++++++++++++ test/lib/link/link.js | 45 ++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 8 deletions(-) create mode 100644 test/lib/data/messages_user.test.js diff --git a/packages/lib/src/data/messages_user.ts b/packages/lib/src/data/messages_user.ts index 2de27c19..34040965 100644 --- a/packages/lib/src/data/messages_user.ts +++ b/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}`); } } } diff --git a/packages/lib/src/link/link.ts b/packages/lib/src/link/link.ts index 3fb2e085..e29131c7 100644 --- a/packages/lib/src/link/link.ts +++ b/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; } } diff --git a/test/lib/data/messages_user.test.js b/test/lib/data/messages_user.test.js new file mode 100644 index 00000000..79c7363d --- /dev/null +++ b/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"]); + }); + }); +}); diff --git a/test/lib/link/link.js b/test/lib/link/link.js index b6bec464..6519a08e 100644 --- a/test/lib/link/link.js +++ b/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, From fc73433ffbff4d42d6771cd4166dd1b2fae24e93 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 15:53:49 +0200 Subject: [PATCH 8/9] Stop host crash on malformed action log lines (#1034) _watchServerLogActions dereferenced two regex results with ! and threw on null. The name regex /^([^ ]+)/ returns null when a line classified as an action starts with a space, and the BAN reason regex /\. Reason: (.+)\.$/ returns null for a [BAN] line with no ". Reason: X." tail. The output listener runs synchronously off the stdout LineSplitter, so the throw becomes an uncaughtException and takes the whole host down with every instance on it. Repro: instance with enable_save_patching off and enable_script_commands on; a control client sends /sc print("2000-01-01 00:00:00 [BAN] evil"). The raw print reaches stdout, parses as a dated BAN action, and the reason deref throws. Skip the line when the name regex misses and default the ban reason to empty when the tail is absent. Apply the same name guard to _watchPlayerPromote. The autosave ![1] in server.ts is safe because its filter regex already guarantees the capture. Co-authored-by: Claude Opus 4.8 --- packages/host/src/Instance.ts | 18 ++++++++++++++---- test/host/Instance.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/packages/host/src/Instance.ts b/packages/host/src/Instance.ts index 3f78c67b..5a9eb35b 100644 --- a/packages/host/src/Instance.ts +++ b/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]); }); } diff --git a/test/host/Instance.js b/test/host/Instance.js index 73e11c1a..e42c02c2 100644 --- a/test/host/Instance.js +++ b/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 . 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", " ")); + }); + }); }); From d32d857eb31c677a5a1725cabd87eb7dd95c6406 Mon Sep 17 00:00:00 2001 From: Bastiaan <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:13:24 +0200 Subject: [PATCH 9/9] Re-apply debug dump ignore flag on control connection resume (#1035) handleDebugDumpWsRequest sets clusterio_ignore_dump on the current socket so the WsServer send wrapper does not echo the dumper's own frames back into debugEvents. The flag was only set on the connector "connect" event, but WsServerConnector.continue() installs a fresh socket and emits "resume", not "connect", so a resumed session lost the flag. After a resume the next outgoing frame was echoed to debugEvents, ws_dumper sent a DebugWsMessageEvent, the wrapper echoed that too, and so on: infinite synchronous recursion ending in a RangeError: Invalid string length from JSON.stringify in link.ts sendEvent. That uncaught error made handleUnhandledErrors exit the controller. Repro: admin sends DebugDumpWsRequest, the control connection drops and resumes (e.g. a network blip), controller crashes on resume. Set the flag on "resume" as well as "connect". Co-authored-by: Claude Opus 4.8 Co-authored-by: Cooldude2606 <25043174+Cooldude2606@users.noreply.github.com> --- packages/controller/src/ControlConnection.ts | 10 ++++-- test/controller/ControlConnection.test.js | 34 +++++++++++++++++++- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/packages/controller/src/ControlConnection.ts b/packages/controller/src/ControlConnection.ts index 60f3e263..3a3af9f4 100644 --- a/packages/controller/src/ControlConnection.ts +++ b/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); diff --git a/test/controller/ControlConnection.test.js b/test/controller/ControlConnection.test.js index b97c5527..d82f6cc9 100644 --- a/test/controller/ControlConnection.test.js +++ b/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, 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;