From d2a366621b2518db462f60e4ea6b068fc186547c Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 17:37:50 +0200 Subject: [PATCH 01/10] feat(maven): fail-loud guard against proxy forwarding / online mode in provisioning FULL_LOGIN bots complete a real offline-mode login over loopback TCP, so a config overlay that flips online-mode=true (server.properties), settings .bungeecord: true (spigot.yml), or proxies.velocity.enabled: true (config/paper-global.yml) would silently break every full-login join with a timeout. LoopbackLoginGuard validates the fully materialized target server directory (after overlay application) at the end of prepare-server and fails the build with an actionable message naming FULL_LOGIN instead. First slice of the S6 PR2 validation matrix: the guard hardens the provisioning invariant the whole matrix relies on (report section 4.5). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../mojo/prepareserver/PrepareServerMojo.java | 4 + .../provisioning/LoopbackLoginGuard.java | 170 ++++++++++++++++++ .../provisioning/LoopbackLoginGuardTest.java | 156 ++++++++++++++++ 3 files changed, 330 insertions(+) create mode 100644 lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java create mode 100644 lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java index 9c991f1a..4dac0141 100644 --- a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/mojo/prepareserver/PrepareServerMojo.java @@ -2,6 +2,7 @@ import lombok.AllArgsConstructor; import lombok.NoArgsConstructor; +import nl.pim16aap2.lightkeeper.maven.provisioning.LoopbackLoginGuard; import nl.pim16aap2.lightkeeper.maven.provisioning.PluginArtifactSpec; import nl.pim16aap2.lightkeeper.maven.provisioning.ResolvedPluginArtifact; import nl.pim16aap2.lightkeeper.maven.provisioning.ServerAssetInstaller; @@ -209,6 +210,9 @@ public void execute() final Path targetServerDirectory = serverProvider.targetServerDirectoryPath(); installServerAssets(targetServerDirectory, executionContext, executionContext.pluginArtifactSpecs()); + // Runs after ALL provisioning mutations (base copy, plugins, overlay): a config overlay is the one + // vector through which online-mode/proxy-forwarding could enter and silently break FULL_LOGIN joins. + LoopbackLoginGuard.validate(targetServerDirectory, getLog()); final RuntimeManifest runtimeManifest = createRuntimeManifest( executionContext.normalizedServerType(), diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java new file mode 100644 index 00000000..06242f29 --- /dev/null +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java @@ -0,0 +1,170 @@ +package nl.pim16aap2.lightkeeper.maven.provisioning; + +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.Log; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Locale; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Fail-loud guard against server configurations that would break {@code FULL_LOGIN} loopback logins. + * + *

LightKeeper's full-login bots complete a real offline-mode login over loopback TCP against the provisioned + * server. Three configuration switches silently break that pipeline for every bot: {@code online-mode=true} + * ({@code server.properties}) makes the server demand Mojang session authentication, and + * {@code settings.bungeecord: true} ({@code spigot.yml}) or {@code proxies.velocity.enabled: true} + * ({@code config/paper-global.yml}) make the server expect proxy-forwarded handshakes that a direct loopback + * client never sends. LightKeeper's own provisioning never writes any of these, but a user config overlay could + * flip them, so this guard validates the fully materialized target server directory — after all overlays are + * applied — and fails the build with an actionable message instead of letting every full-login join time out. + * + *

