From 5e892671a74251109bf9cf4a78bebed9d8085979 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erikj@element.io>
Date: Thu, 16 May 2024 15:04:14 +0100
Subject: [PATCH] Fix bug where push rules would be empty in `/sync` (#17142)

Fixes #16987

Some old accounts seem to have an entry in global account data table for
push rules, which we should ignore
---
 changelog.d/17142.bugfix    |  1 +
 synapse/handlers/sync.py    | 20 ++++++++------------
 tests/handlers/test_sync.py | 29 ++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 13 deletions(-)
 create mode 100644 changelog.d/17142.bugfix

diff --git a/changelog.d/17142.bugfix b/changelog.d/17142.bugfix
new file mode 100644
index 000000000..09b617aed
--- /dev/null
+++ b/changelog.d/17142.bugfix
@@ -0,0 +1 @@
+Fix bug where push rules would be empty in `/sync` for some accounts. Introduced in v1.93.0.
diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py
index 53fe2a6a5..659499af7 100644
--- a/synapse/handlers/sync.py
+++ b/synapse/handlers/sync.py
@@ -1971,23 +1971,19 @@ class SyncHandler:
             )
 
             if push_rules_changed:
-                global_account_data = {
-                    AccountDataTypes.PUSH_RULES: await self._push_rules_handler.push_rules_for_user(
-                        sync_config.user
-                    ),
-                    **global_account_data,
-                }
+                global_account_data = dict(global_account_data)
+                global_account_data[AccountDataTypes.PUSH_RULES] = (
+                    await self._push_rules_handler.push_rules_for_user(sync_config.user)
+                )
         else:
             all_global_account_data = await self.store.get_global_account_data_for_user(
                 user_id
             )
 
-            global_account_data = {
-                AccountDataTypes.PUSH_RULES: await self._push_rules_handler.push_rules_for_user(
-                    sync_config.user
-                ),
-                **all_global_account_data,
-            }
+            global_account_data = dict(all_global_account_data)
+            global_account_data[AccountDataTypes.PUSH_RULES] = (
+                await self._push_rules_handler.push_rules_for_user(sync_config.user)
+            )
 
         account_data_for_user = (
             await sync_config.filter_collection.filter_global_account_data(
diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py
index 9c12a11e3..0299113b9 100644
--- a/tests/handlers/test_sync.py
+++ b/tests/handlers/test_sync.py
@@ -24,7 +24,7 @@ from parameterized import parameterized
 
 from twisted.test.proto_helpers import MemoryReactor
 
-from synapse.api.constants import EventTypes, JoinRules
+from synapse.api.constants import AccountDataTypes, EventTypes, JoinRules
 from synapse.api.errors import Codes, ResourceLimitError
 from synapse.api.filtering import FilterCollection, Filtering
 from synapse.api.room_versions import RoomVersion, RoomVersions
@@ -895,6 +895,33 @@ class SyncTestCase(tests.unittest.HomeserverTestCase):
 
         self.assertIn(private_call_event.event_id, priv_event_ids)
 
+    def test_push_rules_with_bad_account_data(self) -> None:
+        """Some old accounts have managed to set a `m.push_rules` account data,
+        which we should ignore in /sync response.
+        """
+
+        user = self.register_user("alice", "password")
+
+        # Insert the bad account data.
+        self.get_success(
+            self.store.add_account_data_for_user(user, AccountDataTypes.PUSH_RULES, {})
+        )
+
+        sync_result: SyncResult = self.get_success(
+            self.sync_handler.wait_for_sync_for_user(
+                create_requester(user), generate_sync_config(user)
+            )
+        )
+
+        for account_dict in sync_result.account_data:
+            if account_dict["type"] == AccountDataTypes.PUSH_RULES:
+                # We should have lots of push rules here, rather than the bad
+                # empty data.
+                self.assertNotEqual(account_dict["content"], {})
+                return
+
+        self.fail("No push rules found")
+
 
 _request_key = 0