From 96274565ff0dbb7d21b02b04fcef115330426707 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Wed, 16 Mar 2022 12:17:39 -0400
Subject: [PATCH] Fix bundling aggregations if unsigned is not a returned event
 field. (#12234)

An error occured if a filter was supplied with `event_fields` which did not include
`unsigned`.

In that case, bundled aggregations are still added as the spec states it is allowed
for servers to add additional fields.
---
 changelog.d/12234.bugfix            |  1 +
 synapse/events/utils.py             |  9 ++++++---
 tests/rest/client/test_relations.py | 28 ++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 changelog.d/12234.bugfix

diff --git a/changelog.d/12234.bugfix b/changelog.d/12234.bugfix
new file mode 100644
index 000000000..dbb77f36f
--- /dev/null
+++ b/changelog.d/12234.bugfix
@@ -0,0 +1 @@
+Fix a long-standing bug when a `filter` argument with `event_fields` supplied but not including the `unsigned` field could result in a 500 error on `/sync`.
diff --git a/synapse/events/utils.py b/synapse/events/utils.py
index b2a237c1e..a0520068e 100644
--- a/synapse/events/utils.py
+++ b/synapse/events/utils.py
@@ -530,9 +530,12 @@ class EventClientSerializer:
 
         # Include the bundled aggregations in the event.
         if serialized_aggregations:
-            serialized_event["unsigned"].setdefault("m.relations", {}).update(
-                serialized_aggregations
-            )
+            # There is likely already an "unsigned" field, but a filter might
+            # have stripped it off (via the event_fields option). The server is
+            # allowed to return additional fields, so add it back.
+            serialized_event.setdefault("unsigned", {}).setdefault(
+                "m.relations", {}
+            ).update(serialized_aggregations)
 
     def serialize_events(
         self,
diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index 0cbe6c0cf..171f4e97c 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1267,6 +1267,34 @@ class RelationsTestCase(BaseRelationsTestCase):
             [annotation_event_id_good, thread_event_id],
         )
 
+    def test_bundled_aggregations_with_filter(self) -> None:
+        """
+        If "unsigned" is an omitted field (due to filtering), adding the bundled
+        aggregations should not break.
+
+        Note that the spec allows for a server to return additional fields beyond
+        what is specified.
+        """
+        self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a")
+
+        # Note that the sync filter does not include "unsigned" as a field.
+        filter = urllib.parse.quote_plus(
+            b'{"event_fields": ["content", "event_id"], "room": {"timeline": {"limit": 3}}}'
+        )
+        channel = self.make_request(
+            "GET", f"/sync?filter={filter}", access_token=self.user_token
+        )
+        self.assertEqual(200, channel.code, channel.json_body)
+
+        # Ensure the timeline is limited, find the parent event.
+        room_timeline = channel.json_body["rooms"]["join"][self.room]["timeline"]
+        self.assertTrue(room_timeline["limited"])
+        parent_event = self._find_event_in_chunk(room_timeline["events"])
+
+        # Ensure there's bundled aggregations on it.
+        self.assertIn("unsigned", parent_event)
+        self.assertIn("m.relations", parent_event["unsigned"])
+
 
 class RelationRedactionTestCase(BaseRelationsTestCase):
     """