From d101b8e2362190ffb53ee95529fcae50490b4c90 Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 10 Sep 2026 12:50:07 -0500 Subject: [PATCH 01/74] fix(undo): restore full profile with secret key on undo-delete --- src/app.rs | 92 ++++++++++++++++++++++++++++++++++++------------- src/ipc.rs | 4 +-- src/main.rs | 62 +++++++++------------------------ src/profiles.rs | 36 ++++++++++++++----- 4 files changed, 114 insertions(+), 80 deletions(-) diff --git a/src/app.rs b/src/app.rs index b6f9372..f58875e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -22,8 +22,10 @@ pub struct App { pub settings: Settings, /// Derived vault key, present only while the encrypted vault is unlocked. unlock_key: Option, - /// Stack of deleted profiles for undo functionality. - pub undo_history: Vec, + /// Stack of deleted profiles for undo functionality. Holds the full + /// stored record (including secret key material, exactly as it was on + /// disk) so undo restores a working profile, not an empty shell. + pub undo_history: Vec, /// The most recent publish report, persisted across restarts. pub last_publish: Option, /// Active signer mode. @@ -60,7 +62,8 @@ pub struct AppStateView { pub active_profile: Option, pub profiles: Vec, pub settings: Settings, - /// Recently deleted profiles, newest last, for undo. + /// Recently deleted profiles (safe summaries only — never secret material), + /// newest last, for undo. #[serde(skip_serializing_if = "Vec::is_empty")] pub undo_history: Vec, /// The most recent publish report, persisted across restarts. @@ -215,36 +218,45 @@ impl App { Ok(revealed) } - /// Undo the last profile deletion, restoring the profile to the vault. + /// Undo the last profile deletion, restoring the profile — with its real + /// stored secret key — to the vault. /// Returns the restored profile summary, or an error if there is no undo history. pub fn undo_delete(&mut self) -> Result { - if self.undo_history.is_empty() { + let Some(deleted) = self.undo_history.pop() else { return Err(AppError::config("No profile deletions to undo.")); - } - let restored = self.undo_history.pop().unwrap(); - // Re-add the profile to the vault + }; + let restored = deleted.stored.clone(); + // Re-add the profile to the vault unless it is somehow already there. if !self .vault .profiles .iter() - .any(|p| p.public_key == restored.npub) + .any(|p| p.public_key == restored.public_key) { - let stored = StoredProfile { - label: restored.label.clone(), - public_key: restored.npub.clone(), - secret_key: "".to_string(), - created_at: restored.created_at, - picture: restored.picture.clone(), - nip05: restored.nip05.clone(), - signer_mode: SignerMode::Embedded, - }; - self.vault.profiles.push(stored); - // If no active profile, this restored one becomes active - if self.vault.active_profile.is_none() { - self.vault.active_profile = Some(restored.npub.clone()); + // A secret-less record (should not happen for records created by + // delete_profile_record) must not silently create a hollow + // profile: refuse and hand the entry back rather than corrupt the + // vault. + if restored.secret_key.trim().is_empty() { + self.undo_history.push(deleted); + return Err(AppError::internal( + "The undo entry is missing its secret key; the profile was not restored.", + )); } + self.vault + .active_profile + .get_or_insert(restored.public_key.clone()); + self.vault.profiles.push(restored.clone()); } - Ok(restored) + let is_active = self.vault.active_profile.as_deref() == Some(restored.public_key.as_str()); + Ok(ProfileSummary { + label: restored.label.clone(), + npub: restored.public_key.clone(), + created_at: restored.created_at, + is_active, + picture: restored.picture.clone(), + nip05: restored.nip05.clone(), + }) } /// Protect the vault with `new_password`, re-encrypting every stored key. @@ -361,7 +373,11 @@ impl App { active_profile: profiles::active_summary(&self.vault), profiles: profiles::summaries(&self.vault), settings: self.settings.clone(), - undo_history: self.undo_history.clone(), + undo_history: self + .undo_history + .iter() + .map(|deleted| deleted.summary.clone()) + .collect(), last_publish: self.last_publish.clone(), signer_mode: self.signer_mode, } @@ -622,4 +638,32 @@ mod tests { assert_eq!(view.profiles.len(), 2); assert!(view.profiles.iter().all(|p| p.npub.starts_with("npub1"))); } + + #[test] + fn undo_delete_restores_working_profile_without_leaking_secret() { + let mut app = sample_app(); + let target = app.vault.profiles[0].clone(); + let secret = target.secret_key.clone(); + + let deleted = profiles::delete_profile_record(&mut app.vault, &target.public_key).unwrap(); + app.undo_history.push(deleted); + assert_eq!(app.vault.profiles.len(), 1); + + let restored = app.undo_delete().unwrap(); + assert_eq!(restored.npub, target.public_key); + assert_eq!(app.vault.profiles.len(), 2); + let stored = app + .vault + .profiles + .iter() + .find(|p| p.public_key == target.public_key) + .unwrap(); + assert_eq!(stored.secret_key, secret, "undo must restore the real key"); + assert!(!stored.secret_key.is_empty()); + + // The UI-facing view carries summaries only — never secret material. + let view_json = serde_json::to_string(&app.state_view()).unwrap(); + assert!(!view_json.contains(&secret)); + assert!(app.undo_history.is_empty()); + } } diff --git a/src/ipc.rs b/src/ipc.rs index cfa1bed..3edaa24 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -793,9 +793,9 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - let deleted = profiles::delete_profile(&mut app.vault, &npub)?; + let deleted = profiles::delete_profile_record(&mut app.vault, &npub)?; app.save_vault()?; - app.undo_history.push(deleted.clone()); + app.undo_history.push(deleted); Ok(json!(app.state_view())) } Request::UndoDelete => { diff --git a/src/main.rs b/src/main.rs index c578b63..525b731 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,11 +5,11 @@ use keynectr::app::App; use keynectr::bunker::Signer; use keynectr::errors::{AppError, ErrorKind}; use keynectr::ipc; -use keynectr::profiles::{self, ProfileSummary}; +use keynectr::profiles; use keynectr::publish; use keynectr::relays; use keynectr::settings::Theme; -use keynectr::vault::{self, StoredProfile, Vault}; +use keynectr::vault::{self, Vault}; const USAGE: &str = "\ keynectr [args...] @@ -629,26 +629,13 @@ fn cli_info() -> Result { } /// Delete a profile by npub, moving it to the undo stack. -/// Returns the deleted profile summary, or an error if not found. -fn delete_profile_direct(vault: &mut Vault, npub: &str) -> Result { - let pos = vault - .profiles - .iter() - .position(|p| p.public_key == npub) - .ok_or_else(|| AppError::profile_not_found(npub))?; - let stored = vault.profiles.remove(pos); - // Clear the active_profile if it was the one deleted - if vault.active_profile.as_deref() == Some(npub) { - vault.active_profile = None; - } - Ok(ProfileSummary { - label: stored.label, - npub: stored.public_key, - created_at: stored.created_at, - is_active: false, - picture: stored.picture, - nip05: stored.nip05, - }) +/// Returns the full deleted record so the CLI can report it and push the +/// same entry the IPC path uses. +fn delete_profile_direct( + vault: &mut Vault, + npub: &str, +) -> Result { + profiles::delete_profile_record(vault, npub) } fn cli_delete_profile(args: &[String]) -> Result { @@ -657,37 +644,22 @@ fn cli_delete_profile(args: &[String]) -> Result { } let npub = args[2].clone(); let mut app = App::load()?; + // Capture the label BEFORE removal; the profile is gone afterwards. + let label = profiles::profile_label(&app.vault, &npub) + .unwrap_or(&npub) + .to_string(); let deleted = delete_profile_direct(&mut app.vault, &npub)?; app.save_vault()?; - // Add to undo history - app.undo_history.push(deleted.clone()); + // Add to undo history (full record: undo restores a working profile). + app.undo_history.push(deleted); Ok(format!( - "Profile '{}' deleted (npub: {}). Use 'undo-delete' to restore.", - profiles::profile_label(&app.vault, &npub).unwrap_or(&npub), - npub + "Profile '{label}' deleted (npub: {npub}). Use 'undo-delete' to restore." )) } fn cli_undo_delete() -> Result { let mut app = App::load()?; - if app.undo_history.is_empty() { - return Err(AppError::config("No profile deletions to undo.")); - } - let restored = app.undo_history.pop().unwrap(); - // Re-add the profile to the vault - let stored = StoredProfile { - label: restored.label.clone(), - public_key: restored.npub.clone(), - secret_key: "".to_string(), - created_at: restored.created_at, - picture: restored.picture, - nip05: restored.nip05, - signer_mode: keynectr::vault::SignerMode::Embedded, - }; - app.vault.profiles.push(stored); - if app.vault.active_profile.is_none() { - app.vault.active_profile = Some(restored.npub.clone()); - } + let restored = app.undo_delete()?; app.save_vault()?; Ok(format!( "Profile '{}' restored from undo stack.", diff --git a/src/profiles.rs b/src/profiles.rs index ec6ad14..218f229 100644 --- a/src/profiles.rs +++ b/src/profiles.rs @@ -745,9 +745,19 @@ pub fn parse_secret_key(hex_str: &str) -> Result { SecretKey::from_slice(&bytes).map_err(|e| AppError::invalid_secret(format!("{e}"))) } -/// Delete a profile by npub, returning the deleted profile for undo. -/// The vault must not be encrypted, or the key must be provided. -pub fn delete_profile(vault: &mut Vault, npub: &str) -> Result { +/// A profile removed from the vault together with everything needed to put it +/// back: the safe summary for the UI *and* the full `StoredProfile` including +/// its secret key material (plaintext or encrypted blob, exactly as stored). +#[derive(Debug, Clone)] +pub struct DeletedProfile { + pub summary: ProfileSummary, + pub stored: StoredProfile, +} + +/// Delete a profile by npub, returning the deleted record for undo. The +/// returned `DeletedProfile` carries the real stored secret so undo can +/// restore a fully functional profile. Never serialize it to the UI. +pub fn delete_profile_record(vault: &mut Vault, npub: &str) -> Result { let pos = vault .profiles .iter() @@ -757,14 +767,22 @@ pub fn delete_profile(vault: &mut Vault, npub: &str) -> Result Result { + delete_profile_record(vault, npub).map(|deleted| deleted.summary) } #[cfg(test)] From 8780ef3fbbac07aab656f690f94b8dfb7298faa3 Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 10 Sep 2026 12:50:24 -0500 Subject: [PATCH 02/74] checkpoint: document undo-delete secret-preserving fix (2026-09-10) --- CHECKPOINT-encryption.md | 94 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/CHECKPOINT-encryption.md b/CHECKPOINT-encryption.md index 352d9bf..a7f2d34 100644 --- a/CHECKPOINT-encryption.md +++ b/CHECKPOINT-encryption.md @@ -1,3 +1,97 @@ +# Checkpoint — Undo-delete restores working profiles (2026-09-10) + +## Where things are +- Project: `/home/avi/Projects/Keynctr` +- Branch: `master` @ **`d101b8e`** ("fix(undo): restore full profile with secret key on undo-delete"). +- Working tree: **clean for tracked files.** Only untracked entries are the pre-existing + hygiene leftovers plus two Rust scratch files (all intentionally untracked — see + "Still untracked"). + +## What was completed (this session) + +**Step 6 (undo history) — the hollow-undo bug is fixed, landed as `d101b8e`.** +Deleting a profile used to keep only a `ProfileSummary` on the undo stack, so +undo re-created the profile with `secret_key = ""` — a dead shell that could never +sign. The undo stack now keeps the full stored record: + +- **`src/profiles.rs`** — new `DeletedProfile { summary, stored }` struct; + `delete_profile_record()` returns the real stored secret (plaintext or encrypted + blob, exactly as on disk) plus the safe UI summary; `delete_profile()` stays as + the summary-only wrapper for callers that keep no undo entry. +- **`src/app.rs`** — `App.undo_history` is now `Vec` (secret material + never leaves the backend); `undo_delete()` restores the real `StoredProfile`, + refuses to create a hollow profile (empty secret → entry handed back + error), + and `state_view()` still exposes only `summary` items so the renderer never sees + a secret. New regression test + `undo_delete_restores_working_profile_without_leaking_secret` locks this in. +- **`src/ipc.rs`** — `DeleteProfile` routes through `delete_profile_record` so the + GUI undo entry carries the secret (previously it pushed a summary-only entry, + so GUI undo was still hollow). +- **`src/main.rs`** — CLI `delete-profile`/`undo-delete` use the same record path + (`delete_profile_direct` → `delete_profile_record`, `cli_undo_delete` → + `app.undo_delete()`); also fixes the old "label looked up after removal" bug by + capturing the label before deletion, and drops the now-unused imports. +- Compile fixes included: the inherited work-in-progress did not build (`DeletedProfile` + vs `ProfileSummary` mismatch in `ipc.rs`, partial moves in `undo_delete`); both + resolved, plus `cargo fmt` applied. + +Security properties: secret material stays backend-only (`AppStateView.undo_history` +is still `Vec`); undo restores the exact stored blob (no re-derivation, +no logging); empty-secret entries fail closed instead of writing hollow profiles. + +## Commits added this session (newest first) +| Hash | Message | +|------|---------| +| `d101b8e` | fix(undo): restore full profile with secret key on undo-delete | + +(Parent chain — `1d5940f` display/icons checkpoint, `d580139` icon alpha fix, +`0814a53` Linux display compat, `715c99c`/`510cb65` connection-secrets vault +integration — is unchanged.) + +## Verification (run this session, on top of `d101b8e`) +- **Rust**: `cargo test` → **197 passed**, 0 failed (196 pre-existing + 1 new + undo regression test); `cargo clippy --all-targets` → clean (exit 0); + `cargo fmt --check` → clean (exit 0); `cargo build --release` → Finished, exit 0. +- **Frontend** (in `frontend/`, Rust-only change so no frontend files touched): + `npm test` → **116/116 passed**; `npm run typecheck` → exit 0; + `npm run lint` → exit 0; `npm run electron:build` → exit 0; `npm run build` → + exit 0. `npm run format:check` → warns on the same 5 pre-existing files + (`ExportSecretKeyModal.tsx`, `SignerModeScreen.tsx`, `AppProvider.tsx`, + `ExportSecretKey.test.tsx`, `fakeBackend.ts`) documented in earlier checkpoints — + not introduced here, left untouched. + +## How to reproduce / exercise +- Backend: `cargo run --release -- serve` (JSON-lines IPC on stdio) or the CLI in + `src/main.rs`. +- GUI: from `frontend/`, `npm run electron:build && electron .` (prod) or `npm run + start:dev` with `NOSTR_GUI_DEV_URL`. +- Exercise undo: Profiles → delete a profile → Undo delete → the restored profile + signs/publishes (previously it came back secret-less). CLI equivalent: + `keynectr delete-profile ` then `keynectr undo-delete`. + +## Still untracked (do NOT lose; do NOT commit the hygiene junk) +- **Source JPEG** `KeynectrAppIconPossibility02.jpeg` — intentionally untracked. +- Pre-existing untracked hygiene leftovers: `COSMIC_THEME.md`, `.opencode/`, + `.impeccable/critique/`, `.directory`, `deferred/SignerConnectionPanel.tsx.wip/`. +- Rust scratch files (unreferenced, harmless — neither is wired into the build): + `src/signer/nip46_external.rs` (dead stub, not declared in `src/signer/mod.rs`), + `src/publish.rs.bak` (backup copy). Left alone this session; delete or wire up + in a later pass. +- Build artifacts `release/` and `dist/` are gitignored and not committed. +- `profiles_vault.json*` and `target/` remain correctly untracked and uncommitted. + +## Deferred / next steps (unchanged, minus the undo item) +- Step 3 sub-step 2 (IPC reroute) remains the next signer milestone; external + (remote) signing in the publish path still returns "not yet supported". +- External-signer permissions (Step 4), deferred security (Step 5: KDF upgrade, + `--allow-env-secret`, gate deprecated `RevealSecretKey`), hygiene (Step 7: + Keynctr rename incl. `package.json` → `homepage`, legacy Python removal, vault + relocation, Prettier pass over the 5 known files). +- Open question carried forward: `migrate_vault_signer_modes` reports a change on + every load (always `changed = true`), so `App::load` re-saves each start. + +--- + # Checkpoint — NIP-46 Connection Secrets in the Vault (2026-09-04) ## Where things are From 1af79d81cdc5c21540b5707c1c964303886fe93b Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 10 Sep 2026 21:54:32 -0500 Subject: [PATCH 03/74] feat(signer): end-to-end external NIP-46 signing in publish and upload auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 3 sub-step 2 (IPC reroute) — the publish path now actually signs remotely instead of returning 'not yet supported': - src/signer/nip46_client.rs: outbound NIP-46 request half — send sign_event over the encrypted channel, demux responses to waiting callers, 30s timeout, waiters woken on disconnect/fail. Signer::sign_event verifies the returned event matches the requested unsigned event, is signed by the connected identity, and carries a valid signature; no local fallback. Permission-denied audit uses try_lock so a denied in-flight sign cannot deadlock the dispatcher. - src/app.rs: App::signing_for / signing_active — one place that maps a profile's SignerMode to a Signing source. Embedded -> Local(vault key); Nip46Client -> External(live signer) only when connected, otherwise ExternalSignerNotConnected; Nip46Bunker fails closed. - src/publish.rs: publish_with_keys builds the unsigned event from the Signing's own pubkey and signs via Signing::sign; publish_signed entry point for IPC (CLI keeps publish_active local path). - src/ipc.rs: PublishNote and UploadAuth route through signing_active. Nip46Connect/Nip46Disconnect drop the App guard before awaiting connect()/disconnect() (they re-lock internally — latent deadlock). - src/relays.rs: keyless relay pool (open_pool_inner(Option)) so external signing publishes without local keys. - src/uploads.rs: nip98_authorization takes a Signing source, so upload auth signs remotely for external profiles too. - src/profiles.rs: store_remote_profile — connecting a NIP-46 signer creates/refreshes a secretless Nip46Client profile row; refuses to silently convert an existing local profile. Tests: signing selection (local, fail-closed external, no profile), store_remote_profile create + no-clobber. 200 tests pass; clippy clean; fmt clean; release build green. --- src/app.rs | 117 ++++++++++++++++++++ src/ipc.rs | 46 +++++--- src/profiles.rs | 54 ++++++++- src/publish.rs | 56 +++++----- src/relays.rs | 30 ++++- src/signer/nip46_client.rs | 217 ++++++++++++++++++++++++++++++++++--- src/uploads.rs | 108 +++++------------- 7 files changed, 486 insertions(+), 142 deletions(-) diff --git a/src/app.rs b/src/app.rs index f58875e..8ee1134 100644 --- a/src/app.rs +++ b/src/app.rs @@ -9,9 +9,12 @@ use crate::crypto::{self, VaultKey}; use crate::errors::AppError; use crate::profiles::{self, ProfileSummary}; use crate::settings::Settings; +use crate::signer::Signer as SignerTrait; +use crate::signer::Signing; use crate::vault::{ self, KdfParams, SignerMode, StoredProfile, StoredPublishReport, Vault, VaultCrypto, }; +use nostr_sdk::prelude::{Keys, PublicKey}; /// Minimum password length accepted when encrypting the vault. pub const MIN_PASSWORD_LEN: usize = 8; @@ -116,6 +119,56 @@ impl App { self.vault.is_encrypted() && self.unlock_key.is_none() } + /// The [`Signing`] source for user content of a specific profile. + /// + /// The single place the "where does signing happen" decision is made, so + /// no caller branches on signer mode itself: + /// + /// - Profile mode `Embedded` → [`Signing::Local`] with the vault-resolved + /// key (locked vault surfaces as the usual `VaultLocked` error). + /// - Profile mode `Nip46Client` → [`Signing::External`] wrapping the live + /// NIP-46 client signer, **only** when one is present and connected. + /// Never falls back to the local key: an external profile that cannot + /// reach its signer fails with `ExternalSignerNotConnected`. + /// - `Nip46Bunker` (legacy, not wired) → fails closed like a missing + /// connection. + pub async fn signing_for(&self, npub: &str) -> Result { + let profile = profiles::find_stored_profile(&self.vault, npub)?; + match profile.signer_mode { + SignerMode::Embedded => { + let secret_hex = profiles::resolve_secret_key(&self.vault, npub, self.vault_key())?; + let secret_key = profiles::parse_secret_key(&secret_hex)?; + Ok(Signing::Local(Keys::new(secret_key))) + } + SignerMode::Nip46Client => { + let Some(signer) = self.nip46_signer.clone() else { + return Err(AppError::external_signer_not_connected()); + }; + if !signer.is_available().await { + return Err(AppError::external_signer_not_connected()); + } + let profile_pubkey = PublicKey::parse(npub).map_err(|e| { + AppError::internal(format!("Stored profile npub is not valid: {e}")) + })?; + Ok(Signing::External { + signer, + profile_pubkey, + }) + } + SignerMode::Nip46Bunker => Err(AppError::external_signer_not_connected()), + } + } + + /// [`Signing`] for the active profile (see [`App::signing_for`]). + pub async fn signing_active(&self) -> Result { + let npub = self + .vault + .active_profile + .clone() + .ok_or_else(AppError::no_active_profile)?; + self.signing_for(&npub).await + } + /// Verify a password and keep the derived key in memory for the session. pub fn unlock(&mut self, password: &str) -> Result<(), AppError> { let crypto = self @@ -451,6 +504,70 @@ mod tests { } } + #[test] + fn signing_for_embedded_profile_yields_local_signing() { + let app = sample_app(); + let npub = app.vault.profiles[0].public_key.clone(); + let runtime = tokio::runtime::Runtime::new().unwrap(); + let signing = runtime + .block_on(app.signing_for(&npub)) + .expect("embedded profile must select local signing"); + assert!(matches!(signing, crate::signer::Signing::Local(_))); + } + + #[test] + fn signing_for_external_profile_without_connection_fails_closed() { + let mut app = sample_app(); + // Mark the active profile as externally signed; no signer is + // connected (and none can be without a live NIP-46 session). + app.vault.profiles[0].signer_mode = SignerMode::Nip46Client; + let npub = app.vault.profiles[0].public_key.clone(); + let runtime = tokio::runtime::Runtime::new().unwrap(); + match runtime.block_on(app.signing_for(&npub)) { + Err(err) => assert_eq!(err.kind(), ErrorKind::ExternalSignerNotConnected), + Ok(_) => panic!("external profile with no signer must fail closed"), + } + } + + #[test] + fn signing_active_requires_a_profile() { + let mut app = sample_app(); + app.vault.active_profile = None; + let runtime = tokio::runtime::Runtime::new().unwrap(); + match runtime.block_on(app.signing_active()) { + Err(err) => assert_eq!(err.kind(), ErrorKind::NoActiveProfile), + Ok(_) => panic!("no active profile must error"), + } + } + + #[test] + fn store_remote_profile_creates_secretless_external_profile() { + use nostr::nips::nip19::ToBech32; + let mut vault = plaintext_vault(); + let remote = Keys::generate(); + let npub = remote.public_key().to_bech32().unwrap(); + let summary = profiles::store_remote_profile(&mut vault, &npub, "Remote".to_string()) + .expect("remote profile must be created"); + assert_eq!(summary.npub, npub); + assert!(summary.is_active); + let stored = profiles::find_stored_profile(&vault, &npub).unwrap(); + assert_eq!(stored.signer_mode, SignerMode::Nip46Client); + assert!(stored.secret_key.is_empty(), "no local secret for remote"); + } + + #[test] + fn store_remote_profile_refuses_to_clobber_local_profile() { + let mut vault = plaintext_vault(); + let existing = vault.profiles[0].public_key.clone(); + let err = profiles::store_remote_profile(&mut vault, &existing, "Hijack".to_string()) + .expect_err("a local profile must not be converted silently"); + assert!(err.message().contains("local profile")); + // Untouched: still embedded, secret intact, active unchanged. + let stored = profiles::find_stored_profile(&vault, &existing).unwrap(); + assert_eq!(stored.signer_mode, SignerMode::Embedded); + assert!(!stored.secret_key.is_empty()); + } + #[test] fn set_password_encrypts_every_secret() { let mut app = sample_app(); diff --git a/src/ipc.rs b/src/ipc.rs index 3edaa24..14435e3 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -396,15 +396,21 @@ async fn run(app: &Arc>, request: Request) -> Result { - let guard = app.lock().await; - if let Some(signer) = &guard.nip46_signer { - let status = signer.connect(&uri, label).await?; - Ok(json!(status)) - } else { - Err(AppError::config( + // Take the signer handle under the lock, then drop the guard + // before awaiting: connect() re-locks the App internally (to + // persist the connection and resolve its secret), so holding the + // guard across the await would deadlock. + let signer = { + let guard = app.lock().await; + guard.nip46_signer.clone() + }; + let Some(signer) = signer else { + return Err(AppError::config( "NIP-46 signer not initialized. Set signer mode to nip46 first.", - )) - } + )); + }; + let status = signer.connect(&uri, label).await?; + Ok(json!(status)) } Request::Nip46Disconnect => { let guard = app.lock().await; @@ -646,9 +652,17 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - let report = - publish::publish_active(&app.vault, &app.settings, &content, app.vault_key()) - .await?; + // Signer selection lives in App::signing_for: an embedded profile + // signs with the vault key; an external (NIP-46) profile's note + // round-trips to the connected signer, and an unconnected one + // fails closed — never with a silent fallback to the local key. + // + // As before, the shared App guard is held across the publish. + // The signer's background task needs no App lock to deliver the + // sign response (only audit paths take it, briefly), so the + // round-trip completes with the guard held. + let signing = app.signing_active().await?; + let report = publish::publish_signed(&app.settings, &content, &signing).await?; let stored = crate::vault::StoredPublishReport { event_id: report.event_id.clone(), succeeded: report.succeeded.clone(), @@ -765,13 +779,9 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - let authorization = crate::uploads::nip98_authorization( - &app.vault, - &url, - &http_method, - app.vault_key(), - ) - .await?; + let signing = app.signing_active().await?; + let authorization = + crate::uploads::nip98_authorization(&url, &http_method, &signing).await?; Ok(json!({ "authorization": authorization })) } diff --git a/src/profiles.rs b/src/profiles.rs index 218f229..7d66d18 100644 --- a/src/profiles.rs +++ b/src/profiles.rs @@ -9,7 +9,7 @@ use crate::errors::AppError; use crate::publish::RelayFailure; use crate::relays; use crate::settings::Settings; -use crate::vault::{unix_timestamp, StoredProfile, Vault}; +use crate::vault::{unix_timestamp, SignerMode, StoredProfile, Vault}; /// A safe view of a profile that contains no secret key material. #[derive(Debug, Clone, Serialize, PartialEq, Eq)] @@ -463,6 +463,58 @@ pub fn find_stored_profile<'a>( .ok_or_else(|| AppError::profile_not_found(npub)) } +/// Create or refresh the vault profile for a remote (NIP-46) identity. +/// +/// When a NIP-46 client connection is established the identity lives on the +/// remote signer, but the user still needs a profile row so publishing has a +/// selection. The row is marked `Nip46Client` and carries **no secret key** +/// (there is none locally): every signing operation for it must go through +/// the connected signer, and key export refuses it. Re-connecting updates the +/// label and re-activates the profile rather than duplicating it. +pub fn store_remote_profile( + vault: &mut Vault, + npub: &str, + label: String, +) -> Result { + // Validate the identity before writing anything. + PublicKey::parse(npub) + .map_err(|e| AppError::internal(format!("Remote signer identity is not valid: {e}")))?; + // An existing local profile must never be silently converted to remote: + // refuse *before* mutating if it carries a local secret. + if let Some(existing) = vault.profiles.iter().find(|p| p.public_key == npub) { + if existing.signer_mode != SignerMode::Nip46Client && !existing.secret_key.trim().is_empty() + { + return Err(AppError::config( + "That identity already exists as a local profile. Delete it first if you want to use an external signer for it.", + )); + } + } + if let Some(existing) = vault.profiles.iter_mut().find(|p| p.public_key == npub) { + existing.signer_mode = SignerMode::Nip46Client; + existing.label = label; + } else { + vault.profiles.push(StoredProfile { + label: label.clone(), + public_key: npub.to_string(), + secret_key: String::new(), // no local key — identity lives on the signer + created_at: unix_timestamp()?, + picture: None, + nip05: None, + signer_mode: SignerMode::Nip46Client, + }); + } + vault.active_profile = Some(npub.to_string()); + let stored = find_profile(vault, npub)?; + Ok(ProfileSummary { + label: stored.label.clone(), + npub: stored.public_key.clone(), + created_at: stored.created_at, + is_active: true, + picture: stored.picture.clone(), + nip05: stored.nip05.clone(), + }) +} + fn find_profile<'a>(vault: &'a Vault, npub: &str) -> Result<&'a StoredProfile, AppError> { vault .profiles diff --git a/src/publish.rs b/src/publish.rs index de3fbad..30b13b9 100644 --- a/src/publish.rs +++ b/src/publish.rs @@ -48,6 +48,9 @@ impl PublishReport { /// Publish a text note with the active profile. /// /// `key` must be the unlocked vault key when the vault is password-protected. +/// Always signs locally from the vault — used by the CLI, which has no signer +/// instances. GUI callers use [`publish_signed`] with an [`App::signing_for`] +/// signing source so external-signer profiles route to their remote signer. pub async fn publish_active( vault: &Vault, settings: &Settings, @@ -61,6 +64,17 @@ pub async fn publish_active( publish_with_keys(settings, content, &signing).await } +/// Publish a text note through an explicit [`Signing`] source (embedded or +/// external). This is what the GUI publish path uses. +pub async fn publish_signed( + settings: &Settings, + content: &str, + signing: &Signing, +) -> Result { + validate_content(content)?; + publish_with_keys(settings, content, signing).await +} + /// Publish a text note as a specific profile (used by the CLI). /// /// `key` must be the unlocked vault key when the vault is password-protected. @@ -164,33 +178,19 @@ async fn publish_with_keys( return Err(AppError::no_enabled_relays()); } - // Extract &Keys from Signing::Local for EventBuilder operations. - // Currently Signing::Local is used from publish_active/publish_as, - // but the pattern supports External signers in the future. - let keys = match signing { - Signing::Local(k) => k, - Signing::External { - signer: _, - profile_pubkey: _, - } => { - return Err(AppError::sign_failed( - "External signer not yet supported in publish_with_keys", - )); - } - }; + // The pubkey comes from the Signing itself. For an external signer this + // performs identity validation first: a signer that does not control the + // active profile's key fails here, before any event is built. + let pubkey = signing.pubkey().await.map_err(AppError::from)?; - // Build the unsigned event. + // Build the unsigned event under the signing identity, then sign it + // through `Signing` (local key or the NIP-46 round-trip). This is the + // core reroute: the IPC layer never calls Keys::sign_event directly, and + // an external profile never needs a local secret. let builder = EventBuilder::new(Kind::TextNote, content.to_string()).tags(image_tags(content)); - let unsigned = builder - .finalize_async(keys) - .await - .map_err(|e| AppError::sign_failed(format!("{e}")))?; - - // Sign the event through the Signing trait (routes to Keys::sign_event or - // Signer::sign_event depending on the variant). This is the core refactor: - // the IPC layer no longer calls Keys::sign_event directly. + let unsigned = builder.finalize_unsigned(pubkey); let signed = signing - .sign(unsigned.into()) + .sign(unsigned) .await .map_err(|e| AppError::sign_failed(format!("{e}")))?; @@ -199,7 +199,13 @@ async fn publish_with_keys( .to_bech32() .map_err(|e| AppError::internal(format!("Could not encode the event id: {e}")))?; - let client = relays::open_pool(keys.clone(), &relay_urls, None).await?; + // Local signing can answer NIP-42 AUTH challenges; with an external + // signer the app holds no key, so the pool opens without an + // authenticator and auth-gated relays report their rejection per-relay. + let client = match signing { + Signing::Local(keys) => relays::open_pool(keys.clone(), &relay_urls, None).await?, + Signing::External { .. } => relays::open_pool_anon(&relay_urls, None).await?, + }; let (succeeded, failed) = send_to_all_relays(&client, relay_urls, &signed, "note").await; if succeeded.is_empty() { diff --git a/src/relays.rs b/src/relays.rs index 303698c..51b26ba 100644 --- a/src/relays.rs +++ b/src/relays.rs @@ -78,12 +78,36 @@ pub(crate) async fn open_pool( keys: Keys, relay_urls: &[String], wait: Option, +) -> Result { + open_pool_inner(Some(keys), relay_urls, wait).await +} + +/// Open a relay pool with no signing identity. +/// +/// Used when user content is signed by an external (NIP-46) signer: the app +/// holds no key to answer NIP-42 AUTH challenges with, so the pool is built +/// without an authenticator. Relays that demand auth will reject reads/ +/// writes at the protocol level, which `send_to_all_relays` already reports +/// per-relay. +pub(crate) async fn open_pool_anon( + relay_urls: &[String], + wait: Option, +) -> Result { + open_pool_inner(None, relay_urls, wait).await +} + +async fn open_pool_inner( + keys: Option, + relay_urls: &[String], + wait: Option, ) -> Result { // The authenticator answers NIP-42 AUTH challenges automatically on every // path that opens a client (nostr-sdk >= 0.45 has no implicit signer). - let client = Client::builder() - .authenticator(SignerAuthenticator::new(keys)) - .build(); + let builder = match keys { + Some(keys) => Client::builder().authenticator(SignerAuthenticator::new(keys)), + None => Client::builder(), + }; + let client = builder.build(); for url in relay_urls { client .add_relay(url.as_str()) diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 1319443..db578d6 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -31,6 +31,9 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); const APPROVAL_TIMEOUT: Duration = Duration::from_secs(300); /// Maximum number of requests kept waiting for approval at once. const MAX_PENDING_APPROVALS: usize = 20; +/// How long an outbound NIP-46 request (e.g. our own `sign_event`) may wait +/// for the remote signer's response before it is abandoned. +const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); /// Internal state for a pending approval. struct PendingApprovalInner { @@ -39,6 +42,12 @@ struct PendingApprovalInner { sender: oneshot::Sender, } +/// A response awaited from the *remote signer* for a request we sent +/// (the client half of the NIP-46 flow, e.g. our `sign_event` request). +struct PendingRemoteRequest { + sender: oneshot::Sender>, +} + /// Parsed nostrconnect:// URI. struct ConnectUri { peer: PublicKey, @@ -60,6 +69,9 @@ struct Nip46Inner { conversation_key: Option, client: Option, pending: HashMap, + /// Outbound requests we sent to the remote signer (e.g. `sign_event`) + /// waiting for its encrypted response, keyed by request id. + remote_pending: HashMap, keys: Option, active_npub: Option, } @@ -84,6 +96,7 @@ impl Nip46ClientSigner { conversation_key: None, client: None, pending: HashMap::new(), + remote_pending: HashMap::new(), keys: None, active_npub: None, })), @@ -97,10 +110,16 @@ impl Nip46ClientSigner { } /// Emit an audit event for a permission-denied NIP-46 operation. + /// + /// Uses `try_lock`: the IPC dispatcher may hold the App lock while a + /// sign request is in flight (PublishNote), and audit is best-effort — + /// blocking here would deadlock the very request being denied. async fn audit_permission_denied(&self, method: &str) { let npub = self.inner.lock().await.active_npub.clone(); if let Some(npub) = npub { - let mut app = self.app.lock().await; + let Ok(mut app) = self.app.try_lock() else { + return; + }; if let Some(ref mut log) = app.audit_log { let _ = log.record( &npub, @@ -243,8 +262,8 @@ impl Nip46ClientSigner { // Persist the connection AND its secret in the vault. The secret is // encrypted under the vault key (when a password is set) and keyed by // the connection's opaque VaultRef — never stored inline on the - // connection, never logged. - { + // connection, never logged. Evaluates to the remote identity's npub. + let remote_npub = { let mut app = self.app.lock().await; // Remove any existing connection for the same signer from the // same profile (reconnect replaces the old connection). @@ -268,8 +287,19 @@ impl Nip46ClientSigner { crate::vault::delete_connection_secret(&mut app.vault, &vault_ref); } app.vault.nip46_connections.push(connection.clone()); + // The remote identity gets its own profile row so publishing has + // a selection: marked Nip46Client with no local secret, so every + // signing path for it routes through this connection and key + // export refuses it. If the peer key collides with an existing + // local (embedded) profile the connect fails closed. + let remote_npub = PublicKey::from_hex(&connection.signer_pubkey) + .map_err(|e| AppError::internal(format!("Invalid signer public key: {e}")))? + .to_bech32() + .map_err(|e| AppError::internal(format!("Could not encode npub: {e}")))?; + profiles::store_remote_profile(&mut app.vault, &remote_npub, connection.label.clone())?; app.save_vault()?; - } + remote_npub + }; // Update state to connecting { @@ -283,6 +313,9 @@ impl Nip46ClientSigner { inner.connection = Some(connection.clone()); inner.conversation_key = Some(conversation); inner.keys = Some(keys.clone()); + // The signing identity is the remote signer's key, and the vault + // now holds a matching Nip46Client profile row (active). + inner.active_npub = Some(remote_npub); inner.pending.clear(); } @@ -326,6 +359,13 @@ impl Nip46ClientSigner { inner.conversation_key = None; inner.keys = None; inner.pending.clear(); + // Wake any callers awaiting a remote response; their waiters turn + // into `NotConnected` rather than hanging until the request timeout. + for (_, waiter) in inner.remote_pending.drain() { + let _ = waiter.sender.send(Err( + "Disconnected from the signer before it responded.".to_string() + )); + } Ok(()) } @@ -412,6 +452,11 @@ impl Nip46ClientSigner { inner.conversation_key = None; inner.keys = None; inner.pending.clear(); + for (_, waiter) in inner.remote_pending.drain() { + let _ = waiter.sender.send(Err( + "Lost the connection to the signer before it responded.".to_string(), + )); + } } } @@ -447,6 +492,83 @@ impl Nip46ClientSigner { } } + /// Send a NIP-46 request to the connected remote signer and await its + /// encrypted response (the client half of the protocol — e.g. asking the + /// signer to `sign_event` an event for us). + /// + /// Fails closed: no connection, no live session, or a signer error all + /// return [`SigningError`] rather than falling back to any local key. The + /// waiter is always removed from the pending map, even on timeout, so a + /// late response to an abandoned request finds nothing to wake. + async fn send_remote_request( + &self, + method: &str, + params: Vec, + ) -> Result { + let id = uuid::Uuid::new_v4().to_string(); + let (sender, receiver) = oneshot::channel(); + + // Snapshot the live session, register the waiter, and send the + // encrypted request in one lock pass. Holding the lock across the send + // is safe (the send is a relay hand-off, never a re-entry into this + // signer) and guarantees registration cannot interleave with the send. + let send_result = { + let mut inner = self.inner.lock().await; + let client = inner.client.clone().ok_or(SigningError::NotConnected)?; + let keys = inner.keys.clone().ok_or(SigningError::NotConnected)?; + let conversation = inner + .conversation_key + .as_ref() + .cloned() + .ok_or(SigningError::NotConnected)?; + let connection = inner.connection.clone().ok_or(SigningError::NotConnected)?; + if !connection_valid_now(&connection) { + return Err(if connection.revoked_at.is_some() { + SigningError::ConnectionRevoked + } else { + SigningError::ConnectionExpired + }); + } + if !matches!(inner.phase, Nip46Phase::Connected) { + return Err(SigningError::NotConnected); + } + if inner.remote_pending.len() >= MAX_PENDING_APPROVALS { + return Err(SigningError::Internal { + detail: "Too many requests already awaiting the signer.".to_string(), + }); + } + let peer = PublicKey::from_hex(&connection.signer_pubkey).map_err(|e| { + SigningError::Internal { + detail: format!("Invalid signer public key: {e}"), + } + })?; + inner + .remote_pending + .insert(id.clone(), PendingRemoteRequest { sender }); + + let payload = json!({ "id": id, "method": method, "params": params }).to_string(); + self.publish_payload(&client, &keys, &conversation, &peer, &payload) + .await + }; + if let Err(detail) = send_result { + self.inner.lock().await.remote_pending.remove(&id); + return Err(SigningError::Network { detail }); + } + + match tokio::time::timeout(REQUEST_TIMEOUT, receiver).await { + Ok(Ok(result)) => result.map_err(|detail| SigningError::Internal { detail }), + Ok(Err(_)) => { + // The waiter was dropped (disconnect/fail) while awaiting. + Err(SigningError::NotConnected) + } + Err(_) => { + // Abandon the waiter so a late response finds nothing. + self.inner.lock().await.remote_pending.remove(&id); + Err(SigningError::Timeout) + } + } + } + /// Main background task: connect to relays, subscribe, handle requests. async fn run_sign_task(self, uri: ConnectUri) -> Result<(), String> { let (conversation, keys) = { @@ -534,6 +656,35 @@ impl Nip46ClientSigner { Err(_) => continue, }; + // A payload carrying `result`/`error` is a response to a request + // WE sent (e.g. `sign_event`), not an incoming signer request. + // Deliver it to the waiting caller; unknown ids are ignored — a + // stray response must never earn an error reply back to the + // signer, and must never fall through to the request path. + let shaped: serde_json::Value = + serde_json::from_str(&plaintext).unwrap_or(serde_json::Value::Null); + if shaped.get("result").is_some() || shaped.get("error").is_some() { + let id = shaped.get("id").and_then(|v| v.as_str()).unwrap_or(""); + let outcome = if shaped.get("error").is_some() && !shaped["error"].is_null() { + Err(shaped["error"] + .as_str() + .unwrap_or("The signer reported an error.") + .to_string()) + } else { + Ok(shaped["result"].as_str().unwrap_or("").to_string()) + }; + let waiter = self.inner.lock().await.remote_pending.remove(id); + match waiter { + Some(pending) => { + let _ = pending.sender.send(outcome); + } + None => { + eprintln!("Ignoring NIP-46 response with no waiting request: {id}"); + } + } + continue; + } + let request: RawRequest = match serde_json::from_str(&plaintext) { Ok(r) => r, Err(_) => continue, @@ -879,16 +1030,41 @@ impl Signer for Nip46ClientSigner { }) } - async fn sign_event(&self, _event: UnsignedEvent) -> Result { - // For NIP-46 client, signing happens via the NIP-46 channel with user - // approval. The actual flow uses request_approval + - // respond_to_approval; this method exists to satisfy the object-safe - // trait and fails closed if a caller tries to bypass it. - Err(SigningError::Internal { - detail: - "NIP-46 signing uses the async approval flow; direct sign_event is not supported" - .to_string(), - }) + async fn sign_event(&self, event: UnsignedEvent) -> Result { + // The client half of NIP-46: ask the remote signer to sign, await the + // encrypted response, and verify the returned event is exactly what we + // asked for (identity + id + signature) before handing it back. A + // misbehaving or MITM'd signer cannot swap content or keys. + if !self.can_sign_event(event.kind.as_u16()) { + self.audit_permission_denied("sign_event").await; + return Err(SigningError::PermissionDenied { + method: "sign_event".to_string(), + }); + } + let unsigned_json = event.try_as_json().map_err(|e| SigningError::Internal { + detail: format!("Could not encode the event for signing: {e}"), + })?; + let response = self + .send_remote_request("sign_event", vec![unsigned_json]) + .await?; + let signed = Event::from_json(response.as_bytes()).map_err(|e| SigningError::Internal { + detail: format!("The signer returned an unreadable event: {e}"), + })?; + // The signer must sign WITH the identity it is connected as... + let signer_pubkey = Signer::get_public_key(self).await?; + if signed.pubkey != signer_pubkey { + return Err(SigningError::IdentityMismatch); + } + // ...the exact event we sent (same id covers pubkey, kind, tags, + // content, timestamp)... + if signed.id != event.compute_id() { + return Err(SigningError::InvalidSignature); + } + // ...and with a cryptographically valid signature. + signed + .verify() + .map_err(|_| SigningError::InvalidSignature)?; + Ok(signed) } fn get_signer_type(&self) -> SignerType { @@ -1009,6 +1185,19 @@ fn response_ok(id: &str, result: String) -> String { json!({ "id": id, "result": result, "error": null }).to_string() } +/// Whether a stored connection is usable *right now* (not revoked, not +/// expired), computed without taking the signer's async lock. The trait's +/// `is_connection_valid` cannot be used from contexts that already hold it. +fn connection_valid_now(conn: &Nip46Connection) -> bool { + if conn.revoked_at.is_some() { + return false; + } + match conn.expires_at { + Some(expires_at) => crate::vault::unix_timestamp().unwrap_or(0) < expires_at, + None => true, + } +} + fn response_err(id: &str, error: String) -> String { json!({ "id": id, "result": null, "error": error }).to_string() } diff --git a/src/uploads.rs b/src/uploads.rs index a1ed3ac..e194583 100644 --- a/src/uploads.rs +++ b/src/uploads.rs @@ -1,21 +1,22 @@ +use base64::engine::general_purpose::STANDARD as B64; +use base64::Engine; use nostr::nips::nip98::{HttpData, HttpMethod}; use nostr_sdk::prelude::*; -use crate::crypto::VaultKey; use crate::errors::{AppError, ErrorKind}; -use crate::profiles; +use crate::signer::Signing; -/// Sign a NIP-98 HTTP auth event for `url` with the active profile's key and -/// return the `Authorization` header value (`Nostr `). +/// Sign a NIP-98 HTTP auth event for `url` with the given [`Signing`] source +/// and return the `Authorization` header value (`Nostr `). /// /// This is what image hosts like nostr.build require before accepting an -/// upload. Like publishing, it needs an unlocked vault when the vault is -/// password-protected. +/// upload. It follows the same signer selection as publishing: an embedded +/// profile signs with the vault key, an external profile round-trips the +/// auth event through its connected NIP-46 signer. pub async fn nip98_authorization( - vault: &crate::vault::Vault, url: &str, method: &str, - key: Option<&VaultKey>, + signing: &Signing, ) -> Result { let http_method = match method.to_ascii_uppercase().as_str() { "GET" => HttpMethod::GET, @@ -33,112 +34,57 @@ pub async fn nip98_authorization( let parsed_url = Url::parse(url) .map_err(|e| AppError::config(format!("The upload URL is not valid: {e}")))?; - let secret_hex = profiles::resolve_active_secret_key(vault, key)?; - let secret_key = profiles::parse_secret_key(&secret_hex)?; - let keys = Keys::new(secret_key); - - let header = HttpData::new(parsed_url, http_method) - .to_authorization(&keys) + // Build the same event HttpData::to_authorization would build (kind + // 27235 with the u/method tags), but sign it through `Signing` so an + // external profile never needs a local secret. + let http_data = HttpData::new(parsed_url, http_method); + let pubkey = signing.pubkey().await.map_err(AppError::from)?; + let unsigned = IntoEventBuilder::into_event_builder(http_data).finalize_unsigned(pubkey); + let event = signing + .sign(unsigned) .await .map_err(|e| AppError::sign_failed(format!("Could not sign the upload request: {e}")))?; - Ok(header) + let encoded = B64.encode(event.as_json()); + Ok(format!("Nostr {encoded}")) } #[cfg(test)] mod tests { use super::*; - use crate::settings::Settings; - use crate::vault::Vault; - /// Settings with no relays so tests never touch the network. - fn offline_settings() -> Settings { - Settings { - relays: Vec::new(), - ..Default::default() - } - } - - fn vault_with_profile() -> Vault { - let mut vault = Vault::empty(); - crate::profiles::create_profile(&mut vault, "A".to_string(), None, &offline_settings()) - .unwrap(); - vault - } - - #[test] - fn missing_profile_errors() { - let vault = Vault::empty(); - let runtime = tokio::runtime::Runtime::new().unwrap(); - let err = runtime - .block_on(nip98_authorization( - &vault, - "https://nostr.build/api/v2/upload/files", - "POST", - None, - )) - .expect_err("no active profile must error"); - assert_eq!(err.kind(), ErrorKind::NoActiveProfile); + fn local_signing() -> Signing { + Signing::Local(Keys::generate()) } #[test] fn unsupported_method_errors() { - let vault = vault_with_profile(); + let signing = local_signing(); let runtime = tokio::runtime::Runtime::new().unwrap(); let err = runtime .block_on(nip98_authorization( - &vault, "https://example.com/upload", "DELETE", - None, + &signing, )) .expect_err("unsupported method must error"); assert_eq!(err.kind(), ErrorKind::Config); } #[test] - fn locked_encrypted_vault_errors() { - let mut vault = vault_with_profile(); - vault.crypto = Some(crate::vault::VaultCrypto { - kdf: crate::vault::KdfParams { - algorithm: "argon2id".to_string(), - salt: "c2FsdA==".to_string(), - m_cost: 1, - t_cost: 1, - p_cost: 1, - }, - verifier: "dmVyaWZpZXI=".to_string(), - }); - vault.profiles[0].secret_key = "encrypted-blob".to_string(); - let runtime = tokio::runtime::Runtime::new().unwrap(); - let err = runtime - .block_on(nip98_authorization( - &vault, - "https://nostr.build/api/v2/upload/files", - "POST", - None, - )) - .expect_err("locked vault must error"); - assert_eq!(err.kind(), ErrorKind::VaultLocked); - } - - #[test] - fn signs_a_nip98_auth_header_for_the_active_profile() { - let vault = vault_with_profile(); + fn signs_a_nip98_auth_header() { + let signing = local_signing(); let runtime = tokio::runtime::Runtime::new().unwrap(); let header = runtime .block_on(nip98_authorization( - &vault, "https://nostr.build/api/v2/upload/files", "POST", - None, + &signing, )) - .expect("valid profile must sign"); + .expect("local signing must produce a header"); assert!(header.starts_with("Nostr "), "expected a Nostr auth header"); let encoded = header.trim_start_matches("Nostr ").trim(); - use base64::engine::general_purpose::STANDARD as B64; - use base64::Engine as _; let raw = B64 .decode(encoded) .expect("the header payload must be base64"); From 6e5d80ba0b94d3e2faea3eaf277f8efff96cfec2 Mon Sep 17 00:00:00 2001 From: Avi Date: Thu, 10 Sep 2026 21:55:09 -0500 Subject: [PATCH 04/74] checkpoint: external NIP-46 signing end-to-end (Step 3 sub-step 2 done) --- CHECKPOINT-encryption.md | 71 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/CHECKPOINT-encryption.md b/CHECKPOINT-encryption.md index a7f2d34..2050b56 100644 --- a/CHECKPOINT-encryption.md +++ b/CHECKPOINT-encryption.md @@ -1,3 +1,74 @@ +# Checkpoint — External NIP-46 signing works end-to-end (2026-09-10) + +## Where things are +- Project: `/home/avi/Projects/Keynctr` +- Branch: `master` @ **`1af79d8`** ("feat(signer): end-to-end external NIP-46 signing in + publish and upload auth"). +- Working tree: clean for tracked files (untracked leftovers unchanged — see older + "Still untracked" sections). +- Verification: `cargo test` **200 passed / 0 failed**, `cargo clippy --all-targets` + clean, `cargo fmt --check` clean, `cargo build --release` green. + +## What was completed (this session) + +**Step 3 sub-step 2 (IPC reroute) — DONE, landed as `1af79d8`.** Publishing from an +external (NIP-46) profile now signs remotely instead of returning +"External signer not yet supported": + +- **`src/signer/nip46_client.rs`** — the outbound/client half of NIP-46: + `send_remote_request` encrypts a request (NIP-44) and publishes it to the signer's + relay; incoming payloads shaped like responses (`result`/`error`) are demultiplexed + to the waiting caller (a response with no registered id is ignored); 30 s + `REQUEST_TIMEOUT`; pending waiters are woken with errors on disconnect/failure so + callers never hang the full timeout. `Signer::sign_event` is real now: permission + check → remote `sign_event` → verify the returned event (a) is signed by the + connected remote identity, (b) matches the exact unsigned event requested, (c) has a + valid signature — then return it. **No local-key fallback anywhere.** + `audit_permission_denied` uses `try_lock` (audit is best-effort; blocking here would + deadlock the very request being denied while the IPC dispatcher holds the App lock). +- **`src/app.rs`** — `App::signing_for(npub)` / `App::signing_active()`: the single + place that maps a profile's `SignerMode` to a `Signing` source. + `Embedded` → `Signing::Local` with the vault-resolved key; `Nip46Client` → + `Signing::External` wrapping the live signer **only when present and connected**, + else `ExternalSignerNotConnected` (fail closed, never a silent local fallback); + `Nip46Bunker` (not wired) also fails closed. +- **`src/publish.rs`** — `publish_with_keys` builds the unsigned event from the + `Signing`'s own pubkey (external identities validate there before any relay work) + and signs via `Signing::sign`; new `publish_signed` entry point for IPC. The CLI's + `publish_active`/`publish_as` keep the local vault path. +- **`src/ipc.rs`** — `PublishNote` and `UploadAuth` route through + `app.signing_active()`; no handler branches on signer mode anymore. + `Nip46Connect`/`Nip46Disconnect` now clone the signer handle and **drop the App + guard before awaiting** `connect()`/`disconnect()` (they re-lock the App + internally — a latent deadlock, fixed). +- **`src/relays.rs`** — keyless relay pool: `open_pool_inner(Option, …)` so + external signing can publish/relay without local keys (no relay AUTH). +- **`src/uploads.rs`** — `nip98_authorization(url, method, &Signing)` — NIP-98 upload + auth events sign through the same `Signing` source, so uploads authenticate with + the remote signer for external profiles. +- **`src/profiles.rs`** — `store_remote_profile(vault, npub, label)`: connecting a + NIP-46 signer creates/refreshes a **secretless** `Nip46Client` profile row (empty + `secret_key`, made active) so publish has a selection; refuses to silently convert + an existing local profile into a remote one. `connect()` calls it and adopts the + remote npub as the signer's active profile. + +New tests (`src/app.rs`): embedded → `Signing::Local`; external profile with no live +signer → `ExternalSignerNotConnected` (fail closed); no active profile → +`NoActiveProfile`; `store_remote_profile` creates a secretless external profile and +refuses to clobber a local one. + +Security properties: remote-signed events are triple-verified (identity, content +match, signature) before publish; external profiles never touch a local key; the +connection secret stays vault-encrypted (Step 3 sub-step 1 unchanged). + +## Out of scope this session (deliberate) +- `publish_profile_metadata` (kind 0) still signs locally from the vault — a + synchronous key-based path; rerouting it is a separate follow-up. +- Dead `src/signer/nip46_external.rs` stub cleanup (untracked leftover). +- Step 4 (permissions UI), Step 5 (KDF upgrade), Step 7 (rename/hygiene). + +--- + # Checkpoint — Undo-delete restores working profiles (2026-09-10) ## Where things are From f917e5ecfde331484783f6e93a85ac15bbf70a36 Mon Sep 17 00:00:00 2001 From: Avi Date: Fri, 11 Sep 2026 11:08:09 -0500 Subject: [PATCH 05/74] =?UTF-8?q?fix(signer):=20Amber-compatible=20handsha?= =?UTF-8?q?ke=20=E2=80=94=20bunker://=20URIs,=20deferred=20identity,=20ack?= =?UTF-8?q?=20wait?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - parse_connect_uri accepts bunker:// as well as nostrconnect:// - URI authority key is no longer treated as identity (Amber mints a per-connection comms key); real identity learned via get_public_key after the connect ack, then persisted (profile row + secret re-key) - connect ack awaited in a spawned handshake task with a 120s human approval window; session stays Connecting (all signing fails closed) until identity is verified - absent perms= no longer locally denies signing; enforcement is delegated to the signer's approval UI - send_rpc honours its timeout parameter --- src/signer/nip46_client.rs | 288 +++++++++++++++++++++++++++---------- 1 file changed, 214 insertions(+), 74 deletions(-) diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index db578d6..4221df8 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -34,6 +34,9 @@ const MAX_PENDING_APPROVALS: usize = 20; /// How long an outbound NIP-46 request (e.g. our own `sign_event`) may wait /// for the remote signer's response before it is abandoned. const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +/// The connect handshake can involve a human approving the app on the +/// signer's screen, so it gets a far longer leash than ordinary RPCs. +const HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(120); /// Internal state for a pending approval. struct PendingApprovalInner { @@ -74,6 +77,12 @@ struct Nip46Inner { remote_pending: HashMap, keys: Option, active_npub: Option, + /// The remote signer's REAL identity key, learned from the + /// `get_public_key` RPC after the connect handshake completes. The + /// pubkey in the connect URI may be a per-connection communication key + /// (Amber's `bunker://` flow mints one per app) and must never be used + /// as an identity. `None` until the handshake resolves it. + identity: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -99,6 +108,7 @@ impl Nip46ClientSigner { remote_pending: HashMap::new(), keys: None, active_npub: None, + identity: None, })), app, } @@ -132,11 +142,23 @@ impl Nip46ClientSigner { } } - /// Parse a nostrconnect:// URI. + /// Parse a `nostrconnect://` or `bunker://` URI. + /// + /// Both carry the same wire shape (authority = the signer's public key, + /// `relay=` params, optional `secret=`). `nostrconnect://` is the + /// client-initiated flow (we generated the URI); `bunker://` is the + /// signer-initiated flow (Amber and self-hosted bunkers show one of + /// these). NOTE: with `bunker://` the authority key may be a + /// per-connection communication key (Amber mints one per app), NOT the + /// user's identity — identity is learned later via `get_public_key`. fn parse_connect_uri(raw: &str) -> Result { - let rest = raw.trim().strip_prefix("nostrconnect://").ok_or_else(|| { - AppError::config("Paste the nostrconnect:// link from your Nostr app.") - })?; + let rest = raw + .trim() + .strip_prefix("nostrconnect://") + .or_else(|| raw.trim().strip_prefix("bunker://")) + .ok_or_else(|| { + AppError::config("Paste the bunker:// or nostrconnect:// link from your Nostr app.") + })?; let (authority, query) = match rest.split_once('?') { Some((a, q)) => (a, Some(q)), @@ -262,8 +284,13 @@ impl Nip46ClientSigner { // Persist the connection AND its secret in the vault. The secret is // encrypted under the vault key (when a password is set) and keyed by // the connection's opaque VaultRef — never stored inline on the - // connection, never logged. Evaluates to the remote identity's npub. - let remote_npub = { + // connection, never logged. + // + // No profile is created yet: the URI key may be a per-connection + // communication key (Amber's bunker:// flow mints one per app), NOT + // the user's identity. The profile row is created by the handshake + // once `get_public_key` reveals the real identity. + { let mut app = self.app.lock().await; // Remove any existing connection for the same signer from the // same profile (reconnect replaces the old connection). @@ -287,19 +314,8 @@ impl Nip46ClientSigner { crate::vault::delete_connection_secret(&mut app.vault, &vault_ref); } app.vault.nip46_connections.push(connection.clone()); - // The remote identity gets its own profile row so publishing has - // a selection: marked Nip46Client with no local secret, so every - // signing path for it routes through this connection and key - // export refuses it. If the peer key collides with an existing - // local (embedded) profile the connect fails closed. - let remote_npub = PublicKey::from_hex(&connection.signer_pubkey) - .map_err(|e| AppError::internal(format!("Invalid signer public key: {e}")))? - .to_bech32() - .map_err(|e| AppError::internal(format!("Could not encode npub: {e}")))?; - profiles::store_remote_profile(&mut app.vault, &remote_npub, connection.label.clone())?; app.save_vault()?; - remote_npub - }; + } // Update state to connecting { @@ -313,9 +329,10 @@ impl Nip46ClientSigner { inner.connection = Some(connection.clone()); inner.conversation_key = Some(conversation); inner.keys = Some(keys.clone()); - // The signing identity is the remote signer's key, and the vault - // now holds a matching Nip46Client profile row (active). - inner.active_npub = Some(remote_npub); + // Identity is unknown until the handshake's `get_public_key` + // resolves it; nothing may sign before then. + inner.identity = None; + inner.active_npub = None; inner.pending.clear(); } @@ -504,6 +521,21 @@ impl Nip46ClientSigner { &self, method: &str, params: Vec, + ) -> Result { + self.send_rpc(method, params, REQUEST_TIMEOUT, true).await + } + + /// Send an encrypted NIP-46 RPC and await its response. + /// + /// `require_connected` gates on the session phase: ordinary RPCs need a + /// fully established session, while the handshake itself runs while the + /// phase is still `Connecting`. + async fn send_rpc( + &self, + method: &str, + params: Vec, + timeout: Duration, + require_connected: bool, ) -> Result { let id = uuid::Uuid::new_v4().to_string(); let (sender, receiver) = oneshot::channel(); @@ -529,7 +561,7 @@ impl Nip46ClientSigner { SigningError::ConnectionExpired }); } - if !matches!(inner.phase, Nip46Phase::Connected) { + if require_connected && !matches!(inner.phase, Nip46Phase::Connected) { return Err(SigningError::NotConnected); } if inner.remote_pending.len() >= MAX_PENDING_APPROVALS { @@ -555,7 +587,7 @@ impl Nip46ClientSigner { return Err(SigningError::Network { detail }); } - match tokio::time::timeout(REQUEST_TIMEOUT, receiver).await { + match tokio::time::timeout(timeout, receiver).await { Ok(Ok(result)) => result.map_err(|detail| SigningError::Internal { detail }), Ok(Err(_)) => { // The waiter was dropped (disconnect/fail) while awaiting. @@ -625,26 +657,47 @@ impl Nip46ClientSigner { .await .map_err(|e| format!("Could not subscribe: {e}"))?; - // Send connect request - self.send_connect(&client, &keys, &conversation, &uri) - .await?; - - // Mark as connected - self.inner.lock().await.phase = Nip46Phase::Connected; + // The connect handshake runs as its own task: it publishes `connect` + // and then must AWAIT the signer's ack — which can wait on a human + // approving us in Amber — followed by `get_public_key` to learn the + // REAL signing identity. Responses only arrive through the demux + // loop below, so the handshake must not block it. The session stays + // `Connecting` (and every signing path fails closed) until the + // handshake resolves the identity. + { + let handshake = self.clone(); + let peer = uri.peer; + let expected_secret = uri.secret.clone(); + tokio::spawn(async move { + if let Err(e) = handshake.clone().run_handshake(peer, expected_secret).await { + handshake.fail(e); + } + }); + } // Handle incoming requests loop { - let incoming = match notifications.next().await { - Some(nostr_sdk::client::ClientNotification::Event { - subscription_id, - event, - .. - }) if subscription_id == *subscription.id() => event, - Some(nostr_sdk::client::ClientNotification::Shutdown) | None => { - return Err("Connection closed".to_string()); - } - Some(_) => continue, - }; + let incoming = + match tokio::time::timeout(Duration::from_secs(2), notifications.next()).await { + Ok(Some(nostr_sdk::client::ClientNotification::Event { + subscription_id, + event, + .. + })) if subscription_id == *subscription.id() => event, + Ok(Some(nostr_sdk::client::ClientNotification::Shutdown)) | Ok(None) => { + return Err("Connection closed".to_string()); + } + Ok(Some(_)) => continue, + Err(_elapsed) => { + // Idle tick: if the handshake task failed, the session is + // dead — exit so teardown runs instead of listening on a + // connection that can never sign. + if matches!(self.inner.lock().await.phase, Nip46Phase::Error(_)) { + return Err("Connect handshake failed".to_string()); + } + continue; + } + }; let event = *incoming; if event.kind != Kind::NostrConnect || event.pubkey != uri.peer { @@ -932,31 +985,44 @@ impl Nip46ClientSigner { } } - async fn send_connect( - &self, - client: &Client, - keys: &Keys, - conversation: &ConversationKey, - uri: &ConnectUri, + /// The connect handshake, run as its own task alongside the demux loop. + /// + /// 1. Send `connect` (our client pubkey + the URI secret, resolved + /// on-demand from the vault — never from memory). + /// 2. Await the signer's ack. This is where a human approving us in + /// Amber happens, so the wait is long (HANDSHAKE_TIMEOUT). + /// 3. Call `get_public_key` to learn the REAL signing identity. The + /// pubkey in the connect URI may be a per-connection communication + /// key (Amber mints one per app); trusting it would publish under a + /// throwaway key. + /// 4. Persist the identity: create/refresh the secretless + /// `Nip46Client` profile row, re-key the vault secret store under + /// it, and only then flip the phase to `Connected`. + /// + /// Until step 4 completes the session is `Connecting`, so every signing + /// path fails closed — nothing can publish under an unverified key. + async fn run_handshake( + self, + peer: PublicKey, + expected_secret: Option, ) -> Result<(), String> { - // Resolve the nostrconnect secret ON-DEMAND from the vault — the single - // source of truth — rather than reading an in-memory copy. The - // connection carries no secret; it is keyed by its VaultRef and fetched - // fresh here. Fail-closed: if the vault cannot produce the secret - // (e.g. it is encrypted and currently locked) the connect is refused - // instead of being sent without it. - let secret = { + // Resolve the connect secret ON-DEMAND from the vault — the single + // source of truth. Fail-closed: if the vault cannot produce the + // secret the connect is refused rather than sent incomplete. + // (Also snapshot the connection label for the profile row below.) + let (secret, label, connect_ref) = { let connection = { let inner = self.inner.lock().await; inner.connection.clone() } .ok_or("No active NIP-46 connection to resolve the secret for")?; + let label = connection.label.clone(); let vault_ref = crate::signer::VaultRef::from_connection(&connection); let app = self.app.lock().await; // Copy the key out before the immutable borrow of the vault so the // two are never borrowed at once. let vault_key = app.vault_key().copied(); - match crate::vault::resolve_connection_secret( + let secret = match crate::vault::resolve_connection_secret( &app.vault, vault_key.as_ref(), &vault_ref, @@ -968,21 +1034,89 @@ impl Nip46ClientSigner { "Could not resolve the connection secret from the vault: {e}" )) } - } + }; + (secret, label, vault_ref) }; - let mut params = vec![keys.public_key().to_hex()]; + let mut params = vec![{ + let inner = self.inner.lock().await; + inner + .keys + .as_ref() + .ok_or("No session keys")? + .public_key() + .to_hex() + }]; if let Some(secret) = &secret { params.push(secret.clone()); } - let payload = json!({ - "id": uuid::Uuid::new_v4().to_string(), - "method": "connect", - "params": params, - }) - .to_string(); - self.publish_payload(client, keys, conversation, &uri.peer, &payload) + + // Await the ack (may wait on a human approving in Amber). + let ack = self + .send_rpc("connect", params, HANDSHAKE_TIMEOUT, false) .await + .map_err(|e| format!("The signer refused the connection: {e}"))?; + + // Anti-spoofing: per NIP-46, a signer answering a nostrconnect:// + // (secret-carrying) handshake must echo the secret back. A mismatch + // means something other than the intended signer answered. + if let Some(expected) = &expected_secret { + let trimmed = ack.trim(); + if trimmed != expected.as_str() { + return Err( + "The signer did not echo the connection secret; refusing the connection." + .to_string(), + ); + } + } + + // Learn the REAL signing identity from the signer itself. + let identity = self + .send_rpc("get_public_key", vec![], REQUEST_TIMEOUT, false) + .await + .map_err(|e| format!("The signer would not reveal its public key: {e}"))?; + let identity = PublicKey::from_hex(identity.trim()) + .map_err(|e| format!("The signer returned an unreadable public key: {e}"))?; + let identity_npub = identity + .to_bech32() + .map_err(|e| format!("Could not encode identity npub: {e}"))?; + + // Persist: profile row for the identity, connection + secret store + // re-keyed under it. Refuses (fails the handshake) if the identity + // collides with a local profile. + { + let mut app = self.app.lock().await; + profiles::store_remote_profile(&mut app.vault, &identity_npub, label) + .map_err(|e| e.message().to_string())?; + // Re-key the stored secret under the identity npub so the + // connection row, VaultRef, and secret store all agree. + if let Some(s) = &secret { + let new_ref = + crate::signer::VaultRef::new(Some(identity_npub.clone()), peer.to_hex()); + let vault_key = app.vault_key().copied(); + crate::vault::store_connection_secret( + &mut app.vault, + vault_key.as_ref(), + &new_ref, + s, + ) + .map_err(|e| e.message().to_string())?; + crate::vault::delete_connection_secret(&mut app.vault, &connect_ref); + } + if let Some(conn) = app.vault.nip46_connections.iter_mut().find(|c| { + c.signer_pubkey == peer.to_hex() && c.profile_npub != Some(identity_npub.clone()) + }) { + conn.profile_npub = Some(identity_npub.clone()); + } + app.save_vault().map_err(|e| e.message().to_string())?; + } + + // Identity verified: adopt it and open the session for signing. + let mut inner = self.inner.lock().await; + inner.identity = Some(identity); + inner.active_npub = Some(identity_npub); + inner.phase = Nip46Phase::Connected; + Ok(()) } async fn publish_payload( @@ -1021,13 +1155,10 @@ impl Clone for Nip46ClientSigner { impl Signer for Nip46ClientSigner { async fn get_public_key(&self) -> Result { let inner = self.inner.lock().await; - let connection = inner - .connection - .as_ref() - .ok_or(SigningError::NotConnected)?; - PublicKey::from_hex(&connection.signer_pubkey).map_err(|e| SigningError::Internal { - detail: format!("Invalid signer public key: {e}"), - }) + // The identity learned from the signer during the handshake — NOT + // the URI key, which for bunker:// flows is a per-connection comms + // key. Until the handshake resolves it the session is not usable. + inner.identity.clone().ok_or(SigningError::NotConnected) } async fn sign_event(&self, event: UnsignedEvent) -> Result { @@ -1120,31 +1251,40 @@ impl Signer for Nip46ClientSigner { .and_then(|c| c.permissions.clone()) } + // NOTE on the `None` arms below: when the connect URI declared no + // `perms=`, there is no local grant list to enforce — enforcement lives + // on the signer itself (Amber shows an approval screen per request). + // Sending the request and letting the signer decide is the NIP-46 flow; + // refusing locally would make bunker:// connections (which carry no + // perms) unusable. When perms WERE declared, the local list is enforced + // as an additional guard. fn can_sign_event(&self, kind: u16) -> bool { match self.permissions() { Some(ref perms) => perms.is_sign_event_kind_allowed(kind), - None => false, + None => true, } } fn can_encrypt(&self) -> bool { match self.permissions() { Some(ref perms) => perms.is_encrypt_allowed(), - None => false, + None => true, } } fn can_decrypt(&self) -> bool { match self.permissions() { Some(ref perms) => perms.is_decrypt_allowed(), - None => false, + None => true, } } fn can_get_public_key(&self) -> bool { + // `get_public_key` is part of the connect handshake for every + // signer; with no declared perms the signer still answers it. match self.permissions() { Some(ref perms) => perms.is_get_public_key_allowed(), - None => false, + None => true, } } From c09670530c2ae1a6178ee23ea2b12c4e33fe4ec6 Mon Sep 17 00:00:00 2001 From: Avi Date: Fri, 11 Sep 2026 15:11:00 -0500 Subject: [PATCH 06/74] fix(vault): stop rewriting the vault on every load; clippy cleanup - migrate_vault_signer_modes now reports a change only when the vault version actually moves. The unconditional 'changed = true' made App::load re-save the vault on every start (harmless, idempotent, but wasteful). Per-profile signer_mode normalisation was already a no-op: the serde default fills missing fields at parse time and the current version serialises it explicitly. - idempotency test tightened to assert changed == false for a current-version vault (previously ducked the question). - nip46_client.rs: drop clone-on-Copy in get_public_key (clippy). --- src/signer/nip46_client.rs | 2 +- src/vault.rs | 54 ++++++++++++++++++-------------------- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 4221df8..dcab6de 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -1158,7 +1158,7 @@ impl Signer for Nip46ClientSigner { // The identity learned from the signer during the handshake — NOT // the URI key, which for bunker:// flows is a per-connection comms // key. Until the handshake resolves it the session is not usable. - inner.identity.clone().ok_or(SigningError::NotConnected) + inner.identity.ok_or(SigningError::NotConnected) } async fn sign_event(&self, event: UnsignedEvent) -> Result { diff --git a/src/vault.rs b/src/vault.rs index 24d0702..d46ce04 100644 --- a/src/vault.rs +++ b/src/vault.rs @@ -356,35 +356,28 @@ pub fn parse_vault(content: &str) -> Result { /// left untouched. Only profiles with the legacy `None` value (or /// missing the field entirely) are assigned `Embedded`. /// -/// Returns `true` if any profiles were migrated (i.e. the vault should -/// be re-saved). +/// Missing `signer_mode` fields are assigned `Embedded` by the serde +/// default during deserialization, and every vault at the current +/// `VAULT_VERSION` serialises `signer_mode` explicitly — so once the +/// version bump below has been saved, re-saving adds nothing. A change +/// is therefore reported only when the version actually moves, instead +/// of on every load (the old unconditional `changed = true` made +/// `App::load` rewrite the vault on each start). +/// +/// Returns `true` if the vault was migrated (i.e. it should be re-saved). pub fn migrate_vault_signer_modes(vault: &mut Vault) -> bool { - let mut changed = false; - for _profile in &mut vault.profiles { - // The serde default already handles missing fields during - // deserialization, but once loaded, profiles that were stored - // before signer_mode was introduced will have the default value. - // We write it explicitly so the on-disk format is canonical. - // - // After the first save, every profile will have an explicit - // signer_mode and this becomes a no-op. - // - // We cannot distinguish "user explicitly set Embedded" from - // "serde defaulted to Embedded", so we always write it — this is - // safe because Embedded is the correct default and the write is - // idempotent. - changed = true; - } - // Also ensure the nip46_connections vector exists (serde default - // handles this during deserialization, but we normalise here too). - if vault.version < VAULT_VERSION { - vault.version = VAULT_VERSION; - changed = true; - } + // Profiles need no per-field work: the serde default already filled + // any missing `signer_mode` at parse time and serialization at the + // current version writes it explicitly. + // // Legacy connections without profile_npub (None) are left as-is. // Ownership cannot be reliably inferred from active_profile, so these // connections remain unusable until the user re-creates them. - changed + if vault.version < VAULT_VERSION { + vault.version = VAULT_VERSION; + return true; + } + false } /// Store (or replace) a NIP-46 connection secret in the vault, keyed by an @@ -852,11 +845,14 @@ mod tests { }); let changed1 = migrate_vault_signer_modes(&mut vault); - assert!(changed1, "first migration should report change"); + // Vault::empty() is already at the current version with an + // explicit signer_mode, so migration must report no change — + // the old always-true return made App::load rewrite the vault + // on every start. + assert!(!changed1, "current-version vault should not report change"); - let _changed2 = migrate_vault_signer_modes(&mut vault); - // The function always returns true because it normalises the version. - // The important thing is that running it twice doesn't corrupt data. + let changed2 = migrate_vault_signer_modes(&mut vault); + assert!(!changed2, "re-running migration stays a no-op"); assert_eq!(vault.profiles[0].signer_mode, SignerMode::Embedded); assert_eq!(vault.version, VAULT_VERSION); } From 84f11e615dd618162d81e2914f192fb7791291f0 Mon Sep 17 00:00:00 2001 From: Avi Date: Fri, 11 Sep 2026 15:13:16 -0500 Subject: [PATCH 07/74] checkpoint: vault-load rewrite fix (c096705) + Amber verify as next step --- CHECKPOINT-encryption.md | 56 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/CHECKPOINT-encryption.md b/CHECKPOINT-encryption.md index 2050b56..40fe156 100644 --- a/CHECKPOINT-encryption.md +++ b/CHECKPOINT-encryption.md @@ -1,3 +1,59 @@ +# Checkpoint — Vault-load rewrite fix + Amber handshake lands (2026-09-11) + +## Where things are +- Project: `/home/avi/Projects/Keynctr` +- Branch: `master` @ **`c096705`** ("fix(vault): stop rewriting the vault on every + load; clippy cleanup"). Previous: `f917e5e` (Amber-compatible handshake). +- Working tree: clean for tracked files (untracked leftovers unchanged — see older + "Still untracked" sections). +- Verification: `cargo test` **200 passed / 0 failed**, `cargo clippy --all-targets` + 0 warnings, `cargo fmt --check` clean, `cargo build --release` green. + (Frontend untouched this session; vite dev server still running on :5173.) + +## What was completed (this session) + +**Carried-forward open question — CLOSED, landed as `c096705`.** +`migrate_vault_signer_modes` used to return `changed = true` unconditionally, so +`App::load` re-encrypted and re-saved the vault on every single start. Now a change +is reported only when `vault.version` actually moves: per-profile `signer_mode` +normalisation was always a no-op (the serde default fills missing fields at parse +time and the current version serialises it explicitly). The idempotency test was +tightened to assert `changed == false` for a current-version vault. Also dropped a +clone-on-Copy in `nip46_client.rs::get_public_key` (clippy warning from `f917e5e`). + +**`f917e5e` (committed earlier today, before this session):** Amber-compatible +NIP-46 handshake — `bunker://` URIs accepted, URI authority key no longer treated +as identity (Amber mints a per-connection comms key; real identity learned via +`get_public_key` after the connect ack), 120 s human-approval window with the +session held in Connecting (fail closed), absent `perms=` delegates enforcement to +the signer, `send_rpc` honours its timeout. **On-device Amber round-trip has not +been re-verified since this commit — that is the next task.** + +## Commits added (newest first) +- `c096705` fix(vault): stop rewriting the vault on every load; clippy cleanup +- `f917e5e` fix(signer): Amber-compatible handshake — bunker:// URIs, deferred + identity, ack wait (landed earlier today) + +## How to reproduce / exercise +- Dev loop (from memory, unchanged): `npx vite --port 5173`, then + `NOSTR_GUI_DEV_URL=http://localhost:5173 KEYNCTR_ENABLE_GPU=1 npx electron .` in + `frontend/` (backend from `target/release/keynectr serve`). Rust edits need + rebuild + backend restart. +- Exercise vault fix: start the app twice; the vault file's mtime should NOT change + on the second start when nothing was modified. + +## Deferred / next steps +1. **Verify Amber end-to-end on device** (pair via Amber, sign a note, publish). +2. Dead stub `src/signer/nip46_external.rs` (untracked, superseded by + `nip46_client.rs`) — delete or fold its docs; `deferred/SignerConnectionPanel.tsx.wip/` + stays deferred. +3. `publish_profile_metadata` (kind 0) still signs locally — reroute through + `Signing` for external profiles. +4. Step 4 (permissions UI), Step 5 (KDF upgrade), Step 6 (undo history), + Step 7 (rename/hygiene incl. `homepage` URL + 5 Prettier files) — unchanged. + +--- + # Checkpoint — External NIP-46 signing works end-to-end (2026-09-10) ## Where things are From 85756df081e442e76f2c15014e3ec0d6697f9e53 Mon Sep 17 00:00:00 2001 From: Avi Date: Sat, 12 Sep 2026 02:40:40 -0500 Subject: [PATCH 08/74] feat(signer): accept bunker:// URIs, async signer permissions, NIP-46 e2e test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SignerManager/SignerModeScreen parse both nostrconnect:// and bunker:// (Amber presents bunker://; signer pubkey extracted before '@') - Signer permission surface made async (permissions, can_*, is_connection_valid) - tests/nip46_e2e.rs: full client handshake against fake Amber over a local relay — NIP-44 round-trip, get_public_key identity, signed-event verification, vault persistence asserting no secret material for remote profiles - prettier formatting of touched frontend files --- Cargo.lock | 2 + Cargo.toml | 7 + .../src/components/ExportSecretKeyModal.tsx | 8 +- frontend/src/lib/signer/SignerManager.ts | 17 +- frontend/src/screens/SignerModeScreen.tsx | 174 +++++-- frontend/src/state/AppProvider.tsx | 3 +- frontend/src/test/ExportSecretKey.test.tsx | 12 +- frontend/src/test/fakeBackend.ts | 26 +- src/signer/mod.rs | 24 +- src/signer/nip46_client.rs | 47 +- tests/nip46_e2e.rs | 450 ++++++++++++++++++ 11 files changed, 654 insertions(+), 116 deletions(-) create mode 100644 tests/nip46_e2e.rs diff --git a/Cargo.lock b/Cargo.lock index 8dd71c3..e893dd5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1169,6 +1169,7 @@ dependencies = [ "argon2", "async-trait", "base64", + "futures-util", "getrandom 0.2.17", "hex", "keyring", @@ -1179,6 +1180,7 @@ dependencies = [ "serde_json", "sha2 0.10.9", "tokio", + "tokio-tungstenite", "uuid", "zeroize", ] diff --git a/Cargo.toml b/Cargo.toml index 6212a43..cd5303f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,3 +20,10 @@ rpassword = "7" sha2 = "0.10" async-trait = "0.1" keyring = "4.2" + +[dev-dependencies] +base64 = "0.22" +futures-util = "0.3" +getrandom = "0.2" +nostr = "0.45" +tokio-tungstenite = "0.28" diff --git a/frontend/src/components/ExportSecretKeyModal.tsx b/frontend/src/components/ExportSecretKeyModal.tsx index 6aba1c4..c4f9ec5 100644 --- a/frontend/src/components/ExportSecretKeyModal.tsx +++ b/frontend/src/components/ExportSecretKeyModal.tsx @@ -89,9 +89,13 @@ export function ExportSecretKeyModal({ open, onClose, profile }: ExportSecretKey } else if (code === 'profile_not_found') { setFatal({ message: 'That profile is not stored on this computer.' }); setPhase('error'); - } else if (code === 'external_signer_not_connected' || code === 'external_signer_identity_mismatch') { + } else if ( + code === 'external_signer_not_connected' || + code === 'external_signer_identity_mismatch' + ) { setFatal({ - message: 'This profile uses an external signer. Secret key export is not possible for externally managed accounts.', + message: + 'This profile uses an external signer. Secret key export is not possible for externally managed accounts.', }); setPhase('error'); } else { diff --git a/frontend/src/lib/signer/SignerManager.ts b/frontend/src/lib/signer/SignerManager.ts index 063e451..5f58e79 100644 --- a/frontend/src/lib/signer/SignerManager.ts +++ b/frontend/src/lib/signer/SignerManager.ts @@ -322,14 +322,21 @@ export class SignerManager { // ==================== CLIENT MODE (connect TO external signer) ==================== - /** Parse nostrconnect:// URI from external signer (Amber, Nostr Connect, etc.) */ + /** Parse nostrconnect:// or bunker:// URI from external signer (Amber, Nostr Connect, etc.) */ parseExternalSignerURI(uri: string): ExternalSignerConnection { - if (!uri.startsWith('nostrconnect://')) { - throw new SignerError('INVALID_NOSTRCONNECT_URI', 'URI must start with nostrconnect://'); + const isBunker = uri.startsWith('bunker://'); + if (!uri.startsWith('nostrconnect://') && !isBunker) { + throw new SignerError( + 'INVALID_NOSTRCONNECT_URI', + 'URI must start with nostrconnect:// or bunker://', + ); } - const [authority, queryString] = uri.slice('nostrconnect://'.length).split('?'); - const signerPubkey = authority; + const withoutScheme = uri.slice(isBunker ? 'bunker://'.length : 'nostrconnect://'.length); + const [authorityRaw, queryString] = withoutScheme.split('?'); + // bunker://@?relay=… carries a display relay in the + // authority; the key is what precedes the '@'. + const signerPubkey = authorityRaw.split('@')[0]; const params = new URLSearchParams(queryString || ''); const relays = params.getAll('relay'); const secret = params.get('secret') || undefined; diff --git a/frontend/src/screens/SignerModeScreen.tsx b/frontend/src/screens/SignerModeScreen.tsx index dcabe76..2eeb4dd 100644 --- a/frontend/src/screens/SignerModeScreen.tsx +++ b/frontend/src/screens/SignerModeScreen.tsx @@ -33,11 +33,10 @@ export function SignerModeScreen() { // Single source of truth: backend state (defaults to most secure) const mode = (state?.signer_mode ?? 'nip46_client') as SignerMode; - const isNip46Active = (mode === 'nip46_client' || mode === 'nip46_bunker') && !!nip46StatusState?.connected; + const isNip46Active = + (mode === 'nip46_client' || mode === 'nip46_bunker') && !!nip46StatusState?.connected; const isEmbeddedActive = mode === 'embedded' && !!embeddedStatus?.available; - - const refreshStatus = useCallback(async () => { try { // Mode comes from AppProvider state, just refresh signer statuses @@ -53,14 +52,24 @@ export function SignerModeScreen() { const status = await embeddedSignerStatus(); setEmbeddedStatus(status); } catch { - setEmbeddedStatus({ type: 'embedded', available: false, pending_count: 0, pending: [] } as any); + setEmbeddedStatus({ + type: 'embedded', + available: false, + pending_count: 0, + pending: [], + } as any); } } else { try { const status = await nip46Status(); setNip46StatusState(status); } catch { - setNip46StatusState({ connected: false, relays: [], connected_relays: [], pending_approvals: [] } as any); + setNip46StatusState({ + connected: false, + relays: [], + connected_relays: [], + pending_approvals: [], + } as any); } } } catch (err) { @@ -96,12 +105,18 @@ export function SignerModeScreen() { await refresh(); } catch (err) { const msg = err instanceof Error ? err.message : String(err); - if (msg.includes('No keypair') || msg.includes('No active profile') || msg.includes('no active profile')) { + if ( + msg.includes('No keypair') || + msg.includes('No active profile') || + msg.includes('no active profile') + ) { setError('No keypair found: Please import a key first.'); } else if (msg.includes('vault_locked') || msg.toLowerCase().includes('vault locked')) { setError('Vault locked: Please unlock to switch modes.'); } else if (msg.includes('ACTIVE_SESSION') || msg.toLowerCase().includes('active session')) { - setError('Invalid mode transition: Cannot switch while active session exists. Disconnect first.'); + setError( + 'Invalid mode transition: Cannot switch while active session exists. Disconnect first.', + ); } else { setError(msg || 'That operation is not permitted.'); } @@ -112,8 +127,12 @@ export function SignerModeScreen() { const handleNip46Connect = useCallback(async () => { const trimmed = uri.trim(); - if (!trimmed.startsWith('nostrconnect://')) { - setError('Paste a nostrconnect:// link from Amber, Nostr Connect, or your bunker.'); + // Amber and self-hosted bunkers show a bunker:// link; Nostr Connect + // apps use nostrconnect://. Both are accepted by the backend parser. + if (!trimmed.startsWith('nostrconnect://') && !trimmed.startsWith('bunker://')) { + setError( + 'Paste a bunker:// or nostrconnect:// link from Amber, Nostr Connect, or your bunker.', + ); return; } setError(null); @@ -183,12 +202,16 @@ export function SignerModeScreen() { return isEmbeddedActive ? ( Embedded (Least Secure) ) : ( - Embedded {vaultLocked ? '(Vault Locked)' : ''} + + Embedded {vaultLocked ? '(Vault Locked)' : ''} + ); }; const handleImportKey = useCallback(async () => { - const nsec = prompt('Enter your nsec (npub will be derived) or leave blank to generate a new key:'); + const nsec = prompt( + 'Enter your nsec (npub will be derived) or leave blank to generate a new key:', + ); if (nsec === null) return; setError(null); try { @@ -237,11 +260,15 @@ export function SignerModeScreen() {
Keypair - {hasProfile ? `${state?.active_profile?.npub.slice(0, 16)}…` : 'Not imported'} + + {hasProfile ? `${state?.active_profile?.npub.slice(0, 16)}…` : 'Not imported'} +
Vault - {vaultLocked ? 'Locked' : 'Unlocked / No password'} + + {vaultLocked ? 'Locked' : 'Unlocked / No password'} +
Current Mode @@ -249,12 +276,20 @@ export function SignerModeScreen() {
{!hasProfile && ( - )} {hasProfile && vaultLocked && ( - )} @@ -269,7 +304,9 @@ export function SignerModeScreen() {
{/* 1. Most Secure */} -
{/* 2. Moderately Secure */} -
{/* 3. Least Secure */} -