diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml index 818255f1..42b46aa4 100644 --- a/.github/workflows/mutation.yml +++ b/.github/workflows/mutation.yml @@ -60,15 +60,27 @@ jobs: exit 0 fi - # Build --file flags for each changed file - file_args="" - for f in $changed_rs; do - file_args="$file_args --file $f" - done - echo "Running mutation testing for changed files:" echo "$changed_rs" - cargo mutants $file_args + + # These filenames come from a PR diff, so they are + # attacker-controlled. Built as a bash array (never joined into a + # string) and passed with "${file_args[@]}" so a crafted filename + # can only ever be one --file value, never extra argv tokens — the + # string-then-make(ARGS)-then-shell round trip this used to take + # was an argument-injection vector into cargo-mutants/cargo/rustc's + # own flag surface. This deliberately bypasses `make + # mutation-test`, whose $(ARGS) is a plain string splice safe only + # for hand-typed input, so it repeats that target's environment + # here (see the Makefile for why both variables are set). + file_args=() + while IFS= read -r f; do + file_args+=(--file "$f") + done <<< "$changed_rs" + + # Keep these three in sync with the mutation-test target in Makefile. + CARGO_MUTANTS_JOBS=1 RUST_TEST_THREADS=1 MOSTRO_TEST_LN_PORT=18080 \ + cargo mutants "${file_args[@]}" - name: Upload mutation report uses: actions/upload-artifact@v4 @@ -195,7 +207,7 @@ jobs: tool: cargo-mutants - name: Run full mutation testing - run: cargo mutants + run: make mutation-test # Note: Do NOT fail on low score initially (report only mode) continue-on-error: true diff --git a/Makefile b/Makefile index 348bfa2f..e21a6b15 100644 --- a/Makefile +++ b/Makefile @@ -66,3 +66,39 @@ docker-build-startos: cd docker && \ docker compose build mostro-startos +# Two levels of parallelism have to be off here, not one. The LNURL tests +# bind a fixed host port, so anything running them concurrently collides on +# the listener — and a test that fails because it lost that race is scored +# as a killed mutant, silently inflating the number this target measures. +# +# CARGO_MUTANTS_JOBS=1 serialises the mutant workers. cargo-mutants already +# does this by default (verified against 27.1.0: one scratch dir on a +# 16-CPU host with no -j); it is set explicitly so the guarantee is stated +# rather than inherited. +# +# RUST_TEST_THREADS=1 serialises the test threads inside each run. That +# level was never actually covered: `.mutants.toml` carries +# `test_tool_options = ["--", "--test-threads=1"]` for exactly this reason, +# but cargo-mutants reads `.cargo/mutants.toml`, so the file has never been +# loaded (#958). The env var needs no config file to work. +# +# MOSTRO_TEST_LN_PORT moves those tests off 8080 in case something on the +# host already holds it. It does NOT make the suite hermetic: two concurrent +# runs would still collide on 18080. Serialising is what makes the run +# trustworthy; the port override only dodges a pre-existing listener. +# +# The 18080 here deliberately differs from the code's own 8080 default, so +# `cargo test` and `make mutation-test` do bind different ports. That is the +# point: plain `cargo test` keeps exercising the default path, and only this +# target — which runs the suite hundreds of times over — steps aside from a +# port a developer machine is likely to have in use. +# +# ARGS is spliced into the shell command as plain text — only pass +# hand-typed, trusted values (e.g. `make mutation-test ARGS="--file +# src/foo.rs"`). Never build ARGS from PR-diff filenames or other +# attacker-controlled input; that class of data must be turned into a bash +# array and passed to `cargo mutants` directly (see the PR job in +# .github/workflows/mutation.yml). +mutation-test: + CARGO_MUTANTS_JOBS=1 RUST_TEST_THREADS=1 \ + MOSTRO_TEST_LN_PORT=$${MOSTRO_TEST_LN_PORT:-18080} cargo mutants $(ARGS) diff --git a/src/app.rs b/src/app.rs index d5433cd2..d89daff0 100644 --- a/src/app.rs +++ b/src/app.rs @@ -296,6 +296,24 @@ async fn handle_message_action( } } +/// True when a rumor's `created_at` falls outside the 10s replay window. +/// Pulled out of `accept_event` so the boundary can be hit directly instead +/// of only through a live wrapped event. +fn is_stale(created_at: Timestamp, since_time: u64) -> bool { + created_at.as_secs() < since_time +} + +/// True when the inner rumor is missing a required signature. Full-privacy +/// clients reuse the trade key as identity and send unsigned rumors; any +/// other identity/sender split must carry a valid inner signature. +fn missing_inner_signature( + identity: PublicKey, + sender: PublicKey, + signature: Option, +) -> bool { + identity != sender && signature.is_none() +} + /// Decode and fully validate one relay event into a dispatchable /// `(action, message, unwrapped)` triple, or `None` if it must be skipped /// (failed PoW, wrong kind, invalid event signature, spam-gate drop, decrypt @@ -406,7 +424,7 @@ async fn accept_event( .checked_sub_signed(chrono::Duration::seconds(10)) .unwrap() .timestamp() as u64; - if unwrapped.created_at.as_secs() < since_time { + if is_stale(unwrapped.created_at, since_time) { return None; } let message = unwrapped.message.clone(); @@ -415,7 +433,7 @@ async fn accept_event( // unsigned rumors. Any other shape must carry a valid inner // signature — unwrap_message already verified it, so if identity // and sender differ here without a signature we bail out. - if unwrapped.identity != unwrapped.sender && unwrapped.signature.is_none() { + if missing_inner_signature(unwrapped.identity, unwrapped.sender, unwrapped.signature) { tracing::warn!( "Missing inner signature: identity {} differs from trade key {}", unwrapped.identity, @@ -702,6 +720,46 @@ mod tests { } } + #[test] + fn is_stale_rejects_events_older_than_the_cutoff() { + assert!(is_stale(Timestamp::from(99), 100)); + } + + #[test] + fn is_stale_accepts_events_exactly_at_the_cutoff() { + assert!(!is_stale(Timestamp::from(100), 100)); + } + + #[test] + fn is_stale_accepts_events_newer_than_the_cutoff() { + assert!(!is_stale(Timestamp::from(101), 100)); + } + + #[test] + fn missing_inner_signature_true_when_identity_and_sender_differ_unsigned() { + let identity = create_test_keys().public_key(); + let sender = create_test_keys().public_key(); + assert!(missing_inner_signature(identity, sender, None)); + } + + #[test] + fn missing_inner_signature_false_when_identity_equals_sender_unsigned() { + let same = create_test_keys().public_key(); + assert!(!missing_inner_signature(same, same, None)); + } + + #[test] + fn missing_inner_signature_false_when_signed_even_if_identity_differs() { + let identity = create_test_keys().public_key(); + let sender_keys = create_test_keys(); + let sig = sender_keys.sign_schnorr([7u8; 32]); + assert!(!missing_inner_signature( + identity, + sender_keys.public_key(), + Some(sig) + )); + } + #[test] fn test_warning_msg_all_error_types() { let action = Action::NewOrder; @@ -778,7 +836,7 @@ mod tests { use super::*; use crate::spam_gate::{SpamGate, REPLAY_WINDOW_SECS}; use mostro_core::nip59::{wrap_message, WrapOptions}; - use mostro_core::transport::wrap_message_nip44; + use mostro_core::transport::{wrap_message_nip44, wrap_message_with, Transport}; /// A protocol-v2 (kind 14) event addressed to `mostro`, in full-privacy /// mode (trade key doubles as identity, so no identity proof is needed). @@ -928,6 +986,48 @@ mod tests { /// The v2-only policy lives in one place now; both event loops read it /// from here, so this is where it gets covered. + /// The first-contact lane: an unseen sender that clears + /// `pow_first_contact` is let through to the decrypt. + #[tokio::test] + async fn unknown_first_contact_sender_clearing_the_pow_bar_is_accepted() { + let ctx = create_migrated_ctx().await; + let gate = SpamGate::new(REPLAY_WINDOW_SECS); + let mostro = create_test_keys(); + + // `accept` pins pow_first_contact = 0, trivially cleared, and a + // freshly generated trade key is never known to the gate. + assert!( + accept(&ctx, &v2_event(&mostro), &mostro, &gate) + .await + .is_some(), + "an unknown sender over the PoW bar must reach the decrypt" + ); + } + + /// The same lane refusing: only `pow_first_contact` differs, so the + /// toll is what decides, not the event or the gate. + #[tokio::test] + async fn unknown_first_contact_sender_below_the_pow_bar_is_dropped() { + let ctx = create_migrated_ctx().await; + let gate = SpamGate::new(REPLAY_WINDOW_SECS); + let mostro = create_test_keys(); + + // u8::MAX demands more leading-zero bits than any real event id + // will ever have, so this fails deterministically. + let result = accept_event( + &ctx, + &v2_event(&mostro), + &mostro, + 0, + u8::MAX, + NostrKind::from(crate::config::constants::DM_EVENT_KIND), + Some(&gate), + ) + .await; + + assert!(result.is_none()); + } + #[test] fn gate_applies_to_v2_only() { assert!(gate_for(false).is_none(), "v1 must fail open"); @@ -935,6 +1035,96 @@ mod tests { // the v2 arm can only be pinned against the installed state. assert_eq!(gate_for(true).is_some(), SpamGate::global().is_some()); } + + #[allow(deprecated)] // Transport::GiftWrap: still the tested default; see #786. + async fn wrap_test_order( + identity_keys: &Keys, + trade_keys: &Keys, + receiver: PublicKey, + ) -> Event { + // RestoreSession is the one action whose payload is required to be + // None (mostro-core's `verify()`), which keeps this helper free of a + // hand-built `Payload::Order`. The trade-index path it short-circuits + // is covered directly by `check_trade_index_tests`. + wrap_message_with( + Transport::GiftWrap, + &create_test_message(Action::RestoreSession, None), + identity_keys, + trade_keys, + receiver, + WrapOptions::default(), + ) + .await + .expect("gift wrap succeeds") + } + + #[tokio::test] + async fn accepts_a_validly_wrapped_event_and_returns_the_action() { + let ctx = create_migrated_ctx().await; + let identity_keys = create_test_keys(); + let trade_keys = create_test_keys(); + let receiver_keys = create_test_keys(); + let event = + wrap_test_order(&identity_keys, &trade_keys, receiver_keys.public_key()).await; + + let result = accept_event( + &ctx, + &event, + &receiver_keys, + 0, + 0, + NostrKind::GiftWrap, + None, + ) + .await; + + let (action, _message, unwrapped) = result.expect("valid event should be accepted"); + assert_eq!(action, Action::RestoreSession); + assert_eq!(unwrapped.sender, trade_keys.public_key()); + assert_eq!(unwrapped.identity, identity_keys.public_key()); + } + + #[tokio::test] + async fn rejects_an_event_of_the_wrong_kind() { + let ctx = create_migrated_ctx().await; + let identity_keys = create_test_keys(); + let trade_keys = create_test_keys(); + let receiver_keys = create_test_keys(); + let event = + wrap_test_order(&identity_keys, &trade_keys, receiver_keys.public_key()).await; + + // The event is a real, validly-signed GiftWrap; accepted_kind is + // deliberately mismatched to exercise the kind-gate on its own. + let result = accept_event( + &ctx, + &event, + &receiver_keys, + 0, + 0, + NostrKind::TextNote, + None, + ) + .await; + + assert!(result.is_none()); + } + + #[tokio::test] + async fn rejects_an_event_not_addressed_to_the_receiver() { + let ctx = create_migrated_ctx().await; + let identity_keys = create_test_keys(); + let trade_keys = create_test_keys(); + let intended_receiver = create_test_keys(); + let eavesdropper = create_test_keys(); + let event = + wrap_test_order(&identity_keys, &trade_keys, intended_receiver.public_key()).await; + + // unwrap_incoming can't decrypt a rumor sealed for someone else. + let result = + accept_event(&ctx, &event, &eavesdropper, 0, 0, NostrKind::GiftWrap, None).await; + + assert!(result.is_none()); + } } /// Maintenance (drain) mode gate in [`accept_event`]: the three actions diff --git a/src/app/restore_session.rs b/src/app/restore_session.rs index 773ec80e..cf1f35d3 100644 --- a/src/app/restore_session.rs +++ b/src/app/restore_session.rs @@ -3,6 +3,14 @@ use crate::{db::RestoreSessionManager, util::enqueue_restore_session_msg}; use mostro_core::prelude::*; use nostr_sdk::prelude::*; +/// How long a restore-session request waits for results before the requester +/// is told to retry instead of hanging forever. +/// +/// Named so the timeout and the message reporting it cannot drift apart: they +/// were two independent literals, and the log said "1 hour" whatever the +/// duration actually was. +const RESTORE_SESSION_TIMEOUT_SECS: u64 = 60 * 60; + /// Handle restore session action /// This function starts a background task to process the restore session /// and immediately returns, avoiding blocking the main application @@ -41,7 +49,7 @@ pub async fn restore_session_action( /// Handle restore session results in the background async fn handle_restore_session_results(mut manager: RestoreSessionManager, trade_key: String) { // Wait for the result with a timeout - let timeout = tokio::time::Duration::from_secs(60 * 60); // 1 hour timeout + let timeout = tokio::time::Duration::from_secs(RESTORE_SESSION_TIMEOUT_SECS); match tokio::time::timeout(timeout, manager.wait_for_result()).await { Ok(Some(result)) => { @@ -60,7 +68,10 @@ async fn handle_restore_session_results(mut manager: RestoreSessionManager, trad tracing::error!("Restore session result channel closed unexpectedly"); } Err(_) => { - tracing::error!("Restore session timed out after 1 hour"); + // The `Duration` itself, not a hand-converted unit: it is the value + // actually passed to `tokio::time::timeout` above, so the message + // cannot disagree with the timeout for any value of the constant. + tracing::error!("Restore session timed out after {timeout:?}"); // Send timeout message to user if let Err(e) = send_restore_session_timeout(&trade_key).await { tracing::error!("Failed to send timeout message: {}", e); diff --git a/src/lightning/invoice.rs b/src/lightning/invoice.rs index 8300ba0d..18457e16 100644 --- a/src/lightning/invoice.rs +++ b/src/lightning/invoice.rs @@ -323,7 +323,10 @@ mod tests { ) .layer(tower_http::cors::CorsLayer::permissive()); - let listener = TcpListener::bind("127.0.0.1:8080").await.unwrap(); + // Same source of truth as the code that builds the URL. + let listener = TcpListener::bind(("127.0.0.1", crate::lnurl::test_ln_port())) + .await + .unwrap(); let addr = listener.local_addr().unwrap(); let port = addr.port(); diff --git a/src/lnurl.rs b/src/lnurl.rs index 15052e10..48a74883 100644 --- a/src/lnurl.rs +++ b/src/lnurl.rs @@ -239,6 +239,29 @@ async fn lnurl_get(url: Url) -> Result { .map_err(|_| MostroInternalErr(ServiceError::NoAPIResponse)) } +/// Host port the local LNURL test server listens on, and that `cfg!(test)` +/// lightning addresses are resolved against. +/// +/// Overridable via `MOSTRO_TEST_LN_PORT` so a run can dodge a port already +/// taken on the host (`make mutation-test` sets 18080) without disturbing +/// the 8080 default. One definition on purpose: the prod path, its own +/// test, and the test server in `lightning::invoice` must agree, and three +/// hand-copied parses would eventually not. +/// +/// Not gated on `#[cfg(test)]`: the caller sits behind `cfg!(test)`, which +/// is a runtime boolean, so both branches are compiled in every profile. +/// +/// `0` is rejected along with unset and unparseable values: it parses as a +/// valid `u16` but means "let the OS pick" to a listener, which would bind +/// an arbitrary port while the URL built here still said `:0`. +pub(crate) fn test_ln_port() -> u16 { + std::env::var("MOSTRO_TEST_LN_PORT") + .ok() + .and_then(|v| v.parse::().ok()) + .filter(|p| *p != 0) + .unwrap_or(8080) +} + /// Parse a Lightning Address or bech32 LNURL into an http(s) [`Url`]. /// /// - Lightning Address (`user@domain`) → well-known LNURL-pay endpoint URL @@ -263,7 +286,7 @@ async fn extract_lnurl(address: &str) -> Result { None => return Err(MostroInternalErr(ServiceError::LnAddressParseError)), }; let base_url = if cfg!(test) { - format!("http://{domain}:8080") + format!("http://{domain}:{}", test_ln_port()) } else { format!("https://{domain}") }; @@ -527,12 +550,15 @@ mod tests { #[tokio::test] async fn extract_lnurl_builds_wellknown_url_for_lightning_address() { + // cfg!(test) pins lightning addresses to the local test host form, + // so this assertion has to track the same override the code uses. + let port = super::test_ln_port(); let extracted = extract_lnurl("alice@127.0.0.1") .await .expect("lightning address parses"); assert_eq!( extracted.to_string(), - "http://127.0.0.1:8080/.well-known/lnurlp/alice" + format!("http://127.0.0.1:{port}/.well-known/lnurlp/alice") ); } diff --git a/src/nip33.rs b/src/nip33.rs index dac95ba6..ba7f0efe 100644 --- a/src/nip33.rs +++ b/src/nip33.rs @@ -28,6 +28,13 @@ fn create_event( tags.push(Tag::identifier(identifier)); // Add NIP-40 expiration tag if configured and not already provided. + // One string comparison covers both construction paths: `Tag::kind()` + // returns the tag's serialized name (its first cell, `nostr` 0.45's + // `event/tag/mod.rs`), and both `Tag::expiration(..)` and the + // `Tag::custom("expiration", ..)` that `order_to_tags` actually builds + // put that literal there. No enum matching and no sdk-side name + // normalisation is involved, so neither path can slip past this and + // double-stamp an order event. let has_expiration_tag = tags .iter() .chain(extra_tags.iter()) @@ -1627,6 +1634,58 @@ mod tests { ); } + #[test] + fn create_event_does_not_duplicate_a_caller_supplied_expiration_tag() { + // Order events (kind 38383) always get an auto expiration tag from + // config when one isn't already present. Pre-supplying a real NIP-40 + // expiration tag must suppress the auto-add. + init_test_settings(); + let keys = Keys::generate(); + let extra_tags = Tags::from_list(vec![Tag::expiration(Timestamp::from(123_456_u64))]); + + let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) + .expect("order event"); + + let expiration_tags = order + .tags + .iter() + .filter(|t| t.kind() == "expiration") + .count(); + assert_eq!( + expiration_tags, 1, + "caller-supplied expiration tag must not be duplicated" + ); + } + + #[test] + fn a_custom_named_expiration_tag_also_suppresses_the_auto_add() { + // `order_to_tags` builds the expiration tag by its custom name, so + // this is the shape every real order event reaches `create_event` + // with. It satisfies the same check as `Tag::expiration` because both + // serialise "expiration" into the tag's first cell — this pins that + // the custom-named path is covered, alongside the sibling test for + // the typed one. + init_test_settings(); + let keys = Keys::generate(); + let extra_tags = + Tags::from_list(vec![Tag::custom("expiration", vec!["123456".to_string()])]); + + let order = super::new_order_event(&keys, "", "order-id".to_string(), extra_tags) + .expect("order event"); + + let expiration: Vec<&str> = order + .tags + .iter() + .filter(|t| t.kind() == "expiration") + .filter_map(|t| t.content()) + .collect(); + assert_eq!( + expiration, + vec!["123456"], + "the caller's tag must be the only expiration tag on the event" + ); + } + // ── create_rating_tag ──────────────────────────────────────────────── #[test]