identifiers: Fix MatrixToUri parsing for non-url-encoded room aliases

crates/ruma-common/CHANGELOG.md

@@ -1,5 +1,9 @@
 # [unreleased]
 
+Bug fixes:
+
+* Fix `MatrixToUri` parsing for non-url-encoded room aliases
+
 Breaking changes:
 
 * Remove deprecated `EventType` enum

crates/ruma-common/src/identifiers/matrix_uri.rs

@@ -13,7 +13,7 @@ use super::{
     EventId, OwnedEventId, OwnedRoomAliasId, OwnedRoomId, OwnedRoomOrAliasId, OwnedServerName,
     OwnedUserId, RoomAliasId, RoomId, RoomOrAliasId, UserId,
 };
-use crate::PrivOwnedStr;
+use crate::{PrivOwnedStr, ServerName};
 
 const MATRIX_TO_BASE_URL: &str = "https://matrix.to/#/";
 const MATRIX_SCHEME: &str = "matrix";
@@ -298,26 +298,47 @@ impl MatrixToUri {
 
     /// Try parsing a `&str` into a `MatrixToUri`.
     pub fn parse(s: &str) -> Result<Self, Error> {
-        let without_base = if let Some(stripped) = s.strip_prefix(MATRIX_TO_BASE_URL) {
+        // We do not rely on parsing with `url::Url` because the meaningful part
-            stripped
+        // of the URI is in its fragment part.
-        } else {
+        //
-            return Err(MatrixToError::WrongBaseUrl.into());
+        // Even if the fragment part looks like parts of a URI, non-url-encoded
-        };
+        // room aliases (starting with `#`) could be detected as fragments,
+        // messing up the URI parsing.
+        //
+        // A matrix.to URI looks like this: https://matrix.to/#/{MatrixId}?{query};
+        // where the MatrixId should be percent-encoded, but might not, and the query
+        // should also be percent-encoded.
 
-        let url = Url::parse(MATRIX_TO_BASE_URL.trim_end_matches("#/"))
+        let s = s.strip_prefix(MATRIX_TO_BASE_URL).ok_or(MatrixToError::WrongBaseUrl)?;
-            .expect("matrix.to base URL is valid")
+        let s = s.strip_suffix('/').unwrap_or(s);
-            .join(without_base)
-            .map_err(|_| MatrixToError::InvalidUrl)?;
 
-        let id = MatrixId::parse_with_sigil(url.path())?;
+        // Separate the identifiers and the query.
-        let mut via = vec![];
+        let mut parts = s.split('?');
 
-        for (key, value) in url.query_pairs() {
+        let ids_part = parts.next().expect("a split iterator yields at least one value");
-            if key.as_ref() == "via" {
+        let id = MatrixId::parse_with_sigil(ids_part)?;
-                via.push(value.parse()?);
+
-            } else {
+        // Parse the query for routing arguments.
-                return Err(MatrixToError::UnknownArgument.into());
+        let via = parts
-            }
+            .next()
+            .map(|query| {
+                // `form_urlencoded` takes care of percent-decoding the query.
+                let query_parts = form_urlencoded::parse(query.as_bytes());
+
+                query_parts
+                    .map(|(key, value)| {
+                        (key == "via")
+                            .then(|| ServerName::parse(&value))
+                            .unwrap_or_else(|| Err(MatrixToError::UnknownArgument.into()))
+                    })
+                    .collect::<Result<Vec<_>, _>>()
+            })
+            .transpose()?
+            .unwrap_or_default();
+
+        // That would mean there are two `?` in the URL which is not valid.
+        if parts.next().is_some() {
+            return Err(MatrixToError::InvalidUrl.into());
         }
 
         Ok(Self { id, via })
@@ -727,11 +748,15 @@ mod tests {
             .expect("Failed to create MatrixToUri.");
         assert_eq!(matrix_to.id(), &room_alias_id!("#ruma:notareal.hs").into());
 
-        let matrix_to =
+        let matrix_to = MatrixToUri::parse(
-            MatrixToUri::parse("https://matrix.to/#/%21ruma%3Anotareal.hs?via=notareal.hs")
+            "https://matrix.to/#/%21ruma%3Anotareal.hs?via=notareal.hs&via=anotherunreal.hs",
-                .expect("Failed to create MatrixToUri.");
+        )
+        .expect("Failed to create MatrixToUri.");
         assert_eq!(matrix_to.id(), &room_id!("!ruma:notareal.hs").into());
-        assert_eq!(matrix_to.via(), &[server_name!("notareal.hs").to_owned()]);
+        assert_eq!(
+            matrix_to.via(),
+            &[server_name!("notareal.hs").to_owned(), server_name!("anotherunreal.hs").to_owned(),]
+        );
 
         let matrix_to =
             MatrixToUri::parse("https://matrix.to/#/%23ruma%3Anotareal.hs/%24event%3Anotareal.hs")
@@ -751,6 +776,39 @@ mod tests {
         assert_eq!(matrix_to.via().len(), 0);
     }
 
+    #[test]
+    fn parse_matrixtouri_valid_uris_not_urlencoded() {
+        let matrix_to = MatrixToUri::parse("https://matrix.to/#/@jplatte:notareal.hs")
+            .expect("Failed to create MatrixToUri.");
+        assert_eq!(matrix_to.id(), &user_id!("@jplatte:notareal.hs").into());
+
+        let matrix_to = MatrixToUri::parse("https://matrix.to/#/#ruma:notareal.hs")
+            .expect("Failed to create MatrixToUri.");
+        assert_eq!(matrix_to.id(), &room_alias_id!("#ruma:notareal.hs").into());
+
+        let matrix_to = MatrixToUri::parse("https://matrix.to/#/!ruma:notareal.hs?via=notareal.hs")
+            .expect("Failed to create MatrixToUri.");
+        assert_eq!(matrix_to.id(), &room_id!("!ruma:notareal.hs").into());
+        assert_eq!(matrix_to.via(), &[server_name!("notareal.hs").to_owned()]);
+
+        let matrix_to =
+            MatrixToUri::parse("https://matrix.to/#/#ruma:notareal.hs/$event:notareal.hs")
+                .expect("Failed to create MatrixToUri.");
+        assert_eq!(
+            matrix_to.id(),
+            &(room_alias_id!("#ruma:notareal.hs"), event_id!("$event:notareal.hs")).into()
+        );
+
+        let matrix_to =
+            MatrixToUri::parse("https://matrix.to/#/!ruma:notareal.hs/$event:notareal.hs")
+                .expect("Failed to create MatrixToUri.");
+        assert_eq!(
+            matrix_to.id(),
+            &(room_id!("!ruma:notareal.hs"), event_id!("$event:notareal.hs")).into()
+        );
+        assert_eq!(matrix_to.via().len(), 0);
+    }
+
     #[test]
     fn parse_matrixtouri_wrong_base_url() {
         assert_eq!(MatrixToUri::parse("").unwrap_err(), MatrixToError::WrongBaseUrl.into());