From d63838a94b7b40561165486621970eaa686d0c5b Mon Sep 17 00:00:00 2001 From: Strycher Date: Fri, 24 Jul 2026 03:25:21 -0400 Subject: [PATCH] fix(#380): tighten reaction parsing after adversarial review Two of three Gemini findings held up against the code; the third did not. Accepted, message-swallow risk: the emoji check looked only at the first rune against a range list that included arrows, so a multi-line message beginning with an arrow and ending in eight Crockford characters would have been consumed and displayed as a reaction whose "emoji" was the whole sentence. Ben's own capture has U+2192 mid-sentence in ordinary channel traffic, so this was reachable, not theoretical. - drop U+2190-U+21FF and U+2934-U+2935; arrows carry the Unicode Emoji property but read as punctuation in prose - cap the emoji segment at 8 runes, which is clear of the longest ZWJ sequence and nowhere near a sentence. This is the guard that holds regardless of how the range list evolves. Accepted, dedup: the contact path keyed on target hash plus emoji only. A room server is many-party over the 1:1 transport, so two members sending the same emoji collapsed into one and the count stuck at 1, the same defect already fixed on the channel path. The reacting author prefix is now part of the key; in a true 1:1 it is empty and the key is unchanged. Rejected, room-server sender matching: the review claimed getSenderName resolves to the room itself and that room text carries a "Sender: " prefix. Neither is true. _resolveContactSenderName resolves the author via fourByteRoomContactKey to the real per-sender contact name, and room message text is stored bare with the author in its own field. One real sub-case survives: an author who is not in our contacts resolves to null and will not match, which degrades to queue-then-expire with a warn log. Also rejected: switching @[ lookup from first to last occurrence. The reference implementation uses the first, and diverging risks mismatching payloads it accepts. --- lib/connector/meshcore_connector.dart | 10 ++++- lib/helpers/pocketmesh_reaction.dart | 22 ++++++++--- test/helpers/pocketmesh_reaction_test.dart | 44 ++++++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/lib/connector/meshcore_connector.dart b/lib/connector/meshcore_connector.dart index 4289d8b..f8f0018 100644 --- a/lib/connector/meshcore_connector.dart +++ b/lib/connector/meshcore_connector.dart @@ -6546,10 +6546,16 @@ class MeshCoreConnector extends ChangeNotifier { _parsePocketMeshReaction(message.text, isDm: true) ?? _parsePocketMeshReaction(message.text, isDm: false); if (reactionInfo != null) { - // Check if we've already processed this exact reaction + // Check if we've already processed this exact reaction. A room server is + // many-party over the 1:1 transport, so the reacting author belongs in + // the key or two members sending the same emoji collapse into one; in a + // true 1:1 the author prefix is empty and the key is unchanged. _processedContactReactions.putIfAbsent(pubKeyHex, () => {}); + final reactingAuthor = message.fourByteRoomContactKey + .map((b) => b.toRadixString(16).padLeft(2, '0')) + .join(); final reactionIdentifier = - '${reactionInfo.targetHash}_${reactionInfo.emoji}'; + '${reactionInfo.targetHash}_${reactionInfo.emoji}_$reactingAuthor'; final isDuplicate = _processedContactReactions[pubKeyHex]!.contains( reactionIdentifier, diff --git a/lib/helpers/pocketmesh_reaction.dart b/lib/helpers/pocketmesh_reaction.dart index 234dcae..d8b3da1 100644 --- a/lib/helpers/pocketmesh_reaction.dart +++ b/lib/helpers/pocketmesh_reaction.dart @@ -87,7 +87,7 @@ class PocketMeshReaction { required String? sender, required String hash, }) { - if (emoji.isEmpty || !_startsWithEmoji(emoji)) return null; + if (!_isReactionEmoji(emoji)) return null; return PocketMeshReaction( emoji: emoji, targetSenderName: sender, @@ -95,23 +95,35 @@ class PocketMeshReaction { ); } + /// A reaction is one emoji, possibly with a variation selector, a skin-tone + /// modifier or ZWJ joins. Eight runes is well clear of the longest such + /// sequence and nowhere near a sentence. + /// + /// This cap is the guard that matters: without it, any multi-line message + /// starting with a symbol and ending in eight Crockford characters would be + /// swallowed whole and shown as the reaction "emoji". + static const int _maxEmojiRunes = 8; + /// Deliberately conservative: a missed emoji only means the reaction renders /// as text, which is the behaviour we have today, while a false positive /// would swallow a real message. + /// + /// Arrows (U+2190-U+21FF, U+2934-U+2935) are excluded on purpose even though + /// they carry the Unicode Emoji property. They are ordinary punctuation in + /// prose, and a live capture from this mesh contained U+2192 mid-sentence in + /// a normal channel message. static const List> _emojiRanges = [ [0x1F000, 0x1FAFF], [0x2600, 0x27BF], [0x2B00, 0x2BFF], - [0x2190, 0x21FF], - [0x2934, 0x2935], [0x3030, 0x3030], [0x303D, 0x303D], [0x3297, 0x3299], ]; - static bool _startsWithEmoji(String text) { + static bool _isReactionEmoji(String text) { final runes = text.runes; - if (runes.isEmpty) return false; + if (runes.isEmpty || runes.length > _maxEmojiRunes) return false; final first = runes.first; for (final range in _emojiRanges) { if (first >= range[0] && first <= range[1]) return true; diff --git a/test/helpers/pocketmesh_reaction_test.dart b/test/helpers/pocketmesh_reaction_test.dart index b3ab7d8..5d6efd3 100644 --- a/test/helpers/pocketmesh_reaction_test.dart +++ b/test/helpers/pocketmesh_reaction_test.dart @@ -142,6 +142,50 @@ void main() { expect(PocketMeshReaction.parse('\ndyps6yf0', isDm: true), isNull); }); + test('a sentence starting with an arrow is not a reaction', () { + // Arrows carry the Unicode Emoji property but are ordinary punctuation. + // A live capture from this mesh had U+2192 mid-sentence in a normal + // channel message, so the range is excluded outright. + expect( + PocketMeshReaction.parse( + '\u{2190} Turn left at the fork\ndyps6yf0', + isDm: true, + ), + isNull, + ); + expect( + PocketMeshReaction.parse('\u{2192}\ndyps6yf0', isDm: true), + isNull, + ); + }); + + test('a long run of text after a real emoji is not a reaction', () { + // The swallow risk the rune cap exists for: without it the whole body + // would become the reaction "emoji". + expect( + PocketMeshReaction.parse( + '\u{1F44D} thanks, that fixed it for me\ndyps6yf0', + isDm: true, + ), + isNull, + ); + }); + + test('accepts a multi-rune emoji sequence', () { + // Skin tone modifier, then a ZWJ family sequence. + expect( + PocketMeshReaction.parse('\u{1F44D}\u{1F3FD}\ndyps6yf0', isDm: true), + isNotNull, + ); + expect( + PocketMeshReaction.parse( + '\u{1F468}\u{200D}\u{1F469}\u{200D}\u{1F467}\ndyps6yf0', + isDm: true, + ), + isNotNull, + ); + }); + test('the leading character is not an emoji', () { // The realistic false positive: a two-line message whose last line // happens to be eight Crockford characters.