From cc32fa7358641b96f5d3dbc14d0cd068e676e256 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <clokep@users.noreply.github.com>
Date: Mon, 15 Jun 2020 16:20:34 -0400
Subject: [PATCH] Ensure the body is a string before comparing push rules.
 (#7701)

---
 changelog.d/7701.bugfix                |  1 +
 synapse/push/push_rule_evaluator.py    |  4 +--
 tests/push/test_push_rule_evaluator.py | 39 ++++++++++++++++++++------
 3 files changed, 33 insertions(+), 11 deletions(-)
 create mode 100644 changelog.d/7701.bugfix

diff --git a/changelog.d/7701.bugfix b/changelog.d/7701.bugfix
new file mode 100644
index 000000000..e5b10f75f
--- /dev/null
+++ b/changelog.d/7701.bugfix
@@ -0,0 +1 @@
+Do not break push rule evaluation when receiving an event with a non-string body. This is a long-standing bug.
diff --git a/synapse/push/push_rule_evaluator.py b/synapse/push/push_rule_evaluator.py
index 11032491a..aeac257a6 100644
--- a/synapse/push/push_rule_evaluator.py
+++ b/synapse/push/push_rule_evaluator.py
@@ -131,7 +131,7 @@ class PushRuleEvaluatorForEvent(object):
         # XXX: optimisation: cache our pattern regexps
         if condition["key"] == "content.body":
             body = self._event.content.get("body", None)
-            if not body:
+            if not body or not isinstance(body, str):
                 return False
 
             return _glob_matches(pattern, body, word_boundary=True)
@@ -147,7 +147,7 @@ class PushRuleEvaluatorForEvent(object):
             return False
 
         body = self._event.content.get("body", None)
-        if not body:
+        if not body or not isinstance(body, str):
             return False
 
         # Similar to _glob_matches, but do not treat display_name as a glob.
diff --git a/tests/push/test_push_rule_evaluator.py b/tests/push/test_push_rule_evaluator.py
index 9ae6a87d7..af35d23ae 100644
--- a/tests/push/test_push_rule_evaluator.py
+++ b/tests/push/test_push_rule_evaluator.py
@@ -21,7 +21,7 @@ from tests import unittest
 
 
 class PushRuleEvaluatorTestCase(unittest.TestCase):
-    def setUp(self):
+    def _get_evaluator(self, content):
         event = FrozenEvent(
             {
                 "event_id": "$event_id",
@@ -29,37 +29,58 @@ class PushRuleEvaluatorTestCase(unittest.TestCase):
                 "sender": "@user:test",
                 "state_key": "",
                 "room_id": "@room:test",
-                "content": {"body": "foo bar baz"},
+                "content": content,
             },
             RoomVersions.V1,
         )
         room_member_count = 0
         sender_power_level = 0
         power_levels = {}
-        self.evaluator = PushRuleEvaluatorForEvent(
+        return PushRuleEvaluatorForEvent(
             event, room_member_count, sender_power_level, power_levels
         )
 
     def test_display_name(self):
         """Check for a matching display name in the body of the event."""
+        evaluator = self._get_evaluator({"body": "foo bar baz"})
+
         condition = {
             "kind": "contains_display_name",
         }
 
         # Blank names are skipped.
-        self.assertFalse(self.evaluator.matches(condition, "@user:test", ""))
+        self.assertFalse(evaluator.matches(condition, "@user:test", ""))
 
         # Check a display name that doesn't match.
-        self.assertFalse(self.evaluator.matches(condition, "@user:test", "not found"))
+        self.assertFalse(evaluator.matches(condition, "@user:test", "not found"))
 
         # Check a display name which matches.
-        self.assertTrue(self.evaluator.matches(condition, "@user:test", "foo"))
+        self.assertTrue(evaluator.matches(condition, "@user:test", "foo"))
 
         # A display name that matches, but not a full word does not result in a match.
-        self.assertFalse(self.evaluator.matches(condition, "@user:test", "ba"))
+        self.assertFalse(evaluator.matches(condition, "@user:test", "ba"))
 
         # A display name should not be interpreted as a regular expression.
-        self.assertFalse(self.evaluator.matches(condition, "@user:test", "ba[rz]"))
+        self.assertFalse(evaluator.matches(condition, "@user:test", "ba[rz]"))
 
         # A display name with spaces should work fine.
-        self.assertTrue(self.evaluator.matches(condition, "@user:test", "foo bar"))
+        self.assertTrue(evaluator.matches(condition, "@user:test", "foo bar"))
+
+    def test_no_body(self):
+        """Not having a body shouldn't break the evaluator."""
+        evaluator = self._get_evaluator({})
+
+        condition = {
+            "kind": "contains_display_name",
+        }
+        self.assertFalse(evaluator.matches(condition, "@user:test", "foo"))
+
+    def test_invalid_body(self):
+        """A non-string body should not break the evaluator."""
+        condition = {
+            "kind": "contains_display_name",
+        }
+
+        for body in (1, True, {"foo": "bar"}):
+            evaluator = self._get_evaluator({"body": body})
+            self.assertFalse(evaluator.matches(condition, "@user:test", "foo"))