From 182a42e0dbbfb498efd78dbe75a8bc6202951e6e Mon Sep 17 00:00:00 2001
From: evanpelle <evanpelle@gmail.com>
Date: Sun, 25 May 2025 13:24:04 -0700
Subject: [PATCH] Bugfix: don't allow other players to move warships (#879)

## Description:
The MoveWarshipExecution now verifies that the player who requested to
move owns the warship. This prevents players from moving other players'
warships.

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced
- [x] I understand that submitting code with bugs that could have been
caught through manual testing blocks releases and new features for all
contributors

## Please put your Discord username so you can be contacted if a bug or
regression is found:

evan
---
 src/core/execution/ExecutionManager.ts     |  2 +-
 src/core/execution/MoveWarshipExecution.ts | 35 +++++++--------
 tests/Warship.test.ts                      | 50 +++++++++++++++++++++-
 3 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/src/core/execution/ExecutionManager.ts b/src/core/execution/ExecutionManager.ts
index 230784921..0cd160cb0 100644
--- a/src/core/execution/ExecutionManager.ts
+++ b/src/core/execution/ExecutionManager.ts
@@ -63,7 +63,7 @@ export class Executor {
       case "cancel_boat":
         return new BoatRetreatExecution(playerID, intent.unitID);
       case "move_warship":
-        return new MoveWarshipExecution(intent.unitId, intent.tile);
+        return new MoveWarshipExecution(player, intent.unitId, intent.tile);
       case "spawn":
         return new SpawnExecution(
           player.info(),
diff --git a/src/core/execution/MoveWarshipExecution.ts b/src/core/execution/MoveWarshipExecution.ts
index 87036c3e8..13431bc1d 100644
--- a/src/core/execution/MoveWarshipExecution.ts
+++ b/src/core/execution/MoveWarshipExecution.ts
@@ -1,36 +1,33 @@
-import { Execution, Game } from "../game/Game";
-
-const cancelDelay = 2;
+import { Execution, Game, Player, UnitType } from "../game/Game";
+import { TileRef } from "../game/GameMap";
 
 export class MoveWarshipExecution implements Execution {
-  private active = true;
-  private mg: Game | null = null;
-
   constructor(
-    public readonly unitId: number,
-    public readonly position: number,
+    private readonly owner: Player,
+    private readonly unitId: number,
+    private readonly position: TileRef,
   ) {}
 
   init(mg: Game, ticks: number): void {
-    this.mg = mg;
-  }
-
-  tick(ticks: number): void {
-    if (this.mg === null) {
-      throw new Error("Not initialized");
-    }
-    const warship = this.mg.units().find((u) => u.id() === this.unitId);
+    const warship = this.owner
+      .units(UnitType.Warship)
+      .find((u) => u.id() === this.unitId);
     if (!warship) {
-      console.log("MoveWarshipExecution: warship is already dead");
+      console.warn("MoveWarshipExecution: warship not found");
+      return;
+    }
+    if (!warship.isActive()) {
+      console.warn("MoveWarshipExecution: warship is not active");
       return;
     }
     warship.setPatrolTile(this.position);
     warship.setTargetTile(undefined);
-    this.active = false;
   }
 
+  tick(ticks: number): void {}
+
   isActive(): boolean {
-    return this.active;
+    return false;
   }
 
   activeDuringSpawnPhase(): boolean {
diff --git a/tests/Warship.test.ts b/tests/Warship.test.ts
index bbd8ad48d..1fc8b2881 100644
--- a/tests/Warship.test.ts
+++ b/tests/Warship.test.ts
@@ -175,7 +175,7 @@ describe("Warship", () => {
     game.addExecution(new WarshipExecution(warship));
 
     game.addExecution(
-      new MoveWarshipExecution(warship.id(), game.ref(coastX + 5, 15)),
+      new MoveWarshipExecution(player1, warship.id(), game.ref(coastX + 5, 15)),
     );
 
     executeTicks(game, 10);
@@ -211,4 +211,52 @@ describe("Warship", () => {
     // Trade ship should not be captured
     expect(tradeShip.owner().id()).toBe(player2.id());
   });
+
+  test("MoveWarshipExecution fails if player is not the owner", async () => {
+    const originalPatrolTile = game.ref(coastX + 1, 10);
+    const warship = player1.buildUnit(
+      UnitType.Warship,
+      game.ref(coastX + 1, 5),
+      {
+        patrolTile: originalPatrolTile,
+      },
+    );
+    new MoveWarshipExecution(
+      player2,
+      warship.id(),
+      game.ref(coastX + 5, 15),
+    ).init(game, 0);
+    expect(warship.patrolTile()).toBe(originalPatrolTile);
+  });
+
+  test("MoveWarshipExecution fails if warship is not active", async () => {
+    const originalPatrolTile = game.ref(coastX + 1, 10);
+    const warship = player1.buildUnit(
+      UnitType.Warship,
+      game.ref(coastX + 1, 5),
+      {
+        patrolTile: originalPatrolTile,
+      },
+    );
+    warship.delete();
+    new MoveWarshipExecution(
+      player1,
+      warship.id(),
+      game.ref(coastX + 5, 15),
+    ).init(game, 0);
+    expect(warship.patrolTile()).toBe(originalPatrolTile);
+  });
+
+  test("MoveWarshipExecution fails gracefully if warship not found", async () => {
+    const exec = new MoveWarshipExecution(
+      player1,
+      123,
+      game.ref(coastX + 5, 15),
+    );
+
+    // Verify that no error is thrown.
+    exec.init(game, 0);
+
+    expect(exec.isActive()).toBe(false);
+  });
 });