From 594cd5f9fd7b29cee22f878d4df5cb5818ba90e9 Mon Sep 17 00:00:00 2001
From: Gordan Trevis <GitHub@gordantrevis.me>
Date: Thu, 29 Aug 2024 16:34:29 +0200
Subject: [PATCH] Fix Internal Server Error for Non-Local Users in Room Actions
 (#17607)

---
 changelog.d/17607.bugfix                     |  1 +
 synapse/storage/databases/main/roommember.py |  8 +++----
 tests/handlers/test_room_member.py           | 22 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100644 changelog.d/17607.bugfix

diff --git a/changelog.d/17607.bugfix b/changelog.d/17607.bugfix
new file mode 100644
index 000000000..74201135b
--- /dev/null
+++ b/changelog.d/17607.bugfix
@@ -0,0 +1 @@
+Return `400 M_BAD_JSON` upon attempting to complete various room actions with a non-local user ID and unknown room ID, rather than an internal server error.
\ No newline at end of file
diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py
index 1d9f0f52e..71baf5766 100644
--- a/synapse/storage/databases/main/roommember.py
+++ b/synapse/storage/databases/main/roommember.py
@@ -19,6 +19,7 @@
 #
 #
 import logging
+from http import HTTPStatus
 from typing import (
     TYPE_CHECKING,
     AbstractSet,
@@ -39,6 +40,7 @@ from typing import (
 import attr
 
 from synapse.api.constants import EventTypes, Membership
+from synapse.api.errors import Codes, SynapseError
 from synapse.logging.opentracing import trace
 from synapse.metrics import LaterGauge
 from synapse.metrics.background_process_metrics import wrap_as_background_process
@@ -631,10 +633,8 @@ class RoomMemberWorkerStore(EventsWorkerStore, CacheInvalidationWorkerStore):
         """
         # Paranoia check.
         if not self.hs.is_mine_id(user_id):
-            raise Exception(
-                "Cannot call 'get_local_current_membership_for_user_in_room' on "
-                "non-local user %s" % (user_id,),
-            )
+            message = f"Provided user_id {user_id} is a non-local user"
+            raise SynapseError(HTTPStatus.BAD_REQUEST, message, errcode=Codes.BAD_JSON)
 
         results = cast(
             Optional[Tuple[str, str]],
diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py
index 213a66ed1..acb403cb2 100644
--- a/tests/handlers/test_room_member.py
+++ b/tests/handlers/test_room_member.py
@@ -6,7 +6,7 @@ import synapse.rest.admin
 import synapse.rest.client.login
 import synapse.rest.client.room
 from synapse.api.constants import EventTypes, Membership
-from synapse.api.errors import LimitExceededError, SynapseError
+from synapse.api.errors import Codes, LimitExceededError, SynapseError
 from synapse.crypto.event_signing import add_hashes_and_signatures
 from synapse.events import FrozenEventV3
 from synapse.federation.federation_client import SendJoinResult
@@ -383,6 +383,26 @@ class RoomMemberMasterHandlerTestCase(HomeserverTestCase):
         """Tests that a user cannot not forgets a room that has not left."""
         self.get_failure(self.handler.forget(self.alice_ID, self.room_id), SynapseError)
 
+    def test_nonlocal_room_user_action(self) -> None:
+        """
+        Test that non-local user ids cannot perform room actions through
+        this homeserver.
+        """
+        alien_user_id = UserID.from_string("@cheeky_monkey:matrix.org")
+        bad_room_id = f"{self.room_id}+BAD_ID"
+
+        exc = self.get_failure(
+            self.handler.update_membership(
+                create_requester(self.alice),
+                alien_user_id,
+                bad_room_id,
+                "unban",
+            ),
+            SynapseError,
+        ).value
+
+        self.assertEqual(exc.errcode, Codes.BAD_JSON)
+
     def test_rejoin_forgotten_by_user(self) -> None:
         """Test that a user that has forgotten a room can do a re-join.
         The room was not forgotten from the local server.