From 3af36ca86d7ba2b7dd04c878f5c53e0d3e505137 Mon Sep 17 00:00:00 2001 From: tastybento Date: Fri, 10 Jul 2026 07:37:44 -0700 Subject: [PATCH 1/2] fix: cancel denied magic-block break early to stop Jobs reward exploit (#534) Jobs Reborn awards block-break rewards from a BlockBreakEvent handler at EventPriority.HIGHEST with ignoreCancelled=true. AOneBlock also only checked the MAGIC_BLOCK protection flag (and cancelled the break) at HIGHEST. Within a single priority slot, Bukkit fires handlers in plugin-registration order, which is nondeterministic. When Jobs ran before AOneBlock, it paid out before the break was cancelled; since the magic block respawns on a cancelled break, a player lacking the MAGIC_BLOCK flag could mine it endlessly for infinite rewards. Add a dedicated onBlockBreakDeny handler at EventPriority.LOWEST that runs the MAGIC_BLOCK flag check (which cancels the event on denial). Cancelling at LOWEST is strictly before any later ignoreCancelled=true handler, so reward plugins are skipped for denied breaks. Full magic-block processing stays at HIGHEST, and allowed breaks are unaffected. Bump version to 1.25.2. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017nxjWg93etdoy6U6nhTXRX --- pom.xml | 2 +- .../aoneblock/listeners/BlockListener.java | 31 ++++++ .../listeners/BlockListenerTest2.java | 103 ++++++++++++++++++ 3 files changed, 135 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 13548cd..65440e6 100644 --- a/pom.xml +++ b/pom.xml @@ -67,7 +67,7 @@ -LOCAL - 1.25.1 + 1.25.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 efcea2c..4f3410f 100644 --- a/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java +++ b/src/main/java/world/bentobox/aoneblock/listeners/BlockListener.java @@ -219,6 +219,37 @@ public void onBlockFromTo(final BlockFromToEvent e) { e.setCancelled(addon.getIslands().getIslandAt(l).filter(i -> l.equals(i.getCenter())).isPresent()); } + /** + * Cancels a magic-block break as early as possible when the player lacks the + * {@link AOneBlock#MAGIC_BLOCK} permission. + *

+ * The full magic-block processing runs at {@link EventPriority#HIGHEST} so that + * other protection plugins get a chance to cancel first. However, reward-granting + * plugins such as Jobs Reborn also listen at {@code HIGHEST} with + * {@code ignoreCancelled = true}. Within a single priority the execution order is + * just plugin-registration order, so Jobs could pay out before our + * {@code HIGHEST} handler cancels the break. Because the magic block respawns when + * the break is cancelled, that let players mine it endlessly for infinite rewards. + *

+ * Cancelling the denied break here, at {@link EventPriority#LOWEST}, guarantees it + * happens before any {@code ignoreCancelled = true} handler at a later priority, so + * those plugins are skipped and no reward is granted. + * + * @param e The BlockBreakEvent. + * @see Issue #534 + */ + @EventHandler(priority = EventPriority.LOWEST, ignoreCancelled = true) + public void onBlockBreakDeny(final BlockBreakEvent e) { + if (!addon.inWorld(e.getBlock().getWorld())) { + return; + } + Location l = e.getBlock().getLocation(); + // checkIsland cancels the event and sends the protection message if the player + // is not allowed to break the magic block. + addon.getIslands().getIslandAt(l).filter(i -> l.equals(i.getCenter())) + .ifPresent(i -> checkIsland(e, e.getPlayer(), i.getCenter(), addon.MAGIC_BLOCK)); + } + /** * Handles the breaking of the magic block by a player. * @param e The BlockBreakEvent. diff --git a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java index 275908f..b983802 100644 --- a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java +++ b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java @@ -42,6 +42,8 @@ import org.bukkit.block.data.Brushable; import org.bukkit.entity.EntityType; import org.bukkit.entity.Item; +import org.bukkit.event.EventHandler; +import org.bukkit.event.EventPriority; import org.bukkit.event.block.Action; import org.bukkit.event.block.BlockBreakEvent; import org.bukkit.event.entity.EntityInteractEvent; @@ -1050,6 +1052,107 @@ void testOnBlockBreakByMinionNotInWorld() { verify(im, never()).getIslandAt(any()); } + // ========================================================================= + // onBlockBreakDeny — early cancel to stop reward exploits (issue #534) + // ========================================================================= + + /** + * The early-deny handler must run at {@link EventPriority#LOWEST} with + * {@code ignoreCancelled = true}. Reward-granting plugins such as Jobs Reborn + * listen for the {@link BlockBreakEvent} at {@code HIGHEST} with + * {@code ignoreCancelled = true}; cancelling here, before them, guarantees they + * are skipped when the player lacks the magic-block permission. + * See https://github.com/BentoBoxWorld/AOneBlock/issues/534 + */ + @Test + void testOnBlockBreakDenyRegisteredAtLowestPriority() throws NoSuchMethodException { + EventHandler eh = BlockListener.class.getMethod("onBlockBreakDeny", BlockBreakEvent.class) + .getAnnotation(EventHandler.class); + assertNotNull(eh); + assertEquals(EventPriority.LOWEST, eh.priority()); + assertTrue(eh.ignoreCancelled()); + } + + /** + * Test method for + * {@link world.bentobox.aoneblock.listeners.BlockListener#onBlockBreakDeny(BlockBreakEvent)} + * When the player lacks the MAGIC_BLOCK permission the break is cancelled at this + * early stage (via checkIsland), so later reward plugins are skipped. Regression + * test for https://github.com/BentoBoxWorld/AOneBlock/issues/534 + */ + @Test + void testOnBlockBreakDenyCancelsWhenNotAllowed() { + BlockListener spyBl = Mockito.spy(bl); + // Emulate FlagListener.checkIsland's deny behaviour: cancel the event and return false. + Mockito.doAnswer(inv -> { + ((BlockBreakEvent) inv.getArgument(0)).setCancelled(true); + return false; + }).when(spyBl).checkIsland(any(), any(), any(), any()); + + BlockBreakEvent e = new BlockBreakEvent(magicBlock, mockPlayer); + spyBl.onBlockBreakDeny(e); + + assertTrue(e.isCancelled()); + // The flag was checked against the island centre for the breaking player. + verify(spyBl).checkIsland(any(), eq(mockPlayer), eq(location), any()); + } + + /** + * Test method for + * {@link world.bentobox.aoneblock.listeners.BlockListener#onBlockBreakDeny(BlockBreakEvent)} + * When the player is allowed, the early handler leaves the event untouched so that + * normal magic-block processing and legitimate rewards proceed. + */ + @Test + void testOnBlockBreakDenyAllowsWhenPermitted() { + BlockListener spyBl = Mockito.spy(bl); + Mockito.doReturn(true).when(spyBl).checkIsland(any(), any(), any(), any()); + + BlockBreakEvent e = new BlockBreakEvent(magicBlock, mockPlayer); + spyBl.onBlockBreakDeny(e); + + assertFalse(e.isCancelled()); + } + + /** + * Test method for + * {@link world.bentobox.aoneblock.listeners.BlockListener#onBlockBreakDeny(BlockBreakEvent)} + * Not in an addon world → early return, the flag is never checked. + */ + @Test + void testOnBlockBreakDenyNotInWorld() { + when(addon.inWorld(world)).thenReturn(false); + BlockListener spyBl = Mockito.spy(bl); + + BlockBreakEvent e = new BlockBreakEvent(magicBlock, mockPlayer); + spyBl.onBlockBreakDeny(e); + + assertFalse(e.isCancelled()); + verify(spyBl, never()).checkIsland(any(), any(), any(), any()); + } + + /** + * Test method for + * {@link world.bentobox.aoneblock.listeners.BlockListener#onBlockBreakDeny(BlockBreakEvent)} + * Block is in world but is not the island centre (magic block) → the flag is never + * checked, so ordinary block breaking elsewhere on the island is unaffected. + */ + @Test + void testOnBlockBreakDenyNotCenterBlock() { + Block other = mock(Block.class); + when(other.getWorld()).thenReturn(world); + Location otherLoc = mock(Location.class); + when(other.getLocation()).thenReturn(otherLoc); + when(im.getIslandAt(otherLoc)).thenReturn(Optional.of(island)); + BlockListener spyBl = Mockito.spy(bl); + + BlockBreakEvent e = new BlockBreakEvent(other, mockPlayer); + spyBl.onBlockBreakDeny(e); + + assertFalse(e.isCancelled()); + verify(spyBl, never()).checkIsland(any(), any(), any(), any()); + } + // ========================================================================= // onBlockBreak(PlayerBucketFillEvent) guard tests // ========================================================================= From 376353f5321c1825c3d29a187293b9c620c8b82e Mon Sep 17 00:00:00 2001 From: tastybento Date: Fri, 10 Jul 2026 07:41:38 -0700 Subject: [PATCH 2/2] test: use static imports for Mockito spy/doReturn/doAnswer (SonarCloud S8924) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017nxjWg93etdoy6U6nhTXRX --- .../aoneblock/listeners/BlockListenerTest2.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java index b983802..0062193 100644 --- a/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java +++ b/src/test/java/world/bentobox/aoneblock/listeners/BlockListenerTest2.java @@ -13,8 +13,11 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -1082,9 +1085,9 @@ void testOnBlockBreakDenyRegisteredAtLowestPriority() throws NoSuchMethodExcepti */ @Test void testOnBlockBreakDenyCancelsWhenNotAllowed() { - BlockListener spyBl = Mockito.spy(bl); + BlockListener spyBl = spy(bl); // Emulate FlagListener.checkIsland's deny behaviour: cancel the event and return false. - Mockito.doAnswer(inv -> { + doAnswer(inv -> { ((BlockBreakEvent) inv.getArgument(0)).setCancelled(true); return false; }).when(spyBl).checkIsland(any(), any(), any(), any()); @@ -1105,8 +1108,8 @@ void testOnBlockBreakDenyCancelsWhenNotAllowed() { */ @Test void testOnBlockBreakDenyAllowsWhenPermitted() { - BlockListener spyBl = Mockito.spy(bl); - Mockito.doReturn(true).when(spyBl).checkIsland(any(), any(), any(), any()); + BlockListener spyBl = spy(bl); + doReturn(true).when(spyBl).checkIsland(any(), any(), any(), any()); BlockBreakEvent e = new BlockBreakEvent(magicBlock, mockPlayer); spyBl.onBlockBreakDeny(e); @@ -1122,7 +1125,7 @@ void testOnBlockBreakDenyAllowsWhenPermitted() { @Test void testOnBlockBreakDenyNotInWorld() { when(addon.inWorld(world)).thenReturn(false); - BlockListener spyBl = Mockito.spy(bl); + BlockListener spyBl = spy(bl); BlockBreakEvent e = new BlockBreakEvent(magicBlock, mockPlayer); spyBl.onBlockBreakDeny(e); @@ -1144,7 +1147,7 @@ void testOnBlockBreakDenyNotCenterBlock() { Location otherLoc = mock(Location.class); when(other.getLocation()).thenReturn(otherLoc); when(im.getIslandAt(otherLoc)).thenReturn(Optional.of(island)); - BlockListener spyBl = Mockito.spy(bl); + BlockListener spyBl = spy(bl); BlockBreakEvent e = new BlockBreakEvent(other, mockPlayer); spyBl.onBlockBreakDeny(e);