Merge pull request #265 from simoleo89/fix/trade-ownership-batch-counts

fix(trade): verify ownership transfers

Emulator/src/main/java/com/eu/habbo/habbohotel/rooms/RoomTrade.java

@@ -204,11 +204,12 @@ public class RoomTrade {
             int userOneId = userOne.getHabbo().getHabboInfo().getId();
             int userTwoId = userTwo.getHabbo().getHabboInfo().getId();
 
-            try (PreparedStatement statement = connection.prepareStatement("UPDATE items SET user_id = ? WHERE id = ? LIMIT 1")) {
+            try (PreparedStatement statement = connection.prepareStatement("UPDATE items SET user_id = ? WHERE id = ? AND user_id = ? LIMIT 1")) {
                 try (PreparedStatement stmt = connection.prepareStatement("INSERT INTO room_trade_log_items (id, item_id, user_id) VALUES (?, ?, ?)")) {
                     for (HabboItem item : userOne.getItems()) {
                         statement.setInt(1, userTwoId);
                         statement.setInt(2, item.getId());
+                        statement.setInt(3, userOneId);
                         statement.addBatch();
 
                         if (logTrades) {
@@ -222,6 +223,7 @@ public class RoomTrade {
                     for (HabboItem item : userTwo.getItems()) {
                         statement.setInt(1, userOneId);
                         statement.setInt(2, item.getId());
+                        statement.setInt(3, userTwoId);
                         statement.addBatch();
 
                         if (logTrades) {
@@ -237,7 +239,12 @@ public class RoomTrade {
                     }
                 }
 
-                statement.executeBatch();
+                int expectedUpdates = userOne.getItems().size() + userTwo.getItems().size();
+                int[] updateCounts = statement.executeBatch();
+                if (!RoomTrade.allOwnershipUpdatesSucceeded(updateCounts, expectedUpdates)) {
+                    this.sendMessageToUsers(new TradeClosedComposer(userOne.getHabbo().getRoomUnit().getId(), TradeClosedComposer.ITEMS_NOT_FOUND));
+                    return false;
+                }
             }
         } catch (SQLException e) {
             LOGGER.error("Caught SQL exception", e);
@@ -364,6 +371,20 @@ public class RoomTrade {
         return this.users;
     }
 
+    static boolean allOwnershipUpdatesSucceeded(int[] updateCounts, int expectedUpdates) {
+        if (updateCounts == null || updateCounts.length != expectedUpdates) {
+            return false;
+        }
+
+        for (int updateCount : updateCounts) {
+            if (updateCount == Statement.EXECUTE_FAILED || updateCount == 0) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     public static int getCreditsByItem(HabboItem item) {
         if (!Emulator.getConfig().getBoolean("redeem.currency.trade")) return 0;
 

Emulator/src/test/java/com/eu/habbo/habbohotel/rooms/RoomTradeSafetyContractTest.java

@@ -2,6 +2,7 @@ package com.eu.habbo.habbohotel.rooms;
 
 import org.junit.jupiter.api.Test;
 
+import java.sql.Statement;
 import java.nio.file.Files;
 import java.nio.file.Path;
 
@@ -28,11 +29,33 @@ class RoomTradeSafetyContractTest {
     void itemOwnersChangeOnlyAfterDatabaseBatchSucceeds() throws Exception {
         String source = roomTradeSource();
         int firstOwnerMutation = source.indexOf("item.setUserId(");
-        int batchExecution = source.indexOf("statement.executeBatch();");
+        int batchExecution = source.indexOf("int[] updateCounts = statement.executeBatch();");
+        int batchGuard = source.indexOf("allOwnershipUpdatesSucceeded(updateCounts, expectedUpdates)", batchExecution);
 
         assertTrue(firstOwnerMutation > -1, "RoomTrade should update in-memory item owners after commit");
         assertTrue(batchExecution > -1, "RoomTrade should persist item owner changes with a batch update");
-        assertTrue(firstOwnerMutation > batchExecution,
+        assertTrue(batchGuard > batchExecution, "RoomTrade must validate every ownership update before mutating memory");
+        assertTrue(firstOwnerMutation > batchGuard,
                 "In-memory item owners must not change until the database batch has succeeded");
     }
+
+    @Test
+    void ownershipUpdatesRequireExpectedDatabaseOwner() throws Exception {
+        String source = roomTradeSource();
+
+        assertTrue(source.contains("UPDATE items SET user_id = ? WHERE id = ? AND user_id = ? LIMIT 1"),
+                "RoomTrade ownership transfer should only update items still owned by the offering user");
+        assertTrue(source.contains("statement.setInt(3, userOneId)"),
+                "User one offered items must require user one as the current database owner");
+        assertTrue(source.contains("statement.setInt(3, userTwoId)"),
+                "User two offered items must require user two as the current database owner");
+    }
+
+    @Test
+    void zeroBatchUpdatesAbortTheTrade() {
+        assertTrue(RoomTrade.allOwnershipUpdatesSucceeded(new int[]{1, Statement.SUCCESS_NO_INFO}, 2));
+        assertTrue(!RoomTrade.allOwnershipUpdatesSucceeded(new int[]{1, 0}, 2));
+        assertTrue(!RoomTrade.allOwnershipUpdatesSucceeded(new int[]{1}, 2));
+        assertTrue(!RoomTrade.allOwnershipUpdatesSucceeded(new int[]{Statement.EXECUTE_FAILED}, 1));
+    }
 }