diff --git a/AGENTS.md b/AGENTS.md index 84ef39ea..df0fdf58 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,26 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/Platform injectors reflect against server and plugin internals (NMS, ViaVersion, ProtocolLib), + * so signature drift arrives as an {@link Error} - the ViaVersion 5.x + * {@code BukkitChannelInitializer.getOriginal()} removal produced a {@link NoSuchMethodError} (see + * {@code SpigotInjectorViaLegacyPathTest}). {@code enable()} used to catch only {@link Exception}, + * so that {@code Error} escaped {@code onEnable()} and the platform disabled the plugin outright. + */ +class ConnectPlatformEnableFailureContainmentTest { + + @Test + void enableContainsAnInjectorError() throws Exception { + PlatformInjector injector = mock(PlatformInjector.class); + NoSuchMethodError error = new NoSuchMethodError( + "io.netty.channel.ChannelInitializer " + + "com.viaversion.viaversion.bukkit.handlers.BukkitChannelInitializer" + + ".getOriginal()"); + when(injector.inject()).thenThrow(error); + ConnectLogger logger = mock(ConnectLogger.class); + + assertFalse(platform(injector, logger).enable(), + "an injector Error must be reported and logged as a failed injection, with " + + "enable() returning false instead of allowing it to propagate"); + verify(logger).error("Failed to inject the packet listener!", error); + } + + /** An injector that merely returns false is still reported the same way. */ + @Test + void enableReportsAFailedInjection() throws Exception { + PlatformInjector injector = mock(PlatformInjector.class); + when(injector.inject()).thenReturn(false); + + assertFalse(platform(injector, mock(ConnectLogger.class)).enable()); + } + + private static ConnectPlatform platform(PlatformInjector injector, ConnectLogger logger) { + // No admission coordinator: enable() never touches it, and a real one would leak its + // cleanup executor because these tests never reach disable(). + return new ConnectPlatform( + mock(ConnectApi.class), injector, logger, mock(Injector.class), null); + } +} diff --git a/spigot/build.gradle.kts b/spigot/build.gradle.kts index 323b0be8..6161b3d9 100644 --- a/spigot/build.gradle.kts +++ b/spigot/build.gradle.kts @@ -20,6 +20,12 @@ dependencies { testImplementation(testFixtures(projects.core)) testImplementation("com.mojang", "authlib", authlibVersion) testImplementation("io.netty", "netty-transport", Versions.nettyVersion) + // Real ViaVersion 5.x on the test classpath so SpigotInjectorViaLegacyPathTest exercises the + // legacy (non-Paper) injector unwrap against the actual BukkitChannelInitializer API. + // viaversion-bukkit publishes no transitive deps, so viaversion-common (which holds the + // ViaChannelInitializer superclass declaring original()) has to be requested explicitly. + testImplementation("com.viaversion", "viaversion-bukkit", Versions.viaVersionVersion) + testImplementation("com.viaversion", "viaversion-common", Versions.viaVersionVersion) testImplementation("dev.folia", "folia-api", Versions.spigotVersion) { attributes { attribute(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, 17) @@ -51,3 +57,6 @@ relocate("org.yaml") provided("com.mojang", "authlib", authlibVersion) provided("com.google.code.gson", "gson", gsonVersion) provided("com.viaversion", "viaversion-bukkit", Versions.viaVersionVersion) +// Since Via 5.0 BukkitChannelInitializer extends ViaChannelInitializer, which lives here, so javac +// needs it on the compile classpath to resolve the type. Provided by the ViaVersion plugin jar. +provided("com.viaversion", "viaversion-common", Versions.viaVersionVersion) diff --git a/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java b/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java index f4500573..79bdaf82 100644 --- a/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java +++ b/spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java @@ -57,6 +57,14 @@ import org.checkerframework.checker.nullness.qual.NonNull; public final class SpigotInjector extends CommonPlatformInjector { + /** + * Names ViaVersion has used for the accessor returning the wrapped, original channel + * initializer, newest first: {@code original()} since Via 5.0, {@code getOriginal()} on Via 4.x. + * + * @see #unwrapViaInitializer(ChannelInitializer) + */ + private static final String[] VIA_ORIGINAL_ACCESSORS = {"original", "getOriginal"}; + private final ConnectLogger logger; private final BedrockIdentityEnforcer bedrockIdentityEnforcer; private final String packetHandlerName; @@ -303,7 +311,7 @@ private ChannelInitializer getChildHandler(ChannelFuture listeningChann childHandler = (ChannelInitializer) childHandlerField.get(handler); // ViaVersion non-Paper-injector workaround so we aren't double-injecting if (isViaVersion && childHandler instanceof BukkitChannelInitializer) { - childHandler = ((BukkitChannelInitializer) childHandler).getOriginal(); + childHandler = unwrapViaInitializer(childHandler); } break; } catch (Exception e) { @@ -319,6 +327,52 @@ private ChannelInitializer getChildHandler(ChannelFuture listeningChann return childHandler; } + /** + * Unwraps ViaVersion's legacy (non-Paper) channel initializer so we initialize the server's own + * initializer instead of Via's wrapper, which would double-inject Via's handlers. + * + *

The accessor was renamed across Via majors: {@code getOriginal()} on Via 4.x, {@code + * original()} since Via 5.0 (pulled up into {@code + * com.viaversion.viaversion.platform.WrappedChannelInitializer}). Connect supports both, so it + * is resolved reflectively by name instead of being bound at compile time. + * + *

Every failure degrades to "unwrap skipped" instead of propagating. With reflective lookup, + * a missing accessor name raises {@link NoSuchMethodException} and is skipped in favour of the + * next known name; if none match, the wrapper is returned with a warning. + */ + @SuppressWarnings("unchecked") + private ChannelInitializer unwrapViaInitializer( + ChannelInitializer viaInitializer) { + for (String accessor : VIA_ORIGINAL_ACCESSORS) { + Method method; + try { + method = viaInitializer.getClass().getMethod(accessor); + } catch (NoSuchMethodException e) { + continue; // older/newer Via, try the next known name + } + try { + method.setAccessible(true); + Object original = method.invoke(viaInitializer); + if (original instanceof ChannelInitializer) { + return (ChannelInitializer) original; + } + logger.warn(accessor + "() on " + viaInitializer.getClass().getName() + + " returned " + original + " instead of a ChannelInitializer, " + + "continuing without unwrapping it."); + return viaInitializer; + } catch (Throwable throwable) { + logger.warn("Failed to unwrap ViaVersion's channel initializer via " + + accessor + "(), continuing without unwrapping it: " + throwable); + return viaInitializer; + } + } + logger.warn("Could not unwrap ViaVersion's channel initializer: none of " + + String.join("(), ", VIA_ORIGINAL_ACCESSORS) + "() exists on " + + viaInitializer.getClass().getName() + ". Continuing without unwrapping it, " + + "please report this with your ViaVersion version."); + return viaInitializer; + } + @Override public void shutdown() { if (this.allServerChannels != null) { diff --git a/spigot/src/main/java/com/minekube/connect/module/SpigotPlatformModule.java b/spigot/src/main/java/com/minekube/connect/module/SpigotPlatformModule.java index 241aea99..8a98d3bd 100644 --- a/spigot/src/main/java/com/minekube/connect/module/SpigotPlatformModule.java +++ b/spigot/src/main/java/com/minekube/connect/module/SpigotPlatformModule.java @@ -109,7 +109,8 @@ public CommonPlatformInjector platformInjector( final String VIAVERSION_DOWNLOAD_URL = "https://ci.viaversion.com/job/ViaVersion/"; boolean isViaVersion = Bukkit.getPluginManager().getPlugin("ViaVersion") != null; if (isViaVersion) { - // Ensure that we have the latest 4.0.0 changes and not an older ViaVersion version + // Ensure that ViaVersion exposes the API introduced in 4.0.0, rather than an older + // version. The injector resolves later ViaVersion accessor changes reflectively. if (getClassSilently("com.viaversion.viaversion.api.ViaManager") == null) { logger.warn("The plugin version of ViaVersion is too old, " + "please install the latest release from {}", diff --git a/spigot/src/test/java/com/minekube/connect/inject/spigot/SpigotInjectorViaLegacyPathTest.java b/spigot/src/test/java/com/minekube/connect/inject/spigot/SpigotInjectorViaLegacyPathTest.java new file mode 100644 index 00000000..8eb7ac05 --- /dev/null +++ b/spigot/src/test/java/com/minekube/connect/inject/spigot/SpigotInjectorViaLegacyPathTest.java @@ -0,0 +1,178 @@ +package com.minekube.connect.inject.spigot; + +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.minekube.connect.api.logger.ConnectLogger; +import com.viaversion.viaversion.bukkit.handlers.BukkitChannelInitializer; +import io.netty.channel.Channel; +import io.netty.channel.ChannelFuture; +import io.netty.channel.ChannelInboundHandlerAdapter; +import io.netty.channel.ChannelInitializer; +import io.netty.channel.ChannelPipeline; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.Collections; +import org.junit.jupiter.api.Test; + +/** + * Regression guard for the ViaVersion 5.x legacy-injector startup failure on plain + * Spigot/CraftBukkit. + * + *

Root cause. ViaVersion only wraps the server's child handler on servers without + * {@code io.papermc.paper.network.ChannelInitializeListener} - i.e. plain Spigot/CraftBukkit, since + * on Paper {@code BukkitViaInjector} registers a Paper channel-initialize listener instead. On that + * legacy path {@code SpigotInjector#getChildHandler} has to unwrap Via's {@link + * BukkitChannelInitializer} so Connect's local channel does not double-inject Via's handlers. It did + * so with {@code getOriginal()}, which Via 5.0 renamed to {@code original()} (pulled up into {@code + * com.viaversion.viaversion.platform.WrappedChannelInitializer}). Connect compiled against Via + * 4.0.0, so this built fine and threw {@link NoSuchMethodError} at runtime. That is an {@link Error}, + * not an {@link Exception}, so it escaped the {@code catch} in {@code ConnectPlatform.enable()}, + * escaped {@code onEnable()} and Bukkit disabled the plugin: Connect never bound its local channel + * and no Connect player could join. Latent since Via 5.0 (2024). + * + *

Why this test is meaningful. It runs against a real ViaVersion 5.x on the test classpath + * (see {@code spigot/build.gradle.kts}) and executes the real private {@code getChildHandler}, so it + * fails with the original {@code NoSuchMethodError} on the pre-fix code and passes on the fix. + * {@link #testClasspathViaIsThePostRenameApi()} keeps it from going vacuously green if the Via test + * dependency is ever downgraded to a pre-rename 4.x. + */ +class SpigotInjectorViaLegacyPathTest { + + /** + * The server's own child initializer, which is what Connect must end up initializing its local + * channel with. {@code LegacyViaInjector} replaces it in the acceptor with a + * {@link BukkitChannelInitializer} wrapping it. + */ + private final ChannelInitializer serverInitializer = new ChannelInitializer() { + @Override + protected void initChannel(Channel ch) { + } + }; + + @Test + void unwrapsViaVersionLegacyChannelInitializer() throws Exception { + ChannelInitializer viaInitializer = + new BukkitChannelInitializer(serverInitializer); + SpigotInjector injector = new SpigotInjector(mock(ConnectLogger.class), true); + + ChannelInitializer resolved = + getChildHandler(injector, listeningChannelWith(viaInitializer)); + + assertSame(serverInitializer, resolved, + "getChildHandler must unwrap ViaVersion's BukkitChannelInitializer down to the " + + "server's own initializer, otherwise Via's handlers get double-injected " + + "into Connect's local channel"); + } + + /** + * Control, mirroring the pre-fix reproduction: with {@code isViaVersion == false} the unwrap + * branch is skipped entirely, so a failure in the test above is attributable to the unwrap and + * not to this harness. + */ + @Test + void keepsViaInitializerWhenViaVersionIsNotDetected() throws Exception { + ChannelInitializer viaInitializer = + new BukkitChannelInitializer(serverInitializer); + SpigotInjector injector = new SpigotInjector(mock(ConnectLogger.class), false); + + ChannelInitializer resolved = + getChildHandler(injector, listeningChannelWith(viaInitializer)); + + assertSame(viaInitializer, resolved); + } + + /** + * A non-Via child handler is returned untouched even when ViaVersion is detected - Via's + * Paper-injector path (and every non-wrapped setup) hits this. + */ + @Test + void returnsPlainChildHandlerUntouched() throws Exception { + SpigotInjector injector = new SpigotInjector(mock(ConnectLogger.class), true); + + ChannelInitializer resolved = + getChildHandler(injector, listeningChannelWith(serverInitializer)); + + assertSame(serverInitializer, resolved); + } + + /** + * The next rename must degrade, not detonate: when no known accessor exists the wrapper is + * returned as-is and a warning is logged, so Connect still enables (with Via's handlers + * double-injected) instead of throwing an {@link Error} out of {@code onEnable()}. + */ + @Test + void unknownViaInitializerApiDegradesToSkippingTheUnwrap() throws Exception { + ConnectLogger logger = mock(ConnectLogger.class); + SpigotInjector injector = new SpigotInjector(logger, true); + Method unwrap = SpigotInjector.class.getDeclaredMethod( + "unwrapViaInitializer", ChannelInitializer.class); + unwrap.setAccessible(true); + + assertSame(serverInitializer, unwrap.invoke(injector, serverInitializer)); + verify(logger).warn(contains("Could not unwrap ViaVersion's channel initializer")); + } + + /** + * Signature-level guard on the test fixture itself: the ViaVersion on the test classpath must be + * a post-rename 5.x, i.e. expose {@code original()} and no longer {@code getOriginal()}. + * Downgrading the test dependency to Via 4.x would make + * {@link #unwrapsViaVersionLegacyChannelInitializer()} pass for the wrong reason. + */ + @Test + void testClasspathViaIsThePostRenameApi() throws Exception { + assertSame(ChannelInitializer.class, + BukkitChannelInitializer.class.getMethod("original").getReturnType(), + "expected ViaVersion 5.x, whose channel initializer exposes original()"); + assertThrows(NoSuchMethodException.class, + () -> BukkitChannelInitializer.class.getMethod("getOriginal"), + "expected ViaVersion 5.x, which removed getOriginal() - the whole point of this " + + "regression test"); + } + + /** + * Fakes the listening channel {@code getChildHandler} walks: a pipeline holding a single handler + * that declares a {@code childHandler} field, like Netty's {@code ServerBootstrapAcceptor}. + */ + private static ChannelFuture listeningChannelWith(ChannelInitializer childHandler) { + ChannelPipeline pipeline = mock(ChannelPipeline.class); + when(pipeline.names()).thenReturn(Collections.singletonList("acceptor")); + when(pipeline.get("acceptor")).thenReturn(new AcceptorStub(childHandler)); + + Channel channel = mock(Channel.class); + when(channel.pipeline()).thenReturn(pipeline); + + ChannelFuture future = mock(ChannelFuture.class); + when(future.channel()).thenReturn(channel); + return future; + } + + @SuppressWarnings("unchecked") + private static ChannelInitializer getChildHandler( + SpigotInjector injector, ChannelFuture listeningChannel) throws Exception { + Method getChildHandler = + SpigotInjector.class.getDeclaredMethod("getChildHandler", ChannelFuture.class); + getChildHandler.setAccessible(true); + try { + return (ChannelInitializer) getChildHandler.invoke(injector, listeningChannel); + } catch (InvocationTargetException e) { + // Pre-fix this is NoSuchMethodError: BukkitChannelInitializer.getOriginal(). Report it + // as the failure it is instead of an opaque InvocationTargetException. + throw new AssertionError("getChildHandler threw " + e.getCause(), e.getCause()); + } + } + + /** Stand-in for Netty's package-private {@code ServerBootstrapAcceptor}. */ + private static final class AcceptorStub extends ChannelInboundHandlerAdapter { + @SuppressWarnings("unused") // read reflectively by SpigotInjector#getChildHandler + private final ChannelInitializer childHandler; + + AcceptorStub(ChannelInitializer childHandler) { + this.childHandler = childHandler; + } + } +}