Explicitly rewrite legacy plugin message channels for 1.13+ clients.

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java

@@ -671,8 +671,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player {
 
     // Otherwise, we need to see if the player already knows this channel or it's known by the
     // proxy.
-    return minecraftOrFmlMessage || knownChannels.contains(message.getChannel())
+    return minecraftOrFmlMessage || knownChannels.contains(message.getChannel());
-            || server.getChannelRegistrar().registered(message.getChannel());
   }
 
   private class ConnectionRequestBuilderImpl implements ConnectionRequestBuilder {

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessage.java

@@ -1,11 +1,13 @@
 package com.velocitypowered.proxy.protocol.packet;
 
 import static com.velocitypowered.proxy.connection.VelocityConstants.EMPTY_BYTE_ARRAY;
+import static com.velocitypowered.proxy.protocol.util.PluginMessageUtil.transformLegacyToModernChannel;
 
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.proxy.connection.MinecraftSessionHandler;
 import com.velocitypowered.proxy.protocol.MinecraftPacket;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.util.PluginMessageUtil;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -38,13 +40,16 @@ public class PluginMessage implements MinecraftPacket {
   public String toString() {
     return "PluginMessage{"
         + "channel='" + channel + '\''
-        + ", data=<removed>" +
+        + ", data=<removed>"
         + '}';
   }
 
   @Override
   public void decode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
     this.channel = ProtocolUtils.readString(buf);
+    if (version.compareTo(ProtocolVersion.MINECRAFT_1_13) < 0) {
+      this.channel = transformLegacyToModernChannel(this.channel);
+    }
     this.data = new byte[buf.readableBytes()];
     buf.readBytes(data);
   }
@@ -54,7 +59,8 @@ public class PluginMessage implements MinecraftPacket {
     if (channel == null) {
       throw new IllegalStateException("Channel is not specified.");
     }
-    ProtocolUtils.writeString(buf, channel);
+    ProtocolUtils.writeString(buf, version.compareTo(ProtocolVersion.MINECRAFT_1_13) >= 0
+        ? channel : transformLegacyToModernChannel(this.channel));
     buf.writeBytes(data);
   }
 

proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java

@@ -14,6 +14,8 @@ import io.netty.buffer.Unpooled;
 import java.nio.charset.StandardCharsets;
 import java.util.Collection;
 import java.util.List;
+import java.util.Locale;
+import java.util.regex.Pattern;
 
 public class PluginMessageUtil {
 
@@ -143,4 +145,40 @@ public class PluginMessageUtil {
     return newMsg;
   }
 
+  private static final Pattern INVALID_IDENTIFIER_REGEX = Pattern.compile("[^a-z0-9\\-_]*");
+
+  /**
+   * Transform a plugin message channel from a "legacy" (<1.13) form to a modern one.
+   * @param name the existing name
+   * @return the new name
+   */
+  public static String transformLegacyToModernChannel(String name) {
+    checkNotNull(name, "name");
+
+    if (name.indexOf(':') != -1) {
+      // Probably valid. We won't check this for now and go on faith.
+      return name;
+    }
+
+    // Before falling into the fallback, explicitly rewrite certain messages.
+    switch (name) {
+      case REGISTER_CHANNEL_LEGACY:
+        return REGISTER_CHANNEL;
+      case UNREGISTER_CHANNEL_LEGACY:
+        return UNREGISTER_CHANNEL;
+      case BRAND_CHANNEL_LEGACY:
+        return BRAND_CHANNEL;
+      case "BungeeCord":
+        // This is a special historical case we are compelled to support for the benefit of
+        // BungeeQuack.
+        return "bungeecord:main";
+      default:
+        // This is very likely a legacy name, so transform it. Velocity uses the same scheme as
+        // BungeeCord does to transform channels, but also removes clearly invalid characters as
+        // well.
+        String lower = name.toLowerCase(Locale.ROOT);
+        return "legacy:" + INVALID_IDENTIFIER_REGEX.matcher(lower).replaceAll("");
+    }
+  }
+
 }

proxy/src/main/java/com/velocitypowered/proxy/util/VelocityChannelRegistrar.java

@@ -1,15 +1,15 @@
 package com.velocitypowered.proxy.util;
 
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.ChannelRegistrar;
 import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
 
-import java.util.ArrayList;
+import com.velocitypowered.proxy.protocol.util.PluginMessageUtil;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -28,7 +28,13 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
     }
 
     for (ChannelIdentifier identifier : identifiers) {
+      if (identifier instanceof MinecraftChannelIdentifier) {
         identifierMap.put(identifier.getId(), identifier);
+      } else {
+        String rewritten = PluginMessageUtil.transformLegacyToModernChannel(identifier.getId());
+        identifierMap.put(identifier.getId(), identifier);
+        identifierMap.put(rewritten, identifier);
+      }
     }
   }
 
@@ -41,12 +47,22 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
     }
 
     for (ChannelIdentifier identifier : identifiers) {
+      if (identifier instanceof MinecraftChannelIdentifier) {
         identifierMap.remove(identifier.getId());
+      } else {
+        String rewritten = PluginMessageUtil.transformLegacyToModernChannel(identifier.getId());
+        identifierMap.remove(identifier.getId());
+        identifierMap.remove(rewritten);
+      }
     }
   }
 
