From 066d6e37707800fbdd4e4f0fc45dcf3cab81539a Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 15:30:59 -0500 Subject: [PATCH 1/4] test(restore-session): raise mutation score with pure-fn extraction Follow-up of #618. cargo mutants on src/app/restore_session.rs went from 33.3% (4/12) to 83.3% (10/12): - Extract pubkey-hex validation into util::is_valid_hex_pubkey, shared with the identical check already duplicated 5x in db.rs, instead of adding a 6th private copy. - Extract RESTORE_SESSION_TIMEOUT_SECS as a named constant so the 1h timeout computation is directly testable. - Add focused tests for send_restore_session_response/_timeout's early error path on an invalid trade key (no DB/network needed). The 2 remaining survivors replace restore_session_action and handle_restore_session_results wholesale; killing them needs a full AppContext (real pool + nostr client + settings), which is out of proportion for this issue and consistent with admin_add_solver_action being untested at that level too. Closes #637 --- src/app/restore_session.rs | 41 ++++++++++++++++++++++++++++++++---- src/util.rs | 43 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/app/restore_session.rs b/src/app/restore_session.rs index 8c874efe..99ec050f 100644 --- a/src/app/restore_session.rs +++ b/src/app/restore_session.rs @@ -1,8 +1,15 @@ use crate::app::context::AppContext; -use crate::{db::RestoreSessionManager, util::enqueue_restore_session_msg}; +use crate::{ + db::RestoreSessionManager, + util::{enqueue_restore_session_msg, is_valid_hex_pubkey}, +}; use mostro_core::prelude::*; use nostr_sdk::prelude::*; +/// Restore session results wait for this long before the requester is told +/// to retry instead of hanging forever. +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 @@ -17,12 +24,12 @@ pub async fn restore_session_action( let trade_key = event.sender.to_string(); // Validate the master key format - if !master_key.chars().all(|c| c.is_ascii_hexdigit()) || master_key.len() != 64 { + if !is_valid_hex_pubkey(&master_key) { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } // Validate the trade key format - if !trade_key.chars().all(|c| c.is_ascii_hexdigit()) || trade_key.len() != 64 { + if !is_valid_hex_pubkey(&trade_key) { return Err(MostroCantDo(CantDoReason::InvalidPubkey)); } @@ -51,7 +58,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)) => { @@ -116,3 +123,29 @@ async fn send_restore_session_timeout(trade_key: &str) -> Result<(), MostroError Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn restore_session_timeout_is_one_hour() { + assert_eq!(RESTORE_SESSION_TIMEOUT_SECS, 3600); + } + + #[tokio::test] + async fn send_restore_session_response_rejects_invalid_trade_key() { + let err = send_restore_session_response("not-a-pubkey", vec![], vec![]) + .await + .unwrap_err(); + assert_eq!(err, MostroError::MostroCantDo(CantDoReason::InvalidPubkey)); + } + + #[tokio::test] + async fn send_restore_session_timeout_rejects_invalid_trade_key() { + let err = send_restore_session_timeout("not-a-pubkey") + .await + .unwrap_err(); + assert_eq!(err, MostroError::MostroCantDo(CantDoReason::InvalidPubkey)); + } +} diff --git a/src/util.rs b/src/util.rs index f98d0481..69a1fcd8 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1251,6 +1251,12 @@ pub fn bytes_to_string(bytes: &[u8]) -> String { }) } +/// A Nostr pubkey, as carried on the wire, is a 64-char hex string +/// (case-insensitive). +pub fn is_valid_hex_pubkey(key: &str) -> bool { + key.len() == 64 && key.chars().all(|c| c.is_ascii_hexdigit()) +} + pub async fn enqueue_cant_do_msg( request_id: Option, order_id: Option, @@ -1695,6 +1701,43 @@ mod tests { assert_eq!(result, ""); } + const VALID_HEX_PUBKEY: &str = + "1111111111111111111111111111111111111111111111111111111111111111"; + + #[test] + fn valid_hex_pubkey_is_accepted() { + assert_eq!(VALID_HEX_PUBKEY.len(), 64); + assert!(is_valid_hex_pubkey(VALID_HEX_PUBKEY)); + } + + #[test] + fn valid_hex_pubkey_accepts_uppercase() { + assert!(is_valid_hex_pubkey(&VALID_HEX_PUBKEY.to_uppercase())); + } + + #[test] + fn valid_hex_pubkey_rejects_too_short_key() { + assert!(!is_valid_hex_pubkey(&VALID_HEX_PUBKEY[..63])); + } + + #[test] + fn valid_hex_pubkey_rejects_too_long_key() { + let too_long = format!("{VALID_HEX_PUBKEY}1"); + assert!(!is_valid_hex_pubkey(&too_long)); + } + + #[test] + fn valid_hex_pubkey_rejects_non_hex_characters() { + let mut key = VALID_HEX_PUBKEY[..63].to_string(); + key.push('g'); + assert!(!is_valid_hex_pubkey(&key)); + } + + #[test] + fn valid_hex_pubkey_rejects_empty_key() { + assert!(!is_valid_hex_pubkey("")); + } + #[tokio::test] async fn test_send_dm() { initialize(); From 3dcab9300c04f5ec5cda09bbbb79c5b10503b27f Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 17:04:32 -0500 Subject: [PATCH 2/4] chore: gitignore local .claude/ settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Matches the existing .idea/.vscode/.cursor pattern — this holds per-machine tool permissions, not project config. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index fd5316fb..450d5ee5 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ lnurl-test-server/target .idea .vscode .cursor +.claude # settings file settings.toml From 87b2b6fad262af65739e4db19503488afbf623dc Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 17:04:47 -0500 Subject: [PATCH 3/4] ci(mutation): cap cargo-mutants concurrency to avoid OOM Uncapped parallel jobs + per-test thread fan-out exhausted RAM and crashed the machine during a local run. Cap via CARGO_MUTANTS_JOBS=2 (Makefile, verified with strace since .cargo/config.toml's [env] does not propagate to third-party subcommands) and --test-threads=4 (.cargo/mutants.toml). Both CI mutation jobs now go through the same `make mutation-test` target. --- .cargo/mutants.toml | 4 ++++ .github/workflows/mutation.yml | 4 ++-- Makefile | 4 ++++ 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 .cargo/mutants.toml diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml new file mode 100644 index 00000000..f95d399a --- /dev/null +++ b/.cargo/mutants.toml @@ -0,0 +1,4 @@ +# Cap test-thread fan-out per mutant run. Without this, each cargo-mutants +# job spawns a test binary with --test-threads = num_cpus, which multiplies +# with CARGO_MUTANTS_JOBS (see `make mutation-test`) and can exhaust RAM. +additional_cargo_test_args = ["--test-threads=4"] diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml index 818255f1..dea55947 100644 --- a/.github/workflows/mutation.yml +++ b/.github/workflows/mutation.yml @@ -68,7 +68,7 @@ jobs: echo "Running mutation testing for changed files:" echo "$changed_rs" - cargo mutants $file_args + make mutation-test ARGS="$file_args" - name: Upload mutation report uses: actions/upload-artifact@v4 @@ -195,7 +195,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..094f1993 100644 --- a/Makefile +++ b/Makefile @@ -66,3 +66,7 @@ docker-build-startos: cd docker && \ docker compose build mostro-startos +mutation-test: + @set -o pipefail; \ + CARGO_MUTANTS_JOBS=2 cargo mutants $(ARGS) + From 8c0e3f94d4daadcfc751f13cb2182ce57bbe2c30 Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 19:23:57 -0500 Subject: [PATCH 4/4] security(ci): stop splicing PR-diff filenames through make(ARGS)/shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filenames from `git diff --name-only` on a PR are attacker-controlled. The old `file_args="$file_args --file $f"` + `make mutation-test ARGS="$file_args"` path round-tripped that string through Make's macro substitution and a second shell parse, so a crafted "filename" could word-split into standalone argv tokens — argument injection into cargo-mutants/cargo/rustc's own flag surface, not classic shell command substitution (unquoted variable expansion doesn't re-parse $()/backticks, but it does still word-split). Fixed by building a bash array (`file_args+=(--file "$f")`) and expanding it with "${file_args[@]}", so each filename — however it's spelled — can only ever land as the single value of one --file flag. This bypasses `make mutation-test` for this call site specifically; its $(ARGS) stays a plain string splice, fine for human-typed input (the label-triggered baseline job, unaffected, still uses it), not for diff-derived filenames. Comment added to the Makefile target so that distinction doesn't get lost later. Found by CodeRabbit on PR #826's post-rebase re-review. --- .github/workflows/mutation.yml | 23 ++++++++++++++++------- Makefile | 6 ++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml index dea55947..f02ece6b 100644 --- a/.github/workflows/mutation.yml +++ b/.github/workflows/mutation.yml @@ -60,15 +60,24 @@ 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" - make mutation-test ARGS="$file_args" + + # These filenames come from a PR diff, so they're 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 a single + # --file value, never additional argv tokens — the intermediate + # 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 + # human-typed, trusted input (the baseline job below still uses it). + file_args=() + while IFS= read -r f; do + file_args+=(--file "$f") + done <<< "$changed_rs" + + CARGO_MUTANTS_JOBS=2 cargo mutants "${file_args[@]}" - name: Upload mutation report uses: actions/upload-artifact@v4 diff --git a/Makefile b/Makefile index 094f1993..2b417669 100644 --- a/Makefile +++ b/Makefile @@ -66,6 +66,12 @@ docker-build-startos: cd docker && \ docker compose build mostro-startos +# 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 instead (see the +# PR job in .github/workflows/mutation.yml). mutation-test: @set -o pipefail; \ CARGO_MUTANTS_JOBS=2 cargo mutants $(ARGS)