From 2746a194186224a3146674aa56302d92c2f8d2d0 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 20 Aug 2026 19:49:12 -0300 Subject: [PATCH 1/5] fix: verify order book events before admitting them --- lib/data/models/nostr_event.dart | 5 +- .../repositories/open_orders_repository.dart | 35 ++- .../models/nostr_event_extensions_test.dart | 9 +- .../open_orders_order_event_test.dart | 215 ++++++++++++++++++ 4 files changed, 259 insertions(+), 5 deletions(-) create mode 100644 test/data/repositories/open_orders_order_event_test.dart diff --git a/lib/data/models/nostr_event.dart b/lib/data/models/nostr_event.dart index 0a5fb715..1439ee0c 100644 --- a/lib/data/models/nostr_event.dart +++ b/lib/data/models/nostr_event.dart @@ -42,7 +42,10 @@ extension NostrEventExtensions on NostrEvent { DateTime get expirationDate => _getTimeStamp(_getTagValue('expiration')!); String? get expiresAt => _getTagValue('expires_at'); String? get platform => _getTagValue('y'); - String get type => _getTagValue('z')!; + /// The NIP-69 `z` tag. Nullable on purpose: this is read off relay-supplied + /// events before anything about them has been established, so a missing tag + /// has to be something the intake can reject rather than crash on. + String? get type => _getTagValue('z'); String? _getTagValue(String key) { final tag = tags?.firstWhere((t) => t[0] == key, orElse: () => []); diff --git a/lib/data/repositories/open_orders_repository.dart b/lib/data/repositories/open_orders_repository.dart index 089ce2eb..1a99927f 100644 --- a/lib/data/repositories/open_orders_repository.dart +++ b/lib/data/repositories/open_orders_repository.dart @@ -98,8 +98,39 @@ class OpenOrdersRepository implements OrderRepository { ); _subscription = _nostrService.subscribeToEvents(request).listen((event) { - if (event.type == 'order') { - _events[event.orderId!] = event; + if (event.kind == orderEventKind && + event.pubkey == _settings.mostroPublicKey) { + // The filter above states what was asked for, never what arrives. The + // pinned dart_nostr fork parses relay EVENT frames straight into a + // NostrEvent: it neither verifies the signature nor matches the frame + // against the subscription's filter. So `authors` constrains the + // request, and the author field on the way back is the relay's claim — + // the signature is what turns it into the node's. + // + // The order book is what this feeds: amounts, premiums and the maker + // rating a user reads before deciding to trade. + if (!NostrUtils.isValidEventSignature(event)) { + logger.w( + 'Rejecting kind-$orderEventKind order event claiming to be from ' + '${event.pubkey}: signature verification failed', + ); + return; + } + + // Both tags are read off the event itself — one to key the cache, one + // to classify it — so neither can be taken for granted, and a missing + // one used to throw inside this callback rather than be rejected. + final orderId = event.orderId; + if (orderId == null || event.type != 'order') { + logger.w( + 'Ignoring kind-$orderEventKind event ${event.id} from ' + '${event.pubkey}: expected a d tag and z=order, got d=$orderId ' + 'and z=${event.type}', + ); + return; + } + + _events[orderId] = event; _eventStreamController.add(_events.values.toList()); } else if (event.kind == infoEventKind && event.pubkey == _settings.mostroPublicKey) { diff --git a/test/data/models/nostr_event_extensions_test.dart b/test/data/models/nostr_event_extensions_test.dart index 3dcbb548..5fa45d77 100644 --- a/test/data/models/nostr_event_extensions_test.dart +++ b/test/data/models/nostr_event_extensions_test.dart @@ -163,8 +163,13 @@ void main() { expect(() => orderEvent(tags: const []).status, throwsA(anything)); }); - test('throws when reading a type tag that is absent', () { - expect(() => orderEvent(tags: const []).type, throwsA(anything)); + test('reads an absent type tag as null rather than throwing', () { + // `type` is read off relay-supplied events at the order-book intake, + // before anything about them has been established. Throwing there + // raised an uncaught async error from inside the stream callback on + // any event a relay chose to send without a z tag; the intake now + // rejects it instead. + expect(orderEvent(tags: const []).type, isNull); }); }); diff --git a/test/data/repositories/open_orders_order_event_test.dart b/test/data/repositories/open_orders_order_event_test.dart new file mode 100644 index 00000000..460dd63e --- /dev/null +++ b/test/data/repositories/open_orders_order_event_test.dart @@ -0,0 +1,215 @@ +import 'dart:async'; + +import 'package:dart_nostr/dart_nostr.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mockito/mockito.dart'; +import 'package:mostro_mobile/data/models/nostr_event.dart'; +import 'package:mostro_mobile/data/repositories/open_orders_repository.dart'; +import 'package:mostro_mobile/features/settings/settings.dart'; +import 'package:mostro_mobile/shared/utils/nostr_utils.dart'; + +import '../../mocks.mocks.dart'; + +/// Builds a kind-38383 order event of the shape mostrod publishes: empty +/// content, everything carried in tags (NIP-69). +NostrEvent _signedOrderEvent( + NostrKeyPairs keyPair, { + String orderId = 'order-1', + String amount = '50000', + String rating = '5', + List>? tags, +}) { + return NostrEvent.fromPartialData( + kind: 38383, + content: '', + keyPairs: keyPair, + tags: tags ?? + [ + ['d', orderId], + ['k', 'sell'], + ['f', 'USD'], + ['s', 'pending'], + ['amt', amount], + ['rating', rating], + ['y', 'mostro'], + ['z', 'order'], + ], + ); +} + +/// Re-tags [source] while keeping its original id/sig/pubkey — what a relay +/// can do for free, and what `NostrEvent.isVerified()` fails to catch because +/// it never recomputes the id from the serialized event. +NostrEvent _reTagged(NostrEvent source, List> tags) { + return NostrEvent( + id: source.id, + sig: source.sig, + pubkey: source.pubkey, + kind: source.kind, + content: source.content, + createdAt: source.createdAt, + tags: tags, + ); +} + +void main() { + late MockNostrService mockNostrService; + late StreamController eventController; + late NostrKeyPairs nodeKeys; + late Settings settings; + + setUp(() { + nodeKeys = NostrUtils.generateKeyPair(); + mockNostrService = MockNostrService(); + eventController = StreamController.broadcast(); + + settings = Settings( + relays: const ['wss://relay.example'], + fullPrivacyMode: false, + mostroPublicKey: nodeKeys.public, + ); + + when(mockNostrService.isInitialized).thenReturn(true); + when(mockNostrService.subscribeToEvents(any)) + .thenAnswer((_) => eventController.stream); + }); + + tearDown(() async { + await eventController.close(); + }); + + OpenOrdersRepository buildRepository() => + OpenOrdersRepository(mockNostrService, settings); + + // The subscription filter pins `authors` and `kinds`, but the pinned + // dart_nostr fork forwards relay EVENT frames without verifying them or + // matching them against the filter. Everything below is therefore reachable + // by any relay the app is connected to. + group('OpenOrdersRepository order event (kind 38383) verification', () { + test('accepts a genuinely signed order from the configured node', () async { + final repository = buildRepository(); + final event = _signedOrderEvent(nodeKeys); + + eventController.add(event); + await pumpEventQueue(); + + expect(await repository.getOrderById('order-1'), isNotNull); + }); + + test('rejects an order authored by anyone else', () async { + final impostor = NostrUtils.generateKeyPair(); + final repository = buildRepository(); + + eventController.add(_signedOrderEvent(impostor)); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + + test('rejects an order whose tags were rewritten under a genuine id', + () async { + final repository = buildRepository(); + final genuine = _signedOrderEvent(nodeKeys, amount: '50000', rating: '1'); + + // The amount and the maker's rating are exactly what a user reads before + // deciding to trade, and re-tagging costs the relay nothing. + final tampered = _reTagged(genuine, [ + ['d', 'order-1'], + ['k', 'sell'], + ['f', 'USD'], + ['s', 'pending'], + ['amt', '1'], + ['rating', '5'], + ['y', 'mostro'], + ['z', 'order'], + ]); + + eventController.add(tampered); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + + test('a tampered copy cannot displace an order already accepted', () async { + final repository = buildRepository(); + final genuine = _signedOrderEvent(nodeKeys, amount: '50000'); + + eventController.add(genuine); + await pumpEventQueue(); + + eventController.add(_reTagged(genuine, [ + ['d', 'order-1'], + ['z', 'order'], + ['amt', '1'], + ])); + await pumpEventQueue(); + + final stored = await repository.getOrderById('order-1'); + expect(stored!.amount, '50000'); + }); + + test('ignores an event that carries no d tag instead of throwing', + () async { + final repository = buildRepository(); + final event = _signedOrderEvent(nodeKeys, tags: [ + ['k', 'sell'], + ['z', 'order'], + ]); + + eventController.add(event); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + + test('ignores an event that carries no z tag instead of throwing', + () async { + // This used to throw inside the stream callback: `type` bang-asserted + // the tag, and an event without one reached it before any other check. + final repository = buildRepository(); + final event = _signedOrderEvent(nodeKeys, tags: [ + ['d', 'order-1'], + ['k', 'sell'], + ]); + + eventController.add(event); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + + test('ignores a non-order z value on the order kind', () async { + final repository = buildRepository(); + final event = _signedOrderEvent(nodeKeys, tags: [ + ['d', 'order-1'], + ['z', 'something-else'], + ]); + + eventController.add(event); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + + test('ignores z=order carried on a kind the order book does not serve', + () async { + // The z tag alone used to be the whole admission test, with no kind + // check in front of it. + final repository = buildRepository(); + final event = NostrEvent.fromPartialData( + kind: 1, + content: 'not an order', + keyPairs: nodeKeys, + tags: [ + ['d', 'order-1'], + ['z', 'order'], + ], + ); + + eventController.add(event); + await pumpEventQueue(); + + expect(await repository.getAllOrders(), isEmpty); + }); + }); +} From a2f5c95d5fbfb546b5b0dba42fdb3000a1583322 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 20 Aug 2026 19:55:18 -0300 Subject: [PATCH 2/5] fix: verify relay list events and bound the relays one carries --- lib/core/models/relay_list_event.dart | 52 +++++++++- test/features/relays/relay_model_test.dart | 107 +++++++++++++++++++-- 2 files changed, 148 insertions(+), 11 deletions(-) diff --git a/lib/core/models/relay_list_event.dart b/lib/core/models/relay_list_event.dart index 6a248ad3..bd8c4443 100644 --- a/lib/core/models/relay_list_event.dart +++ b/lib/core/models/relay_list_event.dart @@ -1,4 +1,7 @@ import 'package:dart_nostr/dart_nostr.dart'; +import 'package:mostro_mobile/core/test_environment.dart'; +import 'package:mostro_mobile/services/logger_service.dart'; +import 'package:mostro_mobile/shared/utils/nostr_utils.dart'; /// Matches every trailing slash so `wss://relay.example//` normalizes the same /// way as `wss://relay.example/`. @@ -17,19 +20,54 @@ class RelayListEvent { required this.authorPubkey, }); + /// Upper bound on the relays one event may contribute. + /// + /// A node's NIP-65 list is a handful of entries. The number is a sanity + /// bound on an attacker-controlled list, not a policy: without one, a single + /// event can name as many relays as it likes and every one of them joins the + /// active set and receives every subscription. + static const int maxRelays = 50; + /// Parses a kind 10002 Nostr event into a RelayListEvent. - /// Returns null if the event is not a valid kind 10002 event. + /// + /// Returns null unless the event is a kind-10002 event that the author it + /// claims actually signed. This is the only way a [RelayListEvent] is + /// produced, so the check lives here rather than at each call site: what the + /// list names becomes the app's relay set, which is the position every other + /// relay-sourced attack is launched from. The author field on its own is a + /// relay's claim about an event it chose to hand over. + /// + /// Verifying here also makes [publishedAt] mean something: the freshness + /// comparison downstream is against `created_at`, which is covered by the + /// signature and the recomputed id, so a relay cannot backdate or + /// future-date a list to control which one wins. static RelayListEvent? fromEvent(NostrEvent event) { if (event.kind != 10002) return null; + if (!NostrUtils.isValidEventSignature(event)) { + logger.w( + 'Rejecting kind-10002 relay list claiming to be from ${event.pubkey}: ' + 'signature verification failed', + ); + return null; + } + // Extract relay URLs from 'r' tags - final relays = event.tags + var relays = event.tags ?.where((tag) => tag.isNotEmpty && tag[0] == 'r') .where((tag) => tag.length >= 2) .map((tag) => tag[1]) .where((url) => url.isNotEmpty) .toList() ?? []; + if (relays.length > maxRelays) { + logger.w( + 'Relay list from ${event.pubkey} names ${relays.length} relays; ' + 'keeping the first $maxRelays', + ); + relays = relays.sublist(0, maxRelays); + } + // Handle different possible types for createdAt DateTime publishedAt; if (event.createdAt is DateTime) { @@ -49,9 +87,17 @@ class RelayListEvent { /// Validates that all relay URLs are properly formatted WebSocket URLs /// Also normalizes URLs by removing trailing slashes to prevent duplicates + /// + /// Cleartext `ws://` is refused on the same terms as the manual-entry path: + /// only inside the Mortsom test environment, where the relay is a local + /// process on a private address. Accepting it here was a way around that + /// rule — a node's list could downgrade the app's transport to plaintext + /// without the user ever typing a URL. List get validRelays { return relays - .where((url) => url.startsWith('wss://') || url.startsWith('ws://')) + .where((url) => + url.startsWith('wss://') || + (TestEnvironment.allowInsecureRelays && url.startsWith('ws://'))) .map((url) => url.trim()) .map((url) => url.replaceAll(_trailingSlashes, '')) .toList(); diff --git a/test/features/relays/relay_model_test.dart b/test/features/relays/relay_model_test.dart index bc08f27e..64969b32 100644 --- a/test/features/relays/relay_model_test.dart +++ b/test/features/relays/relay_model_test.dart @@ -2,22 +2,25 @@ import 'package:dart_nostr/dart_nostr.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:mostro_mobile/core/models/relay_list_event.dart'; import 'package:mostro_mobile/features/relays/relay.dart'; +import 'package:mostro_mobile/shared/utils/nostr_utils.dart'; final _publishedAt = DateTime.utc(2026, 1, 1); -final _authorPubkey = 'a' * 64; +final _nodeKeys = NostrUtils.generateKeyPair(); +final _authorPubkey = _nodeKeys.public; +/// Signed for real: `fromEvent` verifies the signature before it will hand +/// back a relay list, so an event assembled with a placeholder id and sig no +/// longer stands in for one the node published. NostrEvent relayListEvent({ int kind = 10002, List>? tags, DateTime? createdAt, - String? pubkey, + NostrKeyPairs? keyPair, }) => - NostrEvent( - id: 'event-id', + NostrEvent.fromPartialData( kind: kind, content: '', - sig: 'sig', - pubkey: pubkey ?? _authorPubkey, + keyPairs: keyPair ?? _nodeKeys, createdAt: createdAt ?? _publishedAt, tags: tags ?? const [ @@ -26,6 +29,18 @@ NostrEvent relayListEvent({ ], ); +/// Re-tags [source] while keeping its id, sig and pubkey — free for a relay, +/// and the case `isVerified()` misses because it never recomputes the id. +NostrEvent reTagged(NostrEvent source, List> tags) => NostrEvent( + id: source.id, + sig: source.sig, + pubkey: source.pubkey, + kind: source.kind, + content: source.content, + createdAt: source.createdAt, + tags: tags, + ); + RelayListEvent relayList(List relays, {String author = 'author'}) => RelayListEvent( relays: relays, @@ -235,10 +250,82 @@ void main() { expect(parsed!.relays, isEmpty); expect(parsed.validRelays, isEmpty); }); + + // What this list names becomes the app's relay set, and that set is the + // position every other relay-sourced attack is launched from. The author + // field is the relay's claim about an event it chose to serve. + test('returns null when the signature does not verify', () { + final genuine = relayListEvent(); + final tampered = reTagged(genuine, const [ + ['r', 'wss://attacker.relay'], + ]); + + expect(RelayListEvent.fromEvent(tampered), isNull); + }); + + test('returns null for an event assembled without a real signature', () { + final unsigned = NostrEvent( + id: 'event-id', + sig: 'sig', + pubkey: _authorPubkey, + kind: 10002, + content: '', + createdAt: _publishedAt, + tags: const [ + ['r', 'wss://attacker.relay'], + ], + ); + + expect(RelayListEvent.fromEvent(unsigned), isNull); + }); + + test('accepts a list signed by any author — the caller pins which one', () { + // fromEvent establishes that the claimed author really signed it; + // deciding whether that author is the configured node is the relay + // notifier's job, and it still compares authorPubkey. + final otherNode = NostrUtils.generateKeyPair(); + final parsed = + RelayListEvent.fromEvent(relayListEvent(keyPair: otherNode)); + + expect(parsed, isNotNull); + expect(parsed!.authorPubkey, otherNode.public); + }); + + test('bounds how many relays one event can contribute', () { + final tags = List.generate( + RelayListEvent.maxRelays + 10, + (i) => ['r', 'wss://relay-$i.example'], + ); + + final parsed = RelayListEvent.fromEvent(relayListEvent(tags: tags)); + + expect(parsed!.relays, hasLength(RelayListEvent.maxRelays)); + expect(parsed.relays.first, 'wss://relay-0.example'); + }); + + test('leaves a list at the bound untouched', () { + final tags = List.generate( + RelayListEvent.maxRelays, + (i) => ['r', 'wss://relay-$i.example'], + ); + + final parsed = RelayListEvent.fromEvent(relayListEvent(tags: tags)); + + expect(parsed!.relays, hasLength(RelayListEvent.maxRelays)); + }); + + test('drops cleartext ws:// from a signed list', () { + final parsed = RelayListEvent.fromEvent(relayListEvent(tags: const [ + ['r', 'wss://secure.relay'], + ['r', 'ws://plain.relay'], + ])); + + expect(parsed!.validRelays, ['wss://secure.relay']); + }); }); group('RelayListEvent.validRelays', () { - test('keeps only websocket urls', () { + test('keeps only secure websocket urls', () { final event = relayList([ 'wss://secure.relay', 'ws://plain.relay', @@ -246,7 +333,11 @@ void main() { 'relay.example', ]); - expect(event.validRelays, ['wss://secure.relay', 'ws://plain.relay']); + // Cleartext ws:// is refused on the same terms as the manual-entry + // path, which allows it only inside the Mortsom test environment. + // Accepting it here let a node's list downgrade the transport to + // plaintext without the user typing a URL. + expect(event.validRelays, ['wss://secure.relay']); }); test('strips a single trailing slash', () { From 2f0674e0f70d23edc678ce6d8509fb72d30bb0f1 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 20 Aug 2026 20:00:09 -0300 Subject: [PATCH 3/5] fix: verify NWC events and ignore unusable responses --- lib/services/nwc/nwc_client.dart | 56 ++++++-- test/services/nwc/nwc_client_intake_test.dart | 121 ++++++++++++++++++ 2 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 test/services/nwc/nwc_client_intake_test.dart diff --git a/lib/services/nwc/nwc_client.dart b/lib/services/nwc/nwc_client.dart index 2f3ca6c0..21131095 100644 --- a/lib/services/nwc/nwc_client.dart +++ b/lib/services/nwc/nwc_client.dart @@ -1,5 +1,6 @@ import 'dart:async'; import 'package:dart_nostr/dart_nostr.dart'; +import 'package:flutter/foundation.dart'; import 'package:mostro_mobile/services/logger_service.dart'; import 'package:mostro_mobile/services/nostr_service.dart'; import 'package:mostro_mobile/services/nwc/nwc_connection.dart'; @@ -139,6 +140,30 @@ class NwcClient { } } + /// Whether [event] is a kind-[kind] event the wallet itself signed. + /// + /// Every NWC subscription names a kind and an author in its filter, but the + /// pinned dart_nostr fork parses relay EVENT frames straight into a + /// NostrEvent: it neither verifies the signature nor matches the frame + /// against the filter that was sent. Both are therefore a statement of what + /// was asked for, and the author on the way back is the relay's claim. + /// + /// The relay here is the one named in the user's own wallet URI — precisely + /// the party NIP-47's encryption says you should not have to trust. + @visibleForTesting + bool isFromWallet(NostrEvent event, int kind) { + if (event.kind != kind) return false; + if (event.pubkey != connection.walletPubkey) return false; + if (!NostrUtils.isValidEventSignature(event)) { + logger.w( + 'NWC: ignoring kind-$kind event ${event.id} claiming to be from the ' + 'wallet: signature verification failed', + ); + return false; + } + return true; + } + /// Detects the wallet's supported encryption mode from its info event. Future _detectEncryptionMode() async { try { @@ -159,6 +184,8 @@ class NwcClient { ); final subscription = stream.stream.listen((event) { + if (!isFromWallet(event, 13194)) return; + // Look for the encryption tag final encTag = event.tags?.firstWhere( (t) => t.isNotEmpty && t[0] == 'encryption', @@ -218,6 +245,8 @@ class NwcClient { final subscription = stream.stream.listen((event) async { try { + if (!isFromWallet(event, 23196)) return; + // Verify the notification is addressed to us via 'p' tag final pTag = event.tags?.firstWhere( (t) => t.isNotEmpty && t[0] == 'p', @@ -420,8 +449,7 @@ class NwcClient { final subscription = stream.stream.listen((event) async { try { - logger.d( - 'NWC: Received event kind=${event.kind} from ${event.pubkey.substring(0, 8)}...'); + if (!isFromWallet(event, 23195)) return; // Verify this response references our request via 'e' tag final eTag = event.tags?.firstWhere( @@ -435,13 +463,19 @@ class NwcClient { return; // Not our response } + final content = event.content; + if (content == null || content.isEmpty) { + logger.w('NWC: ignoring response ${event.id} with no content'); + return; + } + logger.d('NWC: Response matched for ${request.method}!'); // Decrypt the response using the detected encryption mode. // Auto-detect from content format as a safety fallback. - final responseMode = NwcCrypto.detectFromContent(event.content!); + final responseMode = NwcCrypto.detectFromContent(content); final decrypted = await NwcCrypto.decrypt( - event.content!, + content, connection.secret, connection.walletPubkey, responseMode, @@ -453,11 +487,15 @@ class NwcClient { completer.complete(response); } } catch (e) { - if (!completer.isCompleted) { - completer.completeError( - NwcException('Failed to process response: $e'), - ); - } + // Ignore the event; never fail the request on one. + // + // The request id travels in the clear on the relay carrying it, so + // addressing a reply to this request costs an attacker nothing. + // Failing here surfaced "payment failed" before the wallet's real + // response had a chance to arrive, and it is the user's retry that + // costs money. The genuine response is still coming; only the + // timeout ends this wait unsuccessfully. + logger.w('NWC: ignoring unusable response event ${event.id}: $e'); } }); diff --git a/test/services/nwc/nwc_client_intake_test.dart b/test/services/nwc/nwc_client_intake_test.dart new file mode 100644 index 00000000..79849054 --- /dev/null +++ b/test/services/nwc/nwc_client_intake_test.dart @@ -0,0 +1,121 @@ +import 'package:dart_nostr/dart_nostr.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mostro_mobile/services/nwc/nwc_client.dart'; +import 'package:mostro_mobile/services/nwc/nwc_connection.dart'; +import 'package:mostro_mobile/shared/utils/nostr_utils.dart'; + +import '../../mocks.mocks.dart'; + +/// Re-tags [source] while keeping its id, sig and pubkey — free for a relay, +/// and the case `isVerified()` misses because it never recomputes the id. +NostrEvent _reTagged(NostrEvent source, List> tags) => NostrEvent( + id: source.id, + sig: source.sig, + pubkey: source.pubkey, + kind: source.kind, + content: source.content, + createdAt: source.createdAt, + tags: tags, + ); + +void main() { + late NostrKeyPairs walletKeys; + late NwcClient client; + + NostrEvent walletEvent({ + int kind = 23195, + String content = 'ciphertext', + List> tags = const [], + NostrKeyPairs? keyPair, + }) => + NostrEvent.fromPartialData( + kind: kind, + content: content, + keyPairs: keyPair ?? walletKeys, + tags: tags, + ); + + setUp(() { + walletKeys = NostrUtils.generateKeyPair(); + client = NwcClient( + connection: NwcConnection( + walletPubkey: walletKeys.public, + relayUrls: const ['wss://wallet.relay.example'], + secret: NostrUtils.generateKeyPair().private, + ), + nostrService: MockNostrService(), + ); + }); + + // Every NWC subscription pins kind and author in its filter, but the pinned + // dart_nostr fork forwards relay EVENT frames without verifying them or + // matching them against the filter. The relay on this path is the one named + // in the user's own wallet URI. + group('NwcClient.isFromWallet', () { + test('accepts an event the wallet actually signed', () { + expect(client.isFromWallet(walletEvent(), 23195), isTrue); + }); + + test('rejects an event of a different kind', () { + expect(client.isFromWallet(walletEvent(kind: 23196), 23195), isFalse); + }); + + test('rejects an event authored by anyone else', () { + final impostor = NostrUtils.generateKeyPair(); + + expect( + client.isFromWallet(walletEvent(keyPair: impostor), 23195), + isFalse, + ); + }); + + test('rejects an event whose tags were rewritten under a genuine id', () { + final genuine = walletEvent(tags: const [ + ['e', 'request-id'], + ]); + final tampered = _reTagged(genuine, const [ + ['e', 'a-different-request'], + ]); + + expect(client.isFromWallet(tampered, 23195), isFalse); + }); + + test('rejects an event assembled with a placeholder signature', () { + // The shape a relay injects: it only has to claim the wallet's pubkey + // and name the request, both of which are public. + final forged = NostrEvent( + id: 'a' * 64, + sig: 'b' * 128, + pubkey: walletKeys.public, + kind: 23195, + content: 'garbage', + createdAt: DateTime.now(), + tags: const [ + ['e', 'request-id'], + ], + ); + + expect(client.isFromWallet(forged, 23195), isFalse); + }); + + test('applies the same rule to the info event kind', () { + final impostor = NostrUtils.generateKeyPair(); + + expect(client.isFromWallet(walletEvent(kind: 13194), 13194), isTrue); + expect( + client.isFromWallet(walletEvent(kind: 13194, keyPair: impostor), 13194), + isFalse, + ); + }); + + test('applies the same rule to the notification kind', () { + final impostor = NostrUtils.generateKeyPair(); + + expect(client.isFromWallet(walletEvent(kind: 23196), 23196), isTrue); + expect( + client.isFromWallet(walletEvent(kind: 23196, keyPair: impostor), 23196), + isFalse, + ); + }); + }); +} From d3bc9b2d64d7438f6c82ccb47881df806c5815b2 Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 20 Aug 2026 20:03:36 -0300 Subject: [PATCH 4/5] fix: fetch from link-supplied relays on a scoped connection --- lib/services/nostr_service.dart | 77 ++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/lib/services/nostr_service.dart b/lib/services/nostr_service.dart index c556fcfa..b2e67059 100644 --- a/lib/services/nostr_service.dart +++ b/lib/services/nostr_service.dart @@ -496,52 +496,57 @@ class NostrService { } } - /// Fetches events from specific relays temporarily + /// Fetches events from [relays] on a connection of their own, leaving the + /// app's relay pool untouched. + /// + /// These URLs come from outside the user's configuration: a `mostro:` deep + /// link supplies them, so whoever gets a link tapped picks them. They used + /// to be merged into `settings.relays` and connected through [updateSettings], + /// with a restore afterwards that could not work — it passed the `settings` + /// getter, already replaced by then, so the relay comparison was the + /// temporary list against itself and skipped as unchanged. + /// + /// Restoring the settings correctly would not have been enough either. + /// dart_nostr's `init` is additive and the fork exposes no per-relay + /// disconnect, so once the app connects to a relay it stays connected for + /// the process lifetime — receiving every subscription filter the app + /// issues, every trade pubkey among them, and able to feed the app's + /// intakes. Nothing short of not connecting it to the shared pool works. + /// + /// The scoped instance is closed when the fetch ends, on every path. Same + /// approach the relay connectivity test uses. Future> _fetchFromSpecificRelays( NostrFilter filter, List relays, ) async { - try { - // Store current relays - final originalRelays = List.from(settings.relays); - - // Temporarily add specific relays if not already present - final allRelays = {...originalRelays, ...relays}.toList(); - - if (!ListEquality().equals(originalRelays, allRelays)) { - logger.i('Temporarily connecting to additional relays: $relays'); - - // Update settings with additional relays - final tempSettings = Settings( - relays: allRelays, - mostroPublicKey: settings.mostroPublicKey, - fullPrivacyMode: settings.fullPrivacyMode, - defaultFiatCode: settings.defaultFiatCode, - selectedLanguage: settings.selectedLanguage, - ); - - await updateSettings(tempSettings); - - // Fetch the events - final events = await fetchEvents(filter); + logger.i('Fetching from link-supplied relays on a scoped connection: ' + '$relays'); - // Restore original relays - await updateSettings(settings); + final scoped = Nostr(); + try { + await scoped.services.relays.init( + relaysUrl: relays, + connectionTimeout: Config.relayConnectionTimeout, + shouldReconnectToRelayOnNotice: false, + retryOnClose: false, + retryOnError: false, + ); - return events; - } else { - // No new relays to add, use normal fetch - return await fetchEvents(filter); - } + return await scoped.services.relays.startEventsSubscriptionAsync( + request: NostrRequest(filters: [filter]), + timeout: Config.nostrOperationTimeout, + ); } catch (e) { logger.e('Error fetching from specific relays: $e'); - // Ensure we restore original settings even on error + rethrow; + } finally { try { - await updateSettings(settings); - } catch (restoreError) { - logger.e('Failed to restore original relay settings: $restoreError'); + await scoped.services.relays.disconnectFromRelays(); + } catch (e) { + // The fetch already returned; a connection left open here would still + // be on the scoped instance, never on the app's pool. + logger.w('Failed to close the scoped relay connection: $e'); } - rethrow; } } } From 457347a4f4261e34e6a7753f9a92c8a4924424ce Mon Sep 17 00:00:00 2001 From: Andrea Diaz Correia Date: Thu, 20 Aug 2026 20:09:36 -0300 Subject: [PATCH 5/5] test: cover the NWC response admission rule --- lib/services/nwc/nwc_client.dart | 115 ++++++------ test/services/nwc/nwc_client_intake_test.dart | 170 +++++++++++++++++- 2 files changed, 233 insertions(+), 52 deletions(-) diff --git a/lib/services/nwc/nwc_client.dart b/lib/services/nwc/nwc_client.dart index 21131095..0c11ccb3 100644 --- a/lib/services/nwc/nwc_client.dart +++ b/lib/services/nwc/nwc_client.dart @@ -140,6 +140,67 @@ class NwcClient { } } + /// Decides what a single kind-23195 event means for the request [requestId] + /// is waiting on, completing [completer] only for a response that is + /// genuinely the wallet's and genuinely readable. + /// + /// Extracted from the subscription callback so the rule can be exercised + /// directly: the client's [Nostr] instance is built in place, so there is no + /// other way to drive an event through this path. + @visibleForTesting + Future handleResponseEvent( + NostrEvent event, + String requestId, + Completer completer, + ) async { + try { + if (!isFromWallet(event, 23195)) return; + + // Verify this response references our request via 'e' tag + final eTag = event.tags?.firstWhere( + (t) => t.isNotEmpty && t[0] == 'e', + orElse: () => [], + ); + + if (eTag == null || eTag.length < 2 || eTag[1] != requestId) { + logger.d('NWC: Ignoring event — e tag mismatch'); + return; // Not our response + } + + final content = event.content; + if (content == null || content.isEmpty) { + logger.w('NWC: ignoring response ${event.id} with no content'); + return; + } + + // Decrypt the response using the detected encryption mode. + // Auto-detect from content format as a safety fallback. + final responseMode = NwcCrypto.detectFromContent(content); + final decrypted = await NwcCrypto.decrypt( + content, + connection.secret, + connection.walletPubkey, + responseMode, + ); + + final response = NwcResponse.fromJson(decrypted); + + if (!completer.isCompleted) { + completer.complete(response); + } + } catch (e) { + // Ignore the event; never fail the request on one. + // + // The request id travels in the clear on the relay carrying it, so + // addressing a reply to this request costs an attacker nothing. Failing + // here surfaced "payment failed" before the wallet's real response had + // a chance to arrive, and it is the user's retry that costs money. The + // genuine response is still coming; only the timeout ends this wait + // unsuccessfully. + logger.w('NWC: ignoring unusable response event ${event.id}: $e'); + } + } + /// Whether [event] is a kind-[kind] event the wallet itself signed. /// /// Every NWC subscription names a kind and an author in its filter, but the @@ -447,57 +508,9 @@ class NwcClient { ), ); - final subscription = stream.stream.listen((event) async { - try { - if (!isFromWallet(event, 23195)) return; - - // Verify this response references our request via 'e' tag - final eTag = event.tags?.firstWhere( - (t) => t.isNotEmpty && t[0] == 'e', - orElse: () => [], - ); - - if (eTag == null || eTag.length < 2 || eTag[1] != requestId) { - logger.d( - 'NWC: Ignoring event — e tag mismatch (expected: ${requestId.substring(0, 8)}..., got: ${eTag != null && eTag.length >= 2 ? eTag[1].substring(0, 8) : "none"}...)'); - return; // Not our response - } - - final content = event.content; - if (content == null || content.isEmpty) { - logger.w('NWC: ignoring response ${event.id} with no content'); - return; - } - - logger.d('NWC: Response matched for ${request.method}!'); - - // Decrypt the response using the detected encryption mode. - // Auto-detect from content format as a safety fallback. - final responseMode = NwcCrypto.detectFromContent(content); - final decrypted = await NwcCrypto.decrypt( - content, - connection.secret, - connection.walletPubkey, - responseMode, - ); - - final response = NwcResponse.fromJson(decrypted); - - if (!completer.isCompleted) { - completer.complete(response); - } - } catch (e) { - // Ignore the event; never fail the request on one. - // - // The request id travels in the clear on the relay carrying it, so - // addressing a reply to this request costs an attacker nothing. - // Failing here surfaced "payment failed" before the wallet's real - // response had a chance to arrive, and it is the user's retry that - // costs money. The genuine response is still coming; only the - // timeout ends this wait unsuccessfully. - logger.w('NWC: ignoring unusable response event ${event.id}: $e'); - } - }); + final subscription = stream.stream.listen( + (event) => handleResponseEvent(event, requestId, completer), + ); _subscriptions[subId] = subscription; diff --git a/test/services/nwc/nwc_client_intake_test.dart b/test/services/nwc/nwc_client_intake_test.dart index 79849054..75936238 100644 --- a/test/services/nwc/nwc_client_intake_test.dart +++ b/test/services/nwc/nwc_client_intake_test.dart @@ -1,7 +1,12 @@ +import 'dart:async'; +import 'dart:convert'; + import 'package:dart_nostr/dart_nostr.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:mostro_mobile/services/nwc/nwc_client.dart'; import 'package:mostro_mobile/services/nwc/nwc_connection.dart'; +import 'package:mostro_mobile/services/nwc/nwc_crypto.dart'; +import 'package:mostro_mobile/services/nwc/nwc_models.dart'; import 'package:mostro_mobile/shared/utils/nostr_utils.dart'; import '../../mocks.mocks.dart'; @@ -20,6 +25,7 @@ NostrEvent _reTagged(NostrEvent source, List> tags) => NostrEvent( void main() { late NostrKeyPairs walletKeys; + late NostrKeyPairs clientKeys; late NwcClient client; NostrEvent walletEvent({ @@ -37,11 +43,12 @@ void main() { setUp(() { walletKeys = NostrUtils.generateKeyPair(); + clientKeys = NostrUtils.generateKeyPair(); client = NwcClient( connection: NwcConnection( walletPubkey: walletKeys.public, relayUrls: const ['wss://wallet.relay.example'], - secret: NostrUtils.generateKeyPair().private, + secret: clientKeys.private, ), nostrService: MockNostrService(), ); @@ -118,4 +125,165 @@ void main() { ); }); }); + + group('NwcClient.handleResponseEvent', () { + const requestId = 'aaaabbbbccccddddeeeeffff00001111'; + + /// A response the wallet really sent: NIP-44 to the client's key, signed + /// by the wallet, tagged with the request it answers. + Future genuineResponse({ + String resultType = 'pay_invoice', + Map result = const {'preimage': 'deadbeef'}, + String eTag = requestId, + }) async { + final payload = jsonEncode({ + 'result_type': resultType, + 'result': result, + }); + final encrypted = await NwcCrypto.encrypt( + payload, + walletKeys.private, + clientKeys.public, + NwcEncryption.nip44, + ); + return NostrEvent.fromPartialData( + kind: 23195, + content: encrypted, + keyPairs: walletKeys, + tags: [ + ['e', eTag], + ['p', clientKeys.public], + ], + ); + } + + test('completes the request with a genuine wallet response', () async { + final completer = Completer(); + + await client.handleResponseEvent( + await genuineResponse(), requestId, completer); + + expect(completer.isCompleted, isTrue); + final response = await completer.future; + expect(response.isSuccess, isTrue); + expect(response.result!['preimage'], 'deadbeef'); + }); + + // The core of the finding. The request id travels in the clear on the + // relay carrying it, so anything reaching this stream can address a reply + // to this request. Failing here reported "payment failed" before the + // wallet's real response arrived, and the retry is what costs money. + test('an undecryptable event does not fail the request', () async { + final completer = Completer(); + final garbage = NostrEvent.fromPartialData( + kind: 23195, + content: 'not-ciphertext', + keyPairs: walletKeys, + tags: const [ + ['e', requestId], + ], + ); + + await client.handleResponseEvent(garbage, requestId, completer); + + expect(completer.isCompleted, isFalse); + }); + + test('the genuine response still lands after an injected one', () async { + final completer = Completer(); + final garbage = NostrEvent.fromPartialData( + kind: 23195, + content: 'not-ciphertext', + keyPairs: walletKeys, + tags: const [ + ['e', requestId], + ], + ); + + await client.handleResponseEvent(garbage, requestId, completer); + await client.handleResponseEvent( + await genuineResponse(), requestId, completer); + + expect(completer.isCompleted, isTrue); + expect((await completer.future).result!['preimage'], 'deadbeef'); + }); + + test('a forged event claiming the wallet key does not fail the request', + () async { + final completer = Completer(); + final forged = NostrEvent( + id: 'a' * 64, + sig: 'b' * 128, + pubkey: walletKeys.public, + kind: 23195, + content: 'garbage', + createdAt: DateTime.now(), + tags: const [ + ['e', requestId], + ], + ); + + await client.handleResponseEvent(forged, requestId, completer); + + expect(completer.isCompleted, isFalse); + }); + + test('a response to a different request is ignored', () async { + final completer = Completer(); + + await client.handleResponseEvent( + await genuineResponse(eTag: 'a-different-request'), + requestId, + completer, + ); + + expect(completer.isCompleted, isFalse); + }); + + test('an event with no content is ignored', () async { + final completer = Completer(); + final empty = NostrEvent.fromPartialData( + kind: 23195, + content: '', + keyPairs: walletKeys, + tags: const [ + ['e', requestId], + ], + ); + + await client.handleResponseEvent(empty, requestId, completer); + + expect(completer.isCompleted, isFalse); + }); + + test('an error response from the wallet still completes the request', + () async { + // Only the wallet's own errors reach the caller; a relay cannot + // manufacture one. + final completer = Completer(); + final payload = jsonEncode({ + 'result_type': 'pay_invoice', + 'error': {'code': 'INSUFFICIENT_BALANCE', 'message': 'no funds'}, + }); + final encrypted = await NwcCrypto.encrypt( + payload, + walletKeys.private, + clientKeys.public, + NwcEncryption.nip44, + ); + final event = NostrEvent.fromPartialData( + kind: 23195, + content: encrypted, + keyPairs: walletKeys, + tags: const [ + ['e', requestId], + ], + ); + + await client.handleResponseEvent(event, requestId, completer); + + expect(completer.isCompleted, isTrue); + expect((await completer.future).isSuccess, isFalse); + }); + }); }