Close #8860: Server checks who may own a unit, on every path that changes it

[HammerGS]

What this changes

The server now decides who may own the units a client sends, instead of trusting the payload.

Three packets change ownership, and all three read the new owner straight out of the packet:

Packet	Handler	Before
ENTITY_ADD	receiveEntityAdd	owner never checked against the sender
ENTITY_ASSIGN	receiveEntitiesAssign	no check at all; took connId and never used it
FORCE_ASSIGN_FULL	receiveForceAssignFull	no check at all; same

The client offers only legal recipients, but a rule enforced on one side only is a rule the other side cannot rely on: two unrelated client changes once combined to hand every connecting player's units to the host for several days.

The rule, as agreed on the issue: the owner is the sender, or the sender holds Gamemaster, or the owner is a bot. It now lives in UnitOwnershipRules and all three handlers call it, so there is one rule rather than three chances to get it wrong.

A reassignment asks it twice, because giving a unit away and receiving one are separate questions: may the sender act for the unit's current owner, and may the recipient receive it. A force moves whole, so one unit the sender may not give refuses the force rather than moving part of it. A refused hand-off is named in chat, which is how an illegal design is already handled.

Every decision is logged, not only refusals, with the clause that applied:

[Ownership] add      by Raven's Nest   to Raven's Nest  : Agrotera AGT-1A  (SENDER_OWNS_THEM)
[Ownership] reassign by Raven's Nest   to Princess      : Agrotera AGT-1A  (RECIPIENT_IS_A_BOT)
[Ownership] add      by Raven's Nest   to Raven's Nest.2: Alpha Wolf Prime (SENDER_IS_GAMEMASTER)

A permitted hand-off that leaves no trace cannot be audited afterwards, and cannot be tested either. That is not incidental: the missing coverage on the reassignment paths went unnoticed precisely because exercising them produced silence.

Closes #8860

The bot clause

That clause is the known weak point and it is deliberate. Since #8808 a unit for your own Princess travels over your connection carrying the bot's owner id, and the server holds no record of which human runs which bot, so it cannot tell your bot from anyone else's. Every hand-off to a bot is logged rather than passed over in silence.

Closing it properly means clients registering the bots they run, which touches connection lifecycle and reconnects. That is its own change.

Testing

Tested in game with a host, a remote second client and a Princess bot. All three permitted clauses were exercised and the log agrees with the client's own recipient list:

(gamemaster: false) may add units to 2 of 3 player(s): [Raven's Nest, Princess]
(gamemaster: true)  may add units to 3 of 3 player(s): [Raven's Nest, Princess, Raven's Nest.2]

• Sender owns them - the host adding to itself, and the remote client adding to itself over its own connection.
• Recipient is a bot - transferring a unit to Princess, which is the ordinary way to stock a bot and the path that previously had no check and no log line.
• Sender is gamemaster - the host adding a unit for the remote player once Gamemaster was on.

No refusals during ordinary play, and nothing new in the log beyond the usual startup noise.

compileJava, checkstyleMain, checkstyleTest, spotlessApply and javadoc pass. EntityAddOwnershipTest is 9 tests, 0 failures, driving the real packet path through handlePacket: adding for yourself, for another human, as a gamemaster, for a bot, for a player not in the game, and the four reassignment cases. The refusal cases fail with the guard disabled, checked for both the add and the reassign paths.

What is not proven yet

• The refusal path has not been seen from a real client, only from tests. A compliant client never offers an illegal recipient, so it cannot be triggered by hand.
• No MekHQ scenario has been launched against this build. AtBGameThread sets the owner to the sending client's own player on all three paths and sends bot forces over the bot's own connection, so MekHQ satisfies the strictest clause, but that is from reading its code rather than running it.
• Force reassignment was not exercised in game. It has the same rule and shares the tested code, but only unit reassignment was done by hand.

---

• This PR is focused on one issue or RFE
• Every file in the diff has a deliberate change
• Tests added or updated, if this implements a rule or changes game state
• Javadoc literals use {@code true} / {@code null} rather than bare or quoted text
• Dev team only: AI tools used -> AI Assisted Development label applied

[Copilot]

🟡 Changes recommended

The new server-side ownership rule needs automated test coverage on the existing ENTITY_ADD packet path to prevent regressions, and there is also a documentation/Javadoc placement issue in the changed region.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens the Total Warfare server’s ENTITY_ADD handling by validating that the sending connection is allowed to assign the incoming entities’ owners, instead of trusting the owner id provided by the client payload (closing #8860).

Changes:

• Added server-side ownership authorization logic (mayAddUnitFor) for incoming ENTITY_ADD entities.
• Enforced the authorization rule in receiveEntityAdd by dropping unauthorized entities and reporting via server chat/logs.

File summaries

File	Description
megamek/src/megamek/server/totalWarfare/TWGameManager.java	Adds and enforces server-side validation of entity ownership on ENTITY_ADD packets, with logging/chat feedback when rejected.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[HammerGS]

Added in f48f825 as EntityAddOwnershipTest, following the C3MulLoadTest pattern you pointed at, so it drives the real ENTITY_ADD packet path rather than calling the guard directly.

Five cases: adding for yourself, for another human, as a gamemaster, for a bot, and for a player who is not in the game. Verified by disabling the guard, where the two refusal cases fail and the three permitted ones still pass.

[HammerGS]

Confirmed, the old block was left describing the wrong method. Moved back onto receiveEntityAdd in f48f825.

[HammerGS]

Tested in game and working. Out of draft.

Scope grew during testing, and it needed to. The guard originally covered adding units only. Transferring a unit is a different packet, ENTITY_ASSIGN, and reassigning a force is a third, FORCE_ASSIGN_FULL; both took the sending connection id and never looked at it. Transferring is the ordinary way to give a unit to a bot, so the lock was on the door nobody uses.

All three now share one rule in UnitOwnershipRules, and every decision is logged rather than only refusals. That second part is why the gap went unnoticed: exercising the permitted paths produced no output, so a correct run and an unreached one looked identical.

Verified with a host, a remote client and a Princess bot. Nine tests, refusal cases confirmed by disabling the guard.