security(auth): harden registration/identity against verified review findings

Adversarial review of the auth/registration surface (findings tracked
privately). Backend half:

- F2: npub-native register no longer binds an npub onto an existing
  wallet row on a seed_fingerprint match alone (unauthenticated account
  takeover). get_or_create_user_by_nostr_pubkey fails closed (409);
  linking an npub goes through the authenticated /auth/nostr/link flow.
  Integration test rewritten to assert the refuse-and-409 contract.
- F3: `all`-mode gate verifies payment is settled (non-consuming) BEFORE
  consuming the single-use invite, so a still-confirming payment can't
  permanently burn the invite. verify_payment_settled split out.
- F4: username availability is pre-checked before any gate token is
  consumed; collisions return 409, not 500 — a taken username no longer
  burns a paid invoice / invite.
- F6: nostr_register now persists the submitted chain keys, so
  receive-by-username and restore self-verification work for npub wallets.
- F10: the pay-to-register not-yet-settled response carries a stable
  PAYMENT_PENDING code so polling clients don't parse a free-text string.

Validated against Postgres: full suite 283 passed / 0 failed.

src/api/auth.rs

@@ -483,6 +483,8 @@ pub async fn extension_register(
         returning_by_pubkey,
         req.altcha_solution.as_ref(),
     )?;