The checks are deliberate line-level scans, not a YAML parse: the goal is failing loudly on the known-bad + * switches in the standard configuration shapes those files ship with, with zero new dependencies. Commented-out + * lines are ignored; files that do not exist are skipped. + */ +public final class LoopbackLoginGuard +{ + /** + * Matches an active {@code bungeecord: true} entry in {@code spigot.yml} (the key only exists under the + * top-level {@code settings} section in stock Spigot configurations), with an optional trailing comment. + */ + private static final Pattern SPIGOT_BUNGEECORD_ENABLED_PATTERN = + Pattern.compile("^\\s*bungeecord:\\s*true\\s*(?:#.*)?$"); + + /** + * Matches the opening of the {@code velocity} section in {@code config/paper-global.yml}, capturing its + * indentation so the guard can tell when the section ends. + */ + private static final Pattern VELOCITY_SECTION_PATTERN = Pattern.compile("^(\\s*)velocity:\\s*(?:#.*)?$"); + + /** + * Matches an active {@code enabled: true} entry (inside the {@code velocity} section), with an optional + * trailing comment. + */ + private static final Pattern VELOCITY_ENABLED_PATTERN = Pattern.compile("^\\s*enabled:\\s*true\\s*(?:#.*)?$"); + + private LoopbackLoginGuard() + { + } + + /** + * Validates a fully materialized target server directory for {@code FULL_LOGIN} compatibility. + * + *

Must run after every provisioning mutation (base-server copy, plugin installs, config overlay), because + * the overlay is the mechanism through which the forbidden settings could enter. + * + * @param targetServerDirectory + * The materialized server directory to validate. + * @param log + * Maven log sink for the pass confirmation. + * @throws MojoExecutionException + * If a configuration file enables online mode or proxy forwarding, or a present file cannot be read. + */ + public static void validate(Path targetServerDirectory, Log log) + throws MojoExecutionException + { + validateServerProperties(targetServerDirectory.resolve("server.properties")); + validateSpigotConfiguration(targetServerDirectory.resolve("spigot.yml")); + validatePaperGlobalConfiguration(targetServerDirectory.resolve("config").resolve("paper-global.yml")); + log.info("LK_GUARD: Loopback-login guard passed: offline mode, no proxy forwarding."); + } + + private static void validateServerProperties(Path serverPropertiesFile) + throws MojoExecutionException + { + for (final String line : readLinesIfPresent(serverPropertiesFile)) + { + final String trimmed = line.trim(); + if (trimmed.startsWith("#") || !trimmed.startsWith("online-mode=")) + continue; + + final String value = trimmed.substring("online-mode=".length()).trim().toLowerCase(Locale.ROOT); + if ("true".equals(value)) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' sets 'online-mode=true'. FULL_LOGIN bots complete a " + + "real offline-mode login over loopback TCP; online mode makes the server demand Mojang " + + "session authentication and breaks every full-login join. Remove the override " + + "(LightKeeper provisions 'online-mode=false').") + .formatted(serverPropertiesFile) + ); + } + } + } + + private static void validateSpigotConfiguration(Path spigotConfigurationFile) + throws MojoExecutionException + { + for (final String line : readLinesIfPresent(spigotConfigurationFile)) + { + if (SPIGOT_BUNGEECORD_ENABLED_PATTERN.matcher(line).matches()) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' enables BungeeCord proxy forwarding " + + "('bungeecord: true'). The server would expect proxy-forwarded handshakes, which breaks " + + "FULL_LOGIN loopback logins. Remove the setting from the config overlay.") + .formatted(spigotConfigurationFile) + ); + } + } + } + + private static void validatePaperGlobalConfiguration(Path paperGlobalConfigurationFile) + throws MojoExecutionException + { + int velocitySectionIndent = -1; + for (final String line : readLinesIfPresent(paperGlobalConfigurationFile)) + { + final String trimmed = line.trim(); + if (trimmed.isEmpty() || trimmed.startsWith("#")) + continue; + + final int indent = line.length() - line.stripLeading().length(); + if (velocitySectionIndent >= 0 && indent <= velocitySectionIndent) + velocitySectionIndent = -1; + + final Matcher velocitySectionMatcher = VELOCITY_SECTION_PATTERN.matcher(line); + if (velocitySectionMatcher.matches()) + { + velocitySectionIndent = velocitySectionMatcher.group(1).length(); + continue; + } + + if (velocitySectionIndent >= 0 && VELOCITY_ENABLED_PATTERN.matcher(line).matches()) + { + throw new MojoExecutionException( + ("Provisioned server configuration '%s' enables Velocity proxy forwarding " + + "('proxies.velocity.enabled: true'). The server would expect proxy-forwarded handshakes, " + + "which breaks FULL_LOGIN loopback logins. Remove the setting from the config overlay.") + .formatted(paperGlobalConfigurationFile) + ); + } + } + } + + private static List readLinesIfPresent(Path configurationFile) + throws MojoExecutionException + { + if (Files.notExists(configurationFile)) + return List.of(); + + try + { + return Files.readAllLines(configurationFile, StandardCharsets.UTF_8); + } + catch (IOException exception) + { + throw new MojoExecutionException( + "Failed to read provisioned server configuration '%s' for the loopback-login guard." + .formatted(configurationFile), + exception + ); + } + } +} diff --git a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java new file mode 100644 index 00000000..46180d58 --- /dev/null +++ b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java @@ -0,0 +1,156 @@ +package nl.pim16aap2.lightkeeper.maven.provisioning; + +import org.apache.maven.plugin.MojoExecutionException; +import org.apache.maven.plugin.logging.SystemStreamLog; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class LoopbackLoginGuardTest +{ + @Test + void validate_shouldPassOnDefaultProvisionedConfiguration(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties(serverDirectory, "online-mode=false", "network-compression-threshold=256"); + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: false"); + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: false", + " online-mode: true" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldPassWhenOptionalConfigurationFilesAreMissing(@TempDir Path serverDirectory) + { + // setup — an empty directory: nothing to validate must mean nothing to fail on. + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldFailWhenOnlineModeIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties(serverDirectory, "server-port=25565", "online-mode=TRUE"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("online-mode=true") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenSpigotBungeecordForwardingIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: true # enabled by overlay"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("BungeeCord") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenPaperVelocityForwardingIsEnabled(@TempDir Path serverDirectory) + throws Exception + { + // setup + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: true", + " secret: hunter2" + ); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("Velocity") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldIgnoreEnabledFlagsOutsideTheVelocitySection(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'enabled: true' belongs to a sibling section that starts after the velocity block ends. + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: false", + " bungee-cord:", + " enabled: true", + "watchdog:", + " enabled: true" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + @Test + void validate_shouldIgnoreCommentedOutForwardingSettings(@TempDir Path serverDirectory) + throws Exception + { + // setup + writeServerProperties(serverDirectory, "# online-mode=true", "online-mode=false"); + writeSpigotConfiguration(serverDirectory, "settings:", " # bungeecord: true"); + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " # enabled: true", + " enabled: false" + ); + + // execute + verify + assertThatCode(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .doesNotThrowAnyException(); + } + + private static void writeServerProperties(Path serverDirectory, String... lines) + throws IOException + { + Files.write(serverDirectory.resolve("server.properties"), List.of(lines)); + } + + private static void writeSpigotConfiguration(Path serverDirectory, String... lines) + throws IOException + { + Files.write(serverDirectory.resolve("spigot.yml"), List.of(lines)); + } + + private static void writePaperGlobalConfiguration(Path serverDirectory, String... lines) + throws IOException + { + final Path configDirectory = serverDirectory.resolve("config"); + Files.createDirectories(configDirectory); + Files.write(configDirectory.resolve("paper-global.yml"), List.of(lines)); + } +} From 735e4cbfcccb3ddc99229d8f6e16d665ccd2de6d Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 17:38:20 +0200 Subject: [PATCH 02/10] feat(testplugin): add /lktestlocale locale observable Logs the invoking player's Player#getLocale in a deterministic single-line format (LK_LOCALE name=... locale=...). The full-login locale IT asserts the server-side view of the configuration-phase ClientInformation locale from the captured console output, because full-login bots have no embedded channel to drain received messages from. Second slice of the S6 PR2 validation matrix: the observable for the withLocale (LK-12) assertion. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../LightkeeperSpigotTestPlugin.java | 10 +++ .../LkTestLocaleCommandExecutor.java | 70 +++++++++++++++ .../src/main/resources/plugin.yml | 3 + .../LkTestLocaleCommandExecutorTest.java | 89 +++++++++++++++++++ 4 files changed, 172 insertions(+) create mode 100644 lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java create mode 100644 lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java diff --git a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java index c49de887..bc4e43a3 100644 --- a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java +++ b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LightkeeperSpigotTestPlugin.java @@ -32,6 +32,10 @@ public final class LightkeeperSpigotTestPlugin extends JavaPlugin implements Lis * Command name used to provoke log events for error-capture integration tests. */ private static final String ERROR_COMMAND_NAME = "lktesterror"; + /** + * Command name used to log the invoking player's client locale for full-login locale integration tests. + */ + private static final String LOCALE_COMMAND_NAME = "lktestlocale"; /** * Prefix used for deterministic block interaction messages. */ @@ -57,8 +61,14 @@ public void onEnable() throw new IllegalStateException( "Required command '/%s' is not declared in plugin metadata.".formatted(ERROR_COMMAND_NAME)); + final PluginCommand localeCommand = getCommand(LOCALE_COMMAND_NAME); + if (localeCommand == null) + throw new IllegalStateException( + "Required command '/%s' is not declared in plugin metadata.".formatted(LOCALE_COMMAND_NAME)); + pluginCommand.setExecutor(new LkTestGuiCommandExecutor(guiMenuService)); errorCommand.setExecutor(new LkTestErrorCommandExecutor(getLogger())); + localeCommand.setExecutor(new LkTestLocaleCommandExecutor(getLogger())); Bukkit.getPluginManager().registerEvents(this, this); } diff --git a/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java new file mode 100644 index 00000000..ec1eb09a --- /dev/null +++ b/lightkeeper-spigot-test-plugin/src/main/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutor.java @@ -0,0 +1,70 @@ +package nl.pim16aap2.lightkeeper.spigot.testplugin; + +import org.bukkit.command.Command; +import org.bukkit.command.CommandExecutor; +import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; + +import java.util.Objects; +import java.util.logging.Logger; + +/** + * Handles {@code /lktestlocale} command execution for the standalone test plugin. + * + *

Logs the invoking player's client locale (as the server sees it via {@code Player#getLocale()}) in a + * deterministic single-line format. Integration tests use this to assert that the locale a {@code FULL_LOGIN} + * bot sent during the configuration phase actually reached the server, by matching the line in the captured + * console output. + */ +final class LkTestLocaleCommandExecutor implements CommandExecutor +{ + /** + * Marker prefix of the logged locale line so tests can identify it in the console output. + */ + static final String LOCALE_MESSAGE_PREFIX = "LK_LOCALE"; + /** + * Reply sent when the command is not invoked by a player (only players have a client locale). + */ + static final String PLAYERS_ONLY_MESSAGE = "Only players can use /lktestlocale."; + + /** + * Logger the locale line is emitted through. + */ + private final Logger logger; + + /** + * @param logger + * Logger to emit the locale line through (the owning plugin's logger). + */ + LkTestLocaleCommandExecutor(Logger logger) + { + this.logger = Objects.requireNonNull(logger, "logger"); + } + + /** + * Logs the sender's client locale, or replies with a hint for non-player senders. + * + * @param sender + * Sender that invoked the command. + * @param command + * Command metadata. + * @param label + * Used command label. + * @param args + * Command arguments (ignored). + * @return + * Always {@code true} because this executor handles all command outcomes directly. + */ + @Override + public boolean onCommand(CommandSender sender, Command command, String label, String[] args) + { + if (!(sender instanceof Player player)) + { + sender.sendMessage(PLAYERS_ONLY_MESSAGE); + return true; + } + + logger.info("%s name=%s locale=%s".formatted(LOCALE_MESSAGE_PREFIX, player.getName(), player.getLocale())); + return true; + } +} diff --git a/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml b/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml index df3e7f1b..d79171b1 100644 --- a/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml +++ b/lightkeeper-spigot-test-plugin/src/main/resources/plugin.yml @@ -12,6 +12,9 @@ commands: lktesterror: description: Emits a SEVERE or WARNING log event for LightKeeper error-capture integration tests usage: /lktesterror + lktestlocale: + description: Logs the invoking player's client locale for LightKeeper full-login locale integration tests + usage: /lktestlocale permissions: lightkeeper.testplugin.lktestgui: description: Allows opening the LightKeeper test GUI. diff --git a/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java b/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java new file mode 100644 index 00000000..36288272 --- /dev/null +++ b/lightkeeper-spigot-test-plugin/src/test/java/nl/pim16aap2/lightkeeper/spigot/testplugin/LkTestLocaleCommandExecutorTest.java @@ -0,0 +1,89 @@ +package nl.pim16aap2.lightkeeper.spigot.testplugin; + +import org.bukkit.command.Command; +import org.bukkit.command.CommandSender; +import org.bukkit.entity.Player; +import org.junit.jupiter.api.Test; + +import java.util.Objects; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class LkTestLocaleCommandExecutorTest +{ + @Test + void onCommand_shouldLogPlayerNameAndLocaleForPlayerSender() + { + // setup + final AtomicReference captured = new AtomicReference<>(); + final Logger logger = loggerWithHandler(captured); + final LkTestLocaleCommandExecutor executor = new LkTestLocaleCommandExecutor(logger); + final Player player = mock(); + when(player.getName()).thenReturn("lklocale"); + when(player.getLocale()).thenReturn("de_de"); + final Command command = mock(); + + // execute + final boolean handled = executor.onCommand(player, command, "lktestlocale", new String[0]); + + // verify + assertThat(handled).isTrue(); + final LogRecord record = Objects.requireNonNull(captured.get(), "record"); + assertThat(record.getLevel()).isEqualTo(Level.INFO); + assertThat(record.getMessage()).isEqualTo("LK_LOCALE name=lklocale locale=de_de"); + } + + @Test + void onCommand_shouldSendPlayersOnlyMessageForNonPlayerSender() + { + // setup + final AtomicReference captured = new AtomicReference<>(); + final Logger logger = loggerWithHandler(captured); + final LkTestLocaleCommandExecutor executor = new LkTestLocaleCommandExecutor(logger); + final CommandSender sender = mock(); + final Command command = mock(); + + // execute + final boolean handled = executor.onCommand(sender, command, "lktestlocale", new String[0]); + + // verify + assertThat(handled).isTrue(); + verify(sender).sendMessage(LkTestLocaleCommandExecutor.PLAYERS_ONLY_MESSAGE); + assertThat(captured.get()).isNull(); + } + + private static Logger loggerWithHandler(AtomicReference captured) + { + final Logger logger = + Logger.getLogger(LkTestLocaleCommandExecutorTest.class.getName() + "." + captured.hashCode()); + logger.setUseParentHandlers(false); + logger.setLevel(Level.ALL); + logger.addHandler(new Handler() + { + @Override + public void publish(LogRecord record) + { + captured.set(record); + } + + @Override + public void flush() + { + } + + @Override + public void close() + { + } + }); + return logger; + } +} From 618e1a87b0823f87de6491cf1ae7c8dc645cf8ea Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 17:38:20 +0200 Subject: [PATCH 03/10] test(integration): full-login event-order, denial, and quit/rejoin matrix Covers on both distros: the documented AsyncPlayerPreLoginEvent -> PlayerLoginEvent -> PlayerJoinEvent order via tick stamps (the Paper/Spigot login-event divergence moves the login event relative to the configuration phase only, so the same ordering is the per-distro expectation), ban and whitelist denials surfaced as typed BotJoinDeniedException carrying the kick reason (whitelist denial asserted fail-fast, then flipped to a successful join via whitelist add), the real PlayerQuitEvent on removal, and quit/rejoin with the same server-derived offline UUID plus position restored from player data. Third slice of the S6 PR2 validation matrix (report tests 1, 3, 4, 5, 6, 13). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../maven/test/FullLoginItSupport.java | 89 +++++++ .../LightkeeperFullLoginValidationIT.java | 243 ++++++++++++++++++ 2 files changed, 332 insertions(+) create mode 100644 lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java create mode 100644 lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java new file mode 100644 index 00000000..e5b714e9 --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/FullLoginItSupport.java @@ -0,0 +1,89 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.EntitySnapshot; +import nl.pim16aap2.lightkeeper.framework.Vec3; +import nl.pim16aap2.lightkeeper.framework.WorldHandle; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; + +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Optional; +import java.util.UUID; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static org.assertj.core.api.Assertions.within; + +/** + * Shared assertions and lookups for the FULL_LOGIN validation-matrix integration tests. + * + *

Kept as plain static helpers so both matrix classes ({@code LightkeeperFullLoginValidationIT} and + * {@code LightkeeperFullLoginSessionIT}) assert identities, positions, and captured events the same way. + */ +final class FullLoginItSupport +{ + private FullLoginItSupport() + { + } + + /** + * Computes the offline-mode UUID the server derives from a player name during a {@code FULL_LOGIN} join. + * + * @param name + * The player name. + * @return The server-derived offline UUID. + */ + static UUID offlineUuid(String name) + { + return UUID.nameUUIDFromBytes(("OfflinePlayer:" + name).getBytes(StandardCharsets.UTF_8)); + } + + /** + * Looks up a player's current position through the live entity query. + * + * @param world + * World to query. + * @param playerId + * UUID of the player to find. + * @return The player's position, or empty when the player is not online in the world. + */ + static Optional playerPosition(WorldHandle world, UUID playerId) + { + return world.entities().ofType("minecraft:player").snapshot().stream() + .filter(snapshot -> snapshot.uuid().equals(playerId)) + .map(EntitySnapshot::position) + .findFirst(); + } + + /** + * Asserts that a position matches the expected coordinates within half a block on every axis. + * + * @param actual + * Observed position. + * @param expected + * Expected position. + */ + static void assertPositionCloseTo(Vec3 actual, Vec3 expected) + { + assertThat(actual.x()).isCloseTo(expected.x(), within(0.5)); + assertThat(actual.y()).isCloseTo(expected.y(), within(0.5)); + assertThat(actual.z()).isCloseTo(expected.z(), within(0.5)); + } + + /** + * Filters captured events down to those whose {@code getPlayer} value references the given player. + * + * @param events + * Captured event snapshots. + * @param playerId + * UUID of the acting player. + * @return The events attributed to the player, in capture order. + */ + static List eventsWithPlayerRef(List events, UUID playerId) + { + return events.stream() + .filter(event -> event.value("getPlayer") instanceof IProtocolValue.PRef playerRef + && playerRef.id().equals(playerId.toString())) + .toList(); + } +} diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java new file mode 100644 index 00000000..5febad49 --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java @@ -0,0 +1,243 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.BotJoinDeniedException; +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.CommandResult; +import nl.pim16aap2.lightkeeper.framework.ILightkeeperFramework; +import nl.pim16aap2.lightkeeper.framework.LightkeeperExtension; +import nl.pim16aap2.lightkeeper.framework.Vec3; +import nl.pim16aap2.lightkeeper.protocol.CommandSource; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.time.Duration; +import java.util.List; +import java.util.Locale; +import java.util.UUID; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.catchThrowable; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.eventually; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.assertPositionCloseTo; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.eventsWithPlayerRef; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.offlineUuid; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.playerPosition; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * FULL_LOGIN validation matrix, part 1: login-event contract, denial matrix, and quit/rejoin identity. + * + *

These tests pin the behaviors only a real login pipeline can deliver — the documented login-event order + * on both distros, whitelist/ban denials surfaced as typed errors, and the persisted offline identity across a + * real quit/rejoin cycle. Part 2 ({@link LightkeeperFullLoginSessionIT}) covers the in-session behaviors. + */ +@ExtendWith(LightkeeperExtension.class) +class LightkeeperFullLoginValidationIT +{ + private static final String PRE_LOGIN_EVENT = "org.bukkit.event.player.AsyncPlayerPreLoginEvent"; + private static final String LOGIN_EVENT = "org.bukkit.event.player.PlayerLoginEvent"; + private static final String JOIN_EVENT = "org.bukkit.event.player.PlayerJoinEvent"; + private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; + + @Test + @Timeout(60) + void fullLogin_shouldFireLoginEventsInDocumentedOrderPerDistro(ILightkeeperFramework framework) + { + // setup — register all three captures BEFORE joining so no login event can be missed. + final var world = framework.worlds().main(); + final String name = "lkevorder"; + final UUID expectedUuid = offlineUuid(name); + + try (var preLoginCapture = framework.events().capture(PRE_LOGIN_EVENT); + var loginCapture = framework.events().capture(LOGIN_EVENT); + var joinCapture = framework.events().capture(JOIN_EVENT)) + { + // execute + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // verify — all three login events fired exactly once for this bot. + eventually(Duration.ofSeconds(10), () -> + assertThat(eventsWithPlayerRef(joinCapture.getCapturedEvents(), expectedUuid)).hasSize(1)); + + final List preLoginEvents = preLoginCapture.getCapturedEvents().stream() + .filter(event -> new IProtocolValue.PString(name).equals(event.value("getName"))) + .toList(); + final List loginEvents = + eventsWithPlayerRef(loginCapture.getCapturedEvents(), expectedUuid); + final List joinEvents = + eventsWithPlayerRef(joinCapture.getCapturedEvents(), expectedUuid); + assertThat(preLoginEvents).hasSize(1); + assertThat(loginEvents).hasSize(1); + assertThat(joinEvents).hasSize(1); + + // The async pre-login carries the server-derived offline UUID; the login result is ALLOWED. + assertThat(preLoginEvents.getFirst().value("getUniqueId")) + .isEqualTo(new IProtocolValue.PUuid(expectedUuid)); + assertThat(loginEvents.getFirst().value("getResult")) + .isInstanceOfSatisfying(IProtocolValue.PEnum.class, result -> + assertThat(result.name()).isEqualTo("ALLOWED")); + + // Documented order — AsyncPlayerPreLoginEvent, then PlayerLoginEvent, then PlayerJoinEvent — + // asserted via tick stamps. This IS the per-distro expectation: the known Paper/Spigot divergence + // (Paper fires PlayerLoginEvent at configuration-finish, Spigot during the login phase) moves the + // login event relative to the configuration phase only, never behind the join, so the same + // ordering holds on both lanes without weakening to an unordered check. + final long preLoginTick = preLoginEvents.getFirst().tick(); + final long loginTick = loginEvents.getFirst().tick(); + final long joinTick = joinEvents.getFirst().tick(); + assertThat(preLoginTick).isLessThanOrEqualTo(loginTick); + assertThat(loginTick).isLessThanOrEqualTo(joinTick); + + player.remove(); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldRejectBannedPlayerWithTypedDenial(ILightkeeperFramework framework) + { + // setup — ban the name BEFORE it ever joins; the console resolves the offline profile by name. + final var world = framework.worlds().main(); + final String name = "lkbanned"; + final String banReason = "lk-ban-reason-e2e"; + final CommandResult banResult = + framework.server().executeCommand(CommandSource.CONSOLE, "ban %s %s".formatted(name, banReason)); + assertThat(banResult.success()).isTrue(); + + try + { + // execute + verify — the join is denied as a typed error carrying the ban reason. + assertThatThrownBy(() -> framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build()) + .isInstanceOf(BotJoinDeniedException.class) + .hasMessageContaining(banReason); + } + finally + { + framework.server().executeCommand(CommandSource.CONSOLE, "pardon " + name); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework framework) + { + // setup + final var world = framework.worlds().main(); + final String name = "lkwhite"; + assertThat(framework.server().executeCommand(CommandSource.CONSOLE, "whitelist on").success()).isTrue(); + + try + { + // execute + verify — the excluded bot is denied fast, as a typed error (not a join timeout). + final long startNanos = System.nanoTime(); + final Throwable denial = catchThrowable(() -> framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build()); + assertThat(denial).isInstanceOf(BotJoinDeniedException.class); + // Normalized check: Spigot/Paper say "whitelisted", older vanilla strings say "white-listed". + assertThat(denial.getMessage().toLowerCase(Locale.ROOT).replace("-", "")) + .as("denial reason should name the whitelist: %s", denial.getMessage()) + .contains("whitelist"); + + final long elapsedMillis = (System.nanoTime() - startNanos) / 1_000_000L; + assertThat(elapsedMillis) + .as("a whitelist denial must fail fast instead of riding the join timeout") + .isLessThan(15_000L); + + // execute + verify — whitelisting the name turns the same join into a success. + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist add " + name); + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + assertThat(player.uniqueId()).isEqualTo(offlineUuid(name)); + player.remove(); + } + finally + { + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist remove " + name); + framework.server().executeCommand(CommandSource.CONSOLE, "whitelist off"); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramework framework) + { + // setup — join, then move to a distinctive position that must survive the quit/rejoin cycle. + final var world = framework.worlds().main(); + final String name = "lkrejoin"; + final Vec3 persistedPosition = new Vec3(21.0, 120.0, 21.0); + + final var firstJoin = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + final UUID firstUuid = firstJoin.uniqueId(); + firstJoin.teleport(world, persistedPosition); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, firstUuid)).hasValueSatisfying(position -> + assertPositionCloseTo(position, persistedPosition))); + + // execute — quit (real quit path saves player data), then rejoin under the same name. + firstJoin.remove(); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, firstUuid)).isEmpty()); + final var rejoined = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // verify — same server-derived offline identity, and the position was restored from player data. + assertThat(rejoined.uniqueId()).isEqualTo(firstUuid).isEqualTo(offlineUuid(name)); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, firstUuid)).hasValueSatisfying(position -> + assertPositionCloseTo(position, persistedPosition))); + + rejoined.remove(); + } + + @Test + @Timeout(60) + void fullLogin_shouldFireRealQuitEventOnRemoval(ILightkeeperFramework framework) + { + // setup — capture quit events BEFORE removal so the event cannot be missed. + final var world = framework.worlds().main(); + final String name = "lkquit"; + + try (var quitCapture = framework.events().capture(QUIT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + final UUID playerId = player.uniqueId(); + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), playerId)).isEmpty(); + + // execute + player.remove(); + + // verify — removal traverses the real quit path: PlayerQuitEvent fires and the player is gone. + eventually(Duration.ofSeconds(10), () -> + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), playerId)).hasSize(1)); + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, playerId)).isEmpty()); + } + } +} From eabb40cc14bc82f182426197f28e848b51d6ef73 Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 17:38:20 +0200 Subject: [PATCH 04/10] test(integration): full-login session matrix + LuckPerms Permissible survival Covers on both distros: teleport acknowledgement (position applied and the session survives the ack round trip), login under the default-on compression threshold (asserted explicitly from the materialized server.properties so a silent flip to disabled fails the test), five back-to-back full logins simultaneously online with distinct offline UUIDs (the synchronized UDS transport allows one in-flight request, so wire-level concurrency is documented as not deliverable rather than faked), the configuration-phase locale observed server-side via /lktestlocale, chat parity through the real AsyncPlayerChatEvent, and LuckPerms Permissible-injection survival: LuckPerms is provisioned from Modrinth (pinned v5.5.53-bukkit) and a console lp grant must become visible through the joined player's live hasPermission. Final slice of the S6 PR2 validation matrix (report tests 2, 7, 8, 9, 10, 12). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- lightkeeper-integration-tests/pom.xml | 9 + .../test/LightkeeperFullLoginSessionIT.java | 280 ++++++++++++++++++ 2 files changed, 289 insertions(+) create mode 100644 lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java diff --git a/lightkeeper-integration-tests/pom.xml b/lightkeeper-integration-tests/pom.xml index 177786f0..36ed8213 100644 --- a/lightkeeper-integration-tests/pom.xml +++ b/lightkeeper-integration-tests/pom.xml @@ -108,6 +108,15 @@ ${project.version} lightkeeper-spigot-test-plugin.jar + + + modrinth + luckperms + v5.5.53-bukkit + ${lightkeeper.minecraft-version} + LuckPerms.jar + diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java new file mode 100644 index 00000000..d52124c9 --- /dev/null +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java @@ -0,0 +1,280 @@ +package nl.pim16aap2.lightkeeper.maven.test; + +import nl.pim16aap2.lightkeeper.framework.CapturedEventSnapshot; +import nl.pim16aap2.lightkeeper.framework.CommandResult; +import nl.pim16aap2.lightkeeper.framework.EntitySnapshot; +import nl.pim16aap2.lightkeeper.framework.ILightkeeperFramework; +import nl.pim16aap2.lightkeeper.framework.LightkeeperExtension; +import nl.pim16aap2.lightkeeper.framework.PlayerHandle; +import nl.pim16aap2.lightkeeper.framework.Vec3; +import nl.pim16aap2.lightkeeper.protocol.CommandSource; +import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.UUID; +import java.util.stream.Collectors; + +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; +import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.eventually; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.assertPositionCloseTo; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.eventsWithPlayerRef; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.offlineUuid; +import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.playerPosition; + +/** + * FULL_LOGIN validation matrix, part 2: in-session behaviors of fully logged-in bots. + * + *

Covers the driver's teleport acknowledgement, login under the (default-on) compression threshold, + * multiple simultaneous sessions, the configuration-phase locale, chat parity with the S4 event path, and the + * LuckPerms Permissible-injection survival that motivated the full-login pipeline in the first place. Part 1 + * ({@link LightkeeperFullLoginValidationIT}) covers the login-event contract and the denial matrix. + */ +@ExtendWith(LightkeeperExtension.class) +class LightkeeperFullLoginSessionIT +{ + private static final String CHAT_EVENT = "org.bukkit.event.player.AsyncPlayerChatEvent"; + private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; + + @Test + @Timeout(60) + void fullLogin_shouldAckTeleportAfterJoin(ILightkeeperFramework framework) + { + // setup — watch for kicks: a driver that failed to acknowledge the teleport would desync the session. + final var world = framework.worlds().main(); + final String name = "lktpack"; + final Vec3 target = new Vec3(33.0, 120.0, 33.0); + + try (var quitCapture = framework.events().capture(QUIT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // execute — the teleport makes the server send a position packet that the driver must answer with + // the teleport-accept packet for the session to stay in sync. + player.teleport(world, target); + + // verify — the position is applied server-side and the session survives the ack round trip. + eventually(Duration.ofSeconds(10), () -> + assertThat(playerPosition(world, player.uniqueId())).hasValueSatisfying(position -> + assertPositionCloseTo(position, target))); + player.andWaitTicks(20); + assertThat(eventsWithPlayerRef(quitCapture.getCapturedEvents(), player.uniqueId())).isEmpty(); + + player.remove(); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldNegotiateCompressionWhenEnabled(ILightkeeperFramework framework) + throws IOException + { + // setup — the provisioner leaves the vanilla default network-compression-threshold=256 in place, so + // compression is negotiated on every login. Assert that explicitly: if provisioning ever flipped it + // to -1 (disabled), this test must fail instead of silently no longer covering the driver's + // compression path. A per-test server.properties override is not possible today: the config overlay + // is copied wholesale after the runtime-port rewrite, so overlaying server.properties would clobber + // the reserved port. + final Path serverProperties = framework.server().directory().resolve("server.properties"); + final int compressionThreshold = readCompressionThreshold(serverProperties); + assertThat(compressionThreshold) + .as("network-compression-threshold in %s (>= 0 means compression is enabled)", serverProperties) + .isGreaterThanOrEqualTo(0); + + // execute — the join must handle the login-phase compression packet; everything after it (the whole + // configuration phase, the join, and the chat below) rides the compressed framing. + final var world = framework.worlds().main(); + final String message = "compressed chat roundtrip"; + try (var chatCapture = framework.events().capture(CHAT_EVENT)) + { + final var player = framework.bots().builder() + .withName("lkcompress") + .atSpawn(world) + .fullLogin() + .build(); + player.chat(message); + + // verify + eventually(Duration.ofSeconds(10), () -> + assertThat(chatCapture.getCapturedEvents()).anySatisfy(event -> + assertThat(event.value("getMessage")).isEqualTo(new IProtocolValue.PString(message)))); + + player.remove(); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldSupportFiveConcurrentJoins(ILightkeeperFramework framework) + { + // setup — the framework transport (UdsAgentTransport.send) is synchronized: exactly one request may + // be in flight per agent connection, so wire-level concurrent joins are not deliverable through this + // framework today. The five bots therefore join back-to-back, and the assertions target what full + // logins must guarantee regardless: five simultaneously online sessions with distinct server-derived + // identities and no cross-talk between their driver sessions. + final var world = framework.worlds().main(); + final List bots = new ArrayList<>(); + + try + { + // execute + for (int index = 1; index <= 5; ++index) + { + bots.add(framework.bots().builder() + .withName("lkconc" + index) + .atSpawn(world) + .fullLogin() + .build()); + } + + // verify — five distinct offline UUIDs, each derived from its own name... + final Set uniqueIds = bots.stream().map(PlayerHandle::uniqueId).collect(Collectors.toSet()); + assertThat(uniqueIds).hasSize(5); + for (final PlayerHandle bot : bots) + assertThat(bot.uniqueId()).isEqualTo(offlineUuid(bot.name())); + + // ...all simultaneously online, observed in one internally consistent entity-query burst. + eventually(Duration.ofSeconds(10), () -> + { + final Set onlineIds = world.entities().ofType("minecraft:player").snapshot().stream() + .map(EntitySnapshot::uuid) + .collect(Collectors.toSet()); + assertThat(onlineIds).containsAll(uniqueIds); + }); + } + finally + { + for (final PlayerHandle bot : bots) + bot.remove(); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldApplyRequestedLocale(ILightkeeperFramework framework) + { + // setup — the driver sends the locale inside ClientInformation during the configuration phase; the + // test plugin's /lktestlocale logs Player#getLocale, so the server-side view of the locale is + // asserted from the captured console output. + final var world = framework.worlds().main(); + final String name = "lklocale"; + + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .withLocale("de_de") + .fullLogin() + .build(); + + // execute + player.executeCommand("lktestlocale"); + + // verify + eventually(Duration.ofSeconds(10), () -> + assertThat(framework.server().output()) + .anyMatch(line -> line.contains("LK_LOCALE name=%s locale=de_de".formatted(name)))); + + player.remove(); + } + + @Test + @Timeout(60) + void fullLogin_shouldChatLikeARealClient(ILightkeeperFramework framework) + { + // setup + final var world = framework.worlds().main(); + final String name = "lkfullchat"; + final String message = "hello from a real login"; + + try (var chatCapture = framework.events().capture(CHAT_EVENT)) + { + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + + // execute — chat parity with the S4 path, now from a fully logged-in session. + player.chat(message); + + // verify — the real AsyncPlayerChatEvent fires, attributed to the full-login bot. + eventually(Duration.ofSeconds(10), () -> + assertThat(chatCapture.getCapturedEvents()).isNotEmpty()); + final CapturedEventSnapshot chatEvent = chatCapture.getCapturedEvents().getFirst(); + assertThat(chatEvent.value("getMessage")).isEqualTo(new IProtocolValue.PString(message)); + assertThat(chatEvent.value("getPlayer")) + .isInstanceOfSatisfying(IProtocolValue.PRef.class, playerRef -> + assertThat(playerRef.id()).isEqualTo(player.uniqueId().toString())); + + player.remove(); + } + } + + @Test + @Timeout(60) + void fullLogin_shouldRetainLuckPermsPermissionInjectedAtLogin(ILightkeeperFramework framework) + { + // setup — LuckPerms is provisioned by this module's pom (Modrinth, pinned version); fail fast with a + // clear reason when that wiring is missing rather than timing out on the permission check below. + assertThat(framework.server().directory().resolve("plugins").resolve("LuckPerms.jar")) + .as("LuckPerms must be provisioned for the Permissible-injection survival check") + .isRegularFile(); + + final var world = framework.worlds().main(); + final String name = "lklperm"; + final String node = "lightkeeper.test.lpnode"; + + final var player = framework.bots().builder() + .withName(name) + .atSpawn(world) + .fullLogin() + .build(); + assertThat(player.permissions().has(node)).isFalse(); + + try + { + // execute — grant through LuckPerms itself (console), NOT LightKeeper's own attachment. + final CommandResult grantResult = framework.server().executeCommand( + CommandSource.CONSOLE, "lp user %s permission set %s true".formatted(name, node)); + assertThat(grantResult.success()).isTrue(); + + // verify — the LIVE hasPermission resolves through the Permissible LuckPerms injected at + // PlayerLoginEvent. Had the injection not survived onto the joined player instance, the + // LuckPerms-stored node would keep reading false here. LuckPerms applies console edits + // asynchronously, hence the retry window. + eventually(Duration.ofSeconds(20), () -> + assertThat(player.permissions().has(node)).isTrue()); + } + finally + { + framework.server().executeCommand( + CommandSource.CONSOLE, "lp user %s permission unset %s".formatted(name, node)); + player.remove(); + } + } + + private static int readCompressionThreshold(Path serverProperties) + throws IOException + { + for (final String line : Files.readAllLines(serverProperties, StandardCharsets.UTF_8)) + { + final String trimmed = line.trim(); + if (trimmed.startsWith("network-compression-threshold=")) + return Integer.parseInt(trimmed.substring("network-compression-threshold=".length()).trim()); + } + throw new AssertionError("No network-compression-threshold entry in " + serverProperties); + } +} From 6027ec41a99611c2e707c8d021a0df048211ff2b Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 23:31:28 +0200 Subject: [PATCH 05/10] test(integration): assert rejoin persistence via inventory, not position The merged bot-round fix (1a5c92f) makes FULL_LOGIN honor the requested placement: the agent teleports a (re)joining bot to the requested location or world spawn post-join, deliberately overriding the position loaded from player data. The rejoin test's persistence observable therefore moves from the live position to the inventory (console give before quit, findItem after rejoin), which the quit path saves and the placement does not touch. The identity assertion (same server-derived offline UUID) is unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../LightkeeperFullLoginValidationIT.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java index 5febad49..dc6cbefb 100644 --- a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java @@ -5,7 +5,6 @@ import nl.pim16aap2.lightkeeper.framework.CommandResult; import nl.pim16aap2.lightkeeper.framework.ILightkeeperFramework; import nl.pim16aap2.lightkeeper.framework.LightkeeperExtension; -import nl.pim16aap2.lightkeeper.framework.Vec3; import nl.pim16aap2.lightkeeper.protocol.CommandSource; import nl.pim16aap2.lightkeeper.protocol.IProtocolValue; import org.junit.jupiter.api.Test; @@ -20,7 +19,6 @@ import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.assertThat; import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.catchThrowable; import static nl.pim16aap2.lightkeeper.framework.assertions.LightkeeperAssertions.eventually; -import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.assertPositionCloseTo; import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.eventsWithPlayerRef; import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.offlineUuid; import static nl.pim16aap2.lightkeeper.maven.test.FullLoginItSupport.playerPosition; @@ -177,10 +175,13 @@ void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework fra @Timeout(60) void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramework framework) { - // setup — join, then move to a distinctive position that must survive the quit/rejoin cycle. + // setup — join, then give the bot a distinctive item that must survive the quit/rejoin cycle. The + // persistence observable is the inventory (saved to player data on the real quit path) rather than the + // position, because FULL_LOGIN honors the requested placement: the agent teleports a (re)joining bot to + // the requested location/world spawn post-join, overriding the position loaded from player data. final var world = framework.worlds().main(); final String name = "lkrejoin"; - final Vec3 persistedPosition = new Vec3(21.0, 120.0, 21.0); + final String persistedItem = "minecraft:diamond"; final var firstJoin = framework.bots().builder() .withName(name) @@ -188,10 +189,11 @@ void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramew .fullLogin() .build(); final UUID firstUuid = firstJoin.uniqueId(); - firstJoin.teleport(world, persistedPosition); + assertThat(framework.server() + .executeCommand(CommandSource.CONSOLE, "give %s %s 3".formatted(name, persistedItem)) + .success()).isTrue(); eventually(Duration.ofSeconds(10), () -> - assertThat(playerPosition(world, firstUuid)).hasValueSatisfying(position -> - assertPositionCloseTo(position, persistedPosition))); + assertThat(firstJoin.inventory().findItem(persistedItem)).isNotNull()); // execute — quit (real quit path saves player data), then rejoin under the same name. firstJoin.remove(); @@ -203,11 +205,10 @@ void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramew .fullLogin() .build(); - // verify — same server-derived offline identity, and the position was restored from player data. + // verify — same server-derived offline identity, and the inventory was restored from player data. assertThat(rejoined.uniqueId()).isEqualTo(firstUuid).isEqualTo(offlineUuid(name)); eventually(Duration.ofSeconds(10), () -> - assertThat(playerPosition(world, firstUuid)).hasValueSatisfying(position -> - assertPositionCloseTo(position, persistedPosition))); + assertThat(rejoined.inventory().findItem(persistedItem)).isNotNull()); rejoined.remove(); } From 5e89184b994bc03a6d946e501135edf543e88546 Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 23:38:39 +0200 Subject: [PATCH 06/10] fix(nms): complete denial outcomes before closing the channel Regression caught by the new denial matrix on both lanes: 1a5c92f's channel close listener (which surfaces raw remote closes) fires INLINE when terminate() closes the channel, and completeDenied called terminate() before completing the outcome. The listener's generic 'Connection closed during ' denial therefore won the one-shot completion race, discarding the server's actual kick reason (ban/whitelist text). The same masking applied to the fail-loud unhandled-packet error and packet-servicing failures. Fix: resolve the outcome first, then terminate. Verified by fullLogin_shouldRejectBannedPlayerWithTypedDenial and fullLogin_shouldRejectWhenWhitelistExcludesPlayer, which assert the kick reason text on Paper and Spigot. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../nms/v121r7/BotLoginSession.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java index 1d0cf204..ab6b1f67 100644 --- a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java +++ b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginSession.java @@ -256,13 +256,15 @@ private void dispatchPacket(@Nullable Object packet) catch (ReflectiveOperationException | RuntimeException exception) { // Any servicing failure (reflective or runtime, e.g. a missing structural accessor) means the - // driver can no longer run this connection reliably: stop servicing, close the channel, and make - // sure the outcome can never be left pending. - terminate(); + // driver can no longer run this connection reliably: resolve the outcome so it can never be left + // pending, then stop servicing and close the channel. Completion MUST precede terminate(): + // closing the channel fires the close listener inline, whose generic transport-close denial + // would otherwise win the completion race and mask the real failure. final IllegalStateException failure = new IllegalStateException( "Failed to handle %s packet %s.".formatted(phase, packetClass.getName()), exception); if (!outcome.completeExceptionally(failure)) LOG.log(System.Logger.Level.WARNING, "Full-login packet servicing failed after join.", failure); + terminate(); } } @@ -275,18 +277,19 @@ private void handleUnknownPacket(Class packetClass) return; } - // Fail-loud: an unhandled login/configuration packet would silently stall the join. - terminate(); + // Fail-loud: an unhandled login/configuration packet would silently stall the join. Completion MUST + // precede terminate() so the close listener's generic denial cannot mask the named error. outcome.completeExceptionally(new IllegalStateException( "Unhandled %s packet from server: %s. Add it to the login driver's packet table." .formatted(phase, packetClass.getName()))); + terminate(); } private void handleTransportDisconnect() { - terminate(); if (!outcome.isDone()) outcome.complete(new IBotLoginOutcome.Denied(phase, "Connection closed during " + phase + ".")); + terminate(); } private void completeJoined() @@ -297,9 +300,13 @@ private void completeJoined() private void completeDenied(Object disconnectPacket) { - terminate(); + // Complete with the packet's kick reason BEFORE terminating: terminate() closes the channel, which + // fires the registered close listener inline (handleTransportDisconnect), and its generic + // "Connection closed" denial would otherwise win the one-shot completion race and discard the + // server's actual denial reason (whitelist/ban kick text). if (!outcome.isDone()) outcome.complete(new IBotLoginOutcome.Denied(phase, reflection.disconnectReason(disconnectPacket))); + terminate(); } private Object connection() From 7f5176f6ff01d97c3a187074bf06ab712066e052 Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Wed, 15 Jul 2026 23:51:28 +0200 Subject: [PATCH 07/10] fix(agent): remove full-login bots through the real disconnect flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second regression caught by the matrix, on the Paper lane: removing a FULL_LOGIN bot through the legacy PlayerList removal left its real TCP connection open. ~30 seconds later the server timed the connection out and ran PlayerList.remove a second time, hitting Paper's EntityScheduler 'Already retired' IllegalStateException — which crashes the whole server ('Encountered an unexpected exception'), killing the agent and every later test in the lane. Fix: track FULL_LOGIN joins and remove those bots via Player#kickPlayer, the real disconnect flow — the play-phase disconnect packet reaches the login driver (which closes its channel), the player is removed exactly once, and PlayerQuitEvent fires on the regular quit path. Legacy-spawn bots keep the reflective removal. Covered by a new agent unit test and by the quit/rejoin integration tests on both lanes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../agent/spigot/AgentPlayerActions.java | 41 ++++++++++-- .../agent/spigot/AgentPlayerActionsTest.java | 65 ++++++++++++++++++- 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java index 4b493ab8..2016ff51 100644 --- a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java +++ b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java @@ -36,7 +36,9 @@ import java.time.Duration; import java.util.Objects; +import java.util.Set; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; @@ -66,6 +68,11 @@ final class AgentPlayerActions * NMS-backed synthetic player implementation. */ private final IBotPlayerNmsAdapter botPlayerNmsAdapter; + /** + * UUIDs of synthetic players that joined through the FULL_LOGIN pipeline and therefore own a real TCP + * connection; their removal must run the real disconnect flow instead of the legacy player-list removal. + */ + private final Set fullLoginPlayerIds = ConcurrentHashMap.newKeySet(); /** * @param plugin @@ -368,6 +375,7 @@ private void registerJoinedPlayer(Player player, CreatePlayer.Command command) player.teleport(target); playerStore.registerSyntheticPlayer(uuid, player); + fullLoginPlayerIds.add(uuid); if (health != null) player.setHealth(Math.min(player.getMaxHealth(), health)); if (permissionsCsv != null && !permissionsCsv.isBlank()) @@ -393,9 +401,7 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command) mainThreadExecutor.callOnMainThread(() -> { final Player player = playerStore.getRequiredPlayer(uuid); - playerStore.removePermissionAttachment(uuid, player); - botPlayerNmsAdapter.removePlayer(player); - playerStore.removeSyntheticPlayer(uuid); + removeRegisteredPlayer(uuid, player); plugin.getLogger().info( "LK_AGENT: Removed synthetic player '%s' (%s)." .formatted(player.getName(), player.getUniqueId()) @@ -406,6 +412,31 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command) return new RemovePlayer.Response(); } + /** + * Removes a registered synthetic player through the path matching how it joined (main thread only). + * + *

A {@code FULL_LOGIN} bot owns a real TCP connection, so it must leave through the real disconnect + * flow ({@code Player#kickPlayer}): the play-phase disconnect packet reaches the login driver, the channel + * closes, and {@code PlayerQuitEvent} fires on the regular quit path. Removing such a bot through the + * legacy player-list removal would leave its connection open — the server times it out ~30 seconds later + * and runs the removal a second time, which crashes Paper ({@code EntityScheduler}: "Already retired"). + * Legacy-spawn bots have no real connection and keep the reflective removal path. + * + * @param uuid + * UUID of the player to remove. + * @param player + * The registered player instance. + */ + private void removeRegisteredPlayer(UUID uuid, Player player) + { + playerStore.removePermissionAttachment(uuid, player); + if (fullLoginPlayerIds.remove(uuid)) + player.kickPlayer("LightKeeper bot removed."); + else + botPlayerNmsAdapter.removePlayer(player); + playerStore.removeSyntheticPlayer(uuid); + } + /** * Handles {@code EXECUTE_PLAYER_COMMAND} by dispatching the command in the player's execution context. * @@ -638,9 +669,7 @@ void cleanupSyntheticPlayers() try { final Player player = playerStore.getRequiredPlayer(uuid); - playerStore.removePermissionAttachment(uuid, player); - botPlayerNmsAdapter.removePlayer(player); - playerStore.removeSyntheticPlayer(uuid); + removeRegisteredPlayer(uuid, player); } catch (Exception exception) { diff --git a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java index a01698c9..f63d563a 100644 --- a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java +++ b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java @@ -20,6 +20,7 @@ import nl.pim16aap2.lightkeeper.protocol.MutatePlayerPermission; import nl.pim16aap2.lightkeeper.protocol.PlacePlayerBlock; import nl.pim16aap2.lightkeeper.protocol.PlayerChat; +import nl.pim16aap2.lightkeeper.protocol.RemovePlayer; import nl.pim16aap2.lightkeeper.protocol.RightClickBlock; import nl.pim16aap2.lightkeeper.protocol.TeleportPlayer; import org.bukkit.Bukkit; @@ -28,12 +29,15 @@ import org.bukkit.block.Block; import org.bukkit.block.BlockFace; import org.bukkit.entity.Player; +import org.bukkit.event.Listener; import org.bukkit.event.block.Action; import org.bukkit.event.player.PlayerDropItemEvent; import org.bukkit.event.player.PlayerInteractEvent; +import org.bukkit.event.player.PlayerJoinEvent; import org.bukkit.inventory.ItemStack; import org.bukkit.inventory.PlayerInventory; import org.bukkit.permissions.PermissionAttachment; +import org.bukkit.plugin.EventExecutor; import org.bukkit.plugin.PluginManager; import org.bukkit.plugin.java.JavaPlugin; import org.bukkit.scheduler.BukkitScheduler; @@ -672,17 +676,76 @@ void handleCreatePlayer_shouldSurfaceMissingJoinEventAsTypedTimeout() } } + @Test + void handleRemovePlayer_shouldKickFullLoginBotInsteadOfLegacyRemoval() + throws Exception + { + // setup — drive a successful FULL_LOGIN join (the stubbed plugin manager fires PlayerJoinEvent on + // registration) so the bot is tracked as owning a real connection. + final PlayerActionsFixture fixture = createPlayerActionsFixture(); + final IBotLoginDriver loginDriver = mock(); + when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver); + when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot")); + final UUID uuid = UUID.randomUUID(); + final Player player = mock(); + when(player.getUniqueId()).thenReturn(uuid); + when(player.getName()).thenReturn("fullbot"); + final CreatePlayer.Command createCommand = new CreatePlayer.Command( + "request-full-rm", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN, null); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) + { + stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player)); + final CreatePlayer.Response created = fixture.playerActions().handleCreatePlayer(createCommand); + assertThat(created.uuid()).isEqualTo(uuid); + + // execute + fixture.playerActions().handleRemovePlayer(new RemovePlayer.Command("request-remove-full", uuid)); + + // verify — the bot leaves through the real disconnect flow (kick), never the legacy player-list + // removal that would leave its TCP connection open (Paper crashes on the second removal when the + // server times that connection out). + verify(player).kickPlayer(anyString()); + verify(fixture.nmsAdapter(), never()).removePlayer(any()); + } + } + + /** + * Creates a plugin-manager mock that immediately fires a {@code PlayerJoinEvent} for the given player + * whenever an event executor is registered, completing a FULL_LOGIN join's join-latch synchronously. + */ + private static PluginManager joinEventFiringPluginManager(Player player) + { + final PluginManager pluginManager = mock(); + doAnswer(invocation -> + { + final Listener listener = invocation.getArgument(1); + final EventExecutor eventExecutor = invocation.getArgument(3); + eventExecutor.execute(listener, new PlayerJoinEvent(player, "joined")); + return null; + }).when(pluginManager).registerEvent(eq(PlayerJoinEvent.class), any(), any(), any(), any()); + return pluginManager; + } + /** * Stubs the Bukkit statics a FULL_LOGIN join touches: NOT the primary thread (the handler rejects * main-thread execution), a server exposing a port, a known world named {@code "world"}, a no-op plugin * manager, and a scheduler that runs main-thread callables inline. */ private static void stubFullLoginBukkitStatics(MockedStatic bukkit) + { + stubFullLoginBukkitStatics(bukkit, mock(PluginManager.class)); + } + + /** + * Variant of {@link #stubFullLoginBukkitStatics(MockedStatic)} with a caller-supplied plugin manager, for + * tests that need event registrations to actually fire events. + */ + private static void stubFullLoginBukkitStatics(MockedStatic bukkit, PluginManager pluginManager) { final org.bukkit.Server server = mock(); when(server.getPort()).thenReturn(25_565); final World world = mock(); - final PluginManager pluginManager = mock(); final BukkitScheduler scheduler = mock(); when(scheduler.callSyncMethod(any(), any())).thenAnswer(invocation -> { From a8607b2eab8e1637ba0b1955b14bdd52c8f0ce8d Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Thu, 16 Jul 2026 00:09:10 +0200 Subject: [PATCH 08/10] fix(nms): wait for channel activation before initiating the login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third regression caught by the matrix (twice, Paper lane): connectToServer syncs on the Netty connect future, but Connection.channel is only assigned in channelActive() on the event loop, so the driver thread can still observe isConnected() == false after connectToServer returns. In that window initiateServerboundPlayConnection silently defers the entire protocol/listener setup into the connection's pending-actions queue — which nothing ever drains for this untucked client-side connection — while send() queues the hello separately. The hello then hits the encoder with no LOGIN protocol and no listener ('Sending unknown packet serverbound/minecraft:hello', 'packetListener is null'), and the join stalls until the server's 30-second ReadTimeoutHandler closes it (now surfaced as 'Connection closed during LOGIN'). Fix: after connectToServer, wait (bounded, 5s) until the channel field is assigned and open — vanilla's isConnected() contract — so the setup and the hello take their immediate, in-program-order paths. Loopback channels activate in well under a millisecond; the wait is a named-constant bounded spin. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../nms/v121r7/BotLoginReflection.java | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java index 0226bf2a..1db2220f 100644 --- a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java +++ b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java @@ -22,6 +22,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.TimeUnit; import java.util.function.Function; /** @@ -40,6 +41,12 @@ final class BotLoginReflection { private static final System.Logger LOG = System.getLogger(BotLoginReflection.class.getName()); + /** + * Upper bound on the wait for a freshly connected channel to become active (see + * {@code awaitChannelActive}); loopback channels activate in well under a millisecond. + */ + private static final long CHANNEL_ACTIVE_TIMEOUT_SECONDS = 5L; + /** * How the session should react to a received clientbound packet, resolved from its class via * {@link #classify(Class)}. @@ -113,6 +120,7 @@ enum PacketKind private final Method setupCompressionMethod; private final Field channelField; private final Method channelCloseMethod; + private final Method channelIsOpenMethod; /** * Paper's per-connection inbound packet-rate counter ({@code Connection.allPacketCounts}), or {@code null} * on distributions without it (Spigot). Paper initializes it from config on EVERY {@code Connection} — @@ -264,6 +272,7 @@ enum PacketKind channelField = NmsReflectionUtils.resolveFieldByNameOrAcceptedType( connectionClass, "channel", channelClass); channelCloseMethod = channelClass.getMethod("close"); + channelIsOpenMethod = channelClass.getMethod("isOpen"); paperPacketLimiterField = resolvePaperPacketLimiterField(connectionClass); channelCloseFutureMethod = channelClass.getMethod("closeFuture"); futureListenerInterface = NmsReflectionUtils.resolveClass( @@ -374,6 +383,15 @@ Object openConnection(int port, Object listenerProxy) final Object connection = connectToServerMethod.invoke(null, address, eventLoopGroupHolder, null); try { + // connectToServer syncs on the Netty CONNECT future, but Connection.channel is only assigned in + // channelActive() on the event loop, so this thread can still observe isConnected() == false here. + // In that window initiateServerboundPlayConnection silently defers the whole protocol/listener + // setup into the connection's pending-actions queue — which nothing ever drains for this untucked + // client-side connection — while send() queues the hello separately. The hello then reaches the + // encoder without a LOGIN protocol or listener ("Sending unknown packet 'serverbound/minecraft: + // hello'") and the join stalls until the server's ReadTimeoutHandler closes it. Waiting for the + // channel to be live makes both calls below take their immediate, in-program-order paths. + awaitChannelActive(connection); disarmPaperPacketLimiter(connection); initiateLoginConnectionMethod.invoke(connection, "127.0.0.1", port, listenerProxy); } @@ -386,6 +404,40 @@ Object openConnection(int port, Object listenerProxy) return connection; } + /** + * Waits (bounded) until the connection's channel is assigned and open, i.e. vanilla's + * {@code isConnected()} contract holds for callers on this thread. + * + * @param connection + * The connection returned by {@code connectToServer}. + * @throws ReflectiveOperationException + * When the reflective channel read fails. + */ + private void awaitChannelActive(Object connection) + throws ReflectiveOperationException + { + final long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(CHANNEL_ACTIVE_TIMEOUT_SECONDS); + while (System.nanoTime() - deadlineNanos < 0) + { + final Object channel = channelField.get(connection); + if (channel != null && Boolean.TRUE.equals(channelIsOpenMethod.invoke(channel))) + return; + try + { + Thread.sleep(1L); + } + catch (InterruptedException exception) + { + Thread.currentThread().interrupt(); + throw new IllegalStateException( + "Interrupted while waiting for the bot connection channel to become active.", exception); + } + } + throw new IllegalStateException( + "Bot connection channel did not become active within %d seconds." + .formatted(CHANNEL_ACTIVE_TIMEOUT_SECONDS)); + } + /** * Invokes {@code callback} as soon as the connection's channel closes — locally or remotely. * From 394311d83b9d80346595d52e5618e3ff4862201c Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Thu, 16 Jul 2026 04:15:51 +0200 Subject: [PATCH 09/10] fix: address pre-PR deep-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1 (MAJOR): removeRegisteredPlayer now detects a vetoed kick. PlayerKickEvent is cancellable on both distros, so a kick-cancelling plugin turned the removal of a full-login bot into a silent no-op while the agent reported success — a permanent ghost bot whose driver keeps answering keep-alives. The disconnect completes synchronously inside kickPlayer on both jars, so an isOnline() probe after the kick reliably detects the veto; it now throws a typed AgentProtocolException(REQUEST_FAILED) naming the cancelled PlayerKickEvent BEFORE any agent state is mutated (store entry retained, removal retryable, no legacy-removal fallback). Covered by a kick-cancelling unit test. 2: all matrix @Timeout values raised 60 -> 180 seconds, safely above the 120s agent join budget and the 150s client watchdog, so a stalled login surfaces as the typed PLAYER_JOIN_TIMEOUT instead of a JUnit interrupt mid-RPC. 3: dropped the whitelist denial's <15s wall-clock assertion (starved-runner flake); the typed BotJoinDeniedException-not-timeout distinction proves the fail-fast semantics, documented in a comment. 4: LoopbackLoginGuard accepts case-insensitive YAML booleans (True/TRUE) for bungeecord/velocity-enabled and legal properties spacing ('online-mode = true'); unit tests extended with all three spellings. 5: awaitChannelActive documents that the non-volatile cross-thread read mirrors vanilla's own isConnected() access pattern and that the reflective Field.get defeats hoisting. 6: renamed fullLogin_shouldSupportFiveConcurrentJoins -> fullLogin_shouldSustainFiveSimultaneousSessions to match what is asserted (sequential joins over the synchronized transport, simultaneous sessions). Skipped per review: offlineUuid dedup across PR1's smoke IT (additive-scope rule; fold on next touch). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../agent/spigot/AgentPlayerActions.java | 23 ++++++++- .../agent/spigot/AgentPlayerActionsTest.java | 41 ++++++++++++++++ .../test/LightkeeperFullLoginSessionIT.java | 14 +++--- .../LightkeeperFullLoginValidationIT.java | 21 ++++----- .../provisioning/LoopbackLoginGuard.java | 18 +++++-- .../provisioning/LoopbackLoginGuardTest.java | 47 +++++++++++++++++++ .../nms/v121r7/BotLoginReflection.java | 2 + 7 files changed, 140 insertions(+), 26 deletions(-) diff --git a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java index 2016ff51..d041f998 100644 --- a/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java +++ b/lightkeeper-agent-spigot/src/main/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActions.java @@ -422,18 +422,37 @@ RemovePlayer.Response handleRemovePlayer(RemovePlayer.Command command) * and runs the removal a second time, which crashes Paper ({@code EntityScheduler}: "Already retired"). * Legacy-spawn bots have no real connection and keep the reflective removal path. * + *

{@code PlayerKickEvent} is cancellable on both distros, so a vetoing plugin would silently turn the + * kick into a no-op while the agent reports success — a permanent ghost bot whose driver keeps answering + * keep-alives. The disconnect completes synchronously inside {@code kickPlayer} on both jars, so the + * {@code isOnline()} probe below reliably reflects the outcome; a vetoed kick fails loud BEFORE any agent + * state is mutated, keeping the removal cleanly retryable. + * * @param uuid * UUID of the player to remove. * @param player * The registered player instance. + * @throws AgentProtocolException + * If a plugin cancelled the {@code PlayerKickEvent} of a full-login bot's removal. */ private void removeRegisteredPlayer(UUID uuid, Player player) { - playerStore.removePermissionAttachment(uuid, player); - if (fullLoginPlayerIds.remove(uuid)) + if (fullLoginPlayerIds.contains(uuid)) + { player.kickPlayer("LightKeeper bot removed."); + if (player.isOnline()) + throw new AgentProtocolException( + AgentErrorCode.REQUEST_FAILED, + ("PlayerKickEvent was cancelled by a plugin: full-login bot '%s' (%s) is still online and " + + "was NOT removed.").formatted(player.getName(), uuid)); + fullLoginPlayerIds.remove(uuid); + playerStore.removePermissionAttachment(uuid, player); + } else + { + playerStore.removePermissionAttachment(uuid, player); botPlayerNmsAdapter.removePlayer(player); + } playerStore.removeSyntheticPlayer(uuid); } diff --git a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java index f63d563a..56e2f36e 100644 --- a/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java +++ b/lightkeeper-agent-spigot/src/test/java/nl/pim16aap2/lightkeeper/agent/spigot/AgentPlayerActionsTest.java @@ -710,6 +710,47 @@ void handleRemovePlayer_shouldKickFullLoginBotInsteadOfLegacyRemoval() } } + @Test + void handleRemovePlayer_shouldFailLoudWhenKickOfFullLoginBotIsCancelled() + throws Exception + { + // setup — a kick-cancelling plugin (PlayerKickEvent is cancellable on both distros) leaves the bot + // online after kickPlayer returns; the agent must fail loud instead of reporting a successful removal. + final PlayerActionsFixture fixture = createPlayerActionsFixture(); + final IBotLoginDriver loginDriver = mock(); + when(fixture.nmsAdapter().loginDriver()).thenReturn(loginDriver); + when(loginDriver.login(any())).thenReturn(new IBotLoginOutcome.Joined("fullbot")); + final UUID uuid = UUID.randomUUID(); + final Player player = mock(); + when(player.getUniqueId()).thenReturn(uuid); + when(player.getName()).thenReturn("fullbot"); + when(player.isOnline()).thenReturn(true); + final CreatePlayer.Command createCommand = new CreatePlayer.Command( + "request-full-veto", "fullbot", null, "world", null, null, null, null, null, JoinMode.FULL_LOGIN, + null); + + try (MockedStatic bukkit = mockStatic(Bukkit.class)) + { + stubFullLoginBukkitStatics(bukkit, joinEventFiringPluginManager(player)); + fixture.playerActions().handleCreatePlayer(createCommand); + + // execute + final Throwable thrown = catchThrowable(() -> fixture.playerActions() + .handleRemovePlayer(new RemovePlayer.Command("request-remove-veto", uuid))); + + // verify — typed failure naming the veto, and no agent state mutated: the bot stays registered + // (the removal is retryable) and the legacy removal path is never used as a fallback. + assertThat(thrown) + .isInstanceOf(AgentProtocolException.class) + .satisfies(failure -> assertThat(((AgentProtocolException) failure).errorCode()) + .isEqualTo(AgentErrorCode.REQUEST_FAILED)) + .hasMessageContaining("PlayerKickEvent was cancelled"); + verify(player).kickPlayer(anyString()); + verify(fixture.nmsAdapter(), never()).removePlayer(any()); + assertThat(fixture.playerStore().getRequiredPlayer(uuid)).isSameAs(player); + } + } + /** * Creates a plugin-manager mock that immediately fires a {@code PlayerJoinEvent} for the given player * whenever an event executor is registered, completing a FULL_LOGIN join's join-latch synchronously. diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java index d52124c9..43fc1f3e 100644 --- a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginSessionIT.java @@ -46,7 +46,7 @@ class LightkeeperFullLoginSessionIT private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldAckTeleportAfterJoin(ILightkeeperFramework framework) { // setup — watch for kicks: a driver that failed to acknowledge the teleport would desync the session. @@ -78,7 +78,7 @@ void fullLogin_shouldAckTeleportAfterJoin(ILightkeeperFramework framework) } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldNegotiateCompressionWhenEnabled(ILightkeeperFramework framework) throws IOException { @@ -117,8 +117,8 @@ void fullLogin_shouldNegotiateCompressionWhenEnabled(ILightkeeperFramework frame } @Test - @Timeout(60) - void fullLogin_shouldSupportFiveConcurrentJoins(ILightkeeperFramework framework) + @Timeout(180) + void fullLogin_shouldSustainFiveSimultaneousSessions(ILightkeeperFramework framework) { // setup — the framework transport (UdsAgentTransport.send) is synchronized: exactly one request may // be in flight per agent connection, so wire-level concurrent joins are not deliverable through this @@ -163,7 +163,7 @@ void fullLogin_shouldSupportFiveConcurrentJoins(ILightkeeperFramework framework) } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldApplyRequestedLocale(ILightkeeperFramework framework) { // setup — the driver sends the locale inside ClientInformation during the configuration phase; the @@ -191,7 +191,7 @@ void fullLogin_shouldApplyRequestedLocale(ILightkeeperFramework framework) } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldChatLikeARealClient(ILightkeeperFramework framework) { // setup @@ -224,7 +224,7 @@ void fullLogin_shouldChatLikeARealClient(ILightkeeperFramework framework) } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldRetainLuckPermsPermissionInjectedAtLogin(ILightkeeperFramework framework) { // setup — LuckPerms is provisioned by this module's pom (Modrinth, pinned version); fail fast with a diff --git a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java index dc6cbefb..fa60d02c 100644 --- a/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java +++ b/lightkeeper-integration-tests/src/test/java/nl/pim16aap2/lightkeeper/maven/test/LightkeeperFullLoginValidationIT.java @@ -40,7 +40,7 @@ class LightkeeperFullLoginValidationIT private static final String QUIT_EVENT = "org.bukkit.event.player.PlayerQuitEvent"; @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldFireLoginEventsInDocumentedOrderPerDistro(ILightkeeperFramework framework) { // setup — register all three captures BEFORE joining so no login event can be missed. @@ -97,7 +97,7 @@ void fullLogin_shouldFireLoginEventsInDocumentedOrderPerDistro(ILightkeeperFrame } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldRejectBannedPlayerWithTypedDenial(ILightkeeperFramework framework) { // setup — ban the name BEFORE it ever joins; the console resolves the offline profile by name. @@ -126,7 +126,7 @@ void fullLogin_shouldRejectBannedPlayerWithTypedDenial(ILightkeeperFramework fra } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework framework) { // setup @@ -136,8 +136,10 @@ void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework fra try { - // execute + verify — the excluded bot is denied fast, as a typed error (not a join timeout). - final long startNanos = System.nanoTime(); + // execute + verify — the excluded bot is denied as a typed error. Fail-fast semantics are proven + // by the TYPE of the failure: a BotJoinDeniedException (the server's login-phase kick) instead of + // a BotJoinTimeoutException. Deliberately no wall-clock bound: elapsed-time assertions flake on + // starved CI runners. final Throwable denial = catchThrowable(() -> framework.bots().builder() .withName(name) .atSpawn(world) @@ -149,11 +151,6 @@ void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework fra .as("denial reason should name the whitelist: %s", denial.getMessage()) .contains("whitelist"); - final long elapsedMillis = (System.nanoTime() - startNanos) / 1_000_000L; - assertThat(elapsedMillis) - .as("a whitelist denial must fail fast instead of riding the join timeout") - .isLessThan(15_000L); - // execute + verify — whitelisting the name turns the same join into a success. framework.server().executeCommand(CommandSource.CONSOLE, "whitelist add " + name); final var player = framework.bots().builder() @@ -172,7 +169,7 @@ void fullLogin_shouldRejectWhenWhitelistExcludesPlayer(ILightkeeperFramework fra } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramework framework) { // setup — join, then give the bot a distinctive item that must survive the quit/rejoin cycle. The @@ -214,7 +211,7 @@ void fullLogin_shouldRejoinAfterQuitWithSamePersistedIdentity(ILightkeeperFramew } @Test - @Timeout(60) + @Timeout(180) void fullLogin_shouldFireRealQuitEventOnRemoval(ILightkeeperFramework framework) { // setup — capture quit events BEFORE removal so the event cannot be missed. diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java index 06242f29..e7b3877d 100644 --- a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java @@ -33,9 +33,10 @@ public final class LoopbackLoginGuard /** * Matches an active {@code bungeecord: true} entry in {@code spigot.yml} (the key only exists under the * top-level {@code settings} section in stock Spigot configurations), with an optional trailing comment. + * The boolean is matched case-insensitively ({@code True}/{@code TRUE} are legal YAML booleans). */ private static final Pattern SPIGOT_BUNGEECORD_ENABLED_PATTERN = - Pattern.compile("^\\s*bungeecord:\\s*true\\s*(?:#.*)?$"); + Pattern.compile("^\\s*bungeecord:\\s*(?i:true)\\s*(?:#.*)?$"); /** * Matches the opening of the {@code velocity} section in {@code config/paper-global.yml}, capturing its @@ -45,9 +46,11 @@ public final class LoopbackLoginGuard /** * Matches an active {@code enabled: true} entry (inside the {@code velocity} section), with an optional - * trailing comment. + * trailing comment. The boolean is matched case-insensitively ({@code True}/{@code TRUE} are legal YAML + * booleans). */ - private static final Pattern VELOCITY_ENABLED_PATTERN = Pattern.compile("^\\s*enabled:\\s*true\\s*(?:#.*)?$"); + private static final Pattern VELOCITY_ENABLED_PATTERN = + Pattern.compile("^\\s*enabled:\\s*(?i:true)\\s*(?:#.*)?$"); private LoopbackLoginGuard() { @@ -81,10 +84,15 @@ private static void validateServerProperties(Path serverPropertiesFile) for (final String line : readLinesIfPresent(serverPropertiesFile)) { final String trimmed = line.trim(); - if (trimmed.startsWith("#") || !trimmed.startsWith("online-mode=")) + if (trimmed.startsWith("#")) continue; - final String value = trimmed.substring("online-mode=".length()).trim().toLowerCase(Locale.ROOT); + // Split at the first '=' so the legal properties spacing 'online-mode = true' is handled too. + final int separatorIndex = trimmed.indexOf('='); + if (separatorIndex < 0 || !"online-mode".equals(trimmed.substring(0, separatorIndex).trim())) + continue; + + final String value = trimmed.substring(separatorIndex + 1).trim().toLowerCase(Locale.ROOT); if ("true".equals(value)) { throw new MojoExecutionException( diff --git a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java index 46180d58..367cd3a9 100644 --- a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java +++ b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java @@ -93,6 +93,53 @@ void validate_shouldFailWhenPaperVelocityForwardingIsEnabled(@TempDir Path serve .hasMessageContaining("FULL_LOGIN"); } + @Test + void validate_shouldFailWhenOnlineModeUsesSpacedAssignment(@TempDir Path serverDirectory) + throws Exception + { + // setup — legal properties spacing around the separator. + writeServerProperties(serverDirectory, "online-mode = true"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("online-mode=true") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenBungeecordUsesCapitalizedYamlBoolean(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'True' is a legal YAML boolean spelling. + writeSpigotConfiguration(serverDirectory, "settings:", " bungeecord: True"); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("BungeeCord") + .hasMessageContaining("FULL_LOGIN"); + } + + @Test + void validate_shouldFailWhenVelocityUsesUppercaseYamlBoolean(@TempDir Path serverDirectory) + throws Exception + { + // setup — 'TRUE' is a legal YAML boolean spelling. + writePaperGlobalConfiguration( + serverDirectory, + "proxies:", + " velocity:", + " enabled: TRUE" + ); + + // execute + verify + assertThatThrownBy(() -> LoopbackLoginGuard.validate(serverDirectory, new SystemStreamLog())) + .isInstanceOf(MojoExecutionException.class) + .hasMessageContaining("Velocity") + .hasMessageContaining("FULL_LOGIN"); + } + @Test void validate_shouldIgnoreEnabledFlagsOutsideTheVelocitySection(@TempDir Path serverDirectory) throws Exception diff --git a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java index 1db2220f..867c4d5e 100644 --- a/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java +++ b/lightkeeper-nms-parent/lightkeeper-nms-v1_21_R7/src/main/java/nl/pim16aap2/lightkeeper/nms/v121r7/BotLoginReflection.java @@ -419,6 +419,8 @@ private void awaitChannelActive(Object connection) final long deadlineNanos = System.nanoTime() + TimeUnit.SECONDS.toNanos(CHANNEL_ACTIVE_TIMEOUT_SECONDS); while (System.nanoTime() - deadlineNanos < 0) { + // Non-volatile cross-thread read by design: vanilla's own isConnected() reads this same field + // cross-thread, and the reflective Field.get defeats any hoisting of the read out of the loop. final Object channel = channelField.get(connection); if (channel != null && Boolean.TRUE.equals(channelIsOpenMethod.invoke(channel))) return; From 83ab6572fa16a61901816763b1eaa36ba17ef732 Mon Sep 17 00:00:00 2001 From: Pim van der Loos Date: Thu, 16 Jul 2026 07:34:20 +0200 Subject: [PATCH 10/10] fix: treat '!' as a properties comment prefix in the loopback guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Goal: address Copilot's bot-round finding on PR #101 — java.util.Properties recognizes both '#' and '!' as comment prefixes, so a commented-out '! online-mode=true' must not fail provisioning. Guard skips both; the commented-settings unit test now covers the '!' spelling. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01P15FjGQzyNmX9T6Qmyq8Dn --- .../lightkeeper/maven/provisioning/LoopbackLoginGuard.java | 3 ++- .../lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java index e7b3877d..ec7bec00 100644 --- a/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java +++ b/lightkeeper-maven-plugin/src/main/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuard.java @@ -84,7 +84,8 @@ private static void validateServerProperties(Path serverPropertiesFile) for (final String line : readLinesIfPresent(serverPropertiesFile)) { final String trimmed = line.trim(); - if (trimmed.startsWith("#")) + // Java Properties comments start with '#' OR '!' (java.util.Properties spec). + if (trimmed.startsWith("#") || trimmed.startsWith("!")) continue; // Split at the first '=' so the legal properties spacing 'online-mode = true' is handled too. diff --git a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java index 367cd3a9..5b56f840 100644 --- a/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java +++ b/lightkeeper-maven-plugin/src/test/java/nl/pim16aap2/lightkeeper/maven/provisioning/LoopbackLoginGuardTest.java @@ -166,7 +166,8 @@ void validate_shouldIgnoreCommentedOutForwardingSettings(@TempDir Path serverDir throws Exception { // setup - writeServerProperties(serverDirectory, "# online-mode=true", "online-mode=false"); + writeServerProperties( + serverDirectory, "# online-mode=true", "! online-mode=true", "online-mode=false"); writeSpigotConfiguration(serverDirectory, "settings:", " # bungeecord: true"); writePaperGlobalConfiguration( serverDirectory,