From 0bae05131666052abdaa97d5a354ef9b8d3146f2 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:10:15 +0700 Subject: [PATCH] fix NIP-4e state hygiene and UX issues --- crates/nip4e/src/lib.rs | 117 ++++++++++++++---------- crates/person/src/lib.rs | 5 +- crates/state/src/nip4e.rs | 15 ++- crates/workspace/src/dialogs/restore.rs | 3 +- crates/workspace/src/lib.rs | 9 +- docs/nip4e-audit.md | 15 ++- 6 files changed, 101 insertions(+), 63 deletions(-) diff --git a/crates/nip4e/src/lib.rs b/crates/nip4e/src/lib.rs index 709adceb..be189731 100644 --- a/crates/nip4e/src/lib.rs +++ b/crates/nip4e/src/lib.rs @@ -10,7 +10,6 @@ use gpui::{ App, AppContext, Context, Entity, EventEmitter, Global, IntoElement, ParentElement, SharedString, Styled, Subscription, Task, Window, div, relative, }; -use instant::Duration; use nostr_sdk::prelude::*; use person::PersonRegistry; use settings::AppSettings; @@ -59,19 +58,14 @@ impl DeviceEvent { /// NIP-4e: https://github.com/nostr-protocol/nips/blob/per-device-keys/4e.md #[derive(Debug)] pub struct DeviceRegistry { + signer: Entity>, /// Whether there is a pending request for encryption key approval pub pending_request: bool, - /// Whether an announcement has been made for this device pub announcement_existed: Arc, - - /// Signer - signer: Entity>, - - /// Async tasks + /// Relay notification listener + notification_listener: Option>>, tasks: Vec>>, - - /// Event subscription _subscriptions: SmallVec<[Subscription; 2]>, } @@ -110,6 +104,9 @@ impl DeviceRegistry { // Observe the user signer cx.subscribe(&nostr, move |this, _nostr, event, cx| { if event.signer_changed() && settings.read(cx).is_nip4e_enabled(cx) { + // The old account's state must not leak into the new one + this.announcement_existed.store(false, Ordering::Relaxed); + this.handle_notifications(cx); this.get_announcement(cx); } }), @@ -125,12 +122,15 @@ impl DeviceRegistry { signer, pending_request: false, announcement_existed: Arc::new(AtomicBool::new(false)), + notification_listener: None, tasks: vec![], _subscriptions: subscriptions, } } fn handle_notifications(&mut self, cx: &mut Context) { + self.notification_listener = None; + let nostr = NostrRegistry::global(cx); let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); @@ -138,7 +138,7 @@ impl DeviceRegistry { let announcement_existed = self.announcement_existed.clone(); let (tx, rx) = flume::bounded::(100); - self.tasks.push(cx.background_spawn(async move { + self.notification_listener = Some(cx.background_spawn(async move { let mut notifications = client.notifications(); let mut processed_events = HashSet::new(); let current_user = signer.get_public_key_async().await.ok(); @@ -245,39 +245,41 @@ impl DeviceRegistry { let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); - self.tasks.push(cx.background_spawn(async move { - let opts = SubscribeAutoCloseOptions::default().exit_policy(ReqExitPolicy::ExitOnEOSE); - let current_user = signer.get_public_key_async().await?; - - // Construct the filter for the device announcement event - let filter = Filter::new() - .kind(Kind::Custom(10044)) - .author(current_user) - .limit(1); - - client - .subscribe(filter) - .close_on(opts) - .with_id(SubscriptionId::new("nip4e")) - .await?; - - Ok(()) - })); - let announcement_existed = self.announcement_existed.clone(); self.tasks.push(cx.spawn(async move |this, cx| { - // Wait for 5 seconds - cx.background_executor().timer(Duration::from_secs(5)).await; + let task: Task> = cx.background_spawn(async move { + let opts = + SubscribeAutoCloseOptions::default().exit_policy(ReqExitPolicy::ExitOnEOSE); + let current_user = signer.get_public_key_async().await?; - // Then check if the msg relays have been found - if announcement_existed.load(Ordering::Acquire) { + // Construct the filter for the device announcement event + let filter = Filter::new() + .kind(Kind::Custom(10044)) + .author(current_user) + .limit(1); + + client + .subscribe(filter) + .close_on(opts) + .with_id(SubscriptionId::new("nip4e")) + .await?; + + Ok(()) + }); + + if let Err(e) = task.await { + this.update(cx, |_this, cx| { + cx.emit(DeviceEvent::error(e.to_string())); + })?; return Ok(()); } - this.update(cx, |_this, cx| { - cx.emit(DeviceEvent::NotSet); - })?; + if !announcement_existed.load(Ordering::Acquire) { + this.update(cx, |_this, cx| { + cx.emit(DeviceEvent::NotSet); + })?; + } Ok(()) })); @@ -340,11 +342,15 @@ impl DeviceRegistry { /// Set encryption key from the announcement event fn set_encryption(&mut self, event: &Event, cx: &mut Context) { + let Some(announcement) = Announcement::from_event(event) else { + log::warn!("Ignoring malformed encryption announcement"); + return; + }; + let nostr = NostrRegistry::global(cx); let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); - let announcement = Announcement::from(event); let device_pubkey = announcement.public_key(); // Get encryption key from the database and compare with the announcement @@ -665,11 +671,15 @@ impl DeviceRegistry { } self.set_pending_request(true, cx); - // Show notification - let notification = self.notification(event, cx); + let notification = match self.notification(event, cx) { + Ok(notification) => notification, + Err(e) => { + log::warn!("Ignoring invalid encryption key request: {e}"); + self.set_pending_request(false, cx); + return; + } + }; - // The registry is global and not bound to a window, so surface the - // request in an open window. if let Some(window) = cx.windows().first().copied() { if let Err(error) = window.update(cx, |_view, window, cx| { window.push_notification(notification, cx); @@ -682,19 +692,34 @@ impl DeviceRegistry { } /// Build a notification for the encryption request. - fn notification(&self, event: Event, cx: &Context) -> Notification { + fn notification(&self, event: Event, cx: &Context) -> Result { const MSG: &str = "You've requested an encryption key from another device. \ Approve to allow Coop to share with it."; - let request = Announcement::from(&event); + let requester = event + .tags + .iter() + .find(|tag| tag.kind() == "P") + .and_then(|tag| tag.content()) + .and_then(|content| PublicKey::parse(content).ok()) + .context("Request has no valid requester key")?; + + let client_name = event + .tags + .iter() + .find(|tag| tag.kind() == "client") + .and_then(|tag| tag.content()) + .map(|c| c.to_string()); + + let request = Announcement::new(requester, client_name); let persons = PersonRegistry::global(cx); - let profile = persons.read(cx).get(&request.public_key(), cx); + let profile = persons.read(cx).get(&requester, cx); let entity = cx.entity().downgrade(); let loading = Rc::new(Cell::new(false)); let key = SharedString::from(event.id.to_hex()); - Notification::new() + Ok(Notification::new() .type_id::(key) .autohide(false) .with_kind(NotificationKind::Info) @@ -786,7 +811,7 @@ impl DeviceRegistry { .ok(); } }) - }) + })) } } diff --git a/crates/person/src/lib.rs b/crates/person/src/lib.rs index c1aadd1b..d4eb870b 100644 --- a/crates/person/src/lib.rs +++ b/crates/person/src/lib.rs @@ -216,7 +216,10 @@ impl PersonRegistry { /// Set profile encryption keys announcement fn set_announcement(&mut self, event: &Event, cx: &mut App) { - let announcement = Announcement::from(event); + let Some(announcement) = Announcement::from_event(event) else { + log::warn!("Ignoring announcement with missing or invalid key tag"); + return; + }; if let Some(person) = self.persons.get(&event.pubkey) { person.update(cx, |person, cx| { diff --git a/crates/state/src/nip4e.rs b/crates/state/src/nip4e.rs index 475795c3..dbb18804 100644 --- a/crates/state/src/nip4e.rs +++ b/crates/state/src/nip4e.rs @@ -11,28 +11,25 @@ pub struct Announcement { client_name: Option, } -impl From<&Event> for Announcement { - fn from(val: &Event) -> Self { - let public_key = val +impl Announcement { + pub fn from_event(event: &Event) -> Option { + let public_key = event .tags .iter() .find(|tag| tag.kind() == "n") .and_then(|tag| tag.content()) - .and_then(|c| PublicKey::parse(c).ok()) - .unwrap_or(val.pubkey); + .and_then(|c| PublicKey::parse(c).ok())?; - let client_name = val + let client_name = event .tags .iter() .find(|tag| tag.kind() == "client") .and_then(|tag| tag.content()) .map(|c| c.to_string()); - Self::new(public_key, client_name) + Some(Self::new(public_key, client_name)) } -} -impl Announcement { pub fn new(public_key: PublicKey, client_name: Option) -> Self { Self { public_key, diff --git a/crates/workspace/src/dialogs/restore.rs b/crates/workspace/src/dialogs/restore.rs index b5211aaf..5511c909 100644 --- a/crates/workspace/src/dialogs/restore.rs +++ b/crates/workspace/src/dialogs/restore.rs @@ -51,8 +51,9 @@ impl RestoreEncryption { let device = DeviceRegistry::global(cx); let content = self.key_input.read(cx).value(); - if !content.is_empty() { + if content.is_empty() { self.set_error("Secret Key cannot be empty.", cx); + return; } let Ok(secret) = SecretKey::parse(&content) else { diff --git a/crates/workspace/src/lib.rs b/crates/workspace/src/lib.rs index 42b226dc..f4ecac10 100644 --- a/crates/workspace/src/lib.rs +++ b/crates/workspace/src/lib.rs @@ -5,11 +5,11 @@ use anyhow::Error; use auto_update::AutoUpdater; use chat::{ChatEvent, ChatRegistry}; use common::download_dir; -use nip4e::{DeviceEvent, DeviceRegistry}; use gpui::{ Action, App, AppContext, Context, Entity, InteractiveElement, IntoElement, ParentElement, Render, SharedString, Styled, Subscription, Task, Window, div, px, }; +use nip4e::{DeviceEvent, DeviceRegistry}; use nip29::{GroupId, GroupKey}; use nostr_sdk::prelude::*; use serde::Deserialize; @@ -120,8 +120,9 @@ impl Workspace { window.push_notification(note, cx); } DeviceEvent::NotSet => { - const MSG: &str = - "User're not setup encryption key yet. Do you want to create one?"; + const MSG: &str = "No encryption key announcement was found. It may \ + still be propagating, or you can restore one \ + from the settings."; let note = Notification::new() .id::() @@ -132,7 +133,7 @@ impl Workspace { move |_this, window, cx| { let device = DeviceRegistry::global(cx); device.update(cx, |this, cx| { - this.set_announcement(Keys::generate(), cx); + this.get_announcement(cx); }); window.clear_notification::(cx); }, diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md index 1e9dee5e..99573565 100644 --- a/docs/nip4e-audit.md +++ b/docs/nip4e-audit.md @@ -13,8 +13,19 @@ > sync), F6 (stale stored 4455s no longer short-circuit `request()` — they are > validated first and a fresh request is published when invalid), F8 (device keys > are saved locally *before* the 10044 announcement is published, closing the -> echo/self-request race). Still open: the P2 state-hygiene items and the UX bugs -> in section 5. +> echo/self-request race). +> +> **Update 3**: The P2/UX findings are fixed — the notification listener is now +> restarted on account switch and `announcement_existed` is reset (no stale +> `current_user` / false negatives after changing accounts), `NotSet` fires on +> EOSE instead of a fixed 5-second timer, `Announcement::from_event` returns +> `None` on a missing/malformed `n` tag instead of falling back to the identity +> pubkey, the 4454 notification parses the requester key from the `P` tag, the +> NotSet "Retry" button re-checks for an announcement instead of silently +> generating (rotating) a new encryption key, and the restore dialog's +> inverted empty-key check is fixed. Still open: the remaining security notes +> in section 4 (plaintext backup file, one-click approval without a +> verification code). **Scope**: `crates/nip4e/src/lib.rs` (key announcement/sync), the send path (`crates/chat/src/room.rs`), the receive path (`crates/chat/src/lib.rs`), UI integration (`crates/workspace`), compared against the [NIP-4e draft](https://github.com/nostr-protocol/nips/blob/per-device-keys/4e.md) and [PsstPsst's implementation](https://github.com/CodyTseng/psstpsst) (`docs/protocols/nip4e.md` + `docs/protocols/nip17-extensions.md`, plus rust-nostr's NIP-59 source to verify crypto mechanics).