Enforce response body size limits in payjoin-cli

[GideonBature]

This PR follows up on #770 and addresses the remaining improvements in comment. It tightens content-length handling on the sender and enforces body size limits in payjoin-cli, so a malicious counterparty cannot trigger unbounded memory allocation.

Summary of changes

1. Sender-side validation

   • Clarifies the sender's ContentTooLarge error so it no longer leaks the internal MAX_CONTENT_LENGTH value, and rewrites the v1 response tests to assert the content-length boundary (under, equal to, and over the limit).

2. payjoin-cli receiver

   • Adds a size limit when reading the request body via a new read_limited_body helper (streaming with into_data_stream()), to avoid unbounded memory allocations from a potentially malicious counterparty.

3. payjoin-cli sender

   • Enforces the same limit when handling response bodies, streaming via bytes_stream() from reqwest and rejecting upfront when the Content-Length header exceeds the cap.

Ref: #770
Closes: #756

[coveralls]

Coverage Report for CI Build 28286095386

Coverage increased (+0.03%) to 85.549%

Details

• Coverage increased (+0.03%) from the base build.
• Patch coverage: 11 uncovered changes across 2 files (52 of 63 lines covered, 82.54%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v1.rs	22	12	54.55%
payjoin-cli/src/app/mod.rs	14	13	92.86%
Total (4 files)	63	52	82.54%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	15168
Covered Lines:	12976
Line Coverage:	85.55%
Coverage Strength:	360.79 hits per line

---

💛 - Coveralls

[GideonBature]

I have implemented the changes suggested. However, I had to introduce about two dependencies, which are:

• futures
• bytes

as a result of that I attempted to update the lock files using the update-lock-files.sh script. However, I kept getting can't find crate for serde`` for bitcoin-0.32.5 and for `rustls-0.22.4`.

I want you to review what I have thus far, let me know if I am going in the right direction, as I find a solution to the error I am having.

[nothingmuch]

This is looking great. Thank you!

The new dependencies are fine IMO, because they are already in our dependency chain if I'm not mistaken, and also because payjoin-cli can be more flexible about new dependencies than the payjoin crate.

The buffering code is very clear. I think a Vec<u8> can replace Bytes, since they are more or less the same on the inside, the main difference is Bytes support cheap clone(), which we don't need, for the cost of slightly more indirection.

Instead of duplicating it in the sender and receiver, a utility function can be added to payjoin-cli's app module.

Finally, as a nitpick, I find additional whitespace to be helpful when reading, but please try to commit such trivial changes in their own commits, that makes it easier to review commit by commit, github's files view uses lexicographical order for files and combines all the changes of all commits, both of which make it harder to spot important changes in between trivial ones.

[GideonBature]

I implemented the suggestions you gave, however, for the utility function, putting it in mod.rs of app, I am encountering a challenge where I am unable to access the hyper dependencies, I really don't know why. And I will be needing it, since I will need to make use of Bytes from it for the utility function.

[nothingmuch]

I didn't quite understand your last comment about hyper & bytes, but i think the read_limited_body function is in the right place?

similar limiting for app/v2.rs can be added in a separate PR, with a fixed response size, and reusing this function, but moving it to the top level mod.rs should be straightforward (just making it pub(crate) and bringing in the necessary imports, namely hyper::body::Bytes)

anyway this is getting close, i only have some fairly small cleanup suggestions

[GideonBature]

Lint is failing with this:

error[E0599]: no variant or associated item named `Parse` found for enum `core::send::error::InternalValidationError` in the current scope
   --> payjoin/src/core/send/mod.rs:699:90
    |
699 |         let res_str = std::str::from_utf8(response).map_err(|_| InternalValidationError::Parse)?;
    |                                                                                          ^^^^^ variant or associated item not found in `core::send::error::InternalValidationError`
    |
   ::: payjoin/src/core/send/error.rs:94:1
    |
 94 | pub(crate) enum InternalValidationError {
    | --------------------------------------- variant or associated item `Parse` not found for this enum

error[E0599]: no variant or associated item named `parse` found for enum `core::send::error::ResponseError` in the current scope
   --> payjoin/src/core/send/mod.rs:700:75
    |
700 |         let proposal = Psbt::from_str(res_str).map_err(|_| ResponseError::parse(res_str))?;
    |                                                                           ^^^^^ variant or associated item not found in `core::send::error::ResponseError`
    |
   ::: payjoin/src/core/send/error.rs:250:1
    |
250 | pub enum ResponseError {
    | ---------------------- variant or associated item `parse` not found for this enum

I don't really know, I observed that the Parse is actually present, but it was feature gated to v1, I don't know if that is what is causing the lint to fail here? Same thing with the parse too in the ResponseError.

[GideonBature]

Resolved merge conflict.

[nothingmuch]

I appear to have compeltely forgotten about this PR, I'm really sorry about this...

there appear to be unrelated changes, likely the result of an incorrect rebase? the changes having to do with the length enforcement seem correct to me but i'm not sure why some things like V1Context are there

[GideonBature]

Oh, I will do a check and fix that the push for your review.

[nothingmuch]

utACK, but commit needs cleanup:

1. lcov.info shouldn't be added, should probably be in gitignore but that's a separate issue

2. i don't understand why rcgen was added to Cargo.toml, i think only stream feature to reqwest was intended in this PR?

[GideonBature]

utACK, but commit needs cleanup:

1. lcov.info shouldn't be added, should probably be in gitignore but that's a separate issue
2. i don't understand why rcgen was added to Cargo.toml, i think only stream feature to reqwest was intended in this PR?

Yes, I think I made a mixup, let me remove it. Then create an issue to add Icov.info to .gitignore.

[DanGould]

@GideonBature do you have plans to continue work here? If not, we can close for now and revisit when there's more capacity for it. Icov.info is now be gitignored.

[GideonBature]

Yes @DanGould I will finish it up... Thank you for reaching out.

[DanGould]

Thanks for the fast turnaround on this @GideonBature. Please rebase the history so that formatting changes are squashed into the commit that introduces the code and so that the rules are followed according to the contributing guidelines, the intermediate commit 6bd1c78 failing in CI suggests that these steps aren't done yet. Seems like there should be two commits passing tests in this PR according to those rules. There are commit message rules there, too. Once those are done, happy to review.

[GideonBature]

Thanks for the fast turnaround on this @GideonBature. Please rebase the history so that formatting changes are squashed into the commit that introduces the code and so that the rules are followed according to the contributing guidelines, the intermediate commit 6bd1c78 failing in CI suggests that these steps aren't done yet. Seems like there should be two commits passing tests in this PR according to those rules. There are commit message rules there, too. Once those are done, happy to review.

Noted. I will do that.

[DanGould]

@GideonBature Thank you for following up. I took a peek at this when it was just posted but was confused because the commit message rules don't seem to be addressed. Since this now needs to be rebased, may you please make sure the commit message follows the rules in CONTRIBUTING.md including rationale for the changes if you would be so kind as to rebase?

[GideonBature]

@GideonBature Thank you for following up. I took a peek at this when it was just posted but was confused because the commit message rules don't seem to be addressed. Since this now needs to be rebased, may you please make sure the commit message follows the rules in CONTRIBUTING.md including rationale for the changes if you would be so kind as to rebase?

Oh my bad. I will resolve this... Thank you...