diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index b021001376..d195755655 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -405,14 +405,15 @@ class FrozenEventV2(EventBase): for name, sigs in event_dict.pop("signatures", {}).items() } - # An event should only have an event_id at this point if it's for a v1/v2 like room. - # In future room versions, the `event_id` is derived from the event canonical JSON. - - # So if we see a `event_id` but the room version doesn't support - # v1/v2 events, then it's invalid and we should reject it. + # In newer room versions (3+), the `event_id` is derived from a hash of the + # event canonical JSON, so it should not be explicitly provided in the event + # dictionary. + # + # If we see an `event_id` in a newer room version, then it's an invalid event + # and we should reject it. assert ( "event_id" not in event_dict - ), "Event ID should not be supplied in non-v1/v2 room" + ), "Event ID should not be provided for events in non-v1/v2 room versions" unsigned = dict(event_dict.pop("unsigned", {})) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index f5a9594906..528f42078b 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -479,9 +479,11 @@ class FederationServer(FederationBase): try: event = event_from_pdu_json(p, room_version) except Exception as e: - # We can only provide feedback to the federating server if we can determine what the event_id is - # but since we failed to parse the event, we can't derive the `event_id` so there is nothing - # to use as the `pdu_results` key. Best we can do is just log for our own record and move on. + # We can only provide feedback to the federating server if we can + # determine what the `event_id` is but since we failed to parse the + # event, we can't derive the `event_id` so there is nothing to use as + # the `pdu_results` key. Best we can do is just log for our own record + # and move on. if possible_event_id != _UNKNOWN_EVENT_ID: pdu_results[possible_event_id] = { "error": f"Failed to convert JSON into event: {e}" diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 035b8ec360..2858c01a85 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -88,6 +88,11 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): self.assertEqual(500, channel.code, channel.result) def test_accept_valid_pdus_and_ignore_invalid(self) -> None: + """ + Test to make sure that old v1/v2 formatted events (that include `event_id`) are + rejected from a newer room version that don't support it but we still accept + properly formatted/valid events from the same batch. + """ user = self.register_user("user1", "test") tok = self.login("user1", "test") room_id = self.helper.create_room_as("user1", tok=tok) @@ -117,9 +122,8 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): event1_json = event1.get_pdu_json() event2_json = event2.get_pdu_json() event3_json = event3.get_pdu_json() - logging.info(event1_json) - # Purposely adding event id that shouldn't be there + # Purposely adding `event_id` that shouldn't be there event2_json["event_id"] = event2.event_id channel = self.make_signed_federation_request( @@ -132,10 +136,12 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): # and that it indicates success for valid events pdus: JsonDict = body["pdus"] self.assertIncludes( - set(pdus.keys()), {event1.event_id, event2.event_id, event3.event_id} + set(pdus.keys()), + {event1.event_id, event2.event_id, event3.event_id}, + exact=True, ) self.assertEqual(pdus[event1.event_id], {}) - self.assertNotEqual(body["pdus"][event2.event_id]["error"], "") + self.assertNotEqual(pdus[event2.event_id]["error"], "") self.assertEqual(pdus[event3.event_id], {}) # Make sure other valid events from the send transaction were persisted successfully