From 5b769adcc5de46107bba5e97dc4c6425e3a420a5 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 02:39:12 +0000 Subject: [PATCH 1/3] Reject host save updates for foreign instances handleInstanceSaveDetailsUpdatesEvent trusted the instanceId in the event and in each SaveDetails without checking the instance was assigned to the sending host. InstanceSaveDetailsUpdatesEvent has host in its src list and validateIngress only checks the source host id, not the payload, so any authenticated host could corrupt another instance's controller-side save records. Repro: a host owning nothing sends the event with a SaveDetails for instance 1002 (on another host) to inject a bogus save, or sends empty updates with instanceId 1002 to make the delete-missing sweep wipe that instance's save list. Guard both paths: bail if event.instanceId names an instance not assigned to this host, and drop the whole event if any update is for an instance not assigned here. Matches handleInstanceStatusChangedEvent's bogus-update warning. Co-Authored-By: Claude Opus 4.8 --- packages/controller/src/HostConnection.ts | 17 +++++ test/controller/HostConnection.test.js | 78 +++++++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 test/controller/HostConnection.test.js diff --git a/packages/controller/src/HostConnection.ts b/packages/controller/src/HostConnection.ts index 3080c1be..ec2f55d9 100644 --- a/packages/controller/src/HostConnection.ts +++ b/packages/controller/src/HostConnection.ts @@ -296,6 +296,23 @@ export default class HostConnection extends BaseConnection { } async handleInstanceSaveDetailsUpdatesEvent(event: lib.InstanceSaveDetailsUpdatesEvent) { + // A host may only report saves for instances assigned to it. Otherwise + // any host could inject save records for, or wipe the save list of, an + // instance on a different host. See handleInstanceStatusChangedEvent. + const assignedHere = (instanceId: number) => { + const instance = this._controller.instances.get(instanceId); + return instance !== undefined && instance.config.get("instance.assigned_host") === this.id; + }; + + if (event.instanceId !== undefined && !assignedHere(event.instanceId)) { + logger.warn(`Got bogus save updates for instance id ${event.instanceId}`); + return; + } + if (event.updates.some(save => !assignedHere(save.instanceId))) { + logger.warn(`Got bogus save updates from host id ${this.id}`); + return; + } + const updates: lib.SaveDetails[] = []; const deletes: lib.SaveDetails[] = []; for (const save of event.updates) { diff --git a/test/controller/HostConnection.test.js b/test/controller/HostConnection.test.js new file mode 100644 index 00000000..fde949d9 --- /dev/null +++ b/test/controller/HostConnection.test.js @@ -0,0 +1,78 @@ +import assert from "node:assert/strict"; +import * as lib from "@clusterio/lib"; +import { HostConnection } from "@clusterio/controller"; + +function makeSave(instanceId, name) { + return new lib.SaveDetails(instanceId, "file", name, 0, 0, false, false, 0, false); +} + +// Mock saves datastore backed by a Map keyed on SaveDetails.id. +class MockSaves { + constructor(saves = []) { + this.saves = new Map(saves.map(s => [s.id, s])); + } + + get(id) { return this.saves.get(id); } + + values() { return this.saves.values(); } + + setMany(saves) { for (const s of saves) { this.saves.set(s.id, s); } } + + deleteMany(saves) { for (const s of saves) { this.saves.delete(s.id); } } +} + +// Mock instances manager where instance id -> assigned host id. +function makeInstances(assignments) { + return { + get(id) { + if (!assignments.has(id)) { + return undefined; + } + const host = assignments.get(id); + return { config: { get: name => (name === "instance.assigned_host" ? host : undefined) } }; + }, + }; +} + +describe("controller/src/HostConnection", function() { + describe(".handleInstanceSaveDetailsUpdatesEvent()", function() { + // Instance 1 is on host 10, instance 2 is on host 20. + const assignments = new Map([[1, 10], [2, 20]]); + + function run(hostId, saves, event) { + const saveStore = new MockSaves(saves); + const ctx = { + id: hostId, + _controller: { instances: makeInstances(assignments), saves: saveStore }, + }; + HostConnection.prototype.handleInstanceSaveDetailsUpdatesEvent.call(ctx, event); + return saveStore; + } + + it("applies updates for an instance assigned to this host", async function() { + const store = run(10, [], new lib.InstanceSaveDetailsUpdatesEvent([makeSave(1, "a.zip")], 1)); + assert.deepEqual([...store.values()].map(s => s.id), ["1/a.zip"]); + }); + + it("sweeps missing saves for an instance assigned to this host", async function() { + const store = run(10, [makeSave(1, "old.zip")], new lib.InstanceSaveDetailsUpdatesEvent([], 1)); + assert.deepEqual([...store.values()], []); + }); + + it("ignores injected saves for an instance on another host", async function() { + const store = run(30, [], new lib.InstanceSaveDetailsUpdatesEvent([makeSave(1, "HACKED.zip")])); + assert.deepEqual([...store.values()], [], "foreign save must not be stored"); + }); + + it("ignores a wipe of an instance on another host", async function() { + const existing = makeSave(1, "keep.zip"); + const store = run(30, [existing], new lib.InstanceSaveDetailsUpdatesEvent([], 1)); + assert.deepEqual([...store.values()].map(s => s.id), ["1/keep.zip"], "existing save must survive"); + }); + + it("ignores updates for a nonexistent instance", async function() { + const store = run(30, [], new lib.InstanceSaveDetailsUpdatesEvent([makeSave(999, "x.zip")])); + assert.deepEqual([...store.values()], []); + }); + }); +}); From cf4e85c6ee73a5bf02171f90108bf693f66964b3 Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 14:11:23 +0000 Subject: [PATCH 2/3] Guard forwarded log messages by instance assignment Log messages share the root cause with the save details updates fixed in the previous commit: the controller trusted host supplied instance fields. LogMessageEvent only declares level and message in its schema, but the Ajv instance is built without removeAdditional and the schema does not set additionalProperties, so instance_id and instance_name sent by a host survive validation and pass through fromJSON untouched. A host could tag a log line with an instance assigned to another host and have the controller store and serve it as that instance's output. Lift the assignment check out of handleInstanceSaveDetailsUpdatesEvent into a private method and use it from both handlers. Strip the instance fields instead of dropping the line. A host racing a reassignment does emit instance log lines for an instance the controller has already moved, and dropping those loses real output exactly when a migration is going wrong. The line is still genuine output from the host, so it is recorded as a host level message, which grants the host nothing it could not already log directly. Warn once per connection so a misbehaving host cannot flood the log. Co-Authored-By: Claude Opus 5 (1M context) --- packages/controller/src/HostConnection.ts | 41 ++++++++++++----- test/controller/HostConnection.test.js | 55 +++++++++++++++++++++-- 2 files changed, 81 insertions(+), 15 deletions(-) diff --git a/packages/controller/src/HostConnection.ts b/packages/controller/src/HostConnection.ts index ec2f55d9..5bbc59e2 100644 --- a/packages/controller/src/HostConnection.ts +++ b/packages/controller/src/HostConnection.ts @@ -18,6 +18,8 @@ import HostRecord from "./HostRecord.js"; export default class HostConnection extends BaseConnection { private info: HostRecord; plugins: Map; + // Warn once per connection, a misbehaving host could log in a loop. + private _warnedBogusLogMessage = false; constructor( registerData: lib.RegisterHostData, @@ -295,20 +297,22 @@ export default class HostConnection extends BaseConnection { await this.send(new lib.SyncUserListsEvent(adminlist, banlist, whitelist)); } - async handleInstanceSaveDetailsUpdatesEvent(event: lib.InstanceSaveDetailsUpdatesEvent) { - // A host may only report saves for instances assigned to it. Otherwise - // any host could inject save records for, or wipe the save list of, an - // instance on a different host. See handleInstanceStatusChangedEvent. - const assignedHere = (instanceId: number) => { - const instance = this._controller.instances.get(instanceId); - return instance !== undefined && instance.config.get("instance.assigned_host") === this.id; - }; + // A host may only report on instances assigned to it. Otherwise any host + // could speak for an instance on a different host. See + // handleInstanceStatusChangedEvent. + private _assignedHere(instanceId: number) { + const instance = this._controller.instances.get(instanceId); + return instance !== undefined && instance.config.get("instance.assigned_host") === this.id; + } - if (event.instanceId !== undefined && !assignedHere(event.instanceId)) { + async handleInstanceSaveDetailsUpdatesEvent(event: lib.InstanceSaveDetailsUpdatesEvent) { + // Otherwise any host could inject save records for, or wipe the save + // list of, an instance on a different host. + if (event.instanceId !== undefined && !this._assignedHere(event.instanceId)) { logger.warn(`Got bogus save updates for instance id ${event.instanceId}`); return; } - if (event.updates.some(save => !assignedHere(save.instanceId))) { + if (event.updates.some(save => !this._assignedHere(save.instanceId))) { logger.warn(`Got bogus save updates from host id ${this.id}`); return; } @@ -337,8 +341,23 @@ export default class HostConnection extends BaseConnection { } async handleLogMessageEvent(event: lib.LogMessageEvent) { + // Fields beyond level and message survive schema validation, so the + // instance a log line claims to come from is whatever the host sent. + const info: typeof event.info & { instance_id?: number, instance_name?: string } = { ...event.info }; + if (info.instance_id !== undefined && !this._assignedHere(info.instance_id)) { + // Demote to a host level line instead of dropping it. The host did + // log this, it just does not get to pin it on someone else's + // instance, and a host racing a reassignment still gets heard. + if (!this._warnedBogusLogMessage) { + this._warnedBogusLogMessage = true; + logger.warn(`Got bogus log message for instance id ${info.instance_id}`); + } + delete info.instance_id; + delete info.instance_name; + } + this._controller.clusterLogger.log({ - ...event.info, + ...info, host_id: this.id, host_name: this.info.name, }); diff --git a/test/controller/HostConnection.test.js b/test/controller/HostConnection.test.js index fde949d9..fe884c98 100644 --- a/test/controller/HostConnection.test.js +++ b/test/controller/HostConnection.test.js @@ -39,13 +39,14 @@ describe("controller/src/HostConnection", function() { // Instance 1 is on host 10, instance 2 is on host 20. const assignments = new Map([[1, 10], [2, 20]]); + // Inherit the prototype, handlers call other methods on this. function run(hostId, saves, event) { const saveStore = new MockSaves(saves); - const ctx = { - id: hostId, + const ctx = Object.assign(Object.create(HostConnection.prototype), { + info: { id: hostId }, _controller: { instances: makeInstances(assignments), saves: saveStore }, - }; - HostConnection.prototype.handleInstanceSaveDetailsUpdatesEvent.call(ctx, event); + }); + ctx.handleInstanceSaveDetailsUpdatesEvent(event); return saveStore; } @@ -75,4 +76,50 @@ describe("controller/src/HostConnection", function() { assert.deepEqual([...store.values()], []); }); }); + + describe(".handleLogMessageEvent()", function() { + // Instance 1 is on host 10, instance 2 is on host 20. + const assignments = new Map([[1, 10], [2, 20]]); + + function run(hostId, info) { + const logged = []; + const ctx = Object.assign(Object.create(HostConnection.prototype), { + info: { id: hostId, name: `host-${hostId}` }, + _controller: { + instances: makeInstances(assignments), + clusterLogger: { log(entry) { logged.push(entry); } }, + }, + }); + ctx.handleLogMessageEvent(new lib.LogMessageEvent(info)); + return logged; + } + + it("keeps instance fields for an instance assigned to this host", async function() { + const logged = run(10, { level: "info", message: "hello", instance_id: 1, instance_name: "one" }); + assert.deepEqual(logged, [{ + level: "info", message: "hello", instance_id: 1, instance_name: "one", + host_id: 10, host_name: "host-10", + }]); + }); + + it("logs host level messages unchanged", async function() { + const logged = run(10, { level: "info", message: "hello" }); + assert.deepEqual(logged, [{ level: "info", message: "hello", host_id: 10, host_name: "host-10" }]); + }); + + it("strips instance fields claiming an instance on another host", async function() { + const logged = run(30, { level: "info", message: "hi", instance_id: 1, instance_name: "one" }); + assert.deepEqual(logged, [{ level: "info", message: "hi", host_id: 30, host_name: "host-30" }]); + }); + + it("strips instance fields claiming a nonexistent instance", async function() { + const logged = run(30, { level: "info", message: "hi", instance_id: 999, instance_name: "ghost" }); + assert.deepEqual(logged, [{ level: "info", message: "hi", host_id: 30, host_name: "host-30" }]); + }); + + it("overrides host fields sent by the host", async function() { + const logged = run(10, { level: "info", message: "hi", host_id: 99, host_name: "spoofed" }); + assert.deepEqual(logged, [{ level: "info", message: "hi", host_id: 10, host_name: "host-10" }]); + }); + }); }); From 70c823ae9f9cf62d9c03f1017c32c1c5ced3c3aa Mon Sep 17 00:00:00 2001 From: bbassie <17990055+bbassie@users.noreply.github.com> Date: Sun, 13 Sep 2026 17:09:53 +0000 Subject: [PATCH 3/3] Correct misaligned instance names on forwarded log messages The instance name on a log line is whatever the host sent, so a host can attribute a line for one of its own instances to the wrong name. Compare it with the name the controller has on record and replace it when they differ. Host connections can live for days, so the warning suppression expires after five minutes instead of latching for the life of the connection, and the bogus instance and misaligned name warnings share it so the two together cannot spam the log. Co-Authored-By: Claude Opus 5 (1M context) --- packages/controller/src/HostConnection.ts | 54 ++++++++++++----- test/controller/HostConnection.test.js | 72 +++++++++++++++++++++-- 2 files changed, 106 insertions(+), 20 deletions(-) diff --git a/packages/controller/src/HostConnection.ts b/packages/controller/src/HostConnection.ts index 5bbc59e2..56d5d489 100644 --- a/packages/controller/src/HostConnection.ts +++ b/packages/controller/src/HostConnection.ts @@ -9,6 +9,9 @@ import InstanceRecord from "./InstanceRecord.js"; import HostRecord from "./HostRecord.js"; +// How long log message warnings are suppressed for after one is logged. +const logMessageWarnIntervalMs = 5 * 60 * 1000; + /** * Represents the connection to a host * @@ -18,8 +21,8 @@ import HostRecord from "./HostRecord.js"; export default class HostConnection extends BaseConnection { private info: HostRecord; plugins: Map; - // Warn once per connection, a misbehaving host could log in a loop. - private _warnedBogusLogMessage = false; + // Shared by all log message warnings, a misbehaving host could log in a loop. + private _lastLogMessageWarnMs = Number.NEGATIVE_INFINITY; constructor( registerData: lib.RegisterHostData, @@ -300,19 +303,30 @@ export default class HostConnection extends BaseConnection { // A host may only report on instances assigned to it. Otherwise any host // could speak for an instance on a different host. See // handleInstanceStatusChangedEvent. - private _assignedHere(instanceId: number) { + private _assignedInstance(instanceId: number) { const instance = this._controller.instances.get(instanceId); - return instance !== undefined && instance.config.get("instance.assigned_host") === this.id; + return instance?.config.get("instance.assigned_host") === this.id ? instance : undefined; + } + + // Host connections live for days, so the suppression expires instead of + // latching for the life of the connection. + private _warnLogMessage(message: string) { + const now = Date.now(); + if (now - this._lastLogMessageWarnMs < logMessageWarnIntervalMs) { + return; + } + this._lastLogMessageWarnMs = now; + logger.warn(message); } async handleInstanceSaveDetailsUpdatesEvent(event: lib.InstanceSaveDetailsUpdatesEvent) { // Otherwise any host could inject save records for, or wipe the save // list of, an instance on a different host. - if (event.instanceId !== undefined && !this._assignedHere(event.instanceId)) { + if (event.instanceId !== undefined && !this._assignedInstance(event.instanceId)) { logger.warn(`Got bogus save updates for instance id ${event.instanceId}`); return; } - if (event.updates.some(save => !this._assignedHere(save.instanceId))) { + if (event.updates.some(save => !this._assignedInstance(save.instanceId))) { logger.warn(`Got bogus save updates from host id ${this.id}`); return; } @@ -344,16 +358,26 @@ export default class HostConnection extends BaseConnection { // Fields beyond level and message survive schema validation, so the // instance a log line claims to come from is whatever the host sent. const info: typeof event.info & { instance_id?: number, instance_name?: string } = { ...event.info }; - if (info.instance_id !== undefined && !this._assignedHere(info.instance_id)) { - // Demote to a host level line instead of dropping it. The host did - // log this, it just does not get to pin it on someone else's - // instance, and a host racing a reassignment still gets heard. - if (!this._warnedBogusLogMessage) { - this._warnedBogusLogMessage = true; - logger.warn(`Got bogus log message for instance id ${info.instance_id}`); + if (info.instance_id !== undefined) { + const instance = this._assignedInstance(info.instance_id); + if (!instance) { + // Demote to a host level line instead of dropping it. The host did + // log this, it just does not get to pin it on someone else's + // instance, and a host racing a reassignment still gets heard. + this._warnLogMessage(`Got bogus log message for instance id ${info.instance_id}`); + delete info.instance_id; + delete info.instance_name; + } else { + // The name the host sent may be stale or made up, ours is authoritative. + const name = instance.config.get("instance.name"); + if (info.instance_name !== name) { + this._warnLogMessage( + `Got log message for instance id ${info.instance_id} named ` + + `${info.instance_name} which is named ${name} on the controller` + ); + info.instance_name = name; + } } - delete info.instance_id; - delete info.instance_name; } this._controller.clusterLogger.log({ diff --git a/test/controller/HostConnection.test.js b/test/controller/HostConnection.test.js index fe884c98..7c86e278 100644 --- a/test/controller/HostConnection.test.js +++ b/test/controller/HostConnection.test.js @@ -21,15 +21,20 @@ class MockSaves { deleteMany(saves) { for (const s of saves) { this.saves.delete(s.id); } } } -// Mock instances manager where instance id -> assigned host id. -function makeInstances(assignments) { +// Mock instances manager where instance id -> assigned host id, and +// optionally instance id -> the name the controller has on record. +function makeInstances(assignments, names = new Map()) { return { get(id) { if (!assignments.has(id)) { return undefined; } const host = assignments.get(id); - return { config: { get: name => (name === "instance.assigned_host" ? host : undefined) } }; + return { config: { get(field) { + if (field === "instance.assigned_host") { return host; } + if (field === "instance.name") { return names.get(id); } + return undefined; + } } }; }, }; } @@ -80,16 +85,34 @@ describe("controller/src/HostConnection", function() { describe(".handleLogMessageEvent()", function() { // Instance 1 is on host 10, instance 2 is on host 20. const assignments = new Map([[1, 10], [2, 20]]); + const names = new Map([[1, "one"], [2, "two"]]); - function run(hostId, info) { + let warnings = []; + let originalWarn; + beforeEach(function() { + warnings = []; + originalWarn = lib.logger.warn; + lib.logger.warn = msg => { warnings.push(msg); }; + }); + afterEach(function() { + lib.logger.warn = originalWarn; + }); + + // Handlers call other methods on this, so inherit the prototype. + function makeConnection(hostId) { const logged = []; const ctx = Object.assign(Object.create(HostConnection.prototype), { info: { id: hostId, name: `host-${hostId}` }, _controller: { - instances: makeInstances(assignments), + instances: makeInstances(assignments, names), clusterLogger: { log(entry) { logged.push(entry); } }, }, }); + return [ctx, logged]; + } + + function run(hostId, info) { + const [ctx, logged] = makeConnection(hostId); ctx.handleLogMessageEvent(new lib.LogMessageEvent(info)); return logged; } @@ -100,6 +123,45 @@ describe("controller/src/HostConnection", function() { level: "info", message: "hello", instance_id: 1, instance_name: "one", host_id: 10, host_name: "host-10", }]); + assert.deepEqual(warnings, [], "a matching name must not warn"); + }); + + it("corrects an instance name which does not match the controller", async function() { + const logged = run(10, { level: "info", message: "hi", instance_id: 1, instance_name: "wrong" }); + assert.deepEqual(logged, [{ + level: "info", message: "hi", instance_id: 1, instance_name: "one", + host_id: 10, host_name: "host-10", + }]); + assert.equal(warnings.length, 1, "a misaligned name must warn"); + }); + + it("warns about a misaligned name again after five minutes", async function() { + const [ctx, logged] = makeConnection(10); + const info = { level: "info", message: "hi", instance_id: 1, instance_name: "wrong" }; + ctx.handleLogMessageEvent(new lib.LogMessageEvent(info)); + ctx.handleLogMessageEvent(new lib.LogMessageEvent(info)); + assert.equal(warnings.length, 1, "the warning must not repeat immediately"); + + // Pretend the last warning happened just over five minutes ago. + ctx._lastLogMessageWarnMs -= 5 * 60 * 1000 + 1; + ctx.handleLogMessageEvent(new lib.LogMessageEvent(info)); + assert.equal(warnings.length, 2, "the warning must resurface after the timeout"); + assert.equal(logged.length, 3); + assert.deepEqual(logged.map(e => e.instance_name), ["one", "one", "one"]); + }); + + it("shares the warning suppression between bogus id and misaligned name", async function() { + const [ctx, logged] = makeConnection(10); + // Instance 2 is on host 20, so this line is bogus. + ctx.handleLogMessageEvent(new lib.LogMessageEvent( + { level: "info", message: "hi", instance_id: 2, instance_name: "two" } + )); + assert.equal(warnings.length, 1); + ctx.handleLogMessageEvent(new lib.LogMessageEvent( + { level: "info", message: "hi", instance_id: 1, instance_name: "wrong" } + )); + assert.equal(warnings.length, 1, "both warnings must share the suppression"); + assert.equal(logged[1].instance_name, "one", "the name is still corrected while suppressed"); }); it("logs host level messages unchanged", async function() {