+    // Reject a taken username BEFORE consuming any single-use gate token.
+    ensure_username_available(&state, returning_by_pubkey, req.username.as_deref()).await?;
     // Invite + payment gates, composed per the operator's gate mode (`all` =
     // every enabled gate; `any` = one-of). PoW above is orthogonal to both.
     enforce_method_gates(
@@ -649,8 +651,16 @@ async fn enforce_method_gates(
         has_payment,
     ) {
         GatePlan::Open => Ok(()),
-        // Conjunction: run both (each self-disables when its own flag is off).
+        // Conjunction: both enabled gates must pass (each self-disables when its
+        // own flag is off). Verify the payment gate is redeemable (settled,
+        // NON-consuming) BEFORE consuming the single-use invite, so a payment that
+        // is still confirming — the common poll state — can never burn the invite.
+        // Residual: a concurrent double-spend of the invoice between this check and
+        // the consume below could still burn the invite; that window is far
+        // narrower than the "still confirming" case. Full atomicity would require
+        // consuming both tokens + the user INSERT in one shared DB transaction.
         GatePlan::All => {
+            verify_payment_settled(state, returning, pubkey_hash, payment_invoice_id).await?;
             enforce_invite(state, returning, invite_code).await?;
             enforce_payment(state, returning, pubkey_hash, payment_invoice_id).await?;
             Ok(())
@@ -782,8 +792,44 @@ async fn enforce_payment(
     pubkey_hash: &str,
     payment_invoice_id: Option<&str>,
 ) -> Result<(), AppError> {
-    if returning || !state.config.registration.payment.require_payment {
+    // Verify (non-consuming) then consume. Callers that must confirm the gate is
+    // redeemable BEFORE committing ANOTHER single-use token (the `All` gate plan
+    // pairs this with the invite) reuse `verify_payment_settled` first so a
+    // still-confirming payment can never cost the invite.
+    let Some(id) =
+        verify_payment_settled(state, returning, pubkey_hash, payment_invoice_id).await?
+    else {
         return Ok(());
+    };
+
+    // Atomic single-use consume: exactly one registration per invoice, even under
+    // a concurrent race (the loser gets the already-used literal).
+    if !state.db.consume_payment_invoice(&id, pubkey_hash).await? {
+        return Err(AppError::ValidationError(
+            "This payment invoice has already been used.".into(),
+        ));
+    }
+    info!(
+        gate = "payment",
+        "payment invoice settled + consumed (new user)"
+    );
+    Ok(())
+}
+
+/// Non-consuming half of the pay-to-register gate: confirm the invoice exists,
+/// is bound to THIS `pubkey_hash`, is unspent, and reads `Settled` from the
+/// processor. Returns `Ok(Some(id))` when ready to consume, `Ok(None)` when the
+/// gate does not apply (returning user / gate off). Splitting the check from the
+/// consume lets the `all`-mode plan verify payment is settled BEFORE it burns the
+/// single-use invite — a pending payment must never cost the invite.
+async fn verify_payment_settled(
+    state: &AppState,
+    returning: bool,
+    pubkey_hash: &str,
+    payment_invoice_id: Option<&str>,
+) -> Result<Option<String>, AppError> {
+    if returning || !state.config.registration.payment.require_payment {
+        return Ok(None);
     }
     // Gate on but no provider built — config validation prevents this, so it is
     // an internal misconfiguration (generic 500), not a client-facing error.
@@ -837,7 +883,10 @@ async fn enforce_payment(
     // binding anchor; a foreign or underpaid invoice never reaches this point).
     let invoice = provider.get_invoice(id).await?;
     if invoice.status != crate::infra::payment::InvoiceStatus::Settled {
-        return Err(AppError::ValidationError(
+        // Distinct machine-readable code (`PAYMENT_PENDING`) so a polling client
+        // can tell "keep waiting" from a terminal failure without matching on the
+        // human string.
+        return Err(AppError::PaymentPending(
             "Payment not yet confirmed. Complete the payment and retry.".into(),
         ));
     }
@@ -851,18 +900,33 @@ async fn enforce_payment(
             ));
         }
     }
+    Ok(Some(id.to_string()))
+}
 
-    // Atomic single-use consume: exactly one registration per invoice, even under
-    // a concurrent race (the loser gets the already-used literal).
-    if !state.db.consume_payment_invoice(id, pubkey_hash).await? {
-        return Err(AppError::ValidationError(
-            "This payment invoice has already been used.".into(),
-        ));
+/// Reject a NEW registration whose requested username is already taken BEFORE any
+/// single-use gate token is consumed, so a collision can't burn a paid invoice /
+/// invite with no account created. Returning users are exempt (get-or-create
+/// only backfills NULL optionals and never re-points an existing username).
+///
+/// Not fully atomic: a concurrent claim can still race the final `create_user`
+/// INSERT between this read and the write. That narrow race is the price of not
+/// threading a shared transaction through the gate consume + user INSERT; it is
+/// far rarer than a user simply picking an already-taken name, which this closes.
+async fn ensure_username_available(
+    state: &AppState,
+    returning: bool,
+    username: Option<&str>,
+) -> Result<(), AppError> {
+    if returning {
+        return Ok(());
+    }
+    if let Some(name) = username.map(str::trim).filter(|n| !n.is_empty()) {
+        if state.db.get_user_by_username(name).await?.is_some() {
+            return Err(AppError::Conflict(
+                "That username is already taken. Choose another.".into(),
+            ));
+        }
     }
-    info!(
-        gate = "payment",
-        "payment invoice settled + consumed (new user)"
-    );
     Ok(())
 }
 
@@ -1646,6 +1710,8 @@ pub async fn nostr_register(
     //    invoices bind to sha256(btc pubkey), so the payment check still uses the
     //    BTC key carried in `keys` even though it is no longer the identity.
     enforce_pow(&state, &pubkey, returning, req.altcha_solution.as_ref())?;
+    // Reject a taken username BEFORE consuming any single-use gate token.
+    ensure_username_available(&state, returning, req.username.as_deref()).await?;
     let btc_hash = req
         .keys
         .iter()
@@ -1676,6 +1742,12 @@ pub async fn nostr_register(
         )
         .await?;
 
+    // 5b. Persist the submitted chain keys (tip addresses + restore anchors).
+    //     The BTC path does this at registration; the npub path previously
+    //     skipped it, leaving user_keys empty so receive-by-username lookups and
+    //     restore self-verification broke for npub-native wallets. Idempotent.
+    upsert_all_keys(&state, user.id, &req.keys).await?;
+
     // 6. Session.
     let pair = issue_session(
         &state,

src/error.rs

@@ -32,6 +32,11 @@ pub enum AppError {
     Forbidden(String),
     /// Conflict: a unique resource (e.g. a username) is already taken.
     Conflict(String),
+    /// Pay-to-register invoice exists and is bound but has not settled yet. A
+    /// SAFE variant with its OWN machine-readable code (`PAYMENT_PENDING`) so a
+    /// polling client can distinguish "keep waiting" from a terminal failure
+    /// without matching on the human-readable string.
+    PaymentPending(String),
     /// Rate limit exceeded.
     RateLimited,
     /// Internal server error.
@@ -49,6 +54,7 @@ impl std::fmt::Display for AppError {
             Self::NotFound(msg) => write!(f, "Not found: {}", msg),
             Self::Forbidden(msg) => write!(f, "Forbidden: {}", msg),
             Self::Conflict(msg) => write!(f, "Conflict: {}", msg),
+            Self::PaymentPending(msg) => write!(f, "Payment pending: {}", msg),
             Self::RateLimited => write!(f, "Rate limited"),
             Self::Internal(msg) => write!(f, "Internal error: {}", msg),
         }
@@ -69,6 +75,7 @@ impl AppError {
             AppError::NotFound(_) => "NOT_FOUND",
             AppError::Forbidden(_) => "FORBIDDEN",
             AppError::Conflict(_) => "CONFLICT",
+            AppError::PaymentPending(_) => "PAYMENT_PENDING",
             AppError::RateLimited => "RATE_LIMITED",
             AppError::Internal(_) => "INTERNAL_ERROR",
         }
@@ -126,6 +133,9 @@ impl IntoResponse for AppError {
             AppError::NotFound(msg) => (StatusCode::NOT_FOUND, msg.clone()),
             AppError::Forbidden(msg) => (StatusCode::FORBIDDEN, msg.clone()),
             AppError::Conflict(msg) => (StatusCode::CONFLICT, msg.clone()),
+            // Keep 400 (not 402) so a legacy client still polling on the status +
+            // string keeps working during rollout; new clients match `code`.
+            AppError::PaymentPending(msg) => (StatusCode::BAD_REQUEST, msg.clone()),
             AppError::RateLimited => (StatusCode::TOO_MANY_REQUESTS, "Rate limited".to_string()),
         };
 
@@ -254,4 +264,17 @@ mod tests {
         assert_eq!(json["error"].as_str().unwrap(), msg);
         assert_eq!(json["code"].as_str().unwrap(), "CONFLICT");
     }
+
+    #[tokio::test]
+    async fn payment_pending_has_stable_code_and_400() {
+        // A still-confirming pay-to-register invoice: 400 (so legacy string-poll
+        // clients keep working) but with the stable PAYMENT_PENDING code new
+        // clients match on to distinguish "keep waiting" from a terminal error.
+        let msg = "Payment not yet confirmed. Complete the payment and retry.";
+        let err = AppError::PaymentPending(msg.into());
+        let (status, json) = extract_json(err.into_response()).await;
+        assert_eq!(status, StatusCode::BAD_REQUEST);
+        assert_eq!(json["error"].as_str().unwrap(), msg);
+        assert_eq!(json["code"].as_str().unwrap(), "PAYMENT_PENDING");
+    }
 }

src/infra/db/users.rs

@@ -357,27 +357,25 @@ impl Database {
                         ));
                     }
                     None => {
-                        let sql = format!(
-                            "UPDATE users SET \
-                               nostr_pubkey     = $2, \
-                               wallet_birthday  = COALESCE(wallet_birthday, $3), \
-                               xmr_start_height = COALESCE(xmr_start_height, $4), \
-                               wow_start_height = COALESCE(wow_start_height, $5), \
-                               updated_at = NOW() \
-                             WHERE id = $1 RETURNING {USER_COLS}"
-                        );
-                        let updated = sqlx::query_as::<_, User>(&sql)
-                            .bind(existing.id)
-                            .bind(nostr_pubkey)
-                            .bind(wallet_birthday)
-                            .bind(xmr_start_height)
-                            .bind(wow_start_height)
-                            .fetch_one(self.pool())
-                            .await
-                            .map_err(unique_violation_as(
-                                "This Nostr identity is already linked to another account",
-                            ))?;
-                        return Ok(updated);
+                        // SECURITY: the seed_fingerprint is a lookup handle, not
+                        // a credential — the client transmits it unauthenticated on
+                        // /auth/check-restore and register, and it is stored server-
+                        // side. Binding an npub onto a pre-existing (BTC-anchored /
+                        // link-less) row on a fingerprint MATCH ALONE would let anyone
+                        // who learns a victim's fingerprint take over that account and
+                        // be issued a session for it. The BTC rotation path guards the
+                        // identical "known fingerprint at a new key" operation behind a
+                        // signature over the ON-FILE key; this register endpoint proves
+                        // control only of the NEW npub, never the on-file key, so it
+                        // MUST NOT re-point the row. Fail closed exactly like the
+                        // different-npub arm above: the wallet links its npub through
+                        // the authenticated POST /auth/nostr/link flow (which requires
+                        // an existing session proving the on-file key) instead.
+                        return Err(AppError::Conflict(
+                            "This wallet already has an account. Sign in with your existing \
+                             credentials and link your Nostr identity from settings."
+                                .into(),
+                        ));
                     }
                 }
             }

tests/nostr_register.rs

@@ -1,7 +1,9 @@
 //! L1 integration: npub-native registration (`POST /auth/nostr/register`) — the
-//! create-by-npub path (no BTC signature) and the `seed_fingerprint` MERGE dedup
-//! that must never split one wallet into two identities. Skips without
-//! `TEST_DATABASE_URL` (see `common`).
+//! create-by-npub path (no BTC signature) and the `seed_fingerprint` handling.
+//! A fingerprint match onto a pre-existing row is REFUSED: the endpoint
+//! proves only npub control, so it must not bind an npub onto an account keyed by
+//! another (on-file) credential without proof. Skips without `TEST_DATABASE_URL`
+//! (see `common`).
 
 mod common;
 
@@ -147,7 +149,14 @@ async fn register_replays_are_rejected() {
 }
 
 #[tokio::test]
-async fn register_merges_onto_existing_seed_fingerprint_row() {
+async fn register_refuses_to_bind_npub_onto_existing_seed_fingerprint_row_without_proof() {
+    // SECURITY: the npub-native register endpoint proves control of the NEW
+    // npub only, never of the key ALREADY ON FILE. seed_fingerprint is a lookup
+    // handle the client sends unauthenticated (check-restore / register), not a
+    // credential — so binding an npub onto a pre-existing BTC-anchored row on a
+    // fingerprint MATCH alone would let anyone who learns a victim's fingerprint
+    // take over that account. The endpoint must fail closed (409); the wallet
+    // links its npub through the authenticated POST /auth/nostr/link flow instead.
     let app = require_app!();
     let base = app
         .state
@@ -174,21 +183,31 @@ async fn register_merges_onto_existing_seed_fingerprint_row() {
         .unwrap();
     assert!(existing.nostr_pubkey.is_none());
 
-    // Register npub-native with the SAME seed -> MERGE onto that row, not a new one.
+    // Register npub-native with the SAME seed -> REJECTED (no unauthenticated merge).
     let sk = random_signer();
     let pk = hex::encode(sk.verifying_key().to_bytes());
     let (status, body) = register(&app, &sk, &base, Some(&fp)).await;
-    assert_eq!(status, StatusCode::OK, "body={body}");
+    assert_eq!(status, StatusCode::CONFLICT, "body={body}");
 
-    let merged = app
+    // The existing row is left UNTOUCHED and the npub binds to nothing.
+    assert!(
+        app.state
+            .db
+            .find_user_by_nostr_pubkey(&pk)
+            .await
+            .unwrap()
+            .is_none(),
+        "a rejected register must not bind the npub to any account"
+    );
+    let after = app
         .state
         .db
-        .find_user_by_nostr_pubkey(&pk)
+        .get_user(existing.id)
         .await
         .unwrap()
-        .expect("npub resolves after register");
-    assert_eq!(
-        merged.id, existing.id,
-        "npub register must MERGE onto the existing seed_fingerprint row, not split identity"
+        .expect("existing row still present");
+    assert!(
+        after.nostr_pubkey.is_none(),
+        "the victim's row must not have gained an npub"
     );
 }