Merge pull request #226 from simoleo89/fix/room-user-inputs

fix(rooms): bound room user inputs

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserActionEvent.java

@@ -28,6 +28,9 @@ public class RoomUserActionEvent extends MessageHandler {
             }
 
             int action = this.packet.readInt();
+            if (!RoomUserInputGuard.isValidAction(action))
+                return;
+
             int wiredAction = 0;
 
             if (action == 5) {

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserBanEvent.java

@@ -12,6 +12,10 @@ public class RoomUserBanEvent extends MessageHandler {
         int roomId = this.packet.readInt();
         String banName = this.packet.readString();
 
+        if (!RoomUserInputGuard.isPositiveId(userId) || !RoomUserInputGuard.isPositiveId(roomId)) {
+            return;
+        }
+
         Room room = this.client.getHabbo().getHabboInfo().getCurrentRoom();
         if (room == null || room.getId() != roomId) {
             return;

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserGiveRightsEvent.java

@@ -13,6 +13,10 @@ public class RoomUserGiveRightsEvent extends MessageHandler {
     public void handle() throws Exception {
         int userId = this.packet.readInt();
 
+        if (!RoomUserInputGuard.isPositiveId(userId)) {
+            return;
+        }
+
         Room room = this.client.getHabbo().getHabboInfo().getCurrentRoom();
 
         if (room == null)

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserInputGuard.java

@@ -0,0 +1,17 @@
+package com.eu.habbo.messages.incoming.rooms.users;
+
+final class RoomUserInputGuard {
+    static final int MIN_ACTION_ID = 0;
+    static final int MAX_ACTION_ID = 7;
+
+    private RoomUserInputGuard() {
+    }
+
+    static boolean isPositiveId(int id) {
+        return id > 0;
+    }
+
+    static boolean isValidAction(int action) {
+        return action >= MIN_ACTION_ID && action <= MAX_ACTION_ID;
+    }
+}

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserKickEvent.java

@@ -21,6 +21,9 @@ public class RoomUserKickEvent extends MessageHandler {
 
         int userId = this.packet.readInt();
 
+        if (!RoomUserInputGuard.isPositiveId(userId))
+            return;
+
         Habbo target = room.getHabbo(userId);
 
         if (target == null)
@@ -35,15 +38,15 @@ public class RoomUserKickEvent extends MessageHandler {
             return;
         }
 
-        UserKickEvent event = new UserKickEvent(this.client.getHabbo(), target);
-        Emulator.getPluginManager().fireEvent(event);
-
-        if (event.isCancelled())
-            return;
-
         if (room.hasRights(this.client.getHabbo()) || this.client.getHabbo().hasPermission(Permission.ACC_ANYROOMOWNER) || this.client.getHabbo().hasPermission(Permission.ACC_AMBASSADOR)) {
             if (target.hasPermission(Permission.ACC_UNKICKABLE)) return;
 
+            UserKickEvent event = new UserKickEvent(this.client.getHabbo(), target);
+            Emulator.getPluginManager().fireEvent(event);
+
+            if (event.isCancelled())
+                return;
+
             room.kickHabbo(target, true);
             AchievementManager.progressAchievement(this.client.getHabbo(), Emulator.getGameEnvironment().getAchievementManager().getAchievement("SelfModKickSeen"));
         }

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserMuteEvent.java

@@ -18,6 +18,10 @@ public class RoomUserMuteEvent extends MessageHandler {
         int roomId = this.packet.readInt();
         int minutes = this.packet.readInt();
 
+        if (!RoomUserInputGuard.isPositiveId(userId) || !RoomUserInputGuard.isPositiveId(roomId)) {
+            return;
+        }
+
         Room room = this.client.getHabbo().getHabboInfo().getCurrentRoom();
         if (room == null || room.getId() != roomId) {
             return;

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserRemoveRightsEvent.java

@@ -27,6 +27,9 @@ public class RoomUserRemoveRightsEvent extends MessageHandler {
             for (int i = 0; i < amount; i++) {
                 int userId = this.packet.readInt();
 
+                if (!RoomUserInputGuard.isPositiveId(userId))
+                    continue;
+
                 room.removeRights(userId);
             }
         }

Emulator/src/main/java/com/eu/habbo/messages/incoming/rooms/users/UnbanRoomUserEvent.java

@@ -10,6 +10,10 @@ public class UnbanRoomUserEvent extends MessageHandler {
         int userId = this.packet.readInt();
         int roomId = this.packet.readInt();
 
+        if (!RoomUserInputGuard.isPositiveId(userId) || !RoomUserInputGuard.isPositiveId(roomId)) {
+            return;
+        }
+
         Room room = this.client.getHabbo().getHabboInfo().getCurrentRoom();
         if (room == null || room.getId() != roomId) {
             return;

Emulator/src/test/java/com/eu/habbo/messages/incoming/rooms/users/RoomUserInputGuardContractTest.java

@@ -0,0 +1,91 @@
+package com.eu.habbo.messages.incoming.rooms.users;
+
+import org.junit.jupiter.api.Test;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class RoomUserInputGuardContractTest {
+    private static String source(String name) throws Exception {
+        return Files.readString(Path.of("src/main/java/com/eu/habbo/messages/incoming/rooms/users/" + name + ".java"));
+    }
+
+    @Test
+    void roomModerationHandlersRejectInvalidUserAndRoomIds() throws Exception {
+        for (String handler : new String[]{"RoomUserBanEvent", "UnbanRoomUserEvent", "RoomUserMuteEvent"}) {
+            String source = source(handler);
+            int userRead = source.indexOf("int userId = this.packet.readInt()");
+            int roomRead = source.indexOf("int roomId = this.packet.readInt()", userRead);
+            int guard = source.indexOf("RoomUserInputGuard.isPositiveId(userId)", roomRead);
+            int roomLookup = source.indexOf("getCurrentRoom()", guard);
+
+            assertTrue(userRead > -1 && roomRead > userRead, handler + " should read user and room ids");
+            assertTrue(guard > roomRead && guard < roomLookup,
+                    handler + " should reject invalid ids before resolving room state");
+        }
+    }
+
+    @Test
+    void rightsMutationHandlersRejectInvalidUserIds() throws Exception {
+        String giveRights = source("RoomUserGiveRightsEvent");
+        String removeRights = source("RoomUserRemoveRightsEvent");
+
+        int giveRead = giveRights.indexOf("int userId = this.packet.readInt()");
+        int giveGuard = giveRights.indexOf("RoomUserInputGuard.isPositiveId(userId)", giveRead);
+        int giveTarget = giveRights.indexOf("room.getHabbo(userId)", giveGuard);
+
+        int removeRead = removeRights.indexOf("int userId = this.packet.readInt()");
+        int removeGuard = removeRights.indexOf("RoomUserInputGuard.isPositiveId(userId)", removeRead);
+        int removeCall = removeRights.indexOf("room.removeRights(userId)", removeGuard);
+
+        assertTrue(giveGuard > giveRead && giveGuard < giveTarget,
+                "give-rights should validate target id before online/friend lookups");
+        assertTrue(removeGuard > removeRead && removeGuard < removeCall,
+                "remove-rights should skip invalid ids before removing rights");
+    }
+
+    @Test
+    void kickPluginEventOnlyFiresAfterPermissionCheck() throws Exception {
+        String source = source("RoomUserKickEvent");
+
+        int userRead = source.indexOf("int userId = this.packet.readInt()");
+        int idGuard = source.indexOf("RoomUserInputGuard.isPositiveId(userId)", userRead);
+        int targetLookup = source.indexOf("room.getHabbo(userId)", idGuard);
+        int permissionCheck = source.indexOf("room.hasRights(this.client.getHabbo())", targetLookup);
+        int event = source.indexOf("new UserKickEvent", permissionCheck);
+        int kick = source.indexOf("room.kickHabbo(target, true)", event);
+
+        assertTrue(idGuard > userRead && idGuard < targetLookup,
+                "kick should validate target id before room lookup");
+        assertTrue(permissionCheck > targetLookup && event > permissionCheck && event < kick,
+                "kick plugin event should only fire once the actor is authorized");
+    }
+
+    @Test
+    void roomActionsRejectUnknownActionIdsBeforeComposersAndWired() throws Exception {
+        String source = source("RoomUserActionEvent");
+
+        int actionRead = source.indexOf("int action = this.packet.readInt()");
+        int guard = source.indexOf("RoomUserInputGuard.isValidAction(action)", actionRead);
+        int composer = source.indexOf("new RoomUserActionComposer", guard);
+        int wired = source.indexOf("WiredManager.triggerUserPerformsAction", guard);
+
+        assertTrue(guard > actionRead && guard < composer,
+                "room actions should reject unknown ids before composing room state");
+        assertTrue(guard < wired,
+                "room actions should reject unknown ids before wired triggers");
+    }
+
+    @Test
+    void helperBoundsExpectedRanges() {
+        assertFalse(RoomUserInputGuard.isPositiveId(0));
+        assertTrue(RoomUserInputGuard.isPositiveId(1));
+        assertFalse(RoomUserInputGuard.isValidAction(-1));
+        assertTrue(RoomUserInputGuard.isValidAction(0));
+        assertTrue(RoomUserInputGuard.isValidAction(7));
+        assertFalse(RoomUserInputGuard.isValidAction(8));
+    }
+}