From 36c61df6595cf8a73b7ea0b8351c07378aac0b31 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 29 Jul 2019 16:07:12 +0200 Subject: Check room ID and type of redacted event --- synapse/storage/events_worker.py | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index cc7df5cf14..f18129f7a3 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -277,27 +277,33 @@ class EventsWorkerStore(SQLBaseStore): # 2. have _get_event_from_row just call the first half of # that - orig_sender = yield self._simple_select_one_onecol( + orig_event_info = yield self._simple_select_one( table="events", keyvalues={"event_id": entry.event.redacts}, - retcol="sender", + retcols=["sender", "room_id", "type"], allow_none=True, ) + if not orig_event_info: + # We don't have the event that is being redacted, so we + # assume that the event isn't authorized for now. (If we + # later receive the event, then we will always redact + # it anyway, since we have this redaction) + continue + + if orig_event_info["room_id"] != entry.event.room_id: + continue + + if orig_event_info["type"] == EventTypes.Redaction: + continue + expected_domain = get_domain_from_id(entry.event.sender) if ( - orig_sender - and get_domain_from_id(orig_sender) == expected_domain + get_domain_from_id(orig_event_info["sender"]) == expected_domain ): # This redaction event is allowed. Mark as not needing a # recheck. entry.event.internal_metadata.recheck_redaction = False - else: - # We don't have the event that is being redacted, so we - # assume that the event isn't authorized for now. (If we - # later receive the event, then we will always redact - # it anyway, since we have this redaction) - continue if allow_rejected or not entry.event.rejected_reason: if check_redacted and entry.redacted_event: @@ -567,6 +573,12 @@ class EventsWorkerStore(SQLBaseStore): # Senders don't match, so the event isn't actually redacted redacted_event = None + if because.room_id != original_ev.room_id: + redacted_event = None + + if original_ev.type == EventTypes.Redaction: + redacted_event = None + cache_entry = _EventCacheEntry( event=original_ev, redacted_event=redacted_event ) -- cgit 1.5.1 From bbd6208b3ea256da0d8920292f55cb5b554b8cfc Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 29 Jul 2019 17:22:42 +0200 Subject: Do checks sooner --- synapse/storage/events_worker.py | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index f18129f7a3..cbb2529d5e 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -538,7 +538,7 @@ class EventsWorkerStore(SQLBaseStore): ) redacted_event = None - if redacted: + if redacted and original_ev.type != EventTypes.Redaction: redacted_event = prune_event(original_ev) redaction_id = yield self._simple_select_one_onecol( @@ -556,28 +556,26 @@ class EventsWorkerStore(SQLBaseStore): ) if because: - # It's fine to do add the event directly, since get_pdu_json - # will serialise this field correctly - redacted_event.unsigned["redacted_because"] = because - - # Starting in room version v3, some redactions need to be - # rechecked if we didn't have the redacted event at the - # time, so we recheck on read instead. - if because.internal_metadata.need_to_check_redaction(): - expected_domain = get_domain_from_id(original_ev.sender) - if get_domain_from_id(because.sender) == expected_domain: - # This redaction event is allowed. Mark as not needing a - # recheck. - because.internal_metadata.recheck_redaction = False - else: - # Senders don't match, so the event isn't actually redacted - redacted_event = None - if because.room_id != original_ev.room_id: redacted_event = None - - if original_ev.type == EventTypes.Redaction: - redacted_event = None + else: + # It's fine to do add the event directly, since get_pdu_json + # will serialise this field correctly + redacted_event.unsigned["redacted_because"] = because + + # Starting in room version v3, some redactions need to be + # rechecked if we didn't have the redacted event at the + # time, so we recheck on read instead. + if because.internal_metadata.need_to_check_redaction(): + expected_domain = get_domain_from_id(original_ev.sender) + if get_domain_from_id(because.sender) == expected_domain: + # This redaction event is allowed. Mark as not needing a + # recheck. + because.internal_metadata.recheck_redaction = False + else: + # Senders don't match, so the event isn't actually + # redacted + redacted_event = None cache_entry = _EventCacheEntry( event=original_ev, redacted_event=redacted_event -- cgit 1.5.1 From 8ced9a2f58783ef9dd93e9313c24114f7924e771 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 30 Jul 2019 15:55:18 +0200 Subject: Don't make the checks depend on recheck_redaction --- synapse/storage/events_worker.py | 43 +++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index cbb2529d5e..b5604cbdef 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -255,6 +255,29 @@ class EventsWorkerStore(SQLBaseStore): # didn't have the redacted event at the time, so we recheck on read # instead. if not allow_rejected and entry.event.type == EventTypes.Redaction: + orig_event_info = yield self._simple_select_one( + table="events", + keyvalues={"event_id": entry.event.redacts}, + retcols=["sender", "room_id", "type"], + allow_none=True, + ) + + if not orig_event_info: + # We don't have the event that is being redacted, so we + # assume that the event isn't authorized for now. (If we + # later receive the event, then we will always redact + # it anyway, since we have this redaction) + continue + + if orig_event_info["room_id"] != entry.event.room_id: + # Don't process redactions if the redacted event doesn't belong to the + # redaction's room. + continue + + if orig_event_info["type"] == EventTypes.Redaction: + # Don't process redactions of redactions. + continue + if entry.event.internal_metadata.need_to_check_redaction(): # XXX: we need to avoid calling get_event here. # @@ -277,26 +300,6 @@ class EventsWorkerStore(SQLBaseStore): # 2. have _get_event_from_row just call the first half of # that - orig_event_info = yield self._simple_select_one( - table="events", - keyvalues={"event_id": entry.event.redacts}, - retcols=["sender", "room_id", "type"], - allow_none=True, - ) - - if not orig_event_info: - # We don't have the event that is being redacted, so we - # assume that the event isn't authorized for now. (If we - # later receive the event, then we will always redact - # it anyway, since we have this redaction) - continue - - if orig_event_info["room_id"] != entry.event.room_id: - continue - - if orig_event_info["type"] == EventTypes.Redaction: - continue - expected_domain = get_domain_from_id(entry.event.sender) if ( get_domain_from_id(orig_event_info["sender"]) == expected_domain -- cgit 1.5.1 From 0fda4e2e50b7ee33458cba58085147bc459453fe Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 30 Jul 2019 15:56:02 +0200 Subject: Should now work, unless we can't find the redaction event which happens for some reason (need to investigate) --- synapse/storage/events_worker.py | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index b5604cbdef..2983aec55b 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -559,26 +559,26 @@ class EventsWorkerStore(SQLBaseStore): ) if because: + # It's fine to do add the event directly, since get_pdu_json + # will serialise this field correctly + redacted_event.unsigned["redacted_because"] = because + + # Starting in room version v3, some redactions need to be + # rechecked if we didn't have the redacted event at the + # time, so we recheck on read instead. + if because.internal_metadata.need_to_check_redaction(): + expected_domain = get_domain_from_id(original_ev.sender) + if get_domain_from_id(because.sender) == expected_domain: + # This redaction event is allowed. Mark as not needing a + # recheck. + because.internal_metadata.recheck_redaction = False + else: + # Senders don't match, so the event isn't actually + # redacted + redacted_event = None + if because.room_id != original_ev.room_id: redacted_event = None - else: - # It's fine to do add the event directly, since get_pdu_json - # will serialise this field correctly - redacted_event.unsigned["redacted_because"] = because - - # Starting in room version v3, some redactions need to be - # rechecked if we didn't have the redacted event at the - # time, so we recheck on read instead. - if because.internal_metadata.need_to_check_redaction(): - expected_domain = get_domain_from_id(original_ev.sender) - if get_domain_from_id(because.sender) == expected_domain: - # This redaction event is allowed. Mark as not needing a - # recheck. - because.internal_metadata.recheck_redaction = False - else: - # Senders don't match, so the event isn't actually - # redacted - redacted_event = None cache_entry = _EventCacheEntry( event=original_ev, redacted_event=redacted_event -- cgit 1.5.1 From c4e56a8ee96ad721a8be6e2448660bf0d93f91f0 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 31 Jul 2019 15:11:27 +0200 Subject: Ignore invalid redactions in _get_event_from_row --- synapse/storage/events_worker.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index 2983aec55b..b771cf98ce 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -579,6 +579,11 @@ class EventsWorkerStore(SQLBaseStore): if because.room_id != original_ev.room_id: redacted_event = None + else: + # The lack of a redaction likely means that the redaction is invalid + # and therefore not returned by get_event, so it should be safe to + # just ignore it here. + redacted_event = None cache_entry = _EventCacheEntry( event=original_ev, redacted_event=redacted_event -- cgit 1.5.1 From 35ec13baab12c1800d8e6d5503643bdc76ef165f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 31 Jul 2019 15:48:57 +0200 Subject: Ignore redactions of redactions in get_events_as_list --- synapse/storage/events_worker.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py index b771cf98ce..5dc49822b5 100644 --- a/synapse/storage/events_worker.py +++ b/synapse/storage/events_worker.py @@ -272,10 +272,7 @@ class EventsWorkerStore(SQLBaseStore): if orig_event_info["room_id"] != entry.event.room_id: # Don't process redactions if the redacted event doesn't belong to the # redaction's room. - continue - - if orig_event_info["type"] == EventTypes.Redaction: - # Don't process redactions of redactions. + logger.info("Ignoring redation in another room.") continue if entry.event.internal_metadata.need_to_check_redaction(): -- cgit 1.5.1