From cce0a835407387a3502740488bd95493172e2b15 Mon Sep 17 00:00:00 2001 From: "Gabriele C." Date: Tue, 29 Sep 2026 00:06:54 +0200 Subject: [PATCH 1/2] fix(security): keep quit processing fail-closed so no stale login survives An exception thrown while a player quit could abort the quit processing before the player was removed from the PlayerCache, leaving the name authenticated after the player had left. On Folia this happened whenever an ender pearl was owned by another region, since the quit handler read the shooter of every pearl. Quit processing: - PlayerListener.onPlayerQuit runs performQuit in a finally block and only reads pearls owned by the current region; the antibot kick list no longer skips it - AsynchronousQuit removes the player from the PlayerCache in a finally block, treats the quit-location save as best-effort, drops the name-keyed proxy and premium auto-login grants and resets a pending 2FA limbo state - a connecting player discards the cached authentication of a name that is not online (AsyncPlayerPreLoginEvent, LOWEST) - onPlayerKick no longer processes the quit: PlayerQuitEvent is fired for kicked players too, and the kick may still be cancelled by another listener Related hardening: - SchedulingAdapter/BukkitService: isOwnedByCurrentThread(Entity|Location), also used by the pearl and vehicle restore at login - console commands are dispatched on the global region (Folia) - the pearl/vehicle snapshot saved on quit no longer records the operator flag, which the limbo merge re-applied on the next login even after a deop - SpawnLoader falls back to the spawn when the respawn location cannot be read, OnShutdownPlayerSaver handles each player separately and the email recall task no longer uses the Bukkit scheduler - unauthenticated players are no longer moved by pearls, chorus fruit, portals or spectator teleports, and can no longer pick up arrows, break hanging entities or vehicles, or use items by clicking in the air Co-Authored-By: Claude Opus 5.5 --- .../xephi/authme/data/limbo/LimboService.java | 50 ++++- .../initialization/OnShutdownPlayerSaver.java | 25 ++- .../authme/initialization/OnStartupTasks.java | 3 +- .../xephi/authme/listener/EntityListener.java | 26 +++ .../xephi/authme/listener/PlayerListener.java | 129 ++++++++++-- .../AbstractSpigotPlatformAdapter.java | 5 + .../authme/platform/SchedulingAdapter.java | 7 + .../authme/process/quit/AsynchronousQuit.java | 102 +++++++--- .../xephi/authme/service/BukkitService.java | 35 +++- .../authme/service/PremiumLoginVerifier.java | 11 ++ .../service/bungeecord/BungeeReceiver.java | 2 + .../fr/xephi/authme/settings/SpawnLoader.java | 12 +- .../authme/data/limbo/LimboServiceTest.java | 69 +++++++ .../authme/listener/EntityListenerTest.java | 66 +++++++ .../authme/listener/PlayerListenerTest.java | 183 ++++++++++++++++-- .../process/quit/AsynchronousQuitTest.java | 147 ++++++++++++++ .../authme/service/BukkitServiceTest.java | 19 ++ .../authme/platform/FoliaPlatformAdapter.java | 6 + 18 files changed, 812 insertions(+), 85 deletions(-) create mode 100644 authme-core/src/test/java/fr/xephi/authme/process/quit/AsynchronousQuitTest.java diff --git a/authme-core/src/main/java/fr/xephi/authme/data/limbo/LimboService.java b/authme-core/src/main/java/fr/xephi/authme/data/limbo/LimboService.java index 3cf57d0519..aebb5e9678 100644 --- a/authme-core/src/main/java/fr/xephi/authme/data/limbo/LimboService.java +++ b/authme-core/src/main/java/fr/xephi/authme/data/limbo/LimboService.java @@ -3,6 +3,7 @@ import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.limbo.persistence.LimboPersistence; import fr.xephi.authme.output.ConsoleLoggerFactory; +import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.TeleportationService; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.SpawnLoader; @@ -61,6 +62,9 @@ public class LimboService { @Inject private TeleportationService teleportationService; + @Inject + private BukkitService bukkitService; + LimboService() { } @@ -287,17 +291,36 @@ private Optional getLimboOrLogError(Player player, String context) * @param pearls tracked ender pearls of the player's in-flight ender pearls */ public void saveEnderPearlsForPlayer(Player player, Collection pearls) { - LimboPlayer limbo = persistence.getLimboPlayer(player); - if (limbo == null) { - limbo = new LimboPlayer(null, player.isOp(), Collections.emptyList(), player.getAllowFlight(), - player.getWalkSpeed(), player.getFlySpeed()); - } + LimboPlayer limbo = getOrCreateEntityCarrierLimbo(player); limbo.setEnderPearls(pearls); persistence.saveLimboPlayer(player, limbo); logger.debug("Saved {0} ender pearl(s) to disk for `{1}`", pearls.size(), player.getName().toLowerCase(Locale.ROOT)); } + /** + * Returns the on-disk limbo record to attach in-flight ender pearl / vehicle data to when an + * authenticated player quits, creating one if none exists. + *

+ * The fallback must NOT capture the quitting player's operator status: this record is read back and + * {@link LimboServiceHelper#merge merged} into the next limbo at join, and the merge ORs the operator + * flag. The player is logged in here, so AuthMe has not revoked anything and ops.json is authoritative; + * recording {@code player.isOp()} would re-grant OP on the next login even after an admin ran + * {@code /deop} while the player was offline. Flight and speeds are kept: they are the player's real + * values and {@link #restoreSpeedsForAutoLogin} restores them after a session or premium auto-login. + * + * @param player the authenticated player who is quitting + * @return the limbo record to store the entity data on + */ + private LimboPlayer getOrCreateEntityCarrierLimbo(Player player) { + LimboPlayer limbo = persistence.getLimboPlayer(player); + if (limbo == null) { + limbo = new LimboPlayer(null, false, Collections.emptyList(), player.getAllowFlight(), + player.getWalkSpeed(), player.getFlySpeed()); + } + return limbo; + } + /** * Saves ender pearl UUIDs without detailed state. This legacy overload is kept so older * callers and persisted data can still be handled even though recreation requires more data. @@ -320,11 +343,7 @@ public void saveEnderPearlsForPlayer(Player player, java.util.Set pearlUui * @param vehicleType the entity type of the vehicle */ public void saveVehicleForPlayer(Player player, UUID vehicleUuid, EntityType vehicleType) { - LimboPlayer limbo = persistence.getLimboPlayer(player); - if (limbo == null) { - limbo = new LimboPlayer(null, player.isOp(), Collections.emptyList(), player.getAllowFlight(), - player.getWalkSpeed(), player.getFlySpeed()); - } + LimboPlayer limbo = getOrCreateEntityCarrierLimbo(player); limbo.setVehicle(vehicleUuid, vehicleType); persistence.saveLimboPlayer(player, limbo); logger.debug("Saved vehicle {0} ({1}) to disk for `{2}`", @@ -369,6 +388,10 @@ private void doRestoreEntities(Player player, LimboPlayer limbo) { outer: for (World world : player.getServer().getWorlds()) { for (Entity entity : world.getEntities()) { + // On Folia, entities in other regions cannot be accessed from the player's thread + if (!bukkitService.isOwnedByCurrentThread(entity)) { + continue; + } if (!pendingPearls.isEmpty() && entity instanceof EnderPearl && pendingPearls.containsKey(entity.getUniqueId())) { ((EnderPearl) entity).setShooter(player); @@ -418,6 +441,13 @@ private boolean recreateEnderPearl(Player player, EnderPearlRestoreData pearl) { pearl.getUuid(), player.getName().toLowerCase(Locale.ROOT)); return false; } + // On Folia, a pearl saved in another region may still exist there without being visible from this + // thread, and entities cannot be spawned outside the current region anyway + if (!bukkitService.isOwnedByCurrentThread(pearl.getLocation())) { + logger.debug("Unable to recreate ender pearl {0} for `{1}` because its saved location is in another region", + pearl.getUuid(), player.getName().toLowerCase(Locale.ROOT)); + return false; + } EnderPearl recreatedPearl = (EnderPearl) pearl.getLocation().getWorld() .spawnEntity(pearl.getLocation(), EntityType.ENDER_PEARL); diff --git a/authme-core/src/main/java/fr/xephi/authme/initialization/OnShutdownPlayerSaver.java b/authme-core/src/main/java/fr/xephi/authme/initialization/OnShutdownPlayerSaver.java index 261e7dd59f..577baecfc8 100644 --- a/authme-core/src/main/java/fr/xephi/authme/initialization/OnShutdownPlayerSaver.java +++ b/authme-core/src/main/java/fr/xephi/authme/initialization/OnShutdownPlayerSaver.java @@ -1,9 +1,11 @@ package fr.xephi.authme.initialization; +import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; +import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.ValidationService; import fr.xephi.authme.settings.Settings; @@ -21,6 +23,8 @@ */ public class OnShutdownPlayerSaver { + private final ConsoleLogger logger = ConsoleLoggerFactory.get(OnShutdownPlayerSaver.class); + @Inject private BukkitService bukkitService; @Inject @@ -44,7 +48,13 @@ public class OnShutdownPlayerSaver { */ public void saveAllPlayers() { for (Player player : bukkitService.getOnlinePlayers()) { - savePlayer(player); + try { + savePlayer(player); + } catch (RuntimeException e) { + // One failing player (e.g. a region-owned read on Folia's shutdown thread) must not prevent the + // limbo data of all the remaining players from being restored + logger.logException("Could not save the data of '" + player.getName() + "' on shutdown:", e); + } } } @@ -53,12 +63,15 @@ private void savePlayer(Player player) { if (PlayerUtils.isNpc(player) || validationService.isUnrestricted(name)) { return; } - if (limboService.hasLimboPlayer(name)) { - limboService.restoreData(player); - } else { - saveLoggedinPlayer(player); + try { + if (limboService.hasLimboPlayer(name)) { + limboService.restoreData(player); + } else { + saveLoggedinPlayer(player); + } + } finally { + playerCache.removePlayer(name); } - playerCache.removePlayer(name); } private void saveLoggedinPlayer(Player player) { diff --git a/authme-core/src/main/java/fr/xephi/authme/initialization/OnStartupTasks.java b/authme-core/src/main/java/fr/xephi/authme/initialization/OnStartupTasks.java index 472220bfff..b141c25bd8 100644 --- a/authme-core/src/main/java/fr/xephi/authme/initialization/OnStartupTasks.java +++ b/authme-core/src/main/java/fr/xephi/authme/initialization/OnStartupTasks.java @@ -99,7 +99,8 @@ public void scheduleRecallEmailTask() { } bukkitService.runTaskTimerAsynchronously(() -> { List loggedPlayersWithEmptyMail = dataSource.getLoggedPlayersWithEmptyMail(); - bukkitService.runTask(() -> { + // Not runTask: the Bukkit scheduler is unsupported on Folia and would throw on every run + bukkitService.runOnGlobalRegion(() -> { for (String playerWithoutMail : loggedPlayersWithEmptyMail) { Player player = bukkitService.getPlayerExact(playerWithoutMail); if (player != null) { diff --git a/authme-core/src/main/java/fr/xephi/authme/listener/EntityListener.java b/authme-core/src/main/java/fr/xephi/authme/listener/EntityListener.java index 35e109204b..59e1c05dfc 100644 --- a/authme-core/src/main/java/fr/xephi/authme/listener/EntityListener.java +++ b/authme-core/src/main/java/fr/xephi/authme/listener/EntityListener.java @@ -13,6 +13,9 @@ import org.bukkit.event.entity.EntityTargetEvent; import org.bukkit.event.entity.FoodLevelChangeEvent; import org.bukkit.event.entity.ProjectileLaunchEvent; +import org.bukkit.event.hanging.HangingBreakByEntityEvent; +import org.bukkit.event.vehicle.VehicleDamageEvent; +import org.bukkit.event.vehicle.VehicleDestroyEvent; import org.bukkit.projectiles.ProjectileSource; import javax.inject.Inject; @@ -96,4 +99,27 @@ public void onShoot(EntityShootBowEvent event) { event.setCancelled(true); } } + + // Non-living entities hit by a player do not go through EntityDamageByEntityEvent: paintings, item frames and + // leash knots are broken through HangingBreakByEntityEvent, boats and minecarts through the vehicle events. + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onHangingBreakByEntity(HangingBreakByEntityEvent event) { + if (listenerService.shouldCancelEvent(event.getRemover())) { + event.setCancelled(true); + } + } + + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onVehicleDamage(VehicleDamageEvent event) { + if (listenerService.shouldCancelEvent(event.getAttacker())) { + event.setCancelled(true); + } + } + + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onVehicleDestroy(VehicleDestroyEvent event) { + if (listenerService.shouldCancelEvent(event.getAttacker())) { + event.setCancelled(true); + } + } } diff --git a/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java b/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java index c5ff10cc6e..09416322da 100644 --- a/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java +++ b/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java @@ -1,12 +1,15 @@ package fr.xephi.authme.listener; +import fr.xephi.authme.ConsoleLogger; import fr.xephi.authme.data.QuickCommandsProtectionManager; import fr.xephi.authme.data.auth.PlayerAuth; +import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.limbo.EnderPearlRestoreData; import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; import fr.xephi.authme.message.Messages; +import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.permission.PlayerStatePermission; import fr.xephi.authme.platform.ChatAdapter; @@ -52,10 +55,13 @@ import org.bukkit.event.player.PlayerKickEvent; import org.bukkit.event.player.PlayerMoveEvent; import org.bukkit.event.entity.EntityPickupItemEvent; +import org.bukkit.event.player.PlayerPickupArrowEvent; +import org.bukkit.event.player.PlayerPortalEvent; import org.bukkit.event.player.PlayerQuitEvent; import org.bukkit.event.player.PlayerRespawnEvent; import org.bukkit.event.player.PlayerShearEntityEvent; import org.bukkit.event.player.PlayerSwapHandItemsEvent; +import org.bukkit.event.player.PlayerTeleportEvent; import org.bukkit.inventory.InventoryView; import javax.inject.Inject; @@ -72,6 +78,8 @@ */ public class PlayerListener implements Listener { + private final ConsoleLogger logger = ConsoleLoggerFactory.get(PlayerListener.class); + @Inject private Settings settings; @Inject @@ -106,14 +114,18 @@ public class PlayerListener implements Listener { private ChatAdapter chatAdapter; @Inject private TeleportAdapter teleportAdapter; + @Inject + private PlayerCache playerCache; // Lowest priority to apply fast protection checks @EventHandler(priority = EventPriority.LOWEST) public void onAsyncPlayerPreLoginEventLowest(AsyncPlayerPreLoginEvent event) { + final String name = event.getName(); + removeStaleAuthentication(name); + if (event.getLoginResult() != AsyncPlayerPreLoginEvent.Result.ALLOWED) { return; } - final String name = event.getName(); // NOTE: getAddress() sometimes returning null, we don't want to handle this race condition if (event.getAddress() == null) { @@ -135,6 +147,30 @@ public void onAsyncPlayerPreLoginEventLowest(AsyncPlayerPreLoginEvent event) { } } + /** + * Discards the authentication cached for the given name if no player with that name is online. Such an + * entry can only be a leftover, e.g. of a quit that was not processed, and it must never be inherited by + * a new connection: the connecting player would be considered logged in without providing any password. + * + * @param name the name of the connecting player + */ + private void removeStaleAuthentication(String name) { + if (playerCache.isAuthenticated(name) && !isPlayerOnline(name)) { + playerCache.removePlayer(name); + logger.warning("Discarded the stale authentication of '" + name + "': no player with this name is " + + "online, their previous quit has not been processed"); + } + } + + private boolean isPlayerOnline(String name) { + for (Player player : bukkitService.getOnlinePlayers()) { + if (player.getName().equalsIgnoreCase(name)) { + return true; + } + } + return false; + } + /* * Login/join/leave events */ @@ -234,25 +270,32 @@ public void onPlayerQuit(PlayerQuitEvent event) { } } - if (antiBotService.wasPlayerKicked(player.getName())) { - return; - } - - // Save in-flight ender pearls and vehicle state to disk so they can be restored on reconnect. - // Only authenticated players can throw pearls or ride vehicles, so we skip unauthenticated ones. - if (!listenerService.shouldCancelEvent(player)) { - saveStateBeforeQuit(player); + try { + // Antibot-kicked names were denied at pre-login and never held a real session, so we skip the + // state save and speed restore for them, but performQuit below must still run (see the finally). + if (!antiBotService.wasPlayerKicked(player.getName())) { + // Save in-flight ender pearls and vehicle state to disk so they can be restored on reconnect. + // Only authenticated players can throw pearls or ride vehicles, so we skip unauthenticated ones. + if (!listenerService.shouldCancelEvent(player)) { + saveStateBeforeQuit(player); + } + + // Restore speed synchronously before the async quit, so the player's + // .dat file is saved with the correct speed (not 0.0f). + limboService.restoreSpeedSync(player); + } + } finally { + // Must run even if the steps above fail or are skipped: the quit process removes the player from + // the PlayerCache, and a leftover entry would let anyone joining with this name in without logging in + management.performQuit(player); } - - // Restore speed synchronously before the async quit, so the player's - // .dat file is saved with the correct speed (not 0.0f). - limboService.restoreSpeedSync(player); - management.performQuit(player); } private void saveStateBeforeQuit(Player player) { Set pearls = player.getServer().getWorlds().stream() .flatMap(world -> world.getEntitiesByClass(EnderPearl.class).stream()) + // On Folia, pearls in other regions cannot be accessed from the quitting player's thread + .filter(bukkitService::isOwnedByCurrentThread) .filter(pearl -> player.equals(pearl.getShooter())) .map(pearl -> new EnderPearlRestoreData(pearl.getUniqueId(), pearl.getLocation(), pearl.getVelocity())) .collect(Collectors.toSet()); @@ -275,13 +318,10 @@ public void onPlayerKick(PlayerKickEvent event) { if (settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION) && chatAdapter.getKickReason(event).contains("You logged in from another location")) { event.setCancelled(true); - return; - } - - final Player player = event.getPlayer(); - if (!antiBotService.wasPlayerKicked(player.getName())) { - management.performQuit(player); } + // The quit itself is processed in onPlayerQuit, which is also fired for kicked players. Processing it + // here as well ran it twice, and ran it even when a later listener cancelled the kick, leaving the + // player online but logged out and without any limbo restrictions. } /* @@ -404,11 +444,50 @@ public void onPlayerRespawn(PlayerRespawnEvent event) { bukkitService.runTaskLater(player, () -> limboService.reapplyLimboRestrictions(player), 1L); } + // PlayerTeleportEvent has its own handler list, so onPlayerMove never sees teleports: block the ones that are + // not initiated by a plugin or command (AuthMe's own spawn/login teleports use the PLUGIN cause). + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onPlayerTeleport(PlayerTeleportEvent event) { + if (isUnauthorizedTeleportCause(event.getCause()) && listenerService.shouldCancelEvent(event)) { + event.setCancelled(true); + } + } + + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onPlayerPortal(PlayerPortalEvent event) { + if (listenerService.shouldCancelEvent(event)) { + event.setCancelled(true); + } + } + + /** + * Returns whether a teleport with the given cause moves a player on their own (ender pearls, chorus fruit, + * portals, spectator teleports) and must therefore not happen before they log in. Compared by name because + * the constants differ across the supported versions (CHORUS_FRUIT was renamed CONSUMABLE_EFFECT in 1.21.2). + */ + private static boolean isUnauthorizedTeleportCause(PlayerTeleportEvent.TeleportCause cause) { + switch (cause.name()) { + case "ENDER_PEARL": + case "CHORUS_FRUIT": + case "CONSUMABLE_EFFECT": + case "NETHER_PORTAL": + case "END_PORTAL": + case "END_GATEWAY": + case "SPECTATE": + return true; + default: + return false; + } + } + /* * Entity/block interaction events */ - @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + // Not ignoreCancelled: Bukkit fires clicks on air already cancelled (no block to use), so an ignoreCancelled + // handler would never see them and the item in hand would still be used (buckets, spawn eggs, ...). + // Cancelling here denies both the block interaction and the use of the item in hand. + @EventHandler(priority = EventPriority.LOWEST) public void onPlayerInteract(PlayerInteractEvent event) { if (listenerService.shouldCancelEvent(event)) { event.setCancelled(true); @@ -493,6 +572,14 @@ public void onPlayerPickupItem(EntityPickupItemEvent event) { } } + // Arrows and tridents are picked up through their own event, not EntityPickupItemEvent + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) + public void onPlayerPickupArrow(PlayerPickupArrowEvent event) { + if (listenerService.shouldCancelEvent(event)) { + event.setCancelled(true); + } + } + @EventHandler(ignoreCancelled = true, priority = EventPriority.LOWEST) public void onPlayerDropItem(PlayerDropItemEvent event) { if (listenerService.shouldCancelEvent(event)) { diff --git a/authme-core/src/main/java/fr/xephi/authme/platform/AbstractSpigotPlatformAdapter.java b/authme-core/src/main/java/fr/xephi/authme/platform/AbstractSpigotPlatformAdapter.java index 15aa1ef953..37342a7ed3 100644 --- a/authme-core/src/main/java/fr/xephi/authme/platform/AbstractSpigotPlatformAdapter.java +++ b/authme-core/src/main/java/fr/xephi/authme/platform/AbstractSpigotPlatformAdapter.java @@ -49,6 +49,11 @@ public boolean isOwnedByCurrentThread(Entity entity) { return Bukkit.isPrimaryThread(); } + @Override + public boolean isOwnedByCurrentThread(Location location) { + return Bukkit.isPrimaryThread(); + } + @Override public boolean isGlobalThread() { return Bukkit.isPrimaryThread(); diff --git a/authme-core/src/main/java/fr/xephi/authme/platform/SchedulingAdapter.java b/authme-core/src/main/java/fr/xephi/authme/platform/SchedulingAdapter.java index 7cf7b295d7..0002016e45 100644 --- a/authme-core/src/main/java/fr/xephi/authme/platform/SchedulingAdapter.java +++ b/authme-core/src/main/java/fr/xephi/authme/platform/SchedulingAdapter.java @@ -2,6 +2,7 @@ import fr.xephi.authme.AuthMe; import fr.xephi.authme.service.CancellableTask; +import org.bukkit.Location; import org.bukkit.entity.Entity; /** @@ -17,6 +18,12 @@ public interface SchedulingAdapter { */ boolean isOwnedByCurrentThread(Entity entity); + /** + * @param location the location whose world area would be accessed + * @return true if the current thread may already safely interact with the world at the location + */ + boolean isOwnedByCurrentThread(Location location); + /** * @return true if the current thread may already safely perform global server work */ diff --git a/authme-core/src/main/java/fr/xephi/authme/process/quit/AsynchronousQuit.java b/authme-core/src/main/java/fr/xephi/authme/process/quit/AsynchronousQuit.java index add373def9..25a5d01a12 100644 --- a/authme-core/src/main/java/fr/xephi/authme/process/quit/AsynchronousQuit.java +++ b/authme-core/src/main/java/fr/xephi/authme/process/quit/AsynchronousQuit.java @@ -1,14 +1,21 @@ package fr.xephi.authme.process.quit; import fr.xephi.authme.AuthMe; +import fr.xephi.authme.ConsoleLogger; +import fr.xephi.authme.data.ProxySessionManager; import fr.xephi.authme.data.VerificationCodeManager; import fr.xephi.authme.data.auth.PlayerAuth; import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.limbo.LimboPlayer; +import fr.xephi.authme.data.limbo.LimboPlayerState; +import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; +import fr.xephi.authme.output.ConsoleLoggerFactory; import fr.xephi.authme.process.AsynchronousProcess; import fr.xephi.authme.process.SyncProcessManager; import fr.xephi.authme.service.CommonService; import fr.xephi.authme.service.DialogStateService; +import fr.xephi.authme.service.PremiumLoginVerifier; import fr.xephi.authme.service.SessionService; import fr.xephi.authme.service.TeleportationService; import fr.xephi.authme.service.ValidationService; @@ -27,6 +34,8 @@ */ public class AsynchronousQuit implements AsynchronousProcess { + private final ConsoleLogger logger = ConsoleLoggerFactory.get(AsynchronousQuit.class); + @Inject private AuthMe plugin; @@ -60,6 +69,15 @@ public class AsynchronousQuit implements AsynchronousProcess { @Inject private DialogStateService dialogStateService; + @Inject + private ProxySessionManager proxySessionManager; + + @Inject + private PremiumLoginVerifier premiumLoginVerifier; + + @Inject + private LimboService limboService; + AsynchronousQuit() { } @@ -75,47 +93,71 @@ public void processQuit(Player player) { String name = player.getName().toLowerCase(Locale.ROOT); teleportationService.clearOriginalJoinLocation(name); dialogStateService.clearDialogOpen(player); + // Drop any name-keyed auto-login grant so it cannot be inherited by a later connection using this name + proxySessionManager.removeLoginRequest(name); + premiumLoginVerifier.removeVerified(name); + // A "password accepted, 2FA code pending" state belongs to this connection only. On Folia the sync + // quit that normally discards the in-memory limbo never runs (the player's scheduler is retired), so + // reset it: the next connection with this name must provide the password again. + LimboPlayer limbo = limboService.getLimboPlayer(name); + if (limbo != null) { + limbo.setState(LimboPlayerState.PASSWORD_REQUIRED); + } boolean wasLoggedIn = playerCache.isAuthenticated(name); - if (wasLoggedIn) { - if (service.getProperty(RestrictionSettings.SAVE_QUIT_LOCATION)) { - Location loc = spawnLoader.getPlayerLocationOrSpawn(player); - PlayerAuth auth = PlayerAuth.builder() - .name(name).location(loc) - .realName(player.getName()).build(); - database.updateQuitLoc(auth); + try { + if (wasLoggedIn) { + saveQuitState(player, name); + } + } finally { + // Security-critical: the player must end up unauthenticated even if a database write or the + // quit-location lookup above threw. A leftover PlayerCache entry would let anyone joining with + // this name in without a password on offline-mode servers. + playerCache.removePlayer(name); + codeManager.unverify(name); + + //always update the database when the player quit the game (if sessions are disabled) + if (wasLoggedIn) { + database.setUnlogged(name); + if (!service.getProperty(PluginSettings.SESSIONS_ENABLED)) { + sessionService.revokeSession(name); + } } - String ip = PlayerUtils.getPlayerIp(player); - PlayerAuth auth = PlayerAuth.builder() - .name(name) - .realName(player.getName()) - .lastIp(ip) - .lastLogin(System.currentTimeMillis()) - .build(); - database.updateSession(auth); + if (plugin.isEnabled()) { + syncProcessManager.processSyncPlayerQuit(player, wasLoggedIn); + } - // TODO: send an update when a messaging service will be implemented (QUITLOC) + // remove player from cache + database.invalidateCache(name); } + } - //always unauthenticate the player - use session only for auto logins on the same ip - playerCache.removePlayer(name); - codeManager.unverify(name); - - //always update the database when the player quit the game (if sessions are disabled) - if (wasLoggedIn) { - database.setUnlogged(name); - if (!service.getProperty(PluginSettings.SESSIONS_ENABLED)) { - sessionService.revokeSession(name); + private void saveQuitState(Player player, String name) { + if (service.getProperty(RestrictionSettings.SAVE_QUIT_LOCATION)) { + try { + Location loc = spawnLoader.getPlayerLocationOrSpawn(player); + PlayerAuth auth = PlayerAuth.builder() + .name(name).location(loc) + .realName(player.getName()).build(); + database.updateQuitLoc(auth); + } catch (RuntimeException e) { + // On Folia, reading the respawn location of a player who quit while dead can throw; the + // quit-location save is best-effort and must never abort the rest of the quit processing. + logger.warning("Could not save quit location for '" + name + "': " + e.getMessage()); } } - if (plugin.isEnabled()) { - syncProcessManager.processSyncPlayerQuit(player, wasLoggedIn); - } + String ip = PlayerUtils.getPlayerIp(player); + PlayerAuth auth = PlayerAuth.builder() + .name(name) + .realName(player.getName()) + .lastIp(ip) + .lastLogin(System.currentTimeMillis()) + .build(); + database.updateSession(auth); - // remove player from cache - database.invalidateCache(name); + // TODO: send an update when a messaging service will be implemented (QUITLOC) } } diff --git a/authme-core/src/main/java/fr/xephi/authme/service/BukkitService.java b/authme-core/src/main/java/fr/xephi/authme/service/BukkitService.java index f08617c876..34296088ab 100644 --- a/authme-core/src/main/java/fr/xephi/authme/service/BukkitService.java +++ b/authme-core/src/main/java/fr/xephi/authme/service/BukkitService.java @@ -102,6 +102,28 @@ public void scheduleSyncDelayedTask(Entity entity, Runnable task, long delay) { runTaskLater(entity, task, delay); } + /** + * Returns whether the current thread may interact with the given entity. On Folia, this is only + * the case if the entity belongs to the region ticked by the current thread. + * + * @param entity the entity to check + * @return true if the entity can be safely accessed from the current thread, false otherwise + */ + public boolean isOwnedByCurrentThread(Entity entity) { + return schedulingAdapter.isOwnedByCurrentThread(entity); + } + + /** + * Returns whether the current thread may interact with the world at the given location. On Folia, + * this is only the case if the location belongs to the region ticked by the current thread. + * + * @param location the location to check + * @return true if the location can be safely accessed from the current thread, false otherwise + */ + public boolean isOwnedByCurrentThread(Location location) { + return schedulingAdapter.isOwnedByCurrentThread(location); + } + /** * Schedules a synchronous task if we are currently on a async thread; if not, it runs the task immediately. * Use this when {@link #runTaskOptionallyAsync(Runnable) optionally asynchronous tasks} have to @@ -407,12 +429,21 @@ public boolean dispatchCommand(CommandSender sender, String commandLine) { /** * Dispatches a command to be run as console user on this server, and executes it if found. + *

+ * On Folia a console command must be dispatched from the global region thread; when this method is + * called from another thread (e.g. a player's region thread while running configured commands) the + * dispatch is handed off to the global region. In that case the command runs slightly later and the + * returned value is not the real dispatch result — no caller relies on it. * * @param commandLine the command + arguments. Example: test abc 123 - * @return returns false if no target is found + * @return false if the command has no target (only meaningful when already on the global thread) */ public boolean dispatchConsoleCommand(String commandLine) { - return Bukkit.dispatchCommand(Bukkit.getConsoleSender(), commandLine); + if (schedulingAdapter.isGlobalThread()) { + return Bukkit.dispatchCommand(Bukkit.getConsoleSender(), commandLine); + } + runOnGlobalRegion(() -> Bukkit.dispatchCommand(Bukkit.getConsoleSender(), commandLine)); + return true; } @Override diff --git a/authme-core/src/main/java/fr/xephi/authme/service/PremiumLoginVerifier.java b/authme-core/src/main/java/fr/xephi/authme/service/PremiumLoginVerifier.java index 1cbd7d36a3..aafbdd8511 100644 --- a/authme-core/src/main/java/fr/xephi/authme/service/PremiumLoginVerifier.java +++ b/authme-core/src/main/java/fr/xephi/authme/service/PremiumLoginVerifier.java @@ -184,6 +184,17 @@ public UUID getVerifiedUuid(String username) { return session.mojangUuid(); } + /** + * Discards any stored verified session for the given username. Called when the player disconnects so + * that a Mojang verification earned by one connection cannot be consumed by a later connection using + * the same (offline-mode, freely chosen) name. + * + * @param username the username whose verified session should be dropped + */ + public void removeVerified(String username) { + verified.remove(username.toLowerCase(Locale.ROOT)); + } + private byte[] rsaDecrypt(byte[] encrypted) throws GeneralSecurityException { Cipher cipher = Cipher.getInstance("RSA/ECB/PKCS1Padding"); cipher.init(Cipher.DECRYPT_MODE, rsaKeyPair.getPrivate()); diff --git a/authme-core/src/main/java/fr/xephi/authme/service/bungeecord/BungeeReceiver.java b/authme-core/src/main/java/fr/xephi/authme/service/bungeecord/BungeeReceiver.java index 56f0d93a8a..df1f53d59b 100644 --- a/authme-core/src/main/java/fr/xephi/authme/service/bungeecord/BungeeReceiver.java +++ b/authme-core/src/main/java/fr/xephi/authme/service/bungeecord/BungeeReceiver.java @@ -274,6 +274,8 @@ private void performLogin(String name, UUID verifiedPremiumUuid) { private void completeProxyLogin(Player player) { // Player is already online: also drive the login directly in case processJoin // has already run past the proxy-session check and created a limbo player. + // Consume the queued request so it cannot be inherited by a later connection with this name. + proxySessionManager.removeLoginRequest(player.getName()); management.forceLoginFromProxy(player); logger.debug("Sending auto-login ACK for {0}", player.getName()); bungeeSender.sendAuthMeBungeecordMessage(player, MessageType.PERFORM_LOGIN_ACK); diff --git a/authme-core/src/main/java/fr/xephi/authme/settings/SpawnLoader.java b/authme-core/src/main/java/fr/xephi/authme/settings/SpawnLoader.java index 6350f54b77..c151b0e27a 100644 --- a/authme-core/src/main/java/fr/xephi/authme/settings/SpawnLoader.java +++ b/authme-core/src/main/java/fr/xephi/authme/settings/SpawnLoader.java @@ -381,7 +381,17 @@ public Location getPlayerLocationOrSpawn(Player player) { * @return the player's respawn location if available, otherwise the configured spawn */ public Location getPlayerRespawnLocationOrSpawn(Player player) { - Location respawnLocation = teleportAdapter.getPlayerRespawnLocation(player); + Location respawnLocation; + try { + respawnLocation = teleportAdapter.getPlayerRespawnLocation(player); + } catch (RuntimeException e) { + // The respawn location is validated against the bed/anchor block. On Folia that block read throws + // when it is not owned by the current thread (async quit processing, a bed in another region), and + // callers such as the quit and limbo processes must not be aborted by it: use the spawn instead. + logger.debug("Could not read the respawn location of `{0}`, using the spawn instead: {1}", + player.getName(), e.getMessage()); + respawnLocation = null; + } if (respawnLocation != null && respawnLocation.getWorld() != null) { return respawnLocation; } diff --git a/authme-core/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java b/authme-core/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java index fbc1bf6042..96db931cb0 100644 --- a/authme-core/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java @@ -4,6 +4,7 @@ import fr.xephi.authme.TestHelper; import fr.xephi.authme.data.limbo.persistence.LimboPersistence; import fr.xephi.authme.permission.PermissionsManager; +import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.TeleportationService; import fr.xephi.authme.settings.Settings; import fr.xephi.authme.settings.SpawnLoader; @@ -13,6 +14,7 @@ import org.bukkit.Server; import org.bukkit.World; import org.bukkit.entity.EnderPearl; +import org.bukkit.entity.Entity; import org.bukkit.entity.EntityType; import org.bukkit.entity.Player; import org.bukkit.util.Vector; @@ -20,11 +22,13 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Map; @@ -79,6 +83,9 @@ class LimboServiceTest { @Mock private TeleportationService teleportationService; + @Mock + private BukkitService bukkitService; + @BeforeAll public static void initLogger() { TestHelper.setupLogger(); @@ -295,6 +302,7 @@ void shouldRecreateMissingEnderPearls() { World pearlWorld = mock(World.class); given(pearlWorld.spawnEntity(any(Location.class), eq(EntityType.ENDER_PEARL))).willReturn(recreatedPearl); Location pearlLocation = new Location(pearlWorld, 5.0, 64.0, -2.5, 15.0f, 30.0f); + given(bukkitService.isOwnedByCurrentThread(pearlLocation)).willReturn(true); Vector pearlVelocity = new Vector(0.01, 0.2, -0.03); UUID pearlUuid = UUID.nameUUIDFromBytes("missing-pearl".getBytes()); @@ -343,6 +351,66 @@ void shouldNotRecreateMissingEnderPearlsWhenDisabledInConfig() { assertThat(limbo.getEnderPearls(), hasSize(0)); } + @Test + void shouldNotCaptureOperatorInEntitySnapshot() { + // given - an OP player with flight quits with a pearl in flight and no existing disk limbo + Player player = newPlayer("Opped", true, 0.5f, true, 0.6f); + given(limboPersistence.getLimboPlayer(player)).willReturn(null); + EnderPearlRestoreData pearl = + new EnderPearlRestoreData(UUID.nameUUIDFromBytes("snapshot-pearl".getBytes()), null, null); + + // when + limboService.saveEnderPearlsForPlayer(player, Collections.singletonList(pearl)); + + // then - the snapshot carries the pearl but NOT the OP flag, so merge() at the next login cannot + // re-grant OP (e.g. after an admin ran /deop while the player was offline). Flight and speeds are + // kept because they are restored after a session/premium auto-login. + ArgumentCaptor captor = ArgumentCaptor.forClass(LimboPlayer.class); + verify(limboPersistence).saveLimboPlayer(eq(player), captor.capture()); + LimboPlayer saved = captor.getValue(); + assertThat(saved.isOperator(), equalTo(false)); + assertThat(saved.isCanFly(), equalTo(true)); + assertThat(saved.getWalkSpeed(), equalTo(0.5f)); + assertThat(saved.getFlySpeed(), equalTo(0.6f)); + assertThat(saved.getEnderPearls(), hasSize(1)); + } + + @Test + void shouldNotAccessEntitiesNorRecreatePearlsOutsideOfCurrentRegion() { + // given + Player player = newPlayer("Pearlie"); + Server server = mock(Server.class); + World world = mock(World.class); + EnderPearl foreignPearl = mock(EnderPearl.class); + Entity vehicle = mock(Entity.class); + UUID vehicleUuid = UUID.nameUUIDFromBytes("vehicle".getBytes()); + given(player.getServer()).willReturn(server); + given(server.getWorlds()).willReturn(Collections.singletonList(world)); + given(world.getEntities()).willReturn(Arrays.asList(foreignPearl, vehicle)); + given(bukkitService.isOwnedByCurrentThread(foreignPearl)).willReturn(false); + given(bukkitService.isOwnedByCurrentThread(vehicle)).willReturn(true); + given(vehicle.getUniqueId()).willReturn(vehicleUuid); + + World pearlWorld = mock(World.class); + Location pearlLocation = new Location(pearlWorld, 5.0, 64.0, -2.5, 15.0f, 30.0f); + given(bukkitService.isOwnedByCurrentThread(pearlLocation)).willReturn(false); + UUID pearlUuid = UUID.nameUUIDFromBytes("foreign-pearl".getBytes()); + + LimboPlayer limbo = new LimboPlayer(null, false, Collections.emptyList(), false, 0.0f, 0.0f); + limbo.setEnderPearls( + Collections.singletonList(new EnderPearlRestoreData(pearlUuid, pearlLocation, new Vector()))); + limbo.setVehicle(vehicleUuid, EntityType.PIG); + getLimboMap().put("pearlie", limbo); + given(settings.getProperty(LimboSettings.RECREATE_ENDER_PEARLS)).willReturn(true); + + // when + limboService.restoreEntities(player); + + // then - the pearl may still exist in its region: it must be neither touched nor duplicated + verifyNoInteractions(foreignPearl, pearlWorld); + verify(vehicle).addPassenger(player); + } + @Test void shouldHandleMissingLimboForReplaceTasks() { // given @@ -390,6 +458,7 @@ void shouldRestoreEnderPearlsForAutoLoginFromDiskLimbo() { World pearlWorld = mock(World.class); given(pearlWorld.spawnEntity(any(Location.class), eq(EntityType.ENDER_PEARL))).willReturn(recreatedPearl); Location pearlLocation = new Location(pearlWorld, 5.0, 64.0, -2.5, 15.0f, 30.0f); + given(bukkitService.isOwnedByCurrentThread(pearlLocation)).willReturn(true); Vector pearlVelocity = new Vector(0.01, 0.2, -0.03); UUID pearlUuid = UUID.nameUUIDFromBytes("premium-pearl".getBytes()); diff --git a/authme-core/src/test/java/fr/xephi/authme/listener/EntityListenerTest.java b/authme-core/src/test/java/fr/xephi/authme/listener/EntityListenerTest.java index e56353f84d..f404346527 100644 --- a/authme-core/src/test/java/fr/xephi/authme/listener/EntityListenerTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/listener/EntityListenerTest.java @@ -11,6 +11,9 @@ import org.bukkit.event.entity.EntityTargetEvent; import org.bukkit.event.entity.FoodLevelChangeEvent; import org.bukkit.event.entity.ProjectileLaunchEvent; +import org.bukkit.event.hanging.HangingBreakByEntityEvent; +import org.bukkit.event.vehicle.VehicleDamageEvent; +import org.bukkit.event.vehicle.VehicleDestroyEvent; import org.bukkit.projectiles.ProjectileSource; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -244,4 +247,67 @@ void shouldNotCancelEntityTargetEvent() { verify(listenerService).shouldCancelEvent(target); verify(event, only()).getTarget(); } + + @Test + void shouldCancelHangingBreakByUnauthenticatedPlayer() { + // given + HangingBreakByEntityEvent event = mock(HangingBreakByEntityEvent.class); + Entity player = mock(Player.class); + given(event.getRemover()).willReturn(player); + given(listenerService.shouldCancelEvent(player)).willReturn(true); + + // when + listener.onHangingBreakByEntity(event); + + // then + verify(event).setCancelled(true); + } + + @Test + void shouldNotCancelHangingBreakByAuthenticatedPlayer() { + // given + HangingBreakByEntityEvent event = mock(HangingBreakByEntityEvent.class); + Entity player = mock(Player.class); + given(event.getRemover()).willReturn(player); + given(listenerService.shouldCancelEvent(player)).willReturn(false); + + // when + listener.onHangingBreakByEntity(event); + + // then + verify(event, only()).getRemover(); + } + + @Test + void shouldCancelVehicleDamageAndDestroyByUnauthenticatedPlayer() { + // given + Entity player = mock(Player.class); + given(listenerService.shouldCancelEvent(player)).willReturn(true); + VehicleDamageEvent damageEvent = mock(VehicleDamageEvent.class); + given(damageEvent.getAttacker()).willReturn(player); + VehicleDestroyEvent destroyEvent = mock(VehicleDestroyEvent.class); + given(destroyEvent.getAttacker()).willReturn(player); + + // when + listener.onVehicleDamage(damageEvent); + listener.onVehicleDestroy(destroyEvent); + + // then + verify(damageEvent).setCancelled(true); + verify(destroyEvent).setCancelled(true); + } + + @Test + void shouldNotCancelVehicleDamageWithoutAttacker() { + // given - e.g. damage from lava or an explosion + VehicleDamageEvent event = mock(VehicleDamageEvent.class); + given(event.getAttacker()).willReturn(null); + given(listenerService.shouldCancelEvent((Entity) null)).willReturn(false); + + // when + listener.onVehicleDamage(event); + + // then + verify(event, only()).getAttacker(); + } } diff --git a/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java b/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java index bf88b1a5b6..411cc0a4a8 100644 --- a/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java @@ -1,7 +1,9 @@ package fr.xephi.authme.listener; +import fr.xephi.authme.TestHelper; import fr.xephi.authme.data.QuickCommandsProtectionManager; import fr.xephi.authme.data.auth.PlayerAuth; +import fr.xephi.authme.data.auth.PlayerCache; import fr.xephi.authme.data.limbo.LimboService; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.message.MessageKey; @@ -48,12 +50,16 @@ import org.bukkit.event.player.PlayerJoinEvent; import org.bukkit.event.player.PlayerKickEvent; import org.bukkit.event.player.PlayerMoveEvent; +import org.bukkit.event.player.PlayerPickupArrowEvent; +import org.bukkit.event.player.PlayerPortalEvent; import org.bukkit.event.player.PlayerQuitEvent; import org.bukkit.event.player.PlayerRespawnEvent; import org.bukkit.event.player.PlayerShearEntityEvent; import org.bukkit.event.player.PlayerSwapHandItemsEvent; +import org.bukkit.event.player.PlayerTeleportEvent; import org.bukkit.inventory.InventoryView; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; @@ -80,6 +86,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.nullValue; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; @@ -139,6 +146,13 @@ class PlayerListenerTest { private ChatAdapter chatAdapter; @Mock private TeleportAdapter teleportAdapter; + @Mock + private PlayerCache playerCache; + + @BeforeAll + static void initLogger() { + TestHelper.setupLogger(); + } @AfterEach void resetSpawnLocationTracker() throws ReflectiveOperationException { @@ -171,18 +185,15 @@ void shouldCancelKick() { void shouldNotCancelKick() { // given given(settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)).willReturn(false); - String name = "Bobby"; - Player player = mockPlayerWithName(name); + Player player = mock(Player.class); PlayerKickEvent event = new PlayerKickEvent(player, "You logged in from another location", ""); - given(antiBotService.wasPlayerKicked(name)).willReturn(false); // when listener.onPlayerKick(event); - // then + // then - the quit is processed by onPlayerQuit, which Bukkit fires for kicked players as well assertThat(event.isCancelled(), equalTo(false)); - verify(antiBotService).wasPlayerKicked(name); - verify(management).performQuit(player); + verifyNoInteractions(management, antiBotService); } @Test @@ -190,18 +201,15 @@ void shouldNotCancelOrdinaryKick() { // given given(settings.getProperty(RestrictionSettings.FORCE_SINGLE_SESSION)).willReturn(true); given(chatAdapter.getKickReason(any())).willAnswer(inv -> inv.getArgument(0, PlayerKickEvent.class).getReason()); - String name = "Bobby"; - Player player = mockPlayerWithName(name); + Player player = mock(Player.class); PlayerKickEvent event = new PlayerKickEvent(player, "No longer desired here!", ""); - given(antiBotService.wasPlayerKicked(name)).willReturn(true); // when listener.onPlayerKick(event); - // then + // then - no quit processing here: a later listener could still cancel the kick assertThat(event.isCancelled(), equalTo(false)); - verify(antiBotService).wasPlayerKicked(name); - verifyNoInteractions(management); + verifyNoInteractions(management, antiBotService); } @Test @@ -217,7 +225,51 @@ void shouldHandleSimpleCancelableEvents() { .check(listener::onPlayerConsumeItem, PlayerItemConsumeEvent.class) .check(listener::onPlayerInteract, PlayerInteractEvent.class) .check(listener::onPlayerInteractEntity, PlayerInteractEntityEvent.class) - .check(listener::onPlayerHeldItem, PlayerItemHeldEvent.class); + .check(listener::onPlayerHeldItem, PlayerItemHeldEvent.class) + .check(listener::onPlayerPortal, PlayerPortalEvent.class) + .check(listener::onPlayerPickupArrow, PlayerPickupArrowEvent.class); + } + + @Test + void shouldCancelSelfInitiatedTeleportOfUnauthenticatedPlayer() { + // given + PlayerTeleportEvent event = new PlayerTeleportEvent(mock(Player.class), mock(Location.class), + mock(Location.class), PlayerTeleportEvent.TeleportCause.ENDER_PEARL); + given(listenerService.shouldCancelEvent(event)).willReturn(true); + + // when + listener.onPlayerTeleport(event); + + // then + assertThat(event.isCancelled(), equalTo(true)); + } + + @Test + void shouldAllowPluginTeleportOfUnauthenticatedPlayer() { + // given - AuthMe's own spawn and login teleports use the PLUGIN cause + PlayerTeleportEvent event = new PlayerTeleportEvent(mock(Player.class), mock(Location.class), + mock(Location.class), PlayerTeleportEvent.TeleportCause.PLUGIN); + + // when + listener.onPlayerTeleport(event); + + // then + assertThat(event.isCancelled(), equalTo(false)); + verifyNoInteractions(listenerService); + } + + @Test + void shouldNotCancelTeleportOfAuthenticatedPlayer() { + // given + PlayerTeleportEvent event = new PlayerTeleportEvent(mock(Player.class), mock(Location.class), + mock(Location.class), PlayerTeleportEvent.TeleportCause.NETHER_PORTAL); + given(listenerService.shouldCancelEvent(event)).willReturn(false); + + // when + listener.onPlayerTeleport(event); + + // then + assertThat(event.isCancelled(), equalTo(false)); } @Test @@ -886,7 +938,10 @@ void shouldRemoveMessageForUnloggedUser() { // then assertThat(event.getQuitMessage(), nullValue()); verify(antiBotService).wasPlayerKicked(name); - verifyNoInteractions(management); + // The state save and speed restore are skipped for an antibot-kicked name... + verifyNoInteractions(limboService); + // ...but performQuit must still run so no stale authentication is left behind + verify(management).performQuit(player); } @Test @@ -930,6 +985,7 @@ void shouldSaveEnderPearlRestoreDataOnQuit() { given(player.getServer()).willReturn(server); given(server.getWorlds()).willReturn(Collections.singletonList(world)); given(world.getEntitiesByClass(EnderPearl.class)).willReturn(Collections.singletonList(pearl)); + given(bukkitService.isOwnedByCurrentThread(pearl)).willReturn(true); given(pearl.getShooter()).willReturn(player); given(pearl.getUniqueId()).willReturn(UUID.nameUUIDFromBytes("quit-pearl".getBytes())); given(pearl.getLocation()).willReturn(pearlLocation); @@ -952,6 +1008,105 @@ void shouldSaveEnderPearlRestoreDataOnQuit() { verify(management).performQuit(player); } + @Test + @SuppressWarnings("unchecked") + void shouldNotAccessEnderPearlsOwnedByOtherRegionsOnQuit() { + // given + String name = "Pearlie"; + Player player = mockPlayerWithName(name); + Server server = mock(Server.class); + World world = mock(World.class); + EnderPearl foreignPearl = mock(EnderPearl.class); + EnderPearl ownPearl = mock(EnderPearl.class); + + given(settings.getProperty(RegistrationSettings.REMOVE_LEAVE_MESSAGE)).willReturn(false); + given(settings.getProperty(RegistrationSettings.REMOVE_UNLOGGED_LEAVE_MESSAGE)).willReturn(false); + given(antiBotService.wasPlayerKicked(name)).willReturn(false); + given(listenerService.shouldCancelEvent(player)).willReturn(false); + given(player.getServer()).willReturn(server); + given(server.getWorlds()).willReturn(Collections.singletonList(world)); + given(world.getEntitiesByClass(EnderPearl.class)).willReturn(Arrays.asList(foreignPearl, ownPearl)); + given(bukkitService.isOwnedByCurrentThread(foreignPearl)).willReturn(false); + given(bukkitService.isOwnedByCurrentThread(ownPearl)).willReturn(true); + given(ownPearl.getShooter()).willReturn(player); + given(ownPearl.getUniqueId()).willReturn(UUID.nameUUIDFromBytes("own-pearl".getBytes())); + + PlayerQuitEvent event = new PlayerQuitEvent(player, "quit"); + org.mockito.ArgumentCaptor> captor = + org.mockito.ArgumentCaptor.forClass(Collection.class); + + // when + listener.onPlayerQuit(event); + + // then + verifyNoInteractions(foreignPearl); + verify(limboService).saveEnderPearlsForPlayer(eq(player), captor.capture()); + assertThat(captor.getValue(), hasSize(1)); + verify(management).performQuit(player); + } + + @Test + void shouldProcessQuitEvenIfSavingStateBeforeQuitFails() { + // given + String name = "Pearlie"; + Player player = mockPlayerWithName(name); + Server server = mock(Server.class); + World world = mock(World.class); + EnderPearl pearl = mock(EnderPearl.class); + + given(settings.getProperty(RegistrationSettings.REMOVE_LEAVE_MESSAGE)).willReturn(false); + given(settings.getProperty(RegistrationSettings.REMOVE_UNLOGGED_LEAVE_MESSAGE)).willReturn(false); + given(antiBotService.wasPlayerKicked(name)).willReturn(false); + given(listenerService.shouldCancelEvent(player)).willReturn(false); + given(player.getServer()).willReturn(server); + given(server.getWorlds()).willReturn(Collections.singletonList(world)); + given(world.getEntitiesByClass(EnderPearl.class)).willReturn(Collections.singletonList(pearl)); + given(bukkitService.isOwnedByCurrentThread(pearl)).willReturn(true); + given(pearl.getShooter()) + .willThrow(new IllegalStateException("Accessing entity state off owning region's thread")); + PlayerQuitEvent event = new PlayerQuitEvent(player, "quit"); + + // when + assertThrows(IllegalStateException.class, () -> listener.onPlayerQuit(event)); + + // then - the player must not stay authenticated, or anyone could join with this name without logging in + verify(management).performQuit(player); + } + + @Test + void shouldDiscardStaleAuthenticationOnPreLogin() { + // given + String name = "Bobby"; + UUID uniqueId = UUID.fromString("753493c9-33ba-4a4a-bf61-1bce9d3c9a71"); + AsyncPlayerPreLoginEvent event = new AsyncPlayerPreLoginEvent(name, createInetAddress("12.34.56.78"), uniqueId); + given(playerCache.isAuthenticated(name)).willReturn(true); + Player otherPlayer = mockPlayerWithName("Bob"); + given(bukkitService.getOnlinePlayers()).willReturn(Collections.singletonList(otherPlayer)); + + // when + listener.onAsyncPlayerPreLoginEventLowest(event); + + // then + verify(playerCache).removePlayer(name); + } + + @Test + void shouldKeepAuthenticationOfOnlinePlayerOnPreLogin() { + // given + String name = "Bobby"; + UUID uniqueId = UUID.fromString("753493c9-33ba-4a4a-bf61-1bce9d3c9a71"); + AsyncPlayerPreLoginEvent event = new AsyncPlayerPreLoginEvent(name, createInetAddress("12.34.56.78"), uniqueId); + given(playerCache.isAuthenticated(name)).willReturn(true); + Player onlinePlayer = mockPlayerWithName("bOBBY"); + given(bukkitService.getOnlinePlayers()).willReturn(Collections.singletonList(onlinePlayer)); + + // when + listener.onAsyncPlayerPreLoginEventLowest(event); + + // then + verify(playerCache, never()).removePlayer(anyString()); + } + @Test void shouldCancelInventoryClickEvent() { // given diff --git a/authme-core/src/test/java/fr/xephi/authme/process/quit/AsynchronousQuitTest.java b/authme-core/src/test/java/fr/xephi/authme/process/quit/AsynchronousQuitTest.java new file mode 100644 index 0000000000..ed30844f75 --- /dev/null +++ b/authme-core/src/test/java/fr/xephi/authme/process/quit/AsynchronousQuitTest.java @@ -0,0 +1,147 @@ +package fr.xephi.authme.process.quit; + +import fr.xephi.authme.AuthMe; +import fr.xephi.authme.TestHelper; +import fr.xephi.authme.data.ProxySessionManager; +import fr.xephi.authme.data.VerificationCodeManager; +import fr.xephi.authme.data.auth.PlayerCache; +import fr.xephi.authme.data.limbo.LimboPlayer; +import fr.xephi.authme.data.limbo.LimboPlayerState; +import fr.xephi.authme.data.limbo.LimboService; +import fr.xephi.authme.datasource.DataSource; +import fr.xephi.authme.process.SyncProcessManager; +import fr.xephi.authme.service.CommonService; +import fr.xephi.authme.service.DialogStateService; +import fr.xephi.authme.service.PremiumLoginVerifier; +import fr.xephi.authme.service.SessionService; +import fr.xephi.authme.service.TeleportationService; +import fr.xephi.authme.service.ValidationService; +import fr.xephi.authme.settings.SpawnLoader; +import fr.xephi.authme.settings.properties.PluginSettings; +import fr.xephi.authme.settings.properties.RestrictionSettings; +import org.bukkit.entity.Player; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.Collections; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +/** + * Test for {@link AsynchronousQuit}, focusing on the guarantee that a quitting player is always + * unauthenticated afterwards (see the offline-mode login-bypass this protects against). + */ +@ExtendWith(MockitoExtension.class) +class AsynchronousQuitTest { + + @InjectMocks + private AsynchronousQuit asynchronousQuit; + + @Mock + private AuthMe plugin; + @Mock + private DataSource database; + @Mock + private CommonService service; + @Mock + private PlayerCache playerCache; + @Mock + private SyncProcessManager syncProcessManager; + @Mock + private SpawnLoader spawnLoader; + @Mock + private ValidationService validationService; + @Mock + private VerificationCodeManager codeManager; + @Mock + private SessionService sessionService; + @Mock + private TeleportationService teleportationService; + @Mock + private DialogStateService dialogStateService; + @Mock + private ProxySessionManager proxySessionManager; + @Mock + private PremiumLoginVerifier premiumLoginVerifier; + @Mock + private LimboService limboService; + + @BeforeAll + static void initLogger() { + TestHelper.setupLogger(); + } + + @Test + void shouldRemovePlayerFromCacheEvenWhenQuitLocationLookupThrows() { + // given - a logged-in player quits and the quit-location lookup throws (e.g. Folia, quit while dead) + Player player = mockPlayer("Bobby"); + TestHelper.mockIpAddressToPlayer(player, "127.0.0.3"); + given(validationService.isUnrestricted("Bobby")).willReturn(false); + given(playerCache.isAuthenticated("bobby")).willReturn(true); + given(service.getProperty(RestrictionSettings.SAVE_QUIT_LOCATION)).willReturn(true); + given(spawnLoader.getPlayerLocationOrSpawn(player)) + .willThrow(new IllegalStateException("Accessing entity state off owning region's thread")); + given(service.getProperty(PluginSettings.SESSIONS_ENABLED)).willReturn(true); + + // when + asynchronousQuit.processQuit(player); + + // then - the throw is swallowed, the session is still updated and the player is unauthenticated + verify(database).updateSession(org.mockito.ArgumentMatchers.any()); + verify(playerCache).removePlayer("bobby"); + verify(database).setUnlogged("bobby"); + verify(database).invalidateCache("bobby"); + } + + @Test + void shouldDropProxyAndPremiumGrantsOnQuit() { + // given - an unauthenticated player quits + Player player = mockPlayer("Bobby"); + given(validationService.isUnrestricted("Bobby")).willReturn(false); + given(playerCache.isAuthenticated("bobby")).willReturn(false); + + // when + asynchronousQuit.processQuit(player); + + // then - name-keyed auto-login grants must not survive the disconnect + verify(proxySessionManager).removeLoginRequest("bobby"); + verify(premiumLoginVerifier).removeVerified("bobby"); + verify(playerCache).removePlayer("bobby"); + // an unauthenticated quit does no session/quit-location DB writes + verify(database, never()).updateSession(org.mockito.ArgumentMatchers.any()); + verify(database, never()).setUnlogged(org.mockito.ArgumentMatchers.anyString()); + } + + @Test + void shouldResetPendingTotpStateOnQuit() { + // given - the player passed the password check and was waiting for the 2FA code when leaving + Player player = mockPlayer("Bobby"); + given(validationService.isUnrestricted("Bobby")).willReturn(false); + LimboPlayer limbo = new LimboPlayer(null, false, Collections.emptyList(), false, 0.2f, 0.1f); + limbo.setState(LimboPlayerState.TOTP_REQUIRED); + given(limboService.getLimboPlayer("bobby")).willReturn(limbo); + given(playerCache.isAuthenticated("bobby")).willReturn(false); + + // when + asynchronousQuit.processQuit(player); + + // then - the pending 2FA state does not outlive the connection that passed the password check + assertThat(limbo.getState(), equalTo(LimboPlayerState.PASSWORD_REQUIRED)); + } + + private Player mockPlayer(String name) { + Player player = mock(Player.class); + lenient().when(player.getName()).thenReturn(name); + return player; + } +} diff --git a/authme-core/src/test/java/fr/xephi/authme/service/BukkitServiceTest.java b/authme-core/src/test/java/fr/xephi/authme/service/BukkitServiceTest.java index 1152bd870a..1f1f553576 100644 --- a/authme-core/src/test/java/fr/xephi/authme/service/BukkitServiceTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/service/BukkitServiceTest.java @@ -25,9 +25,13 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.only; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -76,6 +80,7 @@ void shouldDispatchCommand() { @Test void shouldDispatchConsoleCommand() { // given + given(schedulingAdapter.isGlobalThread()).willReturn(true); ConsoleCommandSender consoleSender = mock(ConsoleCommandSender.class); given(server.getConsoleSender()).willReturn(consoleSender); String command = "my command"; @@ -87,6 +92,20 @@ void shouldDispatchConsoleCommand() { verify(server).dispatchCommand(consoleSender, command); } + @Test + void shouldDispatchConsoleCommandOnGlobalRegionWhenOffThread() { + // given - not on the global region thread (e.g. a Folia player region thread) + given(schedulingAdapter.isGlobalThread()).willReturn(false); + String command = "my command"; + + // when + bukkitService.dispatchConsoleCommand(command); + + // then - the dispatch is handed off to the global region rather than run inline + verify(schedulingAdapter).runOnGlobalThread(eq(authMe), any(Runnable.class)); + verify(server, never()).dispatchCommand(any(), anyString()); + } + @Test void shouldScheduleSyncDelayedTask() { // given diff --git a/authme-folia/src/main/java/fr/xephi/authme/platform/FoliaPlatformAdapter.java b/authme-folia/src/main/java/fr/xephi/authme/platform/FoliaPlatformAdapter.java index 5c5685481b..35978e18b0 100644 --- a/authme-folia/src/main/java/fr/xephi/authme/platform/FoliaPlatformAdapter.java +++ b/authme-folia/src/main/java/fr/xephi/authme/platform/FoliaPlatformAdapter.java @@ -9,6 +9,7 @@ import fr.xephi.authme.listener.PlayerOpenSignListener; import fr.xephi.authme.service.CancellableTask; import io.papermc.paper.threadedregions.scheduler.ScheduledTask; +import org.bukkit.Location; import org.bukkit.entity.Entity; import org.bukkit.event.Listener; import org.bukkit.Bukkit; @@ -31,6 +32,11 @@ public boolean isOwnedByCurrentThread(Entity entity) { return Bukkit.isOwnedByCurrentRegion(entity); } + @Override + public boolean isOwnedByCurrentThread(Location location) { + return Bukkit.isOwnedByCurrentRegion(location); + } + @Override public boolean isGlobalThread() { return Bukkit.isGlobalTickThread(); From e23621ea09d3ed9c9983ebacbdd22d61543807fc Mon Sep 17 00:00:00 2001 From: "Gabriele C." Date: Thu, 1 Oct 2026 21:00:14 +0200 Subject: [PATCH 2/2] fix(security): discard a previous connection's authentication on join Addresses review feedback: with ForceSingleSession disabled, a connection taking over a name through a duplicate login could reach the join before the previous connection's asynchronous quit had removed it from the PlayerCache, and AsynchronousJoin would then treat it as already logged in. A player who has just joined has not logged in yet, so any authentication cached for their name belongs to another connection. PlayerListener now discards it on PlayerJoinEvent (LOWEST), before anything relies on it. The server only lets a duplicate login complete once the old player has been removed, so by then the old session is gone; kicks still need no handling since nothing is processed until an actual quit. Co-Authored-By: Claude Opus 5.5 --- .../fr/xephi/authme/listener/PlayerListener.java | 14 +++++++++++++- .../xephi/authme/listener/PlayerListenerTest.java | 14 ++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java b/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java index 09416322da..163e3bb22c 100644 --- a/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java +++ b/authme-core/src/main/java/fr/xephi/authme/listener/PlayerListener.java @@ -206,6 +206,16 @@ public void onAsyncPlayerPreLoginEventHighest(AsyncPlayerPreLoginEvent event) { } } + // A player who has just joined has not logged in yet, so any authentication cached for their name belongs to + // another connection: a previous one whose quit cleanup (asynchronous) has not run yet, e.g. when this player + // took over the name through a duplicate login, or one still online under the same name in another letter case. + // Discard it before anything relies on it, so it can never be inherited. Lowest priority so that it also happens + // before other plugins query the authentication state on join. + @EventHandler(priority = EventPriority.LOWEST) + public void onPlayerJoinLowest(PlayerJoinEvent event) { + playerCache.removePlayer(event.getPlayer().getName()); + } + @EventHandler(priority = EventPriority.NORMAL) public void onPlayerJoin(PlayerJoinEvent event) { final Player player = event.getPlayer(); @@ -321,7 +331,9 @@ public void onPlayerKick(PlayerKickEvent event) { } // The quit itself is processed in onPlayerQuit, which is also fired for kicked players. Processing it // here as well ran it twice, and ran it even when a later listener cancelled the kick, leaving the - // player online but logged out and without any limbo restrictions. + // player online but logged out and without any limbo restrictions. A connection that takes over the name + // (duplicate login) cannot inherit the authentication before that asynchronous cleanup has run either: + // see onPlayerJoinLowest. } /* diff --git a/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java b/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java index 411cc0a4a8..fa9d7143ab 100644 --- a/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java +++ b/authme-core/src/test/java/fr/xephi/authme/listener/PlayerListenerTest.java @@ -776,6 +776,20 @@ void shouldHandlePlayerRespawnNoChanges() { verify(bukkitService).runTaskLater(eq(player), any(Runnable.class), eq(1L)); } + @Test + void shouldDiscardAuthenticationOfPreviousConnectionOnJoin() { + // given - e.g. a duplicate login took over the name before the previous connection's asynchronous quit ran + Player player = mockPlayerWithName("Bobby"); + PlayerJoinEvent event = new PlayerJoinEvent(player, "join message"); + + // when + listener.onPlayerJoinLowest(event); + + // then - the joining player has not logged in yet and must not inherit the previous authentication + verify(playerCache).removePlayer("Bobby"); + verifyNoInteractions(management); + } + @Test void shouldHandlePlayerJoining() { // given