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.
fix/share-icon-android
Strycher 2 months ago
parent 0e926ad0c2
commit d63838a94b

@ -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,

@ -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<List<int>> _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;

@ -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.

Loading…
Cancel
Save

Powered by TurnKey Linux.