Full mutant coverage by benalleng · Pull Request #928 · payjoin/rust-payjoin

benalleng · 2025-08-05T21:03:58Z

This PR is to expand our mutant coverage to all files within the payjoin/src/ crate. With the introduction of diff mutations this will allow all files changed within the payjoin crate to be checked for new mutations.

coveralls · 2025-08-05T21:07:28Z

Pull Request Test Coverage Report for Build 16945004793

Details

127 of 127 (100.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 86.238%

Totals
Change from base Build 16943032439:	0.3%
Covered Lines:	7670
Relevant Lines:	8894

💛 - Coveralls

DanGould · 2025-08-05T21:12:18Z

.cargo/mutants.toml

 exclude_re = [
 	"impl Debug",
+	"impl fmt::Debug",
 	"impl Display",
+	"impl fmt::Display",


Can regex be used here? something like "impl (fmt::)?(Debug|Display)" ?

It didn't work initially but I haven't given the cargo mutants a thorough read in this part of the docs

Ok, nvm that worked, I must have simply formatted my regex wrong when I tried

benalleng · 2025-08-06T20:57:58Z

There was a mutant reintroduced with the removal of ns1r in #923 however, I realized that the way we tested for it there lacked decent coverage on what was actually mutated. 38da97f covers this mutant in what I believe is a decent way, but it does require quite a lot of testing framework to check it.

Just at first glance does anyone else have a better idea on how to test this mutant?

Found 533 mutants to test
ok       Unmutated baseline in 162.5s build + 10.3s test
 INFO Auto-set test timeout to 52s
MISSED   payjoin/src/core/receive/v2/mod.rs:416:21: replace == with != in Receiver<Initialized>::unchecked_from_payload
 in 3.3s build + 23.3s test

rust-payjoin/payjoin/src/core/receive/v2/mod.rs

Lines 398 to 422 in 0aa1509

    
           fn unchecked_from_payload( 
        
               &mut self, 
        
               payload: &str, 
        
           ) -> Result<v1::UncheckedProposal, ReplyableError> { 
        
               let (base64, padded_query) = payload.split_once('\n').unwrap_or_default(); 
        
               let query = padded_query.trim_matches('\0'); 
        
               log::trace!("Received query: {query}, base64: {base64}"); // my guess is no \n so default is wrong 
        
               let (psbt, mut params) = 
        
                   parse_payload(base64, query, SUPPORTED_VERSIONS).map_err(ReplyableError::Payload)?; 
        
               // Output substitution must be disabled for V1 sessions in V2 contexts. 
        
               // 
        
               // V2 contexts depend on a payjoin directory to store and forward payjoin 
        
               // proposals. Plaintext V1 proposals are vulnerable to output replacement 
        
               // attacks by a malicious directory if output substitution is not disabled. 
        
               // V2 proposals are authenticated and encrypted to prevent such attacks. 
        
               // 
        
               // see: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#unsecured-payjoin-server 
        
               if params.v == Version::One { 
        
                   params.output_substitution = OutputSubstitution::Disabled; 
        
               } 
        
               let inner = v1::UncheckedProposal { psbt, params }; 
        
               Ok(inner) 
        
           }

spacebear21

Exciting work! I have mostly minor comments, my main concern is regarding the new integration test, see below.

spacebear21 · 2025-08-07T08:15:21Z

payjoin/src/core/psbt/mod.rs

+        let txin = &psbt.unsigned_tx.input[0];
+        let mut psbtin = psbt.inputs[0].clone();
+
+        let transaction: Transaction = bitcoin::consensus::encode::deserialize_hex(raw_tx).unwrap();
+        psbtin.non_witness_utxo = Some(transaction);
+
+        let pair: InternalInputPair = InternalInputPair { txin, psbtin: &psbtin };
+        let validated_utxo = pair.validate_utxo();
+        assert_eq!(validated_utxo.unwrap_err(), InternalPsbtInputError::UnequalTxid);
+
+        let txin = &psbt.unsigned_tx.input[0];
+        let mut psbtin = psbt.inputs[0].clone();
+
+        let transaction: Transaction = bitcoin::consensus::encode::deserialize_hex(raw_tx).unwrap();
+        psbtin.non_witness_utxo = Some(transaction);
+        psbtin.witness_utxo = None;
+
+        let pair: InternalInputPair = InternalInputPair { txin, psbtin: &psbtin };
+        let validated_utxo = pair.validate_utxo();
+        assert_eq!(validated_utxo.unwrap_err(), InternalPsbtInputError::UnequalTxid);


I don't see a clear difference between the first and second assert_eq blocks, is it something to do with setting psbtin.witness_utxo = None; explicitly? Explanatory comments might help.

spacebear21 · 2025-08-07T08:19:36Z

payjoin/src/core/psbt/mod.rs

+    fn xpub_inputs() {
+        let mut psbt: Psbt = PARSED_ORIGINAL_PSBT.clone();
+        let mut map = BTreeMap::new();
+        let secp = Secp256k1::new();
+        let seed = Vec::<u8>::from_hex("BEEFCAFE").unwrap();
+        let xpriv = bip32::Xpriv::new_master(NetworkKind::Main, &seed).unwrap();
+        let xpub: bip32::Xpub = bip32::Xpub::from_priv(&secp, &xpriv);
+        let value = (xpriv.fingerprint(&secp), DerivationPath::from_str("42'").unwrap());
+        map.insert(xpub, value);
+        psbt.xpub = map;
+
+        let xpub_mutable = psbt.xpub_mut().clone();
+        assert_eq!(xpub_mutable, psbt.xpub);
+    }
+
+    #[test]
+    fn proprietary_key_inputs() {
+        let mut psbt: Psbt = PARSED_ORIGINAL_PSBT.clone();
+        let mut map = BTreeMap::new();
+        let proprietary_key =
+            ProprietaryKey { prefix: b"mock_prefix".to_vec(), subtype: 0x00, key: vec![] };
+        let value = FromHex::from_hex("BEEFCAFE").unwrap();
+        map.insert(proprietary_key, value);
+        psbt.proprietary = map;
+
+        let proprietary_mutable = psbt.proprietary_mut().clone();
+        assert_eq!(proprietary_mutable, psbt.proprietary);
+    }
+
+    #[test]
+    fn unknown_inputs() {


I don't understand the purpose of these tests.. and also they share a lot of similarities so maybe all belong in one unit?

These are just coverage checks as these methods aren't really used in any tests/code outside of clearing them out here.

rust-payjoin/payjoin/src/core/send/mod.rs

Line 547 in 0aa1509

fn clear_unneeded_fields(psbt: &mut Psbt) {

Wouldn't it be preferable to get this coverage by testing clear_unneeded_fields then?

spacebear21 · 2025-08-07T08:23:36Z

payjoin/tests/integration.rs

+                let enabled_output_substitution =
+                    session.pj_uri().to_string().replace("pjos=0", "pjos=1");


Could just remove the pjos parameter:

Suggested change

let enabled_output_substitution =

session.pj_uri().to_string().replace("pjos=0", "pjos=1");

let enabled_output_substitution =

session.pj_uri().to_string().replace("&pjos=0", "");

spacebear21 · 2025-08-07T08:25:04Z

payjoin/tests/integration.rs

+                    .assume_checked()
+                    .check_pj_supported()
+                    .map_err(|e| e.to_string())?;
+                // We need to check that output substitution has been forced anabled


typo:

Suggested change

// We need to check that output substitution has been forced anabled

// We need to check that output substitution is enabled

(or remove comment)

spacebear21 · 2025-08-07T08:30:42Z

payjoin/tests/integration.rs

        }

+        #[tokio::test]
+        async fn test_enable_output_substitution_v1_to_v2() -> Result<(), BoxSendSyncError> {


This is kind of interesting because under no circumstance should a v2 receiver enable output substitution for v1 sender compatibility. Per BIP 77:

Backwards-compatible receivers MUST disable output substitution by setting pjos=0 to prevent modification by a malicious directory.

This is to avoid a malicious/unsecure directory from modifying the unencrypted PSBT.

I'm not sure how this mutant should be handled, maybe our v1 sender should error if it sees a v2 endpoint with pjos enabled? Paging @DanGould.

Beyond being a mutant this is also the only coverage we have of that function block for ensuring the output substitution is disabled for a v1 -> v2 sender through the directory. So we would still need a test like this even if we error in this case to ensure that the PSBT hasn't been modified.

... if params.v == Version::One { return ReplyableError::Implementation; }

Though we might want a new error type as a possible malicious modification seems beyond a replyable error IMO.

I'd be open to merging this PR without this test, perhaps excluding v1 from cargo mutants for the time being until #948 can be resolved.

benalleng · 2025-08-13T17:36:31Z

Dropping 2338e52 for now as we can address this in a follow-up PR

This pr adds complete mutation coverage onto the payjoin crate within rust-payjoin. We did discover an outstanding mutant that we still want to explore more as to whether we should modify the source code or keep the behavior as is in payjoin#948.

spacebear21

utACK 583c136

DanGould reviewed Aug 5, 2025

View reviewed changes

benalleng force-pushed the full-mutant-coverage branch from 464fe05 to e886ef0 Compare August 6, 2025 17:04

benalleng mentioned this pull request Aug 6, 2025

Expanding Test Coverage #935

Open

benalleng marked this pull request as ready for review August 6, 2025 20:58

spacebear21 reviewed Aug 7, 2025

View reviewed changes

benalleng force-pushed the full-mutant-coverage branch from 38da97f to 36f4853 Compare August 8, 2025 20:37

This was referenced Aug 11, 2025

audit test coverage for spec compliance #844

Open

Backward compatible receiver spec compliance #948

Open

benalleng force-pushed the full-mutant-coverage branch 2 times, most recently from f3e28ad to 2338e52 Compare August 13, 2025 17:19

benalleng force-pushed the full-mutant-coverage branch from 2338e52 to 583c136 Compare August 13, 2025 17:42

spacebear21 approved these changes Aug 14, 2025

View reviewed changes

spacebear21 merged commit ae57a92 into payjoin:master Aug 14, 2025
10 checks passed

		let enabled_output_substitution =
		session.pj_uri().to_string().replace("pjos=0", "pjos=1");

	// We need to check that output substitution has been forced anabled
	// We need to check that output substitution is enabled

Conversation

benalleng commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16945004793

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented Aug 6, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benalleng commented Aug 13, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benalleng commented Aug 5, 2025 •

edited

Loading

coveralls commented Aug 5, 2025 •

edited

Loading

benalleng Aug 7, 2025 •

edited

Loading