From 7f7e67cb70851dde4b7c6d00bd05aa04bd23d16a Mon Sep 17 00:00:00 2001 From: Strycher Date: Wed, 9 Sep 2026 02:30:35 -0400 Subject: [PATCH] fix(#611): apply Gemini review, accept our own channel format on paste Two accepted findings from the standards#145 adversarial review. Finding 4, the sharp one: the app emitted the compact card but could not parse it. A user copying a card out of a channel and pasting it into Add by public key would have been rejected by the app that produced it. Adds Contact.fromChannelShare and tries it after fromShareUri in the dialog, so both real formats are accepted. The parser splits on the FIRST TWO colons and takes the remainder as the name, because names carry colons, spaces, emoji and CJK. It also finds a card embedded in a longer message, which is how one actually arrives, since people caption them. Rendering a received card as a tappable affordance remains #610; this only covers text pasted or scanned into the add flow. #610 is correspondingly smaller now. Finding 2, performance: resolveContactVerification ran an O(N) message scan from a widget build inside a ListView. Now scans newest-first, since a delivered message is overwhelmingly likely to be recent, and advert-verified contacts still return on a single comparison without touching the message list. A lastMessageAt == epoch shortcut was written, then removed after checking _setContactLastMessageAt: it maintains that field only for advTypeChat, so a key-added repeater that had been messaged would have shown the wrong badge. A cheap wrong answer is worse than a slightly slower right one, and the rejected approach is documented in place. Epic #619. Co-Authored-By: Claude Opus 5 --- lib/models/contact.dart | 53 ++++++++++++++++++ lib/widgets/add_contact_by_key_dialog.dart | 12 +++- lib/widgets/contact_verification_badge.dart | 32 ++++++++--- test/models/contact_share_uri_test.dart | 61 +++++++++++++++++++++ 4 files changed, 146 insertions(+), 12 deletions(-) diff --git a/lib/models/contact.dart b/lib/models/contact.dart index bbe3621..df10384 100644 --- a/lib/models/contact.dart +++ b/lib/models/contact.dart @@ -325,6 +325,59 @@ class Contact { return '<$publicKeyHex:$type:$safeName>'; } + /// Parses the compact channel share ``, or null. (#611) + /// + /// The counterpart to [toChannelShare]. Without this the app would emit a + /// format it could not itself accept, and a user copying a card out of a + /// channel and pasting it into the add dialog would be rejected. + /// + /// Splits on the FIRST TWO colons only. The name is the final field and may + /// contain colons, spaces, emoji and CJK, so splitting on the last colon or + /// on every colon corrupts real names. + /// + /// Rendering a received card as a tappable Add Contact affordance is #610 and + /// is separate; this only handles text pasted or scanned into the add flow. + static Contact? fromChannelShare(String text) { + final trimmed = text.trim(); + // Tolerate a card embedded in a longer message, which is how it arrives. + final start = trimmed.indexOf('<'); + final end = trimmed.lastIndexOf('>'); + if (start < 0 || end <= start) return null; + final body = trimmed.substring(start + 1, end); + + final firstColon = body.indexOf(':'); + if (firstColon < 0) return null; + final secondColon = body.indexOf(':', firstColon + 1); + if (secondColon < 0) return null; + + final keyHex = body.substring(0, firstColon).toLowerCase(); + if (keyHex.length != pubKeySize * 2) return null; + final Uint8List publicKey; + try { + publicKey = hex2Uint8List(keyHex); + } on FormatException { + return null; + } + + final type = int.tryParse(body.substring(firstColon + 1, secondColon)); + if (type == null || type < advTypeChat || type > advTypeSensor) return null; + + final name = body.substring(secondColon + 1); + + return Contact( + publicKey: publicKey, + name: name.isEmpty ? 'Unknown' : name, + type: type, + flags: 0, + pathLength: -1, + path: Uint8List(0), + // Same unverified stub as the URI path, and for the same reason: the + // epoch keeps the firmware advert replay guard from muting it. (#620) + lastSeen: DateTime.fromMillisecondsSinceEpoch(0), + rawPacket: null, + ); + } + /// Parses a contact from the reference-app share URI, or null if malformed. /// /// `meshcore://contact/add?name=&public_key=<64 hex>&type=<1-4>` diff --git a/lib/widgets/add_contact_by_key_dialog.dart b/lib/widgets/add_contact_by_key_dialog.dart index 2004f50..7ec374f 100644 --- a/lib/widgets/add_contact_by_key_dialog.dart +++ b/lib/widgets/add_contact_by_key_dialog.dart @@ -72,10 +72,16 @@ class _AddContactByKeyDialogState extends State<_AddContactByKeyDialog> { Contact.buildShareUri(publicKeyHex: _cleanedKey, name: 'x', type: _type), ); - /// If a full contact link was pasted, absorb every field from it rather than - /// making the user retype a name they already have. + /// If a full contact link OR a channel contact card was pasted, absorb every + /// field from it rather than making the user retype a name they already have. + /// + /// Both formats are accepted because both are real and a user will meet both: + /// the `meshcore://` link comes from a QR or a DM, and the compact + /// `` card is what appears in channel traffic, including the + /// cards this app itself posts. Accepting only the link would mean rejecting + /// our own output. void _onKeyChanged(String raw) { - final pasted = Contact.fromShareUri(raw); + final pasted = Contact.fromShareUri(raw) ?? Contact.fromChannelShare(raw); if (pasted != null) { setState(() { _keyController.value = TextEditingValue( diff --git a/lib/widgets/contact_verification_badge.dart b/lib/widgets/contact_verification_badge.dart index 5e150c5..20d9091 100644 --- a/lib/widgets/contact_verification_badge.dart +++ b/lib/widgets/contact_verification_badge.dart @@ -27,20 +27,34 @@ enum ContactVerification { /// Resolves the verification state for [contact]. /// -/// The advert check is first because it is free and covers most contacts; only -/// an unverified contact pays for a message scan, which keeps this cheap on a -/// long contact list. +/// This runs from a widget `build` inside a contact `ListView`, so it does as +/// little as possible: +/// +/// 1. An advert-verified contact returns immediately on a single integer +/// comparison. That is most contacts, and no message list is touched. +/// 2. Only a key-only contact scans, and it scans **newest first**, because a +/// delivered message is overwhelmingly likely to be recent. +/// +/// A `lastMessageAt == epoch` shortcut was considered and **rejected**: +/// `_setContactLastMessageAt` maintains that field only for `advTypeChat`, so +/// a key-added repeater that had been messaged would have reported the wrong +/// badge. A cheap wrong answer is worse than a slightly slower right one. +/// +/// Remaining worst case is a key-only contact carrying many messages of which +/// none ever delivered. If that shows up in practice the fix is a cached flag +/// set once on first delivery, not a bounded scan, which could misreport. ContactVerification resolveContactVerification( Contact contact, MeshCoreConnector connector, ) { if (contact.isAdvertVerified) return ContactVerification.advertVerified; - final delivered = connector - .getMessages(contact) - .any((m) => m.status == MessageStatus.delivered); - return delivered - ? ContactVerification.keyConfirmed - : ContactVerification.keyOnly; + final messages = connector.getMessages(contact); + for (var i = messages.length - 1; i >= 0; i--) { + if (messages[i].status == MessageStatus.delivered) { + return ContactVerification.keyConfirmed; + } + } + return ContactVerification.keyOnly; } /// A small, deliberately calm indicator of how far a contact is confirmed. diff --git a/test/models/contact_share_uri_test.dart b/test/models/contact_share_uri_test.dart index 49fc2eb..8ed02f8 100644 --- a/test/models/contact_share_uri_test.dart +++ b/test/models/contact_share_uri_test.dart @@ -305,6 +305,67 @@ void main() { expect(stub('Bob', advTypeRepeater).toChannelShare(), '<$_key:2:Bob>'); }); + test('what we emit, we can also parse back', () { + // Without this the app would emit a format it could not itself accept, + // and pasting our own card into the add dialog would be rejected. + final c = stub('Roger KY4RS', advTypeRepeater); + final back = Contact.fromChannelShare(c.toChannelShare()); + expect(back, isNotNull); + expect(back!.publicKeyHex, _key); + expect(back.name, 'Roger KY4RS'); + expect(back.type, advTypeRepeater); + // Same unverified stub as the URI path. + expect(back.lastSeen, DateTime.fromMillisecondsSinceEpoch(0)); + expect(back.pathLength, -1); + }); + + test('a name containing colons survives the split', () { + // The name is the final field, so the parser must split on the first two + // colons only. Splitting on the last one would eat the name. + final back = Contact.fromChannelShare('<$_key:1:a:b:c>'); + expect(back, isNotNull); + expect(back!.name, 'a:b:c'); + }); + + test('a card embedded in a longer message is still found', () { + // This is how it actually arrives: someone captions their card. + final back = Contact.fromChannelShare( + 'here is mine <$_key:1:Bob> add me', + ); + expect(back, isNotNull); + expect(back!.name, 'Bob'); + }); + + test('emoji and CJK names round-trip', () { + for (final n in ['DIRT WIZARD πŸ§™', 'δΈ­ζ–‡θŠ‚η‚Ή', 'γƒŽγƒΌγƒ‰']) { + final back = Contact.fromChannelShare( + Contact.buildChannelShare(publicKeyHex: _key, name: n), + ); + expect(back, isNotNull, reason: 'name "$n" should parse'); + expect(back!.name, n); + } + }); + + test('rejects malformed cards', () { + for (final bad in [ + '<$_key:1>', // only one colon + '<$_key>', // no colons + '<0011:1:Bob>', // key too short + '<${_key}ff:1:Bob>', // key too long + '<${_key.replaceRange(0, 2, 'zz')}:1:Bob>', // not hex + '<$_key:9:Bob>', // type out of range + '<$_key:x:Bob>', // type not numeric + 'no brackets at all', + '', + ]) { + expect( + Contact.fromChannelShare(bad), + isNull, + reason: '"$bad" should be rejected', + ); + } + }); + test('the compact form is materially cheaper than the URI', () { // This is the whole reason both formats exist. Channel text shares a // 160-byte payload with the "Sender: " prefix, so the difference is