From 8165ba48b1d7d6a265683b06e32d08935f41fa69 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Date: Thu, 24 Jun 2021 16:00:08 +0100
Subject: [PATCH] Return errors from `send_join` etc if the event is rejected
 (#10243)

Rather than persisting rejected events via `send_join` and friends, raise a 403 if someone tries to pull a fast one.
---
 changelog.d/10243.feature                   |  1 +
 synapse/handlers/federation.py              | 46 +++++++++++++++++----
 tests/federation/transport/test_knocking.py |  4 +-
 3 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 changelog.d/10243.feature

diff --git a/changelog.d/10243.feature b/changelog.d/10243.feature
new file mode 100644
index 0000000000..d16f66ffe9
--- /dev/null
+++ b/changelog.d/10243.feature
@@ -0,0 +1 @@
+Improve validation on federation `send_{join,leave,knock}` endpoints.
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 12f3d85342..d929c65131 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1953,16 +1953,31 @@ class FederationHandler(BaseHandler):
         self, origin: str, event: EventBase
     ) -> EventContext:
         """
-        We have received a join/leave/knock event for a room.
+        We have received a join/leave/knock event for a room via send_join/leave/knock.
 
         Verify that event and send it into the room on the remote homeserver's behalf.
 
+        This is quite similar to on_receive_pdu, with the following principal
+        differences:
+          * only membership events are permitted (and only events with
+            sender==state_key -- ie, no kicks or bans)
+          * *We* send out the event on behalf of the remote server.
+          * We enforce the membership restrictions of restricted rooms.
+          * Rejected events result in an exception rather than being stored.
+
+        There are also other differences, however it is not clear if these are by
+        design or omission. In particular, we do not attempt to backfill any missing
+        prev_events.
+
         Args:
             origin: The homeserver of the remote (joining/invited/knocking) user.
             event: The member event that has been signed by the remote homeserver.
 
         Returns:
             The context of the event after inserting it into the room graph.
+
+        Raises:
+            SynapseError if the event is not accepted into the room
         """
         logger.debug(
             "on_send_membership_event: Got event: %s, signatures: %s",
@@ -1981,7 +1996,7 @@ class FederationHandler(BaseHandler):
         if event.sender != event.state_key:
             raise SynapseError(400, "state_key and sender must match", Codes.BAD_JSON)
 
-        event.internal_metadata.outlier = False
+        assert not event.internal_metadata.outlier
 
         # Send this event on behalf of the other server.
         #
@@ -1991,6 +2006,11 @@ class FederationHandler(BaseHandler):
         event.internal_metadata.send_on_behalf_of = origin
 
         context = await self.state_handler.compute_event_context(event)
+        context = await self._check_event_auth(origin, event, context)
+        if context.rejected:
+            raise SynapseError(
+                403, f"{event.membership} event was rejected", Codes.FORBIDDEN
+            )
 
         # for joins, we need to check the restrictions of restricted rooms
         if event.membership == Membership.JOIN:
@@ -2008,8 +2028,8 @@ class FederationHandler(BaseHandler):
                     403, "This event is not allowed in this context", Codes.FORBIDDEN
                 )
 
-        await self._auth_and_persist_event(origin, event, context)
-
+        # all looks good, we can persist the event.
+        await self._run_push_actions_and_persist_event(event, context)
         return context
 
     async def _check_join_restrictions(
@@ -2179,6 +2199,18 @@ class FederationHandler(BaseHandler):
             backfilled=backfilled,
         )
 
+        await self._run_push_actions_and_persist_event(event, context, backfilled)
+
+    async def _run_push_actions_and_persist_event(
+        self, event: EventBase, context: EventContext, backfilled: bool = False
+    ):
+        """Run the push actions for a received event, and persist it.
+
+        Args:
+            event: The event itself.
+            context: The event context.
+            backfilled: True if the event was backfilled.
+        """
         try:
             if (
                 not event.internal_metadata.is_outlier()
@@ -2492,9 +2524,9 @@ class FederationHandler(BaseHandler):
         origin: str,
         event: EventBase,
         context: EventContext,
-        state: Optional[Iterable[EventBase]],
-        auth_events: Optional[MutableStateMap[EventBase]],
-        backfilled: bool,
+        state: Optional[Iterable[EventBase]] = None,
+        auth_events: Optional[MutableStateMap[EventBase]] = None,
+        backfilled: bool = False,
     ) -> EventContext:
         """
         Checks whether an event should be rejected (for failing auth checks).
diff --git a/tests/federation/transport/test_knocking.py b/tests/federation/transport/test_knocking.py
index 8c215d50f2..aab44bce4a 100644
--- a/tests/federation/transport/test_knocking.py
+++ b/tests/federation/transport/test_knocking.py
@@ -205,9 +205,7 @@ class FederationKnockingTestCase(
 
         # Have this homeserver skip event auth checks. This is necessary due to
         # event auth checks ensuring that events were signed by the sender's homeserver.
-        async def _check_event_auth(
-            origin, event, context, state, auth_events, backfilled
-        ):
+        async def _check_event_auth(origin, event, context, *args, **kwargs):
             return context
 
         homeserver.get_federation_handler()._check_event_auth = _check_event_auth