diff --git a/SVSim.BattleNode/Bridge/BattlePlayer.cs b/SVSim.BattleNode/Bridge/BattlePlayer.cs index aadb460..e7174f4 100644 --- a/SVSim.BattleNode/Bridge/BattlePlayer.cs +++ b/SVSim.BattleNode/Bridge/BattlePlayer.cs @@ -1,5 +1,10 @@ namespace SVSim.BattleNode.Bridge; /// One player slot for a pending battle. Carries the viewer's identity and -/// the per-battle MatchContext snapshot built at do_matching time. +/// the per-battle MatchContext snapshot built at do_matching time. +/// FOOTGUN: this is a record, but transitively holds an +/// IReadOnlyList<long> (the deck), so the synthesized value-equality is REFERENCE-based +/// on that list — two BattlePlayers with equal deck *contents* compare unequal. Don't use +/// BattlePlayer / as dictionary keys or Distinct() / HashSet +/// operands without first giving them content equality. Not exercised today. public sealed record BattlePlayer(long ViewerId, MatchContext Context); diff --git a/SVSim.BattleNode/Bridge/MatchContext.cs b/SVSim.BattleNode/Bridge/MatchContext.cs index da0a684..041ae29 100644 --- a/SVSim.BattleNode/Bridge/MatchContext.cs +++ b/SVSim.BattleNode/Bridge/MatchContext.cs @@ -5,6 +5,9 @@ namespace SVSim.BattleNode.Bridge; /// server-authored frame lifecycle on WS connect. SVSim.BattleNode does not know how to build this — the HTTP-side /// per-mode controller is the source. Snapshot semantics: cosmetic changes between matching /// and WS connect have no effect on the in-battle render. +/// FOOTGUN: as a record holding (an IReadOnlyList), the +/// synthesized value-equality is reference-based on that list — see . +/// Don't use as a dict key / Distinct() operand without content equality. /// public sealed record MatchContext( // Player's drafted deck — exactly 30 entries, idx 1..30 paired with the chosen cardIds diff --git a/SVSim.BattleNode/Reliability/OutboundSequencer.cs b/SVSim.BattleNode/Reliability/OutboundSequencer.cs index 3b5f2a6..7bd751b 100644 --- a/SVSim.BattleNode/Reliability/OutboundSequencer.cs +++ b/SVSim.BattleNode/Reliability/OutboundSequencer.cs @@ -14,6 +14,11 @@ public sealed class OutboundSequencer private const long FirstPlaySeq = 1; private long _next = FirstPlaySeq; + + // Holds every ordered (stocked) push for the WHOLE match — there is no per-ack pruning, so it + // grows with battle length × concurrent battles. Bounded only by Clear() in the terminate cascade. + // Fine at current scale; if battles get long or concurrency scales, prune entries below the peer's + // ack watermark here (contrast the inbound side, which is bounded by InboundTracker.WindowSize). private readonly Dictionary _archive = new(); public IReadOnlyDictionary Archive => _archive; diff --git a/SVSim.BattleNode/Sessions/Dispatch/BattleSessionState.cs b/SVSim.BattleNode/Sessions/Dispatch/BattleSessionState.cs index f3b9fcd..30b100d 100644 --- a/SVSim.BattleNode/Sessions/Dispatch/BattleSessionState.cs +++ b/SVSim.BattleNode/Sessions/Dispatch/BattleSessionState.cs @@ -84,6 +84,11 @@ internal sealed class BattleSessionState /// Echo is mined but never relayed. public void RecordTokensFrom(IBattleParticipant from, IBattleParticipant other, object? orderList) { + // TRUST: isSelf is the SENDER's own perspective flag and idx is unbounded, while RecordToken + // overwrites-on-conflict. A buggy/malicious sender could pass isSelf:0 with a deck-range idx to + // rewrite the OPPONENT's card identity at a seeded slot. Acceptable for the current trusted-LAN + // relay; if peers ever become untrusted, gate on `idx > deckCount` here (generated tokens always + // allocate past the deck) so a sender can't forge over seeded deck cards. foreach (var (idx, cardId, isSelf) in KnownListBuilder.MineAddOps(orderList)) RecordToken(isSelf == CardOwner.Self ? from : other, idx, cardId); } diff --git a/SVSim.BattleNode/Wire/NodeCrypto.cs b/SVSim.BattleNode/Wire/NodeCrypto.cs index cf607be..205d990 100644 --- a/SVSim.BattleNode/Wire/NodeCrypto.cs +++ b/SVSim.BattleNode/Wire/NodeCrypto.cs @@ -73,6 +73,16 @@ public static class NodeCrypto /// is the -char ASCII key the encrypt / /// decrypt path has already validated. /// + /// + /// SECURITY (latent — do NOT "tidy" this into a cached key): the IV is derived from the key, so a + /// fixed key reuses a fixed IV — the classic CBC IV-reuse weakness (equal plaintext prefixes → + /// equal ciphertext prefixes). It is masked ONLY because every server-initiated send mints a fresh + /// key via , so (key, IV) never repeats in practice. A future change that + /// CACHES the session key would silently reintroduce the leak — derive a per-message random IV + /// first. Related: base64-truncates a hex string, so the effective key + /// entropy is well below what "AES-256" implies. We mirror the client's scheme deliberately; both + /// are acceptable only because this is a localhost relay, not a hostile-network transport. + /// private static Aes BuildAes(string key) { var aes = Aes.Create();