From 977af9806900a3585943b1877e2b88cd19f4234e Mon Sep 17 00:00:00 2001 From: Strycher Date: Sat, 18 Jul 2026 22:25:08 -0400 Subject: [PATCH] fix(#250): refuse to block your own node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking yourself silently hid your own traffic with no visible cause, and because blocks are offloaded to the radio it was pushed there and could be pulled back by the connect-time union. BlockService gains a selfKeyHex, injected by the connector when the self key is learned and cleared on disconnect. Every add path refuses it (block, importKeys, maybePromote) — importKeys specifically, so a self-block already on the radio cannot be re-imported. setSelfKey also heals an existing self-block and pushes REMOVE, covering the case where the union pull lands before self-info arrives. Contacts and discovery sheets hide the Block option for your own node. Per Gemini review: heal on an unchanged key too (an early return would strand a self-block introduced after the key was known), and order setSelfKey so all in-memory state settles synchronously, keeping callers consistent even though the connector fires it unawaited. Adds test/services/block_service_test.dart — the first coverage for BlockService (10 tests). Co-Authored-By: Claude Opus 4.8 --- lib/connector/meshcore_connector.dart | 13 +++ lib/screens/contacts_screen.dart | 39 ++++--- lib/screens/discovery_screen.dart | 23 ++-- lib/services/block_service.dart | 45 +++++++- test/services/block_service_test.dart | 154 ++++++++++++++++++++++++++ 5 files changed, 246 insertions(+), 28 deletions(-) create mode 100644 test/services/block_service_test.dart diff --git a/lib/connector/meshcore_connector.dart b/lib/connector/meshcore_connector.dart index 76a449d..6fba8e9 100644 --- a/lib/connector/meshcore_connector.dart +++ b/lib/connector/meshcore_connector.dart @@ -2503,8 +2503,19 @@ class MeshCoreConnector extends ChangeNotifier { _maybeStartInitialChannelSync(); } + /// Keep [BlockService]'s notion of "me" in sync with the connected node, so + /// it can refuse a self-block and self-heal one that already exists — the + /// union pull can land before self-info arrives, so healing matters (#250). + void _applySelfKeyToBlockService() { + final service = _blockService; + if (service == null) return; + final key = _selfPublicKey; + unawaited(service.setSelfKey(key == null ? null : pubKeyToHex(key))); + } + void _resetConnectionHandshakeState() { _selfPublicKey = null; + _applySelfKeyToBlockService(); _selfName = null; _selfLatitude = null; _selfLongitude = null; @@ -2671,6 +2682,7 @@ class MeshCoreConnector extends ChangeNotifier { _conversations.clear(); _loadedConversationKeys.clear(); _selfPublicKey = null; + _applySelfKeyToBlockService(); _selfName = null; _selfLatitude = null; _selfLongitude = null; @@ -4489,6 +4501,7 @@ class MeshCoreConnector extends ChangeNotifier { _currentTxPower = reader.readInt8(); _maxTxPower = reader.readInt8(); _selfPublicKey = reader.readBytes(pubKeySize); + _applySelfKeyToBlockService(); _selfLatitude = reader.readInt32LE() / 1000000.0; _selfLongitude = reader.readInt32LE() / 1000000.0; _multiAcks = reader.readByte(); diff --git a/lib/screens/contacts_screen.dart b/lib/screens/contacts_screen.dart index 2b39a4e..4d178e0 100644 --- a/lib/screens/contacts_screen.dart +++ b/lib/screens/contacts_screen.dart @@ -1261,6 +1261,7 @@ class _ContactsScreenState extends State final isFavorite = contact.isFavorite; final blockService = context.read(); final isBlocked = blockService.isBlocked(contact.publicKeyHex); + final isSelf = blockService.isSelf(contact.publicKeyHex); showModalBottomSheet( context: context, @@ -1268,25 +1269,27 @@ class _ContactsScreenState extends State child: Column( mainAxisSize: MainAxisSize.min, children: [ - ListTile( - leading: Icon( - isBlocked ? Icons.check_circle_outline : Icons.block, - color: isBlocked ? null : Colors.red.shade700, - ), - title: Text( - isBlocked - ? context.l10n.block_unblock - : context.l10n.block_block, + // Blocking your own node is never meaningful (#250). + if (!isSelf) + ListTile( + leading: Icon( + isBlocked ? Icons.check_circle_outline : Icons.block, + color: isBlocked ? null : Colors.red.shade700, + ), + title: Text( + isBlocked + ? context.l10n.block_unblock + : context.l10n.block_block, + ), + onTap: () async { + Navigator.pop(sheetContext); + if (isBlocked) { + await blockService.unblock(contact.publicKeyHex); + } else { + await blockService.block(contact.publicKeyHex); + } + }, ), - onTap: () async { - Navigator.pop(sheetContext); - if (isBlocked) { - await blockService.unblock(contact.publicKeyHex); - } else { - await blockService.block(contact.publicKeyHex); - } - }, - ), if (isRepeater) ...[ ListTile( leading: const Icon(Icons.radar, color: Colors.green), diff --git a/lib/screens/discovery_screen.dart b/lib/screens/discovery_screen.dart index bb49969..d97f16f 100644 --- a/lib/screens/discovery_screen.dart +++ b/lib/screens/discovery_screen.dart @@ -220,6 +220,7 @@ class _DiscoveryScreenState extends State { ) async { final blockService = context.read(); final isBlocked = blockService.isBlocked(contact.publicKeyHex); + final isSelf = blockService.isSelf(contact.publicKeyHex); final action = await showModalBottomSheet( context: context, showDragHandle: true, @@ -239,16 +240,20 @@ class _DiscoveryScreenState extends State { title: Text(l10n.discoveredContacts_copyContact), onTap: () => Navigator.of(sheetContext).pop('copy_contact'), ), - ListTile( - leading: Icon( - isBlocked ? Icons.check_circle_outline : Icons.block, - color: isBlocked ? null : Colors.red.shade700, + // Blocking your own node is never meaningful (#250). + if (!isSelf) + ListTile( + leading: Icon( + isBlocked ? Icons.check_circle_outline : Icons.block, + color: isBlocked ? null : Colors.red.shade700, + ), + title: Text( + isBlocked ? l10n.block_unblock : l10n.block_block, + ), + onTap: () => Navigator.of( + sheetContext, + ).pop(isBlocked ? 'unblock' : 'block'), ), - title: Text(isBlocked ? l10n.block_unblock : l10n.block_block), - onTap: () => Navigator.of( - sheetContext, - ).pop(isBlocked ? 'unblock' : 'block'), - ), ListTile( leading: const Icon(Icons.delete), title: Text(l10n.discoveredContacts_deleteContact), diff --git a/lib/services/block_service.dart b/lib/services/block_service.dart index 07d7403..0b27747 100644 --- a/lib/services/block_service.dart +++ b/lib/services/block_service.dart @@ -22,6 +22,37 @@ class BlockService extends ChangeNotifier { /// (the pull side), so a synced key never echoes back to the radio. void Function(String keyHex, bool blocked)? firmwareSync; + String? _selfKeyHex; + + /// The connected node's own public key, injected by the connector once it is + /// learned and cleared on disconnect. Blocking yourself silently hides your + /// own traffic with no way to see why, so every add path refuses it (#250). + String? get selfKeyHex => _selfKeyHex; + + bool isSelf(String publicKeyHex) { + final self = _selfKeyHex; + return self != null && publicKeyHex.toLowerCase() == self; + } + + /// Called by the connector when the self key is learned (or cleared). If the + /// self key is already stored — blocked before this guard existed, or pulled + /// in from the radio's list — drop it here and tell the radio to remove it. + Future setSelfKey(String? publicKeyHex) async { + final key = publicKeyHex?.toLowerCase(); + final normalized = (key == null || key.isEmpty) ? null : key; + final changed = normalized != _selfKeyHex; + _selfKeyHex = normalized; + // Heal even when the key is unchanged — a self-block can appear after the + // key is already known (a union pull racing self-info, or a stale store), + // and an unchanged-key early return would strand it. + final healed = normalized != null && _blockedKeys.remove(normalized); + if (healed) firmwareSync?.call(normalized, false); + if (changed || healed) notifyListeners(); + // Everything above is synchronous, so a caller that doesn't await this + // still observes consistent state immediately; only the write is deferred. + if (healed) await _store.saveKeys(_blockedKeys); + } + /// Load persisted state. Call once during app startup. Future load() async { _blockedKeys @@ -45,6 +76,7 @@ class BlockService extends ChangeNotifier { Future block(String publicKeyHex) async { final key = publicKeyHex.toLowerCase(); + if (isSelf(key)) return; if (!_blockedKeys.add(key)) return; await _store.saveKeys(_blockedKeys); firmwareSync?.call(key, true); @@ -65,7 +97,11 @@ class BlockService extends ChangeNotifier { Future importKeys(Iterable keysHex) async { var changed = false; for (final k in keysHex) { - if (_blockedKeys.add(k.toLowerCase())) changed = true; + final key = k.toLowerCase(); + // A self-block already pushed to the radio must not come back via the + // union pull (#250). + if (isSelf(key)) continue; + if (_blockedKeys.add(key)) changed = true; } if (!changed) return; await _store.saveKeys(_blockedKeys); @@ -97,6 +133,13 @@ class BlockService extends ChangeNotifier { if (n.isEmpty || !_blockedNames.containsKey(n)) return; final key = publicKeyHex.toLowerCase(); _blockedNames.remove(n); + // Your own name resolving to your own key must not promote into a + // self-block — drop the name block and stop (#250). + if (isSelf(key)) { + await _store.saveNames(_blockedNames); + notifyListeners(); + return; + } final added = _blockedKeys.add(key); await _store.saveNames(_blockedNames); await _store.saveKeys(_blockedKeys); diff --git a/test/services/block_service_test.dart b/test/services/block_service_test.dart new file mode 100644 index 0000000..a65e770 --- /dev/null +++ b/test/services/block_service_test.dart @@ -0,0 +1,154 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/services/block_service.dart'; +import 'package:meshcore_open/storage/block_store.dart'; + +/// In-memory stand-in so the service can be exercised without SharedPreferences. +class _FakeBlockStore implements BlockStore { + Set keys = {}; + Map names = {}; + + @override + Future> loadKeys() async => {...keys}; + + @override + Future saveKeys(Set value) async => keys = {...value}; + + @override + Future> loadNames() async => {...names}; + + @override + Future saveNames(Map value) async => names = {...value}; +} + +void main() { + const selfKey = 'aa11bb22cc33dd44'; + const otherKey = '99ff88ee77dd66cc'; + + late _FakeBlockStore store; + late BlockService service; + late List<({String key, bool blocked})> pushes; + + setUp(() { + store = _FakeBlockStore(); + service = BlockService(store: store); + pushes = []; + service.firmwareSync = (key, blocked) => + pushes.add((key: key, blocked: blocked)); + }); + + group('self-block guard (#250)', () { + test('block() refuses the self key and pushes nothing', () async { + await service.setSelfKey(selfKey); + pushes.clear(); + + await service.block(selfKey); + + expect(service.isBlocked(selfKey), isFalse); + expect(store.keys, isNot(contains(selfKey))); + expect(pushes, isEmpty); + }); + + test('block() still blocks a normal key', () async { + await service.setSelfKey(selfKey); + + await service.block(otherKey); + + expect(service.isBlocked(otherKey), isTrue); + expect(pushes, contains((key: otherKey, blocked: true))); + }); + + test('importKeys() skips self so the radio cannot re-import it', () async { + await service.setSelfKey(selfKey); + + await service.importKeys([selfKey, otherKey]); + + expect(service.isBlocked(selfKey), isFalse); + expect(service.isBlocked(otherKey), isTrue); + }); + + test('setSelfKey() heals an existing self-block and sends REMOVE', () async { + // Blocked before the guard existed (or pulled in before self-info landed). + await service.block(selfKey); + expect(service.isBlocked(selfKey), isTrue); + pushes.clear(); + + await service.setSelfKey(selfKey); + + expect(service.isBlocked(selfKey), isFalse); + expect(store.keys, isNot(contains(selfKey))); + expect(pushes, contains((key: selfKey, blocked: false))); + }); + + test('maybePromote() will not promote a name into a self-block', () async { + await service.setSelfKey(selfKey); + await service.blockName('me'); + pushes.clear(); + + await service.maybePromote('me', selfKey); + + expect(service.isBlocked(selfKey), isFalse); + expect(service.isNameBlocked('me'), isFalse); + expect(pushes, isEmpty); + }); + + test('heals a self-block even when the self key is unchanged', () async { + await service.setSelfKey(selfKey); + + // Stale persisted state reloaded while the self key is already known — + // load() does not filter, so this lands a self-block behind the guards. + store.keys = {selfKey, otherKey}; + await service.load(); + expect(service.isBlocked(selfKey), isTrue); + pushes.clear(); + + // Same key as before, so an unchanged-key early return would strand it. + await service.setSelfKey(selfKey); + + expect(service.isBlocked(selfKey), isFalse); + expect( + service.isBlocked(otherKey), + isTrue, + reason: 'only self is healed', + ); + expect(pushes, contains((key: selfKey, blocked: false))); + }); + + test('repeat setSelfKey with nothing to heal pushes nothing', () async { + await service.setSelfKey(selfKey); + pushes.clear(); + + await service.setSelfKey(selfKey); + + expect(pushes, isEmpty); + expect(service.isSelf(selfKey), isTrue); + }); + + test('state is consistent synchronously when not awaited', () async { + await service.setSelfKey(selfKey); + await service.setSelfKey(null); + await service.block(selfKey); + + // Fire-and-forget, as the connector does via unawaited(). + final future = service.setSelfKey(selfKey); + + expect(service.isSelf(selfKey), isTrue); + expect(service.isBlocked(selfKey), isFalse); + await future; + expect(store.keys, isNot(contains(selfKey))); + }); + + test('isSelf() is false when no self key is known', () { + expect(service.isSelf(selfKey), isFalse); + }); + + test('clearing the self key stops treating the old key as self', () async { + await service.setSelfKey(selfKey); + await service.setSelfKey(null); + + expect(service.isSelf(selfKey), isFalse); + + await service.block(selfKey); + expect(service.isBlocked(selfKey), isTrue); + }); + }); +}