Fix: Ignore invalid block entity data sent by Java server (#4766)

* Proper block entity checks; ignore invalid block entity data sent by Java server

* fix intelliJ warning about potentially null block state

* Use auto-generated block entity types instead of hardcoding them

* undo some diff

* Update BlockRegistryPopulator.java

* Access block entity type of state by getting the block first

* deprecate JavaBlockState#hasBlockEntity

* Simplify check

* Add type check in JavaBlockEntityDataTranslator, ensure deprecated setBlockEntity() method still sets piston behavior

* nullability annotations

* yeet duplicate check

Author: onebeastchris

api/src/main/java/org/geysermc/geyser/api/block/custom/nonvanilla/JavaBlockState.java

@@ -73,7 +73,10 @@ public interface JavaBlockState {
      * Gets whether the block state has block entity
      * 
      * @return whether the block state has block entity
+     * @deprecated Does not have an effect. If you were using this to
+     * set piston behavior, use {@link #pistonBehavior()} instead.
      */
+    @Deprecated(forRemoval = true)
     boolean hasBlockEntity();
 
     /**
@@ -104,6 +107,11 @@ interface Builder {
 
         Builder pistonBehavior(@Nullable String pistonBehavior);
 
+        /**
+         * @deprecated Does not have an effect. If you were using this to
+         *      * set piston behavior, use {@link #pistonBehavior(String)} instead.
+         */
+        @Deprecated(forRemoval = true)
         Builder hasBlockEntity(boolean hasBlockEntity);
 
         JavaBlockState build();

core/src/main/java/org/geysermc/geyser/level/block/Blocks.java

@@ -16,7 +16,6 @@ public class GeyserJavaBlockState implements JavaBlockState {
     boolean canBreakWithHand;
     String pickItem;
     String pistonBehavior;
-    boolean hasBlockEntity;
 
     private GeyserJavaBlockState(Builder builder) {
         this.identifier = builder.identifier;
@@ -28,7 +27,6 @@ private GeyserJavaBlockState(Builder builder) {
         this.canBreakWithHand = builder.canBreakWithHand;
         this.pickItem = builder.pickItem;
         this.pistonBehavior = builder.pistonBehavior;
-        this.hasBlockEntity = builder.hasBlockEntity;
     }
 
     @Override
@@ -76,9 +74,10 @@ public boolean canBreakWithHand() {
         return pistonBehavior;
     }
 
+    @SuppressWarnings("removal")
     @Override
     public boolean hasBlockEntity() {
-        return hasBlockEntity;
+        return false;
     }
 
     public static class Builder implements JavaBlockState.Builder {
@@ -91,7 +90,6 @@ public static class Builder implements JavaBlockState.Builder {
         private boolean canBreakWithHand;
         private String pickItem;
         private String pistonBehavior;
-        private boolean hasBlockEntity;
 
         @Override
         public Builder identifier(@NonNull String identifier) {
@@ -147,9 +145,13 @@ public Builder pistonBehavior(@Nullable String pistonBehavior) {
             return this;
         }
 
+        @SuppressWarnings("removal")
         @Override
         public Builder hasBlockEntity(boolean hasBlockEntity) {
-            this.hasBlockEntity = hasBlockEntity;
+            // keep the current behavior
+            if (this.pistonBehavior == null && hasBlockEntity) {
+                this.pistonBehavior = "BLOCK";
+            }
             return this;
         }
 

core/src/main/java/org/geysermc/geyser/level/block/GeyserJavaBlockState.java

@@ -29,6 +29,7 @@
 import it.unimi.dsi.fastutil.ints.IntList;
 import net.kyori.adventure.key.Key;
 import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 import org.cloudburstmc.math.vector.Vector3i;
 import org.cloudburstmc.protocol.bedrock.data.definitions.BlockDefinition;
 import org.cloudburstmc.protocol.bedrock.packet.UpdateBlockPacket;
@@ -41,6 +42,7 @@
 import org.geysermc.geyser.registry.BlockRegistries;
 import org.geysermc.geyser.session.GeyserSession;
 import org.geysermc.mcprotocollib.protocol.data.game.item.ItemStack;
+import org.geysermc.mcprotocollib.protocol.data.game.level.block.BlockEntityType;
 import org.intellij.lang.annotations.Subst;
 
 import java.util.*;
@@ -55,7 +57,7 @@ public class Block {
      * Can you harvest this with your hand.
      */
     private final boolean requiresCorrectToolForDrops;
-    private final boolean hasBlockEntity;
+    private final @Nullable BlockEntityType blockEntityType;
     private final float destroyTime;
     private final @NonNull PistonBehavior pushReaction;
     /**
@@ -75,7 +77,7 @@ public class Block {
     public Block(@Subst("empty") String javaIdentifier, Builder builder) {
         this.javaIdentifier = Key.key(javaIdentifier);
         this.requiresCorrectToolForDrops = builder.requiresCorrectToolForDrops;
-        this.hasBlockEntity = builder.hasBlockEntity;
+        this.blockEntityType = builder.blockEntityType;
         this.destroyTime = builder.destroyTime;
         this.pushReaction = builder.pushReaction;
         this.pickItem = builder.pickItem;
@@ -181,7 +183,12 @@ public boolean requiresCorrectToolForDrops() {
     }
 
     public boolean hasBlockEntity() {
-        return hasBlockEntity;
+        return blockEntityType != null;
+    }
+
+    @Nullable
+    public BlockEntityType blockEntityType() {
+        return blockEntityType;
     }
 
     public float destroyTime() {
@@ -227,7 +234,7 @@ public static Builder builder() {
     public static final class Builder {
         private final Map<Property<?>, List<Comparable<?>>> states = new LinkedHashMap<>();
         private boolean requiresCorrectToolForDrops = false;
-        private boolean hasBlockEntity = false;
+        private BlockEntityType blockEntityType = null;
         private PistonBehavior pushReaction = PistonBehavior.NORMAL;
         private float destroyTime;
         private Supplier<Item> pickItem;
@@ -271,8 +278,8 @@ public Builder requiresCorrectToolForDrops() {
             return this;
         }
 
-        public Builder setBlockEntity() {
-            this.hasBlockEntity = true;
+        public Builder setBlockEntity(BlockEntityType blockEntityType) {
+            this.blockEntityType = blockEntityType;
             return this;
         }
 

core/src/main/java/org/geysermc/geyser/level/block/type/Block.java

@@ -65,14 +65,6 @@ public <T extends Comparable<T>> T getValueNullable(Property<T> property) {
         return (T) get(property);
     }
 
-    public boolean getValue(Property<Boolean> property, boolean def) {
-        var value = get(property);
-        if (value == null) {
-            return def;
-        }
-        return (Boolean) value;
-    }
-
     public <T extends Comparable<T>> T getValue(Property<T> property, T def) {
         var value = get(property);
         if (value == null) {

core/src/main/java/org/geysermc/geyser/level/block/type/BlockState.java

@@ -435,9 +435,6 @@ private static void registerJavaBlocks() {
                 if (!javaBlockState.canBreakWithHand()) {
                     builder.requiresCorrectToolForDrops();
                 }
-                if (javaBlockState.hasBlockEntity()) {
-                    builder.setBlockEntity();
-                }
                 String cleanJavaIdentifier = BlockUtils.getCleanIdentifier(javaBlockState.identifier());
                 String pickItem = javaBlockState.pickItem();
                 Block block = new Block(cleanJavaIdentifier, builder) {

core/src/main/java/org/geysermc/geyser/registry/populator/BlockRegistryPopulator.java

@@ -37,7 +37,6 @@
 
 @BlockEntity(type = BlockEntityType.BRUSHABLE_BLOCK)
 public class BrushableBlockEntityTranslator extends BlockEntityTranslator implements RequiresBlockState {
-
     @Override
     public void translateTag(GeyserSession session, NbtMapBuilder bedrockNbt, @Nullable NbtMap javaNbt, BlockState blockState) {
         if (javaNbt == null) {

core/src/main/java/org/geysermc/geyser/translator/level/block/entity/BrushableBlockEntityTranslator.java

@@ -34,7 +34,6 @@
 
 @BlockEntity(type = BlockEntityType.DECORATED_POT)
 public class DecoratedPotBlockEntityTranslator extends BlockEntityTranslator {
-
     @Override
     public void translateTag(GeyserSession session, NbtMapBuilder bedrockNbt, NbtMap javaNbt, BlockState blockState) {
         if (javaNbt == null) {

core/src/main/java/org/geysermc/geyser/translator/level/block/entity/DecoratedPotBlockEntityTranslator.java

@@ -27,7 +27,6 @@
 
 import org.cloudburstmc.nbt.NbtMap;
 import org.cloudburstmc.nbt.NbtMapBuilder;
-import org.geysermc.geyser.level.block.Blocks;
 import org.geysermc.geyser.level.block.property.ChestType;
 import org.geysermc.geyser.level.block.property.Properties;
 import org.geysermc.geyser.level.block.type.BlockState;
@@ -40,12 +39,8 @@
  */
 @BlockEntity(type = { BlockEntityType.CHEST, BlockEntityType.TRAPPED_CHEST })
 public class DoubleChestBlockEntityTranslator extends BlockEntityTranslator implements RequiresBlockState {
-
     @Override
     public void translateTag(GeyserSession session, NbtMapBuilder bedrockNbt, NbtMap javaNbt, BlockState blockState) {
-        if (!(blockState.is(Blocks.CHEST) || blockState.is(Blocks.TRAPPED_CHEST))) {
-            return;
-        }
         if (blockState.getValue(Properties.CHEST_TYPE) != ChestType.SINGLE) {
             int x = (int) bedrockNbt.get("x");
             int z = (int) bedrockNbt.get("z");

core/src/main/java/org/geysermc/geyser/translator/level/block/entity/DoubleChestBlockEntityTranslator.java

@@ -30,7 +30,6 @@
 import org.cloudburstmc.nbt.NbtMapBuilder;
 import org.geysermc.geyser.level.block.property.Properties;
 import org.geysermc.geyser.level.block.type.BlockState;
-import org.geysermc.geyser.level.physics.Direction;
 import org.geysermc.geyser.session.GeyserSession;
 import org.geysermc.geyser.translator.inventory.ShulkerInventoryTranslator;
 import org.geysermc.mcprotocollib.protocol.data.game.level.block.BlockEntityType;
@@ -43,6 +42,6 @@ public class ShulkerBoxBlockEntityTranslator extends BlockEntityTranslator imple
      */
     @Override
     public void translateTag(GeyserSession session, NbtMapBuilder bedrockNbt, @Nullable NbtMap javaNbt, BlockState blockState) {
-        bedrockNbt.putByte("facing", (byte) blockState.getValue(Properties.FACING, Direction.UP).ordinal());
+        bedrockNbt.putByte("facing", (byte) blockState.getValue(Properties.FACING).ordinal());
     }
 }