fix: verify relay-sourced events before the app acts on them - #663
fix: verify relay-sourced events before the app acts on them#663AndreaDiazCorreia wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1748bf79e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (relays.length > maxRelays) { | ||
| logger.w( | ||
| 'Relay list from ${event.pubkey} names ${relays.length} relays; ' | ||
| 'keeping the first $maxRelays', | ||
| ); | ||
| relays = relays.sublist(0, maxRelays); | ||
| } | ||
|
|
There was a problem hiding this comment.
Apply the relay cap after filtering and deduplication
When a signed relay list contains more than 50 raw r tags, this truncates the raw list before validRelays removes malformed, insecure, or duplicate entries. For example, 50 duplicate or invalid tags followed by the node's usable relays produces an empty or one-relay active set and discards all later valid relays, potentially leaving the app unable to reach the Mostro instance. Normalize, validate, and deduplicate first, then cap the relays that can actually be contributed.
Useful? React with 👍 / 👎.
14f83f2 to
444af66
Compare
2915ef1 to
455778b
Compare
There was a problem hiding this comment.
Changes requested
The signature checks at the order-book, relay-list, and NWC boundaries are valuable, but two relay-sourced paths still undermine the stated trust boundary:
-
Relay cap before validation/deduplication (
lib/core/models/relay_list_event.dart:55-103): the 50-entry cap is applied to rawrtags. A signed list beginning with 50 duplicate, malformed, or insecure URLs can crowd out all later usable relays, leaving no viable connection. Normalize, validate, and deduplicate first, then cap the usable relay set. -
Unsigned deep-link event intake (
lib/services/nostr_service.dart:350-405and410-496): bothfetchEventByIdandfetchOrderInfoByEventIdaccept a relay-provided ID/kind/pubkey withoutNostrUtils.isValidEventSignature(event). A malicious relay selected by a deep link can fabricate an unsigned event claiming the configured Mostro pubkey and have it parsed/presented as an order. Verify the selected event's signature before parsing in both paths, with forged and retagged-event coverage.
git diff --check is clean. GitHub reports no current-head CI check run, and Flutter is unavailable in this review environment.
There was a problem hiding this comment.
Supplemental blocking finding
Stale signed order events can overwrite newer open-order state (lib/data/repositories/open_orders_repository.dart:100-134). The new intake verifies kind, author, signature, and tags, but unconditionally assigns _events[orderId] = event. A relay can replay an older genuinely signed kind-38383 event for the same d tag after a newer one arrived, replacing the displayed amount, premium, rating, or status with stale state. The PR already applies deterministic NIP-01 replacement ordering to kind-38385 info events; apply the same created_at then lower-event-id tie-break to order events and add newer-then-older regression coverage.
52d4549 to
95c01ae
Compare
455778b to
457347a
Compare
Depends on #659
Cut from
fix/transport-downgrade-protection, which is where the publicNostrUtils.isValidEventSignaturethis builds on landed. Merge #659 first, or review against that branch.What
Four intakes accepted events straight off a relay without establishing that they were what they claimed to be. Every subscription pins
kindsandauthorsin its filter, but the pinned dart_nostr fork parses relay EVENT frames into aNostrEventwithout verifying the signature or matching the frame against the filter that was sent — so both state what was asked for, and the author on the way back is the relay's claim.z=order: no kind check, no author check, no signature. This feeds the amounts, premiums and maker rating shown before a user decides to trade.fromEventalso makespublishedAtmeaningful, since the freshness comparison downstream is againstcreated_at. Cleartextws://is refused on the same terms as the manual-entry path, and one event can no longer contribute an unbounded number of relays.settings.relays, with a restore that could not work: it read the settings getter after it had already been replaced. Restoring correctly would not have helped either — dart_nostr'sinitis additive and the fork has no per-relay disconnect. The fetch now runs on a scopedNostrinstance, closed when it ends.Notes
specificRelaysreaches only those relays, not the union with the app's. The deep link path already falls back to the app's own relays when no candidate is found there.NostrEvent.typedrops its bang assert: an event without aztag reached it before any other check and threw inside the stream callback, whereonErrordoes not catch it. One caller inlib/.flutter analyzeclean;flutter test1142 passing.