diff --git a/lib/connector/meshcore_connector.dart b/lib/connector/meshcore_connector.dart index d11a3ba..9ddc6e0 100644 --- a/lib/connector/meshcore_connector.dart +++ b/lib/connector/meshcore_connector.dart @@ -2768,14 +2768,18 @@ 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). + /// (Re)load the connected radio's per-radio block list and keep BlockService's + /// notion of "me" in sync. Called when the device key is learned (device-info) + /// and on disconnect/reset (null). loadForDevice sets the store scope + self + /// key synchronously, so an unawaited call is safe against a concurrent block + /// LIST dump; it also runs the #250 self-heal. Per-radio scoping is what stops + /// a block on one radio leaking to another and re-infecting a cleared radio + /// (#471). void _applySelfKeyToBlockService() { final service = _blockService; if (service == null) return; final key = _selfPublicKey; - unawaited(service.setSelfKey(key == null ? null : pubKeyToHex(key))); + unawaited(service.loadForDevice(key == null ? null : pubKeyToHex(key))); } void _resetConnectionHandshakeState() { diff --git a/lib/services/block_service.dart b/lib/services/block_service.dart index 9c9b081..c1318fa 100644 --- a/lib/services/block_service.dart +++ b/lib/services/block_service.dart @@ -2,11 +2,15 @@ import 'package:flutter/foundation.dart'; import '../storage/block_store.dart'; -/// App-global block list, backed by [BlockStore] and exposed to the UI. +/// Per-radio block list, backed by [BlockStore] and exposed to the UI. /// /// Source of truth for the client: DMs + adverts are filtered by public key, /// channel posts by resolved key or claimed name. Always active regardless of /// firmware; the firmware offload (Epic B) mirrors this, never replaces it. +/// +/// The set is scoped to the connected radio and swapped by [loadForDevice] on +/// connect; a block set on one radio never leaks to another, and clearing a +/// radio stays cleared (#471). There is no global list. /// See `docs/architecture/block-contract-as-built.md`. class BlockService extends ChangeNotifier { BlockService({BlockStore? store}) : _store = store ?? BlockStore(); @@ -22,6 +26,19 @@ class BlockService extends ChangeNotifier { /// (the pull side), so a synced key never echoes back to the radio. void Function(String keyHex, bool blocked)? firmwareSync; + /// Serializes all state-mutating operations so they can't interleave. The + /// connector fires [loadForDevice] (on connect) and [importKeys] (from the + /// block LIST dump) without awaiting, and they land in different frame + /// handlers; without this chain, loadForDevice's clear+reload could wipe keys + /// importKeys just added, or vice-versa (#471). Runs each op strictly after + /// the previous one settles; failures don't stall the chain. + Future _opChain = Future.value(); + Future _serialize(Future Function() op) { + final result = _opChain.then((_) => op()); + _opChain = result.then((_) {}, onError: (_) {}); + return result; + } + String? _selfKeyHex; /// The connected node's own public key, injected by the connector once it is @@ -34,27 +51,29 @@ class BlockService extends ChangeNotifier { 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(); + /// Call once during app startup. Drops the legacy unscoped global list + /// (#471, drop-and-start-fresh) and starts empty; the real per-radio list + /// loads via [loadForDevice] when a radio connects. + Future load() => _serialize(() async { + await _store.dropLegacyGlobal(); + _blockedKeys.clear(); + _blockedNames.clear(); + _selfKeyHex = null; + notifyListeners(); + }); + + /// (Re)load the block list for the connected radio, keyed by [deviceKeyHex] + /// (null on disconnect). Called by the connector when the device key is + /// learned or changes. Swaps the in-memory set so a block set on one radio + /// never leaks to another, and runs the #250 self-heal for this radio's own + /// key. Serialized against [importKeys] so a concurrent block LIST dump can + /// neither be wiped by the reload nor wipe it (#471). + Future loadForDevice(String? deviceKeyHex) => _serialize(() async { + final key = deviceKeyHex?.toLowerCase(); final normalized = (key == null || key.isEmpty) ? null : key; - final changed = normalized != _selfKeyHex; + _store.setPublicKeyHex = normalized ?? ''; _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 { + await _store.dropLegacyGlobal(); _blockedKeys ..clear() ..addAll(await _store.loadKeys()); @@ -62,8 +81,14 @@ class BlockService extends ChangeNotifier { ..clear() ..addAll(await _store.loadNames()); await _pruneExpiredNames(); + // Self-heal: never keep the connected radio's own key in its own list + // (blocking yourself silently hides your own traffic, #250). + if (normalized != null && _blockedKeys.remove(normalized)) { + await _store.saveKeys(_blockedKeys); + firmwareSync?.call(normalized, false); + } notifyListeners(); - } + }); Set get blockedKeys => Set.unmodifiable(_blockedKeys); Map get blockedNames => Map.unmodifiable(_blockedNames); @@ -74,27 +99,28 @@ class BlockService extends ChangeNotifier { bool isNameBlocked(String name) => _blockedNames.containsKey(name.trim().toLowerCase()); - Future block(String publicKeyHex) async { + Future block(String publicKeyHex) => _serialize(() async { final key = publicKeyHex.toLowerCase(); if (isSelf(key)) return; if (!_blockedKeys.add(key)) return; await _store.saveKeys(_blockedKeys); firmwareSync?.call(key, true); notifyListeners(); - } + }); - Future unblock(String publicKeyHex) async { + Future unblock(String publicKeyHex) => _serialize(() async { final key = publicKeyHex.toLowerCase(); if (!_blockedKeys.remove(key)) return; await _store.saveKeys(_blockedKeys); firmwareSync?.call(key, false); notifyListeners(); - } + }); /// Merge keys learned from the firmware block list into the local set WITHOUT /// echoing them back to the radio (used by the connect-time union pull). - /// Never removes, the union only adds. - Future importKeys(Iterable keysHex) async { + /// Never removes, the union only adds. Serialized against [loadForDevice] so + /// a reload can't wipe these imports and vice-versa (#471). + Future importKeys(Iterable keysHex) => _serialize(() async { var changed = false; for (final k in keysHex) { final key = k.toLowerCase(); @@ -106,21 +132,21 @@ class BlockService extends ChangeNotifier { if (!changed) return; await _store.saveKeys(_blockedKeys); notifyListeners(); - } + }); - Future blockName(String name) async { + Future blockName(String name) => _serialize(() async { final n = name.trim().toLowerCase(); if (n.isEmpty || _blockedNames.containsKey(n)) return; _blockedNames[n] = DateTime.now().millisecondsSinceEpoch; await _store.saveNames(_blockedNames); notifyListeners(); - } + }); - Future unblockName(String name) async { + Future unblockName(String name) => _serialize(() async { if (_blockedNames.remove(name.trim().toLowerCase()) == null) return; await _store.saveNames(_blockedNames); notifyListeners(); - } + }); /// Name-only blocks older than this are pruned on load (self-cleaning). static const Duration _nameBlockTtl = Duration(days: 30); @@ -128,24 +154,25 @@ class BlockService extends ChangeNotifier { /// Promote a name-only block to a durable pubkey block once we learn the /// identity behind it (observed via an advert or a DM). No-op if the name /// isn't name-blocked. - Future maybePromote(String name, String publicKeyHex) async { - final n = name.trim().toLowerCase(); - 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); - if (added) firmwareSync?.call(key, true); - notifyListeners(); - } + Future maybePromote(String name, String publicKeyHex) => + _serialize(() async { + final n = name.trim().toLowerCase(); + 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); + if (added) firmwareSync?.call(key, true); + notifyListeners(); + }); /// Drop name-only blocks that never linked to a pubkey within [_nameBlockTtl]. Future _pruneExpiredNames() async { diff --git a/lib/storage/block_store.dart b/lib/storage/block_store.dart index 0ca7e9d..0a37924 100644 --- a/lib/storage/block_store.dart +++ b/lib/storage/block_store.dart @@ -3,29 +3,62 @@ import 'dart:convert'; import '../utils/app_logger.dart'; import 'prefs_manager.dart'; -/// Persistence for the app-global block list. +/// Per-radio persistence for the block list. /// -/// Deliberately **global**, unlike the other stores it is NOT scoped by the -/// connected device key. A block is "I don't want to see this person," keyed by -/// their public key, and must hold regardless of which radio is connected. -/// See `docs/architecture/block-contract-as-built.md`. +/// Scoped by the connected device key (first 10 hex chars), like the app's +/// other stores. A block belongs to the radio it was set on: the app is the +/// enforcer and registry even on firmware that can't offload, and clearing a +/// radio must stay cleared. There is intentionally **no** global list; a +/// global list unioned across radios re-infects a cleared radio (#471). +/// +/// The pre-#471 build stored one unscoped global list (`block_keys_v1` / +/// `block_names_v1`). That is dropped, not migrated (owner decision, #471): +/// the feature is new and the global list was the source of the stray +/// self-block. See `docs/architecture/block-contract-as-built.md`. class BlockStore { - static const String _keysKey = 'block_keys_v1'; - static const String _namesKey = 'block_names_v1'; + static const String _keysPrefix = 'block_keys_v1'; + static const String _namesPrefix = 'block_names_v1'; + + /// First 10 hex chars of the connected device key. Empty when no radio is + /// connected; loads/saves are no-ops in that state. + String publicKeyHex = ''; + set setPublicKeyHex(String value) => + publicKeyHex = value.length > 10 ? value.substring(0, 10) : ''; + + String get _keysKey => '$_keysPrefix$publicKeyHex'; + String get _namesKey => '$_namesPrefix$publicKeyHex'; + + /// Delete the legacy unscoped global list. Idempotent, cheap after the first + /// run (the guarded reads avoid a prefs rewrite when the keys are absent). + /// Drop-and-start-fresh: the global list is not migrated onto any radio. + Future dropLegacyGlobal() async { + final prefs = PrefsManager.instance; + if (prefs.get(_keysPrefix) != null) { + appLogger.info('Dropping legacy global block key list (#471)'); + await prefs.remove(_keysPrefix); + } + if (prefs.get(_namesPrefix) != null) { + appLogger.info('Dropping legacy global block name list (#471)'); + await prefs.remove(_namesPrefix); + } + } - /// Blocked public keys (lowercased hex). + /// Blocked public keys (lowercased hex) for the current radio. Future> loadKeys() async { + if (publicKeyHex.isEmpty) return {}; final list = PrefsManager.instance.getStringList(_keysKey) ?? const []; return list.map((k) => k.toLowerCase()).toSet(); } Future saveKeys(Set keys) async { + if (publicKeyHex.isEmpty) return; await PrefsManager.instance.setStringList(_keysKey, keys.toList()); } /// Name-only blocks: lowercased claimed name -> first-blocked epoch millis. /// The timestamp feeds the promote-and-prune age-out (Epic A / A7). Future> loadNames() async { + if (publicKeyHex.isEmpty) return {}; final raw = PrefsManager.instance.getString(_namesKey); if (raw == null || raw.isEmpty) return {}; try { @@ -38,6 +71,7 @@ class BlockStore { } Future saveNames(Map names) async { + if (publicKeyHex.isEmpty) return; await PrefsManager.instance.setString(_namesKey, jsonEncode(names)); } } diff --git a/test/services/block_service_test.dart b/test/services/block_service_test.dart index 5d9194e..7f83d93 100644 --- a/test/services/block_service_test.dart +++ b/test/services/block_service_test.dart @@ -2,27 +2,53 @@ 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. +/// In-memory, scope-aware stand-in so the service can be exercised without +/// SharedPreferences. Keys/names are stored per device scope (#471). class _FakeBlockStore implements BlockStore { - Set keys = {}; - Map names = {}; + final Map> _keys = {}; + final Map> _names = {}; + bool legacyDropped = false; @override - Future> loadKeys() async => {...keys}; + String publicKeyHex = ''; @override - Future saveKeys(Set value) async => keys = {...value}; + set setPublicKeyHex(String value) => + publicKeyHex = value.length > 10 ? value.substring(0, 10) : ''; @override - Future> loadNames() async => {...names}; + Future dropLegacyGlobal() async => legacyDropped = true; + + Set keysFor(String scope) => {...?_keys[scope]}; + + @override + Future> loadKeys() async => + publicKeyHex.isEmpty ? {} : {...?_keys[publicKeyHex]}; + + @override + Future saveKeys(Set value) async { + if (publicKeyHex.isEmpty) return; + _keys[publicKeyHex] = {...value}; + } + + @override + Future> loadNames() async => + publicKeyHex.isEmpty ? {} : {...?_names[publicKeyHex]}; @override - Future saveNames(Map value) async => names = {...value}; + Future saveNames(Map value) async { + if (publicKeyHex.isEmpty) return; + _names[publicKeyHex] = {...value}; + } } void main() { - const selfKey = 'aa11bb22cc33dd44'; - const otherKey = '99ff88ee77dd66cc'; + // Radios (their own keys double as the per-radio scope + self key). + const radioA = 'a1a1a1a1a1a1a1a1'; + const radioB = 'b2b2b2b2b2b2b2b2'; + // Contacts to block. + const bob = '99ff88ee77dd66cc'; + const cara = '4444555566667777'; late _FakeBlockStore store; late BlockService service; @@ -37,118 +63,150 @@ void main() { }); group('self-block guard (#250)', () { - test('block() refuses the self key and pushes nothing', () async { - await service.setSelfKey(selfKey); + test('block() refuses the connected radio\'s own key', () async { + await service.loadForDevice(radioA); pushes.clear(); - await service.block(selfKey); + await service.block(radioA); - expect(service.isBlocked(selfKey), isFalse); - expect(store.keys, isNot(contains(selfKey))); + expect(service.isBlocked(radioA), isFalse); + expect(store.keysFor('a1a1a1a1a1'), isNot(contains(radioA))); expect(pushes, isEmpty); }); - test('block() still blocks a normal key', () async { - await service.setSelfKey(selfKey); + test('block() still blocks a normal contact', () async { + await service.loadForDevice(radioA); - await service.block(otherKey); + await service.block(bob); - expect(service.isBlocked(otherKey), isTrue); - expect(pushes, contains((key: otherKey, blocked: true))); + expect(service.isBlocked(bob), isTrue); + expect(pushes, contains((key: bob, blocked: true))); }); - test('importKeys() skips self so the radio cannot re-import it', () async { - await service.setSelfKey(selfKey); + test('importKeys() skips the self key from the radio dump', () async { + await service.loadForDevice(radioA); - await service.importKeys([selfKey, otherKey]); + await service.importKeys([radioA, bob]); - expect(service.isBlocked(selfKey), isFalse); - expect(service.isBlocked(otherKey), isTrue); + expect(service.isBlocked(radioA), isFalse); + expect(service.isBlocked(bob), 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(); + test( + 'loadForDevice heals a stale self-block already in the store', + () async { + // Radio A's stored list contains A's own key (pre-guard / stale). + store.saveKeysForTest('a1a1a1a1a1', {radioA, bob}); - await service.setSelfKey(selfKey); + await service.loadForDevice(radioA); - expect(service.isBlocked(selfKey), isFalse); - expect(store.keys, isNot(contains(selfKey))); - expect(pushes, contains((key: selfKey, blocked: false))); - }); + expect(service.isBlocked(radioA), isFalse, reason: 'self healed'); + expect(service.isBlocked(bob), isTrue); + expect(pushes, contains((key: radioA, blocked: false))); + }, + ); test('maybePromote() will not promote a name into a self-block', () async { - await service.setSelfKey(selfKey); + await service.loadForDevice(radioA); await service.blockName('me'); pushes.clear(); - await service.maybePromote('me', selfKey); + await service.maybePromote('me', radioA); - expect(service.isBlocked(selfKey), isFalse); + expect(service.isBlocked(radioA), 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); + group('per-radio isolation + drop-and-start-fresh (#471)', () { + test('a block on radio A does not appear on radio B', () async { + await service.loadForDevice(radioA); + await service.block(bob); + expect(service.isBlocked(bob), isTrue); - expect(service.isBlocked(selfKey), isFalse); + await service.loadForDevice(radioB); expect( - service.isBlocked(otherKey), - isTrue, - reason: 'only self is healed', + service.isBlocked(bob), + isFalse, + reason: 'radio B has its own list', ); - 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); + await service.block(cara); + expect(service.isBlocked(cara), isTrue); + expect(service.isBlocked(bob), isFalse); - expect(pushes, isEmpty); - expect(service.isSelf(selfKey), isTrue); + // Back to A: A still has bob, not cara. + await service.loadForDevice(radioA); + expect(service.isBlocked(bob), isTrue); + expect(service.isBlocked(cara), isFalse); }); - test('state is consistent synchronously when not awaited', () async { - await service.setSelfKey(selfKey); - await service.setSelfKey(null); - await service.block(selfKey); + test('clearing a radio stays cleared across a reconnect', () async { + await service.loadForDevice(radioA); + await service.block(bob); + await service.unblock(bob); + expect(service.isBlocked(bob), isFalse); + pushes.clear(); // ignore the legitimate ADD/REMOVE above - // 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))); + // Connect another radio, then come back to A. Nothing re-seeds bob. + await service.loadForDevice(radioB); + await service.loadForDevice(radioA); + expect( + service.isBlocked(bob), + isFalse, + reason: 'no global list to re-push the unblocked key', + ); + expect( + pushes.where((p) => p.key == bob && p.blocked), + isEmpty, + reason: 'bob is never re-ADDed to the radio', + ); }); - test('isSelf() is false when no self key is known', () { - expect(service.isSelf(selfKey), isFalse); + test('load() drops the legacy global list and starts empty', () async { + await service.load(); + expect(store.legacyDropped, isTrue); + expect(service.blockedKeys, isEmpty); }); - 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); + test('loadForDevice also drops the legacy global list', () async { + await service.loadForDevice(radioA); + expect(store.legacyDropped, isTrue); + }); - await service.block(selfKey); - expect(service.isBlocked(selfKey), isTrue); + test( + 'concurrent loadForDevice + importKeys does not wipe imports (race)', + () async { + // Fire the reload without awaiting, then import from the radio's LIST + // dump immediately after; the connector does exactly this across two + // frame handlers. Serialization must order them so neither wipes the + // other; bob (from the dump) must survive. + final load = service.loadForDevice(radioA); + final import = service.importKeys([bob]); + await Future.wait([load, import]); + + expect( + service.isBlocked(bob), + isTrue, + reason: 'imported key survived the concurrent reload', + ); + expect(store.keysFor('a1a1a1a1a1'), contains(bob)); + }, + ); + + test('disconnect (null device) clears the in-memory set', () async { + await service.loadForDevice(radioA); + await service.block(bob); + expect(service.isBlocked(bob), isTrue); + + await service.loadForDevice(null); + expect(service.blockedKeys, isEmpty); + expect(service.isSelf(radioA), isFalse); }); }); } + +extension on _FakeBlockStore { + void saveKeysForTest(String scope, Set keys) => _keys[scope] = keys; +}