SVSim/SVSimServer
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

fix(battle-node): hand events are unencrypted JSON arrays, not encrypted dicts

Browse Source 
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<object>))

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 <noreply@anthropic.com>
This commit is contained in:
gamer147
2026-06-02 17:14:13 -04:00
parent 8f270a87f0
commit c7e61c6f8d
2 changed files with 105 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
SVSim.BattleNode/Sessions/Participants/RealParticipant.cs
View File

@@ -274,14 +274,22 @@ public sealed class RealParticipant : IBattleParticipant, IHasHandshakePhase
/// <summary>
/// Ack <c>hand</c> events from the client so the client's <c>stockEmitMessageMgr</c>
/// drains and subsequent emits transmit. Wire shape differs from <c>msg</c>: the body
/// is <c>{"StockHandData":[uri_int, viewerId, udid, ...params, pubSeq], "try":0, "pubSeq":N}</c>
/// (no top-level <c>uri</c>), so we can't reuse <see cref="HandleMsgEventAsync"/> — its
/// <see cref="MsgEnvelope.FromJson"/> path requires a top-level <c>uri</c>.
/// drains and subsequent emits transmit.
/// <para>
/// Wire shape: hand events are <b>not encrypted</b> on the wire — the client's
/// <c>RealTimeNetworkAgent.CreatePackEmitHandData:815-817</c> calls only
/// <c>MessagePackSerializer.Serialize(JsonMapper.ToJson(list))</c>, skipping the
/// <c>CryptAES.encryptForNode</c> wrap that <c>CreatePackEmitData</c> applies to <c>msg</c>
/// events. The msgpack-wrapped string is a JSON array of the form
/// <c>[uri_int, viewerId, udid, pubSeq, ...emit_params]</c> — see
/// <c>EmitMsgUriPack:1456-1458</c> which inserts <c>pubSeq</c> at index 3 of the list
/// for <c>isHandData</c> emits. The dict's top-level <c>pubSeq</c> stays client-local
/// (used by its stockEmitMessageMgr.GetSelectData lookup); it's NOT on the wire.
/// </para>
/// <para>
/// 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 <c>docs/audits/battle-node-sio-events-2026-06-02.md</c>.
/// correct. PvP-side forwarding semantics are unverified — see
/// <c>docs/audits/battle-node-sio-events-2026-06-02.md</c>.
/// </para>
/// <para>
/// 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<string>(frame.BinaryAttachments[0]);
var json = NodeCrypto.DecryptForNode(encryptedString);
// No NodeCrypto.DecryptForNode here — hand events are unencrypted on the wire.
var json = MessagePack.MessagePackSerializer.Deserialize<string>(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)
{

100
SVSim.UnitTests/BattleNode/Sessions/Participants/RealParticipantHandEventTests.cs
View File

@@ -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;
/// <summary>
/// Regression tests for the <c>"hand"</c> SIO event handler. The active bug at
/// <c>docs/audits/battle-node-sio-events-2026-06-02.md</c> §"Active bug" was:
/// client-stocked <c>SELECT_SKILL_URI</c> / <c>SLIDE_OBJECT_URI</c> hand emits arrive
/// with an ack-id; without a server ack, the client's <c>stockEmitMessageMgr</c> 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 <c>"hand"</c> SIO event handler. The wire shape verified at
/// <c>RealTimeNetworkAgent.CreatePackEmitHandData:815-817</c>:
/// <code>
/// return MessagePackSerializer.Serialize(JsonMapper.ToJson(info)); // info = List&lt;object&gt;, NOT encrypted
/// </code>
/// 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
/// <c>docs/audits/battle-node-sio-events-2026-06-02.md</c>.)
/// </summary>
[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<RealParticipant>.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<RealParticipant>.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<RealParticipant>.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<RealParticipant>.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
/// <summary>
/// Enqueue an SIO BinaryEvent("hand", {placeholder}) text frame followed by its single
/// binary attachment (NodeCrypto-encrypted msgpack-string of <paramref name="body"/>).
/// Wire format mirrors what <c>EmitFrontStockData:717-720</c> emits.
/// binary attachment (msgpack-string of the raw JSON, <b>not</b> encrypted —
/// CreatePackEmitHandData:815-817 does not call CryptAES.encryptForNode).
/// </summary>
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));
/// <summary>
/// SIO Ack wire form: EIO Message (4) + SIO Ack (3) + ackId + [arg]. Filter the text
/// sends for that shape.
/// </summary>
private static string? FindAckFrame(TestWebSocket ws, int ackId) =>
AllTextSends(ws).FirstOrDefault(s => s.StartsWith($"43{ackId}["));