From d0a474d312443a0ef6ebdbd9c6d3b3fd24a3500c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 20 Nov 2024 07:48:22 -0700 Subject: [PATCH] Enable authenticated media by default (#17889) Co-authored-by: Olivier 'reivilibre --- changelog.d/17889.feature | 1 + docs/upgrade.md | 23 ++++++ .../configuration/config_documentation.md | 7 +- synapse/config/repository.py | 4 +- tests/media/test_media_storage.py | 80 ++++++++++++++++++- tests/replication/test_multi_media_repo.py | 4 + tests/rest/admin/test_admin.py | 9 ++- tests/rest/admin/test_media.py | 6 ++ tests/rest/admin/test_user.py | 4 +- tests/rest/media/test_domain_blocking.py | 4 +- tests/rest/media/test_url_preview.py | 3 + 11 files changed, 129 insertions(+), 16 deletions(-) create mode 100644 changelog.d/17889.feature diff --git a/changelog.d/17889.feature b/changelog.d/17889.feature new file mode 100644 index 0000000000..221282553b --- /dev/null +++ b/changelog.d/17889.feature @@ -0,0 +1 @@ +Enforce authenticated media by default. Administrators can revert this by configuring `enable_authenticated_media` to `false`. In a future release of Synapse, this option will be removed and become always-on. diff --git a/docs/upgrade.md b/docs/upgrade.md index 9f12d7c34f..45e63b0c5d 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -128,6 +128,29 @@ removing the experimental support for it in this release. The `experimental_features.msc3886_endpoint` configuration option has been removed. +## Authenticated media is now enforced by default + +The [`enable_authenticated_media`] configuration option now defaults to true. + +This means that clients and remote (federated) homeservers now need to use +the authenticated media endpoints in order to download media from your +homeserver. + +As an exception, existing media that was stored on the server prior to +this option changing to `true` will still be accessible over the +unauthenticated endpoints. + +The matrix.org homeserver has already been running with this option enabled +since September 2024, so most common clients and homeservers should already +be compatible. + +With that said, administrators who wish to disable this feature for broader +compatibility can still do so by manually configuring +`enable_authenticated_media: False`. + +[`enable_authenticated_media`]: usage/configuration/config_documentation.md#enable_authenticated_media + + # Upgrading to v1.119.0 ## Minimum supported Python version diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 005633e46b..7a48d76bbb 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -1887,8 +1887,7 @@ Config options related to Synapse's media store. When set to true, all subsequent media uploads will be marked as authenticated, and will not be available over legacy unauthenticated media endpoints (`/_matrix/media/(r0|v3|v1)/download` and `/_matrix/media/(r0|v3|v1)/thumbnail`) - requests for authenticated media over these endpoints will result in a 404. All media, including authenticated media, will be available over the authenticated media endpoints `_matrix/client/v1/media/download` and `_matrix/client/v1/media/thumbnail`. Media uploaded prior to setting this option to true will still be available over the legacy endpoints. Note if the setting is switched to false -after enabling, media marked as authenticated will be available over legacy endpoints. Defaults to false, but -this will change to true in a future Synapse release. +after enabling, media marked as authenticated will be available over legacy endpoints. Defaults to true (previously false). In a future release of Synapse, this option will be removed and become always-on. In all cases, authenticated requests to download media will succeed, but for unauthenticated requests, this case-by-case breakdown describes whether media downloads are permitted: @@ -1910,9 +1909,11 @@ will perpetually be available over the legacy, unauthenticated endpoint, even af This is for backwards compatibility with older clients and homeservers that do not yet support requesting authenticated media; those older clients or homeservers will not be cut off from media they can already see. +_Changed in Synapse 1.120:_ This option now defaults to `True` when not set, whereas before this version it defaulted to `False`. + Example configuration: ```yaml -enable_authenticated_media: true +enable_authenticated_media: false ``` --- ### `enable_media_repo` diff --git a/synapse/config/repository.py b/synapse/config/repository.py index 97ce6de528..27860154e1 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -272,9 +272,7 @@ class ContentRepositoryConfig(Config): remote_media_lifetime ) - self.enable_authenticated_media = config.get( - "enable_authenticated_media", False - ) + self.enable_authenticated_media = config.get("enable_authenticated_media", True) def generate_config_section(self, data_dir_path: str, **kwargs: Any) -> str: assert data_dir_path is not None diff --git a/tests/media/test_media_storage.py b/tests/media/test_media_storage.py index 034d9ece0b..f4fbc0544a 100644 --- a/tests/media/test_media_storage.py +++ b/tests/media/test_media_storage.py @@ -419,6 +419,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): return channel + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_handle_missing_content_type(self) -> None: channel = self._req( b"attachment; filename=out" + self.test_image.extension, @@ -430,6 +435,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): headers.getRawHeaders(b"Content-Type"), [b"application/octet-stream"] ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_disposition_filename_ascii(self) -> None: """ If the filename is filename= then Synapse will decode it as an @@ -450,6 +460,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): ], ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_disposition_filenamestar_utf8escaped(self) -> None: """ If the filename is filename=*utf8'' then Synapse will @@ -475,6 +490,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): ], ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_disposition_none(self) -> None: """ If there is no filename, Content-Disposition should only @@ -491,6 +511,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): [b"inline" if self.test_image.is_inline else b"attachment"], ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_thumbnail_crop(self) -> None: """Test that a cropped remote thumbnail is available.""" self._test_thumbnail( @@ -500,6 +525,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_thumbnail_scale(self) -> None: """Test that a scaled remote thumbnail is available.""" self._test_thumbnail( @@ -509,6 +539,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_invalid_type(self) -> None: """An invalid thumbnail type is never available.""" self._test_thumbnail( @@ -519,7 +554,10 @@ class MediaRepoTests(unittest.HomeserverTestCase): ) @unittest.override_config( - {"thumbnail_sizes": [{"width": 32, "height": 32, "method": "scale"}]} + { + "thumbnail_sizes": [{"width": 32, "height": 32, "method": "scale"}], + "enable_authenticated_media": False, + }, ) def test_no_thumbnail_crop(self) -> None: """ @@ -533,7 +571,10 @@ class MediaRepoTests(unittest.HomeserverTestCase): ) @unittest.override_config( - {"thumbnail_sizes": [{"width": 32, "height": 32, "method": "crop"}]} + { + "thumbnail_sizes": [{"width": 32, "height": 32, "method": "crop"}], + "enable_authenticated_media": False, + } ) def test_no_thumbnail_scale(self) -> None: """ @@ -546,6 +587,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): unable_to_thumbnail=self.test_image.unable_to_thumbnail, ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_thumbnail_repeated_thumbnail(self) -> None: """Test that fetching the same thumbnail works, and deleting the on disk thumbnail regenerates it. @@ -720,6 +766,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): ) ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_x_robots_tag_header(self) -> None: """ Tests that the `X-Robots-Tag` header is present, which informs web crawlers @@ -733,6 +784,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): [b"noindex, nofollow, noarchive, noimageindex"], ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_cross_origin_resource_policy_header(self) -> None: """ Test that the Cross-Origin-Resource-Policy header is set to "cross-origin" @@ -747,6 +803,11 @@ class MediaRepoTests(unittest.HomeserverTestCase): [b"cross-origin"], ) + @unittest.override_config( + { + "enable_authenticated_media": False, + } + ) def test_unknown_v3_endpoint(self) -> None: """ If the v3 endpoint fails, try the r0 one. @@ -985,6 +1046,11 @@ class RemoteDownloadLimiterTestCase(unittest.HomeserverTestCase): d.callback(52428800) return d + @override_config( + { + "enable_authenticated_media": False, + } + ) @patch( "synapse.http.matrixfederationclient.read_body_with_max_size", read_body_with_max_size_30MiB, @@ -1060,6 +1126,7 @@ class RemoteDownloadLimiterTestCase(unittest.HomeserverTestCase): { "remote_media_download_per_second": "50M", "remote_media_download_burst_count": "50M", + "enable_authenticated_media": False, } ) @patch( @@ -1119,7 +1186,12 @@ class RemoteDownloadLimiterTestCase(unittest.HomeserverTestCase): ) assert channel.code == 200 - @override_config({"remote_media_download_burst_count": "87M"}) + @override_config( + { + "remote_media_download_burst_count": "87M", + "enable_authenticated_media": False, + } + ) @patch( "synapse.http.matrixfederationclient.read_body_with_max_size", read_body_with_max_size_30MiB, @@ -1159,7 +1231,7 @@ class RemoteDownloadLimiterTestCase(unittest.HomeserverTestCase): ) assert channel2.code == 429 - @override_config({"max_upload_size": "29M"}) + @override_config({"max_upload_size": "29M", "enable_authenticated_media": False}) @patch( "synapse.http.matrixfederationclient.read_body_with_max_size", read_body_with_max_size_30MiB, diff --git a/tests/replication/test_multi_media_repo.py b/tests/replication/test_multi_media_repo.py index 6fc4600c41..f36af877c4 100644 --- a/tests/replication/test_multi_media_repo.py +++ b/tests/replication/test_multi_media_repo.py @@ -40,6 +40,7 @@ from tests.http import ( from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.server import FakeChannel, FakeTransport, make_request from tests.test_utils import SMALL_PNG +from tests.unittest import override_config logger = logging.getLogger(__name__) @@ -148,6 +149,7 @@ class MediaRepoShardTestCase(BaseMultiWorkerStreamTestCase): return channel, request + @override_config({"enable_authenticated_media": False}) def test_basic(self) -> None: """Test basic fetching of remote media from a single worker.""" hs1 = self.make_worker_hs("synapse.app.generic_worker") @@ -164,6 +166,7 @@ class MediaRepoShardTestCase(BaseMultiWorkerStreamTestCase): self.assertEqual(channel.code, 200) self.assertEqual(channel.result["body"], b"Hello!") + @override_config({"enable_authenticated_media": False}) def test_download_simple_file_race(self) -> None: """Test that fetching remote media from two different processes at the same time works. @@ -203,6 +206,7 @@ class MediaRepoShardTestCase(BaseMultiWorkerStreamTestCase): # We expect only one new file to have been persisted. self.assertEqual(start_count + 1, self._count_remote_media()) + @override_config({"enable_authenticated_media": False}) def test_download_image_race(self) -> None: """Test that fetching remote *images* from two different processes at the same time works. diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 6351326fff..5483f8f37f 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -30,7 +30,7 @@ from twisted.web.resource import Resource import synapse.rest.admin from synapse.http.server import JsonResource from synapse.rest.admin import VersionServlet -from synapse.rest.client import login, room +from synapse.rest.client import login, media, room from synapse.server import HomeServer from synapse.util import Clock @@ -60,6 +60,7 @@ class QuarantineMediaTestCase(unittest.HomeserverTestCase): synapse.rest.admin.register_servlets, synapse.rest.admin.register_servlets_for_media_repo, login.register_servlets, + media.register_servlets, room.register_servlets, ] @@ -74,7 +75,7 @@ class QuarantineMediaTestCase(unittest.HomeserverTestCase): """Ensure a piece of media is quarantined when trying to access it.""" channel = self.make_request( "GET", - f"/_matrix/media/v3/download/{server_and_media_id}", + f"/_matrix/client/v1/media/download/{server_and_media_id}", shorthand=False, access_token=admin_user_tok, ) @@ -131,7 +132,7 @@ class QuarantineMediaTestCase(unittest.HomeserverTestCase): # Attempt to access the media channel = self.make_request( "GET", - f"/_matrix/media/v3/download/{server_name_and_media_id}", + f"/_matrix/client/v1/media/download/{server_name_and_media_id}", shorthand=False, access_token=non_admin_user_tok, ) @@ -295,7 +296,7 @@ class QuarantineMediaTestCase(unittest.HomeserverTestCase): # Attempt to access each piece of media channel = self.make_request( "GET", - f"/_matrix/media/v3/download/{server_and_media_id_2}", + f"/_matrix/client/v1/media/download/{server_and_media_id_2}", shorthand=False, access_token=non_admin_user_tok, ) diff --git a/tests/rest/admin/test_media.py b/tests/rest/admin/test_media.py index f378165513..19c244cfcf 100644 --- a/tests/rest/admin/test_media.py +++ b/tests/rest/admin/test_media.py @@ -36,6 +36,7 @@ from synapse.util import Clock from tests import unittest from tests.test_utils import SMALL_PNG +from tests.unittest import override_config VALID_TIMESTAMP = 1609459200000 # 2021-01-01 in milliseconds INVALID_TIMESTAMP_IN_S = 1893456000 # 2030-01-01 in seconds @@ -126,6 +127,7 @@ class DeleteMediaByIDTestCase(_AdminMediaTests): self.assertEqual(400, channel.code, msg=channel.json_body) self.assertEqual("Can only delete local media", channel.json_body["error"]) + @override_config({"enable_authenticated_media": False}) def test_delete_media(self) -> None: """ Tests that delete a media is successfully @@ -371,6 +373,7 @@ class DeleteMediaByDateSizeTestCase(_AdminMediaTests): self._access_media(server_and_media_id, False) + @override_config({"enable_authenticated_media": False}) def test_keep_media_by_date(self) -> None: """ Tests that media is not deleted if it is newer than `before_ts` @@ -408,6 +411,7 @@ class DeleteMediaByDateSizeTestCase(_AdminMediaTests): self._access_media(server_and_media_id, False) + @override_config({"enable_authenticated_media": False}) def test_keep_media_by_size(self) -> None: """ Tests that media is not deleted if its size is smaller than or equal @@ -443,6 +447,7 @@ class DeleteMediaByDateSizeTestCase(_AdminMediaTests): self._access_media(server_and_media_id, False) + @override_config({"enable_authenticated_media": False}) def test_keep_media_by_user_avatar(self) -> None: """ Tests that we do not delete media if is used as a user avatar @@ -487,6 +492,7 @@ class DeleteMediaByDateSizeTestCase(_AdminMediaTests): self._access_media(server_and_media_id, False) + @override_config({"enable_authenticated_media": False}) def test_keep_media_by_room_avatar(self) -> None: """ Tests that we do not delete media if it is used as a room avatar diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 668ccb89ff..6d050e7784 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -45,6 +45,7 @@ from synapse.rest.client import ( devices, login, logout, + media, profile, register, room, @@ -3517,6 +3518,7 @@ class UserMediaRestTestCase(unittest.HomeserverTestCase): servlets = [ synapse.rest.admin.register_servlets, login.register_servlets, + media.register_servlets, ] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -4023,7 +4025,7 @@ class UserMediaRestTestCase(unittest.HomeserverTestCase): # Try to access a media and to create `last_access_ts` channel = self.make_request( "GET", - f"/_matrix/media/v3/download/{server_and_media_id}", + f"/_matrix/client/v1/media/download/{server_and_media_id}", shorthand=False, access_token=user_token, ) diff --git a/tests/rest/media/test_domain_blocking.py b/tests/rest/media/test_domain_blocking.py index 72205c6bb3..49d81f4b28 100644 --- a/tests/rest/media/test_domain_blocking.py +++ b/tests/rest/media/test_domain_blocking.py @@ -91,7 +91,8 @@ class MediaDomainBlockingTests(unittest.HomeserverTestCase): { # Disable downloads from a domain we won't be requesting downloads from. # This proves we haven't broken anything. - "prevent_media_downloads_from": ["not-listed.com"] + "prevent_media_downloads_from": ["not-listed.com"], + "enable_authenticated_media": False, } ) def test_remote_media_normally_unblocked(self) -> None: @@ -132,6 +133,7 @@ class MediaDomainBlockingTests(unittest.HomeserverTestCase): # This proves we haven't broken anything. "prevent_media_downloads_from": ["not-listed.com"], "dynamic_thumbnails": True, + "enable_authenticated_media": False, } ) def test_remote_media_thumbnail_normally_unblocked(self) -> None: diff --git a/tests/rest/media/test_url_preview.py b/tests/rest/media/test_url_preview.py index a96f0e7fca..103d7662d9 100644 --- a/tests/rest/media/test_url_preview.py +++ b/tests/rest/media/test_url_preview.py @@ -42,6 +42,7 @@ from synapse.util.stringutils import parse_and_validate_mxc_uri from tests import unittest from tests.server import FakeTransport from tests.test_utils import SMALL_PNG +from tests.unittest import override_config try: import lxml @@ -1259,6 +1260,7 @@ class URLPreviewTests(unittest.HomeserverTestCase): self.assertIsNone(_port) return host, media_id + @override_config({"enable_authenticated_media": False}) def test_storage_providers_exclude_files(self) -> None: """Test that files are not stored in or fetched from storage providers.""" host, media_id = self._download_image() @@ -1301,6 +1303,7 @@ class URLPreviewTests(unittest.HomeserverTestCase): "URL cache file was unexpectedly retrieved from a storage provider", ) + @override_config({"enable_authenticated_media": False}) def test_storage_providers_exclude_thumbnails(self) -> None: """Test that thumbnails are not stored in or fetched from storage providers.""" host, media_id = self._download_image()