diff --git a/payjoin/src/core/ohttp.rs b/payjoin/src/core/ohttp.rs index ff7a11831..13200a9f7 100644 --- a/payjoin/src/core/ohttp.rs +++ b/payjoin/src/core/ohttp.rs @@ -322,7 +322,7 @@ impl std::fmt::Display for ParseOhttpKeysError { match self { ParseOhttpKeysError::InvalidFormat => write!(f, "Invalid format"), ParseOhttpKeysError::InvalidPublicKey => write!(f, "Invalid public key"), - ParseOhttpKeysError::DecodeBech32(e) => write!(f, "Failed to decode base64: {e}"), + ParseOhttpKeysError::DecodeBech32(e) => write!(f, "Failed to decode bech32: {e}"), ParseOhttpKeysError::DecodeKeyConfig(e) => write!(f, "Failed to decode KeyConfig: {e}"), } } diff --git a/payjoin/src/core/uri/url_ext.rs b/payjoin/src/core/uri/url_ext.rs index 9a835f2eb..06199b465 100644 --- a/payjoin/src/core/uri/url_ext.rs +++ b/payjoin/src/core/uri/url_ext.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::str::FromStr; use bitcoin::bech32::Hrp; @@ -22,10 +23,11 @@ pub(crate) trait UrlExt { impl UrlExt for Url { /// Retrieve the receiver's public key from the URL fragment fn receiver_pubkey(&self) -> Result { - let value = get_param(self, "RK1", |v| Some(v.to_owned())) + let value = get_param(self, "RK1") + .map_err(ParseReceiverPubkeyParamError::InvalidFragment)? .ok_or(ParseReceiverPubkeyParamError::MissingPubkey)?; - let (hrp, bytes) = crate::bech32::nochecksum::decode(&value) + let (hrp, bytes) = crate::bech32::nochecksum::decode(value) .map_err(ParseReceiverPubkeyParamError::DecodeBech32)?; let rk_hrp: Hrp = Hrp::parse("RK").unwrap(); @@ -43,7 +45,6 @@ impl UrlExt for Url { set_param( self, - "RK1", &crate::bech32::nochecksum::encode(rk_hrp, &pubkey.to_compressed_bytes()) .expect("encoding compressed pubkey bytes should never fail"), ) @@ -51,21 +52,23 @@ impl UrlExt for Url { /// Retrieve the ohttp parameter from the URL fragment fn ohttp(&self) -> Result { - let value = get_param(self, "OH1", |v| Some(v.to_owned())) + let value = get_param(self, "OH1") + .map_err(ParseOhttpKeysParamError::InvalidFragment)? .ok_or(ParseOhttpKeysParamError::MissingOhttpKeys)?; - OhttpKeys::from_str(&value).map_err(ParseOhttpKeysParamError::InvalidOhttpKeys) + OhttpKeys::from_str(value).map_err(ParseOhttpKeysParamError::InvalidOhttpKeys) } /// Set the ohttp parameter in the URL fragment - fn set_ohttp(&mut self, ohttp: OhttpKeys) { set_param(self, "OH1", &ohttp.to_string()) } + fn set_ohttp(&mut self, ohttp: OhttpKeys) { set_param(self, &ohttp.to_string()) } /// Retrieve the exp parameter from the URL fragment fn exp(&self) -> Result { - let value = - get_param(self, "EX1", |v| Some(v.to_owned())).ok_or(ParseExpParamError::MissingExp)?; + let value = get_param(self, "EX1") + .map_err(ParseExpParamError::InvalidFragment)? + .ok_or(ParseExpParamError::MissingExp)?; let (hrp, bytes) = - crate::bech32::nochecksum::decode(&value).map_err(ParseExpParamError::DecodeBech32)?; + crate::bech32::nochecksum::decode(value).map_err(ParseExpParamError::DecodeBech32)?; let ex_hrp: Hrp = Hrp::parse("EX").unwrap(); if hrp != ex_hrp { @@ -94,7 +97,7 @@ impl UrlExt for Url { let exp_str = crate::bech32::nochecksum::encode(ex_hrp, &buf) .expect("encoding u32 timestamp should never fail"); - set_param(self, "EX1", &exp_str) + set_param(self, &exp_str) } } @@ -109,47 +112,118 @@ pub fn parse_with_fragment(endpoint: &str) -> Result { Ok(url) } -fn get_param(url: &Url, prefix: &str, parse: F) -> Option -where - F: Fn(&str) -> Option, -{ - if let Some(fragment) = url.fragment() { - for param in fragment.split('+') { - if param.starts_with(prefix) { - return parse(param); - } +#[derive(Debug)] +pub(crate) enum ParseFragmentError { + InvalidChar(char), + AmbiguousDelimiter, +} + +impl std::error::Error for ParseFragmentError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None } +} + +impl std::fmt::Display for ParseFragmentError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use ParseFragmentError::*; + + match &self { + InvalidChar(c) => write!(f, "invalid character: {c} (must be uppercase)"), + AmbiguousDelimiter => write!(f, "ambiguous fragment delimiter (both + and - found)"), } } - None } -fn set_param(url: &mut Url, prefix: &str, param: &str) { - let fragment = url.fragment().unwrap_or(""); - let mut fragment = fragment.to_string(); - if let Some(start) = fragment.find(prefix) { - let end = fragment[start..].find('+').map_or(fragment.len(), |i| start + i); - fragment.replace_range(start..end, ""); - if fragment.ends_with('+') { - fragment.pop(); +fn check_fragment_delimiter(fragment: &str) -> Result { + // For backwards compatibility, also accept `+` as a + // fragment parameter delimiter. This was previously + // specified, but may be interpreted as ` ` by some + // URI parsoing libraries. Therefore if `-` is missing, + // assume the URI was generated following the older + // version of the spec. + + let has_dash = fragment.contains('-'); + let has_plus = fragment.contains('+'); + + // Even though fragment is a &str, it should be ascii so bytes() correspond + // to chars(), except that it's easier to check that they are in range + for c in fragment.bytes() { + // These character ranges are more permissive than uppercase bech32, but + // also more restrictive than bech32 in general since lowercase is not + // allowed + if !(b'0'..b'9' + 1).contains(&c) + && !(b'A'..b'Z' + 1).contains(&c) + && c != b'-' + && c != b'+' + { + return Err(ParseFragmentError::InvalidChar(c.into())); } } - if !fragment.is_empty() { - fragment.push('+'); + match (has_dash, has_plus) { + (true, true) => Err(ParseFragmentError::AmbiguousDelimiter), + (false, true) => Ok('+'), + _ => Ok('-'), } - fragment.push_str(param); +} - url.set_fragment(if fragment.is_empty() { None } else { Some(&fragment) }); +fn get_param<'a>(url: &'a Url, prefix: &str) -> Result, ParseFragmentError> { + if let Some(fragment) = url.fragment() { + let delim = check_fragment_delimiter(fragment)?; + + // The spec says these MUST be ordered lexicographically. + // However, this was a late spec change, and only matters + // for privacy reasons (fingerprinting implementations). + // To maintain compatibility, we don't care about the order + // of the parameters. + for param in fragment.split(delim) { + if param.starts_with(prefix) { + return Ok(Some(param)); + } + } + } + Ok(None) +} + +/// Set a URL fragment parameter, inserting it or replacing it depending on +/// whether a parameter with the same bech32 HRP is already present. +/// +/// Parameters are sorted lexicographically by prefix. +fn set_param(url: &mut Url, new_param: &str) { + let fragment = url.fragment().unwrap_or(""); + let delim = check_fragment_delimiter(fragment) + .expect("set_param must be called on a URL with a valid fragment"); + + // In case of an invalid fragment parameter the following will still attempt + // to retain the existing data + let mut params = fragment + .split(delim) + .filter(|param| !param.is_empty()) + .map(|param| { + let key = param.split('1').next().unwrap_or(param); + (key, param) + }) + .collect::>(); + + // TODO: change param to Option(&str) to allow deletion? + let key = new_param.split('1').next().unwrap_or(new_param); + params.insert(key, new_param); + + if params.is_empty() { + url.set_fragment(None) + } else { + // Can we avoid intermediate allocation of Vec, intersperse() exists but not in MSRV + let fragment = params.values().copied().collect::>().join("-"); + url.set_fragment(Some(&fragment)); + } } -#[cfg(feature = "v2")] #[derive(Debug)] pub(crate) enum ParseOhttpKeysParamError { MissingOhttpKeys, InvalidOhttpKeys(crate::ohttp::ParseOhttpKeysError), + InvalidFragment(ParseFragmentError), } -#[cfg(feature = "v2")] impl std::fmt::Display for ParseOhttpKeysParamError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { use ParseOhttpKeysParamError::*; @@ -157,20 +231,20 @@ impl std::fmt::Display for ParseOhttpKeysParamError { match &self { MissingOhttpKeys => write!(f, "ohttp keys are missing"), InvalidOhttpKeys(o) => write!(f, "invalid ohttp keys: {o}"), + InvalidFragment(e) => write!(f, "invalid URL fragment: {e}"), } } } -#[cfg(feature = "v2")] #[derive(Debug)] pub(crate) enum ParseExpParamError { MissingExp, InvalidHrp(bitcoin::bech32::Hrp), DecodeBech32(bitcoin::bech32::primitives::decode::CheckedHrpstringError), InvalidExp(bitcoin::consensus::encode::Error), + InvalidFragment(ParseFragmentError), } -#[cfg(feature = "v2")] impl std::fmt::Display for ParseExpParamError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { use ParseExpParamError::*; @@ -181,20 +255,20 @@ impl std::fmt::Display for ParseExpParamError { DecodeBech32(d) => write!(f, "exp is not valid bech32: {d}"), InvalidExp(i) => write!(f, "exp param does not contain a bitcoin consensus encoded u32: {i}"), + InvalidFragment(e) => write!(f, "invalid URL fragment: {e}"), } } } -#[cfg(feature = "v2")] #[derive(Debug)] pub(crate) enum ParseReceiverPubkeyParamError { MissingPubkey, InvalidHrp(bitcoin::bech32::Hrp), DecodeBech32(bitcoin::bech32::primitives::decode::CheckedHrpstringError), InvalidPubkey(crate::hpke::HpkeError), + InvalidFragment(ParseFragmentError), } -#[cfg(feature = "v2")] impl std::fmt::Display for ParseReceiverPubkeyParamError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { use ParseReceiverPubkeyParamError::*; @@ -205,11 +279,11 @@ impl std::fmt::Display for ParseReceiverPubkeyParamError { DecodeBech32(e) => write!(f, "receiver public is not valid base64: {e}"), InvalidPubkey(e) => write!(f, "receiver public key does not represent a valid pubkey: {e}"), + InvalidFragment(e) => write!(f, "invalid URL fragment: {e}"), } } } -#[cfg(feature = "v2")] impl std::error::Error for ParseReceiverPubkeyParamError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { use ParseReceiverPubkeyParamError::*; @@ -219,6 +293,7 @@ impl std::error::Error for ParseReceiverPubkeyParamError { InvalidHrp(_) => None, DecodeBech32(error) => Some(error), InvalidPubkey(error) => Some(error), + InvalidFragment(error) => Some(error), } } } @@ -258,7 +333,7 @@ mod tests { .unwrap(); assert!(matches!( invalid_ohttp_url.ohttp(), - Err(ParseOhttpKeysParamError::InvalidOhttpKeys(_)) + Err(ParseOhttpKeysParamError::InvalidFragment(_)) )); } @@ -279,9 +354,16 @@ mod tests { let missing_exp_url = EXAMPLE_URL.clone(); assert!(matches!(missing_exp_url.exp(), Err(ParseExpParamError::MissingExp))); - let invalid_bech32_exp_url = + let invalid_fragment_exp_url = Url::parse("http://example.com?pj=https://test-payjoin-url#EX1invalid_bech_32") .unwrap(); + assert!(matches!( + invalid_fragment_exp_url.exp(), + Err(ParseExpParamError::InvalidFragment(_)) + )); + + let invalid_bech32_exp_url = + Url::parse("http://example.com?pj=https://test-payjoin-url#EX1INVALIDBECH32").unwrap(); assert!(matches!(invalid_bech32_exp_url.exp(), Err(ParseExpParamError::DecodeBech32(_)))); // Since the HRP is everything to the left of the right-most separator, the invalid url in @@ -304,9 +386,16 @@ mod tests { Err(ParseReceiverPubkeyParamError::MissingPubkey) )); - let invalid_bech32_receiver_pubkey_url = + let invalid_fragment_receiver_pubkey_url = Url::parse("http://example.com?pj=https://test-payjoin-url#RK1invalid_bech_32") .unwrap(); + assert!(matches!( + invalid_fragment_receiver_pubkey_url.receiver_pubkey(), + Err(ParseReceiverPubkeyParamError::InvalidFragment(_)) + )); + + let invalid_bech32_receiver_pubkey_url = + Url::parse("http://example.com?pj=https://test-payjoin-url#RK1INVALIDBECH32").unwrap(); assert!(matches!( invalid_bech32_receiver_pubkey_url.receiver_pubkey(), Err(ParseReceiverPubkeyParamError::DecodeBech32(_)) @@ -374,4 +463,54 @@ mod tests { } Ok(()) } + + #[test] + fn test_fragment_delimeter_backwards_compatibility() { + // ensure + is still accepted as a delimiter + let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=0.01\ + &pjos=0&pj=HTTPS://EXAMPLE.COM/\ + %23EX1C4UC6ES+OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC"; + let pjuri = Uri::try_from(uri).unwrap().assume_checked().check_pj_supported().unwrap(); + + let mut endpoint = pjuri.extras.endpoint().clone(); + assert!(endpoint.ohttp().is_ok()); + assert!(endpoint.exp().is_ok()); + + // Before setting the delimiter should be preserved + assert_eq!( + endpoint.fragment(), + Some("EX1C4UC6ES+OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC") + ); + + // Upon setting any value, the delimiter should be normalized to `-` + endpoint.set_exp(pjuri.extras.endpoint.exp().unwrap()); + assert_eq!( + endpoint.fragment(), + Some("EX1C4UC6ES-OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC") + ); + } + + #[test] + fn test_fragment_lexicographical_order() { + let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=0.01\ + &pjos=0&pj=HTTPS://EXAMPLE.COM/\ + %23OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC-EX1C4UC6ES"; + let pjuri = Uri::try_from(uri).unwrap().assume_checked().check_pj_supported().unwrap(); + + let mut endpoint = pjuri.extras.endpoint().clone(); + assert!(endpoint.ohttp().is_ok()); + assert!(endpoint.exp().is_ok()); + + assert_eq!( + endpoint.fragment(), + Some("OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC-EX1C4UC6ES") + ); + + // Upon setting any value, the order should be normalized to lexicographical + endpoint.set_exp(pjuri.extras.endpoint.exp().unwrap()); + assert_eq!( + endpoint.fragment(), + Some("EX1C4UC6ES-OH1QYPM5JXYNS754Y4R45QWE336QFX6ZR8DQGVQCULVZTV20TFVEYDMFQC") + ); + } }