-  public Collection<String> getIdsForLegacyConnections() {
+  public Collection<String> getLegacyChannelIds() {
-    return ImmutableList.copyOf(identifierMap.keySet());
+    Collection<String> ids = new HashSet<>();
+    for (ChannelIdentifier value : identifierMap.values()) {
+      ids.add(value.getId());
+    }
+    return ids;
   }
 
   /**
@@ -55,19 +71,17 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
    * @return the channel IDs for Minecraft 1.13 and above
    */
   public Collection<String> getModernChannelIds() {
-    Collection<String> ids = new ArrayList<>();
+    Collection<String> ids = new HashSet<>();
     for (ChannelIdentifier value : identifierMap.values()) {
       if (value instanceof MinecraftChannelIdentifier) {
         ids.add(value.getId());
+      } else {
+        ids.add(PluginMessageUtil.transformLegacyToModernChannel(value.getId()));
       }
     }
     return ids;
   }
 
-  public boolean registered(String id) {
-    return identifierMap.containsKey(id);
-  }
-
   public @Nullable ChannelIdentifier getFromId(String id) {
     return identifierMap.get(id);
   }
@@ -81,6 +95,6 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
     if (protocolVersion.compareTo(ProtocolVersion.MINECRAFT_1_13) >= 0) {
       return getModernChannelIds();
     }
-    return getIdsForLegacyConnections();
+    return getLegacyChannelIds();
   }
 }

proxy/src/test/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtilTest.java

@@ -0,0 +1,36 @@
+package com.velocitypowered.proxy.protocol.util;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import org.junit.jupiter.api.Test;
+
+class PluginMessageUtilTest {
+
+  @Test
+  void transformLegacyToModernChannelWorksWithModern() {
+    assertEquals("minecraft:brand", PluginMessageUtil
+        .transformLegacyToModernChannel("minecraft:brand"));
+    assertEquals("velocity:test", PluginMessageUtil
+        .transformLegacyToModernChannel("velocity:test"));
+  }
+
+  @Test
+  void transformLegacyToModernChannelRewritesSpecialCases() {
+    assertEquals("minecraft:brand", PluginMessageUtil
+        .transformLegacyToModernChannel("MC|Brand"));
+    assertEquals("minecraft:register", PluginMessageUtil
+        .transformLegacyToModernChannel("REGISTER"));
+    assertEquals("minecraft:unregister", PluginMessageUtil
+        .transformLegacyToModernChannel("UNREGISTER"));
+    assertEquals("bungeecord:main", PluginMessageUtil
+        .transformLegacyToModernChannel("BungeeCord"));
+  }
+
+  @Test
+  void transformLegacyToModernChannelRewritesGeneral() {
+    assertEquals("legacy:example", PluginMessageUtil
+        .transformLegacyToModernChannel("Example"));
+    assertEquals("legacy:pskeepalive", PluginMessageUtil
+        .transformLegacyToModernChannel("PS|KeepAlive"));
+  }
+}

proxy/src/test/java/com/velocitypowered/proxy/util/VelocityChannelRegistrarTest.java

@@ -0,0 +1,55 @@
+package com.velocitypowered.proxy.util;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import com.google.common.collect.ImmutableSet;
+import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
+import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
+import org.junit.jupiter.api.Test;
+
+class VelocityChannelRegistrarTest {
+
+  private static final MinecraftChannelIdentifier MODERN = MinecraftChannelIdentifier
+      .create("velocity", "test");
+  private static final LegacyChannelIdentifier SIMPLE_LEGACY =
+      new LegacyChannelIdentifier("VelocityTest");
+
+  private static final MinecraftChannelIdentifier MODERN_SPECIAL_REMAP = MinecraftChannelIdentifier
+      .create("bungeecord", "main");
+  private static final LegacyChannelIdentifier SPECIAL_REMAP_LEGACY =
+      new LegacyChannelIdentifier("BungeeCord");
+
+  private static final String SIMPLE_LEGACY_REMAPPED = "legacy:velocitytest";
+
+  @Test
+  void register() {
+    VelocityChannelRegistrar registrar = new VelocityChannelRegistrar();
+    registrar.register(MODERN, SIMPLE_LEGACY);
+
+    // Two channels cover the modern channel (velocity:test) and the legacy-mapped channel
+    // (legacy:velocitytest). Make sure they're what we expect.
+    assertEquals(ImmutableSet.of(MODERN.getId(), SIMPLE_LEGACY_REMAPPED), registrar.getModernChannelIds());
+    assertEquals(ImmutableSet.of(SIMPLE_LEGACY.getId(), MODERN.getId()), registrar.getLegacyChannelIds());
+  }
+
+  @Test
+  void registerSpecialRewrite() {
+    VelocityChannelRegistrar registrar = new VelocityChannelRegistrar();
+    registrar.register(SPECIAL_REMAP_LEGACY, MODERN_SPECIAL_REMAP);
+
+    // This one, just one channel for the modern case.
+    assertEquals(ImmutableSet.of(MODERN_SPECIAL_REMAP.getId()), registrar.getModernChannelIds());
+    assertEquals(ImmutableSet.of(MODERN_SPECIAL_REMAP.getId(), SPECIAL_REMAP_LEGACY.getId()),
+        registrar.getLegacyChannelIds());
+  }
+
+  @Test
+  void unregister() {
+    VelocityChannelRegistrar registrar = new VelocityChannelRegistrar();
+    registrar.register(MODERN, SIMPLE_LEGACY);
+    registrar.unregister(SIMPLE_LEGACY);
+
+    assertEquals(ImmutableSet.of(MODERN.getId()), registrar.getModernChannelIds());
+    assertEquals(ImmutableSet.of(MODERN.getId()), registrar.getLegacyChannelIds());
+  }
+}