From 93368c7eb8c7cd7b4dcf861f7cf99048c6f2abb6 Mon Sep 17 00:00:00 2001 From: Olivier 'reivilibre Date: Fri, 9 Oct 2026 12:30:58 +0100 Subject: [PATCH] Prevent MSC4354 Sticky Events being sticky when they were redacted in the same persistence batch. (#20322) Part of: MSC4354 Part of: #19641 Follows: #20232 (which applies already-persisted redactions to Sticky Events being persisted) In some cases we batch events together during persistence. If one of the events in the batch is a redaction of another event, we should apply it, which provides the same benefit as #20232 (namely: we avoid potentially dodgy event content being persisted & the event naturally loses its stickiness so is not written to `sticky_events`) --------- Signed-off-by: Olivier 'reivilibre --- changelog.d/20322.bugfix | 1 + synapse/storage/databases/main/events.py | 122 ++++++++++++++++------- tests/storage/test_sticky_events.py | 74 ++++++++++++++ 3 files changed, 162 insertions(+), 35 deletions(-) create mode 100644 changelog.d/20322.bugfix diff --git a/changelog.d/20322.bugfix b/changelog.d/20322.bugfix new file mode 100644 index 0000000000..3614b64d8a --- /dev/null +++ b/changelog.d/20322.bugfix @@ -0,0 +1 @@ +Prevent [MSC4354 Sticky Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) being sticky when they were redacted in the same persistence batch. \ No newline at end of file diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index c555c3fce0..ced620f4c1 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -250,6 +250,16 @@ class NewEventChainLinks: links: list[tuple[int, int]] = attr.Factory(list) +@attr.s(slots=True, frozen=True, auto_attribs=True) +class _RedactionInfo: + """Information about a redaction. For internal use (`_apply_existing_redaction_txn`).""" + + event_id: str + redacts_event_id: str + event_sender: str + event_needs_v3_recheck: bool + + class PersistEventsStore: """Contains all the functions for writing events to the database. @@ -2924,6 +2934,10 @@ class PersistEventsStore: If we have any redactions for the events we're about to persist, pre-applies those if they are appropriate. + The redactions may either have been persisted previously + (e.g. we received the redaction over federation before the event it redacts) + or be part of this same batch of events. + Redaction events themselves won't be redacted immediately, to avoid breaking circular redactions (which are tested in `test_circular_redaction`). @@ -2938,51 +2952,89 @@ class PersistEventsStore: Returns a copy of the `events_and_contexts` lists with redactions applied. """ + assert all(ev.room_id == room_id for ev, _ in events_and_contexts) + # Get all the event IDs, whilst also filtering out redaction events # which we don't want to redact immediately anyway - event_ids = [ + event_ids = { ev.event_id for ev, _ in events_and_contexts if ev.type != EventTypes.Redaction - ] + } if not event_ids: # nothing to do here return events_and_contexts + def get_persisted_redactions() -> list[_RedactionInfo]: + """ + Fetches the already-persisted redactions affecting the events in this batch. + """ + event_id_in_list_clause, event_id_in_list_args = make_in_list_sql_clause( + txn.database_engine, + "redactions.redacts", + event_ids, + ) + txn.execute( + f""" + SELECT redactions.event_id, redactions.redacts, redaction_events.sender, redactions.recheck + FROM redactions + INNER JOIN events AS redaction_events + ON redactions.event_id = redaction_events.event_id + AND redaction_events.room_id = ? + WHERE {event_id_in_list_clause} + """, + (room_id, *event_id_in_list_args), + ) + return [ + _RedactionInfo( + event_id=redaction_event_id, + redacts_event_id=redaction_redacts_event_id, + event_sender=redaction_event_sender, + event_needs_v3_recheck=bool(redaction_event_needs_v3_recheck), + ) + for ( + redaction_event_id, + redaction_redacts_event_id, + redaction_event_sender, + redaction_event_needs_v3_recheck, + ) in txn + ] + + def get_redactions_in_batch() -> list[_RedactionInfo]: + """ + Picks out redactions _within this batch_ that affect events also in this batch. + """ + return [ + _RedactionInfo( + event_id=ev.event_id, + redacts_event_id=ev.redacts, + event_sender=ev.sender, + event_needs_v3_recheck=ev.internal_metadata.need_to_check_redaction(), + ) + for ev, ctx in events_and_contexts + if ev.type == EventTypes.Redaction + and ev.redacts is not None + and ev.redacts in event_ids + and not ctx.rejected + ] + events_by_id = {ev.event_id: ev for ev, _ in events_and_contexts} - event_id_in_list_clause, event_id_in_list_args = make_in_list_sql_clause( - txn.database_engine, - "redactions.redacts", - event_ids, - ) - txn.execute( - f""" - SELECT redactions.event_id, redactions.redacts, redaction_events.sender, redactions.recheck - FROM redactions - INNER JOIN events AS redaction_events - ON redactions.event_id = redaction_events.event_id - AND redaction_events.room_id = ? - WHERE {event_id_in_list_clause} - """, - (room_id, *event_id_in_list_args), - ) # map from redacted event ID to the event ID that redacts it redacted_event_id_map = {} - for ( - redaction_event_id, - redaction_redacts_event_id, - redaction_event_sender, - redaction_event_needs_v3_recheck, - ) in txn: - if redaction_redacts_event_id in redacted_event_id_map: + + for redaction in itertools.chain( + get_persisted_redactions(), + get_redactions_in_batch(), + ): + if redaction.redacts_event_id in redacted_event_id_map: # Already redacted continue - event = events_by_id[redaction_redacts_event_id] + event = events_by_id[redaction.redacts_event_id] if ( - redaction_event_needs_v3_recheck + redaction.event_needs_v3_recheck # Normally v1/v2 don't ever need rechecks because the checks are part of auth rules. # However the `recheck` column was only recently introduced as a retrofit, # with a database-level default of `true`. @@ -2991,27 +3043,27 @@ class PersistEventsStore: and room_version.event_format != EventFormatVersions.ROOM_V1_V2 ): # Apply the same logic as `_maybe_redact_event_row` - if get_domain_from_id(redaction_event_sender) != get_domain_from_id( + if get_domain_from_id(redaction.event_sender) != get_domain_from_id( event.sender ): # Sender servers don't match, so the event isn't actually redacted logger.debug( "redaction of %s by %s skipped as it required a v3 recheck and sender servers differ: %r != %r", - redaction_redacts_event_id, - redaction_event_id, - redaction_event_sender, + redaction.redacts_event_id, + redaction.event_id, + redaction.event_sender, event.sender, ) continue # Unlike `_maybe_redact_event_row`, we _don't_ mutate the cached instance of the redaction event to set - # `recheck_redaction` to False here, as we don't actually load the redaction out of the database. + # `recheck_redaction` to False here, as we don't necessarily have the redaction event to hand. - redacted_event_id_map[redaction_redacts_event_id] = redaction_event_id + redacted_event_id_map[redaction.redacts_event_id] = redaction.event_id logger.debug( "%r redacted at persistence time by %r", - redaction_redacts_event_id, - redaction_event_id, + redaction.redacts_event_id, + redaction.event_id, ) out: list[EventPersistencePair] = [] diff --git a/tests/storage/test_sticky_events.py b/tests/storage/test_sticky_events.py index 3e8aadb6ac..e81ce129d4 100644 --- a/tests/storage/test_sticky_events.py +++ b/tests/storage/test_sticky_events.py @@ -14,6 +14,7 @@ import sqlite3 from http import HTTPStatus from immutabledict import immutabledict +from parameterized import parameterized from twisted.internet.testing import MemoryReactor @@ -469,6 +470,79 @@ class StickyEventsTestCase(unittest.HomeserverTestCase): # ...nor shown to clients down sync. self.assertEqual(self._get_visible_sticky_event_ids(), set()) + @parameterized.expand([(True,), (False,)]) + def test_redacted_in_same_batch_not_tracked( + self, flip_order_in_batch: bool + ) -> None: + """ + Tests that a sticky event which is persisted in the same batch as a redaction of it + is not sent down to clients over sync and is not added to the `sticky_events` table. + """ + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + self.helper.join(self.room_id, user2_id, tok=user2_tok) + + persist_controller = self.hs.get_storage_controllers().persistence + assert persist_controller is not None + + sticky_event, sticky_event_context = self.get_success( + create_event( + self.hs, + room_id=self.room_id, + sender=user2_id, + type=EventTypes.Message, + content={"body": "sticky", "msgtype": "m.text"}, + # Corresponds to StickyEvent.EVENT_FIELD_NAME + msc4354_sticky=StickyEventField( + duration_ms=Duration(minutes=1).as_millis() + ), + ) + ) + redaction_event, redaction_event_context = self.get_success( + create_event( + self.hs, + room_id=self.room_id, + sender=user2_id, + type=EventTypes.Redaction, + content={ + "reason": "nothing here but us trees", + "redacts": sticky_event.event_id, + }, + ) + ) + + # Persist both events in a single batch. + # Test both orders of events to make sure it doesn't affect the outcome. + if flip_order_in_batch: + batch = [ + (redaction_event, redaction_event_context), + (sticky_event, sticky_event_context), + ] + else: + batch = [ + (sticky_event, sticky_event_context), + (redaction_event, redaction_event_context), + ] + self.get_success(persist_controller.persist_events(batch)) + + # The event should have been persisted in its redacted form. + event = self.get_success(self.store.get_event(sticky_event.event_id)) + self.assertEqual(event.internal_metadata.redacted_by, redaction_event.event_id) + self.assertEqual(event.content, {}) + self.assertIsNone(event.sticky_duration()) + + # Since it is redacted, it must not have been added to the sticky_events + # table... + sticky_events = self.get_success( + self.store.db_pool.simple_select_list( + table="sticky_events", keyvalues=None, retcols=("event_id",) + ) + ) + self.assertEqual(sticky_events, []) + + # ...nor shown to clients down sync. + self.assertEqual(self._get_visible_sticky_event_ids(), set()) + def test_soft_failed_events_are_tracked(self) -> None: """ Tests that sticky events marked as soft_failed ARE inserted