Skip to content

Commit 4daa533

Browse files
MadLittleModssandhose
authored andcommitted
Sliding Sync: Fix state leaking on incremental sync
1 parent f3fd685 commit 4daa533

File tree

3 files changed

+226
-4
lines changed

3 files changed

+226
-4
lines changed

synapse/handlers/sliding_sync/__init__.py

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
trace,
4040
)
4141
from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary
42+
from synapse.storage.databases.main.state_deltas import StateDelta
4243
from synapse.storage.databases.main.stream import PaginateFunction
4344
from synapse.storage.roommember import (
4445
MemberSummary,
@@ -48,6 +49,7 @@
4849
MutableStateMap,
4950
PersistedEventPosition,
5051
Requester,
52+
RoomStreamToken,
5153
SlidingSyncStreamToken,
5254
StateMap,
5355
StrCollection,
@@ -470,6 +472,64 @@ async def get_current_state_at(
470472

471473
return state_map
472474

475+
@trace
476+
async def get_current_state_deltas_for_room(
477+
self,
478+
room_id: str,
479+
room_membership_for_user_at_to_token: RoomsForUserType,
480+
from_token: RoomStreamToken,
481+
to_token: RoomStreamToken,
482+
) -> List[StateDelta]:
483+
"""
484+
Get the state deltas between two tokens taking into account the user's
485+
membership. If the user is LEAVE/BAN, we will only get the state deltas up to
486+
their LEAVE/BAN event (inclusive).
487+
488+
(> `from_token` and <= `to_token`)
489+
"""
490+
membership = room_membership_for_user_at_to_token.membership
491+
# We don't know how to handle `membership` values other than these. The
492+
# code below would need to be updated.
493+
assert membership in (
494+
Membership.JOIN,
495+
Membership.INVITE,
496+
Membership.KNOCK,
497+
Membership.LEAVE,
498+
Membership.BAN,
499+
)
500+
501+
# People shouldn't see past their leave/ban event
502+
if membership in (
503+
Membership.LEAVE,
504+
Membership.BAN,
505+
):
506+
to_bound = (
507+
room_membership_for_user_at_to_token.event_pos.to_room_stream_token()
508+
)
509+
# If we are participating in the room, we can get the latest current state in
510+
# the room
511+
elif membership == Membership.JOIN:
512+
to_bound = to_token
513+
# We can only rely on the stripped state included in the invite/knock event
514+
# itself so there will never be any state deltas to send down.
515+
elif membership in (Membership.INVITE, Membership.KNOCK):
516+
return []
517+
else:
518+
# We don't know how to handle this type of membership yet
519+
#
520+
# FIXME: We should use `assert_never` here but for some reason
521+
# the exhaustive matching doesn't recognize the `Never` here.
522+
# assert_never(membership)
523+
raise AssertionError(
524+
f"Unexpected membership {membership} that we don't know how to handle yet"
525+
)
526+
527+
return await self.store.get_current_state_deltas_for_room(
528+
room_id=room_id,
529+
from_token=from_token,
530+
to_token=to_bound,
531+
)
532+
473533
@trace
474534
async def get_room_sync_data(
475535
self,
@@ -790,8 +850,9 @@ async def get_room_sync_data(
790850
# TODO: Limit the number of state events we're about to send down
791851
# the room, if its too many we should change this to an
792852
# `initial=True`?
793-
deltas = await self.store.get_current_state_deltas_for_room(
853+
deltas = await self.get_current_state_deltas_for_room(
794854
room_id=room_id,
855+
room_membership_for_user_at_to_token=room_membership_for_user_at_to_token,
795856
from_token=from_bound,
796857
to_token=to_token.room_key,
797858
)

synapse/storage/databases/main/state_deltas.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,13 @@ async def get_current_state_deltas_for_room(
243243
244244
(> `from_token` and <= `to_token`)
245245
"""
246+
# We can bail early if the `from_token` is after the `to_token`
247+
if (
248+
to_token is not None
249+
and from_token is not None
250+
and to_token.is_before_or_eq(from_token)
251+
):
252+
return []
246253

247254
if (
248255
from_token is not None

tests/rest/client/sliding_sync/test_rooms_required_state.py

Lines changed: 157 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -751,9 +751,10 @@ def test_rooms_required_state_me(self) -> None:
751751
self.assertIsNone(response_body["rooms"][room_id1].get("invite_state"))
752752

753753
@parameterized.expand([(Membership.LEAVE,), (Membership.BAN,)])
754-
def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None:
754+
def test_rooms_required_state_leave_ban_initial(self, stop_membership: str) -> None:
755755
"""
756-
Test `rooms.required_state` should not return state past a leave/ban event.
756+
Test `rooms.required_state` should not return state past a leave/ban event when
757+
it's the first "initial" time the room is being sent down the connection.
757758
"""
758759
user1_id = self.register_user("user1", "pass")
759760
user1_tok = self.login(user1_id, "pass")
@@ -788,6 +789,13 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None:
788789
body={"foo": "bar"},
789790
tok=user2_tok,
790791
)
792+
self.helper.send_state(
793+
room_id1,
794+
event_type="org.matrix.bar_state",
795+
state_key="",
796+
body={"bar": "bar"},
797+
tok=user2_tok,
798+
)
791799

792800
if stop_membership == Membership.LEAVE:
793801
# User 1 leaves
@@ -796,6 +804,8 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None:
796804
# User 1 is banned
797805
self.helper.ban(room_id1, src=user2_id, targ=user1_id, tok=user2_tok)
798806

807+
# Get the state_map before we change the state as this is the final state we
808+
# expect User1 to be able to see
799809
state_map = self.get_success(
800810
self.storage_controllers.state.get_current_state(room_id1)
801811
)
@@ -808,12 +818,36 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None:
808818
body={"foo": "qux"},
809819
tok=user2_tok,
810820
)
821+
self.helper.send_state(
822+
room_id1,
823+
event_type="org.matrix.bar_state",
824+
state_key="",
825+
body={"bar": "qux"},
826+
tok=user2_tok,
827+
)
811828
self.helper.leave(room_id1, user3_id, tok=user3_tok)
812829

813830
# Make an incremental Sliding Sync request
831+
#
832+
# Also expand the required state to include the `org.matrix.bar_state` event.
833+
# This is just an extra complication of the test.
834+
sync_body = {
835+
"lists": {
836+
"foo-list": {
837+
"ranges": [[0, 1]],
838+
"required_state": [
839+
[EventTypes.Create, ""],
840+
[EventTypes.Member, "*"],
841+
["org.matrix.foo_state", ""],
842+
["org.matrix.bar_state", ""],
843+
],
844+
"timeline_limit": 3,
845+
}
846+
}
847+
}
814848
response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
815849

816-
# Only user2 and user3 sent events in the 3 events we see in the `timeline`
850+
# We should only see the state up to the leave/ban event
817851
self._assertRequiredStateIncludes(
818852
response_body["rooms"][room_id1]["required_state"],
819853
{
@@ -822,6 +856,126 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None:
822856
state_map[(EventTypes.Member, user2_id)],
823857
state_map[(EventTypes.Member, user3_id)],
824858
state_map[("org.matrix.foo_state", "")],
859+
state_map[("org.matrix.bar_state", "")],
860+
},
861+
exact=True,
862+
)
863+
self.assertIsNone(response_body["rooms"][room_id1].get("invite_state"))
864+
865+
@parameterized.expand([(Membership.LEAVE,), (Membership.BAN,)])
866+
def test_rooms_required_state_leave_ban_incremental(
867+
self, stop_membership: str
868+
) -> None:
869+
"""
870+
Test `rooms.required_state` should not return state past a leave/ban event on
871+
incremental sync.
872+
"""
873+
user1_id = self.register_user("user1", "pass")
874+
user1_tok = self.login(user1_id, "pass")
875+
user2_id = self.register_user("user2", "pass")
876+
user2_tok = self.login(user2_id, "pass")
877+
user3_id = self.register_user("user3", "pass")
878+
user3_tok = self.login(user3_id, "pass")
879+
880+
room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok)
881+
self.helper.join(room_id1, user1_id, tok=user1_tok)
882+
self.helper.join(room_id1, user3_id, tok=user3_tok)
883+
884+
self.helper.send_state(
885+
room_id1,
886+
event_type="org.matrix.foo_state",
887+
state_key="",
888+
body={"foo": "bar"},
889+
tok=user2_tok,
890+
)
891+
self.helper.send_state(
892+
room_id1,
893+
event_type="org.matrix.bar_state",
894+
state_key="",
895+
body={"bar": "bar"},
896+
tok=user2_tok,
897+
)
898+
899+
sync_body = {
900+
"lists": {
901+
"foo-list": {
902+
"ranges": [[0, 1]],
903+
"required_state": [
904+
[EventTypes.Create, ""],
905+
[EventTypes.Member, "*"],
906+
["org.matrix.foo_state", ""],
907+
],
908+
"timeline_limit": 3,
909+
}
910+
}
911+
}
912+
_, from_token = self.do_sync(sync_body, tok=user1_tok)
913+
914+
if stop_membership == Membership.LEAVE:
915+
# User 1 leaves
916+
self.helper.leave(room_id1, user1_id, tok=user1_tok)
917+
elif stop_membership == Membership.BAN:
918+
# User 1 is banned
919+
self.helper.ban(room_id1, src=user2_id, targ=user1_id, tok=user2_tok)
920+
921+
# Get the state_map before we change the state as this is the final state we
922+
# expect User1 to be able to see
923+
state_map = self.get_success(
924+
self.storage_controllers.state.get_current_state(room_id1)
925+
)
926+
927+
# Change the state after user 1 leaves
928+
self.helper.send_state(
929+
room_id1,
930+
event_type="org.matrix.foo_state",
931+
state_key="",
932+
body={"foo": "qux"},
933+
tok=user2_tok,
934+
)
935+
self.helper.send_state(
936+
room_id1,
937+
event_type="org.matrix.bar_state",
938+
state_key="",
939+
body={"bar": "qux"},
940+
tok=user2_tok,
941+
)
942+
self.helper.leave(room_id1, user3_id, tok=user3_tok)
943+
944+
# Make an incremental Sliding Sync request
945+
#
946+
# Also expand the required state to include the `org.matrix.bar_state` event.
947+
# This is just an extra complication of the test.
948+
sync_body = {
949+
"lists": {
950+
"foo-list": {
951+
"ranges": [[0, 1]],
952+
"required_state": [
953+
[EventTypes.Create, ""],
954+
[EventTypes.Member, "*"],
955+
["org.matrix.foo_state", ""],
956+
["org.matrix.bar_state", ""],
957+
],
958+
"timeline_limit": 3,
959+
}
960+
}
961+
}
962+
response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok)
963+
964+
# User1 should only see the state up to the leave/ban event
965+
self._assertRequiredStateIncludes(
966+
response_body["rooms"][room_id1]["required_state"],
967+
{
968+
# User1 should see their leave/ban membership
969+
state_map[(EventTypes.Member, user1_id)],
970+
state_map[("org.matrix.bar_state", "")],
971+
# The commented out state events were already returned in the initial
972+
# sync so we shouldn't see them again on the incremental sync. And we
973+
# shouldn't see the state events that changed after the leave/ban event.
974+
#
975+
# state_map[(EventTypes.Create, "")],
976+
# state_map[(EventTypes.Member, user2_id)],
977+
# state_map[(EventTypes.Member, user3_id)],
978+
# state_map[("org.matrix.foo_state", "")],
825979
},
826980
exact=True,
827981
)

0 commit comments

Comments
 (0)