From 4812837d138d2b67925ac9126caad093f7dec8ac Mon Sep 17 00:00:00 2001 From: timedout Date: Mon, 21 Sep 2026 03:15:46 +0100 Subject: [PATCH] fix: Don't allow `set_unsigned` to panic While an error in this function is probably bad, detonating a metric tonne of TNT on the call tree is probably worse --- src/api/client/context.rs | 21 +++++++--------- src/api/client/message.rs | 10 +++----- src/api/client/room/event.rs | 2 +- src/api/client/room/initial_sync.rs | 11 ++++----- src/api/client/sync/mod.rs | 34 ++++++++++++++++---------- src/api/client/sync/v3/joined.rs | 5 +++- src/api/client/sync/v5.rs | 5 +++- src/core/matrix/pdu/tests.rs | 4 +-- src/core/matrix/pdu/unsigned.rs | 33 +++++++++---------------- src/service/rooms/pdu_metadata/data.rs | 5 +++- src/service/rooms/search/mod.rs | 5 +++- src/service/rooms/threads/mod.rs | 5 +++- 12 files changed, 73 insertions(+), 67 deletions(-) diff --git a/src/api/client/context.rs b/src/api/client/context.rs index 28547df9e..cb1ebf92e 100644 --- a/src/api/client/context.rs +++ b/src/api/client/context.rs @@ -1,6 +1,7 @@ use axum::extract::State; use conduwuit::{ Err, Event, Result, at, debug_warn, err, ref_at, + result::LogErr, utils::{ IterStream, future::TryExtExt, @@ -103,12 +104,10 @@ pub(crate) async fn get_context_route( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); if let Err(e) = services .rooms .pdu_metadata @@ -136,12 +135,10 @@ pub(crate) async fn get_context_route( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); if let Err(e) = services .rooms .pdu_metadata diff --git a/src/api/client/message.rs b/src/api/client/message.rs index 84d2d0269..1a064f438 100644 --- a/src/api/client/message.rs +++ b/src/api/client/message.rs @@ -145,12 +145,10 @@ pub(crate) async fn get_message_events_route( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); if let Err(e) = services .rooms .pdu_metadata diff --git a/src/api/client/room/event.rs b/src/api/client/room/event.rs index fe7d7f106..a54d6d597 100644 --- a/src/api/client/room/event.rs +++ b/src/api/client/room/event.rs @@ -63,7 +63,7 @@ pub(crate) async fn get_room_event_route( .timeline .get_unsigned_context(&event, Some(sender_user)) .await; - event.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because); + event.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because)?; Ok(get_room_event::v3::Response::new(event.into_format())) } diff --git a/src/api/client/room/initial_sync.rs b/src/api/client/room/initial_sync.rs index 415d07399..73a76a7be 100644 --- a/src/api/client/room/initial_sync.rs +++ b/src/api/client/room/initial_sync.rs @@ -1,6 +1,7 @@ use axum::extract::State; use conduwuit::{ Err, Event, Result, at, debug_warn, + result::LogErr, utils::{BoolExt, stream::TryTools}, }; use futures::{FutureExt, TryStreamExt, future::try_join4}; @@ -58,12 +59,10 @@ pub(crate) async fn room_initial_sync_route( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); if let Err(e) = services .rooms .pdu_metadata diff --git a/src/api/client/sync/mod.rs b/src/api/client/sync/mod.rs index 0b1025de5..1ef3bd4b0 100644 --- a/src/api/client/sync/mod.rs +++ b/src/api/client/sync/mod.rs @@ -6,7 +6,9 @@ use std::collections::VecDeque; use conduwuit::{ Event, PduCount, Result, debug_warn, err, matrix::pdu::PduEvent, - ref_at, trace, + ref_at, + result::LogErr, + trace, utils::stream::{BroadbandExt, ReadyExt, TryIgnore, WidebandExt}, }; use conduwuit_service::Services; @@ -89,12 +91,15 @@ async fn load_timeline( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned( + ctx.user_id, + ctx.membership, + ctx.prev_content, + ctx.redacted_because, + ) + .log_err() + .ok(); Some(pdu) }) .then(async move |mut pdu| { @@ -124,12 +129,15 @@ async fn load_timeline( .timeline .get_unsigned_context(&pdu.1, Some(sender_user)) .await; - pdu.1.set_unsigned( - ctx.user_id, - ctx.membership, - ctx.prev_content, - ctx.redacted_because, - ); + pdu.1 + .set_unsigned( + ctx.user_id, + ctx.membership, + ctx.prev_content, + ctx.redacted_because, + ) + .log_err() + .ok(); Some(pdu) }) .then(async move |mut pdu| { diff --git a/src/api/client/sync/v3/joined.rs b/src/api/client/sync/v3/joined.rs index 3fe1d3ee2..f24499d29 100644 --- a/src/api/client/sync/v3/joined.rs +++ b/src/api/client/sync/v3/joined.rs @@ -6,6 +6,7 @@ use conduwuit::{ Event, pdu::{PduCount, PduEvent, sticky}, }, + result::LogErr, trace, utils::{ BoolExt, IterStream, ReadyExt, TryFutureExtExt, @@ -327,7 +328,9 @@ async fn build_sticky_events( .timeline .get_unsigned_context(&pdu, Some(sync_context.syncing_user)) .await; - pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because); + pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); events.push(Event::into_format(pdu)); } diff --git a/src/api/client/sync/v5.rs b/src/api/client/sync/v5.rs index 8411a81e5..94627bd29 100644 --- a/src/api/client/sync/v5.rs +++ b/src/api/client/sync/v5.rs @@ -11,6 +11,7 @@ use conduwuit::{ Event, TypeStateKey, pdu::{PduCount, PduEvent, sticky}, }, + result::LogErr, trace, utils::{ BoolExt, FutureBoolExt, IterStream, ReadyExt, TryFutureExtExt, @@ -1217,7 +1218,9 @@ async fn push_sticky_event( .timeline .get_unsigned_context(&pdu, Some(sender_user)) .await; - pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because); + pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); response .rooms .entry(room_id) diff --git a/src/core/matrix/pdu/tests.rs b/src/core/matrix/pdu/tests.rs index b7d68f2b9..48417d8ed 100644 --- a/src/core/matrix/pdu/tests.rs +++ b/src/core/matrix/pdu/tests.rs @@ -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.set_unsigned(None, None, None, None); + pdu.set_unsigned(None, None, None, None).unwrap(); let unsigned: serde_json::Value = serde_json::from_str(pdu.unsigned.as_deref().expect("unsigned is set").get()) @@ -99,7 +99,7 @@ fn set_unsigned_adds_the_sticky_ttl() { // section let mut pdu = sticky_pdu(&serde_json::json!({ "duration_ms": 300_000 })); pdu.origin_server_ts = ruma::MilliSecondsSinceUnixEpoch::now().get(); - pdu.set_unsigned(None, None, None, None); + pdu.set_unsigned(None, None, None, None).unwrap(); let unsigned: serde_json::Value = serde_json::from_str(pdu.unsigned.as_deref().expect("unsigned is set").get()) diff --git a/src/core/matrix/pdu/unsigned.rs b/src/core/matrix/pdu/unsigned.rs index cf3e5e1f1..550fdeed5 100644 --- a/src/core/matrix/pdu/unsigned.rs +++ b/src/core/matrix/pdu/unsigned.rs @@ -34,40 +34,29 @@ impl Pdu { membership: Option, prev_content: Option>, redacted_because: Option>, - ) { + ) -> crate::Result { // See: https://spec.matrix.org/v1.19/client-server-api/#definition-clientevent_unsigneddata - let Some(mut unsigned) = self.unsigned().map_or_else( - || Some(Unsigned::new()), - |u| serde_json::from_str::(u.get()).ok(), - ) else { - return; - }; + let mut unsigned = self + .unsigned() + .map_or_else(|| Ok(Unsigned::new()), |u| serde_json::from_str::(u.get()))?; let now = MilliSecondsSinceUnixEpoch::now().get(); let now_i: ruma::Int = now.into(); unsigned.insert( "age".to_owned(), - to_raw_value(&now_i.saturating_sub(self.origin_server_ts.into())).unwrap(), + to_raw_value(&now_i.saturating_sub(self.origin_server_ts.into()))?, ); if let Some(membership) = membership { - unsigned.insert("membership".to_owned(), to_raw_value(&membership).unwrap()); + unsigned.insert("membership".to_owned(), to_raw_value(&membership)?); } if let Some(prev_content) = prev_content { - unsigned.insert( - "prev_content".to_owned(), - to_raw_value(&prev_content) - .expect("prev_content must be a valid JSON value for unsigned"), - ); + unsigned.insert("prev_content".to_owned(), to_raw_value(&prev_content)?); } if let Some(redacted_because) = redacted_because { unsigned.remove("org.continuwuity.redacted_by"); - unsigned.insert( - "redacted_because".to_owned(), - to_raw_value(&redacted_because) - .expect("redacted_because must be a valid JSON value for unsigned"), - ); + unsigned.insert("redacted_because".to_owned(), to_raw_value(&redacted_because)?); } // Remove transaction_id unless the user is the sender @@ -82,11 +71,11 @@ impl Pdu { { 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"), + to_raw_value(&expires_at.saturating_sub(u64::from(now)))?, ); } - self.unsigned = Some(to_raw_value(&unsigned).unwrap()); + self.unsigned = Some(to_raw_value(&unsigned)?); + Ok(unsigned) } } diff --git a/src/service/rooms/pdu_metadata/data.rs b/src/service/rooms/pdu_metadata/data.rs index 18ef01f10..be529d20f 100644 --- a/src/service/rooms/pdu_metadata/data.rs +++ b/src/service/rooms/pdu_metadata/data.rs @@ -3,6 +3,7 @@ use std::{mem::size_of, sync::Arc}; use conduwuit::{ arrayvec::ArrayVec, matrix::PduCount, + result::LogErr, utils::{ ReadyExt, stream::{TryIgnore, WidebandExt}, @@ -99,7 +100,9 @@ impl Data { .timeline .get_unsigned_context(&pdu, Some(user_id)) .await; - pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because); + pdu.set_unsigned(ctx.user_id, ctx.membership, ctx.prev_content, ctx.redacted_because) + .log_err() + .ok(); Some((shorteventid, pdu)) }) diff --git a/src/service/rooms/search/mod.rs b/src/service/rooms/search/mod.rs index 24f7ca688..317cf2648 100644 --- a/src/service/rooms/search/mod.rs +++ b/src/service/rooms/search/mod.rs @@ -5,6 +5,7 @@ use conduwuit::{ arrayvec::ArrayVec, debug_warn, matrix::event::{Event, Matches}, + result::LogErr, utils::{ ArrayVecExt, IterStream, ReadyExt, set, stream::{TryIgnore, WidebandExt}, @@ -147,7 +148,9 @@ impl Service { ctx.membership, ctx.prev_content, ctx.redacted_because, - ); + ) + .log_err() + .ok(); Some(pdu) }) diff --git a/src/service/rooms/threads/mod.rs b/src/service/rooms/threads/mod.rs index 6afd11b4d..48622586c 100644 --- a/src/service/rooms/threads/mod.rs +++ b/src/service/rooms/threads/mod.rs @@ -1,5 +1,6 @@ use std::{collections::BTreeMap, sync::Arc}; +use conduwuit::result::LogErr; use conduwuit_core::{ Event, Result, err, matrix::pdu::{PduCount, PduEvent, PduId, RawPduId}, @@ -174,7 +175,9 @@ impl Service { ctx.membership, ctx.prev_content, ctx.redacted_because, - ); + ) + .log_err() + .ok(); Some((pdu_id.shorteventid, pdu)) });