fix: authenticate events before recording them, and anchor message freshness - #660
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e094417ba5
ℹ️ 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".
e094417 to
7794bd9
Compare
|
@coderabbitai review |
|
…vent relay censorship
…d once Introduce InFlightEvents to claim an event id synchronously for the duration of its processing. Now that the durable dedup write happens after the event is authenticated, the hasItem check is asynchronous: two relays delivering the same event can both observe it as unseen and process it twice. The claim is released once processing ends, so a rejected event leaves no trace and a genuine one arriving later is still handled. Applied in MostroService and in both chat notifiers.
…data impersonation Replace `isVerified()` with `NostrUtils.isValidEventSignature` in MostroNodesNotifier and DeepLinkService. The former checks the signature against the event's self-declared id without recomputing it from the serialized event, so a genuine (id, sig, pubkey) triple lifted onto attacker-chosen content still passes. A relay can serve this to impersonate a trusted node in the picker or inject arbitrary order parameters into a deep link.
…event dispute ordering corruption
…t replay freshness forgery Capture the kind-14 `created_at` into `MostroMessage.timestamp` before storage. The outer event's timestamp is covered by the node's signature and recomputed id, so a relay can withhold a message but cannot make an old one look new. This prevents a replayed fiat-sent-ok from re-arming the Release button with a current receive time while the node-signed clock shows it is weeks stale. The v1 NIP-59 path continues to fall back to receive time because the wrap and seal deliberately randomise their timestamps.
…ssages from being accepted
…cks from modifying settled orders
…ages from re-arming settled orders
…daemon replay or race conditions
The stale-message guard ran after cancelSessionTimeoutCleanup and after the user-initiated cancel marker had been removed, so a superseded canceled message spent both before reaching the check that rejects it. The genuine response arriving afterwards then found no marker and was reported as a counterparty inactivity timeout: the wrong notification, and a bonded order's session deletion deferred by 60s instead of immediate. Move the check to the top of the handler, ahead of every message-driven side effect. Add notifier-level coverage driving the real subscribe() path.
updateStateFromMessage applied the restored snapshot without moving the high-water mark, and the message it synthesizes is dated with the order's created_at rather than the moment the snapshot describes. On a device with no remembered mark, every archived lifecycle event still compared as newer than the state just restored and could move it backwards. Carry the orders-details event's signed created_at into restore() and record it as each snapshot is applied. Only the v2 kind-14 path carries a signed clock; NIP-59 randomises the wrap and seal timestamps, so v1 yields null and leaves the mark untouched. Move the forward-only update into AbstractMostroNotifier next to the rest of the freshness logic, and align the regular-order branch with the dispute branch, which already converts Order.createdAt from seconds to milliseconds.
7794bd9 to
ab18f3c
Compare
1 participant
Depends on #659
This branch is cut from
fix/transport-downgrade-protectionand builds on the sender authentication landed there — the freshness work below is only meaningful once the signature coveringcreated_atis actually verified. Merge #659 first, or review this against that branch.What
Removes the client's dependence on the relay for three things it was never entitled to decide: which events are unique, which are current, and which are genuinely signed.
created_at(v2 kind 14) instead of local receive time, bound its age, and refuse any message older than the state it would modify. The high-water mark is persisted outside the databases a restore clears, so wiping history cannot make archived instructions look new.Notes
decryptNIP59Eventno longer wraps authentication errors in a genericException, so "not from the node" is distinguishable from "malformed".applies unverified events with warning) and was inverted.flutter analyzeclean;flutter test1111 passing.