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] 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); + }); + }); });