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

@@ -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();

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());
 }

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<String, Box<RawJsonValue>>;
 
@@ -26,17 +26,18 @@ impl Pdu {
 		redacted_because: Option<Raw<AnyTimelineEvent>>,
 	) {
 		// 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::<Unsigned>(u.get()).ok())
-		else {
+		let Some(mut unsigned) = self.unsigned().map_or_else(
+			|| Some(Unsigned::new()),
+			|u| serde_json::from_str::<Unsigned>(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<RawJsonValue>> = 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<String, JsonValue> = 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"),
+			);
+		}
 	}
 }

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<MembershipState>,
+	pub prev_content: Option<Box<serde_json::value::RawValue>>,
+	pub redacted_because: Option<Raw<AnyTimelineEvent>>,
+}
+
 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::<OwnedEventId>("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::<OwnedEventId>("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::<Raw<AnyTimelineEvent>>)
+				.ok()
+				.inspect(|_| trace!(redacted_because_id=%event_id, "Fetched redacted_because")),
+			| _ => None,
+		};
+		UnsignedContext {
+			user_id,
+			membership,
+			prev_content,
+			redacted_because,
+		}
+	}
 }

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<MembershipState>,
-	pub prev_content: Option<Box<serde_json::value::RawValue>>,
-	pub redacted_because: Option<Raw<AnyTimelineEvent>>,
-}
-
 struct Services {
 	server: Arc<Server>,
 	account_data: Dep<account_data::Service>,
@@ -284,78 +272,4 @@ impl Service {
 	) -> impl Stream<Item = Result<PdusIterItem>> + 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::<OwnedEventId>("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::<OwnedEventId>("redacted_because_id") {
-				| Ok(event_id) => self
-					.get_pdu(&event_id)
-					.await
-					.map(Event::into_format::<Raw<AnyTimelineEvent>>)
-					.ok(),
-				| _ => None,
-			};
-		UnsignedContext {
-			user_id,
-			membership,
-			prev_content,
-			redacted_because,
-		}
-	}
 }