From a3137f2745249c15f2a9b7e00b95dd013b9cfc05 Mon Sep 17 00:00:00 2001 From: rogersheu Date: Thu, 21 Oct 2021 20:39:29 -0700 Subject: [PATCH 1/8] Read receipt restriction (matrix-org #11156) --- synapse/rest/client/receipts.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index 9770413c618d..f8bda0c7c30a 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -13,10 +13,12 @@ # limitations under the License. import logging +import re from typing import TYPE_CHECKING, Tuple from synapse.api.constants import ReadReceiptEventFields from synapse.api.errors import Codes, 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 @@ -52,7 +54,12 @@ async def on_POST( if receipt_type != "m.read": raise SynapseError(400, "Receipt type must be 'm.read'") - body = parse_json_object_from_request(request, allow_empty_body=True) + allow_empty_body = False + user_agent = get_request_user_agent(request) + if re.search(".*Riot.*", user_agent) or re.search("Element/1.[012].*", user_agent) or re.search("SchildiChat/1.[012].*", user_agent): + allow_empty_body = True + + body = parse_json_object_from_request(request, allow_empty_body) hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) if not isinstance(hidden, bool): From e10adca48bcba95b3a1898c4410fd4a23a8dde1b Mon Sep 17 00:00:00 2001 From: rogersheu <78449574+rogersheu@users.noreply.github.com> Date: Thu, 21 Oct 2021 20:42:25 -0700 Subject: [PATCH 2/8] Read receipt restrictions (matrix-org #11156) --- changelog.d/11156.misc | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog.d/11156.misc diff --git a/changelog.d/11156.misc b/changelog.d/11156.misc new file mode 100644 index 000000000000..91b198b74cfe --- /dev/null +++ b/changelog.d/11156.misc @@ -0,0 +1,5 @@ +Limit which clients are allowed to send read receipts without a body (matrix-org #11156) + +synapse\rest\client\receipts.py + +Added in a regex check. From 68e2a029633b84e149e93f2f0c879de8b990cb9a Mon Sep 17 00:00:00 2001 From: rogersheu <78449574+rogersheu@users.noreply.github.com> Date: Fri, 22 Oct 2021 12:40:34 -0700 Subject: [PATCH 3/8] Read receipt restrictions (matrix-org #11156) --- changelog.d/11156.misc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/changelog.d/11156.misc b/changelog.d/11156.misc index 91b198b74cfe..75444c51d1ee 100644 --- a/changelog.d/11156.misc +++ b/changelog.d/11156.misc @@ -1,5 +1 @@ -Limit which clients are allowed to send read receipts without a body (matrix-org #11156) - -synapse\rest\client\receipts.py - -Added in a regex check. +Only allow old Element/Riot Android clients to send read receipts without a request body. All other clients must include a request body as required by the specification. Contributed by @rogersheu. From 23c255a0e6fe0bdf90b2bcc2463586f345d9fe6a Mon Sep 17 00:00:00 2001 From: rogersheu Date: Fri, 22 Oct 2021 12:43:28 -0700 Subject: [PATCH 4/8] Read receipt restrictions (matrix-log #11156 ) --- synapse/rest/client/receipts.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index f8bda0c7c30a..216274c69966 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -54,11 +54,13 @@ async def on_POST( if receipt_type != "m.read": raise SynapseError(400, "Receipt type must be 'm.read'") - allow_empty_body = False user_agent = get_request_user_agent(request) - if re.search(".*Riot.*", user_agent) or re.search("Element/1.[012].*", user_agent) or re.search("SchildiChat/1.[012].*", user_agent): - allow_empty_body = True - + pattern = re.compile(r"(?:Element|SchildiChat)/1\.[012]\.") + + allow_empty_body = False + if "Android" in user_agent: + if pattern.match(user_agent) or "Riot" in user_agent: + allow_empty_body = True body = parse_json_object_from_request(request, allow_empty_body) hidden = body.get(ReadReceiptEventFields.MSC2285_HIDDEN, False) From ec2e9c6890f391dca3abe6339e5e1ab96c446122 Mon Sep 17 00:00:00 2001 From: rogersheu Date: Mon, 1 Nov 2021 23:35:01 -0700 Subject: [PATCH 5/8] Added test cases and capabilities for empty body read receipts Unit tests now try a variety of Android builds. Waiting to see if other versions to test for are provided. Uncertain of current implementation of the custom_headers field in make_request. --- changelog.d/{11156.misc => 11157.misc} | 0 tests/rest/client/test_sync.py | 23 +++++++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) rename changelog.d/{11156.misc => 11157.misc} (100%) diff --git a/changelog.d/11156.misc b/changelog.d/11157.misc similarity index 100% rename from changelog.d/11156.misc rename to changelog.d/11157.misc diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 95be369d4be1..6b0cdda0c3ee 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -14,6 +14,8 @@ # limitations under the License. import json +from parameterized import parameterized + import synapse.rest.admin from synapse.api.constants import ( EventContentFields, @@ -417,7 +419,23 @@ def test_hidden_read_receipts(self): # Test that the first user can't see the other user's hidden read receipt self.assertEqual(self._get_read_receipt(), None) - def test_read_receipt_with_empty_body(self): + @parameterized.expand( + [ + ( + "agent1", + "Element/1.2.2 (Linux; U; Android 9; MatrixAndroidSDK_X 0.0.1)", + 200, + ), + ("agent2", "Element/1.2.2 (Linux; Android 6)", 200), + ("agent3", "SchildiChat/1.3.6 (Android 11)", 200), + ("agent4", "Element (Riot.im) (Android 9)", 200), + ("agent5", "Element/1.2.1", 400), # Does not contain Android + ("agent6", "Element dbg/1.1.8-dev (Android)", 400), # Different format + ] + ) + def test_read_receipt_with_empty_body( + self, name, user_agent: str, expected_status_code: int + ): # Send a message as the first user res = self.helper.send(self.room_id, body="hello", tok=self.tok) @@ -426,8 +444,9 @@ def test_read_receipt_with_empty_body(self): "POST", "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), access_token=self.tok2, + custom_headers=[("User Agent", user_agent)], ) - self.assertEqual(channel.code, 200) + self.assertEqual(channel.code, expected_status_code) def _get_read_receipt(self): """Syncs and returns the read receipt.""" From 779e5f3ec4835ec195dd2ace2221ae6b0f60cd64 Mon Sep 17 00:00:00 2001 From: rogersheu <78449574+rogersheu@users.noreply.github.com> Date: Fri, 5 Nov 2021 11:34:39 -0700 Subject: [PATCH 6/8] Update tests/rest/client/test_sync.py, 1.3.x should fail. Co-authored-by: reivilibre --- tests/rest/client/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 6b0cdda0c3ee..b35c88403657 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -427,7 +427,7 @@ def test_hidden_read_receipts(self): 200, ), ("agent2", "Element/1.2.2 (Linux; Android 6)", 200), - ("agent3", "SchildiChat/1.3.6 (Android 11)", 200), + ("agent3", "SchildiChat/1.3.6 (Android 11)", 400), ("agent4", "Element (Riot.im) (Android 9)", 200), ("agent5", "Element/1.2.1", 400), # Does not contain Android ("agent6", "Element dbg/1.1.8-dev (Android)", 400), # Different format From db2aa61168b6fe6d6bdc2d85020e0e9b0695cbae Mon Sep 17 00:00:00 2001 From: rogersheu Date: Fri, 5 Nov 2021 12:14:42 -0700 Subject: [PATCH 7/8] Moved re.compile out of function, other small changes --- synapse/rest/client/receipts.py | 4 ++-- tests/rest/client/test_sync.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index 216274c69966..5c3cd9752007 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -26,6 +26,8 @@ from ._base import client_patterns +pattern = re.compile(r"(?:Element|SchildiChat)/1\.[012]\.") + if TYPE_CHECKING: from synapse.server import HomeServer @@ -55,8 +57,6 @@ async def on_POST( raise SynapseError(400, "Receipt type must be 'm.read'") user_agent = get_request_user_agent(request) - pattern = re.compile(r"(?:Element|SchildiChat)/1\.[012]\.") - allow_empty_body = False if "Android" in user_agent: if pattern.match(user_agent) or "Riot" in user_agent: diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index b35c88403657..77b4ff6b4631 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -427,10 +427,10 @@ def test_hidden_read_receipts(self): 200, ), ("agent2", "Element/1.2.2 (Linux; Android 6)", 200), - ("agent3", "SchildiChat/1.3.6 (Android 11)", 400), + ("agent3", "SchildiChat/1.3.6 (Android 11)", 400), # Will allow empty body again starting at version 1.3+ ("agent4", "Element (Riot.im) (Android 9)", 200), - ("agent5", "Element/1.2.1", 400), # Does not contain Android - ("agent6", "Element dbg/1.1.8-dev (Android)", 400), # Different format + ("agent5", "Element/1.2.1", 400), # Does not contain "Android" + ("agent6", "Element dbg/1.1.8-dev (Android)", 400), # Different format, missing "/" after Element ] ) def test_read_receipt_with_empty_body( @@ -444,7 +444,7 @@ def test_read_receipt_with_empty_body( "POST", "/rooms/%s/receipt/m.read/%s" % (self.room_id, res["event_id"]), access_token=self.tok2, - custom_headers=[("User Agent", user_agent)], + custom_headers=[("User-Agent", user_agent)], ) self.assertEqual(channel.code, expected_status_code) From af8e8b5b6ad36538ef4f413e18f62ce0fdc385ad Mon Sep 17 00:00:00 2001 From: rogersheu Date: Mon, 8 Nov 2021 12:27:15 -0800 Subject: [PATCH 8/8] Rewrote unit tests: #11157 --- synapse/rest/client/receipts.py | 1 + tests/rest/client/test_sync.py | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index 5c3cd9752007..2b25b9aad6a3 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -56,6 +56,7 @@ async def on_POST( if receipt_type != "m.read": 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: diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 77b4ff6b4631..c42768637678 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -421,16 +421,23 @@ def test_hidden_read_receipts(self): @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, ), - ("agent2", "Element/1.2.2 (Linux; Android 6)", 200), - ("agent3", "SchildiChat/1.3.6 (Android 11)", 400), # Will allow empty body again starting at version 1.3+ - ("agent4", "Element (Riot.im) (Android 9)", 200), - ("agent5", "Element/1.2.1", 400), # Does not contain "Android" - ("agent6", "Element dbg/1.1.8-dev (Android)", 400), # Different format, missing "/" after Element + # 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(