reya/coop
1
0
Fork 1
Code Issues Pull Requests 1 Releases 3 Activity

fix NIP-4e state hygiene and UX issues

Browse Source
This commit is contained in:
reya
2026-10-04 13:10:15 +07:00
parent bdf9249eab
commit 0bae051316
6 changed files with 101 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/nip4e/src/lib.rs
+71 -46
View File
@@ -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<Option<UniversalSigner>>,
/// 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<AtomicBool>,
/// Signer
signer: Entity<Option<UniversalSigner>>,
/// Async tasks
/// Relay notification listener
notification_listener: Option<Task<Result<(), Error>>>,
tasks: Vec<Task<Result<(), Error>>>,
/// 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>) {
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::<Event>(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<Result<(), Error>> = 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<Self>) {
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<Self>) -> Notification {
fn notification(&self, event: Event, cx: &Context<Self>) -> Result<Notification, Error> {
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::<DeviceNotification>(key)
.autohide(false)
.with_kind(NotificationKind::Info)
@@ -786,7 +811,7 @@ impl DeviceRegistry {
.ok();
}
})
})
}))
}
}
crates/person/src/lib.rs
+4 -1
View File
@@ -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| {
crates/state/src/nip4e.rs
+6 -9
View File
@@ -11,28 +11,25 @@ pub struct Announcement {
client_name: Option<String>,
}
impl From<&Event> for Announcement {
fn from(val: &Event) -> Self {
let public_key = val
impl Announcement {
pub fn from_event(event: &Event) -> Option<Self> {
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<String>) -> Self {
Self {
public_key,
crates/workspace/src/dialogs/restore.rs
+2 -1
View File
@@ -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 {
crates/workspace/src/lib.rs
+5 -4
View File
@@ -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::<DeviceNotifcation>()
@@ -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::<DeviceNotifcation>(cx);
},
docs/nip4e-audit.md
+13 -2
View File
@@ -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).