Skip to content

Commit 89b3795

Browse files
mikhail-dclclaude
andauthored
fix: bound retained chat-reaction message IDs and validate relay identity (#9571)
* fix: bound retained chat-reaction message IDs and validate relay identity 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> * fix: pass the warm-up token to CommunitiesFeatureAccess in the chat bus 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> * Stabilize emoji panel positioning --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 3699c28 commit 89b3795

15 files changed

Lines changed: 422 additions & 46 deletions

Explorer/Assets/DCL/Chat/MessageBus/LiveKitChatMessagesBus.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ namespace DCL.Chat.MessageBus
2626
{
2727
public class LiveKitChatMessagesBus : IChatMessagesBus
2828
{
29+
// Hard ceiling on the dedup stamps retained per period. A flood of distinct timestamps
30+
// restarts the window instead of growing it, bounding the memory a sender can make this
31+
// cache hold.
32+
private const int MAX_DEDUP_ENTRIES = 2048;
33+
2934
private readonly IMessagePipesHub messagePipesHub;
3035
private readonly IMessageDeduplication<double> messageDeduplication;
3136
private readonly CancellationTokenSource cancellationTokenSource = new ();
@@ -52,7 +57,7 @@ public LiveKitChatMessagesBus(IMessagePipesHub messagePipesHub,
5257
IRoomHub roomHub)
5358
{
5459
this.messagePipesHub = messagePipesHub;
55-
messageDeduplication = new MessageDeduplication<double>();
60+
messageDeduplication = new MessageDeduplication<double>(MAX_DEDUP_ENTRIES);
5661
this.userBlockingCache = userBlockingCache;
5762
this.identityCache = identityCache;
5863
this.messageFactory = messageFactory;

Explorer/Assets/DCL/Chat/_Refactor/ChatInput/ChatInputView.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ public class EmojiContainer
2323
[field: SerializeField] internal EmojiPanelView emojiPanel { get; private set; }
2424
[field: SerializeField] internal AudioClipConfig addEmojiAudio { get; private set; }
2525
[field: SerializeField] internal AudioClipConfig openEmojiPanelAudio { get; private set; }
26+
27+
[field: SerializeField]
28+
[field: Tooltip("Space kept between the input field's top edge and the emoji panel's bottom edge.")]
29+
internal float emojiPanelGap { get; private set; } = 5f;
2630
}
2731

2832
[field: SerializeField] public CustomInputField inputField { get; private set; }

Explorer/Assets/DCL/Chat/_Refactor/ChatInput/States/EmojiPanelChatInputState.cs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using DCL.UI.CustomInputField;
55
using MVC;
66
using System;
7+
using UnityEngine;
78

89
namespace DCL.Chat.ChatInput
910
{
@@ -13,6 +14,7 @@ public class EmojiPanelChatInputState : IndependentMVCState, IDisposable
1314
private readonly EmojiPanelView emojiPanelView;
1415
private readonly ChatInputView.EmojiContainer emojiContainer;
1516
private readonly CustomInputField inputField;
17+
private readonly RectTransform inputFieldRect;
1618
private readonly ChatClickDetectionHandler clickDetectionHandler;
1719

1820
public EmojiPanelChatInputState(ChatInputView view, EmojiPanelPresenter emojiPanelPresenter, EmojiPanelView emojiPanelView)
@@ -23,6 +25,10 @@ public EmojiPanelChatInputState(ChatInputView view, EmojiPanelPresenter emojiPan
2325

2426
inputField = view.inputField;
2527

28+
// The field, not its container: the container keeps a fixed height while the field grows upwards as
29+
// the text wraps.
30+
inputFieldRect = (RectTransform)view.inputField.transform;
31+
2632
clickDetectionHandler = new ChatClickDetectionHandler(
2733
emojiPanelView.transform,
2834
emojiContainer.emojiPanelButton.transform);
@@ -32,7 +38,10 @@ public EmojiPanelChatInputState(ChatInputView view, EmojiPanelPresenter emojiPan
3238

3339
protected override void Activate()
3440
{
35-
emojiPanelView.ResetToDefaultPosition();
41+
// Measured from the input field on every open: the chat panel is re-laid-out whenever the voice-chat
42+
// panel changes height, and the field itself grows with wrapped text — neither of which a stored
43+
// position would survive.
44+
emojiPanelView.PositionAbove(inputFieldRect, emojiContainer.emojiPanelGap);
3645
emojiPanelPresenter.SetPanelVisibility(true);
3746
emojiContainer.emojiPanelButton.SetState(true);
3847
emojiPanelPresenter.EmojiSelected += OnEmojiSelected;

Explorer/Assets/DCL/Chat/_Refactor/ChatReactions/Core/ChatReactionsFactory.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using DCL.Chat.ChatReactions.Simulation.World;
77
using DCL.Chat.History;
88
using DCL.Diagnostics;
9+
using DCL.FeatureFlags;
910
using DCL.Friends.UserBlocking;
1011
using DCL.Multiplayer.Connections.DecentralandUrls;
1112
using DCL.Multiplayer.Connections.Messaging.Hubs;
@@ -155,6 +156,15 @@ private static IReactionMessageBus CreateReactionBus(
155156
ChatReactionsConfig reactionsConfig,
156157
int maxValidEmojiIndex)
157158
{
159+
// Gated here rather than only in the UI: subscribing the pipes would let any
160+
// co-located peer drive reaction decoding and retention on a client that cannot
161+
// show reactions at all.
162+
if (!FeatureFlagsConfiguration.Instance.IsEnabled(FeatureFlagsStrings.CHAT_REACTIONS_ENABLED))
163+
{
164+
ReportHub.Log(ReportCategory.CHAT_MESSAGES, "[ChatPlugin] Chat reactions disabled — using NullReactionMessageBus (no pipes subscribed)");
165+
return new NullReactionMessageBus();
166+
}
167+
158168
string serverEnv = environment switch
159169
{
160170
DecentralandEnvironment.Org => "prd",

Explorer/Assets/DCL/Chat/_Refactor/ChatReactions/Networking/MultiplayerReactionMessageBus.cs

Lines changed: 49 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
using DCL.Multiplayer.Connections.Messaging.Pipe;
1313
using DCL.Multiplayer.Deduplication;
1414
using DCL.Utilities;
15+
using DCL.Web3;
1516
using DCL.Web3.Identities;
1617
using Decentraland.Kernel.Comms.Rfc4;
1718
using LiveKit.Proto;
@@ -27,15 +28,25 @@ public sealed class MultiplayerReactionMessageBus : IReactionMessageBus
2728
// The effective bound is tightened to the actual atlas tile count via the constructor.
2829
private const int MAX_VALID_EMOJI_INDEX = 4096;
2930

31+
// Message IDs are produced locally in one of two shapes: ChatUtils.GetId
32+
// ("{42-char address}:{invariant double}", ~67 characters) or a 36-character GUID for
33+
// system messages. 96 clears both with headroom while keeping an ID that arrives from
34+
// the network bounded before it is used to build anything.
35+
private const int MAX_MESSAGE_ID_LENGTH = 96;
36+
37+
// Hard ceiling on the dedup keys either cache retains. A flood of distinct keys restarts
38+
// the window instead of growing it, bounding the memory a peer can make this bus hold.
39+
private const int MAX_DEDUP_ENTRIES = 2048;
40+
3041
private readonly IMessagePipesHub messagePipesHub;
3142
private readonly IUserBlockingCache userBlockingCache;
3243
private readonly IWeb3IdentityCache identityCache;
3344
private readonly string routingUser;
3445
private readonly int maxValidEmojiIndex;
3546
private readonly int situationalReceiveCountCap;
3647
private readonly CancellationTokenSource cts = new ();
37-
private readonly IMessageDeduplication<float> situationalDedup = new MessageDeduplication<float>();
38-
private readonly IMessageDeduplication<string> chatReactionDedup = new MessageDeduplication<string>();
48+
private readonly IMessageDeduplication<float> situationalDedup = new MessageDeduplication<float>(MAX_DEDUP_ENTRIES);
49+
private readonly IMessageDeduplication<string> chatReactionDedup = new MessageDeduplication<string>(MAX_DEDUP_ENTRIES);
3950
private readonly PerSenderRateLimiter situationalRateLimiter;
4051
private readonly PerSenderRateLimiter chatReactionRateLimiter;
4152

@@ -187,25 +198,25 @@ private void OnChatReactionReceived(ReceivedMessage<ChatReaction> receivedMessag
187198
{
188199
using (receivedMessage)
189200
{
190-
// For community messages relayed through the message-router, FromWalletId
191-
// is the relay's identity, not the original sender. Fall back to Payload.Address
192-
// only in that case. For direct messages (nearby/DM), always use the trusted
193-
// transport-level FromWalletId to prevent identity spoofing.
194-
// NOTE: Add server-stamped ForwardedFrom to the ChatReaction protocol (like Chat
195-
// has) so the relay writes the verified sender identity, not the client.
196-
string walletId = receivedMessage.FromWalletId == routingUser
197-
&& !string.IsNullOrEmpty(receivedMessage.Payload.Address)
198-
? receivedMessage.Payload.Address
199-
: receivedMessage.FromWalletId;
200-
201-
ReportHub.Log(ReportCategory.CHAT_MESSAGES, $"[MultiplayerReactionBus] OnChatReactionReceived raw: emoji={receivedMessage.Payload.EmojiIndex} messageId={receivedMessage.Payload.MessageId} address={receivedMessage.Payload.Address} fromWallet={receivedMessage.FromWalletId}");
202-
203201
if (cts.IsCancellationRequested)
204202
{
205203
ReportHub.LogWarning(ReportCategory.CHAT_MESSAGES, "[MultiplayerReactionBus] OnChatReactionReceived skipped — CTS cancelled");
206204
return;
207205
}
208206

207+
string messageId = receivedMessage.Payload.MessageId;
208+
209+
// Checked before the dedup keys exist, so an ID no local message could carry
210+
// never reaches the dedup cache. Dropped silently: ReportHub.LogWarning is not
211+
// compiled out, so naming the ID here would allocate it once per packet in
212+
// retail builds.
213+
if (string.IsNullOrEmpty(messageId) || messageId.Length > MAX_MESSAGE_ID_LENGTH)
214+
return;
215+
216+
string walletId = ResolveSenderWalletId(receivedMessage);
217+
218+
ReportHub.Log(ReportCategory.CHAT_MESSAGES, $"[MultiplayerReactionBus] OnChatReactionReceived raw: emoji={receivedMessage.Payload.EmojiIndex} messageId={messageId} fromWallet={receivedMessage.FromWalletId} resolved={walletId}");
219+
209220
if (IsUserBlocked(walletId))
210221
{
211222
ReportHub.Log(ReportCategory.CHAT_MESSAGES, $"[MultiplayerReactionBus] OnChatReactionReceived skipped — user blocked: {walletId}");
@@ -223,8 +234,8 @@ private void OnChatReactionReceived(ReceivedMessage<ChatReaction> receivedMessag
223234

224235
// Use raw value in dedup key so add/remove have distinct keys.
225236
// Evict the opposite key so toggling (add→remove→add) isn't blocked.
226-
string dedupKey = $"{receivedMessage.Payload.MessageId}:{rawEmojiIndex}";
227-
string oppositeKey = $"{receivedMessage.Payload.MessageId}:{ReactionWireEncoding.Encode(emojiIndex, !isRemoval)}";
237+
string dedupKey = $"{messageId}:{rawEmojiIndex}";
238+
string oppositeKey = $"{messageId}:{ReactionWireEncoding.Encode(emojiIndex, !isRemoval)}";
228239

229240
if (!chatReactionDedup.TryPass(walletId, dedupKey))
230241
{
@@ -239,18 +250,37 @@ private void OnChatReactionReceived(ReceivedMessage<ChatReaction> receivedMessag
239250
if (!chatReactionRateLimiter.TryPass(walletId, UnityEngine.Time.unscaledTime))
240251
return;
241252

242-
ReportHub.Log(ReportCategory.CHAT_MESSAGES, $"[MultiplayerReactionBus] Received chat reaction: emoji={emojiIndex} isRemoval={isRemoval} messageId={receivedMessage.Payload.MessageId} from={walletId}");
253+
ReportHub.Log(ReportCategory.CHAT_MESSAGES, $"[MultiplayerReactionBus] Received chat reaction: emoji={emojiIndex} isRemoval={isRemoval} messageId={messageId} from={walletId}");
243254

244255
ReactionReceived?.Invoke(new ReactionReceivedArgs(
245256
walletId,
246257
emojiIndex,
247258
1,
248259
ReactionType.Message,
249-
receivedMessage.Payload.MessageId,
260+
messageId,
250261
isRemoval));
251262
}
252263
}
253264

265+
private string ResolveSenderWalletId(ReceivedMessage<ChatReaction> receivedMessage)
266+
{
267+
// Community reactions relayed through the message-router arrive carrying the relay's
268+
// identity in FromWalletId, so the original sender can only come from the payload.
269+
// Reading it solely from the router, and solely when it is a canonical wallet
270+
// address, keeps a direct peer from asserting an arbitrary identity and keeps the
271+
// block, dedup and rate-limit keys bounded.
272+
// NOTE: Add a server-stamped ForwardedFrom to the ChatReaction protocol (like Chat
273+
// has) so the relay writes the verified sender identity, not the client.
274+
if (receivedMessage.FromWalletId != routingUser)
275+
return receivedMessage.FromWalletId;
276+
277+
string relayedAddress = receivedMessage.Payload.Address;
278+
279+
return Web3Address.IsValidWalletAddress(relayedAddress)
280+
? relayedAddress
281+
: receivedMessage.FromWalletId;
282+
}
283+
254284
private bool IsUserBlocked(string userAddress) =>
255285
userBlockingCache.UserIsBlocked(userAddress);
256286
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
using System;
2+
3+
namespace DCL.Chat.ChatReactions.Networking
4+
{
5+
/// <summary>
6+
/// Inert bus used while the chat-reactions feature flag is off. It subscribes to no
7+
/// message pipe and drops every send, so no reaction traffic is decoded, retained or
8+
/// emitted while the feature is unavailable.
9+
/// </summary>
10+
public sealed class NullReactionMessageBus : IReactionMessageBus
11+
{
12+
/// <summary>
13+
/// Never raised — nothing is subscribed, so no reaction can arrive.
14+
/// </summary>
15+
public event Action<ReactionReceivedArgs> ReactionReceived
16+
{
17+
add { }
18+
remove { }
19+
}
20+
21+
public void Dispose() { }
22+
23+
public void SendSituationalReaction(int emojiIndex, int count = 1, float overrideTimestamp = 0f) { }
24+
25+
public void SendMessageReaction(int emojiIndex, string messageId, ReactionChannelRouting routing) { }
26+
}
27+
}

Explorer/Assets/DCL/Chat/_Refactor/ChatReactions/Networking/NullReactionMessageBus.cs.meta

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)