From 208630bbb07fac1bf8f22d4a6f059e2ac6e0dbd5 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 8 Aug 2024 14:59:23 +0200 Subject: [PATCH 1/6] Use `max_upload_size` as the limit when following the `Location` header --- synapse/http/matrixfederationclient.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index 6fd75fd3817..b78cf2bafbd 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -464,6 +464,8 @@ def __init__( self.max_long_retries = hs.config.federation.max_long_retries self.max_short_retries = hs.config.federation.max_short_retries + self.max_download_size = hs.config.media.max_upload_size + self._cooperator = Cooperator(scheduler=_make_scheduler(self.reactor)) self._sleeper = AwakenableSleeper(self.reactor) @@ -1756,8 +1758,10 @@ async def federation_get_file( request.destination, str_url, ) + # We don't know how large the response upfront, so limit it to + # the `max_upload_size` config value. length, headers, _, _ = await self._simple_http_client.get_file( - str_url, output_stream, expected_size + str_url, output_stream, self.max_download_size ) logger.info( From 9a3cb5a245516f064c7b42884835723a31fb0f13 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 8 Aug 2024 15:04:04 +0200 Subject: [PATCH 2/6] Changelog --- changelog.d/17543.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17543.bugfix diff --git a/changelog.d/17543.bugfix b/changelog.d/17543.bugfix new file mode 100644 index 00000000000..152b305e587 --- /dev/null +++ b/changelog.d/17543.bugfix @@ -0,0 +1 @@ +Fix authenticated media responses using a wrong limit when following redirects over federation. \ No newline at end of file From f7bca60920fd2c5711996cf67152692c8b93f0c7 Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 8 Aug 2024 15:05:15 +0200 Subject: [PATCH 3/6] Add missing words --- synapse/http/matrixfederationclient.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/http/matrixfederationclient.py b/synapse/http/matrixfederationclient.py index b78cf2bafbd..12c41c39e9a 100644 --- a/synapse/http/matrixfederationclient.py +++ b/synapse/http/matrixfederationclient.py @@ -1758,7 +1758,7 @@ async def federation_get_file( request.destination, str_url, ) - # We don't know how large the response upfront, so limit it to + # We don't know how large the response will be upfront, so limit it to # the `max_upload_size` config value. length, headers, _, _ = await self._simple_http_client.get_file( str_url, output_stream, self.max_download_size From 3306a02e70de239bb914eaa0dd9d59f2ed5722da Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 22 Aug 2024 14:02:04 +0200 Subject: [PATCH 4/6] Add test to validate redirects work --- tests/http/test_matrixfederationclient.py | 68 +++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index e2f033fdae3..05ca5dfc24d 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/tests/http/test_matrixfederationclient.py @@ -17,6 +17,7 @@ # [This file includes modifications made by New Vector Limited] # # +import io from typing import Any, Dict, Generator from unittest.mock import ANY, Mock, create_autospec @@ -32,7 +33,9 @@ from twisted.web.http_headers import Headers from synapse.api.errors import HttpResponseException, RequestSendFailed +from synapse.api.ratelimiting import Ratelimiter from synapse.config._base import ConfigError +from synapse.config.ratelimiting import RatelimitSettings from synapse.http.matrixfederationclient import ( ByteParser, MatrixFederationHttpClient, @@ -337,6 +340,71 @@ def test_client_gets_headers(self) -> None: r = self.successResultOf(d) self.assertEqual(r.code, 200) + def test_authed_media_redirect_response(self) -> None: + """ + Validate that, when following a `Location` redirect, the + maximum size is _not_ set to the initial response `Content-Length` and + the media file can be downloaded. + """ + limiter = Ratelimiter( + store=self.hs.get_datastores().main, + clock=self.clock, + cfg=RatelimitSettings(key="", per_second=0.17, burst_count=1048576), + ) + + output_stream = io.BytesIO() + + d = defer.ensureDeferred( + self.cl.federation_get_file( + "testserv:8008", "path", output_stream, limiter, "127.0.0.1", 10000 + ) + ) + + self.pump() + + conn = Mock() + clients = self.reactor.tcpClients + client = clients[0][2].buildProtocol(None) + client.makeConnection(conn) + + # Deferred does not have a result + self.assertNoResult(d) + + redirect_data = b"\r\n\r\n--6067d4698f8d40a0a794ea7d7379d53a\r\nContent-Type: application/json\r\n\r\n{}\r\n--6067d4698f8d40a0a794ea7d7379d53a\r\nLocation: http://testserv:8008/ab/c1/2345.txt\r\n\r\n--6067d4698f8d40a0a794ea7d7379d53a--\r\n\r\n" + client.dataReceived( + b"HTTP/1.1 200 OK\r\n" + b"Server: Fake\r\n" + b"Content-Length: %i\r\n" + b"Content-Type: multipart/mixed; boundary=6067d4698f8d40a0a794ea7d7379d53a\r\n\r\n" + % (len(redirect_data)) + ) + client.dataReceived(redirect_data) + + # Still no result, not followed the redirect yet + self.assertNoResult(d) + + # Now send the response returned by the server at `Location` + conn = Mock() + clients = self.reactor.tcpClients + client = clients[1][2].buildProtocol(None) + client.makeConnection(conn) + + # make sure the length is longer than the initial response + data = b"Hello world!" * 30 + client.dataReceived( + b"HTTP/1.1 200 OK\r\n" + b"Server: Fake\r\n" + b"Content-Length: %i\r\n" + b"Content-Type: text/plain\r\n" + b"\r\n" + b"%s\r\n" + b"\r\n" % (len(data), data) + ) + + # We should get a successful response + length, _, _ = self.successResultOf(d) + self.assertEqual(length, len(data)) + @parameterized.expand(["get_json", "post_json", "delete_json", "put_json"]) def test_timeout_reading_body(self, method_name: str) -> None: """ From 43fa77acaba84a1929b90536bad86a128c4c3813 Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Thu, 29 Aug 2024 08:30:38 +0200 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: Erik Johnston --- tests/http/test_matrixfederationclient.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index 05ca5dfc24d..c8d68ef7db9 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/tests/http/test_matrixfederationclient.py @@ -386,6 +386,9 @@ def test_authed_media_redirect_response(self) -> None: # Now send the response returned by the server at `Location` conn = Mock() clients = self.reactor.tcpClients + (host, port, factory, _timeout, _bindAddress) = clients[1] + self.assertEqual(host, "testserv") + self.assertEqual(port, 8008) client = clients[1][2].buildProtocol(None) client.makeConnection(conn) @@ -404,6 +407,7 @@ def test_authed_media_redirect_response(self) -> None: # We should get a successful response length, _, _ = self.successResultOf(d) self.assertEqual(length, len(data)) + self.assertEqual(output_stream.getvalue(), data) @parameterized.expand(["get_json", "post_json", "delete_json", "put_json"]) def test_timeout_reading_body(self, method_name: str) -> None: From b55f80ed15deda5a4085548f90d437666af957ca Mon Sep 17 00:00:00 2001 From: Till Faelligen <2353100+S7evinK@users.noreply.github.com> Date: Thu, 29 Aug 2024 08:44:46 +0200 Subject: [PATCH 6/6] Use a real transport instead of a mock --- tests/http/test_matrixfederationclient.py | 26 ++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/http/test_matrixfederationclient.py b/tests/http/test_matrixfederationclient.py index c8d68ef7db9..68274123736 100644 --- a/tests/http/test_matrixfederationclient.py +++ b/tests/http/test_matrixfederationclient.py @@ -362,39 +362,45 @@ def test_authed_media_redirect_response(self) -> None: self.pump() - conn = Mock() clients = self.reactor.tcpClients - client = clients[0][2].buildProtocol(None) - client.makeConnection(conn) + self.assertEqual(len(clients), 1) + (host, port, factory, _timeout, _bindAddress) = clients[0] + self.assertEqual(host, "1.2.3.4") + self.assertEqual(port, 8008) + + # complete the connection and wire it up to a fake transport + protocol = factory.buildProtocol(None) + transport = StringTransport() + protocol.makeConnection(transport) # Deferred does not have a result self.assertNoResult(d) redirect_data = b"\r\n\r\n--6067d4698f8d40a0a794ea7d7379d53a\r\nContent-Type: application/json\r\n\r\n{}\r\n--6067d4698f8d40a0a794ea7d7379d53a\r\nLocation: http://testserv:8008/ab/c1/2345.txt\r\n\r\n--6067d4698f8d40a0a794ea7d7379d53a--\r\n\r\n" - client.dataReceived( + protocol.dataReceived( b"HTTP/1.1 200 OK\r\n" b"Server: Fake\r\n" b"Content-Length: %i\r\n" b"Content-Type: multipart/mixed; boundary=6067d4698f8d40a0a794ea7d7379d53a\r\n\r\n" % (len(redirect_data)) ) - client.dataReceived(redirect_data) + protocol.dataReceived(redirect_data) # Still no result, not followed the redirect yet self.assertNoResult(d) # Now send the response returned by the server at `Location` - conn = Mock() clients = self.reactor.tcpClients (host, port, factory, _timeout, _bindAddress) = clients[1] - self.assertEqual(host, "testserv") + self.assertEqual(host, "1.2.3.4") self.assertEqual(port, 8008) - client = clients[1][2].buildProtocol(None) - client.makeConnection(conn) + protocol = factory.buildProtocol(None) + transport = StringTransport() + protocol.makeConnection(transport) # make sure the length is longer than the initial response data = b"Hello world!" * 30 - client.dataReceived( + protocol.dataReceived( b"HTTP/1.1 200 OK\r\n" b"Server: Fake\r\n" b"Content-Length: %i\r\n"