From 67f8f5db49786cbf18deb6f9b3728d7e045d8d8d Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 12:08:06 +0700 Subject: [PATCH 01/10] chore: split cached and live loading of the group list --- crates/nip29/src/lib.rs | 45 +++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/crates/nip29/src/lib.rs b/crates/nip29/src/lib.rs index 18384202..cb6df300 100644 --- a/crates/nip29/src/lib.rs +++ b/crates/nip29/src/lib.rs @@ -126,6 +126,7 @@ impl GroupsRegistry { if event.signer_changed() { this.reset(cx); this.handle_notifications(cx); + this.load_cached(cx); this.load(cx); } })); @@ -133,6 +134,7 @@ impl GroupsRegistry { cx.defer(move |cx| { if let Err(error) = this.update(cx, |this, cx| { this.handle_notifications(cx); + this.load_cached(cx); if nostr.read(cx).current_user().is_some() { this.load(cx); @@ -349,20 +351,21 @@ impl GroupsRegistry { cx.notify(); } - fn load(&mut self, cx: &mut Context) { + fn load_cached(&mut self, cx: &mut Context) { let nostr = NostrRegistry::global(cx); + + if nostr.read(cx).current_user().is_none() { + return; + } + let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); + // Show the cached groups before the relays answer. let task: Task> = cx.background_spawn(async move { let me = signer.get_public_key_async().await?; let filter = Filter::new().author(me).kind(Kind::SimpleGroups).limit(1); - client - .subscribe(ReqTarget::auto([filter.clone()])) - .with_id(list_subscription_id()) - .await?; - match client.database().query(filter).await?.into_iter().next() { Some(event) => GroupList::parse(&event), None => Ok(GroupList::default()), @@ -373,11 +376,37 @@ impl GroupsRegistry { .push(cx.spawn(async move |this, cx| match task.await { Ok(list) => { if let Err(error) = this.update(cx, |this, cx| this.merge(list, cx)) { - log::warn!("nip29: applying the group list failed: {error}"); + log::warn!("nip29: applying the cached group list failed: {error}"); } } - Err(error) => log::warn!("nip29: loading the group list failed: {error}"), + Err(error) => log::warn!("nip29: loading the cached group list failed: {error}"), })); + } + + fn load(&mut self, cx: &mut Context) { + let nostr = NostrRegistry::global(cx); + let client = nostr.read(cx).client(); + let signer = nostr.read(cx).signer(); + + let updates: Task> = cx.background_spawn(async move { + let me = signer.get_public_key_async().await?; + let opts = SubscribeAutoCloseOptions::default().exit_policy(ReqExitPolicy::ExitOnEOSE); + let filter = Filter::new().author(me).kind(Kind::SimpleGroups).limit(1); + + client + .subscribe(ReqTarget::auto([filter])) + .with_id(list_subscription_id()) + .close_on(opts) + .await?; + + Ok(()) + }); + + self.tasks.push(cx.spawn(async move |_this, _cx| { + if let Err(error) = updates.await { + log::warn!("nip29: subscribing to the group list failed: {error}"); + } + })); self.watch_forks(cx); } -- 2.54.0 From 045151fe5c700ec78f22fe084429ef421615c45e Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 12:24:42 +0700 Subject: [PATCH 02/10] chore: rename `device` crate to `nip4e` --- Cargo.lock | 48 ++++++++++++------------- crates/chat/Cargo.toml | 2 +- crates/chat/src/room.rs | 2 +- crates/{device => nip4e}/Cargo.toml | 2 +- crates/{device => nip4e}/src/lib.rs | 0 crates/workspace/Cargo.toml | 2 +- crates/workspace/src/dialogs/restore.rs | 5 ++- crates/workspace/src/lib.rs | 2 +- desktop/Cargo.toml | 2 +- desktop/src/main.rs | 2 +- web/Cargo.toml | 2 +- web/src/lib.rs | 2 +- 12 files changed, 35 insertions(+), 36 deletions(-) rename crates/{device => nip4e}/Cargo.toml (97%) rename crates/{device => nip4e}/src/lib.rs (100%) diff --git a/Cargo.lock b/Cargo.lock index 070d77cd..c9f72605 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1073,7 +1073,6 @@ version = "1.0.2" dependencies = [ "anyhow", "common", - "device", "flume 0.11.1", "futures", "fuzzy-matcher", @@ -1081,6 +1080,7 @@ dependencies = [ "instant", "itertools 0.13.0", "log", + "nip4e", "nostr-sdk", "person", "settings", @@ -1413,7 +1413,6 @@ dependencies = [ "auto_update", "chat", "common", - "device", "gpui-pre", "gpui-pre-linux", "gpui-pre-macos", @@ -1422,6 +1421,7 @@ dependencies = [ "gpui-pre-windows", "log", "nip29", + "nip4e", "nostr-sdk", "person", "settings", @@ -1441,7 +1441,6 @@ dependencies = [ "common", "console_error_panic_hook", "console_log", - "device", "getrandom 0.2.17", "getrandom 0.3.4", "getrandom 0.4.3", @@ -1451,6 +1450,7 @@ dependencies = [ "instant", "log", "nip29", + "nip4e", "person", "settings", "state", @@ -1823,26 +1823,6 @@ dependencies = [ "unicode-xid", ] -[[package]] -name = "device" -version = "1.0.2" -dependencies = [ - "anyhow", - "common", - "flume 0.11.1", - "gpui-pre", - "instant", - "log", - "nostr-sdk", - "person", - "settings", - "smallvec", - "smol", - "state", - "theme", - "ui", -] - [[package]] name = "digest" version = "0.10.7" @@ -4577,6 +4557,26 @@ dependencies = [ "ui", ] +[[package]] +name = "nip4e" +version = "1.0.2" +dependencies = [ + "anyhow", + "common", + "flume 0.11.1", + "gpui-pre", + "instant", + "log", + "nostr-sdk", + "person", + "settings", + "smallvec", + "smol", + "state", + "theme", + "ui", +] + [[package]] name = "no_std_io2" version = "0.9.4" @@ -9438,12 +9438,12 @@ dependencies = [ "chat", "chat_ui", "common", - "device", "gpui-pre", "instant", "log", "nip29", "nip29_ui", + "nip4e", "nostr-connect", "nostr-sdk", "person", diff --git a/crates/chat/Cargo.toml b/crates/chat/Cargo.toml index ad7fd71b..e0dcd9f7 100644 --- a/crates/chat/Cargo.toml +++ b/crates/chat/Cargo.toml @@ -7,7 +7,7 @@ publish.workspace = true [dependencies] common = { path = "../common" } state = { path = "../state" } -device = { path = "../device" } +nip4e = { path = "../nip4e" } person = { path = "../person" } settings = { path = "../settings" } diff --git a/crates/chat/src/room.rs b/crates/chat/src/room.rs index 00bd2294..59c3545d 100644 --- a/crates/chat/src/room.rs +++ b/crates/chat/src/room.rs @@ -3,7 +3,7 @@ use std::hash::{Hash, Hasher}; use anyhow::{Error, anyhow}; use common::EventExt; -use device::DeviceRegistry; +use nip4e::DeviceRegistry; use gpui::{App, AppContext, Context, EventEmitter, SharedString, Task}; use instant::Duration; use itertools::Itertools; diff --git a/crates/device/Cargo.toml b/crates/nip4e/Cargo.toml similarity index 97% rename from crates/device/Cargo.toml rename to crates/nip4e/Cargo.toml index fa2b0c37..9ec26d9b 100644 --- a/crates/device/Cargo.toml +++ b/crates/nip4e/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "device" +name = "nip4e" version.workspace = true edition.workspace = true publish.workspace = true diff --git a/crates/device/src/lib.rs b/crates/nip4e/src/lib.rs similarity index 100% rename from crates/device/src/lib.rs rename to crates/nip4e/src/lib.rs diff --git a/crates/workspace/Cargo.toml b/crates/workspace/Cargo.toml index 90e002e4..018905b9 100644 --- a/crates/workspace/Cargo.toml +++ b/crates/workspace/Cargo.toml @@ -9,7 +9,7 @@ ui = { path = "../ui" } theme = { path = "../theme" } common = { path = "../common" } state = { path = "../state" } -device = { path = "../device" } +nip4e = { path = "../nip4e" } chat = { path = "../chat" } chat_ui = { path = "../chat_ui" } nip29 = { path = "../nip29" } diff --git a/crates/workspace/src/dialogs/restore.rs b/crates/workspace/src/dialogs/restore.rs index 7196bc00..b5211aaf 100644 --- a/crates/workspace/src/dialogs/restore.rs +++ b/crates/workspace/src/dialogs/restore.rs @@ -1,12 +1,11 @@ -use instant::Duration; - use anyhow::Error; -use device::DeviceRegistry; use gpui::prelude::FluentBuilder; use gpui::{ AppContext, Context, Entity, IntoElement, ParentElement, Render, SharedString, Styled, Subscription, Task, Window, div, }; +use instant::Duration; +use nip4e::DeviceRegistry; use nostr_sdk::prelude::*; use theme::ActiveTheme; use ui::button::{Button, ButtonVariants}; diff --git a/crates/workspace/src/lib.rs b/crates/workspace/src/lib.rs index 72cd17a0..42b226dc 100644 --- a/crates/workspace/src/lib.rs +++ b/crates/workspace/src/lib.rs @@ -5,7 +5,7 @@ use anyhow::Error; use auto_update::AutoUpdater; use chat::{ChatEvent, ChatRegistry}; use common::download_dir; -use device::{DeviceEvent, DeviceRegistry}; +use nip4e::{DeviceEvent, DeviceRegistry}; use gpui::{ Action, App, AppContext, Context, Entity, InteractiveElement, IntoElement, ParentElement, Render, SharedString, Styled, Subscription, Task, Window, div, px, diff --git a/desktop/Cargo.toml b/desktop/Cargo.toml index c587cccb..a9f13c51 100644 --- a/desktop/Cargo.toml +++ b/desktop/Cargo.toml @@ -33,7 +33,7 @@ ui = { path = "../crates/ui" } theme = { path = "../crates/theme" } common = { path = "../crates/common" } state = { path = "../crates/state" } -device = { path = "../crates/device" } +nip4e = { path = "../crates/nip4e" } chat = { path = "../crates/chat" } nip29 = { path = "../crates/nip29" } settings = { path = "../crates/settings" } diff --git a/desktop/src/main.rs b/desktop/src/main.rs index c19a3b30..a1efb187 100644 --- a/desktop/src/main.rs +++ b/desktop/src/main.rs @@ -73,7 +73,7 @@ fn main() { // Initialize device signer // // NIP-4e: https://github.com/nostr-protocol/nips/blob/per-device-keys/4e.md - device::init(cx); + nip4e::init(cx); // Initialize app registry chat::init(cx); diff --git a/web/Cargo.toml b/web/Cargo.toml index 2e701222..51482e8b 100644 --- a/web/Cargo.toml +++ b/web/Cargo.toml @@ -14,7 +14,7 @@ ui = { path = "../crates/ui" } theme = { path = "../crates/theme" } common = { path = "../crates/common" } state = { path = "../crates/state" } -device = { path = "../crates/device" } +nip4e = { path = "../crates/nip4e" } chat = { path = "../crates/chat" } nip29 = { path = "../crates/nip29" } settings = { path = "../crates/settings" } diff --git a/web/src/lib.rs b/web/src/lib.rs index ce22e58a..91398653 100644 --- a/web/src/lib.rs +++ b/web/src/lib.rs @@ -65,7 +65,7 @@ pub fn run() -> Result<(), JsValue> { // Initialize device signer // // NIP-4e: https://github.com/nostr-protocol/nips/blob/per-device-keys/4e.md - device::init(cx); + nip4e::init(cx); // Initialize app registry chat::init(cx); -- 2.54.0 From e34f325164b1d30b047f241a6969e0ec4dd46357 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 12:43:14 +0700 Subject: [PATCH 03/10] add plan --- docs/nip4e-audit.md | 111 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 docs/nip4e-audit.md diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md new file mode 100644 index 00000000..5f32a8d4 --- /dev/null +++ b/docs/nip4e-audit.md @@ -0,0 +1,111 @@ +# NIP-4e Audit Report + +**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). + +## Executive summary + +The key-sync side (kinds 10044/4454/4455) is a reasonable sketch of the spec, but **the actual message encryption is inverted and non-functional end-to-end**: + +- **Sending** seals/signs with the **device encryption key** (should be the identity key). +- **Receiving** unwraps gift wraps with the **identity key** (should be the device key when the wrap is addressed to your encryption key `E`). + +Every NIP-4e message between two Coop clients fails at decryption; messages to/from PsstPsst fail in both directions. On top of that there are ~10 sync/UX bugs that make even key distribution unreliable. + +## 1. Critical — the message path is broken (P0) + +rust-nostr's `GiftWrapBuilder::finalize_async(signer)` (verified against upstream source): + +- Seal **content** = `nip44_encrypt(conversation_key(signer_sk, receiver_pk), rumor)` +- Seal is **signed by `signer`** → `seal.pubkey = signer_pk` +- Gift wrap = `nip44_encrypt(conversation_key(ephemeral_sk, receiver_pk), seal)` +- Unwrap requires the receiver's secret matching `receiver_pk`; then rust-nostr **rejects if `rumor.pubkey != seal.pubkey`** ("sender mismatch"). + +NIP-4e draft says the *sender* keeps using their **identity key** (`ecdh(b, E)` — Bob's `b`) and only the *target* changes to `E`. So: + +| # | Finding | Location | +|---|---------|----------| +| **C1** | **Send path signs the seal with the device key.** `select_signer` returns the encryption signer, and `GiftWrapBuilder::finalize_async(device_signer)` signs the seal with `E` while `rumor.pubkey` = identity (`Room::rumor` uses `current_user`). Result: (a) receivers can't attribute the sender, (b) Coop's own `rumor.pubkey != seal.pubkey` check rejects it, (c) PsstPsst verifies the seal against the sender's *identity* key → invalid. | `crates/chat/src/room.rs:482-495, 590-596, 643-646`; rumor pubkey at `room.rs:404-417` | +| **C2** | **Receive path unwraps with the identity key.** `handle_notifications` captures the *user* signer and passes it to `extract_rumor` → `nip44_decrypt(ecdh(a, wrap_pubkey))`. A 4e wrap is encrypted to your `E`, so decryption always fails → "Failed to extract rumor" → trash. The correct unwrap key for 4e messages is the **device key** (`ecdh(e, wrap_pubkey)`), then `ecdh(e, seal.pubkey)` for the rumor — both succeed with the device signer in rust-nostr's unwrapper. | `crates/chat/src/lib.rs:184, 252, 743-801` | +| **C3** | **The `DEVICE_GIFTWRAP` subscription is never created.** The constant exists (`state/src/constants.rs:24`), its ID is used for EOSE matching (`chat/src/lib.rs:190`), but no code ever `subscribe()`s with it (presumably intended: `#p = E` filter on inbox relays). Only `USER_GIFTWRAP` (`#p = A`) exists. | `crates/chat/src/lib.rs:190-191, 410-418` | + +**Consequences**: + +| Path | Status | +|------|--------| +| Coop → Coop (4e) | ❌ Fails at receiver wrap-decryption (C2); even with a device-key unwrap it fails the sender-mismatch check (C1) | +| Coop → PsstPsst | ❌ PsstPsst unwraps fine (wrap → `E`), but the seal signature doesn't verify against your identity key and there's no seal `n` tag → rejected | +| PsstPsst → Coop | ❌ Wrap routed via identity `p` tag arrives, but Coop unwraps with the identity key → fail (C2) | +| Coop → self backup copy | ❌ Same broken envelope, so cross-device history sync also fails | +| Non-4e (`SignerKind::User`) | ✅ Works — the bug only bites when 4e is enabled, which is why it looks "half-baked" rather than fully broken | + +> Note on interop targets: the **draft** wants `seal signed by identity, content = ecdh(identity_sk, receiver_E)` — which is exactly `GiftWrapBuilder::finalize_async(user_signer)` with `receiver = E`. **PsstPsst's profile** goes further: seal signed by identity **plus** an `n` tag with the sender's `E`, content encrypted with `ecdh(sender_e, receiver_E)`. So minimal correctness (Coop↔Coop) = swap the signers; PsstPsst interop additionally requires implementing their split-key envelope (`n` tag, sender-`e` content key). + +```mermaid +sequenceDiagram + participant S as Sender (Coop) + participant R as Receiver (Coop) + Note over S: TODAY (broken)
seal signed by device key Es
wrap → Er, unwrap with identity a + S->>R: gift wrap (→ Er) + R->>R: nip44_decrypt(a, wrap) ✗ FAILS + Note over S: CORRECT (draft)
seal signed by identity As
wrap → Er, unwrap with device e + S->>R: gift wrap (→ Er) + R->>R: nip44_decrypt(e, wrap) ✓
nip44_decrypt(e, seal.pubkey) ✓ +``` + +## 2. Key-sync bugs (P1) + +| # | Finding | Location | +|---|---------|----------| +| **F1** | **No `p`-tag filter on 4455.** `wait_for_approval` subscribes to *all* your 4455s (`author=me, since=now`). `extract_encryption` then tries to decrypt transfers meant for *other* devices → NIP-44 failure → `DeviceEvent::Error` → error notification. The approving device also picks up its own just-published 4455 and errors on it. Spec/PsstPsst: filter by `p = own client key` and *ignore* undecryptable responses. | `nip4e/src/lib.rs:476-482, 163-165, 489-526` | +| **F2** | **No validation of the received key.** After decrypting a 4455, the key is installed without comparing `derived_pubkey` to the current 10044 `n` value. A stale 4455 (pre-rotation) silently installs a dead key. PsstPsst: "accepts the key only when that public key equals the current kind 10044 `n` value". | `nip4e/src/lib.rs:489-526` | +| **F3** | **`since(Timestamp::now())` on both subscriptions.** The approver misses all 4454s published while it was offline (the draft says Cope "looks for **all** 4454 events" — i.e. backfill). The requester likewise misses 4455s published while it was offline; recovery depends on LMDB having synced them. This is the main "works only when both apps are open at the same time" behavior. | `nip4e/src/lib.rs:388-394, 476-482` | +| **F4** | **`pending_request` is never reset to false.** After the first approval in a session, every subsequent device request is silently dropped (`ask_for_approval` early-returns). | `nip4e/src/lib.rs:597-601` (no `set_pending_request(false)` anywhere) | +| **F5** | **No deletion after success.** The draft explicitly says the requester deletes its 4454 + the received 4455 (NIP-09). Coop leaves them on relays forever, and combined with F6 below, stale events get re-applied on later launches. | absent | +| **F6** | **`request()` short-circuits on *any* old 4455 in the local DB** (`limit(1)`, no recency check, no decrypt-check before committing to it) — stale after rotation. | `nip4e/src/lib.rs:414-422` | +| **F7** | **4454/4455 tag deviations vs PsstPsst profile**: no `n` tag (which key is requested) or `relay` hints on 4454; no identity `p` tag on 4455; `P` tag in 4455 is the device key `E` instead of a freshly generated client key (`c` per the draft). The last one is mostly cosmetic (recipients read `P` either way) but links every transfer to your announcement. | `nip4e/src/lib.rs:426-432, 559-565` | +| **F8** | **Self-request race.** `set_encryption` races the 10044 echo against the local `set_keys` write: if the announcement you just published comes back before the DB save completes, `get_keys` fails → the *master* publishes a 4454 asking *itself*, then shows "You've requested an encryption key from another device" about its own profile. | `nip4e/src/lib.rs:342-375` vs `335` | + +## 3. Robustness / state management (P2) + +- **`announcement_existed` never resets** and the notification loop resolves `current_user` **once** at startup (`nip4e/src/lib.rs:144`) — after an account switch, 10044/4454/4455 filtering uses the stale pubkey, and a new account without an announcement never gets `NotSet`. +- **`NotSet` after a fixed 5s timer**, not EOSE — slow relays produce false "no key" prompts (`nip4e/src/lib.rs:269-283`). +- **`get_announcement` runs on *every* settings change** (including theme changes) — each call pushes 2 more tasks into `self.tasks` (never drained, grows all session) and re-creates the `wait_for_request` subscription on every `Set` (`nip4e/src/lib.rs:100-116, 295`). +- **Publish-before-save**: `create_encryption` publishes 10044 *then* saves locally; if `set_keys` fails, the key exists only in memory while 10044 advertises it → permanent unlockable requests (`nip4e/src/lib.rs:317-338`). +- Unbounded `processed_events` HashSet in the 4e notification loop (chat's version prunes at 10k; this one doesn't). + +## 4. Security considerations + +- **One-click approval with no out-of-band check.** Anyone who can get a 4454 signed by your identity (i.e., any of your devices — fine) is approved by client-name alone. PsstPsst requires comparing an 8-hex-char authorization code derived from the requester's client key. Given this event transfers your *master encryption key*, an approval-code comparison is worth copying. +- **`backup()` writes the bech32 secret key to a plaintext file** (`nip4e/src/lib.rs:222-240`). Consider at least warning in the UI. +- **`Announcement::from` falls back to `val.pubkey`** when the `n` tag is missing/malformed (`state/src/nip4e.rs:14-23`) → a malformed 10044 makes `deliver()` believe 4e is available and produce hybrid messages signed with the device key but wrapped to the identity — worst of both worlds. PsstPsst treats a missing `n` as "not a 4e peer". +- Plaintext-encrypted device key sits in kind-30078 local DB events — local-only, acceptable. + +## 5. UI/UX bugs + +| # | Finding | Location | +|---|---------|----------| +| **U1** | **The "Retry" button is a key-rotation footgun.** On `NotSet`, the notification's action calls `set_announcement(Keys::generate())`. If the `NotSet` was a false negative (F-timer) and an announcement *does* exist, clicking "Retry" silently **rotates your encryption key**, breaking decryption on all other devices until each re-requests. The label says retry; the code creates a new key. | `workspace/src/lib.rs:130-140` | +| **U2** | **Inverted empty-check** in restore: `if !content.is_empty() { set_error("cannot be empty") }` — errors when the input is *not* empty, then falls through and reports "invalid" for empty input. | `workspace/src/dialogs/restore.rs:54-56` | +| **U3** | Restore/reset republish a 10044 with a new key without warning that other devices will need to re-request (reset dialog warns about message loss — good — but restore doesn't warn at all). | `workspace/src/lib.rs:466-473` | +| **U4** | Every spurious decrypt failure from F1 surfaces as a persistent error notification. | `workspace/src/lib.rs:152-154` | + +## 6. What's already correct + +- 10044 construction matches the draft (`n` tag, replaceable kind, extra `client` tag is harmless). +- 4454 tags (`P`, `client`) match the draft; request/response events are correctly signed by the identity key, so requests are authenticated. +- Client keys are generated per install and stored in the OS keyring — reasonable. +- Local key escrow (kind 30078, NIP-44 self-encrypted, `d = "coop:device"`, never published) is a sound design; the `IDENTIFIER` value surviving the crate rename is fine (changing it would orphan stored keys). +- Dedup of relay echoes, opt-in `nip4e: false` default, and `Auto` mode degrading to the identity path when no announcement exists (this fallback is why normal DMs still work). + +## 7. Recommended fix roadmap (ordered) + +1. **P0 — fix the envelope (C1+C2+C3)**, Coop↔Coop: + - Send: always sign the seal with the **identity** signer (`GiftWrapBuilder::finalize_async(user_signer)` with `receiver = announcement E`). The device key is *not* needed to send. + - Receive: create the `DEVICE_GIFTWRAP` subscription (`#p = E`) on the user's inbox relays; in `extract_rumor`, unwrap with the **device signer** for wraps addressed to `E` (fall back to identity for wraps addressed to `A`); re-process trash when `DeviceEvent::Set` arrives. + - Keep `rumor.pubkey = identity` — the sender-mismatch check then passes by construction. +2. **P0.5 — decide the interop target**: if PsstPsst interop matters, adopt their split-key envelope (seal `n` tag = sender `E`, seal content encrypted with `ecdh(sender_e, receiver_E)`, seal signed by identity). rust-nostr's builder can't express that split today — you'd build the seal manually. +3. **P1 — sync hardening**: add `p`-tag filters, validate decrypted key against the current 10044 `n`, replace `since(now)` with a startup backfill query, reset `pending_request`, delete 4454/4455 after success, ignore (don't error on) transfers for other devices, and save locally *before* publishing. +4. **P2 — UX**: fix the Retry button to actually retry (`get_announcement`) with a separate explicit "Create key" action; fix the inverted restore check; add an approval code comparison before sharing the key. +5. **P3 — state hygiene**: reset `announcement_existed` + re-resolve `current_user` on signer change, drive `NotSet` off EOSE, bound the task vec/dedup set. + +**Validation status**: audit only, no code changed; the workspace compiled cleanly at the last `cargo check`. The rust-nostr NIP-59 behavior was verified against upstream master source (the workspace pins `nostr-sdk` to that git repo; the seal-content/signing behavior has been stable across recent versions). -- 2.54.0 From c15bd3a82da0c069c67a66523311f91e6f75d14b Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 12:54:11 +0700 Subject: [PATCH 04/10] fix P0 issues in NIP-4e gift wrap handling --- crates/chat/src/lib.rs | 117 ++++++++++++++++++++++++++++++++++------ crates/chat/src/room.rs | 71 ++---------------------- docs/nip4e-audit.md | 5 ++ 3 files changed, 112 insertions(+), 81 deletions(-) diff --git a/crates/chat/src/lib.rs b/crates/chat/src/lib.rs index 1d3a3cf2..a2e47ade 100644 --- a/crates/chat/src/lib.rs +++ b/crates/chat/src/lib.rs @@ -12,6 +12,7 @@ use gpui::{ WeakEntity, }; use instant::Duration; +use nip4e::{DeviceEvent, DeviceRegistry}; use nostr_sdk::prelude::*; use smallvec::{SmallVec, smallvec}; use state::{DEVICE_GIFTWRAP, NostrRegistry, USER_GIFTWRAP}; @@ -138,17 +139,24 @@ impl ChatRegistry { let (tx, rx) = flume::unbounded::(); let mut subscriptions = smallvec![]; - subscriptions.push( - // Subscribe to the signer event - cx.subscribe(&nostr, |this, _nostr, event, cx| { - if event.signer_changed() { - this.reset(cx); - this.handle_notifications(cx); - this.get_metadata(cx); - this.get_rooms(cx); - }; - }), - ); + let device = DeviceRegistry::global(cx); + + subscriptions.push(cx.subscribe(&nostr, |this, _nostr, event, cx| { + if event.signer_changed() { + this.reset(cx); + this.handle_notifications(cx); + this.get_metadata(cx); + this.get_rooms(cx); + }; + })); + + subscriptions.push(cx.subscribe(&device, |this, _device, event, cx| { + if matches!(event, DeviceEvent::Set) { + this.handle_notifications(cx); + this.get_messages(cx); + this.reprocess_trash(cx); + } + })); cx.defer(move |cx| { let _ = entity.update(cx, |this, cx| { @@ -183,6 +191,9 @@ impl ChatRegistry { let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); + let device = DeviceRegistry::global(cx); + let device_signer = device.read(cx).signer(cx); + let seen = self.seen.clone(); let event_map = self.event_map.clone(); let trash = self.trash.downgrade(); @@ -249,7 +260,14 @@ impl ChatRegistry { } // Extract the rumor from the gift wrap event - match extract_rumor(&client, &signer, event.as_ref()).await { + match extract_rumor( + &client, + &signer, + device_signer.as_ref(), + event.as_ref(), + ) + .await + { Ok(rumor) => { let Some(rumor_id) = rumor.id else { log::error!("Rumor missing id after ensure_id"); @@ -385,6 +403,9 @@ impl ChatRegistry { let client = nostr.read(cx).client(); let signer = nostr.read(cx).signer(); + let device = DeviceRegistry::global(cx); + let device_signer = device.read(cx).signer(cx); + self.tasks.push(cx.spawn(async move |this, cx| { let task: Task> = cx.background_spawn(async move { let public_key = signer.get_public_key_async().await?; @@ -417,6 +438,20 @@ impl ChatRegistry { client.subscribe(target).with_id(id).await?; + // Also pick up gift wraps addressed to this account's decoupled encryption key + if let Some(device_signer) = device_signer.as_ref() { + let device_public_key = device_signer.get_public_key_async().await?; + + let filter = Filter::new().kind(Kind::GiftWrap).pubkey(device_public_key); + let id = SubscriptionId::new(DEVICE_GIFTWRAP); + + let target: HashMap = nip17::extract_relay_list(&event) + .map(|relay| (relay, filter.clone())) + .collect(); + + client.subscribe(target).with_id(id).await?; + } + Ok(()) }); @@ -430,6 +465,49 @@ impl ChatRegistry { })); } + /// Retry unwrapping trashed gift wraps with the newly available encryption key. + fn reprocess_trash(&mut self, cx: &mut Context) { + let nostr = NostrRegistry::global(cx); + let client = nostr.read(cx).client(); + let signer = nostr.read(cx).signer(); + let device_signer = DeviceRegistry::global(cx).read(cx).signer(cx); + + let failed: Vec = self.trash.read(cx).iter().cloned().collect(); + + if failed.is_empty() { + return; + } + + self.tasks.push(cx.spawn(async move |this, cx| { + for message in failed { + let Ok(event) = Event::from_json(message.raw_event.as_str()) else { + continue; + }; + + if extract_rumor(&client, &signer, device_signer.as_ref(), &event) + .await + .is_err() + { + continue; + } + + let Ok(rumor) = get_rumor(&client, event.id).await else { + continue; + }; + + this.update(cx, |this, cx| { + this.trash.update(cx, |trash, cx| { + trash.remove(&message); + cx.notify(); + }); + this.new_message(NewMessage::new(event.id, rumor), cx); + })?; + } + + Ok(()) + })); + } + /// Reload the chat registry, fetching messages and contact list from relays. pub fn reload(&mut self, cx: &mut Context) { self.reset(cx); @@ -740,21 +818,30 @@ impl ChatRegistry { } /// Unwraps a gift-wrapped event and processes its contents. -async fn extract_rumor( +async fn extract_rumor( client: &Client, signer: &S, + device_signer: Option<&D>, gift_wrap: &Event, ) -> Result where S: AsyncNip44 + ?Sized, + D: AsyncNip44 + ?Sized, { // Try to get cached rumor first if let Ok(rumor) = get_rumor(client, gift_wrap.id).await { return Ok(rumor); } - // Try to unwrap with the available signer - let unwrapped = try_unwrap_with(signer, gift_wrap).await?; + let unwrapped = match try_unwrap_with(signer, gift_wrap).await { + Ok(unwrapped) => unwrapped, + Err(error) => { + let Some(device_signer) = device_signer else { + return Err(error); + }; + try_unwrap_with(device_signer, gift_wrap).await? + } + }; let mut rumor = unwrapped.rumor; // Verify rumor author matches the seal sender (as per mobile implementation) diff --git a/crates/chat/src/room.rs b/crates/chat/src/room.rs index 59c3545d..d1885300 100644 --- a/crates/chat/src/room.rs +++ b/crates/chat/src/room.rs @@ -3,7 +3,6 @@ use std::hash::{Hash, Hasher}; use anyhow::{Error, anyhow}; use common::EventExt; -use nip4e::DeviceRegistry; use gpui::{App, AppContext, Context, EventEmitter, SharedString, Task}; use instant::Duration; use itertools::Itertools; @@ -15,7 +14,6 @@ use state::{NostrRegistry, TIMEOUT}; use crate::{FileAttachment, KIND_FILE_MESSAGE, NewMessage}; const NO_DEKEY: &str = "User hasn't set up a decoupled encryption key yet."; -const USER_NO_DEKEY: &str = "You haven't set up a decoupled encryption key or it's not available."; /// The outcome of delivering an encrypted rumor to a single receiver. #[derive(Debug, Clone)] @@ -472,35 +470,10 @@ impl Room { tags } - /// Select the appropriate signer based on signer kind and available keys. - fn select_signer<'a, S>( - signer_kind: &SignerKind, - has_announcement: bool, - encryption_signer: &'a Option, - user_signer: &'a S, - ) -> &'a S { - match signer_kind { - SignerKind::Auto => { - if has_announcement { - encryption_signer.as_ref().unwrap_or(user_signer) - } else { - user_signer - } - } - SignerKind::Encryption => encryption_signer - .as_ref() - .expect("encryption signer must be set"), - SignerKind::User => user_signer, - } - } - /// Send the rumor to every member's messaging relays. pub fn send(&self, rumor: UnsignedEvent, cx: &App) -> Option>> { let config = self.config.clone(); - let device = DeviceRegistry::global(cx); - let encryption_signer = device.read(cx).signer(cx); - let nostr = NostrRegistry::global(cx); let client = nostr.read(cx).client(); let user_signer = nostr.read(cx).signer(); @@ -522,31 +495,14 @@ impl Room { let signer_kind = config.signer_kind(); // Deliver to every receiver concurrently - let sends = receivers.into_iter().map(|receiver| { - deliver( - &client, - signer_kind, - receiver, - &rumor, - &encryption_signer, - &user_signer, - ) - }); + let sends = receivers + .into_iter() + .map(|receiver| deliver(&client, signer_kind, receiver, &rumor, &user_signer)); let mut reports: Vec = futures::future::join_all(sends).await; // Back up the message to ourselves once at least one receiver got it if config.backup() && reports.iter().any(SendReport::success) { - reports.push( - deliver( - &client, - signer_kind, - sender, - &rumor, - &encryption_signer, - &user_signer, - ) - .await, - ); + reports.push(deliver(&client, signer_kind, sender, &rumor, &user_signer).await); } reports @@ -560,7 +516,6 @@ async fn deliver( signer_kind: &SignerKind, receiver: Person, rumor: &UnsignedEvent, - encryption_signer: &Option, user_signer: &S, ) -> SendReport where @@ -577,25 +532,9 @@ where result: Err(NO_DEKEY.into()), }; } - - // Sender didn't set up a decoupled encryption key - if encryption_signer.is_none() { - return SendReport { - receiver: public_key, - result: Err(USER_NO_DEKEY.into()), - }; - } } - // Determine the signer to use - let signer = Room::select_signer( - signer_kind, - announcement.is_some(), - encryption_signer, - user_signer, - ); - - match send_gift_wrap(client, signer, &receiver, rumor, signer_kind).await { + match send_gift_wrap(client, user_signer, &receiver, rumor, signer_kind).await { Ok(output) => SendReport { receiver: public_key, result: Ok(output), diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md index 5f32a8d4..82f407d2 100644 --- a/docs/nip4e-audit.md +++ b/docs/nip4e-audit.md @@ -1,5 +1,10 @@ # NIP-4e Audit Report +> **Update**: The P0 findings (C1, C2, C3) are now 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. + **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). ## Executive summary -- 2.54.0 From bdf9249eabd6fba5ccc65c52dabd6452614f21c8 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:02:24 +0700 Subject: [PATCH 05/10] 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). -- 2.54.0 From 0bae05131666052abdaa97d5a354ef9b8d3146f2 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:10:15 +0700 Subject: [PATCH 06/10] 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). -- 2.54.0 From a8df0df0cd8708234f438899d1eb2fea92a0c50d Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:15:48 +0700 Subject: [PATCH 07/10] show authorization code during device key approval --- crates/nip4e/src/lib.rs | 45 +++++++++++++++++++------------------ crates/state/src/nip4e.rs | 31 +++++-------------------- crates/workspace/src/lib.rs | 9 ++++---- docs/nip4e-audit.md | 10 ++++++--- 4 files changed, 41 insertions(+), 54 deletions(-) diff --git a/crates/nip4e/src/lib.rs b/crates/nip4e/src/lib.rs index be189731..b08ffe52 100644 --- a/crates/nip4e/src/lib.rs +++ b/crates/nip4e/src/lib.rs @@ -39,7 +39,7 @@ pub enum DeviceEvent { /// User have not setup encryption key NotSet, /// The device is requesting an encryption key - Requesting, + Requesting(SharedString), /// An error occurred Error(SharedString), } @@ -319,10 +319,7 @@ impl DeviceRegistry { cx.background_spawn(async move { // Construct an announcement event let event = EventBuilder::new(Kind::Custom(10044), "") - .tags(vec![ - Tag::custom("n", vec![n]), - Tag::custom("client", vec![CLIENT_NAME]), - ]) + .tag(Tag::custom("n", vec![n])) .finalize_async(&signer) .await?; @@ -433,10 +430,7 @@ impl DeviceRegistry { // 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]), - ]) + .tags(vec![Tag::custom("P", vec![app_pubkey])]) .finalize_async(&signer) .await?; @@ -475,8 +469,6 @@ impl DeviceRegistry { let signer = nostr.read(cx).signer(); let app_keys_task = get_or_init_app_keys(cx); - cx.emit(DeviceEvent::Requesting); - self.tasks.push(cx.spawn(async move |this, cx| { let app_keys = match app_keys_task.await { Ok(app_keys) => app_keys, @@ -488,6 +480,12 @@ impl DeviceRegistry { } }; + this.update(cx, |_this, cx| { + cx.emit(DeviceEvent::Requesting(authorization_code( + &app_keys.public_key(), + ))); + })?; + let task: Task> = cx.background_spawn(async move { let public_key = signer.get_public_key_async().await?; @@ -694,7 +692,9 @@ impl DeviceRegistry { /// Build a notification for the encryption request. 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."; + Approve to allow Coop to share with it. Before approving, \ + compare the code with the one shown on the requesting \ + device."; let requester = event .tags @@ -704,16 +704,10 @@ impl DeviceRegistry { .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 code = authorization_code(&requester); let persons = PersonRegistry::global(cx); let profile = persons.read(cx).get(&requester, cx); + let code_label = code.clone(); let entity = cx.entity().downgrade(); let loading = Rc::new(Cell::new(false)); @@ -776,7 +770,7 @@ impl DeviceRegistry { .font_semibold() .text_xs() .text_color(cx.theme().text_muted) - .child(SharedString::from("Client:")), + .child(SharedString::from("Code:")), ) .child( div() @@ -785,7 +779,7 @@ impl DeviceRegistry { .px_2() .rounded(cx.theme().radius) .bg(cx.theme().elevated_surface_background) - .child(request.client_name()), + .child(code_label.clone()), ), ), ) @@ -817,6 +811,13 @@ impl DeviceRegistry { struct DeviceNotification; +/// The first eight hexadecimal characters of the client public key. +fn authorization_code(public_key: &PublicKey) -> SharedString { + let hex = public_key.to_hex(); + let code = hex[..8].to_uppercase(); + SharedString::from(format!("{} {}", &code[..4], &code[4..])) +} + /// Get or create new app keys (async, returns a task) fn get_or_init_app_keys(cx: &App) -> Task> { let read = cx.read_credentials(CLIENT_NAME); diff --git a/crates/state/src/nip4e.rs b/crates/state/src/nip4e.rs index dbb18804..179e863f 100644 --- a/crates/state/src/nip4e.rs +++ b/crates/state/src/nip4e.rs @@ -1,4 +1,3 @@ -use gpui::SharedString; use nostr_sdk::prelude::*; /// Announcement @@ -6,12 +5,12 @@ use nostr_sdk::prelude::*; pub struct Announcement { /// The public key of the device that created this announcement. public_key: PublicKey, - - /// The name of the device that created this announcement. - client_name: Option, } impl Announcement { + /// Parse an announcement from an event, returning `None` when the `n` + /// tag is missing or malformed so callers never mistake the author's + /// identity key for an encryption key. pub fn from_event(event: &Event) -> Option { let public_key = event .tags @@ -20,33 +19,15 @@ impl Announcement { .and_then(|tag| tag.content()) .and_then(|c| PublicKey::parse(c).ok())?; - let client_name = event - .tags - .iter() - .find(|tag| tag.kind() == "client") - .and_then(|tag| tag.content()) - .map(|c| c.to_string()); - - Some(Self::new(public_key, client_name)) + Some(Self { public_key }) } - pub fn new(public_key: PublicKey, client_name: Option) -> Self { - Self { - public_key, - client_name, - } + pub fn new(public_key: PublicKey) -> Self { + Self { public_key } } /// Returns the public key of the device that created this announcement. pub fn public_key(&self) -> PublicKey { self.public_key } - - /// Returns the client name of the device that created this announcement. - pub fn client_name(&self) -> SharedString { - self.client_name - .as_ref() - .map(SharedString::from) - .unwrap_or(SharedString::from("Unknown")) - } } diff --git a/crates/workspace/src/lib.rs b/crates/workspace/src/lib.rs index f4ecac10..8135dd4e 100644 --- a/crates/workspace/src/lib.rs +++ b/crates/workspace/src/lib.rs @@ -106,14 +106,15 @@ impl Workspace { // Observe all events emitted by the device registry cx.subscribe_in(&device, window, |_this, _device, event, window, cx| { match event { - DeviceEvent::Requesting => { - const MSG: &str = - "Please open other client and approve the request for encryption key."; + DeviceEvent::Requesting(code) => { + const MSG: &str = "Please open another client and approve the request \ + for the encryption key. Compare the code with the \ + one shown there before approving."; let note = Notification::new() .id::() .autohide(false) - .title("Wait for approval") + .title(format!("Wait for approval · {code}")) .message(MSG) .with_kind(NotificationKind::Info); diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md index 99573565..0eb70499 100644 --- a/docs/nip4e-audit.md +++ b/docs/nip4e-audit.md @@ -23,9 +23,13 @@ > 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). +> inverted empty-key check is fixed. Additionally, the `client` tag and +> `client_name` were dropped from announcements/requests, and the PsstPsst +> authorization code was adapted: both sides display the first 8 hex chars of +> the requester client pubkey (`XXXX XXXX`) — the approver in the request +> notification, the requester in the "Wait for approval" notification — so the +> transfer can be verified out of band before approving. Still open: the +> plaintext backup file in section 4. **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). -- 2.54.0 From 758ffaf7d952268586f9b1906f32b6ed2990a693 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:27:43 +0700 Subject: [PATCH 08/10] build NIP-59 seals manually for nip4e interop --- Cargo.lock | 1 + Cargo.toml | 2 +- crates/chat/Cargo.toml | 1 + crates/chat/src/lib.rs | 10 +++- crates/chat/src/room.rs | 116 +++++++++++++++++++++++++++++++++++----- docs/nip4e-audit.md | 12 +++++ 6 files changed, 127 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c9f72605..a5710ca7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1083,6 +1083,7 @@ dependencies = [ "nip4e", "nostr-sdk", "person", + "rand 0.10.3", "settings", "smallvec", "smol", diff --git a/Cargo.toml b/Cargo.toml index 8ca3bc30..c17f1307 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,7 +44,7 @@ hkdf = "0.12" chacha20 = "0.9" hmac = "0.12" # Pinned to the instance `nostr-sdk` already builds, so NIP-44 nonces share it -rand = { version = "0.10", default-features = false, features = [ "std", "sys_rng" ] } +rand = { version = "0.10", default-features = false, features = [ "std", "sys_rng", "thread_rng" ] } # Others anyhow = "1.0.44" diff --git a/crates/chat/Cargo.toml b/crates/chat/Cargo.toml index e0dcd9f7..efc4f0e4 100644 --- a/crates/chat/Cargo.toml +++ b/crates/chat/Cargo.toml @@ -17,6 +17,7 @@ instant.workspace = true anyhow.workspace = true itertools.workspace = true +rand.workspace = true smallvec.workspace = true log.workspace = true flume.workspace = true diff --git a/crates/chat/src/lib.rs b/crates/chat/src/lib.rs index a2e47ade..98c14cdd 100644 --- a/crates/chat/src/lib.rs +++ b/crates/chat/src/lib.rs @@ -874,9 +874,17 @@ where let seal: Event = Event::from_json(seal)?; seal.verify()?; + let sender_encryption_key = seal + .tags + .iter() + .find(|tag| tag.kind() == "n") + .and_then(|tag| tag.content()) + .and_then(|content| PublicKey::parse(content).ok()) + .unwrap_or(seal.pubkey); + // Get the rumor event let rumor = signer - .nip44_decrypt_async(&seal.pubkey, &seal.content) + .nip44_decrypt_async(&sender_encryption_key, &seal.content) .await?; let rumor = UnsignedEvent::from_json(rumor)?; diff --git a/crates/chat/src/room.rs b/crates/chat/src/room.rs index d1885300..4c031a1c 100644 --- a/crates/chat/src/room.rs +++ b/crates/chat/src/room.rs @@ -6,14 +6,16 @@ use common::EventExt; use gpui::{App, AppContext, Context, EventEmitter, SharedString, Task}; use instant::Duration; use itertools::Itertools; +use nip4e::DeviceRegistry; use nostr_sdk::prelude::*; use person::{Person, PersonRegistry}; use settings::{RoomConfig, SignerKind}; -use state::{NostrRegistry, TIMEOUT}; +use state::{NostrRegistry, TIMEOUT, UniversalSigner}; use crate::{FileAttachment, KIND_FILE_MESSAGE, NewMessage}; const NO_DEKEY: &str = "User hasn't set up a decoupled encryption key yet."; +const USER_NO_DEKEY: &str = "You haven't set up a decoupled encryption key yet. Set one up in the settings to message them."; /// The outcome of delivering an encrypted rumor to a single receiver. #[derive(Debug, Clone)] @@ -474,6 +476,9 @@ impl Room { pub fn send(&self, rumor: UnsignedEvent, cx: &App) -> Option>> { let config = self.config.clone(); + let device = DeviceRegistry::global(cx); + let encryption_signer = device.read(cx).signer(cx); + let nostr = NostrRegistry::global(cx); let client = nostr.read(cx).client(); let user_signer = nostr.read(cx).signer(); @@ -495,14 +500,33 @@ impl Room { let signer_kind = config.signer_kind(); // Deliver to every receiver concurrently - let sends = receivers - .into_iter() - .map(|receiver| deliver(&client, signer_kind, receiver, &rumor, &user_signer)); + let sends = receivers.into_iter().map(|receiver| { + deliver( + &client, + signer_kind, + receiver, + &rumor, + encryption_signer.as_ref(), + &user_signer, + ) + }); + let mut reports: Vec = futures::future::join_all(sends).await; + let minimum_success = reports.iter().any(SendReport::success); // Back up the message to ourselves once at least one receiver got it - if config.backup() && reports.iter().any(SendReport::success) { - reports.push(deliver(&client, signer_kind, sender, &rumor, &user_signer).await); + if config.backup() && minimum_success { + reports.push( + deliver( + &client, + signer_kind, + sender, + &rumor, + encryption_signer.as_ref(), + &user_signer, + ) + .await, + ); } reports @@ -516,6 +540,7 @@ async fn deliver( signer_kind: &SignerKind, receiver: Person, rumor: &UnsignedEvent, + encryption_signer: Option<&UniversalSigner>, user_signer: &S, ) -> SendReport where @@ -532,9 +557,26 @@ where result: Err(NO_DEKEY.into()), }; } + + // Sender didn't set up a decoupled encryption key + if encryption_signer.is_none() { + return SendReport { + receiver: public_key, + result: Err(USER_NO_DEKEY.into()), + }; + } } - match send_gift_wrap(client, user_signer, &receiver, rumor, signer_kind).await { + match send_gift_wrap( + client, + &receiver, + rumor, + signer_kind, + encryption_signer, + user_signer, + ) + .await + { Ok(output) => SendReport { receiver: public_key, result: Ok(output), @@ -549,10 +591,11 @@ where /// Build the gift-wrapped event for a rumor and send it. async fn send_gift_wrap( client: &Client, - signer: &S, receiver: &Person, rumor: &UnsignedEvent, config: &SignerKind, + encryption_signer: Option<&UniversalSigner>, + user_signer: &S, ) -> Result, Error> where S: AsyncGetPublicKey + AsyncSignEvent + AsyncNip44, @@ -578,11 +621,50 @@ where SignerKind::User => receiver.public_key(), }; - // Construct the gift wrap event - let event = nip59::GiftWrapBuilder::new(receiver_key, rumor.clone()) - .extra_tags(extra_tags) - .finalize_async(signer) - .await?; + // Make sure the rumor carries an event ID before it's serialized + let mut rumor = rumor.clone(); + rumor.ensure_id(); + + let rumor_json = rumor.as_json(); + + let seal: Event = match encryption_signer { + Some(encryption_signer) => { + let encryption_pubkey = encryption_signer.get_public_key_async().await?; + let content = encryption_signer + .nip44_encrypt_async(&receiver_key, &rumor_json) + .await?; + + EventBuilder::new(Kind::Seal, content) + .tags(vec![Tag::custom("n", [encryption_pubkey.to_hex()])]) + .custom_created_at(tweaked_timestamp()) + .finalize_async(user_signer) + .await? + } + None => { + let content = user_signer + .nip44_encrypt_async(&receiver_key, &rumor_json) + .await?; + + EventBuilder::new(Kind::Seal, content) + .custom_created_at(tweaked_timestamp()) + .finalize_async(user_signer) + .await? + } + }; + + let ephemeral_keys = Keys::generate(); + let content = nip44::encrypt( + ephemeral_keys.secret_key(), + &receiver_key, + seal.as_json(), + nip44::Version::default(), + )?; + extra_tags.push(Tag::public_key(receiver_key)); + + let event = EventBuilder::new(Kind::GiftWrap, content) + .tags(extra_tags) + .custom_created_at(tweaked_timestamp()) + .finalize(&ephemeral_keys)?; // Send to the receiver's NIP-17 relays. client @@ -591,3 +673,11 @@ where .await .map_err(Into::into) } + +/// A tweaked timestamp to thwart time-analysis attacks, per NIP-59 +fn tweaked_timestamp() -> Timestamp { + const MAX_TWEAK_SECONDS: u64 = 2 * 24 * 60 * 60; + + let seconds = rand::random_range(0..=MAX_TWEAK_SECONDS); + Timestamp::from_secs(Timestamp::now().as_secs().saturating_sub(seconds)) +} diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md index 0eb70499..4e6b5f78 100644 --- a/docs/nip4e-audit.md +++ b/docs/nip4e-audit.md @@ -30,6 +30,18 @@ > notification, the requester in the "Wait for approval" notification — so the > transfer can be verified out of band before approving. Still open: the > plaintext backup file in section 4. +> +> **Update 4**: Verified against the PsstPsst source (nip17-gift-wrap.ts, +> encryption-key.service.ts) and closed the remaining envelope incompatibility. +> Coop now emits PsstPsst's split-key NIP-59 format when the sender has an +> encryption key: the seal (kind 13) is signed by the identity, the rumor +> content is NIP-44'd under the sender's encryption key, and the seal carries +> the sender's encryption pubkey in an `n` tag; the wrap uses a one-shot +> ephemeral key. Receiving supports both formats: the seal's `n` tag selects +> the content-decryption key, falling back to the seal author for legacy +> unified seals. Interop matrix is now green in all four directions, with one +> caveat: a Coop sender without an encryption key degrades to the legacy +> format, which PsstPsst ignores on its relay intake path. **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). -- 2.54.0 From 5cceaa0fc86ab8e5c070f35cd342dfc4cf418eb1 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:28:41 +0700 Subject: [PATCH 09/10] clean up --- docs/nip4e-audit.md | 154 -------------------------------------------- 1 file changed, 154 deletions(-) delete mode 100644 docs/nip4e-audit.md diff --git a/docs/nip4e-audit.md b/docs/nip4e-audit.md deleted file mode 100644 index 4e6b5f78..00000000 --- a/docs/nip4e-audit.md +++ /dev/null @@ -1,154 +0,0 @@ -# NIP-4e Audit Report - -> **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`. -> -> **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). -> -> **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. Additionally, the `client` tag and -> `client_name` were dropped from announcements/requests, and the PsstPsst -> authorization code was adapted: both sides display the first 8 hex chars of -> the requester client pubkey (`XXXX XXXX`) — the approver in the request -> notification, the requester in the "Wait for approval" notification — so the -> transfer can be verified out of band before approving. Still open: the -> plaintext backup file in section 4. -> -> **Update 4**: Verified against the PsstPsst source (nip17-gift-wrap.ts, -> encryption-key.service.ts) and closed the remaining envelope incompatibility. -> Coop now emits PsstPsst's split-key NIP-59 format when the sender has an -> encryption key: the seal (kind 13) is signed by the identity, the rumor -> content is NIP-44'd under the sender's encryption key, and the seal carries -> the sender's encryption pubkey in an `n` tag; the wrap uses a one-shot -> ephemeral key. Receiving supports both formats: the seal's `n` tag selects -> the content-decryption key, falling back to the seal author for legacy -> unified seals. Interop matrix is now green in all four directions, with one -> caveat: a Coop sender without an encryption key degrades to the legacy -> format, which PsstPsst ignores on its relay intake path. - -**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). - -## Executive summary - -The key-sync side (kinds 10044/4454/4455) is a reasonable sketch of the spec, but **the actual message encryption is inverted and non-functional end-to-end**: - -- **Sending** seals/signs with the **device encryption key** (should be the identity key). -- **Receiving** unwraps gift wraps with the **identity key** (should be the device key when the wrap is addressed to your encryption key `E`). - -Every NIP-4e message between two Coop clients fails at decryption; messages to/from PsstPsst fail in both directions. On top of that there are ~10 sync/UX bugs that make even key distribution unreliable. - -## 1. Critical — the message path is broken (P0) - -rust-nostr's `GiftWrapBuilder::finalize_async(signer)` (verified against upstream source): - -- Seal **content** = `nip44_encrypt(conversation_key(signer_sk, receiver_pk), rumor)` -- Seal is **signed by `signer`** → `seal.pubkey = signer_pk` -- Gift wrap = `nip44_encrypt(conversation_key(ephemeral_sk, receiver_pk), seal)` -- Unwrap requires the receiver's secret matching `receiver_pk`; then rust-nostr **rejects if `rumor.pubkey != seal.pubkey`** ("sender mismatch"). - -NIP-4e draft says the *sender* keeps using their **identity key** (`ecdh(b, E)` — Bob's `b`) and only the *target* changes to `E`. So: - -| # | Finding | Location | -|---|---------|----------| -| **C1** | **Send path signs the seal with the device key.** `select_signer` returns the encryption signer, and `GiftWrapBuilder::finalize_async(device_signer)` signs the seal with `E` while `rumor.pubkey` = identity (`Room::rumor` uses `current_user`). Result: (a) receivers can't attribute the sender, (b) Coop's own `rumor.pubkey != seal.pubkey` check rejects it, (c) PsstPsst verifies the seal against the sender's *identity* key → invalid. | `crates/chat/src/room.rs:482-495, 590-596, 643-646`; rumor pubkey at `room.rs:404-417` | -| **C2** | **Receive path unwraps with the identity key.** `handle_notifications` captures the *user* signer and passes it to `extract_rumor` → `nip44_decrypt(ecdh(a, wrap_pubkey))`. A 4e wrap is encrypted to your `E`, so decryption always fails → "Failed to extract rumor" → trash. The correct unwrap key for 4e messages is the **device key** (`ecdh(e, wrap_pubkey)`), then `ecdh(e, seal.pubkey)` for the rumor — both succeed with the device signer in rust-nostr's unwrapper. | `crates/chat/src/lib.rs:184, 252, 743-801` | -| **C3** | **The `DEVICE_GIFTWRAP` subscription is never created.** The constant exists (`state/src/constants.rs:24`), its ID is used for EOSE matching (`chat/src/lib.rs:190`), but no code ever `subscribe()`s with it (presumably intended: `#p = E` filter on inbox relays). Only `USER_GIFTWRAP` (`#p = A`) exists. | `crates/chat/src/lib.rs:190-191, 410-418` | - -**Consequences**: - -| Path | Status | -|------|--------| -| Coop → Coop (4e) | ❌ Fails at receiver wrap-decryption (C2); even with a device-key unwrap it fails the sender-mismatch check (C1) | -| Coop → PsstPsst | ❌ PsstPsst unwraps fine (wrap → `E`), but the seal signature doesn't verify against your identity key and there's no seal `n` tag → rejected | -| PsstPsst → Coop | ❌ Wrap routed via identity `p` tag arrives, but Coop unwraps with the identity key → fail (C2) | -| Coop → self backup copy | ❌ Same broken envelope, so cross-device history sync also fails | -| Non-4e (`SignerKind::User`) | ✅ Works — the bug only bites when 4e is enabled, which is why it looks "half-baked" rather than fully broken | - -> Note on interop targets: the **draft** wants `seal signed by identity, content = ecdh(identity_sk, receiver_E)` — which is exactly `GiftWrapBuilder::finalize_async(user_signer)` with `receiver = E`. **PsstPsst's profile** goes further: seal signed by identity **plus** an `n` tag with the sender's `E`, content encrypted with `ecdh(sender_e, receiver_E)`. So minimal correctness (Coop↔Coop) = swap the signers; PsstPsst interop additionally requires implementing their split-key envelope (`n` tag, sender-`e` content key). - -```mermaid -sequenceDiagram - participant S as Sender (Coop) - participant R as Receiver (Coop) - Note over S: TODAY (broken)
seal signed by device key Es
wrap → Er, unwrap with identity a - S->>R: gift wrap (→ Er) - R->>R: nip44_decrypt(a, wrap) ✗ FAILS - Note over S: CORRECT (draft)
seal signed by identity As
wrap → Er, unwrap with device e - S->>R: gift wrap (→ Er) - R->>R: nip44_decrypt(e, wrap) ✓
nip44_decrypt(e, seal.pubkey) ✓ -``` - -## 2. Key-sync bugs (P1) - -| # | Finding | Location | -|---|---------|----------| -| **F1** | **No `p`-tag filter on 4455.** `wait_for_approval` subscribes to *all* your 4455s (`author=me, since=now`). `extract_encryption` then tries to decrypt transfers meant for *other* devices → NIP-44 failure → `DeviceEvent::Error` → error notification. The approving device also picks up its own just-published 4455 and errors on it. Spec/PsstPsst: filter by `p = own client key` and *ignore* undecryptable responses. | `nip4e/src/lib.rs:476-482, 163-165, 489-526` | -| **F2** | **No validation of the received key.** After decrypting a 4455, the key is installed without comparing `derived_pubkey` to the current 10044 `n` value. A stale 4455 (pre-rotation) silently installs a dead key. PsstPsst: "accepts the key only when that public key equals the current kind 10044 `n` value". | `nip4e/src/lib.rs:489-526` | -| **F3** | **`since(Timestamp::now())` on both subscriptions.** The approver misses all 4454s published while it was offline (the draft says Cope "looks for **all** 4454 events" — i.e. backfill). The requester likewise misses 4455s published while it was offline; recovery depends on LMDB having synced them. This is the main "works only when both apps are open at the same time" behavior. | `nip4e/src/lib.rs:388-394, 476-482` | -| **F4** | **`pending_request` is never reset to false.** After the first approval in a session, every subsequent device request is silently dropped (`ask_for_approval` early-returns). | `nip4e/src/lib.rs:597-601` (no `set_pending_request(false)` anywhere) | -| **F5** | **No deletion after success.** The draft explicitly says the requester deletes its 4454 + the received 4455 (NIP-09). Coop leaves them on relays forever, and combined with F6 below, stale events get re-applied on later launches. | absent | -| **F6** | **`request()` short-circuits on *any* old 4455 in the local DB** (`limit(1)`, no recency check, no decrypt-check before committing to it) — stale after rotation. | `nip4e/src/lib.rs:414-422` | -| **F7** | **4454/4455 tag deviations vs PsstPsst profile**: no `n` tag (which key is requested) or `relay` hints on 4454; no identity `p` tag on 4455; `P` tag in 4455 is the device key `E` instead of a freshly generated client key (`c` per the draft). The last one is mostly cosmetic (recipients read `P` either way) but links every transfer to your announcement. | `nip4e/src/lib.rs:426-432, 559-565` | -| **F8** | **Self-request race.** `set_encryption` races the 10044 echo against the local `set_keys` write: if the announcement you just published comes back before the DB save completes, `get_keys` fails → the *master* publishes a 4454 asking *itself*, then shows "You've requested an encryption key from another device" about its own profile. | `nip4e/src/lib.rs:342-375` vs `335` | - -## 3. Robustness / state management (P2) - -- **`announcement_existed` never resets** and the notification loop resolves `current_user` **once** at startup (`nip4e/src/lib.rs:144`) — after an account switch, 10044/4454/4455 filtering uses the stale pubkey, and a new account without an announcement never gets `NotSet`. -- **`NotSet` after a fixed 5s timer**, not EOSE — slow relays produce false "no key" prompts (`nip4e/src/lib.rs:269-283`). -- **`get_announcement` runs on *every* settings change** (including theme changes) — each call pushes 2 more tasks into `self.tasks` (never drained, grows all session) and re-creates the `wait_for_request` subscription on every `Set` (`nip4e/src/lib.rs:100-116, 295`). -- **Publish-before-save**: `create_encryption` publishes 10044 *then* saves locally; if `set_keys` fails, the key exists only in memory while 10044 advertises it → permanent unlockable requests (`nip4e/src/lib.rs:317-338`). -- Unbounded `processed_events` HashSet in the 4e notification loop (chat's version prunes at 10k; this one doesn't). - -## 4. Security considerations - -- **One-click approval with no out-of-band check.** Anyone who can get a 4454 signed by your identity (i.e., any of your devices — fine) is approved by client-name alone. PsstPsst requires comparing an 8-hex-char authorization code derived from the requester's client key. Given this event transfers your *master encryption key*, an approval-code comparison is worth copying. -- **`backup()` writes the bech32 secret key to a plaintext file** (`nip4e/src/lib.rs:222-240`). Consider at least warning in the UI. -- **`Announcement::from` falls back to `val.pubkey`** when the `n` tag is missing/malformed (`state/src/nip4e.rs:14-23`) → a malformed 10044 makes `deliver()` believe 4e is available and produce hybrid messages signed with the device key but wrapped to the identity — worst of both worlds. PsstPsst treats a missing `n` as "not a 4e peer". -- Plaintext-encrypted device key sits in kind-30078 local DB events — local-only, acceptable. - -## 5. UI/UX bugs - -| # | Finding | Location | -|---|---------|----------| -| **U1** | **The "Retry" button is a key-rotation footgun.** On `NotSet`, the notification's action calls `set_announcement(Keys::generate())`. If the `NotSet` was a false negative (F-timer) and an announcement *does* exist, clicking "Retry" silently **rotates your encryption key**, breaking decryption on all other devices until each re-requests. The label says retry; the code creates a new key. | `workspace/src/lib.rs:130-140` | -| **U2** | **Inverted empty-check** in restore: `if !content.is_empty() { set_error("cannot be empty") }` — errors when the input is *not* empty, then falls through and reports "invalid" for empty input. | `workspace/src/dialogs/restore.rs:54-56` | -| **U3** | Restore/reset republish a 10044 with a new key without warning that other devices will need to re-request (reset dialog warns about message loss — good — but restore doesn't warn at all). | `workspace/src/lib.rs:466-473` | -| **U4** | Every spurious decrypt failure from F1 surfaces as a persistent error notification. | `workspace/src/lib.rs:152-154` | - -## 6. What's already correct - -- 10044 construction matches the draft (`n` tag, replaceable kind, extra `client` tag is harmless). -- 4454 tags (`P`, `client`) match the draft; request/response events are correctly signed by the identity key, so requests are authenticated. -- Client keys are generated per install and stored in the OS keyring — reasonable. -- Local key escrow (kind 30078, NIP-44 self-encrypted, `d = "coop:device"`, never published) is a sound design; the `IDENTIFIER` value surviving the crate rename is fine (changing it would orphan stored keys). -- Dedup of relay echoes, opt-in `nip4e: false` default, and `Auto` mode degrading to the identity path when no announcement exists (this fallback is why normal DMs still work). - -## 7. Recommended fix roadmap (ordered) - -1. **P0 — fix the envelope (C1+C2+C3)**, Coop↔Coop: - - Send: always sign the seal with the **identity** signer (`GiftWrapBuilder::finalize_async(user_signer)` with `receiver = announcement E`). The device key is *not* needed to send. - - Receive: create the `DEVICE_GIFTWRAP` subscription (`#p = E`) on the user's inbox relays; in `extract_rumor`, unwrap with the **device signer** for wraps addressed to `E` (fall back to identity for wraps addressed to `A`); re-process trash when `DeviceEvent::Set` arrives. - - Keep `rumor.pubkey = identity` — the sender-mismatch check then passes by construction. -2. **P0.5 — decide the interop target**: if PsstPsst interop matters, adopt their split-key envelope (seal `n` tag = sender `E`, seal content encrypted with `ecdh(sender_e, receiver_E)`, seal signed by identity). rust-nostr's builder can't express that split today — you'd build the seal manually. -3. **P1 — sync hardening**: add `p`-tag filters, validate decrypted key against the current 10044 `n`, replace `since(now)` with a startup backfill query, reset `pending_request`, delete 4454/4455 after success, ignore (don't error on) transfers for other devices, and save locally *before* publishing. -4. **P2 — UX**: fix the Retry button to actually retry (`get_announcement`) with a separate explicit "Create key" action; fix the inverted restore check; add an approval code comparison before sharing the key. -5. **P3 — state hygiene**: reset `announcement_existed` + re-resolve `current_user` on signer change, drive `NotSet` off EOSE, bound the task vec/dedup set. - -**Validation status**: audit only, no code changed; the workspace compiled cleanly at the last `cargo check`. The rust-nostr NIP-59 behavior was verified against upstream master source (the workspace pins `nostr-sdk` to that git repo; the seal-content/signing behavior has been stable across recent versions). -- 2.54.0 From 417829dbe92f9376cc10ff8d1425abddf7fba998 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 4 Oct 2026 13:32:33 +0700 Subject: [PATCH 10/10] only seal gift wraps when the receiver announced encryption support --- crates/chat/src/room.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/chat/src/room.rs b/crates/chat/src/room.rs index 4c031a1c..691d8c29 100644 --- a/crates/chat/src/room.rs +++ b/crates/chat/src/room.rs @@ -626,9 +626,10 @@ where rumor.ensure_id(); let rumor_json = rumor.as_json(); + let has_encryption_announcement = receiver.announcement().is_some(); - let seal: Event = match encryption_signer { - Some(encryption_signer) => { + let seal: Event = match (encryption_signer, has_encryption_announcement) { + (Some(encryption_signer), true) => { let encryption_pubkey = encryption_signer.get_public_key_async().await?; let content = encryption_signer .nip44_encrypt_async(&receiver_key, &rumor_json) @@ -640,7 +641,7 @@ where .finalize_async(user_signer) .await? } - None => { + _ => { let content = user_signer .nip44_encrypt_async(&receiver_key, &rumor_json) .await?; -- 2.54.0