From c186fda08621fd7bb91bd550ddd09c1d0f72d89b Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 18 Nov 2020 11:50:36 +0000 Subject: [PATCH 1/3] Consistently use room_id from federation request body Some federation APIs have a redundant `room_id` path param (see https://github.com/matrix-org/matrix-doc/issues/2330). We should make sure we consistently use either the path param or the body param, and the body param is easier. --- changelog.d/8776.bugfix | 1 + synapse/federation/federation_server.py | 21 +++++++++------------ synapse/federation/transport/server.py | 20 +++++++++----------- synapse/handlers/federation.py | 10 +++++----- 4 files changed, 24 insertions(+), 28 deletions(-) create mode 100644 changelog.d/8776.bugfix diff --git a/changelog.d/8776.bugfix b/changelog.d/8776.bugfix new file mode 100644 index 000000000000..dd7ebbeb865f --- /dev/null +++ b/changelog.d/8776.bugfix @@ -0,0 +1 @@ +Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 23278e36b733..4ad59f1fdff7 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -49,6 +49,7 @@ from synapse.federation.persistence import TransactionActions from synapse.federation.units import Edu, Transaction from synapse.http.endpoint import parse_server_name +from synapse.http.servlet import assert_params_in_dict from synapse.logging.context import ( make_deferred_yieldable, nested_logging_context, @@ -514,11 +515,12 @@ async def on_invite_request( return {"event": ret_pdu.get_pdu_json(time_now)} async def on_send_join_request( - self, origin: str, content: JsonDict, room_id: str + self, origin: str, content: JsonDict ) -> Dict[str, Any]: logger.debug("on_send_join_request: content: %s", content) - room_version = await self.store.get_room_version(room_id) + assert_params_in_dict(content, ["room_id"]) + room_version = await self.store.get_room_version(content["room_id"]) pdu = event_from_pdu_json(content, room_version) origin_host, _ = parse_server_name(origin) @@ -547,12 +549,11 @@ async def on_make_leave_request( time_now = self._clock.time_msec() return {"event": pdu.get_pdu_json(time_now), "room_version": room_version} - async def on_send_leave_request( - self, origin: str, content: JsonDict, room_id: str - ) -> dict: + async def on_send_leave_request(self, origin: str, content: JsonDict) -> dict: logger.debug("on_send_leave_request: content: %s", content) - room_version = await self.store.get_room_version(room_id) + assert_params_in_dict(content, ["room_id"]) + room_version = await self.store.get_room_version(content["room_id"]) pdu = event_from_pdu_json(content, room_version) origin_host, _ = parse_server_name(origin) @@ -748,12 +749,8 @@ async def exchange_third_party_invite( ) return ret - async def on_exchange_third_party_invite_request( - self, room_id: str, event_dict: Dict - ): - ret = await self.handler.on_exchange_third_party_invite_request( - room_id, event_dict - ) + async def on_exchange_third_party_invite_request(self, event_dict: Dict): + ret = await self.handler.on_exchange_third_party_invite_request(event_dict) return ret async def check_server_matches_acl(self, server_name: str, room_id: str): diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index a0933fae88c8..2ed796c58465 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -528,7 +528,7 @@ class FederationV1SendLeaveServlet(BaseFederationServlet): PATH = "/send_leave/(?P[^/]*)/(?P[^/]*)" async def on_PUT(self, origin, content, query, room_id, event_id): - content = await self.handler.on_send_leave_request(origin, content, room_id) + content = await self.handler.on_send_leave_request(origin, content) return 200, (200, content) @@ -538,7 +538,7 @@ class FederationV2SendLeaveServlet(BaseFederationServlet): PREFIX = FEDERATION_V2_PREFIX async def on_PUT(self, origin, content, query, room_id, event_id): - content = await self.handler.on_send_leave_request(origin, content, room_id) + content = await self.handler.on_send_leave_request(origin, content) return 200, content @@ -550,24 +550,24 @@ async def on_GET(self, origin, content, query, context, event_id): class FederationV1SendJoinServlet(BaseFederationServlet): - PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" - async def on_PUT(self, origin, content, query, context, event_id): + async def on_PUT(self, origin, content, query, room_id, event_id): # TODO(paul): assert that context/event_id parsed from path actually # match those given in content - content = await self.handler.on_send_join_request(origin, content, context) + content = await self.handler.on_send_join_request(origin, content) return 200, (200, content) class FederationV2SendJoinServlet(BaseFederationServlet): - PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" PREFIX = FEDERATION_V2_PREFIX - async def on_PUT(self, origin, content, query, context, event_id): + async def on_PUT(self, origin, content, query, room_id, event_id): # TODO(paul): assert that context/event_id parsed from path actually # match those given in content - content = await self.handler.on_send_join_request(origin, content, context) + content = await self.handler.on_send_join_request(origin, content) return 200, content @@ -616,9 +616,7 @@ class FederationThirdPartyInviteExchangeServlet(BaseFederationServlet): PATH = "/exchange_third_party_invite/(?P[^/]*)" async def on_PUT(self, origin, content, query, room_id): - content = await self.handler.on_exchange_third_party_invite_request( - room_id, content - ) + content = await self.handler.on_exchange_third_party_invite_request(content) return 200, content diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 69bc5ba44db2..b9799090f784 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -55,6 +55,7 @@ from synapse.events.snapshot import EventContext from synapse.events.validator import EventValidator from synapse.handlers._base import BaseHandler +from synapse.http.servlet import assert_params_in_dict from synapse.logging.context import ( make_deferred_yieldable, nested_logging_context, @@ -2688,7 +2689,7 @@ async def exchange_third_party_invite( ) async def on_exchange_third_party_invite_request( - self, room_id: str, event_dict: JsonDict + self, event_dict: JsonDict ) -> None: """Handle an exchange_third_party_invite request from a remote server @@ -2696,12 +2697,11 @@ async def on_exchange_third_party_invite_request( into a normal m.room.member invite. Args: - room_id: The ID of the room. - - event_dict (dict[str, Any]): Dictionary containing the event body. + event_dict: Dictionary containing the event body. """ - room_version = await self.store.get_room_version_id(room_id) + assert_params_in_dict(event_dict, ["room_id"]) + room_version = await self.store.get_room_version_id(event_dict["room_id"]) # NB: event_dict has a particular specced format we might need to fudge # if we change event formats too much. From 83bdf0f007c6e991fb440846cc6a4eee4a4d2187 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 18 Nov 2020 13:11:04 +0000 Subject: [PATCH 2/3] fix broken test --- tests/handlers/test_federation.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index 9ef80fe5024b..bf866dacf3f2 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -59,7 +59,6 @@ def test_exchange_revoked_invite(self): ) d = self.handler.on_exchange_third_party_invite_request( - room_id=room_id, event_dict={ "type": EventTypes.Member, "room_id": room_id, From 4da6fd76201f3d9e6084dbc6ce6c7f98a5053c25 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 18 Nov 2020 16:37:11 +0000 Subject: [PATCH 3/3] Kill off some references to "context" Once upon a time, "rooms" were known as "contexts". I think this kills of the last references to "contexts". --- synapse/federation/federation_server.py | 2 +- synapse/federation/transport/server.py | 48 ++++++++++++------------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 4ad59f1fdff7..4b6ab470d059 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -392,7 +392,7 @@ async def _process_edu(edu_dict): TRANSACTION_CONCURRENCY_LIMIT, ) - async def on_context_state_request( + async def on_room_state_request( self, origin: str, room_id: str, event_id: str ) -> Tuple[int, Dict[str, Any]]: origin_host, _ = parse_server_name(origin) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 2ed796c58465..b53e7a20ecc1 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -440,13 +440,13 @@ async def on_GET(self, origin, content, query, event_id): class FederationStateV1Servlet(BaseFederationServlet): - PATH = "/state/(?P[^/]*)/?" + PATH = "/state/(?P[^/]*)/?" - # This is when someone asks for all data for a given context. - async def on_GET(self, origin, content, query, context): - return await self.handler.on_context_state_request( + # This is when someone asks for all data for a given room. + async def on_GET(self, origin, content, query, room_id): + return await self.handler.on_room_state_request( origin, - context, + room_id, parse_string_from_args(query, "event_id", None, required=False), ) @@ -463,16 +463,16 @@ async def on_GET(self, origin, content, query, room_id): class FederationBackfillServlet(BaseFederationServlet): - PATH = "/backfill/(?P[^/]*)/?" + PATH = "/backfill/(?P[^/]*)/?" - async def on_GET(self, origin, content, query, context): + async def on_GET(self, origin, content, query, room_id): versions = [x.decode("ascii") for x in query[b"v"]] limit = parse_integer_from_args(query, "limit", None) if not limit: return 400, {"error": "Did not include limit param"} - return await self.handler.on_backfill_request(origin, context, versions, limit) + return await self.handler.on_backfill_request(origin, room_id, versions, limit) class FederationQueryServlet(BaseFederationServlet): @@ -487,9 +487,9 @@ async def on_GET(self, origin, content, query, query_type): class FederationMakeJoinServlet(BaseFederationServlet): - PATH = "/make_join/(?P[^/]*)/(?P[^/]*)" + PATH = "/make_join/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, _content, query, context, user_id): + async def on_GET(self, origin, _content, query, room_id, user_id): """ Args: origin (unicode): The authenticated server_name of the calling server @@ -511,16 +511,16 @@ async def on_GET(self, origin, _content, query, context, user_id): supported_versions = ["1"] content = await self.handler.on_make_join_request( - origin, context, user_id, supported_versions=supported_versions + origin, room_id, user_id, supported_versions=supported_versions ) return 200, content class FederationMakeLeaveServlet(BaseFederationServlet): - PATH = "/make_leave/(?P[^/]*)/(?P[^/]*)" + PATH = "/make_leave/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, content, query, context, user_id): - content = await self.handler.on_make_leave_request(origin, context, user_id) + async def on_GET(self, origin, content, query, room_id, user_id): + content = await self.handler.on_make_leave_request(origin, room_id, user_id) return 200, content @@ -543,17 +543,17 @@ async def on_PUT(self, origin, content, query, room_id, event_id): class FederationEventAuthServlet(BaseFederationServlet): - PATH = "/event_auth/(?P[^/]*)/(?P[^/]*)" + PATH = "/event_auth/(?P[^/]*)/(?P[^/]*)" - async def on_GET(self, origin, content, query, context, event_id): - return await self.handler.on_event_auth(origin, context, event_id) + async def on_GET(self, origin, content, query, room_id, event_id): + return await self.handler.on_event_auth(origin, room_id, event_id) class FederationV1SendJoinServlet(BaseFederationServlet): PATH = "/send_join/(?P[^/]*)/(?P[^/]*)" async def on_PUT(self, origin, content, query, room_id, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content content = await self.handler.on_send_join_request(origin, content) return 200, (200, content) @@ -565,16 +565,16 @@ class FederationV2SendJoinServlet(BaseFederationServlet): PREFIX = FEDERATION_V2_PREFIX async def on_PUT(self, origin, content, query, room_id, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content content = await self.handler.on_send_join_request(origin, content) return 200, content class FederationV1InviteServlet(BaseFederationServlet): - PATH = "/invite/(?P[^/]*)/(?P[^/]*)" + PATH = "/invite/(?P[^/]*)/(?P[^/]*)" - async def on_PUT(self, origin, content, query, context, event_id): + async def on_PUT(self, origin, content, query, room_id, event_id): # We don't get a room version, so we have to assume its EITHER v1 or # v2. This is "fine" as the only difference between V1 and V2 is the # state resolution algorithm, and we don't use that for processing @@ -589,12 +589,12 @@ async def on_PUT(self, origin, content, query, context, event_id): class FederationV2InviteServlet(BaseFederationServlet): - PATH = "/invite/(?P[^/]*)/(?P[^/]*)" + PATH = "/invite/(?P[^/]*)/(?P[^/]*)" PREFIX = FEDERATION_V2_PREFIX - async def on_PUT(self, origin, content, query, context, event_id): - # TODO(paul): assert that context/event_id parsed from path actually + async def on_PUT(self, origin, content, query, room_id, event_id): + # TODO(paul): assert that room_id/event_id parsed from path actually # match those given in content room_version = content["room_version"]