fix: bound retained chat-reaction message IDs and validate relay identity - #9571
fix: bound retained chat-reaction message IDs and validate relay identity#9571mikhail-dcl wants to merge 3 commits into
Conversation
…tity Inbound ChatReaction built its dedup key from the peer-controlled MessageId and retained it before anything verified that the message existed, so a peer could flood unique packet-sized IDs and grow the dedup set unbounded for the full 5-minute window. The relayed Payload.Address was trusted with no format validation at all, which also handed an attacker the wallet half of that key. - Reject empty or over-length MessageId at intake, before any key is built. Locally produced IDs are either ChatUtils.GetId or a GUID, so the cap clears both with headroom. Dropped silently: ReportHub.LogWarning is not compiled out, so naming the ID would allocate it once per packet in retail builds. - Give MessageDeduplication<T> an opt-in capacity (default unbounded, leaving other callers unchanged) and bound both reaction caches plus the nearby-chat one. At capacity Register drops the window instead of growing it. - Trust Payload.Address only when it arrives from the message-router and passes Web3Address.IsValidWalletAddress, matching the Chat ForwardedFrom fix (#9501). - Return a NullReactionMessageBus while alfa-chat-reactions is off so no pipe is subscribed; the flag previously gated the UI only. SEC-085. The rate limiter deliberately stays after dedup: nearby reactions legitimately arrive on both the island and scene pipes, so moving it earlier would halve every honest client's budget. Closing the attribution leg needs a server-stamped sender on the ChatReaction wire type, which has no forwarded_from field - that is comms-message-sfu work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…essageid-retention
…us tests dev does not compile on its own: #9501 added this fixture calling CommunitiesFeatureAccess(identityCache, appArgs) while #9472 added a required third warmUpCt parameter. Both merged without rebasing against each other, so the EditMode assembly fails with CS7036 and no test can run. Unrelated to SEC-085 — kept as its own commit so it can be dropped or moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warnings count reduced: 13975 => 13974 Warnings/errors in files changed by this PR (9) |
|
New build in progress, come back later! |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix: bound retained chat-reaction message IDs and validate relay identity
STEP 2 — Root-cause check ✅ PASS
The problem is unbounded memory growth in the dedup cache from peer-controlled MessageId values, and untrusted Payload.Address on the relay path. This PR fixes the cause, not a symptom:
- Message IDs are validated (empty/over-length) at intake before any dedup key is constructed.
MessageDeduplication<T>gains an opt-in capacity that drops the window when the bound is reached.Payload.Addressis only trusted from the knownroutingUseridentity and only when it passesWeb3Address.IsValidWalletAddress.NullReactionMessageBuseliminates the entire attack surface whenalfa-chat-reactionsis disabled.
STEP 3 — Design & integration ✅ PASS
NullReactionMessageBus — Correct null-object pattern. The empty add { } remove { } event accessors prevent subscriber leaks; no-op method bodies drop every send. The factory is the right gate point: CreateReactionBus is the composition root for the bus, so the decision between real and null implementations belongs there. All downstream code (ChatMessageReactionService, ReactionRouter, SituationalReactionFacade) operates against IReactionMessageBus without any conditional branches. Dispose() is a correct no-op. No lifecycle reconciliation or new long-lived unit is introduced — this is just a factory decision.
ResolveSenderWalletId extraction — Private method within the owning class, not a new unit. The security-critical wallet resolution logic (router check + IsValidWalletAddress validation) is cleanly isolated. Single use is justified for readability in a security-sensitive handler.
MessageDeduplication capacity — Opt-in parameterization of an existing class. The UNBOUNDED_CAPACITY default preserves backward compatibility — existing callers are unchanged. No new lifecycle owner, no persistent state outside ECS (this is a networking utility, not an ECS system).
Owner search: MessageDeduplication<T> is created as a field initializer in MultiplayerReactionMessageBus and LiveKitChatMessagesBus; both are created by their respective factories and disposed via EventSubscriptionScope / container disposal. No new lifecycle ownership is introduced.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
MessageDeduplication.UNBOUNDED_CAPACITY (public const) |
Default parameter on constructor; MessageDeduplicationShould.TreatTheUnboundedConstantAsNoLimit test |
Reasonable API — gives a name to the sentinel so callers don't pass raw 0. Consider narrowing to internal since the parameterless constructor already provides unbounded behavior. |
MessageDeduplication(int capacity) (public ctor) |
MultiplayerReactionMessageBus (×2), LiveKitChatMessagesBus (×1) |
Clean — a convenience overload that delegates to the full constructor. |
NullReactionMessageBus (all members) |
All are IReactionMessageBus interface implementations; created by ChatReactionsFactory.CreateReactionBus. |
Correct null-object pattern. |
ResolveSenderWalletId (private) |
Called once from OnChatReactionReceived. |
Justified single-use extraction — isolates security logic for auditability. |
STEP 5 — Line-level review
Security review: No issues found. The messageId length check is placed before dedup key construction. The wallet validation uses a robust format check (exactly 42 chars, 0x prefix, hex-only). The capacity bound prevents HashSet growth. The NullReactionMessageBus eliminates the attack surface when the feature is off. Pre-existing limitation (relay trusts client-supplied Payload.Address) is partially mitigated and explicitly documented as requiring server-stamped ForwardedFrom — that is out of scope (comms-message-sfu work).
Teardown/consumption trace: MultiplayerReactionMessageBus.Dispose() → cts.SafeCancelAndDispose() — unchanged and correct. NullReactionMessageBus.Dispose() — correct no-op (nothing to clean up). No new subscriptions, event hookups, or connections are introduced that lack a matching teardown.
One P2 finding noted as inline comment below.
Observations (not blocking):
MAX_DEDUP_ENTRIES = 2048is declared independently in bothLiveKitChatMessagesBusandMultiplayerReactionMessageBus. Since these serve different subsystems (chat messages vs. reactions) and could diverge intentionally, this is acceptable. A brief comment noting the independence would prevent future consolidation mistakes.DateTime.NowvsDateTime.UtcNowinMessageDeduplicationis pre-existing.UtcNowavoids timezone conversion overhead and DST edge cases — worth a follow-up but not introduced by this PR.
STEP 6 — Complexity: COMPLEX
Modifies multiplayer deduplication infrastructure (MessageDeduplication<T>), chat-reactions networking (MultiplayerReactionMessageBus), and adds security-critical input validation. Touches 8 meaningful C# files across 2 subsystems.
STEP 7 — QA: YES
Runtime code changes affecting chat reactions (behind alfa-chat-reactions feature flag). Changes affect what users see (reaction attribution, reaction processing). QA plan is well-specified in the PR description.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies multiplayer deduplication infrastructure and chat-reactions networking with security-critical input validation and relay identity checks
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
| public MessageDeduplication(TimeSpan cleanPerPeriod, int capacity = UNBOUNDED_CAPACITY) | ||
| { | ||
| this.cleanPerPeriod = cleanPerPeriod; | ||
| this.capacity = capacity; | ||
| previousClean = DateTime.Now; | ||
| } |
There was a problem hiding this comment.
[P2] Missing argument validation for negative capacity. Passing a negative value (e.g. capacity = -1) makes registeredStamps.Count >= capacity always true, so every Register call clears the set — dedup becomes completely ineffective. While all current callers pass known positive constants, a future caller could trigger this silently.
| public MessageDeduplication(TimeSpan cleanPerPeriod, int capacity = UNBOUNDED_CAPACITY) | |
| { | |
| this.cleanPerPeriod = cleanPerPeriod; | |
| this.capacity = capacity; | |
| previousClean = DateTime.Now; | |
| } | |
| public MessageDeduplication(TimeSpan cleanPerPeriod, int capacity = UNBOUNDED_CAPACITY) | |
| { | |
| if (capacity < 0) | |
| throw new ArgumentOutOfRangeException(nameof(capacity), capacity, "Capacity must be non-negative; use UNBOUNDED_CAPACITY (0) for no limit."); | |
| this.cleanPerPeriod = cleanPerPeriod; | |
| this.capacity = capacity; | |
| previousClean = DateTime.Now; | |
| } |
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
All Unity tests passed ✅
|
2 participants
Closes SEC-085.
Problem
Inbound
ChatReactionbuilt its dedup key from the peer-controlledMessageIdand retained it before anything checked the message existed, so a peer could flood unique packet-sized IDs and grow the dedup set unbounded for the full 5-minute window.Payload.Addresswas trusted on the relay path with no format validation, handing an attacker the wallet half of that key too.#9489's rate limiter already capped the downstream work, so the residual was retained memory, not a CPU/GC storm.
Changes
MessageIdat intake, before any key is built. Dropped silently:ReportHub.LogWarningis not compiled out, so naming the ID would allocate it once per packet in retail builds.MessageDeduplication<T>gains an opt-in capacity (default unbounded, so other callers are unchanged). Both reaction caches and the nearby-chat one are bounded; at capacityRegisterdrops the window instead of growing it.Payload.Addressonly when it arrives from the message-router and passesWeb3Address.IsValidWalletAddress(the validator fix: authenticate chat sender identity before trusting ForwardedFrom #9501 just added).NullReactionMessageBuswhilealfa-chat-reactionsis off, so no pipe is subscribed — the flag previously gated the UI only.Deliberately unchanged: the rate limiter stays after dedup. Nearby reactions legitimately arrive on both the island and scene pipes, so moving it earlier would halve every honest client's budget.
Heads-up on the second commit
devdoes not compile on its own — #9501's newLiveKitChatMessagesBusShouldcallsCommunitiesFeatureAccess(identityCache, appArgs)while #9472 added a required thirdwarmUpCtparameter.CS7036kills the whole EditMode assembly, so no test runs on dev at all. Fixed in its own commit here so it can be dropped or moved to a hotfix.QA
Reactions are behind
alfa-chat-reactions.message-router-{env}-0. Check bothzoneandorg—routingUseris environment-derived. This is the main regression risk.Under sustained flood a dedup cache at capacity drops its window, so a duplicate nearby message can render once more. Reactions converge, being set operations.
Out of scope
Fully closing attribution needs a server-stamped sender on
ChatReaction, which has noforwarded_fromfield — that iscomms-message-sfuwork.Verified: 96/96 EditMode tests, 0 compile errors, no new lint findings.
🤖 Generated with Claude Code