P3-11 review fixes : direction guard + warn on priority out-of-range

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.
This commit is contained in:
notevil
2026-04-23 15:28:28 +02:00
parent 2a4ec170ef
commit ab93dc80be
2 changed files with 64 additions and 4 deletions

View File

@@ -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<NetworkEvent.Context> 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);
}
}

View File

@@ -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é.
*
* <p>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<NetworkEvent.Context> 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() {