From 1c101cd22ca8bde3e0eec2b66d995f9d115ef752 Mon Sep 17 00:00:00 2001 From: DanGould Date: Tue, 14 Jan 2025 18:23:15 -0500 Subject: [PATCH 1/8] Rename receive::Error variants based on source Describe the variants based on their source, not the response they're supposed to produce in the v1 protocol, which is what they were set to do before this change. --- payjoin-cli/src/app/v1.rs | 37 +++++++++++++++--------------- payjoin-cli/src/app/v2.rs | 20 ++++++++--------- payjoin/src/receive/error.rs | 42 ++++++++++++++++++++++------------- payjoin/src/receive/v1.rs | 12 +++++----- payjoin/src/receive/v2/mod.rs | 16 ++++++------- 5 files changed, 70 insertions(+), 57 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index c4b70af4e..ac53a31f6 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -224,7 +224,7 @@ impl App { .handle_payjoin_post(req) .await .map_err(|e| match e { - Error::BadRequest(e) => { + Error::Validation(e) => { log::error!("Error handling request: {}", e); Response::builder().status(400).body(full(e.to_string())).unwrap() } @@ -248,9 +248,9 @@ impl App { ) -> Result>, Error> { let address = self .bitcoind() - .map_err(|e| Error::Server(e.into()))? + .map_err(|e| Error::Implementation(e.into()))? .get_new_address(None, None) - .map_err(|e| Error::Server(e.into()))? + .map_err(|e| Error::Implementation(e.into()))? .assume_checked(); let uri_string = if let Some(amount) = amount { format!( @@ -263,7 +263,7 @@ impl App { format!("{}?pj={}", address.to_qr_uri(), self.config.pj_endpoint) }; let uri = Uri::try_from(uri_string.clone()) - .map_err(|_| Error::Server(anyhow!("Could not parse payjoin URI string.").into()))?; + .map_err(|_| Error::Implementation(anyhow!("Could not parse payjoin URI string.").into()))?; let _ = uri.assume_checked(); // we just got it from bitcoind above Ok(Response::new(full(uri_string))) @@ -276,7 +276,8 @@ impl App { let (parts, body) = req.into_parts(); let headers = Headers(&parts.headers); let query_string = parts.uri.query().unwrap_or(""); - let body = body.collect().await.map_err(|e| Error::Server(e.into()))?.aggregate().reader(); + let body = + body.collect().await.map_err(|e| Error::Implementation(e.into()))?.aggregate().reader(); let proposal = UncheckedProposal::from_request(body, query_string, headers)?; let payjoin_proposal = self.process_v1_proposal(proposal)?; @@ -290,22 +291,22 @@ impl App { } fn process_v1_proposal(&self, proposal: UncheckedProposal) -> Result { - let bitcoind = self.bitcoind().map_err(|e| Error::Server(e.into()))?; + let bitcoind = self.bitcoind().map_err(|e| Error::Implementation(e.into()))?; // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); // The network is used for checks later - let network = bitcoind.get_blockchain_info().map_err(|e| Error::Server(e.into()))?.chain; + let network = bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; // Receive Check 1: Can Broadcast let proposal = proposal.check_broadcast_suitability(None, |tx| { let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx); let mempool_results = - bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Server(e.into()))?; + bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Implementation(e.into()))?; match mempool_results.first() { Some(result) => Ok(result.allowed), - None => Err(Error::Server( + None => Err(Error::Implementation( anyhow!("No mempool results returned on broadcast check").into(), )), } @@ -318,7 +319,7 @@ impl App { bitcoind .get_address_info(&address) .map(|info| info.is_mine.unwrap_or(false)) - .map_err(|e| Error::Server(e.into())) + .map_err(|e| Error::Implementation(e.into())) } else { Ok(false) } @@ -327,7 +328,7 @@ impl App { // Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers. let payjoin = proposal.check_no_inputs_seen_before(|input| { - self.db.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into())) + self.db.insert_input_seen_before(*input).map_err(|e| Error::Implementation(e.into())) })?; log::trace!("check3"); @@ -336,7 +337,7 @@ impl App { bitcoind .get_address_info(&address) .map(|info| info.is_mine.unwrap_or(false)) - .map_err(|e| Error::Server(e.into())) + .map_err(|e| Error::Implementation(e.into())) } else { Ok(false) } @@ -346,12 +347,12 @@ impl App { .substitute_receiver_script( &bitcoind .get_new_address(None, None) - .map_err(|e| Error::Server(e.into()))? + .map_err(|e| Error::Implementation(e.into()))? .require_network(network) - .map_err(|e| Error::Server(e.into()))? + .map_err(|e| Error::Implementation(e.into()))? .script_pubkey(), ) - .map_err(|e| Error::Server(e.into()))? + .map_err(|e| Error::Implementation(e.into()))? .commit_outputs(); let provisional_payjoin = try_contributing_inputs(payjoin.clone(), &bitcoind) @@ -364,12 +365,12 @@ impl App { |psbt: &Psbt| { bitcoind .wallet_process_psbt(&psbt.to_string(), None, None, Some(false)) - .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Server(e.into()))) - .map_err(|e| Error::Server(e.into()))? + .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into()))) + .map_err(|e| Error::Implementation(e.into()))? }, Some(bitcoin::FeeRate::MIN), self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { - FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Server("Invalid fee rate".into())) + FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Implementation("Invalid fee rate".into())) })?, )?; Ok(payjoin_proposal) diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 6afd04525..ad9ce5224 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -251,21 +251,21 @@ impl App { &self, proposal: payjoin::receive::v2::UncheckedProposal, ) -> Result { - let bitcoind = self.bitcoind().map_err(|e| Error::Server(e.into()))?; + let bitcoind = self.bitcoind().map_err(|e| Error::Implementation(e.into()))?; // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); // The network is used for checks later - let network = bitcoind.get_blockchain_info().map_err(|e| Error::Server(e.into()))?.chain; + let network = bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; // Receive Check 1: Can Broadcast let proposal = proposal.check_broadcast_suitability(None, |tx| { let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx); let mempool_results = - bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Server(e.into()))?; + bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Implementation(e.into()))?; match mempool_results.first() { Some(result) => Ok(result.allowed), - None => Err(Error::Server( + None => Err(Error::Implementation( anyhow!("No mempool results returned on broadcast check").into(), )), } @@ -278,7 +278,7 @@ impl App { bitcoind .get_address_info(&address) .map(|info| info.is_mine.unwrap_or(false)) - .map_err(|e| Error::Server(e.into())) + .map_err(|e| Error::Implementation(e.into())) } else { Ok(false) } @@ -287,7 +287,7 @@ impl App { // Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers. let payjoin = proposal.check_no_inputs_seen_before(|input| { - self.db.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into())) + self.db.insert_input_seen_before(*input).map_err(|e| Error::Implementation(e.into())) })?; log::trace!("check3"); @@ -297,7 +297,7 @@ impl App { bitcoind .get_address_info(&address) .map(|info| info.is_mine.unwrap_or(false)) - .map_err(|e| Error::Server(e.into())) + .map_err(|e| Error::Implementation(e.into())) } else { Ok(false) } @@ -314,12 +314,12 @@ impl App { |psbt: &Psbt| { bitcoind .wallet_process_psbt(&psbt.to_string(), None, None, Some(false)) - .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Server(e.into()))) - .map_err(|e| Error::Server(e.into()))? + .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into()))) + .map_err(|e| Error::Implementation(e.into()))? }, Some(bitcoin::FeeRate::MIN), self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { - FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Server("Invalid fee rate".into())) + FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Implementation("Invalid fee rate".into())) })?, )?; let payjoin_proposal_psbt = payjoin_proposal.psbt(); diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 99a69dab8..5a6c900b5 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -1,20 +1,32 @@ use std::error; use std::fmt::{self, Display}; +/// The top-level error type for the payjoin receiver, representing all possible failures that can occur +/// during the processing of a payjoin request. +/// +/// The error handling is designed to: +/// 1. Provide structured error responses for protocol-level failures +/// 2. Hide implementation details of external errors for security +/// 3. Support proper error propagation through the receiver stack +/// 4. Provide errors according to BIP-78 JSON error specifications for return using [`Error::to_json`] #[derive(Debug)] pub enum Error { - /// To be returned as HTTP 400 - BadRequest(RequestError), - // To be returned as HTTP 500 - Server(Box), + /// Error arising from the payjoin state machine + /// + /// e.g. PSBT validation, HTTP request validation, protocol version checks + Validation(RequestError), + /// Error arising due to the specific receiver implementation + /// + /// e.g. database errors, network failures, wallet errors + Implementation(Box), } impl Error { pub fn to_json(&self) -> String { match self { - Self::BadRequest(e) => e.to_string(), - Self::Server(_) => - "{{ \"errorCode\": \"server-error\", \"message\": \"Internal server error\" }}" + Self::Validation(e) => e.to_string(), + Self::Implementation(_) => + "{{ \"errorCode\": \"unavailable\", \"message\": \"Receiver error\" }}" .to_string(), } } @@ -23,8 +35,8 @@ impl Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { - Self::BadRequest(e) => e.fmt(f), - Self::Server(e) => write!(f, "Internal Server Error: {}", e), + Self::Validation(e) => e.fmt(f), + Self::Implementation(e) => write!(f, "Internal Server Error: {}", e), } } } @@ -32,28 +44,28 @@ impl fmt::Display for Error { impl error::Error for Error { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match &self { - Self::BadRequest(_) => None, - Self::Server(e) => Some(e.as_ref()), + Self::Validation(_) => None, + Self::Implementation(e) => Some(e.as_ref()), } } } impl From for Error { - fn from(e: RequestError) -> Self { Error::BadRequest(e) } + fn from(e: RequestError) -> Self { Error::Validation(e) } } impl From for Error { - fn from(e: InternalRequestError) -> Self { Error::BadRequest(e.into()) } + fn from(e: InternalRequestError) -> Self { Error::Validation(e.into()) } } #[cfg(feature = "v2")] impl From for Error { - fn from(e: crate::hpke::HpkeError) -> Self { Error::Server(Box::new(e)) } + fn from(e: crate::hpke::HpkeError) -> Self { Error::Implementation(Box::new(e)) } } #[cfg(feature = "v2")] impl From for Error { - fn from(e: crate::ohttp::OhttpEncapsulationError) -> Self { Error::Server(Box::new(e)) } + fn from(e: crate::ohttp::OhttpEncapsulationError) -> Self { Error::Implementation(Box::new(e)) } } /// Error that may occur when the request from sender is malformed. diff --git a/payjoin/src/receive/v1.rs b/payjoin/src/receive/v1.rs index e2d3c94b6..535611ee0 100644 --- a/payjoin/src/receive/v1.rs +++ b/payjoin/src/receive/v1.rs @@ -197,15 +197,15 @@ impl MaybeInputsOwned { .scan(&mut err, |err, input| match input.previous_txout() { Ok(txout) => Some(txout.script_pubkey.to_owned()), Err(e) => { - **err = Err(Error::BadRequest(InternalRequestError::PrevTxOut(e).into())); + **err = Err(Error::Validation(InternalRequestError::PrevTxOut(e).into())); None } }) .find_map(|script| match is_owned(&script) { Ok(false) => None, Ok(true) => - Some(Error::BadRequest(InternalRequestError::InputOwned(script).into())), - Err(e) => Some(Error::Server(e.into())), + Some(Error::Validation(InternalRequestError::InputOwned(script).into())), + Err(e) => Some(Error::Implementation(e.into())), }) { return Err(e); @@ -237,11 +237,11 @@ impl MaybeInputsSeen { Ok(false) => Ok::<(), Error>(()), Ok(true) => { log::warn!("Request contains an input we've seen before: {}. Preventing possible probing attack.", input.txin.previous_output); - Err(Error::BadRequest( + Err(Error::Validation( InternalRequestError::InputSeen(input.txin.previous_output).into(), ))? }, - Err(e) => Err(Error::Server(e.into()))?, + Err(e) => Err(Error::Implementation(e.into()))?, } })?; @@ -279,7 +279,7 @@ impl OutputsUnknown { .collect::, _>>()?; if owned_vouts.is_empty() { - return Err(Error::BadRequest(InternalRequestError::MissingPayment.into())); + return Err(Error::Validation(InternalRequestError::MissingPayment.into())); } let mut params = self.params.clone(); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index eb4379c2a..da998620b 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -117,7 +117,7 @@ impl Receiver { ) -> Result, Error> { let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = body.try_into().map_err(|_| { - Error::Server(Box::new(SessionError::from( + Error::Implementation(Box::new(SessionError::from( InternalSessionError::UnexpectedResponseSize(body.len()), ))) })?; @@ -510,7 +510,7 @@ impl PayjoinProposal { .context .directory .join(&sender_subdir.to_string()) - .map_err(|e| Error::Server(e.into()))?; + .map_err(|e| Error::Implementation(e.into()))?; body = encrypt_message_b(payjoin_bytes, &self.context.s, e)?; method = "POST"; } else { @@ -521,7 +521,7 @@ impl PayjoinProposal { .context .directory .join(&receiver_subdir.to_string()) - .map_err(|e| Error::Server(e.into()))?; + .map_err(|e| Error::Implementation(e.into()))?; method = "PUT"; } log::debug!("Payjoin PSBT target: {}", target_resource.as_str()); @@ -550,7 +550,7 @@ impl PayjoinProposal { ) -> Result<(), Error> { let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = res.try_into().map_err(|_| { - Error::Server(Box::new(SessionError::from( + Error::Implementation(Box::new(SessionError::from( InternalSessionError::UnexpectedResponseSize(res.len()), ))) })?; @@ -558,7 +558,7 @@ impl PayjoinProposal { if res.status().is_success() { Ok(()) } else { - Err(Error::Server( + Err(Error::Implementation( format!("Payjoin Post failed, expected Success status, got {}", res.status()) .into(), )) @@ -623,16 +623,16 @@ mod test { let server_error = proposal .clone() - .check_broadcast_suitability(None, |_| Err(Error::Server("mock error".into()))) + .check_broadcast_suitability(None, |_| Err(Error::Implementation("mock error".into()))) .err() .unwrap(); assert_eq!( server_error.to_json(), - "{{ \"errorCode\": \"server-error\", \"message\": \"Internal server error\" }}" + "{{ \"errorCode\": \"unavailable\", \"message\": \"Receiver error\" }}" ); let (_req, _ctx) = proposal.clone().extract_err_req(&server_error, &EXAMPLE_OHTTP_RELAY)?; - let internal_error = Error::BadRequest(RequestError(InternalRequestError::MissingPayment)); + let internal_error = Error::Validation(RequestError(InternalRequestError::MissingPayment)); let (_req, _ctx) = proposal.extract_err_req(&internal_error, &EXAMPLE_OHTTP_RELAY)?; Ok(()) } From f6f1281e5f07c4811017f87df6d737c00b41893f Mon Sep 17 00:00:00 2001 From: DanGould Date: Wed, 8 Jan 2025 12:53:43 -0500 Subject: [PATCH 2/8] Separate receive::{v1,v2} error modules This is the big move, but it doesn't completely clean up the variants into clean module fault lines. It chunks the work into submodules but still has some leaky abstractions. The next commits will try to draw cleaner lines. --- payjoin-cli/src/app/v1.rs | 20 ++- payjoin-cli/src/app/v2.rs | 15 +- payjoin/src/receive/error.rs | 181 ++++++++++++----------- payjoin/src/receive/mod.rs | 7 +- payjoin/src/receive/v1/error.rs | 81 ++++++++++ payjoin/src/receive/{v1.rs => v1/mod.rs} | 49 +++--- payjoin/src/receive/v2/error.rs | 61 ++++++++ payjoin/src/receive/v2/mod.rs | 24 ++- 8 files changed, 296 insertions(+), 142 deletions(-) create mode 100644 payjoin/src/receive/v1/error.rs rename payjoin/src/receive/{v1.rs => v1/mod.rs} (96%) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index ac53a31f6..150241184 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -262,8 +262,9 @@ impl App { } else { format!("{}?pj={}", address.to_qr_uri(), self.config.pj_endpoint) }; - let uri = Uri::try_from(uri_string.clone()) - .map_err(|_| Error::Implementation(anyhow!("Could not parse payjoin URI string.").into()))?; + let uri = Uri::try_from(uri_string.clone()).map_err(|_| { + Error::Implementation(anyhow!("Could not parse payjoin URI string.").into()) + })?; let _ = uri.assume_checked(); // we just got it from bitcoind above Ok(Response::new(full(uri_string))) @@ -297,13 +298,15 @@ impl App { let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); // The network is used for checks later - let network = bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; + let network = + bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; // Receive Check 1: Can Broadcast let proposal = proposal.check_broadcast_suitability(None, |tx| { let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx); - let mempool_results = - bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Implementation(e.into()))?; + let mempool_results = bitcoind + .test_mempool_accept(&[raw_tx]) + .map_err(|e| Error::Implementation(e.into()))?; match mempool_results.first() { Some(result) => Ok(result.allowed), None => Err(Error::Implementation( @@ -365,12 +368,15 @@ impl App { |psbt: &Psbt| { bitcoind .wallet_process_psbt(&psbt.to_string(), None, None, Some(false)) - .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into()))) + .map(|res| { + Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into())) + }) .map_err(|e| Error::Implementation(e.into()))? }, Some(bitcoin::FeeRate::MIN), self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { - FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Implementation("Invalid fee rate".into())) + FeeRate::from_sat_per_vb(fee_rate) + .ok_or(Error::Implementation("Invalid fee rate".into())) })?, )?; Ok(payjoin_proposal) diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index ad9ce5224..b3590f2ec 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -257,12 +257,14 @@ impl App { let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); // The network is used for checks later - let network = bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; + let network = + bitcoind.get_blockchain_info().map_err(|e| Error::Implementation(e.into()))?.chain; // Receive Check 1: Can Broadcast let proposal = proposal.check_broadcast_suitability(None, |tx| { let raw_tx = bitcoin::consensus::encode::serialize_hex(&tx); - let mempool_results = - bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Implementation(e.into()))?; + let mempool_results = bitcoind + .test_mempool_accept(&[raw_tx]) + .map_err(|e| Error::Implementation(e.into()))?; match mempool_results.first() { Some(result) => Ok(result.allowed), None => Err(Error::Implementation( @@ -314,12 +316,15 @@ impl App { |psbt: &Psbt| { bitcoind .wallet_process_psbt(&psbt.to_string(), None, None, Some(false)) - .map(|res| Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into()))) + .map(|res| { + Psbt::from_str(&res.psbt).map_err(|e| Error::Implementation(e.into())) + }) .map_err(|e| Error::Implementation(e.into()))? }, Some(bitcoin::FeeRate::MIN), self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { - FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Implementation("Invalid fee rate".into())) + FeeRate::from_sat_per_vb(fee_rate) + .ok_or(Error::Implementation("Invalid fee rate".into())) })?, )?; let payjoin_proposal_psbt = payjoin_proposal.psbt(); diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 5a6c900b5..6371c796b 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -1,5 +1,8 @@ -use std::error; -use std::fmt::{self, Display}; +use std::{error, fmt}; + +use crate::receive::v1; +#[cfg(feature = "v2")] +use crate::receive::v2; /// The top-level error type for the payjoin receiver, representing all possible failures that can occur /// during the processing of a payjoin request. @@ -14,7 +17,7 @@ pub enum Error { /// Error arising from the payjoin state machine /// /// e.g. PSBT validation, HTTP request validation, protocol version checks - Validation(RequestError), + Validation(ValidationError), /// Error arising due to the specific receiver implementation /// /// e.g. database errors, network failures, wallet errors @@ -26,8 +29,7 @@ impl Error { match self { Self::Validation(e) => e.to_string(), Self::Implementation(_) => - "{{ \"errorCode\": \"unavailable\", \"message\": \"Receiver error\" }}" - .to_string(), + "{{ \"errorCode\": \"unavailable\", \"message\": \"Receiver error\" }}".to_string(), } } } @@ -44,46 +46,82 @@ impl fmt::Display for Error { impl error::Error for Error { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match &self { - Self::Validation(_) => None, + Self::Validation(e) => e.source(), Self::Implementation(e) => Some(e.as_ref()), } } } -impl From for Error { - fn from(e: RequestError) -> Self { Error::Validation(e) } +impl From for Error { + fn from(e: InternalPayloadError) -> Self { + Error::Validation(ValidationError::Payload(e.into())) + } } -impl From for Error { - fn from(e: InternalRequestError) -> Self { Error::Validation(e.into()) } +impl From for Error { + fn from(e: v1::InternalRequestError) -> Self { Error::Validation(e.into()) } } -#[cfg(feature = "v2")] -impl From for Error { - fn from(e: crate::hpke::HpkeError) -> Self { Error::Implementation(Box::new(e)) } +/// An error that occurs during validation of a payjoin request, encompassing all possible validation +/// failures across different protocol versions and stages. +/// +/// This abstraction serves as the primary error type for the validation phase of request processing, +/// allowing uniform error handling while maintaining protocol version specifics internally. +#[derive(Debug)] +pub enum ValidationError { + /// Error arising from validation of the original PSBT payload + Payload(PayloadError), + /// Protocol-specific errors for BIP-78 v1 requests (e.g. HTTP request validation, parameter checks) + V1(v1::RequestError), + /// Protocol-specific errors for BIP-77 v2 sessions (e.g. session management, OHTTP, HPKE encryption) + #[cfg(feature = "v2")] + V2(v2::RequestError), +} + +impl From for ValidationError { + fn from(e: InternalPayloadError) -> Self { ValidationError::Payload(e.into()) } +} + +impl From for ValidationError { + fn from(e: v1::InternalRequestError) -> Self { ValidationError::V1(e.into()) } } #[cfg(feature = "v2")] -impl From for Error { - fn from(e: crate::ohttp::OhttpEncapsulationError) -> Self { Error::Implementation(Box::new(e)) } +impl From for ValidationError { + fn from(e: v2::InternalRequestError) -> Self { ValidationError::V2(e.into()) } +} + +impl fmt::Display for ValidationError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + ValidationError::Payload(e) => write!(f, "{}", e), + ValidationError::V1(e) => write!(f, "{}", e), + #[cfg(feature = "v2")] + ValidationError::V2(e) => write!(f, "{}", e), + } + } +} + +impl std::error::Error for ValidationError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ValidationError::Payload(e) => Some(e), + ValidationError::V1(e) => Some(e), + #[cfg(feature = "v2")] + ValidationError::V2(e) => Some(e), + } + } } -/// Error that may occur when the request from sender is malformed. -/// -/// This is currently opaque type because we aren't sure which variants will stay. -/// You can only display it. #[derive(Debug)] -pub struct RequestError(pub(crate) InternalRequestError); +pub struct PayloadError(pub(crate) InternalPayloadError); + +impl From for PayloadError { + fn from(value: InternalPayloadError) -> Self { PayloadError(value) } +} #[derive(Debug)] -pub(crate) enum InternalRequestError { - Psbt(bitcoin::psbt::Error), - Base64(bitcoin::base64::DecodeError), - Io(std::io::Error), - MissingHeader(&'static str), - InvalidContentType(String), - InvalidContentLength(std::num::ParseIntError), - ContentLengthTooLarge(u64), +pub(crate) enum InternalPayloadError { SenderParams(super::optional_parameters::Error), /// The raw PSBT fails bip78-specific validation. InconsistentPsbt(crate::psbt::InconsistentPsbt), @@ -102,11 +140,6 @@ pub(crate) enum InternalRequestError { /// Original PSBT input has been seen before. Only automatic receivers, aka "interactive" in the spec /// look out for these to prevent probing attacks. InputSeen(bitcoin::OutPoint), - /// Serde deserialization failed - #[cfg(feature = "v2")] - ParsePsbt(bitcoin::psbt::PsbtParseError), - #[cfg(feature = "v2")] - Utf8(std::string::FromUtf8Error), /// Original PSBT fee rate is below minimum fee rate set by the receiver. /// /// First argument is the calculated fee rate of the original PSBT. @@ -117,35 +150,20 @@ pub(crate) enum InternalRequestError { FeeTooHigh(bitcoin::FeeRate, bitcoin::FeeRate), } -impl From for RequestError { - fn from(value: InternalRequestError) -> Self { RequestError(value) } -} - -impl fmt::Display for RequestError { +impl fmt::Display for PayloadError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fn write_error(f: &mut fmt::Formatter, code: &str, message: impl Display) -> fmt::Result { + use InternalPayloadError::*; + + fn write_error( + f: &mut fmt::Formatter, + code: &str, + message: impl fmt::Display, + ) -> fmt::Result { write!(f, r#"{{ "errorCode": "{}", "message": "{}" }}"#, code, message) } match &self.0 { - InternalRequestError::Psbt(e) => write_error(f, "psbt-error", e), - InternalRequestError::Base64(e) => write_error(f, "base64-decode-error", e), - InternalRequestError::Io(e) => write_error(f, "io-error", e), - InternalRequestError::MissingHeader(header) => - write_error(f, "missing-header", format!("Missing header: {}", header)), - InternalRequestError::InvalidContentType(content_type) => write_error( - f, - "invalid-content-type", - format!("Invalid content type: {}", content_type), - ), - InternalRequestError::InvalidContentLength(e) => - write_error(f, "invalid-content-length", e), - InternalRequestError::ContentLengthTooLarge(length) => write_error( - f, - "content-length-too-large", - format!("Content length too large: {}.", length), - ), - InternalRequestError::SenderParams(e) => match e { + SenderParams(e) => match e { super::optional_parameters::Error::UnknownVersion { supported_versions } => { write!( f, @@ -159,31 +177,22 @@ impl fmt::Display for RequestError { } _ => write_error(f, "sender-params-error", e), }, - InternalRequestError::InconsistentPsbt(e) => - write_error(f, "original-psbt-rejected", e), - InternalRequestError::PrevTxOut(e) => + InconsistentPsbt(e) => write_error(f, "original-psbt-rejected", e), + PrevTxOut(e) => write_error(f, "original-psbt-rejected", format!("PrevTxOut Error: {}", e)), - InternalRequestError::MissingPayment => - write_error(f, "original-psbt-rejected", "Missing payment."), - InternalRequestError::OriginalPsbtNotBroadcastable => write_error( + MissingPayment => write_error(f, "original-psbt-rejected", "Missing payment."), + OriginalPsbtNotBroadcastable => write_error( f, "original-psbt-rejected", "Can't broadcast. PSBT rejected by mempool.", ), - InternalRequestError::InputOwned(_) => + InputOwned(_) => write_error(f, "original-psbt-rejected", "The receiver rejected the original PSBT."), - InternalRequestError::InputWeight(e) => + InputWeight(e) => write_error(f, "original-psbt-rejected", format!("InputWeight Error: {}", e)), - InternalRequestError::InputSeen(_) => + InputSeen(_) => write_error(f, "original-psbt-rejected", "The receiver rejected the original PSBT."), - #[cfg(feature = "v2")] - InternalRequestError::ParsePsbt(e) => write_error(f, "Error parsing PSBT:", e), - #[cfg(feature = "v2")] - InternalRequestError::Utf8(e) => write_error(f, "Error parsing PSBT:", e), - InternalRequestError::PsbtBelowFeeRate( - original_psbt_fee_rate, - receiver_min_fee_rate, - ) => write_error( + PsbtBelowFeeRate(original_psbt_fee_rate, receiver_min_fee_rate) => write_error( f, "original-psbt-rejected", format!( @@ -191,7 +200,7 @@ impl fmt::Display for RequestError { original_psbt_fee_rate, receiver_min_fee_rate ), ), - InternalRequestError::FeeTooHigh(proposed_feerate, max_feerate) => write_error( + FeeTooHigh(proposed_feerate, max_feerate) => write_error( f, "original-psbt-rejected", format!( @@ -203,22 +212,16 @@ impl fmt::Display for RequestError { } } -impl std::error::Error for RequestError { +impl std::error::Error for PayloadError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use InternalPayloadError::*; match &self.0 { - InternalRequestError::Psbt(e) => Some(e), - InternalRequestError::Base64(e) => Some(e), - InternalRequestError::Io(e) => Some(e), - InternalRequestError::InvalidContentLength(e) => Some(e), - InternalRequestError::SenderParams(e) => Some(e), - InternalRequestError::InconsistentPsbt(e) => Some(e), - InternalRequestError::PrevTxOut(e) => Some(e), - InternalRequestError::InputWeight(e) => Some(e), - #[cfg(feature = "v2")] - InternalRequestError::ParsePsbt(e) => Some(e), - #[cfg(feature = "v2")] - InternalRequestError::Utf8(e) => Some(e), - InternalRequestError::PsbtBelowFeeRate(_, _) => None, + SenderParams(e) => Some(e), + InconsistentPsbt(e) => Some(e), + PrevTxOut(e) => Some(e), + InputWeight(e) => Some(e), + PsbtBelowFeeRate(_, _) => None, + FeeTooHigh(_, _) => None, _ => None, } } diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index b8154b1b8..24f41da26 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -1,13 +1,12 @@ use bitcoin::{psbt, AddressType, TxIn, TxOut}; -pub use error::{ - Error, InputContributionError, OutputSubstitutionError, RequestError, SelectionError, -}; +pub(crate) use error::InternalPayloadError; +pub use error::{Error, OutputSubstitutionError, PayloadError, SelectionError}; pub use crate::psbt::PsbtInputError; use crate::psbt::{InternalInputPair, InternalPsbtInputError}; mod error; -mod optional_parameters; +pub(crate) mod optional_parameters; pub mod v1; #[cfg(feature = "v2")] pub mod v2; diff --git a/payjoin/src/receive/v1/error.rs b/payjoin/src/receive/v1/error.rs new file mode 100644 index 000000000..e9d9350e2 --- /dev/null +++ b/payjoin/src/receive/v1/error.rs @@ -0,0 +1,81 @@ +use core::fmt; +use std::error; + +/// Error that occurs during validation of an incoming v1 payjoin request. +/// +/// This type provides a stable public API for v1 request validation errors while keeping internal +/// error variants private. It handles validation of: +/// - PSBT parsing and validation +/// - I/O operations during request processing +/// - HTTP headers (Content-Type, Content-Length) +/// +/// The error messages are formatted as JSON strings according to the BIP-78 spec with appropriate +/// error codes and human-readable messages. +#[derive(Debug)] +pub struct RequestError(InternalRequestError); + +#[derive(Debug)] +pub(crate) enum InternalRequestError { + /// Error parsing or validating the PSBT + Psbt(bitcoin::psbt::Error), + /// Error decoding the base64 encoded PSBT + Base64(bitcoin::base64::DecodeError), + /// I/O error while reading the request body + Io(std::io::Error), + /// A required HTTP header is missing from the request + MissingHeader(&'static str), + /// The Content-Type header has an invalid value + InvalidContentType(String), + /// The Content-Length header could not be parsed as a number + InvalidContentLength(std::num::ParseIntError), + /// The Content-Length value exceeds the maximum allowed size + ContentLengthTooLarge(u64), +} + +impl From for RequestError { + fn from(value: InternalRequestError) -> Self { RequestError(value) } +} + +impl fmt::Display for RequestError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fn write_error( + f: &mut fmt::Formatter, + code: &str, + message: impl fmt::Display, + ) -> fmt::Result { + write!(f, r#"{{ "errorCode": "{}", "message": "{}" }}"#, code, message) + } + + match &self.0 { + InternalRequestError::Psbt(e) => write_error(f, "psbt-error", e), + InternalRequestError::Base64(e) => write_error(f, "base64-decode-error", e), + InternalRequestError::Io(e) => write_error(f, "io-error", e), + InternalRequestError::MissingHeader(header) => + write_error(f, "missing-header", format!("Missing header: {}", header)), + InternalRequestError::InvalidContentType(content_type) => write_error( + f, + "invalid-content-type", + format!("Invalid content type: {}", content_type), + ), + InternalRequestError::InvalidContentLength(e) => + write_error(f, "invalid-content-length", e), + InternalRequestError::ContentLengthTooLarge(length) => write_error( + f, + "content-length-too-large", + format!("Content length too large: {}.", length), + ), + } + } +} + +impl error::Error for RequestError { + fn source(&self) -> Option<&(dyn error::Error + 'static)> { + match &self.0 { + InternalRequestError::Psbt(e) => Some(e), + InternalRequestError::Base64(e) => Some(e), + InternalRequestError::Io(e) => Some(e), + InternalRequestError::InvalidContentLength(e) => Some(e), + _ => None, + } + } +} diff --git a/payjoin/src/receive/v1.rs b/payjoin/src/receive/v1/mod.rs similarity index 96% rename from payjoin/src/receive/v1.rs rename to payjoin/src/receive/v1/mod.rs index 535611ee0..3c2c9fc96 100644 --- a/payjoin/src/receive/v1.rs +++ b/payjoin/src/receive/v1/mod.rs @@ -32,16 +32,19 @@ use bitcoin::psbt::Psbt; use bitcoin::secp256k1::rand::seq::SliceRandom; use bitcoin::secp256k1::rand::{self, Rng}; use bitcoin::{Amount, FeeRate, OutPoint, Script, TxIn, TxOut, Weight}; +pub(crate) use error::InternalRequestError; +pub use error::RequestError; use super::error::{ - InternalInputContributionError, InternalOutputSubstitutionError, InternalRequestError, + InputContributionError, InternalInputContributionError, InternalOutputSubstitutionError, InternalSelectionError, }; use super::optional_parameters::Params; -use super::{ - Error, InputContributionError, InputPair, OutputSubstitutionError, RequestError, SelectionError, -}; +use super::{Error, InputPair, OutputSubstitutionError, SelectionError}; use crate::psbt::PsbtExt; +use crate::receive::InternalPayloadError; + +mod error; const SUPPORTED_VERSIONS: &[usize] = &[1]; @@ -79,7 +82,7 @@ impl UncheckedProposal { mut body: impl std::io::Read, query: &str, headers: impl Headers, - ) -> Result { + ) -> Result { let content_type = headers .get_header("content-type") .ok_or(InternalRequestError::MissingHeader("Content-Type"))?; @@ -102,12 +105,12 @@ impl UncheckedProposal { let base64 = BASE64_STANDARD.decode(&buf).map_err(InternalRequestError::Base64)?; let unchecked_psbt = Psbt::deserialize(&base64).map_err(InternalRequestError::Psbt)?; - let psbt = unchecked_psbt.validate().map_err(InternalRequestError::InconsistentPsbt)?; + let psbt = unchecked_psbt.validate().map_err(InternalPayloadError::InconsistentPsbt)?; log::debug!("Received original psbt: {:?}", psbt); let pairs = url::form_urlencoded::parse(query.as_bytes()); let params = Params::from_query_pairs(pairs, SUPPORTED_VERSIONS) - .map_err(InternalRequestError::SenderParams)?; + .map_err(InternalPayloadError::SenderParams)?; log::debug!("Received request with params: {:?}", params); // TODO check that params are valid for the request's Original PSBT @@ -148,7 +151,7 @@ impl UncheckedProposal { let original_psbt_fee_rate = self.psbt_fee_rate()?; if let Some(min_fee_rate) = min_fee_rate { if original_psbt_fee_rate < min_fee_rate { - return Err(InternalRequestError::PsbtBelowFeeRate( + return Err(InternalPayloadError::PsbtBelowFeeRate( original_psbt_fee_rate, min_fee_rate, ) @@ -158,7 +161,7 @@ impl UncheckedProposal { if can_broadcast(&self.psbt.clone().extract_tx_unchecked_fee_rate())? { Ok(MaybeInputsOwned { psbt: self.psbt, params: self.params }) } else { - Err(InternalRequestError::OriginalPsbtNotBroadcastable.into()) + Err(InternalPayloadError::OriginalPsbtNotBroadcastable.into()) } } @@ -197,14 +200,14 @@ impl MaybeInputsOwned { .scan(&mut err, |err, input| match input.previous_txout() { Ok(txout) => Some(txout.script_pubkey.to_owned()), Err(e) => { - **err = Err(Error::Validation(InternalRequestError::PrevTxOut(e).into())); + **err = Err(Error::Validation(InternalPayloadError::PrevTxOut(e).into())); None } }) .find_map(|script| match is_owned(&script) { Ok(false) => None, Ok(true) => - Some(Error::Validation(InternalRequestError::InputOwned(script).into())), + Some(Error::Validation(InternalPayloadError::InputOwned(script).into())), Err(e) => Some(Error::Implementation(e.into())), }) { @@ -238,7 +241,7 @@ impl MaybeInputsSeen { Ok(true) => { log::warn!("Request contains an input we've seen before: {}. Preventing possible probing attack.", input.txin.previous_output); Err(Error::Validation( - InternalRequestError::InputSeen(input.txin.previous_output).into(), + InternalPayloadError::InputSeen(input.txin.previous_output).into(), ))? }, Err(e) => Err(Error::Implementation(e.into()))?, @@ -279,7 +282,7 @@ impl OutputsUnknown { .collect::, _>>()?; if owned_vouts.is_empty() { - return Err(Error::Validation(InternalRequestError::MissingPayment.into())); + return Err(Error::Validation(InternalPayloadError::MissingPayment.into())); } let mut params = self.params.clone(); @@ -677,7 +680,7 @@ impl ProvisionalProposal { &mut self, min_feerate: Option, max_feerate: FeeRate, - ) -> Result<&Psbt, RequestError> { + ) -> Result<&Psbt, InternalPayloadError> { let min_feerate = min_feerate.unwrap_or(FeeRate::MIN); log::trace!("min_feerate: {:?}", min_feerate); log::trace!("params.min_feerate: {:?}", self.params.min_feerate); @@ -734,7 +737,7 @@ impl ProvisionalProposal { if receiver_additional_fee > max_fee { let proposed_feerate = receiver_additional_fee / (input_contribution_weight + output_contribution_weight); - return Err(InternalRequestError::FeeTooHigh(proposed_feerate, max_feerate).into()); + return Err(InternalPayloadError::FeeTooHigh(proposed_feerate, max_feerate)); } if receiver_additional_fee > Amount::ZERO { // Remove additional miner fee from the receiver's specified output @@ -744,14 +747,14 @@ impl ProvisionalProposal { } /// Calculate the additional input weight contributed by the receiver - fn additional_input_weight(&self) -> Result { - fn inputs_weight(psbt: &Psbt) -> Result { + fn additional_input_weight(&self) -> Result { + fn inputs_weight(psbt: &Psbt) -> Result { psbt.input_pairs().try_fold( Weight::ZERO, - |acc, input_pair| -> Result { + |acc, input_pair| -> Result { let input_weight = input_pair .expected_input_weight() - .map_err(InternalRequestError::InputWeight)?; + .map_err(InternalPayloadError::InputWeight)?; Ok(acc + input_weight) }, ) @@ -783,7 +786,7 @@ impl ProvisionalProposal { } /// Prepare the PSBT by clearing the fields that the sender expects to be empty - fn prepare_psbt(mut self, processed_psbt: Psbt) -> Result { + fn prepare_psbt(mut self, processed_psbt: Psbt) -> PayjoinProposal { self.payjoin_psbt = processed_psbt; log::trace!("Preparing PSBT {:#?}", self.payjoin_psbt); for output in self.payjoin_psbt.outputs_mut() { @@ -806,7 +809,7 @@ impl ProvisionalProposal { self.payjoin_psbt.inputs[i].tap_key_sig = None; } - Ok(PayjoinProposal { payjoin_psbt: self.payjoin_psbt, params: self.params }) + PayjoinProposal { payjoin_psbt: self.payjoin_psbt, params: self.params } } /// Return the indexes of the sender inputs @@ -851,7 +854,7 @@ impl ProvisionalProposal { psbt.inputs[i].tap_key_sig = None; } let psbt = wallet_process_psbt(&psbt)?; - let payjoin_proposal = self.prepare_psbt(psbt)?; + let payjoin_proposal = self.prepare_psbt(psbt); Ok(payjoin_proposal) } } @@ -904,7 +907,7 @@ pub(crate) mod test { } } - pub fn proposal_from_test_vector() -> Result { + pub(crate) fn proposal_from_test_vector() -> Result { // OriginalPSBT Test Vector from BIP // | InputScriptType | Orginal PSBT Fee rate | maxadditionalfeecontribution | additionalfeeoutputindex| // |-----------------|-----------------------|------------------------------|-------------------------| diff --git a/payjoin/src/receive/v2/error.rs b/payjoin/src/receive/v2/error.rs index 858e143dc..9505c9817 100644 --- a/payjoin/src/receive/v2/error.rs +++ b/payjoin/src/receive/v2/error.rs @@ -3,6 +3,57 @@ use std::error; use crate::ohttp::OhttpEncapsulationError; +/// Error that may occur when the v2 request from sender is malformed. +/// +/// This is currently opaque type because we aren't sure which variants will stay. +/// You can only display it. +#[derive(Debug)] +pub struct RequestError(InternalRequestError); + +#[derive(Debug)] +pub(crate) enum InternalRequestError { + /// Serde deserialization failed + ParsePsbt(bitcoin::psbt::PsbtParseError), + Utf8(std::string::FromUtf8Error), +} + +impl From for RequestError { + fn from(value: InternalRequestError) -> Self { RequestError(value) } +} + +impl fmt::Display for RequestError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use InternalRequestError::*; + fn write_error( + f: &mut fmt::Formatter, + code: &str, + message: impl fmt::Display, + ) -> fmt::Result { + write!(f, r#"{{ "errorCode": "{}", "message": "{}" }}"#, code, message) + } + + match &self.0 { + ParsePsbt(e) => write_error(f, "Error parsing PSBT:", e), + Utf8(e) => write_error(f, "Error parsing PSBT:", e), + } + } +} + +impl std::error::Error for RequestError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use InternalRequestError::*; + + match &self.0 { + ParsePsbt(e) => Some(e), + Utf8(e) => Some(e), + } + } +} + +impl From for super::Error { + fn from(e: InternalRequestError) -> Self { super::Error::Validation(e.into()) } +} + #[derive(Debug)] pub struct SessionError(InternalSessionError); @@ -56,3 +107,13 @@ impl From for SessionError { SessionError(InternalSessionError::OhttpEncapsulation(e)) } } + +impl From for super::Error { + fn from(e: crate::hpke::HpkeError) -> Self { super::Error::Implementation(Box::new(e)) } +} + +impl From for super::Error { + fn from(e: crate::ohttp::OhttpEncapsulationError) -> Self { + super::Error::Implementation(Box::new(e)) + } +} diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index da998620b..7acb2539e 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -4,15 +4,14 @@ use std::time::{Duration, SystemTime}; use bitcoin::hashes::{sha256, Hash}; use bitcoin::psbt::Psbt; use bitcoin::{Address, FeeRate, OutPoint, Script, TxOut}; +pub(crate) use error::{InternalRequestError, InternalSessionError}; +pub use error::{RequestError, SessionError}; use serde::de::Deserializer; use serde::{Deserialize, Serialize}; use url::Url; -use super::error::InternalRequestError; -use super::v2::error::{InternalSessionError, SessionError}; -use super::{ - v1, Error, InputContributionError, OutputSubstitutionError, RequestError, SelectionError, -}; +use super::error::InputContributionError; +use super::{v1, Error, InternalPayloadError, OutputSubstitutionError, SelectionError}; use crate::hpke::{decrypt_message_a, encrypt_message_b, HpkeKeyPair, HpkePublicKey}; use crate::ohttp::{ohttp_decapsulate, ohttp_encapsulate, OhttpEncapsulationError, OhttpKeys}; use crate::psbt::PsbtExt; @@ -146,31 +145,28 @@ impl Receiver { } fn extract_proposal_from_v1(&mut self, response: String) -> Result { - Ok(self.unchecked_from_payload(response)?) + self.unchecked_from_payload(response) } fn extract_proposal_from_v2(&mut self, response: Vec) -> Result { let (payload_bytes, e) = decrypt_message_a(&response, self.context.s.secret_key().clone())?; self.context.e = Some(e); let payload = String::from_utf8(payload_bytes).map_err(InternalRequestError::Utf8)?; - Ok(self.unchecked_from_payload(payload)?) + self.unchecked_from_payload(payload) } - fn unchecked_from_payload( - &mut self, - payload: String, - ) -> Result { + fn unchecked_from_payload(&mut self, payload: String) -> Result { let (base64, padded_query) = payload.split_once('\n').unwrap_or_default(); let query = padded_query.trim_matches('\0'); log::trace!("Received query: {}, base64: {}", query, base64); // my guess is no \n so default is wrong let unchecked_psbt = Psbt::from_str(base64).map_err(InternalRequestError::ParsePsbt)?; - let psbt = unchecked_psbt.validate().map_err(InternalRequestError::InconsistentPsbt)?; + let psbt = unchecked_psbt.validate().map_err(InternalPayloadError::InconsistentPsbt)?; log::debug!("Received original psbt: {:?}", psbt); let mut params = Params::from_query_pairs( url::form_urlencoded::parse(query.as_bytes()), SUPPORTED_VERSIONS, ) - .map_err(InternalRequestError::SenderParams)?; + .map_err(InternalPayloadError::SenderParams)?; // Output substitution must be disabled for V1 sessions in V2 contexts. // @@ -632,7 +628,7 @@ mod test { ); let (_req, _ctx) = proposal.clone().extract_err_req(&server_error, &EXAMPLE_OHTTP_RELAY)?; - let internal_error = Error::Validation(RequestError(InternalRequestError::MissingPayment)); + let internal_error = Error::Validation(InternalPayloadError::MissingPayment.into()); let (_req, _ctx) = proposal.extract_err_req(&internal_error, &EXAMPLE_OHTTP_RELAY)?; Ok(()) } From bc3d5df6e7c3d6191c4189e7b4f7f5849f5b795e Mon Sep 17 00:00:00 2001 From: DanGould Date: Tue, 14 Jan 2025 14:17:40 -0500 Subject: [PATCH 3/8] Unify PayloadError::{ParsePsbt,Base64,Utf8} var'ts Unify error variants produced by both v1 and v2 state machines but handled in different paths. Deduplicate the code to handle them together. --- payjoin/src/receive/error.rs | 22 ++++++++++++++++++++++ payjoin/src/receive/v1/error.rs | 4 ---- payjoin/src/receive/v1/mod.rs | 7 +++---- payjoin/src/receive/v2/mod.rs | 4 ++-- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 6371c796b..1592dd777 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -113,6 +113,19 @@ impl std::error::Error for ValidationError { } } +/// An error that occurs during validation of the original PSBT payload sent by the sender. +/// +/// This type provides a public abstraction over internal validation errors while maintaining a stable public API. +/// It handles various failure modes like: +/// - Invalid UTF-8 encoding +/// - PSBT parsing errors +/// - BIP-78 specific PSBT validation failures +/// - Fee rate validation +/// - Input ownership validation +/// - Previous transaction output validation +/// +/// The error messages are formatted as JSON strings suitable for HTTP responses according to the BIP-78 spec, +/// with appropriate error codes and human-readable messages. #[derive(Debug)] pub struct PayloadError(pub(crate) InternalPayloadError); @@ -122,6 +135,11 @@ impl From for PayloadError { #[derive(Debug)] pub(crate) enum InternalPayloadError { + /// The payload is not valid utf-8 + Utf8(std::string::FromUtf8Error), + /// The payload is not a valid PSBT + ParsePsbt(bitcoin::psbt::PsbtParseError), + /// Invalid sender parameters SenderParams(super::optional_parameters::Error), /// The raw PSBT fails bip78-specific validation. InconsistentPsbt(crate::psbt::InconsistentPsbt), @@ -163,6 +181,8 @@ impl fmt::Display for PayloadError { } match &self.0 { + Utf8(e) => write_error(f, "original-psbt-rejected", e), + ParsePsbt(e) => write_error(f, "original-psbt-rejected", e), SenderParams(e) => match e { super::optional_parameters::Error::UnknownVersion { supported_versions } => { write!( @@ -216,6 +236,8 @@ impl std::error::Error for PayloadError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { use InternalPayloadError::*; match &self.0 { + Utf8(e) => Some(e), + ParsePsbt(e) => Some(e), SenderParams(e) => Some(e), InconsistentPsbt(e) => Some(e), PrevTxOut(e) => Some(e), diff --git a/payjoin/src/receive/v1/error.rs b/payjoin/src/receive/v1/error.rs index e9d9350e2..0e03ff355 100644 --- a/payjoin/src/receive/v1/error.rs +++ b/payjoin/src/receive/v1/error.rs @@ -18,8 +18,6 @@ pub struct RequestError(InternalRequestError); pub(crate) enum InternalRequestError { /// Error parsing or validating the PSBT Psbt(bitcoin::psbt::Error), - /// Error decoding the base64 encoded PSBT - Base64(bitcoin::base64::DecodeError), /// I/O error while reading the request body Io(std::io::Error), /// A required HTTP header is missing from the request @@ -48,7 +46,6 @@ impl fmt::Display for RequestError { match &self.0 { InternalRequestError::Psbt(e) => write_error(f, "psbt-error", e), - InternalRequestError::Base64(e) => write_error(f, "base64-decode-error", e), InternalRequestError::Io(e) => write_error(f, "io-error", e), InternalRequestError::MissingHeader(header) => write_error(f, "missing-header", format!("Missing header: {}", header)), @@ -72,7 +69,6 @@ impl error::Error for RequestError { fn source(&self) -> Option<&(dyn error::Error + 'static)> { match &self.0 { InternalRequestError::Psbt(e) => Some(e), - InternalRequestError::Base64(e) => Some(e), InternalRequestError::Io(e) => Some(e), InternalRequestError::InvalidContentLength(e) => Some(e), _ => None, diff --git a/payjoin/src/receive/v1/mod.rs b/payjoin/src/receive/v1/mod.rs index 3c2c9fc96..afae60312 100644 --- a/payjoin/src/receive/v1/mod.rs +++ b/payjoin/src/receive/v1/mod.rs @@ -25,9 +25,8 @@ //! [reference implementation](https://github.com/payjoin/rust-payjoin/tree/master/payjoin-cli) use std::cmp::{max, min}; +use std::str::FromStr; -use bitcoin::base64::prelude::BASE64_STANDARD; -use bitcoin::base64::Engine; use bitcoin::psbt::Psbt; use bitcoin::secp256k1::rand::seq::SliceRandom; use bitcoin::secp256k1::rand::{self, Rng}; @@ -102,8 +101,8 @@ impl UncheckedProposal { // enforce the limit let mut buf = vec![0; content_length as usize]; // 4_000_000 * 4 / 3 fits in u32 body.read_exact(&mut buf).map_err(InternalRequestError::Io)?; - let base64 = BASE64_STANDARD.decode(&buf).map_err(InternalRequestError::Base64)?; - let unchecked_psbt = Psbt::deserialize(&base64).map_err(InternalRequestError::Psbt)?; + let base64 = String::from_utf8(buf).map_err(InternalPayloadError::Utf8)?; + let unchecked_psbt = Psbt::from_str(&base64).map_err(InternalPayloadError::ParsePsbt)?; let psbt = unchecked_psbt.validate().map_err(InternalPayloadError::InconsistentPsbt)?; log::debug!("Received original psbt: {:?}", psbt); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index 7acb2539e..ecacb0077 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -151,7 +151,7 @@ impl Receiver { fn extract_proposal_from_v2(&mut self, response: Vec) -> Result { let (payload_bytes, e) = decrypt_message_a(&response, self.context.s.secret_key().clone())?; self.context.e = Some(e); - let payload = String::from_utf8(payload_bytes).map_err(InternalRequestError::Utf8)?; + let payload = String::from_utf8(payload_bytes).map_err(InternalPayloadError::Utf8)?; self.unchecked_from_payload(payload) } @@ -159,7 +159,7 @@ impl Receiver { let (base64, padded_query) = payload.split_once('\n').unwrap_or_default(); let query = padded_query.trim_matches('\0'); log::trace!("Received query: {}, base64: {}", query, base64); // my guess is no \n so default is wrong - let unchecked_psbt = Psbt::from_str(base64).map_err(InternalRequestError::ParsePsbt)?; + let unchecked_psbt = Psbt::from_str(base64).map_err(InternalPayloadError::ParsePsbt)?; let psbt = unchecked_psbt.validate().map_err(InternalPayloadError::InconsistentPsbt)?; log::debug!("Received original psbt: {:?}", psbt); let mut params = Params::from_query_pairs( From 70e71bd81a084ae52e101069f269bb307742e685 Mon Sep 17 00:00:00 2001 From: DanGould Date: Tue, 14 Jan 2025 14:29:15 -0500 Subject: [PATCH 4/8] Make SessionError the v2-specific module error The v2-specific error types in the receive module deserve their own type refleting those types in v1. I do not believe they are yet fully handled in payjoin-cli so its error handling will need to be audited. --- payjoin/src/receive/error.rs | 6 +-- payjoin/src/receive/v2/error.rs | 91 ++++++++++----------------------- payjoin/src/receive/v2/mod.rs | 28 ++++------ 3 files changed, 40 insertions(+), 85 deletions(-) diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 1592dd777..1f07e6b5a 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -75,7 +75,7 @@ pub enum ValidationError { V1(v1::RequestError), /// Protocol-specific errors for BIP-77 v2 sessions (e.g. session management, OHTTP, HPKE encryption) #[cfg(feature = "v2")] - V2(v2::RequestError), + V2(v2::SessionError), } impl From for ValidationError { @@ -87,8 +87,8 @@ impl From for ValidationError { } #[cfg(feature = "v2")] -impl From for ValidationError { - fn from(e: v2::InternalRequestError) -> Self { ValidationError::V2(e.into()) } +impl From for ValidationError { + fn from(e: v2::InternalSessionError) -> Self { ValidationError::V2(e.into()) } } impl fmt::Display for ValidationError { diff --git a/payjoin/src/receive/v2/error.rs b/payjoin/src/receive/v2/error.rs index 9505c9817..a87673db8 100644 --- a/payjoin/src/receive/v2/error.rs +++ b/payjoin/src/receive/v2/error.rs @@ -1,80 +1,60 @@ use core::fmt; use std::error; +use super::Error; +use crate::hpke::HpkeError; use crate::ohttp::OhttpEncapsulationError; -/// Error that may occur when the v2 request from sender is malformed. +/// Error that may occur during a v2 session typestate change /// /// This is currently opaque type because we aren't sure which variants will stay. /// You can only display it. #[derive(Debug)] -pub struct RequestError(InternalRequestError); - -#[derive(Debug)] -pub(crate) enum InternalRequestError { - /// Serde deserialization failed - ParsePsbt(bitcoin::psbt::PsbtParseError), - Utf8(std::string::FromUtf8Error), -} - -impl From for RequestError { - fn from(value: InternalRequestError) -> Self { RequestError(value) } -} - -impl fmt::Display for RequestError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use InternalRequestError::*; - fn write_error( - f: &mut fmt::Formatter, - code: &str, - message: impl fmt::Display, - ) -> fmt::Result { - write!(f, r#"{{ "errorCode": "{}", "message": "{}" }}"#, code, message) - } - - match &self.0 { - ParsePsbt(e) => write_error(f, "Error parsing PSBT:", e), - Utf8(e) => write_error(f, "Error parsing PSBT:", e), - } - } -} - -impl std::error::Error for RequestError { - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use InternalRequestError::*; +pub struct SessionError(InternalSessionError); - match &self.0 { - ParsePsbt(e) => Some(e), - Utf8(e) => Some(e), - } - } +impl From for SessionError { + fn from(value: InternalSessionError) -> Self { SessionError(value) } } -impl From for super::Error { - fn from(e: InternalRequestError) -> Self { super::Error::Validation(e.into()) } +impl From for super::Error { + fn from(e: InternalSessionError) -> Self { super::Error::Validation(e.into()) } } -#[derive(Debug)] -pub struct SessionError(InternalSessionError); - #[derive(Debug)] pub(crate) enum InternalSessionError { /// The session has expired Expired(std::time::SystemTime), /// OHTTP Encapsulation failed OhttpEncapsulation(OhttpEncapsulationError), + /// Hybrid Public Key Encryption failed + Hpke(HpkeError), /// Unexpected response size UnexpectedResponseSize(usize), /// Unexpected status code UnexpectedStatusCode(http::StatusCode), } +impl From for Error { + fn from(e: std::time::SystemTime) -> Self { InternalSessionError::Expired(e).into() } +} + +impl From for Error { + fn from(e: OhttpEncapsulationError) -> Self { + InternalSessionError::OhttpEncapsulation(e).into() + } +} + +impl From for Error { + fn from(e: HpkeError) -> Self { InternalSessionError::Hpke(e).into() } +} + impl fmt::Display for SessionError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self.0 { InternalSessionError::Expired(expiry) => write!(f, "Session expired at {:?}", expiry), InternalSessionError::OhttpEncapsulation(e) => write!(f, "OHTTP Encapsulation Error: {}", e), + InternalSessionError::Hpke(e) => write!(f, "Hpke decryption failed: {}", e), InternalSessionError::UnexpectedResponseSize(size) => write!( f, "Unexpected response size {}, expected {} bytes", @@ -92,28 +72,9 @@ impl error::Error for SessionError { match &self.0 { InternalSessionError::Expired(_) => None, InternalSessionError::OhttpEncapsulation(e) => Some(e), + InternalSessionError::Hpke(e) => Some(e), InternalSessionError::UnexpectedResponseSize(_) => None, InternalSessionError::UnexpectedStatusCode(_) => None, } } } - -impl From for SessionError { - fn from(e: InternalSessionError) -> Self { SessionError(e) } -} - -impl From for SessionError { - fn from(e: OhttpEncapsulationError) -> Self { - SessionError(InternalSessionError::OhttpEncapsulation(e)) - } -} - -impl From for super::Error { - fn from(e: crate::hpke::HpkeError) -> Self { super::Error::Implementation(Box::new(e)) } -} - -impl From for super::Error { - fn from(e: crate::ohttp::OhttpEncapsulationError) -> Self { - super::Error::Implementation(Box::new(e)) - } -} diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index ecacb0077..766045950 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -4,8 +4,8 @@ use std::time::{Duration, SystemTime}; use bitcoin::hashes::{sha256, Hash}; use bitcoin::psbt::Psbt; use bitcoin::{Address, FeeRate, OutPoint, Script, TxOut}; -pub(crate) use error::{InternalRequestError, InternalSessionError}; -pub use error::{RequestError, SessionError}; +pub(crate) use error::InternalSessionError; +pub use error::SessionError; use serde::de::Deserializer; use serde::{Deserialize, Serialize}; use url::Url; @@ -96,7 +96,7 @@ impl Receiver { pub fn extract_req( &mut self, ohttp_relay: &Url, - ) -> Result<(Request, ohttp::ClientResponse), SessionError> { + ) -> Result<(Request, ohttp::ClientResponse), Error> { if SystemTime::now() > self.context.expiry { return Err(InternalSessionError::Expired(self.context.expiry).into()); } @@ -116,9 +116,7 @@ impl Receiver { ) -> Result, Error> { let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = body.try_into().map_err(|_| { - Error::Implementation(Box::new(SessionError::from( - InternalSessionError::UnexpectedResponseSize(body.len()), - ))) + Error::Validation(InternalSessionError::UnexpectedResponseSize(body.len()).into()) })?; log::trace!("decapsulating directory response"); let response = ohttp_decapsulate(context, response_array)?; @@ -279,17 +277,15 @@ impl UncheckedProposal { body: &[u8], context: ohttp::ClientResponse, ) -> Result<(), SessionError> { - let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = - body.try_into().map_err(|_| { - SessionError::from(InternalSessionError::UnexpectedResponseSize(body.len())) - })?; - let response = ohttp_decapsulate(context, response_array)?; + let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = body + .try_into() + .map_err(|_| InternalSessionError::UnexpectedResponseSize(body.len()))?; + let response = ohttp_decapsulate(context, response_array) + .map_err(InternalSessionError::OhttpEncapsulation)?; match response.status() { http::StatusCode::OK => Ok(()), - _ => Err(SessionError::from(InternalSessionError::UnexpectedStatusCode( - response.status(), - ))), + _ => Err(InternalSessionError::UnexpectedStatusCode(response.status()).into()), } } } @@ -546,9 +542,7 @@ impl PayjoinProposal { ) -> Result<(), Error> { let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] = res.try_into().map_err(|_| { - Error::Implementation(Box::new(SessionError::from( - InternalSessionError::UnexpectedResponseSize(res.len()), - ))) + Error::Validation(InternalSessionError::UnexpectedResponseSize(res.len()).into()) })?; let res = ohttp_decapsulate(ohttp_context, response_array)?; if res.status().is_success() { From 8bc8927bc56bee4701280ba19c9116ea6fed8bcd Mon Sep 17 00:00:00 2001 From: DanGould Date: Tue, 14 Jan 2025 18:43:38 -0500 Subject: [PATCH 5/8] Expose receive::Error only in receive module It is not a generic payjoin error. It's receive-specific. --- payjoin-cli/src/app/v1.rs | 3 ++- payjoin-cli/src/app/v2.rs | 3 ++- payjoin/src/lib.rs | 2 -- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index 150241184..c75daa120 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -16,8 +16,9 @@ use hyper_util::rt::TokioIo; use payjoin::bitcoin::psbt::Psbt; use payjoin::bitcoin::{self, FeeRate}; use payjoin::receive::v1::{PayjoinProposal, UncheckedProposal}; +use payjoin::receive::Error; use payjoin::send::v1::SenderBuilder; -use payjoin::{Error, Uri, UriExt}; +use payjoin::{Uri, UriExt}; use tokio::net::TcpListener; use super::config::AppConfig; diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index b3590f2ec..4d8a79274 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -7,8 +7,9 @@ use payjoin::bitcoin::consensus::encode::serialize_hex; use payjoin::bitcoin::psbt::Psbt; use payjoin::bitcoin::{Amount, FeeRate}; use payjoin::receive::v2::{Receiver, UncheckedProposal}; +use payjoin::receive::Error; use payjoin::send::v2::{Sender, SenderBuilder}; -use payjoin::{bitcoin, Error, Uri}; +use payjoin::{bitcoin, Uri}; use tokio::signal; use tokio::sync::watch; diff --git a/payjoin/src/lib.rs b/payjoin/src/lib.rs index a985c21d1..92b3fd7ab 100644 --- a/payjoin/src/lib.rs +++ b/payjoin/src/lib.rs @@ -21,8 +21,6 @@ pub extern crate bitcoin; #[cfg(feature = "receive")] pub mod receive; -#[cfg(feature = "receive")] -pub use crate::receive::Error; #[cfg(feature = "send")] pub mod send; From bfd6f98a7010b24c5b1781028e6ea8f1bc9f7c25 Mon Sep 17 00:00:00 2001 From: DanGould Date: Wed, 15 Jan 2025 14:15:12 -0500 Subject: [PATCH 6/8] Specify explicit PayloadError::source() variants A `_ => None` catchall could cause new error variants to be overlooked. --- payjoin/src/receive/error.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 1f07e6b5a..444ac6969 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -244,7 +244,10 @@ impl std::error::Error for PayloadError { InputWeight(e) => Some(e), PsbtBelowFeeRate(_, _) => None, FeeTooHigh(_, _) => None, - _ => None, + MissingPayment => None, + OriginalPsbtNotBroadcastable => None, + InputOwned(_) => None, + InputSeen(_) => None, } } } From ec4cd3133d280fc000b0fe0d553f548232e57171 Mon Sep 17 00:00:00 2001 From: DanGould Date: Wed, 15 Jan 2025 14:28:21 -0500 Subject: [PATCH 7/8] Return internal error from private psbt_fee_rate Functions should return the least abstract appropriate error which is InternalRequestError in this case. --- payjoin/src/receive/v1/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/payjoin/src/receive/v1/mod.rs b/payjoin/src/receive/v1/mod.rs index afae60312..ec6cc1dca 100644 --- a/payjoin/src/receive/v1/mod.rs +++ b/payjoin/src/receive/v1/mod.rs @@ -122,7 +122,7 @@ impl UncheckedProposal { self.psbt.clone().extract_tx_unchecked_fee_rate() } - fn psbt_fee_rate(&self) -> Result { + fn psbt_fee_rate(&self) -> Result { let original_psbt_fee = self.psbt.fee().map_err(InternalRequestError::Psbt)?; Ok(original_psbt_fee / self.extract_tx_to_schedule_broadcast().weight()) } From 44cbf91fc270e9d7d9d5c2d8f5c235a032470059 Mon Sep 17 00:00:00 2001 From: DanGould Date: Thu, 16 Jan 2025 12:26:01 -0500 Subject: [PATCH 8/8] Specify explicit RequestError::source() variants A `_` catchall could cause new error variants to be overlooked. --- payjoin/src/receive/v1/error.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/payjoin/src/receive/v1/error.rs b/payjoin/src/receive/v1/error.rs index 0e03ff355..5b5371699 100644 --- a/payjoin/src/receive/v1/error.rs +++ b/payjoin/src/receive/v1/error.rs @@ -71,7 +71,9 @@ impl error::Error for RequestError { InternalRequestError::Psbt(e) => Some(e), InternalRequestError::Io(e) => Some(e), InternalRequestError::InvalidContentLength(e) => Some(e), - _ => None, + InternalRequestError::MissingHeader(_) => None, + InternalRequestError::InvalidContentType(_) => None, + InternalRequestError::ContentLengthTooLarge(_) => None, } } }