From 95cbc99809dacab0af0b9cc638ebccefe8ac8da3 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 31 Oct 2024 18:52:00 +0100 Subject: [PATCH 01/16] Tolerate transaction pdus that contain superfluous event ids --- synapse/federation/federation_server.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 1932fa82a4a..f8dc8ef03f7 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -56,7 +56,7 @@ SynapseError, UnsupportedRoomVersionError, ) -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, EventFormatVersions, RoomVersion from synapse.crypto.event_signing import compute_event_signature from synapse.events import EventBase from synapse.events.snapshot import EventContext @@ -432,6 +432,8 @@ async def _handle_pdus_in_txn( newest_pdu_ts = 0 + pdu_results = {} + for p in transaction.pdus: # FIXME (richardv): I don't think this works: # https://github.com/matrix-org/synapse/issues/8429 @@ -469,13 +471,22 @@ async def _handle_pdus_in_txn( logger.info("Ignoring PDU: %s", e) continue + if possible_event_id != "": + if room_version.event_format != EventFormatVersions.ROOM_V1_V2: + logger.info(f"""Rejecting event {possible_event_id} from {origin} + because the event was made for a v1 room, + while {room_id} is a {room_version} room""") + msg = "Event ID incorrectly supplied in non-v1/v2 room" + pdu_results[possible_event_id] = {"error": msg} + continue + event = event_from_pdu_json(p, room_version) pdus_by_room.setdefault(room_id, []).append(event) if event.origin_server_ts > newest_pdu_ts: newest_pdu_ts = event.origin_server_ts - pdu_results = {} + # we can process different rooms in parallel (which is useful if they # require callouts to other servers to fetch missing events), but From 730822cde5c09b70196bb82a5cffdcc69848e658 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 31 Oct 2024 19:31:32 +0100 Subject: [PATCH 02/16] Try to always process transactions even if a PDU can't be parsed --- synapse/federation/federation_server.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index f8dc8ef03f7..0b2fe5eb3c6 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -480,7 +480,15 @@ async def _handle_pdus_in_txn( pdu_results[possible_event_id] = {"error": msg} continue - event = event_from_pdu_json(p, room_version) + try: + event = event_from_pdu_json(p, room_version) + except Exception as e: + if possible_event_id != "": + msg = f"Failed to convert json to event" + pdu_results[possible_event_id] = {"error": f"Failed to convert json into event, {e}"} + logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, because of {e}") + continue + pdus_by_room.setdefault(room_id, []).append(event) if event.origin_server_ts > newest_pdu_ts: From e89add86894152582f4bb627d93f985ded17deac Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 31 Oct 2024 19:43:37 +0100 Subject: [PATCH 03/16] add changelog --- changelog.d/17893.doc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17893.doc diff --git a/changelog.d/17893.doc b/changelog.d/17893.doc new file mode 100644 index 00000000000..077ea544d90 --- /dev/null +++ b/changelog.d/17893.doc @@ -0,0 +1 @@ +Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir From 07578bd4d0e3bc0ea39c9a56435681fa85a8e56c Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 31 Oct 2024 22:54:12 +0100 Subject: [PATCH 04/16] minor changes to strings --- synapse/federation/federation_server.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 0b2fe5eb3c6..98eb201ad57 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -56,7 +56,11 @@ SynapseError, UnsupportedRoomVersionError, ) -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, EventFormatVersions, RoomVersion +from synapse.api.room_versions import ( + KNOWN_ROOM_VERSIONS, + EventFormatVersions, + RoomVersion, +) from synapse.crypto.event_signing import compute_event_signature from synapse.events import EventBase from synapse.events.snapshot import EventContext @@ -473,20 +477,18 @@ async def _handle_pdus_in_txn( if possible_event_id != "": if room_version.event_format != EventFormatVersions.ROOM_V1_V2: - logger.info(f"""Rejecting event {possible_event_id} from {origin} - because the event was made for a v1 room, - while {room_id} is a {room_version} room""") - msg = "Event ID incorrectly supplied in non-v1/v2 room" - pdu_results[possible_event_id] = {"error": msg} + logger.info(f"Rejecting event {possible_event_id} from {origin} " + f"because the event was made for a v1 room, " + f"while {room_id} is a v{room_version.identifier} room") + pdu_results[possible_event_id] = {"error": "Event ID incorrectly supplied in non-v1/v2 room"} continue try: event = event_from_pdu_json(p, room_version) except Exception as e: if possible_event_id != "": - msg = f"Failed to convert json to event" pdu_results[possible_event_id] = {"error": f"Failed to convert json into event, {e}"} - logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, because of {e}") + logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}") continue pdus_by_room.setdefault(room_id, []).append(event) @@ -494,8 +496,6 @@ async def _handle_pdus_in_txn( if event.origin_server_ts > newest_pdu_ts: newest_pdu_ts = event.origin_server_ts - - # we can process different rooms in parallel (which is useful if they # require callouts to other servers to fetch missing events), but # impose a limit to avoid going too crazy with ram/cpu. From 9953a366ff94ca78482d41cbc09a270cf1adca90 Mon Sep 17 00:00:00 2001 From: morguldir Date: Wed, 6 Nov 2024 23:08:40 +0100 Subject: [PATCH 05/16] make constant for magic value used for unknown event ids Co-authored-by: Eric Eastwood --- synapse/federation/federation_server.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 98eb201ad57..c543da090b3 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -133,6 +133,8 @@ # federation. _INBOUND_EVENT_HANDLING_LOCK_NAME = "federation_inbound_pdu" +_UNKNOWN_EVENT_ID = "" + class FederationServer(FederationBase): def __init__(self, hs: "HomeServer"): @@ -452,7 +454,7 @@ async def _handle_pdus_in_txn( # We try and pull out an event ID so that if later checks fail we # can log something sensible. We don't mandate an event ID here in # case future event formats get rid of the key. - possible_event_id = p.get("event_id", "") + possible_event_id = p.get("event_id", _UNKNOWN_EVENT_ID) # Now we get the room ID so that we can check that we know the # version of the room. @@ -475,7 +477,7 @@ async def _handle_pdus_in_txn( logger.info("Ignoring PDU: %s", e) continue - if possible_event_id != "": + if possible_event_id != _UNKNOWN_EVENT_ID: if room_version.event_format != EventFormatVersions.ROOM_V1_V2: logger.info(f"Rejecting event {possible_event_id} from {origin} " f"because the event was made for a v1 room, " @@ -486,7 +488,7 @@ async def _handle_pdus_in_txn( try: event = event_from_pdu_json(p, room_version) except Exception as e: - if possible_event_id != "": + if possible_event_id != _UNKNOWN_EVENT_ID: pdu_results[possible_event_id] = {"error": f"Failed to convert json into event, {e}"} logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}") continue From f55e645b705dbd7682cedaaae1afe0891bb91a26 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 6 Nov 2024 23:18:51 +0100 Subject: [PATCH 06/16] add explanatory comments and clarifications --- synapse/federation/federation_server.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index c543da090b3..a6e663ca6a9 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -477,17 +477,25 @@ async def _handle_pdus_in_txn( logger.info("Ignoring PDU: %s", e) continue + # 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. if possible_event_id != _UNKNOWN_EVENT_ID: if room_version.event_format != EventFormatVersions.ROOM_V1_V2: logger.info(f"Rejecting event {possible_event_id} from {origin} " f"because the event was made for a v1 room, " f"while {room_id} is a v{room_version.identifier} room") - pdu_results[possible_event_id] = {"error": "Event ID incorrectly supplied in non-v1/v2 room"} + pdu_results[possible_event_id] = {"error": "Event ID should not be supplied in non-v1/v2 room"} continue 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 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}"} logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}") From 464e9cf46c18a9900ef0b5c49e1cf6fb77ac18b7 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 7 Nov 2024 01:20:34 +0100 Subject: [PATCH 07/16] ruff --- synapse/federation/federation_server.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index a6e663ca6a9..7a03b831f0d 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -484,10 +484,14 @@ async def _handle_pdus_in_txn( # v1/v2 events, then it's invalid and we should reject it. if possible_event_id != _UNKNOWN_EVENT_ID: if room_version.event_format != EventFormatVersions.ROOM_V1_V2: - logger.info(f"Rejecting event {possible_event_id} from {origin} " - f"because the event was made for a v1 room, " - f"while {room_id} is a v{room_version.identifier} room") - pdu_results[possible_event_id] = {"error": "Event ID should not be supplied in non-v1/v2 room"} + logger.info( + f"Rejecting event {possible_event_id} from {origin} " + f"because the event was made for a v1 room, " + f"while {room_id} is a v{room_version.identifier} room" + ) + pdu_results[possible_event_id] = { + "error": "Event ID should not be supplied in non-v1/v2 room" + } continue try: @@ -497,8 +501,12 @@ async def _handle_pdus_in_txn( # but since we 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}"} - logger.warning("Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}") + pdu_results[possible_event_id] = { + "error": f"Failed to convert json into event, {e}" + } + logger.warning( + "Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}" + ) continue pdus_by_room.setdefault(room_id, []).append(event) From b6b5d81c2757aa574153d954ff0140e61c4016f4 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 7 Nov 2024 01:21:01 +0100 Subject: [PATCH 08/16] Add test in test_federation_server, ensuring that good events are kept --- tests/federation/test_federation_server.py | 53 +++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 88261450b1f..a0ddb0a4553 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -25,7 +25,9 @@ from twisted.test.proto_helpers import MemoryReactor -from synapse.api.room_versions import KNOWN_ROOM_VERSIONS +from synapse.api.constants import EventTypes +from synapse.api.errors import NotFoundError +from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.events import EventBase, make_event_from_dict from synapse.rest import admin @@ -84,6 +86,55 @@ async def failing_handler(_origin: str, _content: JsonDict) -> None: ) self.assertEqual(500, channel.code, channel.result) + def test_accept_valid_pdus_and_ignore_invalid(self) -> None: + user = self.register_user("nex", "test") + tok = self.login("nex", "test") + room_id = self.helper.create_room_as("nex", tok=tok) + + builder = self.hs.get_event_builder_factory().for_room_version( + RoomVersions.V10, + { + "type": EventTypes.Message, + "sender": user, + "room_id": room_id, + "content": {"body": "hello i am nexy", "msgtype": "m.text"}, + }, + ) + event1, _ = self.get_success( + self.hs.get_event_creation_handler().create_new_client_event(builder) + ) + event2, _ = self.get_success( + self.hs.get_event_creation_handler().create_new_client_event( + builder, prev_event_ids=[event1.event_id] + ) + ) + + event1_json = event1.get_pdu_json() + event2_json = event2.get_pdu_json() + + logging.info("Purposefully adding event id that shouldn't be there") + event2_json["event_id"] = event2.event_id + + channel = self.make_signed_federation_request( + "PUT", + "/_matrix/federation/v1/send/txn", + {"pdus": [event1_json, event2_json]}, + ) + body = channel.json_body + logging.info(f"Response body: {body}") + self.assertTrue(body["pdus"][event1.event_id] == {}) + self.assertTrue(body["pdus"][event2.event_id]["error"] != "") + result = self.get_success( + self.hs.get_storage_controllers().main.get_event(event1.event_id) + ) + self.assertEqual(result.event_id, event1.event_id) + + # Make sure the corrupt event isn't persisted + self.get_failure( + self.hs.get_storage_controllers().main.get_event(event2.event_id), + NotFoundError, + ) + class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: From 89f91aabb31a7ba4d4cfd62bf3c0eadceb20c8a7 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 7 Nov 2024 21:06:20 +0100 Subject: [PATCH 09/16] Remove explicit event_id that's already caught by the catch-all --- synapse/federation/federation_server.py | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 7a03b831f0d..cd8b203b562 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -477,35 +477,18 @@ async def _handle_pdus_in_txn( logger.info("Ignoring PDU: %s", e) continue - # 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. - if possible_event_id != _UNKNOWN_EVENT_ID: - if room_version.event_format != EventFormatVersions.ROOM_V1_V2: - logger.info( - f"Rejecting event {possible_event_id} from {origin} " - f"because the event was made for a v1 room, " - f"while {room_id} is a v{room_version.identifier} room" - ) - pdu_results[possible_event_id] = { - "error": "Event ID should not be supplied in non-v1/v2 room" - } - continue - 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 we failed to parse the event, we can't derive the `event_id` so there is nothing + # 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}" + "error": f"Failed to convert JSON into event: {e}" } logger.warning( - "Failed to parse event {possible_event_id} in transaction from {origin}, due to {e}" + f"Failed to parse event {possible_event_id} in transaction from {origin}, due to: {e}" ) continue From 3ca943a9b7e520a3d12900f8aacbe05d4a25b0c9 Mon Sep 17 00:00:00 2001 From: morguldir Date: Thu, 7 Nov 2024 21:08:49 +0100 Subject: [PATCH 10/16] Add additional event, identify events more clearly, and adjust logs --- tests/federation/test_federation_server.py | 67 ++++++++++++++-------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index a0ddb0a4553..efc44e477e4 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -87,43 +87,54 @@ async def failing_handler(_origin: str, _content: JsonDict) -> None: self.assertEqual(500, channel.code, channel.result) def test_accept_valid_pdus_and_ignore_invalid(self) -> None: - user = self.register_user("nex", "test") - tok = self.login("nex", "test") - room_id = self.helper.create_room_as("nex", tok=tok) - - builder = self.hs.get_event_builder_factory().for_room_version( - RoomVersions.V10, - { - "type": EventTypes.Message, - "sender": user, - "room_id": room_id, - "content": {"body": "hello i am nexy", "msgtype": "m.text"}, - }, - ) - event1, _ = self.get_success( - self.hs.get_event_creation_handler().create_new_client_event(builder) - ) - event2, _ = self.get_success( - self.hs.get_event_creation_handler().create_new_client_event( - builder, prev_event_ids=[event1.event_id] + user = self.register_user("user1", "test") + tok = self.login("user1", "test") + room_id = self.helper.create_room_as("user1", tok=tok) + + def builder(message): + return self.hs.get_event_builder_factory().for_room_version( + RoomVersions.V10, + { + "type": EventTypes.Message, + "sender": user, + "room_id": room_id, + "content": {"body": message, "msgtype": "m.text"}, + } ) - ) + def make_event(message): + event, _ = self.get_success( + self.hs.get_event_creation_handler().create_new_client_event( + builder(message), + ) + ) + return event + event1 = make_event("event1") + event2 = make_event("event2") + event3 = make_event("event3") event1_json = event1.get_pdu_json() event2_json = event2.get_pdu_json() + event3_json = event3.get_pdu_json() + logging.info(event1_json) - logging.info("Purposefully 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( "PUT", "/_matrix/federation/v1/send/txn", - {"pdus": [event1_json, event2_json]}, + {"pdus": [event1_json, event2_json, event3_json]}, ) body = channel.json_body - logging.info(f"Response body: {body}") - self.assertTrue(body["pdus"][event1.event_id] == {}) - self.assertTrue(body["pdus"][event2.event_id]["error"] != "") + # Ensure the response indicates an error for the corrupt event + # 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}) + self.assertEqual(pdus[event1.event_id], {}) + self.assertNotEqual(body["pdus"][event2.event_id]["error"], "") + self.assertEqual(pdus[event3.event_id], {}) + + # Make sure other valid events from the send transaction were persisted successfully result = self.get_success( self.hs.get_storage_controllers().main.get_event(event1.event_id) ) @@ -135,6 +146,12 @@ def test_accept_valid_pdus_and_ignore_invalid(self) -> None: NotFoundError, ) + # Verify that we continue looking at events that come after the corrupted one + result = self.get_success( + self.hs.get_storage_controllers().main.get_event(event3.event_id) + ) + self.assertEqual(result.event_id, event3.event_id) + class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: From b8ce471eae891750fabd1b7aea816a5b2b961241 Mon Sep 17 00:00:00 2001 From: morguldir Date: Fri, 8 Nov 2024 02:06:47 +0100 Subject: [PATCH 11/16] Rename changelog file --- changelog.d/{17893.doc => 17893.bugfix} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename changelog.d/{17893.doc => 17893.bugfix} (56%) diff --git a/changelog.d/17893.doc b/changelog.d/17893.bugfix similarity index 56% rename from changelog.d/17893.doc rename to changelog.d/17893.bugfix index 077ea544d90..9df676e35c4 100644 --- a/changelog.d/17893.doc +++ b/changelog.d/17893.bugfix @@ -1 +1 @@ -Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir +Fix a bug where all messages from a server could be blocked because of one bad event. Contributed by @morguldir. From b2916f1f08edb2c3a0b21b0ab8e8357bbac33e8a Mon Sep 17 00:00:00 2001 From: morguldir Date: Fri, 8 Nov 2024 02:07:09 +0100 Subject: [PATCH 12/16] Add comments around event format assert and make it return a message --- synapse/events/__init__.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 2e56b671f06..95354b131d2 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -405,7 +405,12 @@ def __init__( for name, sigs in event_dict.pop("signatures", {}).items() } - assert "event_id" not in event_dict + # 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. + assert ("event_id" not in event_dict), "Event ID should not be supplied in non-v1/v2 room" unsigned = dict(event_dict.pop("unsigned", {})) From 7b3a695c8a960dcf1ef7fe6af025c47ea04f0c6a Mon Sep 17 00:00:00 2001 From: morguldir Date: Fri, 8 Nov 2024 02:07:52 +0100 Subject: [PATCH 13/16] Remove unnecessary assert Co-authored-by: Eric Eastwood --- tests/federation/test_federation_server.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index efc44e477e4..998bdc018a7 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -135,10 +135,9 @@ def make_event(message): self.assertEqual(pdus[event3.event_id], {}) # Make sure other valid events from the send transaction were persisted successfully - result = self.get_success( + self.get_success( self.hs.get_storage_controllers().main.get_event(event1.event_id) ) - self.assertEqual(result.event_id, event1.event_id) # Make sure the corrupt event isn't persisted self.get_failure( @@ -147,11 +146,9 @@ def make_event(message): ) # Verify that we continue looking at events that come after the corrupted one - result = self.get_success( + self.get_success( self.hs.get_storage_controllers().main.get_event(event3.event_id) ) - self.assertEqual(result.event_id, event3.event_id) - class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: From 78e04ddb5dbc909f56d7fdccbe35a90a8a76ba86 Mon Sep 17 00:00:00 2001 From: morguldir Date: Fri, 8 Nov 2024 02:17:12 +0100 Subject: [PATCH 14/16] Run lint --- synapse/events/__init__.py | 4 +++- synapse/federation/federation_server.py | 1 - tests/federation/test_federation_server.py | 15 ++++++++++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 95354b131d2..b0210013769 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -410,7 +410,9 @@ def __init__( # 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. - assert ("event_id" not in event_dict), "Event ID should not be supplied in non-v1/v2 room" + assert ( + "event_id" not in event_dict + ), "Event ID should not be supplied in non-v1/v2 room" unsigned = dict(event_dict.pop("unsigned", {})) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index cd8b203b562..f5a9594906d 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -58,7 +58,6 @@ ) from synapse.api.room_versions import ( KNOWN_ROOM_VERSIONS, - EventFormatVersions, RoomVersion, ) from synapse.crypto.event_signing import compute_event_signature diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 998bdc018a7..035b8ec360e 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -30,6 +30,7 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersions from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.events import EventBase, make_event_from_dict +from synapse.events.builder import EventBuilder from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer @@ -91,7 +92,7 @@ def test_accept_valid_pdus_and_ignore_invalid(self) -> None: tok = self.login("user1", "test") room_id = self.helper.create_room_as("user1", tok=tok) - def builder(message): + def builder(message: str) -> EventBuilder: return self.hs.get_event_builder_factory().for_room_version( RoomVersions.V10, { @@ -99,12 +100,13 @@ def builder(message): "sender": user, "room_id": room_id, "content": {"body": message, "msgtype": "m.text"}, - } + }, ) - def make_event(message): + + def make_event(message: str) -> EventBase: event, _ = self.get_success( self.hs.get_event_creation_handler().create_new_client_event( - builder(message), + builder(message), ) ) return event @@ -129,7 +131,9 @@ def make_event(message): # Ensure the response indicates an error for the corrupt event # 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}) + self.assertIncludes( + set(pdus.keys()), {event1.event_id, event2.event_id, event3.event_id} + ) self.assertEqual(pdus[event1.event_id], {}) self.assertNotEqual(body["pdus"][event2.event_id]["error"], "") self.assertEqual(pdus[event3.event_id], {}) @@ -150,6 +154,7 @@ def make_event(message): self.hs.get_storage_controllers().main.get_event(event3.event_id) ) + class ServerACLsTestCase(unittest.TestCase): def test_blocked_server(self) -> None: e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]}) From df079300aaa4078f9f576394d21cebdf58758fc5 Mon Sep 17 00:00:00 2001 From: morguldir Date: Mon, 11 Nov 2024 22:20:43 +0100 Subject: [PATCH 15/16] Apply suggestions from code review Comment updates, docstring, remove extra logs Co-authored-by: Eric Eastwood --- synapse/events/__init__.py | 13 +++++++------ synapse/federation/federation_server.py | 8 +++++--- tests/federation/test_federation_server.py | 14 ++++++++++---- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index b0210013769..d195755655b 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -405,14 +405,15 @@ def __init__( 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 f5a9594906d..528f42078ba 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -479,9 +479,11 @@ async def _handle_pdus_in_txn( 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 035b8ec360e..2858c01a85e 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -88,6 +88,11 @@ async def failing_handler(_origin: str, _content: JsonDict) -> None: 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 @@ def make_event(message: str) -> EventBase: 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 @@ def make_event(message: str) -> EventBase: # 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 From f61de20d673867b8bebeb86cdd4fb7c68b464161 Mon Sep 17 00:00:00 2001 From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> Date: Mon, 9 Dec 2024 11:52:12 +0000 Subject: [PATCH 16/16] Add comment --- synapse/federation/federation_server.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 528f42078ba..8b10ad368ec 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -484,6 +484,9 @@ async def _handle_pdus_in_txn( # 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 an `event_id` happened to be provided in the + # event dictionary, then use that. if possible_event_id != _UNKNOWN_EVENT_ID: pdu_results[possible_event_id] = { "error": f"Failed to convert JSON into event: {e}"