Fragment parameter fixes

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}"),
         }
     }

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<HpkePublicKey, ParseReceiverPubkeyParamError> {
-        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,29 +45,30 @@ 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"),
         )
     }
 
     /// Retrieve the ohttp parameter from the URL fragment
     fn ohttp(&self) -> Result<OhttpKeys, ParseOhttpKeysParamError> {
-        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<std::time::SystemTime, ParseExpParamError> {
-        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,68 +112,139 @@ pub fn parse_with_fragment(endpoint: &str) -> Result<Url, BadEndpointError> {
     Ok(url)
 }
 
-fn get_param<F, T>(url: &Url, prefix: &str, parse: F) -> Option<T>
-where
-    F: Fn(&str) -> Option<T>,
-{
-    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<char, ParseFragmentError> {
+    // 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<Option<&'a str>, 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::<BTreeMap<&str, &str>>();
+
+    // 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::<Vec<_>>().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::*;
 
         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")
+        );
+    }
 }