From 39ff2e3e96084bea871a75369ff65df8f4472455 Mon Sep 17 00:00:00 2001 From: tastybento Date: Mon, 14 Sep 2026 13:08:38 +0100 Subject: [PATCH] fix: handle missing phase in BlockListener.processPhase without NPE Sonar (S2259) flagged a guaranteed NullPointerException in processPhase: OneBlocksManager.getPhase() is @Nullable, and the phase returned by handleGoto() was dereferenced without a check. A goto pointing below the first phase, or phase files failing to load, would crash the block-break handler and could leave the island without its magic block. processPhase now resolves the goto first and checks the resulting phase once. When no phase covers the block number it logs an error naming the island and block number, cancels the event so the block stays in place, and returns null; process() returns early on a null result. handlePhaseChange had the same latent requireNonNull and now uses a plain null check that the following line already expected. Adds two BlockListenerTest cases covering no-phase and goto-to-no-phase. Bumps version to 1.27.2. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0161Uw7dZoSjSbSH6T8hMsUQ --- pom.xml | 2 +- .../aoneblock/listeners/BlockListener.java | 21 +++++-- .../listeners/BlockListenerTest.java | 56 +++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/pom.xml b/pom.xml index 630a18b..f276d56 100644 --- a/pom.xml +++ b/pom.xml @@ -67,7 +67,7 @@ -LOCAL - 1.27.1 + 1.27.2 BentoBoxWorld_AOneBlock bentobox-world diff --git a/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java b/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java index 6c006e6..8fc6384 100644 --- a/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java +++ b/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java @@ -423,7 +423,7 @@ private void process(@NonNull Cancellable e, @NonNull Island island, @Nullable P // Process phase changes and requirements ProcessPhaseResult phaseResult = processPhase(e, island, is, player, world, block); - if (e.isCancelled()) { + if (phaseResult == null || e.isCancelled()) { return; } @@ -455,16 +455,29 @@ private record ProcessPhaseResult(OneBlockPhase phase, boolean isCurrPhaseNew, i * @param player - player involved * @param world - world where processing occurs * @param block - block being processed - * @return ProcessPhaseResult containing phase details + * @return ProcessPhaseResult containing phase details, or null if no phase covers the + * island's block number. In that case the event is cancelled so the magic block + * is not lost, and an error is logged for the admin. */ + @Nullable private ProcessPhaseResult processPhase(Cancellable e, Island i, OneBlockIslands is, Player player, World world, Block block) { OneBlockPhase phase = oneBlocksManager.getPhase(is.getBlockNumber()); String prevPhaseName = is.getPhaseName(); - if (Objects.requireNonNull(phase).getGotoBlock() != null) { + if (phase != null && phase.getGotoBlock() != null) { phase = handleGoto(is, phase.getGotoBlock()); } + if (phase == null) { + // No phase covers this block number. This happens when no phase files loaded, + // when the first phase does not start at block 0, or when a goto points below + // the first phase. Keep the block in place rather than breaking the island. + addon.logError("No phase found for block number " + is.getBlockNumber() + " on island " + + i.getUniqueId() + ". Check the phase files in the phases folder."); + e.setCancelled(true); + return null; + } + String currPhaseName = phase.getPhaseName() == null ? "" : phase.getPhaseName(); handlePhaseChange(is, currPhaseName); @@ -516,7 +529,7 @@ private void handleNewPhase(Player player, Island i, OneBlockIslands is, OneBloc */ private void handlePhaseChange(OneBlockIslands is, String currPhaseName) { OneBlockPhase nextPhase = oneBlocksManager.getPhase(is.getBlockNumber() + 1); - if (Objects.requireNonNull(nextPhase).getGotoBlock() != null) { + if (nextPhase != null && nextPhase.getGotoBlock() != null) { nextPhase = oneBlocksManager.getPhase(nextPhase.getGotoBlock()); } String nextPhaseName = nextPhase == null || nextPhase.getPhaseName() == null ? "" : nextPhase.getPhaseName(); diff --git a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java index fcd087c..786a48d 100644 --- a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java +++ b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest.java @@ -3,6 +3,8 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -17,7 +19,10 @@ import org.bukkit.World; import org.bukkit.block.Block; import org.bukkit.block.BlockFace; +import org.bukkit.entity.ArmorStand; +import org.bukkit.entity.EntityType; import org.bukkit.event.block.BlockFromToEvent; +import org.bukkit.event.entity.EntityInteractEvent; import org.eclipse.jdt.annotation.NonNull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -192,6 +197,57 @@ void testOnBlockFromToCenterBlock() { assertTrue(e.isCancelled()); } + /** + * When no phase covers the island's block number (e.g. phase files failed to load or + * a goto points below the first phase), the break must be cancelled and an error logged + * instead of throwing a NullPointerException. + */ + @Test + void testProcessNoPhaseForBlockNumberCancelsAndLogs() { + when(addon.inWorld(any(World.class))).thenReturn(true); + island.setCenter(location); + when(im.getIslandAt(location)).thenReturn(Optional.of(island)); + when(obm.getPhase(anyInt())).thenReturn(null); + + Block block = mock(Block.class); + when(block.getLocation()).thenReturn(location); + when(block.getWorld()).thenReturn(world); + ArmorStand minion = mock(ArmorStand.class); + when(minion.getType()).thenReturn(EntityType.ARMOR_STAND); + + EntityInteractEvent e = new EntityInteractEvent(minion, block); + bl.onBlockBreakByMinion(e); + + assertTrue(e.isCancelled()); + verify(addon).logError(anyString()); + } + + /** + * A phase whose goto target lies below the first phase must also be handled gracefully. + */ + @Test + void testProcessGotoBelowFirstPhaseCancelsAndLogs() { + when(addon.inWorld(any(World.class))).thenReturn(true); + island.setCenter(location); + when(im.getIslandAt(location)).thenReturn(Optional.of(island)); + OneBlockPhase gotoPhase = new OneBlockPhase("0"); + gotoPhase.setGotoBlock(-5); + when(obm.getPhase(0)).thenReturn(gotoPhase); + when(obm.getPhase(-5)).thenReturn(null); + + Block block = mock(Block.class); + when(block.getLocation()).thenReturn(location); + when(block.getWorld()).thenReturn(world); + ArmorStand minion = mock(ArmorStand.class); + when(minion.getType()).thenReturn(EntityType.ARMOR_STAND); + + EntityInteractEvent e = new EntityInteractEvent(minion, block); + bl.onBlockBreakByMinion(e); + + assertTrue(e.isCancelled()); + verify(addon).logError(anyString()); + } + /** * Test method for {@link world.bentobox.aoneblock.listeners.BlockListener#saveCache()}. */