Packet filtering for bundled packets in 1.19.4 (#2258)

Since Minecraft 1.19.4, the protocol supports bundling consecutive packets to ensure the client processes them in one tick. However, Packet Events are not called for the individual packets in such a bundle in the current dev build of ProtocolLib. For example, no packet events are currently sent for the ENTITY_METADATA packet when an entity is first spawned as the packet is bundled with the ENTITY_SPAWN packet. However, if the entity metadata is changed later on, the event will be called.
This PR proposes to fix this by unpacking the bundled packets and invoking the packet filtering for each packet.

I also want to briefly explain how the bundling works. A bundle starts with a PACKET_DELIMITER (0x00, net.minecraft.network.protocol.BundleDelimiterPacket) packet followed by all packets that should be bundled and finished with another PACKET_DELIMITER (0x00). Within the Netty pipeline, this sequence is transformed into one synthesized packet found in net.minecraft.network.protocol.game.ClientboundBundlePacket, which is essentially just a list of packets. At the stage at which ProtocolLib injects into the clientbound netty pipeline, this packet has not been unpacked yet. Thus, we need to handle the ClientboundBundlePacket, which unfortunately is not registered in ProtocolLib. The fact that two different classes map to the same packet currently requires a dirty remapping in the packet structure modifier.

Author: lukalt

pom.xml

@@ -125,7 +125,7 @@
 
 			<plugin>
 				<artifactId>maven-javadoc-plugin</artifactId>
-				<version>3.4.0</version>
+				<version>3.5.0</version>
 				<configuration>
 					<failOnError>false</failOnError>
 					<encoding>ISO-8859-1</encoding>

src/main/java/com/comphenix/protocol/events/AbstractStructure.java

@@ -1121,6 +1121,12 @@ public <T> StructureModifier<Optional<T>> getOptionals(EquivalentConverter<T> co
         return structureModifier.withType(Optional.class, Converters.optional(converter));
     }
 
+    public StructureModifier<Iterable<PacketContainer>> getPacketBundles() {
+        return structureModifier.withType(Iterable.class, Converters.iterable(
+            BukkitConverters.getPacketContainerConverter(), ArrayList::new, ArrayList::new
+        ));
+    }
+
     /**
      * Represents an equivalent converter for ItemStack arrays.
      * @author Kristian

src/main/java/com/comphenix/protocol/events/PacketEvent.java

@@ -40,6 +40,8 @@
 import org.bukkit.entity.Player;
 import org.bukkit.event.Cancellable;
 
+import javax.annotation.Nullable;
+
 /**
  * Represents a packet sending or receiving event. Changes to the packet will be reflected in the final version to be
  * sent or received. It is also possible to cancel an event.
@@ -70,6 +72,9 @@ public class PacketEvent extends EventObject implements Cancellable {
 	private boolean readOnly;
 	private boolean filtered;
 
+	@Nullable
+	private PacketEvent bundle;
+
 	/**
 	 * Use the static constructors to create instances of this event.
 	 *
@@ -81,27 +86,28 @@ public PacketEvent(Object source) {
 	}
 
 	private PacketEvent(Object source, PacketContainer packet, Player player, boolean serverPacket) {
-		this(source, packet, null, player, serverPacket, true);
+		this(source, packet, null, player, serverPacket, true, null);
 	}
 
 	private PacketEvent(Object source, PacketContainer packet, NetworkMarker marker, Player player, boolean serverPacket,
-			boolean filtered) {
+			boolean filtered, @Nullable PacketEvent bundleEvent) {
 		super(source);
 		this.packet = packet;
 		this.playerReference = new WeakReference<>(player);
 		this.networkMarker = marker;
 		this.serverPacket = serverPacket;
 		this.filtered = filtered;
+		this.bundle = bundleEvent;
 	}
 
-	private PacketEvent(PacketEvent origial, AsyncMarker asyncMarker) {
-		super(origial.source);
-		this.packet = origial.packet;
-		this.playerReference = origial.getPlayerReference();
-		this.cancel = origial.cancel;
-		this.serverPacket = origial.serverPacket;
-		this.filtered = origial.filtered;
-		this.networkMarker = origial.networkMarker;
+	private PacketEvent(PacketEvent original, AsyncMarker asyncMarker) {
+		super(original.source);
+		this.packet = original.packet;
+		this.playerReference = original.getPlayerReference();
+		this.cancel = original.cancel;
+		this.serverPacket = original.serverPacket;
+		this.filtered = original.filtered;
+		this.networkMarker = original.networkMarker;
 		this.asyncMarker = asyncMarker;
 		this.asynchronous = true;
 	}
@@ -128,7 +134,7 @@ public static PacketEvent fromClient(Object source, PacketContainer packet, Play
 	 * @return The event.
 	 */
 	public static PacketEvent fromClient(Object source, PacketContainer packet, NetworkMarker marker, Player client) {
-		return new PacketEvent(source, packet, marker, client, false, true);
+		return new PacketEvent(source, packet, marker, client, false, true, null);
 	}
 
 	/**
@@ -145,7 +151,7 @@ public static PacketEvent fromClient(Object source, PacketContainer packet, Netw
 	 */
 	public static PacketEvent fromClient(Object source, PacketContainer packet, NetworkMarker marker, Player client,
 			boolean filtered) {
-		return new PacketEvent(source, packet, marker, client, false, filtered);
+		return new PacketEvent(source, packet, marker, client, false, filtered, null);
 	}
 
 	/**
@@ -170,7 +176,7 @@ public static PacketEvent fromServer(Object source, PacketContainer packet, Play
 	 * @return The event.
 	 */
 	public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient) {
-		return new PacketEvent(source, packet, marker, recipient, true, true);
+		return new PacketEvent(source, packet, marker, recipient, true, true, null);
 	}
 
 	/**
@@ -187,7 +193,25 @@ public static PacketEvent fromServer(Object source, PacketContainer packet, Netw
 	 */
 	public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient,
 			boolean filtered) {
-		return new PacketEvent(source, packet, marker, recipient, true, filtered);
+		return fromServer(source, packet, marker, recipient, filtered, null);
+	}
+
+	/**
+	 * Creates an event representing a server packet transmission.
+	 * <p>
+	 * If <i>filtered</i> is FALSE, then this event is only processed by packet monitors.
+	 *
+	 * @param source    - the event source.
+	 * @param packet    - the packet.
+	 * @param marker    - the network marker.
+	 * @param recipient - the client that will receieve the packet.
+	 * @param filtered  - whether this packet event is processed by every packet listener.
+	 * @param bundle    - The corresponding packet event of the bundle if this packet is part of a bundle. Otherwise, null.
+	 * @return The event.
+	 */
+	public static PacketEvent fromServer(Object source, PacketContainer packet, NetworkMarker marker, Player recipient,
+			boolean filtered, @Nullable PacketEvent bundle) {
+		return new PacketEvent(source, packet, marker, recipient, true, filtered, bundle);
 	}
 
 	/**
@@ -517,6 +541,15 @@ private void readObject(ObjectInputStream input) throws ClassNotFoundException,
 		}
 	}
 
+	/**
+	 * Returns the packet event corresponding to the bundle if this packet is sent as a part of a bundle, t. Otherwise, null.
+	 * @return Corresponding packet event or null.
+	 */
+	@Nullable
+	public PacketEvent getBundle() {
+		return bundle;
+	}
+
 	@Override
 	public String toString() {
 		return "PacketEvent[player=" + getPlayer() + ", packet=" + packet + "]";

src/main/java/com/comphenix/protocol/injector/SortedPacketListenerList.java

@@ -17,17 +17,25 @@
 
 package com.comphenix.protocol.injector;
 
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 
+import com.comphenix.protocol.PacketType;
 import com.comphenix.protocol.concurrency.AbstractConcurrentListenerMultimap;
 import com.comphenix.protocol.error.ErrorReporter;
 import com.comphenix.protocol.events.ListenerPriority;
+import com.comphenix.protocol.events.PacketContainer;
 import com.comphenix.protocol.events.PacketEvent;
 import com.comphenix.protocol.events.PacketListener;
+import com.comphenix.protocol.injector.packet.PacketRegistry;
+import com.comphenix.protocol.reflect.StructureModifier;
 import com.comphenix.protocol.timing.TimedListenerManager;
 import com.comphenix.protocol.timing.TimedListenerManager.ListenerType;
 import com.comphenix.protocol.timing.TimedTracker;
 
+import javax.annotation.Nullable;
+
 /**
  * Registry of synchronous packet listeners.
  * 
@@ -108,7 +116,7 @@ public void invokePacketRecieving(ErrorReporter reporter, PacketEvent event, Lis
 	 * @param event - the related packet event.
 	 * @param element - the listener to invoke.
 	 */
-	private final void invokeReceivingListener(ErrorReporter reporter, PacketEvent event, PrioritizedListener<PacketListener> element) {
+	private void invokeReceivingListener(ErrorReporter reporter, PacketEvent event, PrioritizedListener<PacketListener> element) {
 		try {
 			event.setReadOnly(element.getPriority() == ListenerPriority.MONITOR);
 			element.getListener().onPacketReceiving(event);
@@ -130,59 +138,63 @@ private final void invokeReceivingListener(ErrorReporter reporter, PacketEvent e
 	 * @param event - the packet event to invoke.
 	 */
 	public void invokePacketSending(ErrorReporter reporter, PacketEvent event) {
-		Collection<PrioritizedListener<PacketListener>> list = getListener(event.getPacketType());
-		
-		if (list == null)
-			return;
-		
-		if (timedManager.isTiming()) {
-			for (PrioritizedListener<PacketListener> element : list) {
-				TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
-				long token = tracker.beginTracking();
-				
-				// Measure and record the execution time
-				invokeSendingListener(reporter, event, element);
-				tracker.endTracking(token, event.getPacketType());
-			}
-		} else {
-			for (PrioritizedListener<PacketListener> element : list) {
-				invokeSendingListener(reporter, event, element);
-			}
-		}
+		invokePacketSending(reporter, event, null);
 	}
 
 	/**
 	 * Invokes the given packet event for every registered listener of the given priority.
 	 * @param reporter - the error reporter that will be used to inform about listener exceptions.
 	 * @param event - the packet event to invoke.
-	 * @param priorityFilter - the required priority for a listener to be invoked.
+	 * @param priorityFilter - the priority for a listener to be invoked. If null is provided, every registered listener will be invoked
 	 */
-	public void invokePacketSending(ErrorReporter reporter, PacketEvent event, ListenerPriority priorityFilter) {
+	public void invokePacketSending(ErrorReporter reporter, PacketEvent event, @Nullable ListenerPriority priorityFilter) {
+		invokeUnpackedPacketSending(reporter, event, priorityFilter);
+		if(event.getPacketType() == PacketType.Play.Server.DELIMITER && !event.isCancelled()) {
+			// unpack the bundle and invoke for each packet in the bundle
+			Iterable<PacketContainer> packets = event.getPacket().getPacketBundles().read(0);
+			List<PacketContainer> outPackets = new ArrayList<>();
+			for(PacketContainer subPacket : packets) {
+				PacketEvent subPacketEvent = PacketEvent.fromServer(this, subPacket, event.getNetworkMarker(), event.getPlayer());
+				invokeUnpackedPacketSending(reporter, subPacketEvent, priorityFilter);
+
+				if(!subPacketEvent.isCancelled()) {
+					outPackets.add(subPacketEvent.getPacket()); // if the packet event has been cancelled, the packet will be removed from the bundle.
+				}
+			}
+			if(packets.iterator().hasNext()) { // are there still packets in this bundle?
+				event.getPacket().getPacketBundles().write(0, outPackets);
+			} else {
+				event.setCancelled(true); // cancel packet if each individual packet has been canceled
+			}
+		}
+	}
+
+	private void invokeUnpackedPacketSending(ErrorReporter reporter, PacketEvent event, @org.jetbrains.annotations.Nullable ListenerPriority priorityFilter) {
 		Collection<PrioritizedListener<PacketListener>> list = getListener(event.getPacketType());
-		
+
 		if (list == null)
 			return;
-		
+
 		if (timedManager.isTiming()) {
 			for (PrioritizedListener<PacketListener> element : list) {
-				if (element.getPriority() == priorityFilter) {
-				TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
-				long token = tracker.beginTracking();
-				
-				// Measure and record the execution time
-				invokeSendingListener(reporter, event, element);
-				tracker.endTracking(token, event.getPacketType());
+				if (priorityFilter == null || element.getPriority() == priorityFilter) {
+					TimedTracker tracker = timedManager.getTracker(element.getListener(), ListenerType.SYNC_SERVER_SIDE);
+					long token = tracker.beginTracking();
+
+					// Measure and record the execution time
+					invokeSendingListener(reporter, event, element);
+					tracker.endTracking(token, event.getPacketType());
 				}
 			}
 		} else {
 			for (PrioritizedListener<PacketListener> element : list) {
-				if (element.getPriority() == priorityFilter) {
+				if (priorityFilter == null || element.getPriority() == priorityFilter) {
 					invokeSendingListener(reporter, event, element);
 				}
 			}
 		}
 	}
-	
+
 	/**
 	 * Invoke a particular sending listener.
 	 * @param reporter - the error reporter.

src/main/java/com/comphenix/protocol/injector/StructureCache.java

@@ -135,6 +135,11 @@ public static StructureModifier<Object> getStructure(final PacketType packetType
 
 		return STRUCTURE_MODIFIER_CACHE.computeIfAbsent(packetType, type -> {
 			Class<?> packetClass = PacketRegistry.getPacketClassFromType(type);
+
+			// We need to map the Bundle Delimiter to the synthetic bundle packet which contains a list of all packets in a bundle
+			if(MinecraftVersion.atOrAbove(MinecraftVersion.FEATURE_PREVIEW_2) && packetClass.equals(MinecraftReflection.getBundleDelimiterClass())) {
+				packetClass = MinecraftReflection.getPackedBundlePacketClass();
+			}
 			return new StructureModifier<>(packetClass, MinecraftReflection.getPacketClass(), true);
 		});
 	}

src/main/java/com/comphenix/protocol/injector/netty/channel/NettyChannelInjector.java

@@ -27,12 +27,7 @@
 import com.comphenix.protocol.reflect.accessors.Accessors;
 import com.comphenix.protocol.reflect.accessors.FieldAccessor;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract;
-import com.comphenix.protocol.utility.ByteBuddyGenerated;
-import com.comphenix.protocol.utility.MinecraftFields;
-import com.comphenix.protocol.utility.MinecraftMethods;
-import com.comphenix.protocol.utility.MinecraftProtocolVersion;
-import com.comphenix.protocol.utility.MinecraftReflection;
-import com.comphenix.protocol.utility.MinecraftVersion;
+import com.comphenix.protocol.utility.*;
 import com.comphenix.protocol.wrappers.WrappedGameProfile;
 import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandler;
@@ -558,7 +553,7 @@ <T> T processOutbound(T action) {
 		}
 
 		// no listener and no marker - no magic :)
-		if (!this.channelListener.hasListener(packet.getClass()) && marker == null) {
+		if (!this.channelListener.hasListener(packet.getClass()) && marker == null && !Util.isBundlePacket(packet.getClass())) {
 			return action;
 		}
 

src/main/java/com/comphenix/protocol/injector/netty/manager/NetworkManagerInjector.java

@@ -28,6 +28,7 @@
 import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract;
 import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
 import com.comphenix.protocol.utility.MinecraftReflection;
+import com.comphenix.protocol.utility.Util;
 import com.comphenix.protocol.wrappers.Pair;
 import io.netty.channel.ChannelFuture;
 import org.bukkit.Server;
@@ -90,7 +91,7 @@ public NetworkManagerInjector(Plugin plugin, Server server, ListenerInvoker list
 	public PacketEvent onPacketSending(Injector injector, Object packet, NetworkMarker marker) {
 		// check if we need to intercept the packet
 		Class<?> packetClass = packet.getClass();
-		if (this.outboundListeners.contains(packetClass) || marker != null) {
+		if (this.outboundListeners.contains(packetClass) || marker != null || Util.isBundlePacket(packetClass)) {
 			// wrap packet and construct the event
 			PacketContainer container = new PacketContainer(PacketRegistry.getPacketType(packetClass), packet);
 			PacketEvent packetEvent = PacketEvent.fromServer(this, container, marker, injector.getPlayer());

src/main/java/com/comphenix/protocol/injector/packet/PacketRegistry.java

@@ -30,6 +30,7 @@
 import com.comphenix.protocol.reflect.fuzzy.FuzzyFieldContract;
 import com.comphenix.protocol.utility.MinecraftReflection;
 import com.comphenix.protocol.utility.MinecraftVersion;
+import com.comphenix.protocol.utility.Util;
 
 /**
  * Static packet registry in Minecraft.
@@ -382,6 +383,9 @@ public static Class<?> getPacketClassFromType(PacketType type) {
 	 * @return The packet type, or NULL if not found.
 	 */
 	public static PacketType getPacketType(Class<?> packet) {
+		if(Util.isBundlePacket(packet)) {
+			return PacketType.Play.Server.DELIMITER;
+		}
 		initialize();
 		return REGISTER.classToType.get(packet);
 	}

src/main/java/com/comphenix/protocol/utility/MinecraftReflection.java

@@ -628,6 +628,14 @@ public static Class<?> getIChatBaseComponentClass() {
 		return getMinecraftClass("network.chat.IChatBaseComponent", "network.chat.IChatbaseComponent", "network.chat.Component", "IChatBaseComponent");
 	}
 
+	public static Class<?> getPackedBundlePacketClass() {
+		return getMinecraftClass("network.protocol.game.ClientboundBundlePacket", "ClientboundBundlePacket");
+	}
+
+	public static Class<?> getBundleDelimiterClass() {
+		return getMinecraftClass("network.protocol.BundleDelimiterPacket","BundleDelimiterPacket");
+	}
+
 	public static Class<?> getIChatBaseComponentArrayClass() {
 		return getArrayClass(getIChatBaseComponentClass());
 	}

src/main/java/com/comphenix/protocol/utility/Util.java

@@ -22,6 +22,7 @@
 public final class Util {
 
 	private static final boolean SPIGOT = classExists("org.spigotmc.SpigotConfig");
+	private static Class<?> cachedBundleClass;
 
 	public static boolean classExists(String className) {
 		try {
@@ -59,4 +60,14 @@ public static boolean isCurrentlyReloading() {
 		}
 		return false;
 	}
+
+	public static boolean isBundlePacket(Class<?> packetClass) {
+		if(!MinecraftVersion.atOrAbove(MinecraftVersion.FEATURE_PREVIEW_2)) {
+			return false;
+		}
+		if(cachedBundleClass == null) {
+			cachedBundleClass = MinecraftReflection.getPackedBundlePacketClass();
+		}
+		return packetClass.equals(cachedBundleClass);
+	}
 }