From bdf9249eabd6fba5ccc65c52dabd6452614f21c8 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:02:24 +0700 Subject: [PATCH] fix NIP-4e sync findings and clean up consumed events --- crates/nip4e/src/lib.rs | 207 ++++++++++++++++++++++++++++++---------- docs/nip4e-audit.md | 15 ++- 2 files changed, 171 insertions(+), 51 deletions(-) diff --git a/crates/nip4e/src/lib.rs b/crates/nip4e/src/lib.rs index d74bc577..709adceb 100644 --- a/crates/nip4e/src/lib.rs +++ b/crates/nip4e/src/lib.rs @@ -324,6 +324,9 @@ impl DeviceRegistry { .finalize_async(&signer) .await?; + // Persist the key locally before announcing it + set_keys(&client, &signer, &secret).await?; + // Publish announcement client .send_event(&event) @@ -331,9 +334,6 @@ impl DeviceRegistry { .ack_policy(AckPolicy::none()) .await?; - // Save device keys to the database - set_keys(&client, &signer, &secret).await?; - Ok(keys) }) } @@ -384,11 +384,8 @@ impl DeviceRegistry { let public_key = signer.get_public_key_async().await?; let id = SubscriptionId::new("dekey-requests"); - // Construct a filter for encryption key requests - let filter = Filter::new() - .kind(Kind::Custom(4454)) - .author(public_key) - .since(Timestamp::now()); + // Construct a filter for encryption key requests. + let filter = Filter::new().kind(Kind::Custom(4454)).author(public_key); // Subscribe to the device key requests on user's write relays client.subscribe(vec![filter]).with_id(id).await?; @@ -417,26 +414,30 @@ impl DeviceRegistry { .pubkey(app_pubkey) .limit(1); - match client.database().query(filter).await?.into_iter().next() { - // Found an approval event - Some(event) => Ok(Some(event)), - // No approval event found, construct a request event - None => { - // Construct an event for device key request - let event = EventBuilder::new(Kind::Custom(4454), "") - .tags(vec![ - Tag::custom("P", vec![app_pubkey]), - Tag::custom("client", vec![CLIENT_NAME]), - ]) - .finalize_async(&signer) - .await?; - - // Send the event to write relays - client.send_event(&event).to_nip65().await?; - - Ok(None) + if let Some(event) = client.database().query(filter).await?.into_iter().next() { + // Only reuse the stored approval if its key is still valid, + // otherwise it's stale and a fresh request is needed + if decrypt_approval(&client, &app_keys, public_key, &event) + .await + .is_ok() + { + return Ok(Some(event)); } } + + // No usable approval event found, construct a request event + let event = EventBuilder::new(Kind::Custom(4454), "") + .tags(vec![ + Tag::custom("P", vec![app_pubkey]), + Tag::custom("client", vec![CLIENT_NAME]), + ]) + .finalize_async(&signer) + .await?; + + // Send the event to write relays + client.send_event(&event).to_nip65().await?; + + Ok(None) }); self.tasks.push(cx.spawn(async move |this, cx| { @@ -466,20 +467,41 @@ impl DeviceRegistry { let nostr = NostrRegistry::global(cx); let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); + let app_keys_task = get_or_init_app_keys(cx); cx.emit(DeviceEvent::Requesting); - self.tasks.push(cx.background_spawn(async move { - let public_key = signer.get_public_key_async().await?; + self.tasks.push(cx.spawn(async move |this, cx| { + let app_keys = match app_keys_task.await { + Ok(app_keys) => app_keys, + Err(e) => { + this.update(cx, |_this, cx| { + cx.emit(DeviceEvent::error(e.to_string())); + })?; + return Ok(()); + } + }; - // Construct a filter for device key requests - let filter = Filter::new() - .kind(Kind::Custom(4455)) - .author(public_key) - .since(Timestamp::now()); + let task: Task> = cx.background_spawn(async move { + let public_key = signer.get_public_key_async().await?; - // Subscribe to the device key requests on user's write relays - client.subscribe(filter).await?; + // Only approvals addressed to this device's app key matter + let filter = Filter::new() + .kind(Kind::Custom(4455)) + .author(public_key) + .pubkey(app_keys.public_key()); + + // Subscribe to the device key approvals on user's write relays + client.subscribe(filter).await?; + + Ok(()) + }); + + if let Err(e) = task.await { + this.update(cx, |_this, cx| { + cx.emit(DeviceEvent::error(e.to_string())); + })?; + } Ok(()) })); @@ -488,22 +510,16 @@ impl DeviceRegistry { /// Parse the approval event to get encryption key then set it fn extract_encryption(&mut self, event: Event, cx: &mut Context) { let app_keys_task = get_or_init_app_keys(cx); + let nostr = NostrRegistry::global(cx); + let client = nostr.read(cx).client(); + let signer = nostr.read(cx).signer(); + + let approval_id = event.id; let task: Task> = cx.background_spawn(async move { let app_keys = app_keys_task.await?; - let master = event - .tags - .iter() - .find(|tag| tag.kind() == "P") - .and_then(|tag| tag.content()) - .and_then(|content| PublicKey::parse(content).ok()) - .context("Invalid event's tags")?; - - let payload = event.content.as_str(); - let decrypted = app_keys.nip44_decrypt_async(&master, payload).await?; - - let secret = SecretKey::from_hex(&decrypted)?; - let keys = Keys::new(secret); + let identity = signer.get_public_key_async().await?; + let keys = decrypt_approval(&client, &app_keys, identity, &event).await?; Ok(keys) }); @@ -513,6 +529,8 @@ impl DeviceRegistry { Ok(keys) => { this.update(cx, |this, cx| { this.set_signer(keys, cx); + // Clean up our request and the approval event from relays + this.cleanup_sync_events(approval_id, cx); })?; } Err(e) => { @@ -525,6 +543,47 @@ impl DeviceRegistry { })); } + /// Delete our key request and the consumed approval event from relays + fn cleanup_sync_events(&mut self, approval_id: EventId, cx: &mut Context) { + let nostr = NostrRegistry::global(cx); + let client = nostr.read(cx).client(); + let signer = nostr.read(cx).signer(); + + self.tasks.push(cx.background_spawn(async move { + let result: Result<(), Error> = async { + let public_key = signer.get_public_key_async().await?; + + // Collect our key request events + let filter = Filter::new().kind(Kind::Custom(4454)).author(public_key); + + let mut ids: Vec = client + .database() + .query(filter) + .await? + .into_iter() + .map(|event| event.id) + .collect(); + ids.push(approval_id); + + let deletion = EventBuilder::new(Kind::EventDeletion, "") + .tags(Tags::from_list(ids.into_iter().map(Tag::event).collect())) + .finalize_async(&signer) + .await?; + + client.send_event(&deletion).to_nip65().await?; + + Ok(()) + } + .await; + + if let Err(e) = result { + log::error!("Failed to delete sync events: {e}"); + } + + Ok(()) + })); + } + /// Approve requests for device keys from other devices fn approve(&mut self, event: &Event, window: &mut Window, cx: &mut Context) { let nostr = NostrRegistry::global(cx); @@ -570,9 +629,14 @@ impl DeviceRegistry { Ok(()) }); - self.tasks.push(cx.spawn_in(window, async move |_this, cx| { + self.tasks.push(cx.spawn_in(window, async move |this, cx| { match task.await { Ok(_) => { + this.update(cx, |this, cx| { + // Allow future requests to surface again + this.set_pending_request(false, cx); + })?; + cx.update(|window, cx| { window.clear_notification_by_id::(id, cx); }) @@ -758,6 +822,51 @@ fn get_or_init_app_keys(cx: &App) -> Task> { }) } +/// Decrypt and validate an approval event against the current announcement. +async fn decrypt_approval( + client: &Client, + app_keys: &Keys, + identity: PublicKey, + event: &Event, +) -> Result { + let master = event + .tags + .iter() + .find(|tag| tag.kind() == "P") + .and_then(|tag| tag.content()) + .and_then(|content| PublicKey::parse(content).ok()) + .context("Invalid event's tags")?; + + let decrypted = app_keys + .nip44_decrypt_async(&master, event.content.as_str()) + .await?; + + let secret = SecretKey::from_hex(&decrypted)?; + let keys = Keys::new(secret); + + // Reject approvals carrying a key that no longer matches the published announcement. + let filter = Filter::new() + .kind(Kind::Custom(10044)) + .author(identity) + .limit(1); + + if let Some(announcement) = client.database().query(filter).await?.into_iter().next() { + let announced = announcement + .tags + .iter() + .find(|tag| tag.kind() == "n") + .and_then(|tag| tag.content()) + .and_then(|content| PublicKey::parse(content).ok()) + .context("Announcement has no valid encryption key")?; + + if keys.public_key() != announced { + return Err(anyhow!("Approval key doesn't match the announcement")); + } + } + + Ok(keys) +} + /// Encrypt and store device keys in the local database. async fn set_keys(client: &Client, signer: &S, secret: &str) -> Result<(), Error> where diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md index 82f407d2..1e9dee5e 100644 --- a/docs/nip4e-audit.md +++ b/docs/nip4e-audit.md @@ -1,9 +1,20 @@ # NIP-4e Audit Report -> **Update**: The P0 findings (C1, C2, C3) are now fixed — seals are always signed by the +> **Update**: The P0 findings (C1, C2, C3) are fixed — seals are always signed by the > identity key, `extract_rumor` falls back to the device key, the `DEVICE_GIFTWRAP` > subscription is created, and trashed wraps are retried on `DeviceEvent::Set`. -> The remaining findings (F1–F8, P2 items, UX bugs) are still open. +> +> **Update 2**: The P1 sync findings are fixed — F1 (4455 approvals now filtered by +> the requester's `p` tag so the approver no longer processes its own response), F2 +> (decrypted keys are validated against the current 10044 announcement via +> `decrypt_approval`), F3 (`since(now)` removed so offline requests/approvals +> backfill), F4 (`pending_request` resets after an approval completes), F5 (a NIP-09 +> deletion now removes the 4454 requests and the consumed 4455 after a successful +> 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. **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).