From e65da2d18c4e05a81cc079fa4f389c6abebc863f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 15:40:59 +0000 Subject: [PATCH 1/7] Fix error when sending message into deleted room. When a room is deleted in Synapse we remove the event forward extremities in the room, so if (say a bot) tries to send a message into the room we error out due to not being able to calculate prev events for the new event *before* we check if the sender is in the room. Fixes #8094 --- synapse/handlers/message.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index da129ec16a4a..435e08ebeedc 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1169,6 +1169,17 @@ async def create_new_client_event( len(prev_event_ids), ) else: + # If we don't have any prev event IDs specified then we need to + # check that the host is in the room (as otherwise populating the + # prev events will fail), at which point we may as well check the + # local user is in the room. + user_id = requester.user.to_string() + is_user_in_room = await self.store.check_local_user_in_room( + requester.user.to_string(), builder.room_id + ) + if not is_user_in_room: + raise AuthError(403, f"User {user_id} not in room {builder.room_id}") + prev_event_ids = await self.store.get_prev_events_for_room(builder.room_id) # Do a quick sanity check here, rather than waiting until we've created the From c7e11d359400210dbe57565b71fafe2b7bc16d3a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 15:43:05 +0000 Subject: [PATCH 2/7] Newsfile --- changelog.d/15235.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15235.bugfix diff --git a/changelog.d/15235.bugfix b/changelog.d/15235.bugfix new file mode 100644 index 000000000000..1d8b71988345 --- /dev/null +++ b/changelog.d/15235.bugfix @@ -0,0 +1 @@ +Fix error when sending message into deleted room. From c629d80b927ec820f0a5c4710eb00a13158246ad Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 15:43:21 +0000 Subject: [PATCH 3/7] Newsfile --- changelog.d/15235.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/15235.bugfix b/changelog.d/15235.bugfix index 1d8b71988345..e6a6bb1b9d08 100644 --- a/changelog.d/15235.bugfix +++ b/changelog.d/15235.bugfix @@ -1 +1 @@ -Fix error when sending message into deleted room. +Fix long-standing error when sending message into deleted room. From f1be709041329deaaceb535ff3ab6c36fe68b343 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 15:49:02 +0000 Subject: [PATCH 4/7] Typing --- synapse/handlers/message.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 435e08ebeedc..e3c8e3dbc71f 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1173,12 +1173,13 @@ async def create_new_client_event( # check that the host is in the room (as otherwise populating the # prev events will fail), at which point we may as well check the # local user is in the room. - user_id = requester.user.to_string() is_user_in_room = await self.store.check_local_user_in_room( - requester.user.to_string(), builder.room_id + builder.sender, builder.room_id ) if not is_user_in_room: - raise AuthError(403, f"User {user_id} not in room {builder.room_id}") + raise AuthError( + 403, f"User {builder.sender} not in room {builder.room_id}" + ) prev_event_ids = await self.store.get_prev_events_for_room(builder.room_id) From 297f5960c8787e611f9ed241411559b8f5aed9bb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 17:06:16 +0000 Subject: [PATCH 5/7] Fixup --- synapse/handlers/message.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index e3c8e3dbc71f..a8f852d5c25a 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -987,10 +987,11 @@ async def create_and_send_nonmember_event( # a situation where event persistence can't keep up, causing # extremities to pile up, which in turn leads to state resolution # taking longer. - async with self.limiter.queue(event_dict["room_id"]): + room_id = event_dict["room_id"] + async with self.limiter.queue(room_id): if txn_id: event = await self.get_event_from_transaction( - requester, txn_id, event_dict["room_id"] + requester, txn_id, room_id ) if event: # we know it was persisted, so must have a stream ordering @@ -999,6 +1000,14 @@ async def create_and_send_nonmember_event( event, event.internal_metadata.stream_ordering, ) + # If we don't have any prev event IDs specified then we need to + # check that the host is in the room (as otherwise populating the + # prev events will fail), at which point we may as well check the + # local user is in the room. + user_id = requester.user.to_string() + is_user_in_room = await self.store.check_local_user_in_room(user_id, room_id) + if not is_user_in_room: + raise AuthError(403, f"User {user_id} not in room {room_id}") # Try several times, it could fail with PartialStateConflictError # in handle_new_client_event, cf comment in except block. @@ -1169,18 +1178,6 @@ async def create_new_client_event( len(prev_event_ids), ) else: - # If we don't have any prev event IDs specified then we need to - # check that the host is in the room (as otherwise populating the - # prev events will fail), at which point we may as well check the - # local user is in the room. - is_user_in_room = await self.store.check_local_user_in_room( - builder.sender, builder.room_id - ) - if not is_user_in_room: - raise AuthError( - 403, f"User {builder.sender} not in room {builder.room_id}" - ) - prev_event_ids = await self.store.get_prev_events_for_room(builder.room_id) # Do a quick sanity check here, rather than waiting until we've created the From 8568e79d1b928543ec008f27fb2ffc8a75e7b943 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 9 Mar 2023 17:21:26 +0000 Subject: [PATCH 6/7] Fixup --- synapse/handlers/message.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index a8f852d5c25a..4c75433a635f 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1000,14 +1000,18 @@ async def create_and_send_nonmember_event( event, event.internal_metadata.stream_ordering, ) + # If we don't have any prev event IDs specified then we need to # check that the host is in the room (as otherwise populating the # prev events will fail), at which point we may as well check the # local user is in the room. - user_id = requester.user.to_string() - is_user_in_room = await self.store.check_local_user_in_room(user_id, room_id) - if not is_user_in_room: - raise AuthError(403, f"User {user_id} not in room {room_id}") + if not prev_event_ids: + user_id = requester.user.to_string() + is_user_in_room = await self.store.check_local_user_in_room( + user_id, room_id + ) + if not is_user_in_room: + raise AuthError(403, f"User {user_id} not in room {room_id}") # Try several times, it could fail with PartialStateConflictError # in handle_new_client_event, cf comment in except block. From 6d5cbfeb9fd8153edc447b57f04d45bc060257b1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 20 Mar 2023 17:51:22 +0000 Subject: [PATCH 7/7] Add test --- tests/rest/admin/test_room.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 9dbb778679d1..eb50086c508e 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -402,6 +402,21 @@ def test_shutdown_room_block_peek(self) -> None: # Assert we can no longer peek into the room self._assert_peek(self.room_id, expect_code=403) + def test_room_delete_send(self) -> None: + """Test that sending into a deleted room returns a 403""" + channel = self.make_request( + "DELETE", + self.url, + content={}, + access_token=self.admin_user_tok, + ) + + self.assertEqual(200, channel.code, msg=channel.json_body) + + self.helper.send( + self.room_id, "test message", expect_code=403, tok=self.other_user_tok + ) + def _is_blocked(self, room_id: str, expect: bool = True) -> None: """Assert that the room is blocked or not""" d = self.store.is_room_blocked(room_id)