From 8e9a02f55a97601a8b00377aac3224f77916ddd6 Mon Sep 17 00:00:00 2001 From: Tori Date: Tue, 18 Aug 2026 22:29:50 -0500 Subject: [PATCH 1/6] test(restore-session): name the 1h timeout and cover it cargo mutants left the `60 * 60` timeout computation untestable inline. Extract it as RESTORE_SESSION_TIMEOUT_SECS with a test that pins the value. The hex-validation extraction this commit originally carried is dropped: its only two call sites are the guards #848 removes as unreachable (`identity`/`sender` are `PublicKey`, so `.to_string()` is always 64 hex), and the two invalid-key tests it added already exist on main from #803. --- src/app/restore_session.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/app/restore_session.rs b/src/app/restore_session.rs index 8565ad53..4620924b 100644 --- a/src/app/restore_session.rs +++ b/src/app/restore_session.rs @@ -3,6 +3,10 @@ use crate::{db::RestoreSessionManager, util::enqueue_restore_session_msg}; 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 @@ -51,7 +55,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)) => { @@ -128,6 +132,11 @@ mod tests { use sqlx::SqlitePool; use std::sync::Arc; + #[test] + fn restore_session_timeout_is_one_hour() { + assert_eq!(RESTORE_SESSION_TIMEOUT_SECS, 3600); + } + async fn create_test_pool() -> SqlitePool { let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); sqlx::migrate!().run(&pool).await.unwrap(); From c9e402194c724c5c40570ecd39e6ea2e6620a0d8 Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 17:04:32 -0500 Subject: [PATCH 2/6] 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 d62839725ae6884db405b7f2e8c58ef308ac736c Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 17:04:47 -0500 Subject: [PATCH 3/6] 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 2b9e0c817c820dcad8e5b6758ab2b73d8b65bed5 Mon Sep 17 00:00:00 2001 From: Tori Date: Mon, 20 Jul 2026 19:23:57 -0500 Subject: [PATCH 4/6] 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) From b31d3d01a477c62d8396251811d7af537aaec5e6 Mon Sep 17 00:00:00 2001 From: Tori Date: Tue, 4 Aug 2026 10:27:54 -0500 Subject: [PATCH 5/6] fix(mutation): remove the config that broke every mutation run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.cargo/mutants.toml` (added in 87b2b6f, this PR) set additional_cargo_test_args = ["--test-threads=4"] cargo-mutants places those args before `cargo test`'s own `--`, so cargo rejects the flag rather than forwarding it to libtest: *** cargo test --verbose --package=mostro@0.18.0 --test-threads=4 error: unexpected argument '--test-threads' found *** result: Failure(1) ERROR cargo test failed in an unmutated tree, so no mutants were tested The baseline never passed, so no mutant was ever tested — via the Makefile target or the CI job, since cargo-mutants reads this file regardless of how it is invoked. Intended as an OOM guard, it silently disabled the thing it was guarding. No config-file or CLI mechanism in cargo-mutants 27.1.0 forwards arguments past that `--`, and `CARGO_MUTANTS_JOBS` is the cap that actually binds. Removing the file restores the baseline: the suite now runs to completion (1021 passed locally, the one failure being the known hardcoded-8080 `AddrInUse` flake that PR #849 fixes). --- .cargo/mutants.toml | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 .cargo/mutants.toml diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml deleted file mode 100644 index f95d399a..00000000 --- a/.cargo/mutants.toml +++ /dev/null @@ -1,4 +0,0 @@ -# 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"] From efd8a7c3dcbefd87f7e93c6d615d5eb83acedaa9 Mon Sep 17 00:00:00 2001 From: Tori Date: Tue, 18 Aug 2026 22:48:45 -0500 Subject: [PATCH 6/6] fix(restore-session): derive the timeout log from the constant The timeout branch reported a hardcoded "1 hour" while the duration comes from RESTORE_SESSION_TIMEOUT_SECS, so the two drift apart the moment the constant changes. Format the log from the constant instead. --- src/app/restore_session.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/app/restore_session.rs b/src/app/restore_session.rs index 4620924b..19225e33 100644 --- a/src/app/restore_session.rs +++ b/src/app/restore_session.rs @@ -74,7 +74,9 @@ 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 {RESTORE_SESSION_TIMEOUT_SECS} seconds" + ); // Send timeout message to user if let Err(e) = send_restore_session_timeout(&trade_key).await { tracing::error!("Failed to send timeout message: {}", e);