Limit response sizes for v1

[shinghim]

Add content length checks in v1 send and payjoin-cli.

Fixes #483

[coveralls]

Pull Request Test Coverage Report for Build 15169166399

Details

• 41 of 48 (85.42%) changed or added relevant lines in 2 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.09%) to 83.525%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/error.rs	0	2	0.0%
payjoin/src/send/v1.rs	41	46	89.13%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/send/error.rs	2	39.72%

Totals
Change from base Build 15167733837:	0.09%
Covered Lines:	5947
Relevant Lines:	7120

---

💛 - Coveralls

[benalleng]

Looking good, I left a comment about cleaning up the test to make sure we don't get any false positives. The private errors can make it difficult to drill down to get the precise error but for now I think string matches are ok.

[benalleng]

Thanks for cleaning up the naming on those test-utils consts! I have another approach that I have decided I think is better with regards to the test cases. Sorry to lead you on a goose chase with these

[shinghim]

This error is being mapped to InternalValidationError::Parse, so we won't see the same error message from the rust std lib. Instead, we'll see

let expected_error =
            "The receiver sent an invalid response: couldn't decode as PSBT or JSON";

defined here:

impl fmt::Display for ValidationError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        use InternalValidationError::*;

        match &self.0 {
            Parse => write!(f, "couldn't decode as PSBT or JSON",),
            Io(e) => write!(f, "couldn't read PSBT: {}", e),
            ContentTooLarge => write!(f, "content is larger than {} bytes", MAX_CONTENT_LENGTH),
            Proposal(e) => write!(f, "proposal PSBT error: {}", e),
            #[cfg(feature = "v2")]
            V2Encapsulation(e) => write!(f, "v2 encapsulation error: {}", e),
        }
    }
}

[benalleng]

You're totally right, I got my wires crossed somehow. So we could use the same match code as above in that case to not rely on this expected_error string.

[shinghim]

Done. Good idea on comparing the strings since the PartialEq stuff is kinda hairy with the internal/external stuff

[benalleng]

My last suggestion as it was just pointed out to me as a better practice is Ithat we should probably improve the panic strings here when something goes wrong. Something like.
Ok(_) => panic!("PSBT is malformed and should error") and
Ok(_) => panic!("Response string is malformed and should error")

[benalleng]

tACK everything is looking good. I maybe would like a unit test specifically for the MAX_CONTENT_LENGTH but that's something we can catch when we come back through for send mutants

[spacebear21]

ACK 3725c02

[DanGould]

rats. This one needs rebase

[shinghim]

done 👍🏼

[DanGould]

Thanks for the rebase. I do have one additional comment you might be able to clear up for me

[DanGould]

Since I assume this mitigation would need to be implemented on all senders, wouldn't it make more sense to expose the constant defined in payjoin::send::v1 and use that here?

[shinghim]

I added this at the lib level cause it made sense to me that if we were sending something with the max size of MAX_CONTENT_LENGTH, we would also receive something with that same limit. Exposing it in payjoin and using it here makes sense and would be better to just have it defined in one place

[DanGould]

Would it not make sense to expose this as pub const for use in the implementation? Or to make it `pub(crate) for _core and NOT(v1) but pub for v1?

[shinghim]

you're right, it does. Updated to expose this in lib and use in the implementation

[spacebear21]

ACK - I took the liberty to rebase onto latest master.