🛡️ Sentinel: [HIGH] Fix XSS in event preview formatting

.jules/sentinel.md

@@ -0,0 +1,4 @@
+## 2024-05-13 - XSS in Event Preview Formatting
+**Vulnerability:** User-controlled content inside `AnyOtherStateEventContentChange` and `OtherMessageLike` (such as custom room aliases, canonical aliases, guest access rules, history visibility, and join rules) was directly embedded into `format!` strings without sanitization when generating HTML previews in `src/event_preview.rs`.
+**Learning:** Rust's `format!` macro seamlessly handles standard string integration, making it easy to overlook that data dynamically generated by external users (like custom Matrix rules) still requires explicit HTML escaping when the output will be rendered via a rich text/HTML widget.
+**Prevention:** Always trace the origin of fields within structs passed to preview formatters (like `event_type` and custom Matrix rules) and apply `htmlize::escape_text` when the `format_as_html` parameter is true.

src/event_preview.rs

@@ -98,7 +98,7 @@ pub fn text_preview_of_timeline_item(
                     String::from("[Live Location]"),
                     BeforeText::UsernameWithColon,
                 )),
-                MsgLikeKind::Other(oml) => text_preview_of_other_message_like(oml),
+                MsgLikeKind::Other(oml) => text_preview_of_other_message_like(oml, true),
             }
         }
         TimelineItemContent::MembershipChange(membership_change) => {
@@ -173,7 +173,7 @@ pub fn plaintext_body_of_timeline_item(
                     String::from("[Live Location]")
                 }
                 MsgLikeKind::Other(other_msg_like) => {
-                    text_preview_of_other_message_like(other_msg_like)
+                    text_preview_of_other_message_like(other_msg_like, false)
                         .format_with(&utils::get_or_fetch_event_sender(event_tl_item, None), false)}
             }
         }
@@ -436,9 +436,16 @@ pub fn text_preview_of_encrypted_message(
 /// Returns a plaintext preview of the given other message-like event.
 pub fn text_preview_of_other_message_like(
     other_msg_like: &OtherMessageLike,
+    format_as_html: bool,
 ) -> TextPreview {
+    let event_type_owned = other_msg_like.event_type().to_string();
+    let event_type_str = if format_as_html {
+        htmlize::escape_text(&event_type_owned)
+    } else {
+        Cow::Borrowed(event_type_owned.as_str())
+    };
     TextPreview::from((
-        format!("[Other message type: {}]", other_msg_like.event_type()),
+        format!("[Other message type: {}]", event_type_str),
         BeforeText::UsernameWithColon,
     ))
 }
@@ -453,7 +460,12 @@ pub fn text_preview_of_other_state(
             let mut s = String::from("set this room's aliases to ");
             let last_alias = content.aliases.len() - 1;
             for (i, alias) in content.aliases.iter().enumerate() {
-                s.push_str(alias.as_str());
+                let alias_str = if format_as_html {
+                    htmlize::escape_text(alias.as_str())
+                } else {
+                    Cow::Borrowed(alias.as_str())
+                };
+                s.push_str(&alias_str);
                 if i != last_alias {
                     s.push_str(", ");
                 }
@@ -465,9 +477,13 @@ pub fn text_preview_of_other_state(
             Some(String::from("set this room's avatar picture."))
         }
         AnyOtherStateEventContentChange::RoomCanonicalAlias(StateEventContentChange::Original { content, .. }) => {
-            Some(format!("set the main address of this room to {}.",
-                content.alias.as_ref().map(|a| a.as_str()).unwrap_or("none")
-            ))
+            let alias = content.alias.as_ref().map(|a| a.as_str()).unwrap_or("none");
+            let alias_str = if format_as_html {
+                htmlize::escape_text(alias)
+            } else {
+                Cow::Borrowed(alias)
+            };
+            Some(format!("set the main address of this room to {}.", alias_str))
         }
         AnyOtherStateEventContentChange::RoomCreate(StateEventContentChange::Original { content, .. }) => {
             Some(format!("created this room (v{}).", content.room_version.as_str()))
@@ -479,17 +495,28 @@ pub fn text_preview_of_other_state(
             Some(match &content.guest_access {
                 GuestAccess::CanJoin => String::from("has allowed guests to join this room."),
                 GuestAccess::Forbidden => String::from("has forbidden guests from joining this room."),
-                custom => format!("has set custom guest access rules for this room: {}", custom.as_str()),
+                custom => {
+                    let custom_str = if format_as_html {
+                        htmlize::escape_text(custom.as_str())
+                    } else {
+                        Cow::Borrowed(custom.as_str())
+                    };
+                    format!("has set custom guest access rules for this room: {}", custom_str)
+                }
             })
         }
         AnyOtherStateEventContentChange::RoomHistoryVisibility(StateEventContentChange::Original { content, .. }) => {
             Some(format!("set this room's history to be visible by {}",
                 match &content.history_visibility {
-                    HistoryVisibility::Invited => "invited users, since they were invited.",
-                    HistoryVisibility::Joined => "joined users, since they joined.",
-                    HistoryVisibility::Shared => "joined users, for all of time.",
-                    HistoryVisibility::WorldReadable => "anyone for all time.",
-                    custom => custom.as_str(),
+                    HistoryVisibility::Invited => Cow::Borrowed("invited users, since they were invited."),
+                    HistoryVisibility::Joined => Cow::Borrowed("joined users, since they joined."),
+                    HistoryVisibility::Shared => Cow::Borrowed("joined users, for all of time."),
+                    HistoryVisibility::WorldReadable => Cow::Borrowed("anyone for all time."),
+                    custom => if format_as_html {
+                        htmlize::escape_text(custom.as_str())
+                    } else {
+                        Cow::Borrowed(custom.as_str())
+                    },
                 },
             ))
         }
@@ -501,7 +528,14 @@ pub fn text_preview_of_other_state(
                 JoinRule::Restricted(_) => String::from("set this room to be joinable by invite only or with restrictions."),
                 JoinRule::KnockRestricted(_) => String::from("set this room to be joinable by invite only or requestable with restrictions."),
                 JoinRule::Invite  => String::from("set this room to be joinable by invite only."),
-                custom => format!("set custom join rules for this room: {}", custom.as_str()),
+                custom => {
+                    let custom_str = if format_as_html {
+                        htmlize::escape_text(custom.as_str())
+                    } else {
+                        Cow::Borrowed(custom.as_str())
+                    };
+                    format!("set custom join rules for this room: {}", custom_str)
+                }
             })
         }
         AnyOtherStateEventContentChange::RoomPinnedEvents(StateEventContentChange::Original { content, .. }) => {

src/home/room_screen.rs

@@ -4415,7 +4415,7 @@ impl SmallStateEventContent for OtherMessageLike {
     ) -> (WidgetRef, ItemDrawnStatus) {
         item.label(cx, ids!(content)).set_text(
             cx,
-            &text_preview_of_other_message_like(self).format_with(username, false),
+            &text_preview_of_other_message_like(self, false).format_with(username, false),
         );
         new_drawn_status.content_drawn = true;
         (item, new_drawn_status)