fix(nip46): spec-correct connect params, full-handshake [NIP46] trace, visible handshake errors

tests/nip46_e2e.rs

@@ -335,12 +335,18 @@ async fn run_fake_amber(relay_url: String, comms: Keys, identity: Keys, approval
 // ---------------------------------------------------------------------------
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+/// Serializes the e2e tests against each other (see the whole-body `_vault_guard`
+/// below). `await_holding_lock` is allowed here deliberately: the guard is a
+/// test-only serialization lock, never nested, never shared with production
+/// code — holding it across awaits is the entire point.
+#[allow(clippy::await_holding_lock)]
 async fn nip46_client_handshake_and_sign_against_fake_amber() {
     // Isolated vault so the test never touches the real user vault.
-    // XDG_DATA_HOME is process-global and both tests in this binary set it,
-    // so vault setup + App::load are serialized.
+    // XDG_DATA_HOME is process-global and every test in this binary sets it,
+    // so the lock is held for the WHOLE test: data_dir() re-reads the env on
+    // every save, and a setup-only guard lets parallel tests cross-write.
+    let _vault_guard = VAULT_ENV_LOCK.lock().unwrap();
     let app = {
-        let _guard = VAULT_ENV_LOCK.lock().unwrap();
         let tmp = std::env::temp_dir().join(format!("keynectr-e2e-{}", std::process::id()));
         // data_dir() is $XDG_DATA_HOME/keynectr — the isolation vault must
         // live there. Writing it one level too shallow left the app finding
@@ -478,6 +484,230 @@ async fn nip46_client_handshake_and_sign_against_fake_amber() {
     let _ = PublicKey::from_hex; // keep import used across cfg variations
 }
 
+// ---------------------------------------------------------------------------
+// Strict Amber for the bunker:// (paste-URI) flow: validates the `connect`
+// request against NIP-46 instead of acking anything. params[0] MUST be the
+// remote signer's pubkey (the URI authority) — the client's own pubkey there
+// is a spec violation that real signers answer with silence, stalling the
+// handshake with zero feedback. This test FAILS on the old param order and
+// passes on the fixed one.
+// ---------------------------------------------------------------------------
+
+/// Run Amber in strict mode: enforce the NIP-46 `connect` shape, then behave
+/// like the lenient fake (delayed approval ack, real identity, remote sign).
+async fn run_strict_amber(
+    relay_url: String,
+    comms: Keys,
+    identity: Keys,
+    approval_delay: Duration,
+) {
+    let (mut ws, _) = tokio_tungstenite::connect_async(&relay_url)
+        .await
+        .expect("strict amber connect");
+    ws.send(Message::Text(
+        json!(["REQ", "strict-amber", {"kinds": [24133]}])
+            .to_string()
+            .into(),
+    ))
+    .await
+    .unwrap();
+
+    let comms_pub = comms.public_key();
+    let comms_hex = comms_pub.to_hex();
+
+    while let Some(Ok(msg)) = ws.next().await {
+        let Message::Text(text) = msg else { continue };
+        let Ok(arr) = serde_json::from_str::<Vec<Value>>(&text) else {
+            continue;
+        };
+        if arr.first().and_then(|v| v.as_str()) != Some("EVENT") {
+            continue;
+        }
+        let Some(ev) = arr
+            .get(2)
+            .and_then(|v| v.as_object())
+            .and_then(|o| Event::from_json(serde_json::to_string(o).ok()?.as_bytes()).ok())
+        else {
+            continue;
+        };
+        if ev.pubkey == comms_pub {
+            continue;
+        }
+        let Ok(conversation) = ConversationKey::derive(comms.secret_key(), &ev.pubkey) else {
+            continue;
+        };
+        let Some(plain) = nip44_dec(&conversation, &ev.content) else {
+            continue;
+        };
+        let Ok(req) = serde_json::from_str::<Value>(&plain) else {
+            continue;
+        };
+        let Some(method) = req.get("method").and_then(|m| m.as_str()) else {
+            continue;
+        };
+        let id = req
+            .get("id")
+            .and_then(|v| v.as_str())
+            .unwrap_or("")
+            .to_string();
+
+        let response: Value = match method {
+            "connect" => {
+                let first_param = req
+                    .get("params")
+                    .and_then(|p| p.get(0))
+                    .and_then(|v| v.as_str())
+                    .unwrap_or("");
+                if first_param != comms_hex {
+                    json!({"id": id, "error": "connect params must start with the remote signer pubkey"})
+                } else {
+                    tokio::time::sleep(approval_delay).await;
+                    json!({"id": id, "result": "ack"})
+                }
+            }
+            "get_public_key" => json!({"id": id, "result": identity.public_key().to_hex()}),
+            "sign_event" => {
+                let unsigned_json = req["params"].get(0).and_then(|v| v.as_str());
+                match unsigned_json.and_then(|s| serde_json::from_str::<Value>(s).ok()) {
+                    Some(mut v) => {
+                        if v.get("pubkey").is_none() {
+                            v["pubkey"] = json!(identity.public_key().to_hex());
+                        }
+                        match serde_json::from_value::<UnsignedEvent>(v)
+                            .ok()
+                            .and_then(|u| identity.sign_event(u).ok())
+                        {
+                            Some(signed) => {
+                                json!({"id": id, "result": signed.as_json()})
+                            }
+                            None => json!({"id": id, "error": "sign failed"}),
+                        }
+                    }
+                    None => json!({"id": id, "error": "bad params"}),
+                }
+            }
+            other => json!({"id": id, "error": format!("unsupported: {other}")}),
+        };
+
+        let content = nip44_enc(&conversation, &response.to_string());
+        let out = EventBuilder::new(Kind::NostrConnect, content)
+            .tags([Tag::parse(["p", ev.pubkey.to_hex().as_str()]).unwrap()])
+            .finalize(&comms)
+            .unwrap();
+        ws.send(Message::Text(
+            json!([
+                "EVENT",
+                serde_json::from_str::<Value>(&out.as_json()).unwrap()
+            ])
+            .to_string()
+            .into(),
+        ))
+        .await
+        .unwrap();
+    }
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+/// Serializes the e2e tests against each other (see the whole-body `_vault_guard`
+/// below). `await_holding_lock` is allowed here deliberately: the guard is a
+/// test-only serialization lock, never nested, never shared with production
+/// code — holding it across awaits is the entire point.
+#[allow(clippy::await_holding_lock)]
+async fn nip46_bunker_connect_params_match_spec_against_strict_amber() {
+    // Whole-body env lock: data_dir() re-reads XDG_DATA_HOME on every save.
+    let _vault_guard = VAULT_ENV_LOCK.lock().unwrap();
+    let app = {
+        let tmp = std::env::temp_dir().join(format!("keynectr-e2e-strict-{}", std::process::id()));
+        let app_dir = tmp.join("keynectr");
+        std::fs::create_dir_all(&app_dir).unwrap();
+        std::fs::write(
+            app_dir.join("profiles_vault.json"),
+            serde_json::to_string(&Vault::empty()).unwrap(),
+        )
+        .unwrap();
+        std::env::set_var("XDG_DATA_HOME", &tmp);
+        let app = std::sync::Arc::new(Mutex::new(App::load().expect("load app")));
+        assert!(
+            app.try_lock().unwrap().vault.profiles.is_empty(),
+            "e2e vault isolation failed: a non-empty vault was loaded"
+        );
+        app
+    };
+
+    // `comms` is the per-connection key in the bunker:// URI; `identity` is
+    // the REAL signing identity, never in the URI.
+    let comms = Keys::generate();
+    let identity = Keys::generate();
+
+    let relay_url = start_relay().await;
+    tokio::spawn(run_strict_amber(
+        relay_url.clone(),
+        comms.clone(),
+        identity.clone(),
+        Duration::from_millis(200),
+    ));
+
+    let signer = Nip46ClientSigner::new(app.clone());
+
+    // No secret in the URI: the strict signer must still ack a well-formed
+    // connect whose params[0] is its own pubkey.
+    let uri = format!(
+        "bunker://{}?relay={}",
+        comms.public_key().to_hex(),
+        relay_url
+    );
+    let status = signer
+        .connect(&uri, "strict amber".to_string())
+        .await
+        .expect("connect");
+    assert!(!status.connected, "must not be connected before handshake");
+
+    let deadline = tokio::time::Instant::now() + Duration::from_secs(20);
+    loop {
+        let status = signer.status().await;
+        if let Some(err) = &status.error {
+            panic!("strict handshake failed: {err}");
+        }
+        if status.connected {
+            break;
+        }
+        assert!(
+            tokio::time::Instant::now() < deadline,
+            "strict handshake never completed; last status: {:?}",
+            signer.status().await
+        );
+        tokio::time::sleep(Duration::from_millis(100)).await;
+    }
+
+    // Identity must be the REAL key from get_public_key — the actual user
+    // pubkey the account manager stores — never the URI comms key and never
+    // the client's own ephemeral key.
+    let resolved = SignerTrait::get_public_key(&signer)
+        .await
+        .expect("identity resolved");
+    assert_eq!(resolved, identity.public_key());
+    assert_ne!(resolved, comms.public_key());
+
+    let identity_npub = identity.public_key().to_bech32().unwrap();
+    let app_guard = app.lock().await;
+    assert!(
+        app_guard
+            .vault
+            .profiles
+            .iter()
+            .any(|p| p.public_key == identity_npub && p.secret_key.trim().is_empty()),
+        "remote profile row for the real identity must be stored with no secret"
+    );
+    assert_eq!(
+        app_guard.vault.active_profile.as_deref(),
+        Some(identity_npub.as_str()),
+        "the connected account must become the active profile"
+    );
+    drop(app_guard);
+
+    signer.disconnect().await.ok();
+}
+
 // ---------------------------------------------------------------------------
 // QR pairing (client-initiated nostrconnect://): the fake signer plays the
 // scanner role — it reads the pairing token the GUI would render, sends the
@@ -625,6 +855,11 @@ async fn run_fake_scanner(
 }
 
 #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+/// Serializes the e2e tests against each other (see the whole-body `_vault_guard`
+/// below). `await_holding_lock` is allowed here deliberately: the guard is a
+/// test-only serialization lock, never nested, never shared with production
+/// code — holding it across awaits is the entire point.
+#[allow(clippy::await_holding_lock)]
 async fn nip46_qr_pairing_handshake_and_sign() {
     run_qr_pairing("request").await;
 }
@@ -633,16 +868,24 @@ async fn nip46_qr_pairing_handshake_and_sign() {
 /// what Amber sends — is a connect *response* (`{"id","result":"<secret>"}`),
 /// not a `connect` request. Pairing must complete on that shape too.
 #[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+/// Serializes the e2e tests against each other (see the whole-body `_vault_guard`
+/// below). `await_holding_lock` is allowed here deliberately: the guard is a
+/// test-only serialization lock, never nested, never shared with production
+/// code — holding it across awaits is the entire point.
+#[allow(clippy::await_holding_lock)]
 async fn nip46_qr_pairing_connect_response_shape() {
     run_qr_pairing("response").await;
 }
 
+/// Whole-body env lock like the test fns above (test-only, never nested).
+#[allow(clippy::await_holding_lock)]
 async fn run_qr_pairing(connect_shape: &'static str) {
     let relay_url = start_relay().await;
 
-    // Isolated vault + pairing relay, serialized with the other e2e test.
+    // Isolated vault + pairing relay; the env lock is held for the whole
+    // helper body (see above) so parallel tests cannot cross-write vaults.
+    let _vault_guard = VAULT_ENV_LOCK.lock().unwrap();
     let app = {
-        let _guard = VAULT_ENV_LOCK.lock().unwrap();
         let tmp = std::env::temp_dir().join(format!(
             "keynectr-e2e-pair-{}-{}",
             std::process::id(),