From b774c555d821170e4f16de7d48f01484c3a1d740 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 4 Dec 2020 10:51:56 -0500 Subject: [PATCH] Add additional validation to pusher URLs. (#8865) Pusher URLs now must end in `/_matrix/push/v1/notify` per the specification. --- changelog.d/8865.bugfix | 1 + synapse/push/__init__.py | 3 +- synapse/push/httppusher.py | 16 +++- tests/push/test_http.py | 108 +++++++++++++++++++------ tests/replication/test_pusher_shard.py | 8 +- tests/rest/admin/test_user.py | 2 +- 6 files changed, 107 insertions(+), 31 deletions(-) create mode 100644 changelog.d/8865.bugfix diff --git a/changelog.d/8865.bugfix b/changelog.d/8865.bugfix new file mode 100644 index 0000000000..a1e625f552 --- /dev/null +++ b/changelog.d/8865.bugfix @@ -0,0 +1 @@ +Add additional validation to pusher URLs to be compliant with the specification. diff --git a/synapse/push/__init__.py b/synapse/push/__init__.py index 5a437f9810..e462fb2e13 100644 --- a/synapse/push/__init__.py +++ b/synapse/push/__init__.py @@ -15,5 +15,4 @@ class PusherConfigException(Exception): - def __init__(self, msg): - super().__init__(msg) + """An error occurred when creating a pusher.""" diff --git a/synapse/push/httppusher.py b/synapse/push/httppusher.py index 0e845212a9..6a0ee8274c 100644 --- a/synapse/push/httppusher.py +++ b/synapse/push/httppusher.py @@ -14,6 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import urllib.parse from prometheus_client import Counter @@ -97,9 +98,22 @@ class HttpPusher: if self.data is None: raise PusherConfigException("data can not be null for HTTP pusher") + # Validate that there's a URL and it is of the proper form. if "url" not in self.data: raise PusherConfigException("'url' required in data for HTTP pusher") - self.url = self.data["url"] + + url = self.data["url"] + if not isinstance(url, str): + raise PusherConfigException("'url' must be a string") + url_parts = urllib.parse.urlparse(url) + # Note that the specification also says the scheme must be HTTPS, but + # it isn't up to the homeserver to verify that. + if url_parts.path != "/_matrix/push/v1/notify": + raise PusherConfigException( + "'url' must have a path of '/_matrix/push/v1/notify'" + ) + + self.url = url self.http_client = hs.get_proxied_blacklisted_http_client() self.data_minus_url = {} self.data_minus_url.update(self.data) diff --git a/tests/push/test_http.py b/tests/push/test_http.py index e8cea39c83..8b4af74c51 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -18,6 +18,7 @@ from twisted.internet.defer import Deferred import synapse.rest.admin from synapse.logging.context import make_deferred_yieldable +from synapse.push import PusherConfigException from synapse.rest.client.v1 import login, room from synapse.rest.client.v2_alpha import receipts @@ -34,6 +35,11 @@ class HTTPPusherTests(HomeserverTestCase): user_id = True hijack_auth = False + def default_config(self): + config = super().default_config() + config["start_pushers"] = True + return config + def make_homeserver(self, reactor, clock): self.push_attempts = [] @@ -46,15 +52,49 @@ class HTTPPusherTests(HomeserverTestCase): m.post_json_get_json = post_json_get_json - config = self.default_config() - config["start_pushers"] = True - - hs = self.setup_test_homeserver( - config=config, proxied_blacklisted_http_client=m - ) + hs = self.setup_test_homeserver(proxied_blacklisted_http_client=m) return hs + def test_invalid_configuration(self): + """Invalid push configurations should be rejected.""" + # Register the user who gets notified + user_id = self.register_user("user", "pass") + access_token = self.login("user", "pass") + + # Register the pusher + user_tuple = self.get_success( + self.hs.get_datastore().get_user_by_access_token(access_token) + ) + token_id = user_tuple.token_id + + def test_data(data): + self.get_failure( + self.hs.get_pusherpool().add_pusher( + user_id=user_id, + access_token=token_id, + kind="http", + app_id="m.http", + app_display_name="HTTP Push Notifications", + device_display_name="pushy push", + pushkey="a@example.com", + lang=None, + data=data, + ), + PusherConfigException, + ) + + # Data must be provided with a URL. + test_data(None) + test_data({}) + test_data({"url": 1}) + # A bare domain name isn't accepted. + test_data({"url": "example.com"}) + # A URL without a path isn't accepted. + test_data({"url": "http://example.com"}) + # A url with an incorrect path isn't accepted. + test_data({"url": "http://example.com/foo"}) + def test_sends_http(self): """ The HTTP pusher will send pushes for each message to a HTTP endpoint @@ -84,7 +124,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -119,7 +159,9 @@ class HTTPPusherTests(HomeserverTestCase): # One push was attempted to be sent -- it'll be the first message self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual( self.push_attempts[0][2]["notification"]["content"]["body"], "Hi!" ) @@ -139,7 +181,9 @@ class HTTPPusherTests(HomeserverTestCase): # Now it'll try and send the second push message, which will be the second one self.assertEqual(len(self.push_attempts), 2) - self.assertEqual(self.push_attempts[1][1], "example.com") + self.assertEqual( + self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual( self.push_attempts[1][2]["notification"]["content"]["body"], "There!" ) @@ -196,7 +240,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -232,7 +276,9 @@ class HTTPPusherTests(HomeserverTestCase): # Check our push made it with high priority self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual(self.push_attempts[0][2]["notification"]["prio"], "high") # Add yet another person — we want to make this room not a 1:1 @@ -270,7 +316,9 @@ class HTTPPusherTests(HomeserverTestCase): # Advance time a bit, so the pusher will register something has happened self.pump() self.assertEqual(len(self.push_attempts), 2) - self.assertEqual(self.push_attempts[1][1], "example.com") + self.assertEqual( + self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "high") def test_sends_high_priority_for_one_to_one_only(self): @@ -312,7 +360,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -328,7 +376,9 @@ class HTTPPusherTests(HomeserverTestCase): # Check our push made it with high priority — this is a one-to-one room self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual(self.push_attempts[0][2]["notification"]["prio"], "high") # Yet another user joins @@ -347,7 +397,9 @@ class HTTPPusherTests(HomeserverTestCase): # Advance time a bit, so the pusher will register something has happened self.pump() self.assertEqual(len(self.push_attempts), 2) - self.assertEqual(self.push_attempts[1][1], "example.com") + self.assertEqual( + self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify" + ) # check that this is low-priority self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "low") @@ -394,7 +446,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -410,7 +462,9 @@ class HTTPPusherTests(HomeserverTestCase): # Check our push made it with high priority self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual(self.push_attempts[0][2]["notification"]["prio"], "high") # Send another event, this time with no mention @@ -419,7 +473,9 @@ class HTTPPusherTests(HomeserverTestCase): # Advance time a bit, so the pusher will register something has happened self.pump() self.assertEqual(len(self.push_attempts), 2) - self.assertEqual(self.push_attempts[1][1], "example.com") + self.assertEqual( + self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify" + ) # check that this is low-priority self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "low") @@ -467,7 +523,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -487,7 +543,9 @@ class HTTPPusherTests(HomeserverTestCase): # Check our push made it with high priority self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) self.assertEqual(self.push_attempts[0][2]["notification"]["prio"], "high") # Send another event, this time as someone without the power of @room @@ -498,7 +556,9 @@ class HTTPPusherTests(HomeserverTestCase): # Advance time a bit, so the pusher will register something has happened self.pump() self.assertEqual(len(self.push_attempts), 2) - self.assertEqual(self.push_attempts[1][1], "example.com") + self.assertEqual( + self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify" + ) # check that this is low-priority self.assertEqual(self.push_attempts[1][2]["notification"]["prio"], "low") @@ -572,7 +632,7 @@ class HTTPPusherTests(HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "http://example.com/_matrix/push/v1/notify"}, ) ) @@ -591,7 +651,9 @@ class HTTPPusherTests(HomeserverTestCase): # Check our push made it self.assertEqual(len(self.push_attempts), 1) - self.assertEqual(self.push_attempts[0][1], "example.com") + self.assertEqual( + self.push_attempts[0][1], "http://example.com/_matrix/push/v1/notify" + ) # Check that the unread count for the room is 0 # diff --git a/tests/replication/test_pusher_shard.py b/tests/replication/test_pusher_shard.py index f894bcd6e7..800ad94a04 100644 --- a/tests/replication/test_pusher_shard.py +++ b/tests/replication/test_pusher_shard.py @@ -67,7 +67,7 @@ class PusherShardTestCase(BaseMultiWorkerStreamTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "https://push.example.com/push"}, + data={"url": "https://push.example.com/_matrix/push/v1/notify"}, ) ) @@ -109,7 +109,7 @@ class PusherShardTestCase(BaseMultiWorkerStreamTestCase): http_client_mock.post_json_get_json.assert_called_once() self.assertEqual( http_client_mock.post_json_get_json.call_args[0][0], - "https://push.example.com/push", + "https://push.example.com/_matrix/push/v1/notify", ) self.assertEqual( event_id, @@ -161,7 +161,7 @@ class PusherShardTestCase(BaseMultiWorkerStreamTestCase): http_client_mock2.post_json_get_json.assert_not_called() self.assertEqual( http_client_mock1.post_json_get_json.call_args[0][0], - "https://push.example.com/push", + "https://push.example.com/_matrix/push/v1/notify", ) self.assertEqual( event_id, @@ -183,7 +183,7 @@ class PusherShardTestCase(BaseMultiWorkerStreamTestCase): http_client_mock2.post_json_get_json.assert_called_once() self.assertEqual( http_client_mock2.post_json_get_json.call_args[0][0], - "https://push.example.com/push", + "https://push.example.com/_matrix/push/v1/notify", ) self.assertEqual( event_id, diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 54d46f4bd3..35c546aa69 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -1256,7 +1256,7 @@ class PushersRestTestCase(unittest.HomeserverTestCase): device_display_name="pushy push", pushkey="a@example.com", lang=None, - data={"url": "example.com"}, + data={"url": "https://example.com/_matrix/push/v1/notify"}, ) )