test: mutation testing for Nostr event handling

.github/workflows/mutation.yml

@@ -60,15 +60,26 @@ 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"
+
+          CARGO_MUTANTS_JOBS=1 MOSTRO_TEST_LN_PORT=18080 \
+            cargo mutants "${file_args[@]}"
 
       - name: Upload mutation report
         uses: actions/upload-artifact@v4
@@ -195,7 +206,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
 

Makefile

@@ -66,3 +66,32 @@ docker-build-startos:
 	cd docker && \
 	docker compose build mostro-startos
 
+# cargo-mutants tests one mutant at a time by default (verified against
+# 27.1.0: a single scratch dir on a 16-CPU host with no -j). This target
+# sets it explicitly anyway, because the suite genuinely cannot tolerate
+# more: the LNURL tests bind a fixed host port, so parallel workers collide
+# on the listener, and a test that fails for its own reasons scores as a
+# killed mutant — silently inflating the number this target exists to
+# measure.
+#
+# 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 workers
+# would still collide with each other 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:
+	@set -o pipefail; \
+	CARGO_MUTANTS_JOBS=1 MOSTRO_TEST_LN_PORT=$${MOSTRO_TEST_LN_PORT:-18080} cargo mutants $(ARGS)

src/app.rs

@@ -294,6 +294,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<Signature>,
+) -> 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
@@ -404,7 +422,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();
@@ -413,7 +431,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,
@@ -680,6 +698,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;
@@ -906,6 +964,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");
@@ -1093,6 +1193,111 @@ mod tests {
         }
     }
 
+    mod accept_event_tests {
+        use super::*;
+        use crate::app::context::test_utils::{test_settings, TestContextBuilder};
+        use mostro_core::prelude::*;
+        use sqlx::SqlitePool;
+        use std::sync::Arc;
+
+        async fn create_test_ctx() -> AppContext {
+            let pool = Arc::new(SqlitePool::connect(":memory:").await.unwrap());
+            TestContextBuilder::new()
+                .with_pool(pool)
+                .with_settings(test_settings())
+                .build()
+        }
+
+        #[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`.
+            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_test_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_test_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_test_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());
+        }
+    }
+
     mod handle_message_action_tests {
         use super::*;
         use crate::app::context::test_utils::{test_settings, TestContextBuilder};

src/app/restore_session.rs

@@ -3,6 +3,19 @@ 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.
+///
+/// Written as a literal rather than `60 * 60`: the product is a computation
+/// with nothing to compute, and it only exists for a mutation operator to
+/// flip into `60 + 60`. No test can meaningfully catch that without
+/// restating the constant, so the operator is better removed than guarded.
+const RESTORE_SESSION_TIMEOUT_SECS: u64 = 3_600; // 1 hour
+
 /// 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 +54,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 +73,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");
+            tracing::error!(
+                "Restore session timed out after {} minutes",
+                RESTORE_SESSION_TIMEOUT_SECS / 60
+            );
             // Send timeout message to user
             if let Err(e) = send_restore_session_timeout(&trade_key).await {
                 tracing::error!("Failed to send timeout message: {}", e);

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();