Add tests to ensure that email notification links are sanitized (#19741)

Fix #2860

Also cleans up comments around plans to define `ALLOWED_SCHEMES` as we can rely on Bleach's `ALLOWED_PROTOCOLS` defaults (`http`, `https`  and `mailto`).

changelog.d/19741.misc

@@ -0,0 +1 @@
+Added tests to ensure that email notification links are sanitized. Contributed by Noah Markert.

synapse/push/mailer.py

@@ -111,8 +111,6 @@ ALLOWED_ATTRS = {
     # would make sense if we did
     "img": ["src"],
 }
-# When bleach release a version with this option, we can specify schemes
-# ALLOWED_SCHEMES = ["http", "https", "ftp", "mailto"]
 
 
 class Mailer:
@@ -971,8 +969,6 @@ def safe_markup(raw_html: str) -> Markup:
                 raw_html,
                 tags=ALLOWED_TAGS,
                 attributes=ALLOWED_ATTRS,
-                # bleach master has this, but it isn't released yet
-                # protocols=ALLOWED_SCHEMES,
                 strip=True,
             )
         )

tests/push/test_email.py

@@ -379,6 +379,73 @@ class EmailPusherTests(HomeserverTestCase):
 
         self.assertIn("_matrix/media/v1/thumbnail/DUMMY_MEDIA_ID", html)
 
+    @parameterized.expand(
+        [
+            (
+                "https allowed",
+                '<a href="https://example.com">test click me</a>',
+                '<a href="https://example.com" rel="nofollow">test click me</a>',
+            ),
+            (
+                "http allowed",
+                '<a href="http://example.com">test click me</a>',
+                '<a href="http://example.com" rel="nofollow">test click me</a>',
+            ),
+            (
+                "mailto allowed",
+                '<a href="mailto:test@example.com">test click me</a>',
+                '<a href="mailto:test@example.com">test click me</a>',
+            ),
+            (
+                "javascript disallowed",
+                '<a href="javascript:stealCookies()" rel="nofollow">test click me</a>',
+                "<a>test click me</a>",
+            ),
+            (
+                "data disallowed",
+                '<a href="data:,Hello%2C%20World%21" rel="nofollow">test click me</a>',
+                "<a>test click me</a>",
+            ),
+        ],
+    )
+    def test_link_schemes_sanitized(
+        self,
+        _test_label: str,
+        input: str,
+        expected: str,
+    ) -> None:
+        # Create a room
+        room = self.helper.create_room_as(self.user_id, tok=self.access_token)
+
+        self.helper.invite(
+            room=room, src=self.user_id, tok=self.access_token, targ=self.others[0].id
+        )
+        self.helper.join(room=room, user=self.others[0].id, tok=self.others[0].token)
+
+        self.helper.send_event(
+            room,
+            type="m.room.message",
+            content={
+                "msgtype": "m.text",
+                "format": "org.matrix.custom.html",
+                "formatted_body": input,
+                "body": "test click me",  # Plain-text Fallback
+            },
+            tok=self.others[0].token,
+        )
+
+        args, kwargs = self._check_for_mail()
+        msg: bytes = args[5]
+        html_message = email.message_from_bytes(msg).get_payload(i=1)
+        assert isinstance(html_message, email.message.Message)
+
+        # Extract the `bytes` from the html Message object, and decode to a `str`.
+        html = html_message.get_payload(decode=True)
+        assert isinstance(html, bytes)
+        html = html.decode()
+
+        self.assertIn(expected, html)
+
     def test_empty_room(self) -> None:
         """All users leaving a room shouldn't cause the pusher to break."""
         # Create a simple room with two users