mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
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 <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
This commit is contained in:
co-authored by
Tyler Longwell
parent
1c01889929
commit
dde37183e3
@@ -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<nostr::PublicKey, String> {
|
||||
// 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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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<std::path::PathBuf> {
|
||||
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<String> {
|
||||
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.
|
||||
|
||||
@@ -107,9 +107,18 @@ pub fn decrypt_ncryptsec(input: &str, password: &str) -> Result<Keys, String> {
|
||||
/// 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<Keys, String> {
|
||||
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)
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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";
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user