From 69ce55c51082d03e549863f2149b4cf10cb1de19 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 15:21:25 +0000 Subject: [PATCH 1/7] Don't filter out dummy events when we're checking the visibility of state --- synapse/handlers/message.py | 2 +- synapse/visibility.py | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 0c84c6cec4de..b743fc2dcc16 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -160,7 +160,7 @@ def get_state_events( raise NotFoundError("Can't find event for token %s" % (at_token,)) visible_events = yield filter_events_for_client( - self.storage, user_id, last_events, apply_retention_policies=False + self.storage, user_id, last_events, filter_send_to_client=False ) event = last_events[0] diff --git a/synapse/visibility.py b/synapse/visibility.py index a48a4f3dfe49..1d538b206d77 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -49,7 +49,7 @@ def filter_events_for_client( events, is_peeking=False, always_include_ids=frozenset(), - apply_retention_policies=True, + filter_send_to_client=True, ): """ Check which events a user is allowed to see. If the user can see the event but its @@ -65,10 +65,9 @@ def filter_events_for_client( events always_include_ids (set(event_id)): set of event ids to specifically include (unless sender is ignored) - apply_retention_policies (bool): Whether to filter out events that's older than - allowed by the room's retention policy. Useful when this function is called - to e.g. check whether a user should be allowed to see the state at a given - event rather than to know if it should send an event to a user's client(s). + filter_send_to_client (bool): Whether we're checking an event that's going to be + sent to a client. This might not always be the case since this function can + also be called to check whether a user can see the state at a given point. Returns: Deferred[list[synapse.events.EventBase]] @@ -96,7 +95,7 @@ def filter_events_for_client( erased_senders = yield storage.main.are_users_erased((e.sender for e in events)) - if apply_retention_policies: + if not filter_send_to_client: room_ids = {e.room_id for e in events} retention_policies = {} @@ -119,7 +118,7 @@ def allowed(event): the original event if they can see it as normal. """ - if event.type == "org.matrix.dummy_event": + if event.type == "org.matrix.dummy_event" and filter_send_to_client: return None if not event.is_state() and event.sender in ignore_list: @@ -134,7 +133,7 @@ def allowed(event): # Don't try to apply the room's retention policy if the event is a state event, as # MSC1763 states that retention is only considered for non-state events. - if apply_retention_policies and not event.is_state(): + if filter_send_to_client and not event.is_state(): retention_policy = retention_policies[event.room_id] max_lifetime = retention_policy.get("max_lifetime") From 9c0775e86ab39b193670723927a1caf67f6bfc11 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 15:32:07 +0000 Subject: [PATCH 2/7] Fix condition --- synapse/visibility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index 1d538b206d77..d0b2241e4843 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -95,7 +95,7 @@ def filter_events_for_client( erased_senders = yield storage.main.are_users_erased((e.sender for e in events)) - if not filter_send_to_client: + if filter_send_to_client: room_ids = {e.room_id for e in events} retention_policies = {} From 2dce68c65110d4fe41efcc7150c9c6300ac71d2c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 17:04:18 +0000 Subject: [PATCH 3/7] Also don't filter out events sent by ignored users when checking state visibility --- synapse/visibility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index d0b2241e4843..82a213242780 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -121,7 +121,7 @@ def allowed(event): if event.type == "org.matrix.dummy_event" and filter_send_to_client: return None - if not event.is_state() and event.sender in ignore_list: + if not event.is_state() and event.sender in ignore_list and filter_send_to_client: return None # Until MSC2261 has landed we can't redact malicious alias events, so for From 1cde4cf3f15413b941c699ac5048c464a49137a4 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 18:03:56 +0000 Subject: [PATCH 4/7] Changelog --- changelog.d/7066.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/7066.bugfix diff --git a/changelog.d/7066.bugfix b/changelog.d/7066.bugfix new file mode 100644 index 000000000000..94bb09628764 --- /dev/null +++ b/changelog.d/7066.bugfix @@ -0,0 +1 @@ +Fix a bug that would cause Synapse to respond with an error about event visibility if a client tried to request the state of a room at a given token. From e38c44b418328e79e8da3e8ed259ee51d2f1c215 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 18:06:07 +0000 Subject: [PATCH 5/7] Lint --- synapse/visibility.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index 82a213242780..fce5855413d2 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -121,7 +121,11 @@ def allowed(event): if event.type == "org.matrix.dummy_event" and filter_send_to_client: return None - if not event.is_state() and event.sender in ignore_list and filter_send_to_client: + if ( + not event.is_state() + and event.sender in ignore_list + and filter_send_to_client + ): return None # Until MSC2261 has landed we can't redact malicious alias events, so for From 37a9873f6360a8e6f243c3d3d081ff7abc0f9da1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 18:43:41 +0000 Subject: [PATCH 6/7] Also don't fail on aliases events in this case --- synapse/visibility.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index fce5855413d2..56603eb276b9 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -132,7 +132,7 @@ def allowed(event): # now we temporarily filter out m.room.aliases entirely to mitigate # abuse, while we spec a better solution to advertising aliases # on rooms. - if event.type == EventTypes.Aliases: + if event.type == EventTypes.Aliases and filter_send_to_client: return None # Don't try to apply the room's retention policy if the event is a state event, as From 8120a238a465de576ad4d171e3072b28e5df32ac Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 11 Mar 2020 18:49:41 +0000 Subject: [PATCH 7/7] Refactor a bit --- synapse/visibility.py | 58 ++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/synapse/visibility.py b/synapse/visibility.py index 56603eb276b9..bab41182b9a0 100644 --- a/synapse/visibility.py +++ b/synapse/visibility.py @@ -118,34 +118,36 @@ def allowed(event): the original event if they can see it as normal. """ - if event.type == "org.matrix.dummy_event" and filter_send_to_client: - return None - - if ( - not event.is_state() - and event.sender in ignore_list - and filter_send_to_client - ): - return None - - # Until MSC2261 has landed we can't redact malicious alias events, so for - # now we temporarily filter out m.room.aliases entirely to mitigate - # abuse, while we spec a better solution to advertising aliases - # on rooms. - if event.type == EventTypes.Aliases and filter_send_to_client: - return None - - # Don't try to apply the room's retention policy if the event is a state event, as - # MSC1763 states that retention is only considered for non-state events. - if filter_send_to_client and not event.is_state(): - retention_policy = retention_policies[event.room_id] - max_lifetime = retention_policy.get("max_lifetime") - - if max_lifetime is not None: - oldest_allowed_ts = storage.main.clock.time_msec() - max_lifetime - - if event.origin_server_ts < oldest_allowed_ts: - return None + # Only run some checks if these events aren't about to be sent to clients. This is + # because, if this is not the case, we're probably only checking if the users can + # see events in the room at that point in the DAG, and that shouldn't be decided + # on those checks. + if filter_send_to_client: + if event.type == "org.matrix.dummy_event": + return None + + if not event.is_state() and event.sender in ignore_list: + return None + + # Until MSC2261 has landed we can't redact malicious alias events, so for + # now we temporarily filter out m.room.aliases entirely to mitigate + # abuse, while we spec a better solution to advertising aliases + # on rooms. + if event.type == EventTypes.Aliases: + return None + + # Don't try to apply the room's retention policy if the event is a state + # event, as MSC1763 states that retention is only considered for non-state + # events. + if not event.is_state(): + retention_policy = retention_policies[event.room_id] + max_lifetime = retention_policy.get("max_lifetime") + + if max_lifetime is not None: + oldest_allowed_ts = storage.main.clock.time_msec() - max_lifetime + + if event.origin_server_ts < oldest_allowed_ts: + return None if event.event_id in always_include_ids: return event