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] 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,