docs(battlenode): document four latent low-tier hygiene hazards
Comment-only; behavior-preserving; 231 BattleNode tests green. - OutboundSequencer._archive: name the unbounded-per-match growth + ack-prune point. - NodeCrypto.BuildAes: SECURITY remarks on key-derived IV reuse + base64 entropy loss; warn against caching the session key. - MatchContext/BattlePlayer: FOOTGUN notes on reference-based record equality over the deck list. - RecordTokensFrom: TRUST note on isSelf/idx overwrite; name the idx>deckCount guard for untrusted peers (not added — trusted-LAN today). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,10 @@
|
|||||||
namespace SVSim.BattleNode.Bridge;
|
namespace SVSim.BattleNode.Bridge;
|
||||||
|
|
||||||
/// <summary>One player slot for a pending battle. Carries the viewer's identity and
|
/// <summary>One player slot for a pending battle. Carries the viewer's identity and
|
||||||
/// the per-battle MatchContext snapshot built at do_matching time.</summary>
|
/// the per-battle MatchContext snapshot built at do_matching time.
|
||||||
|
/// <para>FOOTGUN: this is a <c>record</c>, but <see cref="Context"/> transitively holds an
|
||||||
|
/// <c>IReadOnlyList<long></c> (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 / <see cref="MatchContext"/> as dictionary keys or <c>Distinct()</c> / <c>HashSet</c>
|
||||||
|
/// operands without first giving them content equality. Not exercised today.</para></summary>
|
||||||
public sealed record BattlePlayer(long ViewerId, MatchContext Context);
|
public sealed record BattlePlayer(long ViewerId, MatchContext Context);
|
||||||
|
|||||||
@@ -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
|
/// 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
|
/// per-mode controller is the source. Snapshot semantics: cosmetic changes between matching
|
||||||
/// and WS connect have no effect on the in-battle render.
|
/// and WS connect have no effect on the in-battle render.
|
||||||
|
/// <para>FOOTGUN: as a record holding <see cref="SelfDeckCardIds"/> (an IReadOnlyList), the
|
||||||
|
/// synthesized value-equality is reference-based on that list — see <see cref="BattlePlayer"/>.
|
||||||
|
/// Don't use as a dict key / <c>Distinct()</c> operand without content equality.</para>
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public sealed record MatchContext(
|
public sealed record MatchContext(
|
||||||
// Player's drafted deck — exactly 30 entries, idx 1..30 paired with the chosen cardIds
|
// Player's drafted deck — exactly 30 entries, idx 1..30 paired with the chosen cardIds
|
||||||
|
|||||||
@@ -14,6 +14,11 @@ public sealed class OutboundSequencer
|
|||||||
private const long FirstPlaySeq = 1;
|
private const long FirstPlaySeq = 1;
|
||||||
|
|
||||||
private long _next = FirstPlaySeq;
|
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<long, MsgEnvelope> _archive = new();
|
private readonly Dictionary<long, MsgEnvelope> _archive = new();
|
||||||
|
|
||||||
public IReadOnlyDictionary<long, MsgEnvelope> Archive => _archive;
|
public IReadOnlyDictionary<long, MsgEnvelope> Archive => _archive;
|
||||||
|
|||||||
@@ -84,6 +84,11 @@ internal sealed class BattleSessionState
|
|||||||
/// Echo is mined but never relayed.</summary>
|
/// Echo is mined but never relayed.</summary>
|
||||||
public void RecordTokensFrom(IBattleParticipant from, IBattleParticipant other, object? orderList)
|
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))
|
foreach (var (idx, cardId, isSelf) in KnownListBuilder.MineAddOps(orderList))
|
||||||
RecordToken(isSelf == CardOwner.Self ? from : other, idx, cardId);
|
RecordToken(isSelf == CardOwner.Self ? from : other, idx, cardId);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -73,6 +73,16 @@ public static class NodeCrypto
|
|||||||
/// <paramref name="key"/> is the <see cref="KeyLength"/>-char ASCII key the encrypt /
|
/// <paramref name="key"/> is the <see cref="KeyLength"/>-char ASCII key the encrypt /
|
||||||
/// decrypt path has already validated.
|
/// decrypt path has already validated.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// 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 <see cref="GenerateKey"/>, 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: <see cref="GenerateKey"/> 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.
|
||||||
|
/// </remarks>
|
||||||
private static Aes BuildAes(string key)
|
private static Aes BuildAes(string key)
|
||||||
{
|
{
|
||||||
var aes = Aes.Create();
|
var aes = Aes.Create();
|
||||||
|
|||||||
Reference in New Issue
Block a user