From c802f416cd9c405aa9aa13690e688d98b29ffafb Mon Sep 17 00:00:00 2001 From: jwinterm Date: Mon, 6 Jul 2026 04:23:11 -0400 Subject: [PATCH] security(auth): harden registration/identity against verified review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 98 +++++++++++++++++++++++++++++++++++------ src/error.rs | 23 ++++++++++ src/infra/db/users.rs | 40 ++++++++--------- tests/nostr_register.rs | 43 +++++++++++++----- 4 files changed, 158 insertions(+), 46 deletions(-) diff --git a/src/api/auth.rs b/src/api/auth.rs index 7312faa..b8d0dfc 100644 --- a/src/api/auth.rs +++ b/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, 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, diff --git a/src/error.rs b/src/error.rs index 8bf39fa..e9c7c29 100644 --- a/src/error.rs +++ b/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"); + } } diff --git a/src/infra/db/users.rs b/src/infra/db/users.rs index ca9e112..730048f 100644 --- a/src/infra/db/users.rs +++ b/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(), + )); } } } diff --git a/tests/nostr_register.rs b/tests/nostr_register.rs index 31cd21f..ba77cf8 100644 --- a/tests/nostr_register.rs +++ b/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" ); }