Merge pull request #220 from simoleo89/fix/room-settings-guards

fix(rooms): validate room settings inputs
This commit is contained in:
DuckieTM
2026-06-17 09:54:41 +02:00
committed by GitHub
2 changed files with 134 additions and 17 deletions
@@ -15,6 +15,15 @@ import java.util.Set;
public class RoomSettingsSaveEvent extends MessageHandler {
private static final Logger LOGGER = LoggerFactory.getLogger(RoomSettingsSaveEvent.class);
private static final int MAX_ROOM_PASSWORD_LENGTH = 64;
private static final int MAX_TAGS = 2;
private static final int MIN_USERS_MAX = 1;
private static final int MAX_USERS_MAX = 200;
private static final int MIN_THICKNESS = -2;
private static final int MAX_THICKNESS = 1;
private static final int MAX_OPTION_LEVEL = 2;
private static final int MIN_CHAT_DISTANCE = 1;
private static final int MAX_CHAT_DISTANCE = 99;
@Override
public void handle() throws Exception {
@@ -47,19 +56,33 @@ public class RoomSettingsSaveEvent extends MessageHandler {
return;
}
RoomState state = RoomState.values()[this.packet.readInt() % RoomState.values().length];
int stateId = this.packet.readInt();
if (stateId < 0 || stateId >= RoomState.values().length) {
return;
}
RoomState state = RoomState.values()[stateId];
String password = this.packet.readString();
if (password.length() > MAX_ROOM_PASSWORD_LENGTH) {
return;
}
if (state == RoomState.PASSWORD && password.isEmpty() && (room.getPassword() == null || room.getPassword().isEmpty())) {
this.client.sendResponse(new RoomEditSettingsErrorComposer(room.getId(), RoomEditSettingsErrorComposer.PASSWORD_REQUIRED, ""));
return;
}
int usersMax = this.packet.readInt();
if (usersMax < MIN_USERS_MAX || usersMax > MAX_USERS_MAX) {
return;
}
int categoryId = this.packet.readInt();
StringBuilder tags = new StringBuilder();
Set<String> uniqueTags = new HashSet<>();
int count = Math.min(this.packet.readInt(), 2);
int count = this.packet.readInt();
if (count < 0 || count > MAX_TAGS) {
return;
}
for (int i = 0; i < count; i++) {
String tag = this.packet.readString();
@@ -113,22 +136,52 @@ public class RoomSettingsSaveEvent extends MessageHandler {
}
int tradeMode = this.packet.readInt();
boolean allowPets = this.packet.readBoolean();
boolean allowPetsEat = this.packet.readBoolean();
boolean allowWalkthrough = this.packet.readBoolean();
boolean hideWall = this.packet.readBoolean();
int wallSize = this.packet.readInt();
int floorSize = this.packet.readInt();
int muteOption = this.packet.readInt();
int kickOption = this.packet.readInt();
int banOption = this.packet.readInt();
int chatMode = this.packet.readInt();
int chatWeight = this.packet.readInt();
int chatSpeed = this.packet.readInt();
int chatDistance = this.packet.readInt();
int chatProtection = this.packet.readInt();
if (!isInRange(tradeMode, 0, MAX_OPTION_LEVEL)
|| !isInRange(wallSize, MIN_THICKNESS, MAX_THICKNESS)
|| !isInRange(floorSize, MIN_THICKNESS, MAX_THICKNESS)
|| !isInRange(muteOption, 0, MAX_OPTION_LEVEL)
|| !isInRange(kickOption, 0, MAX_OPTION_LEVEL)
|| !isInRange(banOption, 0, MAX_OPTION_LEVEL)
|| !isInRange(chatMode, 0, MAX_OPTION_LEVEL)
|| !isInRange(chatWeight, 0, MAX_OPTION_LEVEL)
|| !isInRange(chatSpeed, 0, MAX_OPTION_LEVEL)
|| !isInRange(chatDistance, MIN_CHAT_DISTANCE, MAX_CHAT_DISTANCE)
|| !isInRange(chatProtection, 0, MAX_OPTION_LEVEL)) {
return;
}
room.setTags(tags.toString());
room.setTradeMode(this.packet.readInt());
room.setAllowPets(this.packet.readBoolean());
room.setAllowPetsEat(this.packet.readBoolean());
room.setAllowWalkthrough(this.packet.readBoolean());
room.setHideWall(this.packet.readBoolean());
room.setWallSize(this.packet.readInt());
room.setFloorSize(this.packet.readInt());
room.setMuteOption(this.packet.readInt());
room.setKickOption(this.packet.readInt());
room.setBanOption(this.packet.readInt());
room.setChatMode(this.packet.readInt());
room.setChatWeight(this.packet.readInt());
room.setChatSpeed(this.packet.readInt());
room.setChatDistance(Math.abs(this.packet.readInt()));
room.setChatProtection(this.packet.readInt());
room.setTradeMode(tradeMode);
room.setAllowPets(allowPets);
room.setAllowPetsEat(allowPetsEat);
room.setAllowWalkthrough(allowWalkthrough);
room.setHideWall(hideWall);
room.setWallSize(wallSize);
room.setFloorSize(floorSize);
room.setMuteOption(muteOption);
room.setKickOption(kickOption);
room.setBanOption(banOption);
room.setChatMode(chatMode);
room.setChatWeight(chatWeight);
room.setChatSpeed(chatSpeed);
room.setChatDistance(chatDistance);
room.setChatProtection(chatProtection);
if (this.packet.bytesAvailable() > 0) {
room.setAllowUnderpass(this.packet.readBoolean());
@@ -144,4 +197,8 @@ public class RoomSettingsSaveEvent extends MessageHandler {
}
}
}
private static boolean isInRange(int value, int min, int max) {
return value >= min && value <= max;
}
}
@@ -0,0 +1,60 @@
package com.eu.habbo.messages.incoming.rooms;
import org.junit.jupiter.api.Test;
import java.nio.file.Files;
import java.nio.file.Path;
import static org.junit.jupiter.api.Assertions.assertTrue;
class RoomSettingsInputGuardContractTest {
private static String source() throws Exception {
return Files.readString(Path.of("src/main/java/com/eu/habbo/messages/incoming/rooms/RoomSettingsSaveEvent.java"));
}
@Test
void roomStateAndTagCountAreValidatedWithoutModuloOrTruncation() throws Exception {
String source = source();
int stateRead = source.indexOf("int stateId = this.packet.readInt()");
int stateGuard = source.indexOf("stateId < 0 || stateId >= RoomState.values().length", stateRead);
int stateAssign = source.indexOf("RoomState state = RoomState.values()[stateId]", stateGuard);
int tagCount = source.indexOf("int count = this.packet.readInt()", stateAssign);
int tagGuard = source.indexOf("count < 0 || count > MAX_TAGS", tagCount);
int tagLoop = source.indexOf("for (int i = 0; i < count; i++)", tagGuard);
assertTrue(source.contains("MAX_TAGS = 2"), "Room settings tag count should have an explicit cap");
assertTrue(!source.contains("% RoomState.values().length"),
"Room state must not use modulo because negative values can crash or remap input");
assertTrue(!source.contains("Math.min(this.packet.readInt(), 2)"),
"Room settings must reject oversized tag counts instead of truncating and desynchronizing the packet");
assertTrue(stateRead > -1 && stateGuard > stateRead && stateAssign > stateGuard,
"Room state must be range-checked before indexing RoomState.values()");
assertTrue(tagCount > -1 && tagGuard > tagCount && tagLoop > tagGuard,
"Tag count must be range-checked before reading tag strings");
}
@Test
void roomSettingsOptionsAreValidatedBeforeMutatingRoom() throws Exception {
String source = source();
int tradeMode = source.indexOf("int tradeMode = this.packet.readInt()");
int validation = source.indexOf("!isInRange(tradeMode, 0, MAX_OPTION_LEVEL)", tradeMode);
int setTags = source.indexOf("room.setTags(tags.toString())", validation);
int setChatDistance = source.indexOf("room.setChatDistance(chatDistance)", setTags);
assertTrue(source.contains("MAX_ROOM_PASSWORD_LENGTH = 64"),
"Room password should have a bounded server-side length");
assertTrue(source.contains("MAX_USERS_MAX = 200"),
"Room capacity should have a bounded server-side maximum");
assertTrue(source.contains("MIN_CHAT_DISTANCE = 1") && source.contains("MAX_CHAT_DISTANCE = 99"),
"Room chat distance should be explicitly bounded");
assertTrue(!source.contains("Math.abs(this.packet.readInt())"),
"Room settings must reject invalid chat distance instead of converting negative values");
assertTrue(validation > tradeMode, "Room options must be validated after reading them");
assertTrue(validation < setTags, "Room options must be validated before mutating room fields");
assertTrue(setChatDistance > setTags, "Validated chat distance should be applied after the guard block");
assertTrue(source.contains("private static boolean isInRange"),
"Room settings should use one clear range helper for numeric option guards");
}
}