From 05ee24df7d4b6729d8f9f037f1706890665152ca Mon Sep 17 00:00:00 2001 From: timedout Date: Mon, 21 Sep 2026 03:01:34 +0100 Subject: [PATCH] fix: Move helper to helpers.rs, rename field, don't recurse unsigned in redacted_because Renamed `redacted_because_id` to `org.continuwuity.redacted_by` for proper namespacing; `get_unsigned_context` now lives in helpers.rs instead of `mod.rs`. The helper function also now explicitly wipes `unsigned` for `redacted_because` to avoid leaks (see comment). --- src/core/matrix/pdu/redact.rs | 2 +- src/core/matrix/pdu/tests.rs | 7 +- src/core/matrix/pdu/unsigned.rs | 77 +++++-------------- src/service/rooms/timeline/helpers.rs | 105 +++++++++++++++++++++++++- src/service/rooms/timeline/mod.rs | 92 +--------------------- 5 files changed, 129 insertions(+), 154 deletions(-) diff --git a/src/core/matrix/pdu/redact.rs b/src/core/matrix/pdu/redact.rs index 875086563..ae6ec8abf 100644 --- a/src/core/matrix/pdu/redact.rs +++ b/src/core/matrix/pdu/redact.rs @@ -26,7 +26,7 @@ impl super::Pdu { if let Some(event_id) = redacted_because { self.unsigned = to_raw_value(&json!({ - "redacted_because_id": event_id, + "org.continuwuity.redacted_by": event_id, })) .expect("Failed to serialize unsigned") .into(); diff --git a/src/core/matrix/pdu/tests.rs b/src/core/matrix/pdu/tests.rs index a535be89f..b7d68f2b9 100644 --- a/src/core/matrix/pdu/tests.rs +++ b/src/core/matrix/pdu/tests.rs @@ -64,7 +64,7 @@ fn nonsense_sticky_object_does_not_reject_the_pdu() { #[test] fn redaction_removes_stickiness() { let mut pdu = sticky_pdu(&serde_json::json!({ "duration_ms": 300_000 })); - pdu.redact(&ruma::RoomVersionId::V11, serde_json::json!({})) + pdu.redact(&ruma::RoomVersionId::V11, None) .expect("redaction succeeds"); assert!(pdu.sticky.is_none()); @@ -74,7 +74,7 @@ fn redaction_removes_stickiness() { fn sticky_ttl_is_added_to_unsigned() { let mut pdu = sticky_pdu(&serde_json::json!({ "duration_ms": 300_000 })); pdu.origin_server_ts = ruma::MilliSecondsSinceUnixEpoch::now().get(); - pdu.add_sticky_duration_ttl().expect("ttl is added"); + pdu.set_unsigned(None, None, None, None); let unsigned: serde_json::Value = serde_json::from_str(pdu.unsigned.as_deref().expect("unsigned is set").get()) @@ -88,8 +88,7 @@ fn sticky_ttl_is_added_to_unsigned() { #[test] fn no_ttl_for_events_that_are_not_sticky() { - let mut pdu = sticky_pdu(&serde_json::json!({ "duration_ms": "nonsense" })); - pdu.add_sticky_duration_ttl().expect("ttl is skipped"); + let pdu = sticky_pdu(&serde_json::json!({ "duration_ms": "nonsense" })); assert!(pdu.unsigned.is_none()); } diff --git a/src/core/matrix/pdu/unsigned.rs b/src/core/matrix/pdu/unsigned.rs index 47bd1f9ff..6bdc0f79e 100644 --- a/src/core/matrix/pdu/unsigned.rs +++ b/src/core/matrix/pdu/unsigned.rs @@ -5,10 +5,10 @@ use ruma::{ events::{AnyTimelineEvent, room::member::MembershipState}, serde::Raw, }; -use serde_json::value::{RawValue as RawJsonValue, Value as JsonValue, to_raw_value}; +use serde_json::value::{RawValue as RawJsonValue, to_raw_value}; use super::{Pdu, sticky}; -use crate::{Event, Result, err, result::LogErr}; +use crate::Event; type Unsigned = BTreeMap>; @@ -26,17 +26,18 @@ impl Pdu { redacted_because: Option>, ) { // See: https://spec.matrix.org/v1.19/client-server-api/#definition-clientevent_unsigneddata - let Some(mut unsigned) = self - .unsigned() - .and_then(|u| serde_json::from_str::(u.get()).ok()) - else { + let Some(mut unsigned) = self.unsigned().map_or_else( + || Some(Unsigned::new()), + |u| serde_json::from_str::(u.get()).ok(), + ) else { return; }; - let now: ruma::Int = MilliSecondsSinceUnixEpoch::now().get().into(); + let now = MilliSecondsSinceUnixEpoch::now().get(); + let now_i: ruma::Int = now.into(); unsigned.insert( "age".to_owned(), - to_raw_value(&now.saturating_sub(self.origin_server_ts.into())).unwrap(), + to_raw_value(&now_i.saturating_sub(self.origin_server_ts.into())).unwrap(), ); if let Some(membership) = membership { @@ -51,7 +52,7 @@ impl Pdu { ); } if let Some(redacted_because) = redacted_because { - unsigned.remove("redacted_because_id"); + unsigned.remove("org.continuwuity.redacted_by"); unsigned.insert( "redacted_because".to_owned(), to_raw_value(&redacted_because) @@ -63,57 +64,17 @@ impl Pdu { if user_id.is_none_or(|u| u != self.sender()) { unsigned.remove("transaction_id"); } - self.add_sticky_duration_ttl().log_err().ok(); - } - pub fn add_sticky_duration_ttl(&mut self) -> Result { - use BTreeMap as Map; - - let now = u64::from(MilliSecondsSinceUnixEpoch::now().get()); - let Some(expires_at) = self + if let Some(expires_at) = self .sticky .as_deref() - .and_then(|sticky| sticky::expires_at(self.origin_server_ts, sticky, now)) - else { - return Ok(()); - }; - - let mut unsigned: Map<&str, Box> = self - .unsigned - .as_deref() - .map(RawJsonValue::get) - .map_or_else(|| Ok(Map::new()), serde_json::from_str) - .map_err(|e| err!(Database("Invalid unsigned in pdu event: {e}")))?; - - unsigned.insert(sticky::TTL_UNSIGNED_KEY, to_raw_value(&expires_at.saturating_sub(now))?); - self.unsigned = Some(to_raw_value(&unsigned)?); - - Ok(()) - } - - pub fn add_relation(&mut self, name: &str, pdu: Option<&Self>) -> Result { - use serde_json::Map; - - let mut unsigned: Map = self - .unsigned - .as_deref() - .map(RawJsonValue::get) - .map_or_else(|| Ok(Map::new()), serde_json::from_str) - .map_err(|e| err!(Database("Invalid unsigned in pdu event: {e}")))?; - - let pdu = pdu - .map(serde_json::to_value) - .transpose()? - .unwrap_or_else(|| JsonValue::Object(Map::new())); - - unsigned - .entry("m.relations") - .or_insert(JsonValue::Object(Map::new())) - .as_object_mut() - .map(|object| object.insert(name.to_owned(), pdu)); - - self.unsigned = Some(to_raw_value(&unsigned)?); - - Ok(()) + .and_then(|sticky| sticky::expires_at(self.origin_server_ts, sticky, u64::from(now))) + { + unsigned.insert( + sticky::TTL_UNSIGNED_KEY.to_owned(), + to_raw_value(&expires_at.saturating_sub(u64::from(now))) + .expect("sticky event TTL must be a valid JSON value for unsigned"), + ); + } } } diff --git a/src/service/rooms/timeline/helpers.rs b/src/service/rooms/timeline/helpers.rs index b2a595f18..f2926cab1 100644 --- a/src/service/rooms/timeline/helpers.rs +++ b/src/service/rooms/timeline/helpers.rs @@ -1,13 +1,14 @@ //! Helpers for submitting events with the right checks performed use conduwuit::{ - Err, Result, err, + Err, Event, Result, err, matrix::pdu::{PartialPdu, sticky}, + trace, }; use ruma::{ MilliSecondsSinceUnixEpoch, OwnedEventId, RoomId, UserId, events::{ - AnyStateEventContent, StateEventType, + AnyStateEventContent, AnyTimelineEvent, StateEventType, room::{ canonical_alias::RoomCanonicalAliasEventContent, history_visibility::{HistoryVisibility, RoomHistoryVisibilityEventContent}, @@ -22,6 +23,14 @@ use ruma::{ use crate::rooms::state::RoomMutexGuard; +#[derive(Default, Clone)] +pub struct UnsignedContext<'a> { + pub user_id: Option<&'a UserId>, + pub membership: Option, + pub prev_content: Option>, + pub redacted_because: Option>, +} + impl super::Service { #[allow(clippy::too_many_arguments)] pub async fn send_state_event_for_key( @@ -325,4 +334,96 @@ impl super::Service { Ok(()) } + + /// Fetches contextual unsigned information, like prev content, membership, + /// and redacted because. + #[tracing::instrument(skip_all, fields(event_id=%event.event_id(), ?user_id))] + pub async fn get_unsigned_context<'a, E>( + &self, + event: E, + user_id: Option<&'a UserId>, + ) -> UnsignedContext<'a> + where + E: Event, + { + let ssh = self + .services + .state_accessor + .pdu_shortstatehash(event.event_id()) + .await; + let (membership, prev_content) = if let Some(sender_user) = user_id + && let Ok(ssh) = ssh + { + trace!("Fetching membership"); + let membership = Some( + self.services + .state_accessor + .user_membership(ssh, sender_user) + .await, + ); + let prev_content = match event.get_unsigned_property::("replaces_state") + { + | Ok(event_id) => { + // Only include prev content if the sender is allowed to see + // the event + let can_see = self + .services + .state_accessor + .user_can_see_event( + sender_user, + &event.room_id_or_hash(), + event.event_id(), + ) + .await; + if can_see { + trace!(prev_event_id=%event_id, "Fetching prev content"); + self.get_pdu(&event_id).await.map(|p| p.content).ok() + } else { + trace!( + prev_event_id=%event_id, + "Not fetching prev content as sender cannot see the event" + ); + None + } + }, + | _ => None, + }; + + (membership, prev_content) + } else { + trace!(?ssh, "No shortstatehash, cannot fetch prev content or membership"); + (None, None) + }; + + // N.B. Theoretically, the redaction event should be included in a + // redacted capacity if the requesting user isn't allowed to see the + // redaction event. However, the cost of looking up that information + // is extraordinary compared to the value it would provide, so is + // deliberately not done. + let redacted_because = match event + .get_unsigned_property::("org.continuwuity.redacted_by") + { + | Ok(event_id) => self + .get_pdu(&event_id) + .await + .map(|mut e| { + // We don't include unsigned here since the other option is + // to recursively generate unsigned data, which is + // expensive. + // Synapse seems to do that, but I don't see a use for it. + e.unsigned = None; + e + }) + .map(Event::into_format::>) + .ok() + .inspect(|_| trace!(redacted_because_id=%event_id, "Fetched redacted_because")), + | _ => None, + }; + UnsignedContext { + user_id, + membership, + prev_content, + redacted_because, + } + } } diff --git a/src/service/rooms/timeline/mod.rs b/src/service/rooms/timeline/mod.rs index d3d05488d..4279db1b2 100644 --- a/src/service/rooms/timeline/mod.rs +++ b/src/service/rooms/timeline/mod.rs @@ -21,17 +21,13 @@ use conduwuit_core::{ }; use futures::{Future, Stream, TryStreamExt, pin_mut}; use ruma::{ - CanonicalJsonObject, EventId, OwnedEventId, OwnedRoomId, RoomId, UserId, - events::{ - AnyTimelineEvent, - room::{encrypted::Relation, member::MembershipState}, - }, - serde::Raw, + CanonicalJsonObject, EventId, OwnedEventId, OwnedRoomId, RoomId, + events::room::encrypted::Relation, }; use serde::Deserialize; use self::data::Data; -pub use self::{create::pdu_fits, data::PdusIterItem}; +pub use self::{create::pdu_fits, data::PdusIterItem, helpers::UnsignedContext}; use crate::{ Dep, account_data, admin, appservice, config, globals, pusher, rooms, sending, server_keys, sync, users, @@ -65,14 +61,6 @@ pub struct Service { pub mutex_insert: RoomMutexMap, } -#[derive(Default, Clone)] -pub struct UnsignedContext<'a> { - pub user_id: Option<&'a UserId>, - pub membership: Option, - pub prev_content: Option>, - pub redacted_because: Option>, -} - struct Services { server: Arc, account_data: Dep, @@ -284,78 +272,4 @@ impl Service { ) -> impl Stream> + Send + 'a { self.db.pdus(room_id, from.unwrap_or_else(PduCount::min)) } - - /// Fetches contextual unsigned information, like prev content, membership, - /// and redacted because. - pub async fn get_unsigned_context<'a, E>( - &self, - event: E, - user_id: Option<&'a UserId>, - ) -> UnsignedContext<'a> - where - E: Event, - { - let ssh = self - .services - .state_accessor - .pdu_shortstatehash(event.event_id()) - .await; - let (membership, prev_content) = if let Some(sender_user) = user_id - && let Ok(ssh) = ssh - { - let membership = Some( - self.services - .state_accessor - .user_membership(ssh, sender_user) - .await, - ); - let prev_content = match event.get_unsigned_property::("replaces_state") - { - | Ok(event_id) => { - // Only include prev content if the sender is allowed to see - // the event - let can_see = self - .services - .state_accessor - .user_can_see_event( - sender_user, - &event.room_id_or_hash(), - event.event_id(), - ) - .await; - if can_see { - self.get_pdu(&event_id).await.map(|p| p.content).ok() - } else { - None - } - }, - | _ => None, - }; - - (membership, prev_content) - } else { - (None, None) - }; - - // N.B. Theoretically, the redaction event should be included in a - // redacted capacity if the requesting user isn't allowed to see the - // redaction event. However, the cost of looking up that information - // is extraordinary compared to the value it would provide, so is - // deliberately not done. - let redacted_because = - match event.get_unsigned_property::("redacted_because_id") { - | Ok(event_id) => self - .get_pdu(&event_id) - .await - .map(Event::into_format::>) - .ok(), - | _ => None, - }; - UnsignedContext { - user_id, - membership, - prev_content, - redacted_because, - } - } }