Stricter parsing of fragment parameters

Ambiguity in the fragment parameter delimiter or any invalid characters
are no longer allowed.

The HRPs EX, OH, and RK are within the uppercase bech32 character set.
Only this character set along with the HRP delimiter `1` are now
allowed, with either `+` or `-` as a delimiter (but not both).

Author: nothingmuch

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

@@ -23,10 +23,9 @@ 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()))
-            .ok_or(ParseReceiverPubkeyParamError::MissingPubkey)?;
+        let value = get_param(self, "RK1")?.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();
@@ -52,21 +51,19 @@ impl UrlExt for Url {
 
     /// 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()))
-            .ok_or(ParseOhttpKeysParamError::MissingOhttpKeys)?;
-        OhttpKeys::from_str(&value).map_err(ParseOhttpKeysParamError::InvalidOhttpKeys)
+        let value = get_param(self, "OH1")?.ok_or(ParseOhttpKeysParamError::MissingOhttpKeys)?;
+        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()) }
 
     /// 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")?.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 {
@@ -110,22 +107,57 @@ 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() {
-        let mut delim = '-';
-
-        // 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.
-        if !fragment.contains(delim) {
-            delim = '+';
+#[derive(Debug)]
+enum ParseFragmentError {
+    InvalidChar(char),
+    AmbiguousDelimiter,
+}
+
+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.
+    if fragment.is_empty() {
+        return Ok('-');
+    }
+
+    let mut has_plus = false;
+    let mut has_dash = false;
+
+    // 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() {
+        // check for allowed delimiters
+        if c == b'-' {
+            has_dash = true;
+        } else if c == b'+' {
+            has_plus = true;
+        }
+        // Check that the character is in the uppercase bech32 character set unioned with '1' for HRP
+        else if !(48..57 + 1).contains(&c)
+            && c != 65
+            && !(67..72 + 1).contains(&c)
+            && !(74..79 + 1).contains(&c)
+            && !(80..90 + 1).contains(&c)
+        {
+            return Err(ParseFragmentError::InvalidChar(c.into()));
         }
+    }
+
+    match (has_dash, has_plus) {
+        (true, false) => Ok('-'),
+        (false, false) => Ok('-'),
+        (false, true) => Ok('+'),
+        (true, true) => Err(ParseFragmentError::AmbiguousDelimiter),
+    }
+}
+
+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
@@ -134,21 +166,17 @@ where
         // of the parameters.
         for param in fragment.split(delim) {
             if param.starts_with(prefix) {
-                return parse(param);
+                return Ok(Some(param));
             }
         }
     }
