From c87f3c0e6af389d82e41dfd0606097b79103ad40 Mon Sep 17 00:00:00 2001 From: DanGould Date: Thu, 24 Sep 2026 13:14:19 +0800 Subject: [PATCH] Return an error when a ClientResponse is reused Each process_response call takes the OHTTP context out of the ClientResponse it is given. Bindings hand ClientResponse out as a shared object and pass it by reference, so nothing stops a caller from passing the same one twice, for example when retrying after a transient failure. The second call panicked, which crosses the FFI boundary as an opaque exception, or aborts the process when built with panic=abort. Make the conversion fallible and return a new ClientResponseError from every process_response and process_error_response instead. These methods now throw in the foreign bindings; their return types are unchanged. --- payjoin-ffi/src/ohttp.rs | 22 ++++++++-- payjoin-ffi/src/receive/mod.rs | 73 ++++++++++++++++++++++++++++------ payjoin-ffi/src/send/mod.rs | 64 ++++++++++++++++++++++++----- 3 files changed, 134 insertions(+), 25 deletions(-) diff --git a/payjoin-ffi/src/ohttp.rs b/payjoin-ffi/src/ohttp.rs index c3d8edb79..d08b1b625 100644 --- a/payjoin-ffi/src/ohttp.rs +++ b/payjoin-ffi/src/ohttp.rs @@ -27,13 +27,29 @@ impl OhttpKeys { use std::sync::Mutex; +/// The OHTTP context needed to decapsulate the response to one request. +/// +/// A context can process exactly one response. Passing it to a second +/// `process_response` call returns [`ClientResponseError::AlreadyUsed`]. #[derive(uniffi::Object)] pub struct ClientResponse(Mutex>); -impl From<&ClientResponse> for payjoin::OhttpResponse { - fn from(value: &ClientResponse) -> Self { +/// Error returned when a [`ClientResponse`] is passed to more than one +/// `process_response` call. +#[derive(Debug, thiserror::Error, uniffi::Error)] +pub enum ClientResponseError { + /// The OHTTP context was already used to process a response. Create a + /// new request to get a fresh context before retrying. + #[error("OHTTP response context was already used")] + AlreadyUsed, +} + +impl TryFrom<&ClientResponse> for payjoin::OhttpResponse { + type Error = ClientResponseError; + + fn try_from(value: &ClientResponse) -> Result { let mut data_guard = value.0.lock().unwrap(); - Option::take(&mut *data_guard).expect("ClientResponse moved out of memory") + Option::take(&mut *data_guard).ok_or(ClientResponseError::AlreadyUsed) } } diff --git a/payjoin-ffi/src/receive/mod.rs b/payjoin-ffi/src/receive/mod.rs index a0e81f90f..4980e9d58 100644 --- a/payjoin-ffi/src/receive/mod.rs +++ b/payjoin-ffi/src/receive/mod.rs @@ -21,7 +21,7 @@ use crate::validation::{ validate_fee_rate_sat_per_vb_opt, validate_optional_script, validate_script_bytes, validate_script_vec, validate_weight_units, validate_witness_stack, }; -use crate::{ClientResponse, OutputSubstitution, Request}; +use crate::{ClientResponse, ClientResponseError, OutputSubstitution, Request}; pub mod error; @@ -714,10 +714,17 @@ impl Initialized { /// Returns an [`InitializedTransition`] that, once persisted, yields either /// an [`UncheckedOriginalPayload`] if the sender's Original PSBT is available, /// or [`Initialized`] if no proposal has arrived yet. - pub fn process_response(&self, body: &[u8], ctx: &ClientResponse) -> InitializedTransition { - InitializedTransition(Arc::new(RwLock::new(Some( - self.0.clone().process_response(body, ctx.into()), - )))) + /// + /// Returns [`ClientResponseError::AlreadyUsed`] if `ctx` was already used + /// to process a response. + pub fn process_response( + &self, + body: &[u8], + ctx: &ClientResponse, + ) -> Result { + Ok(InitializedTransition(Arc::new(RwLock::new(Some( + self.0.clone().process_response(body, ctx.try_into()?), + ))))) } /// Build a V2 Payjoin URI from the receiver's context @@ -1408,14 +1415,17 @@ impl PayjoinProposal { /// This function decapsulates the response using the provided OHTTP context. If the response status is successful, it indicates that the Payjoin proposal has been accepted. Otherwise, it returns an error with the status code. /// /// After this function is called, the receiver can either wait for the Payjoin transaction to be broadcast or choose to broadcast the original PSBT. + /// + /// Returns [`ClientResponseError::AlreadyUsed`] if `ohttp_context` was + /// already used to process a response. pub fn process_response( &self, body: &[u8], ohttp_context: &ClientResponse, - ) -> PayjoinProposalTransition { - PayjoinProposalTransition(Arc::new(RwLock::new(Some( - self.0.clone().process_response(body, ohttp_context.into()), - )))) + ) -> Result { + Ok(PayjoinProposalTransition(Arc::new(RwLock::new(Some( + self.0.clone().process_response(body, ohttp_context.try_into()?), + ))))) } } @@ -1508,14 +1518,17 @@ impl HasReplyableError { /// completes the error reporting and either yields a /// [`ReceiverPendingFallback`] if current session has validated fallback tx, /// or otherwise closes the session. + /// + /// Returns [`ClientResponseError::AlreadyUsed`] if `ohttp_context` was + /// already used to process a response. pub fn process_error_response( &self, body: &[u8], ohttp_context: &ClientResponse, - ) -> HasReplyableErrorTransition { - HasReplyableErrorTransition(Arc::new(RwLock::new(Some( - self.0.clone().process_error_response(body, ohttp_context.into()), - )))) + ) -> Result { + Ok(HasReplyableErrorTransition(Arc::new(RwLock::new(Some( + self.0.clone().process_error_response(body, ohttp_context.try_into()?), + ))))) } } @@ -1833,3 +1846,37 @@ impl payjoin::persist::AsyncSessionPersister for AsyncCallbackPersisterAdapter { async move { persister.close().await } } } + +#[cfg(all(test, feature = "_test-utils"))] +mod tests { + use payjoin::persist::InMemoryPersister; + use payjoin::receive::v2::ReceiverBuilder; + use payjoin_test_utils::EXAMPLE_URL; + + use super::*; + + #[test] + fn reusing_ohttp_context_returns_error() { + let address = + payjoin::bitcoin::Address::from_str("tb1q6d3a2w975yny0asuvd9a67ner4nks58ff0q8g4") + .expect("valid address") + .assume_checked(); + let ohttp_keys = payjoin::OhttpKeys::decode(&payjoin_test_utils::ohttp_key_config_bytes()) + .expect("valid ohttp keys"); + let receiver: Initialized = ReceiverBuilder::new(address, EXAMPLE_URL, ohttp_keys) + .expect("valid receiver builder") + .build() + .save(&InMemoryPersister::default()) + .expect("in-memory persister is infallible") + .into(); + + let ctx = receiver.create_poll_request(EXAMPLE_URL.to_string()).expect("valid request"); + // An undersized body is a transient failure, so a caller may retry. + // Retrying with the same, now consumed, context must return an error. + receiver + .process_response(&[0u8; 1], &ctx.client_response) + .expect("first use of the context"); + let reused = receiver.process_response(&[0u8; 1], &ctx.client_response); + assert!(matches!(reused, Err(ClientResponseError::AlreadyUsed))); + } +} diff --git a/payjoin-ffi/src/send/mod.rs b/payjoin-ffi/src/send/mod.rs index cae799714..982d3552f 100644 --- a/payjoin-ffi/src/send/mod.rs +++ b/payjoin-ffi/src/send/mod.rs @@ -8,7 +8,7 @@ pub use error::{ use crate::error::ForeignError; pub use crate::error::{ImplementationError, SerdeJsonError}; -use crate::ohttp::ClientResponse; +use crate::ohttp::{ClientResponse, ClientResponseError}; use crate::request::Request; use crate::send::error::{SenderPersistedError, SenderReplayError}; use crate::uri::PjUri; @@ -539,14 +539,17 @@ impl WithReplyKey { /// A successful response can either be `None` if the relay has no response yet, /// or `Some(Psbt)`. /// If the response is a valid PSBT you should sign and broadcast it. + /// + /// Returns [`ClientResponseError::AlreadyUsed`] if `post_ctx` was already + /// used to process a response. pub fn process_response( &self, response: &[u8], post_ctx: &ClientResponse, - ) -> WithReplyKeyTransition { - WithReplyKeyTransition(Arc::new(RwLock::new(Some( - self.0.clone().process_response(response, post_ctx.into()), - )))) + ) -> Result { + Ok(WithReplyKeyTransition(Arc::new(RwLock::new(Some( + self.0.clone().process_response(response, post_ctx.try_into()?), + ))))) } } @@ -659,14 +662,17 @@ impl PollingForProposal { /// A successful response can either be `None` if the relay has no response yet, /// or `Some(Psbt)`. /// If the response is a valid PSBT you should sign and broadcast it. + /// + /// Returns [`ClientResponseError::AlreadyUsed`] if `ohttp_ctx` was already + /// used to process a response. pub fn process_response( &self, response: &[u8], ohttp_ctx: &ClientResponse, - ) -> PollingForProposalTransition { - PollingForProposalTransition(Arc::new(RwLock::new(Some( - self.0.clone().process_response(response, ohttp_ctx.into()), - )))) + ) -> Result { + Ok(PollingForProposalTransition(Arc::new(RwLock::new(Some( + self.0.clone().process_response(response, ohttp_ctx.try_into()?), + ))))) } } @@ -882,4 +888,44 @@ mod tests { SenderBuilder::new(ORIGINAL_PSBT.to_string(), pj_uri(V2_PJ_URI)) .expect("v2 URI must be accepted"); } + + #[test] + fn reusing_ohttp_context_returns_error() { + use payjoin::persist::InMemoryPersister; + use payjoin::receive::v2::ReceiverBuilder; + use payjoin_test_utils::{EXAMPLE_URL, PARSED_ORIGINAL_PSBT}; + + let address = payjoin::bitcoin::Address::from_str("2N47mmrWXsNBvQR6k78hWJoTji57zXwNcU7") + .expect("valid address") + .assume_checked(); + let ohttp_keys = payjoin::OhttpKeys::decode(&payjoin_test_utils::ohttp_key_config_bytes()) + .expect("valid ohttp keys"); + let pj_uri = ReceiverBuilder::new(address, EXAMPLE_URL, ohttp_keys) + .expect("valid receiver builder") + .build() + .save(&InMemoryPersister::default()) + .expect("in-memory persister is infallible") + .pj_uri(); + let payjoin::PjParam::V2(pj_param) = pj_uri.extras().pj_param() else { + panic!("receiver URI must carry a v2 pj param"); + }; + let sender: WithReplyKey = payjoin::send::v2::SenderBuilder::from_parts( + PARSED_ORIGINAL_PSBT.clone(), + pj_param, + pj_uri.address(), + pj_uri.amount(), + ) + .build_recommended(payjoin::bitcoin::FeeRate::BROADCAST_MIN) + .expect("valid sender builder") + .save(&InMemoryPersister::default()) + .expect("in-memory persister is infallible") + .into(); + + let ctx = sender.create_v2_post_request(EXAMPLE_URL.to_string()).expect("valid request"); + // An undersized body is a transient failure, so a caller may retry. + // Retrying with the same, now consumed, context must return an error. + sender.process_response(&[0u8; 1], &ctx.ohttp_ctx).expect("first use of the context"); + let reused = sender.process_response(&[0u8; 1], &ctx.ohttp_ctx); + assert!(matches!(reused, Err(ClientResponseError::AlreadyUsed))); + } }