From c7e61c6f8dd98b8711dfaff3bfac717ed7b89e59 Mon Sep 17 00:00:00 2001 From: gamer147 Date: Tue, 2 Jun 2026 17:14:13 -0400 Subject: [PATCH] fix(battle-node): hand events are unencrypted JSON arrays, not encrypted dicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior 'hand'-ack fix worked in test but failed in prod because both the handler and the test used the wrong wire shape. Re-tracing the client emit path: RealTimeNetworkAgent.cs:783-786 (msg path): return MessagePackSerializer.Serialize( CryptAES.encryptForNode(JsonMapper.ToJson(info))); // ← encrypted RealTimeNetworkAgent.cs:815-817 (hand path): return MessagePackSerializer.Serialize( JsonMapper.ToJson(info)); // ← NOT encrypted And EmitFrontStockData:717-723 picks "hand" as the SIO event name only when frontData["StockHandData"] exists; in that branch it passes the StockHandData list (NOT the dict) to CreatePackEmitHandData. So the wire body is: msgpack_string(JsonMapper.ToJson(List)) i.e. a JSON array, unencrypted. EmitMsgUriPack:1456-1458 puts pubSeq at index 3 of that array (after uri_int / viewerId / udid). The dict's top-level pubSeq stays client-local for stockEmitMessageMgr.GetSelectData. Handler now: - Skips NodeCrypto.DecryptForNode (was throwing FormatException on the unencrypted bytes — caught and swallowed silently by the existing outer try/catch, so the bug presented as 'no warning, no ack') - Parses RootElement.ValueKind: - Array → arr[3] is the pubSeq - Object → top-level "pubSeq" (defensive; not used by prod today) - Falls back to ack arg=0 if neither extraction works (the client's GetSelectData lookup misses but its OnAck path still fires — same as a normal cache-miss — so the queue still drains) Diagnostic [hand-rx] log added (gated by DiagnosticLogging) so we can see the actual body content per-frame during verification. Test was also wrong (encrypted dict shape); rewritten to use the real wire shape (unencrypted JSON array). +1 net new test covering the dict-shape defensive path. 176 battle-node tests passing. Co-Authored-By: Claude Opus 4.7 --- .../Sessions/Participants/RealParticipant.cs | 65 +++++++++--- .../RealParticipantHandEventTests.cs | 100 ++++++++++-------- 2 files changed, 105 insertions(+), 60 deletions(-) diff --git a/SVSim.BattleNode/Sessions/Participants/RealParticipant.cs b/SVSim.BattleNode/Sessions/Participants/RealParticipant.cs index a1ff664..dfd53d0 100644 --- a/SVSim.BattleNode/Sessions/Participants/RealParticipant.cs +++ b/SVSim.BattleNode/Sessions/Participants/RealParticipant.cs @@ -274,14 +274,22 @@ public sealed class RealParticipant : IBattleParticipant, IHasHandshakePhase /// /// Ack hand events from the client so the client's stockEmitMessageMgr - /// drains and subsequent emits transmit. Wire shape differs from msg: the body - /// is {"StockHandData":[uri_int, viewerId, udid, ...params, pubSeq], "try":0, "pubSeq":N} - /// (no top-level uri), so we can't reuse — its - /// path requires a top-level uri. + /// drains and subsequent emits transmit. + /// + /// Wire shape: hand events are not encrypted on the wire — the client's + /// RealTimeNetworkAgent.CreatePackEmitHandData:815-817 calls only + /// MessagePackSerializer.Serialize(JsonMapper.ToJson(list)), skipping the + /// CryptAES.encryptForNode wrap that CreatePackEmitData applies to msg + /// events. The msgpack-wrapped string is a JSON array of the form + /// [uri_int, viewerId, udid, pubSeq, ...emit_params] — see + /// EmitMsgUriPack:1456-1458 which inserts pubSeq at index 3 of the list + /// for isHandData emits. The dict's top-level pubSeq stays client-local + /// (used by its stockEmitMessageMgr.GetSelectData lookup); it's NOT on the wire. + /// /// /// In scripted/Bot mode the server has no opponent to forward touches to; ack-only is - /// correct. PvP-side forwarding (so the other player's client can render opponent - /// touch/cursor UI) is unverified — see docs/audits/battle-node-sio-events-2026-06-02.md. + /// correct. PvP-side forwarding semantics are unverified — see + /// docs/audits/battle-node-sio-events-2026-06-02.md. /// /// /// Fire-and-forget hand frames (TOUCH_URI / SELECT_OBJECT_URI / TURN_END_READY_URI) arrive @@ -299,20 +307,49 @@ public sealed class RealParticipant : IBattleParticipant, IHasHandshakePhase } try { - var encryptedString = MessagePack.MessagePackSerializer.Deserialize(frame.BinaryAttachments[0]); - var json = NodeCrypto.DecryptForNode(encryptedString); + // No NodeCrypto.DecryptForNode here — hand events are unencrypted on the wire. + var json = MessagePack.MessagePackSerializer.Deserialize(frame.BinaryAttachments[0]); + if (_diagnosticLogging) + { + _log.LogInformation( + "[hand-rx] viewer={Vid} ackId={AckId} bodyLen={Len} body={Body}", + ViewerId, frame.AckId, json.Length, + json.Length > 200 ? json.Substring(0, 200) + "..." : json); + } + using var doc = System.Text.Json.JsonDocument.Parse(json); - if (!doc.RootElement.TryGetProperty("pubSeq", out var psEl) - || psEl.ValueKind != System.Text.Json.JsonValueKind.Number) + long? pubSeq = null; + var rootKind = doc.RootElement.ValueKind; + if (rootKind == System.Text.Json.JsonValueKind.Array) + { + // Prod shape: [uri_int, viewerId, udid, pubSeq, ...emit_params]. + var arr = doc.RootElement; + if (arr.GetArrayLength() > 3 + && arr[3].ValueKind == System.Text.Json.JsonValueKind.Number) + { + pubSeq = arr[3].GetInt64(); + } + } + else if (rootKind == System.Text.Json.JsonValueKind.Object + && doc.RootElement.TryGetProperty("pubSeq", out var psEl) + && psEl.ValueKind == System.Text.Json.JsonValueKind.Number) + { + // Defensive: dict root with top-level pubSeq isn't what the client sends today, + // but the StockHandData dict shape exists on the client side and a future + // wire-format change could expose it. Cheap to handle. + pubSeq = psEl.GetInt64(); + } + + if (pubSeq is null) { _log.LogWarning( - "RealParticipant viewer={Vid}: 'hand' event ackId={AckId} body missing numeric pubSeq; " + - "acking with 0 as a fallback (client's stockEmitMessageMgr lookup will see null selectData).", - ViewerId, frame.AckId); + "RealParticipant viewer={Vid}: 'hand' event ackId={AckId} body has no extractable pubSeq " + + "(rootKind={Kind}, bodyLen={Len}); acking with 0 as fallback.", + ViewerId, frame.AckId, rootKind, json.Length); await SendSioAckAsync(frame.AckId.Value, 0); return; } - await SendSioAckAsync(frame.AckId.Value, psEl.GetInt64()); + await SendSioAckAsync(frame.AckId.Value, pubSeq.Value); } catch (Exception ex) { diff --git a/SVSim.UnitTests/BattleNode/Sessions/Participants/RealParticipantHandEventTests.cs b/SVSim.UnitTests/BattleNode/Sessions/Participants/RealParticipantHandEventTests.cs index 706c37d..ee77949 100644 --- a/SVSim.UnitTests/BattleNode/Sessions/Participants/RealParticipantHandEventTests.cs +++ b/SVSim.UnitTests/BattleNode/Sessions/Participants/RealParticipantHandEventTests.cs @@ -1,6 +1,5 @@ using System.Net.WebSockets; using System.Text; -using System.Text.Json; using MessagePack; using Microsoft.Extensions.Logging.Abstractions; using NUnit.Framework; @@ -14,35 +13,31 @@ using SVSim.UnitTests.BattleNode.Infrastructure; namespace SVSim.UnitTests.BattleNode.Sessions.Participants; /// -/// Regression tests for the "hand" SIO event handler. The active bug at -/// docs/audits/battle-node-sio-events-2026-06-02.md §"Active bug" was: -/// client-stocked SELECT_SKILL_URI / SLIDE_OBJECT_URI hand emits arrive -/// with an ack-id; without a server ack, the client's stockEmitMessageMgr deadlocks -/// behind them and every subsequent emit (PlayActions, TurnEndActions, TurnEnd) is queued -/// but never transmitted. These tests drive a hand frame through the WS read loop and -/// inspect the outbound text-frame queue for the SIO ack. +/// Regression tests for the "hand" SIO event handler. The wire shape verified at +/// RealTimeNetworkAgent.CreatePackEmitHandData:815-817: +/// +/// return MessagePackSerializer.Serialize(JsonMapper.ToJson(info)); // info = List<object>, NOT encrypted +/// +/// is the source of truth this test must match. (An earlier version of this test +/// wrapped the body in an encrypted dict shape — that was wrong and shipped a handler +/// that softlocked in prod despite passing the test. See +/// docs/audits/battle-node-sio-events-2026-06-02.md.) /// [TestFixture] public class RealParticipantHandEventTests { - // Any 32-char ASCII string is a valid AES key for NodeCrypto — DecryptForNode reads the - // key from the first 32 chars of the encrypted blob. - private const string TestKey = "abcdefghijklmnopqrstuvwxyz012345"; - [Test] - public async Task Stocked_hand_event_acks_with_body_pubSeq() + public async Task Stocked_hand_event_acks_with_array_index3_pubSeq() { + // Prod wire shape per EmitMsgUriPack:1454-1458 — Insert(3, num) puts pubSeq at index 3: + // [uri_int, viewerId, udid, pubSeq, ...select-skill params] + const long expectedPubSeq = 42L; + var bodyJson = $"[2,906243102,\"d08367be-1152-4009-aaaf-2d47d1d9112c\",{expectedPubSeq},1,false,false]"; + var ws = new TestWebSocket(); var p = new RealParticipant(ws, viewerId: 906_243_102L, FixtureCtx(), NullLogger.Instance); - - // Client-shape hand body for a SELECT_SKILL emit: StockHandData[0] = uri_int - // (HAND_URI_TYPE.SELECT_SKILL_URI = 2), top-level "try" + "pubSeq" added by - // EmitMsgUriPack. The handler reads top-level "pubSeq" — body contents otherwise - // don't matter for ack semantics. - const long expectedPubSeq = 42L; - var body = $"{{\"StockHandData\":[2,906243102,\"u\",1,0,{expectedPubSeq}],\"try\":0,\"pubSeq\":{expectedPubSeq}}}"; - EnqueueHandFrame(ws, ackId: 26, body: body); + EnqueueHandFrame(ws, ackId: 26, bodyJson: bodyJson); ws.CompleteIncoming(); await p.RunAsync(CancellationToken.None); @@ -51,7 +46,28 @@ public class RealParticipantHandEventTests Assert.That(ackFrame, Is.Not.Null, $"Expected an SIO Ack frame for ackId=26 in outbound sends; got: [{string.Join(", ", AllTextSends(ws))}]"); Assert.That(ackFrame, Does.Contain($"[{expectedPubSeq}]"), - "Ack arg must echo the body's pubSeq so the client's stockEmitMessageMgr.GetSelectData lookup succeeds."); + "Ack arg must echo the body's pubSeq (array index 3) so client's stockEmitMessageMgr.GetSelectData succeeds."); + } + + [Test] + public async Task Stocked_hand_event_with_dict_root_acks_with_top_level_pubSeq() + { + // Defensive: not what the client sends today, but the StockHandData dict shape + // exists client-side and could surface on the wire with a future format change. + const long expectedPubSeq = 17L; + var bodyJson = $"{{\"StockHandData\":[2,1,\"u\",{expectedPubSeq}],\"try\":0,\"pubSeq\":{expectedPubSeq}}}"; + + var ws = new TestWebSocket(); + var p = new RealParticipant(ws, viewerId: 1, FixtureCtx(), + NullLogger.Instance); + EnqueueHandFrame(ws, ackId: 33, bodyJson: bodyJson); + ws.CompleteIncoming(); + + await p.RunAsync(CancellationToken.None); + + var ackFrame = FindAckFrame(ws, ackId: 33); + Assert.That(ackFrame, Is.Not.Null); + Assert.That(ackFrame, Does.Contain($"[{expectedPubSeq}]")); } [Test] @@ -59,43 +75,41 @@ public class RealParticipantHandEventTests { // Fire-and-forget hand emits (TOUCH_URI, SELECT_OBJECT_URI, TURN_END_READY_URI) arrive // without an ack-id and don't block the client's emit queue. We should swallow them - // without trying to decode or ack. + // without decoding or acking. var ws = new TestWebSocket(); var p = new RealParticipant(ws, viewerId: 1, FixtureCtx(), NullLogger.Instance); - var body = "{\"StockHandData\":[1,1,\"u\",0],\"try\":0}"; - EnqueueHandFrame(ws, ackId: null, body: body); + EnqueueHandFrame(ws, ackId: null, bodyJson: "[1,1,\"u\",0,0]"); ws.CompleteIncoming(); await p.RunAsync(CancellationToken.None); - // Only the EIO Open handshake should be in Sends; no Ack frame. var ackFrames = AllTextSends(ws).Where(s => s.StartsWith("43")).ToList(); Assert.That(ackFrames, Is.Empty, $"No-ack-id hand frame must not produce an Ack; got: [{string.Join(", ", ackFrames)}]"); } [Test] - public async Task Hand_event_with_missing_pubSeq_falls_back_to_ack_arg_0() + public async Task Hand_event_with_unparseable_pubSeq_position_falls_back_to_ack_arg_0() { - // If a stocked hand frame ever arrives without a pubSeq, we still ack so the - // client doesn't softlock — but with arg=0 (the client's GetSelectData lookup - // will miss and OnAck fires with null selectData, which is the same path as a - // normal cache-miss; not great, but not a deadlock). + // If a stocked hand frame ever arrives with an array shorter than 4 elements (or a + // non-numeric index 3), we still ack so the client doesn't softlock — but with + // arg=0. The client's GetSelectData lookup misses and OnAck fires with null + // selectData, which is a normal cache-miss path (not a deadlock). + var bodyJson = "[2,1,\"u\"]"; // length 3, no index-3 pubSeq + var ws = new TestWebSocket(); var p = new RealParticipant(ws, viewerId: 1, FixtureCtx(), NullLogger.Instance); - - var body = "{\"StockHandData\":[2,1,\"u\"],\"try\":0}"; // no pubSeq - EnqueueHandFrame(ws, ackId: 99, body: body); + EnqueueHandFrame(ws, ackId: 99, bodyJson: bodyJson); ws.CompleteIncoming(); await p.RunAsync(CancellationToken.None); var ackFrame = FindAckFrame(ws, ackId: 99); Assert.That(ackFrame, Is.Not.Null, - "Missing-pubSeq fallback should still ack (arg=0), not silently swallow."); + "Malformed hand body should still ack (arg=0), not silently swallow."); Assert.That(ackFrame, Does.Contain("[0]"), "Fallback ack arg should be 0."); } @@ -106,19 +120,17 @@ public class RealParticipantHandEventTests /// /// Enqueue an SIO BinaryEvent("hand", {placeholder}) text frame followed by its single - /// binary attachment (NodeCrypto-encrypted msgpack-string of ). - /// Wire format mirrors what EmitFrontStockData:717-720 emits. + /// binary attachment (msgpack-string of the raw JSON, not encrypted — + /// CreatePackEmitHandData:815-817 does not call CryptAES.encryptForNode). /// - private static void EnqueueHandFrame(TestWebSocket ws, int? ackId, string body) + private static void EnqueueHandFrame(TestWebSocket ws, int? ackId, string bodyJson) { - // Text frame: 4 (EIO Message) + 5 (SIO BinaryEvent) + 1- (1 attachment) + ackId + json var ackPart = ackId.HasValue ? ackId.Value.ToString() : ""; var text = $"451-{ackPart}[\"hand\",{{\"_placeholder\":true,\"num\":0}}]"; ws.EnqueueIncoming(Encoding.UTF8.GetBytes(text), WebSocketMessageType.Text); - // Binary attachment: EIO Message prefix (0x04) + msgpack-string(NodeCrypto.Encrypt(json)). - var encrypted = NodeCrypto.EncryptForNode(body, TestKey); - var msgpackBytes = MessagePackSerializer.Serialize(encrypted); + // Binary attachment: EIO Message prefix (0x04) + msgpack-string(bodyJson). + var msgpackBytes = MessagePackSerializer.Serialize(bodyJson); var prefixed = new byte[msgpackBytes.Length + 1]; prefixed[0] = (byte)EngineIoPacketType.Message; Buffer.BlockCopy(msgpackBytes, 0, prefixed, 1, msgpackBytes.Length); @@ -130,10 +142,6 @@ public class RealParticipantHandEventTests .Where(f => f.Type == WebSocketMessageType.Text) .Select(f => Encoding.UTF8.GetString(f.Payload)); - /// - /// SIO Ack wire form: EIO Message (4) + SIO Ack (3) + ackId + [arg]. Filter the text - /// sends for that shape. - /// private static string? FindAckFrame(TestWebSocket ws, int ackId) => AllTextSends(ws).FirstOrDefault(s => s.StartsWith($"43{ackId}["));