fix(trade): verify ownership transfers

This commit is contained in:
simoleo89
2026-06-17 21:26:04 +02:00
parent 0109c25c80
commit 7cf83cf158
2 changed files with 48 additions and 4 deletions
@@ -204,11 +204,12 @@ public class RoomTrade {
int userOneId = userOne.getHabbo().getHabboInfo().getId(); int userOneId = userOne.getHabbo().getHabboInfo().getId();
int userTwoId = userTwo.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 (?, ?, ?)")) { try (PreparedStatement stmt = connection.prepareStatement("INSERT INTO room_trade_log_items (id, item_id, user_id) VALUES (?, ?, ?)")) {
for (HabboItem item : userOne.getItems()) { for (HabboItem item : userOne.getItems()) {
statement.setInt(1, userTwoId); statement.setInt(1, userTwoId);
statement.setInt(2, item.getId()); statement.setInt(2, item.getId());
statement.setInt(3, userOneId);
statement.addBatch(); statement.addBatch();
if (logTrades) { if (logTrades) {
@@ -222,6 +223,7 @@ public class RoomTrade {
for (HabboItem item : userTwo.getItems()) { for (HabboItem item : userTwo.getItems()) {
statement.setInt(1, userOneId); statement.setInt(1, userOneId);
statement.setInt(2, item.getId()); statement.setInt(2, item.getId());
statement.setInt(3, userTwoId);
statement.addBatch(); statement.addBatch();
if (logTrades) { 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) { } catch (SQLException e) {
LOGGER.error("Caught SQL exception", e); LOGGER.error("Caught SQL exception", e);
@@ -364,6 +371,20 @@ public class RoomTrade {
return this.users; 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) { public static int getCreditsByItem(HabboItem item) {
if (!Emulator.getConfig().getBoolean("redeem.currency.trade")) return 0; if (!Emulator.getConfig().getBoolean("redeem.currency.trade")) return 0;
@@ -2,6 +2,7 @@ package com.eu.habbo.habbohotel.rooms;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import java.sql.Statement;
import java.nio.file.Files; import java.nio.file.Files;
import java.nio.file.Path; import java.nio.file.Path;
@@ -28,11 +29,33 @@ class RoomTradeSafetyContractTest {
void itemOwnersChangeOnlyAfterDatabaseBatchSucceeds() throws Exception { void itemOwnersChangeOnlyAfterDatabaseBatchSucceeds() throws Exception {
String source = roomTradeSource(); String source = roomTradeSource();
int firstOwnerMutation = source.indexOf("item.setUserId("); 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(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(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"); "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));
}
} }