From dde37183e3fe7328d5ae678e09e706b90ef4faec Mon Sep 17 00:00:00 2001 From: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Date: Sat, 25 Jul 2026 23:45:09 -0400 Subject: [PATCH] Address implementation-review blockers: import ordering, site-granular inventory, uppercase bech32 Blocker 1: import_identity persisted-before-cleanup ordering. New commit_imported_identity helper runs durable persistence FIRST; a failed different-key import now leaves both the old in-memory identity and its valid canonical identity.ncryptsec intact. Stale-backup cleanup runs after the durable commit and is deliberately best-effort (logged, not surfaced as a half-applied-import error). Regression tests cover the failure path and prove cleanup cannot precede persist. Blocker 2: the /events inventory tripwire is now site-granular. Each inventoried file pairs an expected non-comment /events occurrence count with an expected egress-guard call count, so an unguarded ninth site in an already-listed file (or a deleted guard call) fails the scan. The scan core is pure over (path, content) pairs, with mutation-style tests demonstrating all three drift classes. Hardening: bech32 permits an all-uppercase encoding, so the egress guard now rejects NCRYPTSEC1... too (mixed case stays unblocked - it cannot decode), and both import classifiers (Rust recover_keys_from_input, TS classifyKeyImportInput/isPlausibleNcryptsec) route uppercase-valid blobs to the encrypted path consistently. Co-authored-by: Tyler Longwell Signed-off-by: Tyler Longwell --- desktop/src-tauri/src/commands/identity.rs | 172 ++++++++++++--- desktop/src-tauri/src/egress_guard.rs | 14 +- desktop/src-tauri/src/egress_guard_tests.rs | 202 +++++++++++++++--- desktop/src-tauri/src/key_backup.rs | 11 +- desktop/src-tauri/src/key_backup_tests.rs | 22 ++ .../onboarding/lib/keyImportInput.test.mjs | 16 ++ .../features/onboarding/lib/keyImportInput.ts | 14 +- 7 files changed, 379 insertions(+), 72 deletions(-) diff --git a/desktop/src-tauri/src/commands/identity.rs b/desktop/src-tauri/src/commands/identity.rs index e723a5637..bb3ef796b 100644 --- a/desktop/src-tauri/src/commands/identity.rs +++ b/desktop/src-tauri/src/commands/identity.rs @@ -326,35 +326,14 @@ pub async fn import_identity( std::fs::create_dir_all(&data_dir).map_err(|e| format!("create app data dir: {e}"))?; let key_path = data_dir.join("identity.key"); - // Importing a different identity invalidates the app-managed backup: - // it encrypts the previous key and must not linger mislabeled. - let previous_pubkey = state.keys.lock().map_err(|e| e.to_string())?.public_key(); - crate::key_backup::cleanup_stale_backup(&previous_pubkey, &keys.public_key(), &data_dir)?; - - // Persist into the OS keyring first (store → read-back verify → marker → - // delete file). Falls back to the 0o600 file when the keyring is - // unavailable; returns Err only when both backends fail. - let store = crate::secret_store::SecretStore::shared(crate::app_state::keyring_service()); - crate::app_state::persist_imported_identity(store, &keys, &key_path, &data_dir)?; - - // Update in-memory keys BEFORE clearing recovery flags. The Release - // stores below pair with Acquire loads in get_identity: a reader - // observing false is guaranteed to see the updated keys. - let pubkey = keys.public_key(); - *state.keys.lock().map_err(|e| e.to_string())? = keys; - - // Clear both recovery flags — an import is valid in either lost or - // keyring-locked state and resolves both. In the locked case the - // keyring is unreachable, so persist_imported_identity already fell - // back to identity.key; on the next Unreachable boot the file is - // loaded directly and when the keyring returns the adoption path - // picks it up. - state - .identity_lost - .store(false, std::sync::atomic::Ordering::Release); - state - .keyring_locked - .store(false, std::sync::atomic::Ordering::Release); + let pubkey = commit_imported_identity(&state, &data_dir, keys, |keys| { + // Persist into the OS keyring first (store → read-back verify → + // marker → delete file). Falls back to the 0o600 file when the + // keyring is unavailable; returns Err only when both backends fail. + let store = + crate::secret_store::SecretStore::shared(crate::app_state::keyring_service()); + crate::app_state::persist_imported_identity(store, keys, &key_path, &data_dir) + })?; let pubkey_hex = pubkey.to_hex(); let display_name = truncated_display_name(&pubkey)?; @@ -373,6 +352,65 @@ pub async fn import_identity( .map_err(|e| format!("spawn_blocking failed: {e}"))? } +/// Commit an imported identity: durably persist, swap in-memory keys, clear +/// recovery flags, then remove the previous identity's stale app-managed +/// backup. Caller must hold `state.identity_mutation`. +/// +/// Ordering is the contract: +/// +/// 1. `persist` runs FIRST. If it fails (`Err` from both keyring and file +/// fallback), nothing has changed — the previous identity stays live in +/// memory AND its valid canonical `identity.ncryptsec` stays on disk. +/// 2. Only after durable persistence do we swap `state.keys` and clear the +/// recovery flags. +/// 3. Stale-backup cleanup runs LAST and is deliberately best-effort: at that +/// point the import is durably committed, so reporting a cleanup failure +/// as a command `Err` would claim a half-applied import that actually +/// succeeded. The leftover blob is still passphrase-encrypted and is +/// replaced by the next backup creation; we log and move on. +fn commit_imported_identity( + state: &AppState, + data_dir: &std::path::Path, + keys: nostr::Keys, + persist: impl FnOnce(&nostr::Keys) -> Result<(), String>, +) -> Result { + // Capture the previous pubkey up front for post-commit cleanup. + let previous_pubkey = state.keys.lock().map_err(|e| e.to_string())?.public_key(); + + persist(&keys)?; + + // Update in-memory keys BEFORE clearing recovery flags. The Release + // stores below pair with Acquire loads in get_identity: a reader + // observing false is guaranteed to see the updated keys. + let pubkey = keys.public_key(); + *state.keys.lock().map_err(|e| e.to_string())? = keys; + + // Clear both recovery flags — an import is valid in either lost or + // keyring-locked state and resolves both. In the locked case the + // keyring is unreachable, so the persist step already fell back to + // identity.key; on the next Unreachable boot the file is loaded + // directly and when the keyring returns the adoption path picks it up. + state + .identity_lost + .store(false, std::sync::atomic::Ordering::Release); + state + .keyring_locked + .store(false, std::sync::atomic::Ordering::Release); + + // Importing a different identity invalidates the app-managed backup: it + // encrypts the previous key and must not linger mislabeled. Best-effort + // per the ordering contract above. + if let Err(e) = crate::key_backup::cleanup_stale_backup(&previous_pubkey, &pubkey, data_dir) { + eprintln!( + "buzz-desktop: import committed, but stale key backup cleanup failed: {e}; \ + the leftover identity.ncryptsec encrypts the PREVIOUS key and will be \ + replaced by the next backup creation" + ); + } + + Ok(pubkey) +} + /// Make the current ephemeral identity durable by persisting it to the OS /// keyring (or falling back to identity.key). This is called when the user /// chooses to start a new identity instead of re-importing their previous one @@ -798,6 +836,82 @@ mod key_backup_command_tests { assert!(!crate::key_backup::backup_file_path(dir.path()).exists()); } + /// Blocker-1 regression (Wren, implementation review): a failed + /// different-key import must leave BOTH the old in-memory identity and + /// the old canonical backup intact. Persistence runs before cleanup, so + /// an `Err` from persist means nothing was mutated or deleted. + #[test] + fn failed_import_persistence_preserves_old_identity_and_backup() { + let state = build_app_state(); + let dir = tempfile::tempdir().unwrap(); + let old_pubkey = state.keys.lock().unwrap().public_key(); + + // A valid canonical backup for the live (old) identity. + create_and_persist_backup_with_log_n(&state, dir.path(), PASSWORD, FAST_LOG_N).unwrap(); + let backup_path = crate::key_backup::backup_file_path(dir.path()); + let backup_before = std::fs::read_to_string(&backup_path).unwrap(); + + // Different-key import whose durable persistence fails (both + // keyring and file fallback down). + let _guard = state.identity_mutation.lock().unwrap(); + let err = super::commit_imported_identity(&state, dir.path(), Keys::generate(), |_| { + Err("keyring and file both unavailable".to_string()) + }) + .unwrap_err(); + assert!(err.contains("unavailable"), "{err}"); + + // Old identity still live; old backup untouched byte-for-byte. + assert_eq!(state.keys.lock().unwrap().public_key(), old_pubkey); + assert_eq!( + std::fs::read_to_string(&backup_path).unwrap(), + backup_before + ); + assert!( + crate::key_backup::decrypt_ncryptsec(&backup_before, PASSWORD) + .unwrap() + .public_key() + == old_pubkey, + "surviving backup must still recover the still-live identity" + ); + } + + /// Successful different-key import removes the previous identity's + /// backup — cleanup runs after the durable commit, not before. + #[test] + fn successful_import_removes_stale_backup_after_commit() { + let state = build_app_state(); + let dir = tempfile::tempdir().unwrap(); + + create_and_persist_backup_with_log_n(&state, dir.path(), PASSWORD, FAST_LOG_N).unwrap(); + let backup_path = crate::key_backup::backup_file_path(dir.path()); + assert!(backup_path.exists()); + + let new_keys = Keys::generate(); + let backup_present_at_persist = std::cell::Cell::new(false); + let _guard = state.identity_mutation.lock().unwrap(); + let pubkey = super::commit_imported_identity(&state, dir.path(), new_keys.clone(), |_| { + // Ordering probe: the old backup must still exist while + // persistence is running (cleanup has not happened yet). + backup_present_at_persist.set(backup_path.exists()); + Ok(()) + }) + .unwrap(); + + assert!( + backup_present_at_persist.get(), + "cleanup must not precede persist" + ); + assert_eq!(pubkey, new_keys.public_key()); + assert_eq!( + state.keys.lock().unwrap().public_key(), + new_keys.public_key() + ); + assert!( + !backup_path.exists(), + "stale backup must be removed post-commit" + ); + } + /// Concurrent identity swap vs backup creation: `identity_mutation` /// serializes both, so every persisted blob decrypts to the identity that /// was live for the whole of its create operation — never a torn state. diff --git a/desktop/src-tauri/src/egress_guard.rs b/desktop/src-tauri/src/egress_guard.rs index 6dc2141a6..01c04bf70 100644 --- a/desktop/src-tauri/src/egress_guard.rs +++ b/desktop/src-tauri/src/egress_guard.rs @@ -25,14 +25,20 @@ /// Bech32 HRP of NIP-49 encrypted secret keys. const NCRYPTSEC_PREFIX: &str = "ncryptsec1"; +/// Bech32 also permits an ALL-UPPERCASE encoding of the same payload +/// (BIP-173); an uppercased valid backup decodes identically, so the guard +/// must reject it too. Mixed case is invalid bech32 and cannot decode — a +/// substring matching either all-lower or all-upper prefix covers every +/// decodable form. +const NCRYPTSEC_PREFIX_UPPER: &str = "NCRYPTSEC1"; /// Reject `text` if it contains NIP-49 key-backup material. /// -/// Returns `Err` when an `ncryptsec1…` substring is present. Callers MUST -/// abort the network operation on `Err` — this is a fail-closed guard, not a -/// warning. +/// Returns `Err` when an `ncryptsec1…` (or uppercase `NCRYPTSEC1…`) +/// substring is present. Callers MUST abort the network operation on `Err` — +/// this is a fail-closed guard, not a warning. pub fn assert_no_key_backup(text: &str, context: &'static str) -> Result<(), String> { - if text.contains(NCRYPTSEC_PREFIX) { + if text.contains(NCRYPTSEC_PREFIX) || text.contains(NCRYPTSEC_PREFIX_UPPER) { return Err(format!( "blocked {context}: payload contains NIP-49 key-backup material \ (ncryptsec); the local key backup must never be transmitted to a relay" diff --git a/desktop/src-tauri/src/egress_guard_tests.rs b/desktop/src-tauri/src/egress_guard_tests.rs index 7a3f8a649..cc8cd483c 100644 --- a/desktop/src-tauri/src/egress_guard_tests.rs +++ b/desktop/src-tauri/src/egress_guard_tests.rs @@ -24,6 +24,19 @@ fn rejects_ncryptsec_anywhere_in_text() { ); } +/// Bech32 permits an all-uppercase encoding of the same payload — an +/// uppercased valid backup must not bypass the guard (text and bytes). +/// Mixed case is invalid bech32 (cannot decode) and is deliberately not +/// blocked. +#[test] +fn rejects_uppercase_ncryptsec() { + let upper = NCRYPTSEC.to_ascii_uppercase(); + assert_guard_error(&assert_no_key_backup(&upper, "test").unwrap_err()); + assert_guard_error(&assert_no_key_backup_bytes(upper.as_bytes(), "test").unwrap_err()); + // Mixed case cannot decode; not blocked. + assert!(assert_no_key_backup("nCrYpTsEc1qgg9947r", "test").is_ok()); +} + #[test] fn passes_clean_payloads_including_raw_nsec() { assert!(assert_no_key_backup("hello world", "test").is_ok()); @@ -207,55 +220,174 @@ fn src_rust_files() -> Vec { out } -/// Inventory completeness: every `/events` URL-construction site in -/// `desktop/src-tauri/src` must be in the guarded set. A future ninth -/// submission path fails this test until its guard is wired and it is added -/// to the allowlist below (with its egress_guard.rs table row + injection -/// test). -#[test] -fn events_url_inventory_is_fully_guarded() { - // (file suffix, guarded construction sites expected in that file) - let allowlist: &[&str] = &[ - "src/relay.rs", // boundaries 2, 3, 4 - "src/relay/submit.rs", // boundary 1 - "src/huddle/pipeline.rs", // boundary 5 - "src/commands/team_snapshot.rs", // boundary 6 - "src/commands/personas/snapshot/import.rs", // boundary 7 - // test-only relay stubs / fixtures (no production egress): - "src/relay_admission.rs", - "src/archive/mod_tests.rs", - "src/managed_agents/persona_events/tests.rs", - "src/commands/team_snapshot/tests.rs", - "src/egress_guard_tests.rs", - ]; +/// Site-granular `/events` inventory: `(file suffix, expected non-comment +/// `/events` occurrences, expected guard call sites — full-path calls into +/// the egress-guard module)`. +/// +/// Every entry pairs the URL-construction count with the guard-call count for +/// that file, so BOTH of these fail the scan (not just a brand-new file): +/// - adding an unguarded ninth `/events` site inside an already-listed file +/// (count goes up without a matching table update), and +/// - removing/refactoring away a guard call while its egress site remains. +/// +/// Updating a row here is the deliberate act that must accompany wiring the +/// guard + adding an injection test for the new site. +const EVENTS_INVENTORY: &[(&str, usize, usize)] = &[ + // Production egress boundaries (see egress_guard.rs table): + ("src/relay.rs", 3, 3), // boundaries 2, 3, 4 + ("src/relay/submit.rs", 1, 1), // boundary 1 + ("src/huddle/pipeline.rs", 1, 1), // boundary 5 + ("src/commands/team_snapshot.rs", 1, 1), // boundary 6 + ("src/commands/personas/snapshot/import.rs", 2, 1), // boundary 7 + its in-file injection-test fixture URL + ("src/native_websocket.rs", 0, 2), // boundary 8 (WS frames; no events URL) + // Test-only fixtures — no production egress, no guard: + ("src/relay_admission.rs", 1, 0), + ("src/archive/mod_tests.rs", 1, 0), + ("src/managed_agents/persona_events/tests.rs", 1, 0), + ("src/commands/team_snapshot/tests.rs", 1, 0), +]; - let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); +// Needles are assembled at runtime so this scan file itself contains no +// contiguous match and needs no self-referential inventory row. +fn events_needle() -> String { + ["/ev", "ents"].concat() +} +fn guard_needle() -> String { + ["egress_guard::", "assert_no_key_backup"].concat() +} + +/// Pure scan core over `(relative path, content)` pairs. Returns violations; +/// empty means every file matches its inventory row exactly (files absent +/// from the table are expected to have zero `/events` sites and zero guard +/// calls). +fn events_inventory_violations(files: &[(String, String)]) -> Vec { + let events = events_needle(); + let guard = guard_needle(); let mut violations = Vec::new(); - for path in src_rust_files() { - let rel = path - .strip_prefix(root) - .unwrap() - .to_string_lossy() - .replace('\\', "/"); - let content = std::fs::read_to_string(&path).unwrap(); + + for (rel, content) in files { + let expected = EVENTS_INVENTORY + .iter() + .find(|(suffix, _, _)| rel.ends_with(suffix)) + .map(|&(_, e, g)| (e, g)) + .unwrap_or((0, 0)); + + let mut event_sites = Vec::new(); for (i, line) in content.lines().enumerate() { - let trimmed = line.trim_start(); - if trimmed.starts_with("//") { + if line.trim_start().starts_with("//") { continue; // doc/comment mentions } - if line.contains("/events") && !allowlist.iter().any(|a| rel.ends_with(a)) { - violations.push(format!("{rel}:{}: {}", i + 1, line.trim())); + if line.contains(&events) { + event_sites.push(format!(" {rel}:{}: {}", i + 1, line.trim())); } } + let guard_count = content.matches(&guard).count(); + + if (event_sites.len(), guard_count) != expected { + violations.push(format!( + "{rel}: found {} events-URL site(s) + {} guard call(s), inventory \ + expects {} + {}. Sites found:\n{}", + event_sites.len(), + guard_count, + expected.0, + expected.1, + if event_sites.is_empty() { + " (none)".to_string() + } else { + event_sites.join("\n") + }, + )); + } } + violations +} + +fn read_src_files() -> Vec<(String, String)> { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")); + src_rust_files() + .into_iter() + .map(|path| { + let rel = path + .strip_prefix(root) + .unwrap() + .to_string_lossy() + .replace('\\', "/"); + let content = std::fs::read_to_string(&path).unwrap(); + (rel, content) + }) + .collect() +} + +/// Inventory completeness: every `/events` URL-construction site in +/// `desktop/src-tauri/src` must match the site-granular inventory above. A +/// future ninth submission path — in a NEW file or an ALREADY-LISTED one — +/// fails this test until its guard is wired, its injection test exists, and +/// its inventory row is updated. +#[test] +fn events_url_inventory_is_fully_guarded() { + let violations = events_inventory_violations(&read_src_files()); assert!( violations.is_empty(), - "new `/events` egress site(s) outside the guarded inventory — wire \ - crate::egress_guard and add an injection test before allowlisting:\n{}", + "events-URL egress inventory drift — wire crate::egress_guard, add an \ + injection test, then update EVENTS_INVENTORY:\n{}", violations.join("\n") ); } +/// Mutation-style proof of the tripwire's guarantee: an unguarded ninth +/// `/events` site added to an already-inventoried file (relay.rs) is caught. +#[test] +fn inventory_scan_catches_new_site_in_allowlisted_file() { + let mut files = read_src_files(); + let relay = files + .iter_mut() + .find(|(rel, _)| rel.ends_with("src/relay.rs")) + .expect("relay.rs must be in the scan set"); + relay.1.push_str(&format!( + "\nfn sneaky_ninth_site(base: &str) -> String {{ format!(\"{{base}}{}\") }}\n", + events_needle() + )); + let violations = events_inventory_violations(&files); + assert!( + violations.iter().any(|v| v.contains("src/relay.rs")), + "an unguarded ninth events-URL site in relay.rs must trip the scan: {violations:?}" + ); +} + +/// The pairing also fires in reverse: a guard call deleted while its egress +/// site remains is caught. +#[test] +fn inventory_scan_catches_removed_guard_call() { + let mut files = read_src_files(); + let relay = files + .iter_mut() + .find(|(rel, _)| rel.ends_with("src/relay.rs")) + .expect("relay.rs must be in the scan set"); + relay.1 = relay.1.replacen(&guard_needle(), "removed_guard", 1); + let violations = events_inventory_violations(&files); + assert!( + violations.iter().any(|v| v.contains("src/relay.rs")), + "a removed guard call in relay.rs must trip the scan: {violations:?}" + ); +} + +/// A brand-new file with an `/events` site (no inventory row) is caught. +#[test] +fn inventory_scan_catches_new_unlisted_file() { + let mut files = read_src_files(); + files.push(( + "src/brand_new_egress.rs".to_string(), + format!("let url = format!(\"{{}}{}\", base);", events_needle()), + )); + let violations = events_inventory_violations(&files); + assert!( + violations + .iter() + .any(|v| v.contains("src/brand_new_egress.rs")), + "{violations:?}" + ); +} + /// Source allowlist: NIP-49 material handling is confined to the identity / /// backup / import / guard files. Anything else touching ncryptsec or the /// nip49 codec is structural drift. diff --git a/desktop/src-tauri/src/key_backup.rs b/desktop/src-tauri/src/key_backup.rs index fe7b110fd..6db9eaa38 100644 --- a/desktop/src-tauri/src/key_backup.rs +++ b/desktop/src-tauri/src/key_backup.rs @@ -107,9 +107,18 @@ pub fn decrypt_ncryptsec(input: &str, password: &str) -> Result { /// Recover identity keys from an import input: `ncryptsec1…` (requires the /// passphrase, decrypted in Rust) or anything `Keys::parse` accepts (raw /// `nsec1…`/hex — byte-for-byte the pre-NIP-49 path). +/// +/// Classification is case-insensitive on the HRP: bech32 permits an +/// ALL-UPPERCASE encoding, so `NCRYPTSEC1…` routes to the encrypted path +/// (where the bech32 decoder accepts it); mixed case routes there too and +/// fails parsing with the accurate "invalid ncryptsec" error rather than +/// falling through to the raw-key parser. pub fn recover_keys_from_input(input: &str, password: Option<&str>) -> Result { let trimmed = input.trim(); - if trimmed.starts_with(NCRYPTSEC_HRP) { + let hrp_match = trimmed + .get(..NCRYPTSEC_HRP.len()) + .is_some_and(|head| head.eq_ignore_ascii_case(NCRYPTSEC_HRP)); + if hrp_match { let password = password.ok_or_else(|| "encrypted backup requires a passphrase".to_string())?; decrypt_ncryptsec(trimmed, password) diff --git a/desktop/src-tauri/src/key_backup_tests.rs b/desktop/src-tauri/src/key_backup_tests.rs index f2c77016b..2f92343f2 100644 --- a/desktop/src-tauri/src/key_backup_tests.rs +++ b/desktop/src-tauri/src/key_backup_tests.rs @@ -99,6 +99,28 @@ fn recover_keys_ncryptsec_wrong_password() { assert_eq!(err, "wrong passphrase or corrupted backup"); } +/// Bech32 permits an all-uppercase encoding: `NCRYPTSEC1…` must classify as +/// an encrypted backup (matching the egress guard's blocking scope), never +/// fall through to the raw-key parser. Mixed case classifies encrypted too +/// and fails with the accurate ncryptsec error, not "Invalid private key". +#[test] +fn recover_keys_uppercase_ncryptsec_classifies_as_encrypted() { + let upper = SPEC_NCRYPTSEC.to_ascii_uppercase(); + // Routing proof: encrypted path demands a passphrase. + let err = recover_keys_from_input(&upper, None).unwrap_err(); + assert_eq!(err, "encrypted backup requires a passphrase"); + // With the passphrase, the bech32 decoder accepts the uppercase form. + let keys = recover_keys_from_input(&upper, Some("nostr")).unwrap(); + assert_eq!(keys.secret_key().to_secret_hex(), SPEC_SECRET_HEX); + + // Mixed case: still routed to the encrypted path, rejected as invalid + // ncryptsec (mixed-case bech32 cannot decode). + let mut mixed = SPEC_NCRYPTSEC.to_string(); + mixed.replace_range(0..1, "N"); + let err = recover_keys_from_input(&mixed, Some("nostr")).unwrap_err(); + assert!(err.contains("invalid ncryptsec"), "{err}"); +} + #[test] fn recover_keys_raw_nsec_path_unchanged() { let keys = Keys::generate(); diff --git a/desktop/src/features/onboarding/lib/keyImportInput.test.mjs b/desktop/src/features/onboarding/lib/keyImportInput.test.mjs index 8c5d8047f..1a01da219 100644 --- a/desktop/src/features/onboarding/lib/keyImportInput.test.mjs +++ b/desktop/src/features/onboarding/lib/keyImportInput.test.mjs @@ -28,6 +28,22 @@ test("classify_by_hrp_with_whitespace_tolerance", () => { assert.equal(classifyKeyImportInput("nsec1"), "nsec"); }); +test("uppercase_bech32_encoding_classifies_and_gates_like_lowercase", () => { + // Bech32 permits an all-uppercase encoding; it must route to the + // encrypted path (matching Rust) and be submit-plausible. + const upper = NCRYPTSEC.toUpperCase(); + assert.equal(classifyKeyImportInput(upper), "ncryptsec"); + assert.equal(isPlausibleNcryptsec(upper), true); + assert.equal(keyImportSubmitEnabled(upper, ""), false); + assert.equal(keyImportSubmitEnabled(upper, "hunter2hunter2"), true); + // Mixed case: routed encrypted (Rust reports the accurate error) but + // never plausible/submittable — mixed-case bech32 cannot decode. + const mixed = `N${NCRYPTSEC.slice(1)}`; + assert.equal(classifyKeyImportInput(mixed), "ncryptsec"); + assert.equal(isPlausibleNcryptsec(mixed), false); + assert.equal(keyImportSubmitEnabled(mixed, "hunter2hunter2"), false); +}); + test("plausible_ncryptsec_requires_bech32_charset", () => { assert.equal(isPlausibleNcryptsec(NCRYPTSEC), true); // '1' and 'b' / 'i' / 'o' are not in the bech32 charset. diff --git a/desktop/src/features/onboarding/lib/keyImportInput.ts b/desktop/src/features/onboarding/lib/keyImportInput.ts index 2477154c9..12e76054e 100644 --- a/desktop/src/features/onboarding/lib/keyImportInput.ts +++ b/desktop/src/features/onboarding/lib/keyImportInput.ts @@ -12,12 +12,20 @@ import { nsecToNpub } from "@/shared/lib/nostrUtils"; export type KeyImportKind = "nsec" | "ncryptsec" | "unknown"; -/** Bech32 charset after the `ncryptsec1` HRP; anything else can't decode. */ -const NCRYPTSEC_SHAPE = /^ncryptsec1[02-9ac-hj-np-z]{10,}$/; +/** + * Bech32 charset after the `ncryptsec1` HRP; anything else can't decode. + * Bech32 also permits an ALL-UPPERCASE encoding of the same payload, so both + * casings are plausible (mixed case is invalid and stays implausible). + */ +const NCRYPTSEC_SHAPE = + /^(?:ncryptsec1[02-9ac-hj-np-z]{10,}|NCRYPTSEC1[02-9AC-HJ-NP-Z]{10,})$/; export function classifyKeyImportInput(input: string): KeyImportKind { const trimmed = input.trim(); - if (trimmed.startsWith("ncryptsec1")) return "ncryptsec"; + // Case-insensitive on the HRP to match the Rust classifier: an uppercase + // valid backup routes to the encrypted path (and decodes there); mixed + // case routes there too and fails in Rust with the accurate error. + if (trimmed.slice(0, 10).toLowerCase() === "ncryptsec1") return "ncryptsec"; if (trimmed.startsWith("nsec1")) return "nsec"; return "unknown"; }