From ab93dc80be116828ffbcff160e6bde352c9afece Mon Sep 17 00:00:00 2001 From: notevil Date: Thu, 23 Apr 2026 15:28:28 +0200 Subject: [PATCH] P3-11 review fixes : direction guard + warn on priority out-of-range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH RISK-001 : ModNetwork.reg() helper n enforce pas NetworkDirection, guard défensif ajouté dans handleOnClient (reject non-S→C + WARN). Fix systémique du helper tracé en backlog séparé. LOW SMELL-001 + SMELL-002 : priority() fallback silencieux + idx<0 dead après masking. WARN log ajouté sur out-of-range. Condition nettoyée. --- .../remake/rig/network/PacketPlayRigAnim.java | 29 ++++++++++++-- .../rig/network/PacketPlayRigAnimTest.java | 39 +++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/tiedup/remake/rig/network/PacketPlayRigAnim.java b/src/main/java/com/tiedup/remake/rig/network/PacketPlayRigAnim.java index ae7f65c..0530175 100644 --- a/src/main/java/com/tiedup/remake/rig/network/PacketPlayRigAnim.java +++ b/src/main/java/com/tiedup/remake/rig/network/PacketPlayRigAnim.java @@ -45,9 +45,15 @@ public record PacketPlayRigAnim( } public Layer.Priority priority() { - // Guard out-of-range si un peer malveillant envoie un byte invalide + // Guard out-of-range si un peer malveillant envoie un byte invalide. + // Note : priorityOrdinal & 0xFF donne unsigned [0,255] donc idx < 0 est + // impossible ici — seul le bound haut est à tester (SMELL-002). int idx = priorityOrdinal & 0xFF; - if (idx < 0 || idx >= PRIORITY_COUNT) { + if (idx >= PRIORITY_COUNT) { + TiedUpRigConstants.LOGGER.warn( + "[PacketPlayRigAnim] priority ordinal out of range ({}), falling back to LOWEST. Possible protocol mismatch.", + idx + ); return Layer.Priority.LOWEST; // fallback safe } return Layer.Priority.values()[idx]; @@ -73,13 +79,28 @@ public record PacketPlayRigAnim( * patch, playAnimation) viendra en P3-12. */ public static void handleOnClient(PacketPlayRigAnim pkt, Supplier ctx) { - ctx.get().enqueueWork(() -> { + NetworkEvent.Context context = ctx.get(); + + // Direction guard : ce packet est S→C uniquement. ModNetwork.reg() helper + // n'enforce pas NetworkDirection, donc un client malveillant pourrait + // spoofer ce packet vers le serveur. Guard défensif (voir P3-11 review + // RISK-001 — fix systémique du helper reg() tracé en backlog séparé). + if (context.getDirection() != net.minecraftforge.network.NetworkDirection.PLAY_TO_CLIENT) { + TiedUpRigConstants.LOGGER.warn( + "[PacketPlayRigAnim] rejected non-S→C packet direction: {} (possible spoofing attempt)", + context.getDirection() + ); + context.setPacketHandled(true); + return; + } + + context.enqueueWork(() -> { TiedUpRigConstants.LOGGER.debug( "[PacketPlayRigAnim] received (stub P3-11): entityId={}, animId={}, transition={}s, priority={}", pkt.entityId, pkt.animId, pkt.transitionTime, pkt.priority() ); // TODO P3-12 : resolve entity + patch + animator.playAnimation }); - ctx.get().setPacketHandled(true); + context.setPacketHandled(true); } } diff --git a/src/test/java/com/tiedup/remake/rig/network/PacketPlayRigAnimTest.java b/src/test/java/com/tiedup/remake/rig/network/PacketPlayRigAnimTest.java index dbd6a03..c9fe95c 100644 --- a/src/test/java/com/tiedup/remake/rig/network/PacketPlayRigAnimTest.java +++ b/src/test/java/com/tiedup/remake/rig/network/PacketPlayRigAnimTest.java @@ -6,10 +6,19 @@ package com.tiedup.remake.rig.network; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.function.Supplier; import io.netty.buffer.Unpooled; import net.minecraft.network.FriendlyByteBuf; import net.minecraft.resources.ResourceLocation; +import net.minecraftforge.network.NetworkDirection; +import net.minecraftforge.network.NetworkEvent; import org.junit.jupiter.api.Test; @@ -137,6 +146,36 @@ class PacketPlayRigAnimTest { assertEquals("deep/path/with_underscores", decoded.animId().getPath()); } + /** + * Direction guard (RISK-001) : handleOnClient doit rejeter les packets + * reçus avec direction PLAY_TO_SERVER (spoofing client malveillant). + * On mock NetworkEvent.Context + son Supplier, on stub getDirection() et + * on vérifie que enqueueWork() n'est PAS appelé + setPacketHandled(true) + * est appelé. + * + *

Note : on n'exécute pas la branche PLAY_TO_CLIENT ici car enqueueWork + * invoque LogicalSidedProvider.WORKQUEUE qui NPE sans bootstrap MC — + * testable uniquement en runClient/gameday. + */ + @Test + void handleOnClient_rejectsServerBoundDirection() { + ResourceLocation anim = ResourceLocation.fromNamespaceAndPath("tiedup", "test"); + PacketPlayRigAnim pkt = PacketPlayRigAnim.of(42, anim, 0.15F, Layer.Priority.MIDDLE); + + NetworkEvent.Context context = mock(NetworkEvent.Context.class); + when(context.getDirection()).thenReturn(NetworkDirection.PLAY_TO_SERVER); + + @SuppressWarnings("unchecked") + Supplier ctxSupplier = mock(Supplier.class); + when(ctxSupplier.get()).thenReturn(context); + + PacketPlayRigAnim.handleOnClient(pkt, ctxSupplier); + + // Guard doit short-circuit : setPacketHandled(true) appelé, enqueueWork jamais. + verify(context, times(1)).setPacketHandled(true); + verify(context, never()).enqueueWork(org.mockito.ArgumentMatchers.any()); + } + /** Sanity check : deux packets distincts ne sont pas equal. */ @Test void records_notEqual_whenDifferent() {