From db10f2c037ff59124776a10e198ab432aec2bdc6 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 11 May 2022 16:34:17 +0100
Subject: [PATCH] No longer permit empty body when sending receipts (#12709)

---
 changelog.d/12709.removal       |  1 +
 synapse/rest/client/receipts.py | 13 +------------
 tests/rest/client/test_sync.py  | 30 ++++--------------------------
 3 files changed, 6 insertions(+), 38 deletions(-)
 create mode 100644 changelog.d/12709.removal

diff --git a/changelog.d/12709.removal b/changelog.d/12709.removal
new file mode 100644
index 000000000..6bb03e289
--- /dev/null
+++ b/changelog.d/12709.removal
@@ -0,0 +1 @@
+Require a body in POST requests to `/rooms/{roomId}/receipt/{receiptType}/{eventId}`, as required by the [Matrix specification](https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3roomsroomidreceiptreceipttypeeventid). This breaks compatibility with Element Android 1.2.0 and earlier: users of those clients will be unable to send read receipts.
diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py
index f9caab663..4b03eb876 100644
--- a/synapse/rest/client/receipts.py
+++ b/synapse/rest/client/receipts.py
@@ -13,12 +13,10 @@
 # limitations under the License.
 
 import logging
-import re
 from typing import TYPE_CHECKING, Tuple
 
 from synapse.api.constants import ReceiptTypes
 from synapse.api.errors import SynapseError
-from synapse.http import get_request_user_agent
 from synapse.http.server import HttpServer
 from synapse.http.servlet import RestServlet, parse_json_object_from_request
 from synapse.http.site import SynapseRequest
@@ -26,8 +24,6 @@ from synapse.types import JsonDict
 
 from ._base import client_patterns
 
-pattern = re.compile(r"(?:Element|SchildiChat)/1\.[012]\.")
-
 if TYPE_CHECKING:
     from synapse.server import HomeServer
 
@@ -69,14 +65,7 @@ class ReceiptRestServlet(RestServlet):
         ):
             raise SynapseError(400, "Receipt type must be 'm.read'")
 
-        # Do not allow older SchildiChat and Element Android clients (prior to Element/1.[012].x) to send an empty body.
-        user_agent = get_request_user_agent(request)
-        allow_empty_body = False
-        if "Android" in user_agent:
-            if pattern.match(user_agent) or "Riot" in user_agent:
-                allow_empty_body = True
-        # This call makes sure possible empty body is handled correctly
-        parse_json_object_from_request(request, allow_empty_body)
+        parse_json_object_from_request(request, allow_empty_body=False)
 
         await self.presence_handler.bump_presence_active_time(requester.user)
 
diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py
index 010833764..2722bf26e 100644
--- a/tests/rest/client/test_sync.py
+++ b/tests/rest/client/test_sync.py
@@ -13,6 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import json
+from http import HTTPStatus
 from typing import List, Optional
 
 from parameterized import parameterized
@@ -485,30 +486,7 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase):
         # Test that we didn't override the public read receipt
         self.assertIsNone(self._get_read_receipt())
 
-    @parameterized.expand(
-        [
-            # Old Element version, expected to send an empty body
-            (
-                "agent1",
-                "Element/1.2.2 (Linux; U; Android 9; MatrixAndroidSDK_X 0.0.1)",
-                200,
-            ),
-            # Old SchildiChat version, expected to send an empty body
-            ("agent2", "SchildiChat/1.2.1 (Android 10)", 200),
-            # Expected 400: Denies empty body starting at version 1.3+
-            ("agent3", "Element/1.3.6 (Android 10)", 400),
-            ("agent4", "SchildiChat/1.3.6 (Android 11)", 400),
-            # Contains "Riot": Receipts with empty bodies expected
-            ("agent5", "Element (Riot.im) (Android 9)", 200),
-            # Expected 400: Does not contain "Android"
-            ("agent6", "Element/1.2.1", 400),
-            # Expected 400: Different format, missing "/" after Element; existing build that should allow empty bodies, but minimal ongoing usage
-            ("agent7", "Element dbg/1.1.8-dev (Android)", 400),
-        ]
-    )
-    def test_read_receipt_with_empty_body(
-        self, name: str, user_agent: str, expected_status_code: int
-    ) -> None:
+    def test_read_receipt_with_empty_body_is_rejected(self) -> None:
         # Send a message as the first user
         res = self.helper.send(self.room_id, body="hello", tok=self.tok)
 
@@ -517,9 +495,9 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase):
             "POST",
             f"/rooms/{self.room_id}/receipt/m.read/{res['event_id']}",
             access_token=self.tok2,
-            custom_headers=[("User-Agent", user_agent)],
         )
-        self.assertEqual(channel.code, expected_status_code)
+        self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST)
+        self.assertEqual(channel.json_body["errcode"], "M_NOT_JSON", channel.json_body)
 
     def _get_read_receipt(self) -> Optional[JsonDict]:
         """Syncs and returns the read receipt."""