-    None
+    Ok(None)
 }
 
 fn set_param(url: &mut Url, prefix: &str, param: &str) {
     let fragment = url.fragment().unwrap_or("");
-
-    // See above for `-` vs `+` backwards compatibility
-    let mut delim = '-';
-    if !fragment.contains(delim) {
-        delim = '+';
-    }
+    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
@@ -175,6 +203,7 @@ fn set_param(url: &mut Url, prefix: &str, param: &str) {
 pub(crate) enum ParseOhttpKeysParamError {
     MissingOhttpKeys,
     InvalidOhttpKeys(crate::ohttp::ParseOhttpKeysError),
+    AmbiguousDelimiter,
 }
 
 impl std::fmt::Display for ParseOhttpKeysParamError {
@@ -184,6 +213,27 @@ impl std::fmt::Display for ParseOhttpKeysParamError {
         match &self {
             MissingOhttpKeys => write!(f, "ohttp keys are missing"),
             InvalidOhttpKeys(o) => write!(f, "invalid ohttp keys: {o}"),
+            AmbiguousDelimiter =>
+                write!(f, "invalid URL fragment (both + and - fragment parameter delimiters)"),
+        }
+    }
+}
+
+impl From<ParseFragmentError> for ParseOhttpKeysParamError {
+    fn from(err: ParseFragmentError) -> Self {
+        match err {
+            ParseFragmentError::AmbiguousDelimiter => ParseOhttpKeysParamError::AmbiguousDelimiter,
+
+            // TODO CharError.into() -> UncheckedHrpstringError is available, but .into().into() needs more finessing
+            ParseFragmentError::InvalidChar(c) => ParseOhttpKeysParamError::InvalidOhttpKeys(
+                crate::ohttp::ParseOhttpKeysError::DecodeBech32(
+                    bitcoin::bech32::primitives::decode::CheckedHrpstringError::Parse(
+                        bitcoin::bech32::primitives::decode::UncheckedHrpstringError::Char(
+                            bitcoin::bech32::primitives::decode::CharError::InvalidChar(c),
+                        ),
+                    ),
+                ),
+            ),
         }
     }
 }
@@ -194,6 +244,7 @@ pub(crate) enum ParseExpParamError {
     InvalidHrp(bitcoin::bech32::Hrp),
     DecodeBech32(bitcoin::bech32::primitives::decode::CheckedHrpstringError),
     InvalidExp(bitcoin::consensus::encode::Error),
+    AmbiguousDelimiter,
 }
 
 impl std::fmt::Display for ParseExpParamError {
@@ -206,17 +257,54 @@ 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}"),
+            AmbiguousDelimiter =>
+                write!(f, "invalid URL fragment (both + and - fragment parameter delimiters)"),
+        }
+    }
+}
+
+impl From<ParseFragmentError> for ParseExpParamError {
+    fn from(err: ParseFragmentError) -> Self {
+        match err {
+            ParseFragmentError::AmbiguousDelimiter => ParseExpParamError::AmbiguousDelimiter,
+
+            // TODO CharError.into() -> UncheckedHrpstringError is available, but .into().into() needs more finessing
+            ParseFragmentError::InvalidChar(c) => ParseExpParamError::DecodeBech32(
+                bitcoin::bech32::primitives::decode::CheckedHrpstringError::Parse(
+                    bitcoin::bech32::primitives::decode::UncheckedHrpstringError::Char(
+                        bitcoin::bech32::primitives::decode::CharError::InvalidChar(c),
+                    ),
+                ),
+            ),
         }
     }
 }
 
-#[cfg(feature = "v2")]
 #[derive(Debug)]
 pub(crate) enum ParseReceiverPubkeyParamError {
     MissingPubkey,
     InvalidHrp(bitcoin::bech32::Hrp),
     DecodeBech32(bitcoin::bech32::primitives::decode::CheckedHrpstringError),
     InvalidPubkey(crate::hpke::HpkeError),
+    AmbiguousDelimiter,
+}
+
+impl From<ParseFragmentError> for ParseReceiverPubkeyParamError {
+    fn from(err: ParseFragmentError) -> Self {
+        match err {
+            ParseFragmentError::AmbiguousDelimiter =>
+                ParseReceiverPubkeyParamError::AmbiguousDelimiter,
+
+            // TODO CharError.into() -> UncheckedHrpstringError is available, but .into().into() needs more finessing
+            ParseFragmentError::InvalidChar(c) => ParseReceiverPubkeyParamError::DecodeBech32(
+                bitcoin::bech32::primitives::decode::CheckedHrpstringError::Parse(
+                    bitcoin::bech32::primitives::decode::UncheckedHrpstringError::Char(
+                        bitcoin::bech32::primitives::decode::CharError::InvalidChar(c),
+                    ),
+                ),
+            ),
+        }
+    }
 }
 
 impl std::fmt::Display for ParseReceiverPubkeyParamError {
@@ -229,6 +317,8 @@ 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}"),
+            AmbiguousDelimiter =>
+                write!(f, "invalid URL fragment (both + and - fragment parameter delimiters)"),
         }
     }
 }
@@ -242,6 +332,7 @@ impl std::error::Error for ParseReceiverPubkeyParamError {
             InvalidHrp(_) => None,
             DecodeBech32(error) => Some(error),
             InvalidPubkey(error) => Some(error),
+            AmbiguousDelimiter => None,
         }
     }
 }
@@ -281,7 +372,9 @@ mod tests {
                 .unwrap();
         assert!(matches!(
             invalid_ohttp_url.ohttp(),
-            Err(ParseOhttpKeysParamError::InvalidOhttpKeys(_))
+            Err(ParseOhttpKeysParamError::InvalidOhttpKeys(
+                crate::ohttp::ParseOhttpKeysError::DecodeBech32(_)
+            ))
         ));
     }