-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sync: Avoid crash on invalid state events #3124
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,11 +11,12 @@ use std::{ | |||||||||||||||
|
||||||||||||||||
use bitflags::bitflags; | ||||||||||||||||
pub use members::RoomMember; | ||||||||||||||||
pub use normal::{Room, RoomInfo, RoomInfoUpdate, RoomState, RoomStateFilter}; | ||||||||||||||||
pub use normal::{RawRoomInfo, Room, RoomInfo, RoomInfoUpdate, RoomState, RoomStateFilter}; | ||||||||||||||||
use ruma::{ | ||||||||||||||||
assign, | ||||||||||||||||
canonical_json::redact_in_place, | ||||||||||||||||
events::{ | ||||||||||||||||
call::member::CallMemberEventContent, | ||||||||||||||||
call::member::{CallMemberEvent, CallMemberEventContent}, | ||||||||||||||||
macros::EventContent, | ||||||||||||||||
room::{ | ||||||||||||||||
avatar::RoomAvatarEventContent, | ||||||||||||||||
|
@@ -32,9 +33,10 @@ use ruma::{ | |||||||||||||||
}, | ||||||||||||||||
tag::{TagName, Tags}, | ||||||||||||||||
AnyStrippedStateEvent, AnySyncStateEvent, EmptyStateKey, RedactContent, | ||||||||||||||||
RedactedStateEventContent, StaticStateEventContent, SyncStateEvent, | ||||||||||||||||
RedactedStateEventContent, StateEventType, StaticStateEventContent, SyncStateEvent, | ||||||||||||||||
}, | ||||||||||||||||
room::RoomType, | ||||||||||||||||
serde::Raw, | ||||||||||||||||
EventId, OwnedUserId, RoomVersionId, | ||||||||||||||||
}; | ||||||||||||||||
use serde::{Deserialize, Serialize}; | ||||||||||||||||
|
@@ -116,12 +118,163 @@ pub struct BaseRoomInfo { | |||||||||||||||
pub(crate) notable_tags: RoomNotableTags, | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// This contains the same fields as BaseRoomInfo, but events are in the raw, unparsed form. | ||||||||||||||||
#[derive(Clone, Debug, Default, Serialize, Deserialize)] | ||||||||||||||||
pub struct RawBaseRoomInfo { | ||||||||||||||||
/// The avatar URL of this room. | ||||||||||||||||
pub avatar: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The canonical alias of this room. | ||||||||||||||||
pub canonical_alias: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The `m.room.create` event content of this room. | ||||||||||||||||
pub create: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// A list of user ids this room is considered as direct message, if this | ||||||||||||||||
/// room is a DM. | ||||||||||||||||
pub dm_targets: HashSet<OwnedUserId>, | ||||||||||||||||
/// The `m.room.encryption` event content that enabled E2EE in this room. | ||||||||||||||||
pub encryption: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The guest access policy of this room. | ||||||||||||||||
pub guest_access: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The history visibility policy of this room. | ||||||||||||||||
pub history_visibility: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The join rule policy of this room. | ||||||||||||||||
pub join_rules: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The maximal power level that can be found in this room. | ||||||||||||||||
pub max_power_level: i64, | ||||||||||||||||
/// The `m.room.name` of this room. | ||||||||||||||||
pub name: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The `m.room.tombstone` event content of this room. | ||||||||||||||||
pub tombstone: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// The topic of this room. | ||||||||||||||||
pub topic: Option<Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// All minimal state events that containing one or more running matrixRTC | ||||||||||||||||
/// memberships. | ||||||||||||||||
#[serde(skip_serializing_if = "BTreeMap::is_empty", default)] | ||||||||||||||||
pub rtc_member: BTreeMap<OwnedUserId, Raw<AnySyncStateEvent>>, | ||||||||||||||||
/// Whether this room has been manually marked as unread. | ||||||||||||||||
#[serde(default)] | ||||||||||||||||
pub is_marked_unread: bool, | ||||||||||||||||
/// Some notable tags. | ||||||||||||||||
/// | ||||||||||||||||
/// We are not interested by all the tags. Some tags are more important than | ||||||||||||||||
/// others, and this field collects them. | ||||||||||||||||
#[serde(skip_serializing_if = "RoomNotableTags::is_empty", default)] | ||||||||||||||||
pub notable_tags: RoomNotableTags, | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
impl RawBaseRoomInfo { | ||||||||||||||||
pub fn map_events( | ||||||||||||||||
&mut self, | ||||||||||||||||
f: &dyn Fn(&mut Option<Raw<AnySyncStateEvent>>, StateEventType, &str), | ||||||||||||||||
f_rtc_member: &dyn Fn(&mut BTreeMap<OwnedUserId, Raw<AnySyncStateEvent>>), | ||||||||||||||||
) { | ||||||||||||||||
f(&mut self.avatar, StateEventType::RoomAvatar, ""); | ||||||||||||||||
f(&mut self.canonical_alias, StateEventType::RoomCanonicalAlias, ""); | ||||||||||||||||
f(&mut self.create, StateEventType::RoomCreate, ""); | ||||||||||||||||
f(&mut self.encryption, StateEventType::RoomEncryption, ""); | ||||||||||||||||
f(&mut self.guest_access, StateEventType::RoomGuestAccess, ""); | ||||||||||||||||
f(&mut self.history_visibility, StateEventType::RoomHistoryVisibility, ""); | ||||||||||||||||
f(&mut self.join_rules, StateEventType::RoomJoinRules, ""); | ||||||||||||||||
f(&mut self.name, StateEventType::RoomName, ""); | ||||||||||||||||
f(&mut self.tombstone, StateEventType::RoomTombstone, ""); | ||||||||||||||||
f(&mut self.topic, StateEventType::RoomTopic, ""); | ||||||||||||||||
f_rtc_member(&mut self.rtc_member); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
pub fn handle_redaction(&mut self, event_id: &EventId, room_version: &RoomVersionId) { | ||||||||||||||||
self.map_events( | ||||||||||||||||
// Clean up normal events | ||||||||||||||||
&|event, _event_type, _state_key| { | ||||||||||||||||
if event | ||||||||||||||||
.as_ref() | ||||||||||||||||
.and_then(|a| a.deserialize().ok()) | ||||||||||||||||
.is_some_and(|e| e.event_id() == event_id) | ||||||||||||||||
{ | ||||||||||||||||
if let Ok(mut object) = event.as_ref().unwrap().deserialize_as() { | ||||||||||||||||
Comment on lines
+187
to
+192
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto, the event is deserialized twice here: can we deserialize it once with |
||||||||||||||||
if redact_in_place(&mut object, room_version, None).is_ok() { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use a match and log the error message instead? |
||||||||||||||||
*event = Some(Raw::new(&object).unwrap().cast()); | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
}, | ||||||||||||||||
// Clean up rtc_members | ||||||||||||||||
&|map| { | ||||||||||||||||
Comment on lines
+198
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style nit: please let the code breath 😁
Suggested change
|
||||||||||||||||
for event in map.values_mut() { | ||||||||||||||||
if event.deserialize().is_ok_and(|e| e.event_id() == event_id) { | ||||||||||||||||
if let Ok(mut object) = event.deserialize_as() { | ||||||||||||||||
Comment on lines
+202
to
+203
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we avoid deserializing twice here? Maybe call |
||||||||||||||||
if redact_in_place(&mut object, room_version, None).is_ok() { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto here |
||||||||||||||||
*event = Raw::new(&object).unwrap().cast(); | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
}, | ||||||||||||||||
); | ||||||||||||||||
} | ||||||||||||||||
pub fn extend_with_changes( | ||||||||||||||||
Comment on lines
+212
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Comment on lines
+212
to
+213
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also badly needs a comment. I have no ideas what it's doing, just from looking at the name, signature, or body... |
||||||||||||||||
&mut self, | ||||||||||||||||
changes: &BTreeMap<StateEventType, BTreeMap<String, Raw<AnySyncStateEvent>>>, | ||||||||||||||||
) { | ||||||||||||||||
self.map_events( | ||||||||||||||||
&|event, event_type, state_key| { | ||||||||||||||||
if let Some(e) = changes.get(&event_type).and_then(|c| c.get(state_key)) { | ||||||||||||||||
*event = Some(e.clone()); | ||||||||||||||||
} | ||||||||||||||||
}, | ||||||||||||||||
&|map| { | ||||||||||||||||
if let Some(rtc_changes) = changes.get(&StateEventType::CallMember) { | ||||||||||||||||
map.extend( | ||||||||||||||||
rtc_changes | ||||||||||||||||
.clone() | ||||||||||||||||
.into_iter() | ||||||||||||||||
.filter_map(|(u, r)| OwnedUserId::try_from(u).ok().map(|u| (u, r))), | ||||||||||||||||
); | ||||||||||||||||
// Remove all events that don't contain any memberships anymore. | ||||||||||||||||
map.retain(|_, ev| { | ||||||||||||||||
ev.deserialize_as::<CallMemberEvent>().is_ok_and(|c| { | ||||||||||||||||
c.as_original() | ||||||||||||||||
.is_some_and(|o| !o.content.active_memberships(None).is_empty()) | ||||||||||||||||
}) | ||||||||||||||||
}); | ||||||||||||||||
} | ||||||||||||||||
}, | ||||||||||||||||
); | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
impl BaseRoomInfo { | ||||||||||||||||
/// Create a new, empty base room info. | ||||||||||||||||
pub fn new() -> Self { | ||||||||||||||||
Self::default() | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/// Converts BaseRoomInfo into RawBaseRoomInfo, potentially losing information about some events. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: in doc comments, prefer the |
||||||||||||||||
pub fn into_raw_lossy(self) -> RawBaseRoomInfo { | ||||||||||||||||
RawBaseRoomInfo { | ||||||||||||||||
avatar: self.avatar.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
canonical_alias: self.canonical_alias.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
create: self.create.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
dm_targets: self.dm_targets, | ||||||||||||||||
encryption: self.encryption.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
guest_access: self.guest_access.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
history_visibility: self | ||||||||||||||||
.history_visibility | ||||||||||||||||
.and_then(|e| Raw::new(&e).ok()) | ||||||||||||||||
.map(|r| r.cast()), | ||||||||||||||||
join_rules: self.join_rules.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
max_power_level: self.max_power_level, | ||||||||||||||||
name: self.name.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
tombstone: self.tombstone.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
topic: self.topic.and_then(|e| Raw::new(&e).ok()).map(|r| r.cast()), | ||||||||||||||||
rtc_member: self | ||||||||||||||||
.rtc_member | ||||||||||||||||
.into_iter() | ||||||||||||||||
.filter_map(|(u, e)| Raw::new(&e).ok().map(|r| (u, r.cast()))) | ||||||||||||||||
.collect(), | ||||||||||||||||
is_marked_unread: self.is_marked_unread, | ||||||||||||||||
notable_tags: self.notable_tags, | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
pub(crate) fn calculate_room_name( | ||||||||||||||||
&self, | ||||||||||||||||
joined_member_count: u64, | ||||||||||||||||
|
@@ -190,7 +343,7 @@ impl BaseRoomInfo { | |||||||||||||||
return false; | ||||||||||||||||
}; | ||||||||||||||||
|
||||||||||||||||
// we modify the event so that `origin_sever_ts` gets copied into | ||||||||||||||||
// we modify the event so that `origin_server_ts` gets copied into | ||||||||||||||||
// `content.created_ts` | ||||||||||||||||
let mut o_ev = o_ev.clone(); | ||||||||||||||||
o_ev.content.set_created_ts_if_none(o_ev.origin_server_ts); | ||||||||||||||||
|
@@ -318,7 +471,7 @@ bitflags! { | |||||||||||||||
/// others, and this struct describes them. | ||||||||||||||||
#[repr(transparent)] | ||||||||||||||||
#[derive(Debug, Default, Clone, Copy, Deserialize, Serialize)] | ||||||||||||||||
pub(crate) struct RoomNotableTags: u8 { | ||||||||||||||||
pub struct RoomNotableTags: u8 { | ||||||||||||||||
/// The `m.favourite` tag. | ||||||||||||||||
const FAVOURITE = 0b0000_0001; | ||||||||||||||||
|
||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a code comment. Are
f
andf_rtc_member
called both for rtc member events? What's thestr
passed as the third argument tof
? (also, is it useful, since it's always the empty static string?) Why are the signatures of both functions so different?Also, should the function's signature be different, so that it returns a boolean indicating whether it did find the event? If I understand correctly, there's at most one event that we need to redact, so there's no need to iterate over all the events once we've found the one we wanted to redact? Maybe the best API would be use to the
ControlFlow
data structure from the stdlib.