diff --git a/changelog.d/20324.bugfix b/changelog.d/20324.bugfix new file mode 100644 index 0000000000..62478357dd --- /dev/null +++ b/changelog.d/20324.bugfix @@ -0,0 +1 @@ +Fix the expiry of [MSC4354 Sticky Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) being reported differently to how the server tracks them. \ No newline at end of file diff --git a/rust/src/events/internal_metadata.rs b/rust/src/events/internal_metadata.rs index 841965db6f..269a5cc55e 100644 --- a/rust/src/events/internal_metadata.rs +++ b/rust/src/events/internal_metadata.rs @@ -544,6 +544,11 @@ impl EventInternalMetadata { pub fn policy_server_spammy(&self) -> PyResult { Ok(self.read_inner()?.get_policy_server_spammy()) } + + /// Whether a spam checker marked this event as spammy. + pub fn spam_checker_spammy(&self) -> PyResult { + Ok(self.read_inner()?.get_spam_checker_spammy()) + } } /// Helper to convert `None` to an `AttributeError` for a property getter. @@ -656,7 +661,7 @@ impl EventInternalMetadata { } #[getter] - fn get_received_ts(&self) -> PyResult> { + pub fn get_received_ts(&self) -> PyResult> { Ok(self.read_inner()?.received_ts) } #[setter] diff --git a/rust/src/events/mod.rs b/rust/src/events/mod.rs index 0d746a2ec8..2f519d8dcb 100644 --- a/rust/src/events/mod.rs +++ b/rust/src/events/mod.rs @@ -48,7 +48,7 @@ //! string describing why auth rejected the event. //! -use std::sync::Arc; +use std::{cmp::min, sync::Arc}; use anyhow::Error; use pyo3::{ @@ -396,10 +396,30 @@ impl Event { Ok(new_event) } - /// If this event has the `msc4354_sticky` top-level field, returns a - /// `SynapseDuration` representing the sticky duration. Otherwise returns - /// `None`. - fn sticky_duration(&self) -> Option { + /// If this event has the `msc4354_sticky` top-level field and is eligible + /// to be sticky, returns a `SynapseDuration` representing the sticky duration. + /// Otherwise returns `None`. + /// + /// The duration is capped at 1 hour according to MSC4354. + /// Spammy events (according to spam checkers and policy servers) are not eligible to be sticky + /// so we return `None` for those. + /// + /// See [`Self::locally_sticky_until_ts`] to get the effective stickiness expiry timestamp. + fn sticky_duration(&self) -> PyResult> { + if self.internal_metadata.policy_server_spammy()? + || self.internal_metadata.spam_checker_spammy()? + || self.rejected_reason.is_some() + { + // Spammy and rejected events are not sticky. + // + // As per MSC4354: + // > Policy servers and similar homeserver-specific anti-spam techniques (e.g. custom spam checker modules) still apply to these events, + // > including events received over federation. If the anti-spam technique classifies a sticky event as spam, + // > it is treated as a regular non-sticky event and does not enjoy the properties that an unexpired sticky event does. + // > — https://github.com/matrix-org/matrix-spec-proposals/blob/35bf5589c313c81b74c60969701f4657acb9ccb6/proposals/4354-sticky-events.md?plain=1#L107 + return Ok(None); + } + const MAX_DURATION: SynapseDuration = SynapseDuration::from_hours(1); let sticky_obj = self @@ -410,19 +430,50 @@ impl Event { let sticky_obj = match sticky_obj { Some(serde_json::Value::Object(obj)) => obj, - _ => return None, + _ => return Ok(None), }; // Check for a valid duration field. The MSC requires `duration_ms` to // be a non-negative integer. If it's missing or invalid, we treat the // event as non-sticky by returning `None`. - let duration_ms = sticky_obj.get("duration_ms")?.as_u64()?; + let Some(duration_ms) = sticky_obj.get("duration_ms").and_then(|v| v.as_u64()) else { + return Ok(None); + }; let duration = SynapseDuration::from_milliseconds(duration_ms); let duration = std::cmp::min(duration, MAX_DURATION); - Some(duration) + Ok(Some(duration)) + } + + /// If this event has the `msc4354_sticky` top-level field and is eligible + /// to be sticky (isn't spammy), returns a timestamp (in milliseconds since the epoch) of + /// the expiry of the event's stickiness, as seen locally on this homeserver. + /// + /// It is the caller's responsibility to check this time is not in the past. + fn locally_sticky_until_ts(&self) -> PyResult> { + let Some(sticky_duration) = self.sticky_duration()? else { + return Ok(None); + }; + let Some(received_ts) = self.internal_metadata.get_received_ts()? else { + // Either this event hasn't been persisted or this event is from before Synapse v0.16.0 + // If before v0.16.0, we don't want this event to be sticky (even if someone was prescient + // enough to create a Sticky Event with a forged `origin_server_ts` far into the future...) + return Ok(None); + }; + + // Clamp to when it was received so a forged origin timestamp can't produce + // an event that is sticky beyond the 1 hour cap. + let start_ts = min( + self.parsed_event.common_fields.origin_server_ts, + received_ts, + ); + + // From this, calculate the stickiness expiry time. + // The `as i64` cast is safe as sticky durations are capped to an + // hour, which is well within the i64 range. + Ok(Some(start_ts + sticky_duration.as_millis() as i64)) } fn __str__(&self) -> PyResult { diff --git a/rust/src/events/serialize.rs b/rust/src/events/serialize.rs index bb3eb4df06..77a42ce5d2 100644 --- a/rust/src/events/serialize.rs +++ b/rust/src/events/serialize.rs @@ -519,18 +519,12 @@ fn serialize_event_value( } if config.msc4354_enabled { - if let Some(sticky_duration) = event.sticky_duration() { - // min() ensures the origin server can't claim a time in the future - // to exceed the stickiness duration limit. - // - // The `as i64` cast is safe as sticky duration are capped to an - // hour, which is well within the i64 range. - let expires_at = std::cmp::min(event.origin_server_ts(), time_now_ms) - + sticky_duration.as_millis() as i64; - if expires_at > time_now_ms { + if let Some(sticky_expiry_until) = event.locally_sticky_until_ts()? { + let sticky_remaining_ms = sticky_expiry_until - time_now_ms; + if sticky_remaining_ms > 0 { unsigned.insert( unsigned_field::STICKY_TTL.to_owned(), - Value::Number(Number::from(expires_at - time_now_ms)), + Value::Number(Number::from(sticky_remaining_ms)), ); } } diff --git a/synapse/storage/databases/main/sticky_events.py b/synapse/storage/databases/main/sticky_events.py index a45d7d9363..9730554c97 100644 --- a/synapse/storage/databases/main/sticky_events.py +++ b/synapse/storage/databases/main/sticky_events.py @@ -349,7 +349,6 @@ class StickyEventsWorkerStore(StateGroupWorkerStore, CacheInvalidationWorkerStor Skips inserting events: - if they are considered spammy by the policy server; - (unsure if correct, track: https://github.com/matrix-org/matrix-spec-proposals/pull/4354#discussion_r2727593350) - if they are considered spammy by a Synapse spam checker module; - if they are rejected; - if they are outliers (they should be reconsidered for insertion when de-outliered); or @@ -372,27 +371,19 @@ class StickyEventsWorkerStore(StateGroupWorkerStore, CacheInvalidationWorkerStor # event, expires_at sticky_events: list[tuple[EventBase, int]] = [] for ev in events: - # MSC: Note: policy servers and other similar antispam techniques still apply to these events. - # We don't filter out soft-failed events altogether (in case they get re-evaluated later), - # so filter out `spam_checker_spammy` events specifically as we don't want to re-evaluate _those_ later. - if ( - ev.internal_metadata.policy_server_spammy - or ev.internal_metadata.spam_checker_spammy - ): - continue - # We shouldn't be passed rejected events, but if we do, we filter them out too. - if ev.rejected_reason is not None: - continue # We can't persist outlier sticky events as we don't know the room state at that event if ev.internal_metadata.is_outlier(): continue - sticky_duration = ev.sticky_duration() - if sticky_duration is None: - continue + # Calculate the end time as start_time + effective sticky duration - expires_at = min(ev.origin_server_ts, now_ms) + sticky_duration.as_millis() - # Filter out already expired sticky events - if expires_at <= now_ms: + expires_at = ev.locally_sticky_until_ts() + if ( + # Not a sticky event altogether + expires_at is None + or + # Filter out already expired sticky events + expires_at <= now_ms + ): continue sticky_events.append((ev, expires_at)) diff --git a/synapse/synapse_rust/events.pyi b/synapse/synapse_rust/events.pyi index 07e94b769b..4b45bbdf20 100644 --- a/synapse/synapse_rust/events.pyi +++ b/synapse/synapse_rust/events.pyi @@ -313,9 +313,26 @@ class Event: not affect the original.""" def sticky_duration(self) -> Duration | None: - """If this event has the ``msc4354_sticky`` top-level field, returns a - ``SynapseDuration`` representing the sticky duration. Otherwise returns - ``None``.""" + """ + If this event has the `msc4354_sticky` top-level field and is eligible + to be sticky, returns a `SynapseDuration` representing the sticky duration. + Otherwise returns `None`. + + The duration is capped at 1 hour according to MSC4354. + Spammy events (according to spam checkers and policy servers) are not eligible to be sticky + so we return `None` for those. + + See `locally_sticky_until_ts` to get the effective stickiness expiry timestamp. + """ + + def locally_sticky_until_ts(self) -> int | None: + """ + If this event has the `msc4354_sticky` top-level field and is eligible + to be sticky (isn't spammy), returns a timestamp (in milliseconds since the epoch) of + the expiry of the event's stickiness, as seen locally on this homeserver. + + It is the caller's responsibility to check this time is not in the past. + """ class ThreadAggregation: """The bundled thread summary for an event."""