diff --git a/synapse/media/media_repository.py b/synapse/media/media_repository.py index 8d5a103574..648c1cce6c 100644 --- a/synapse/media/media_repository.py +++ b/synapse/media/media_repository.py @@ -19,6 +19,7 @@ # [This file includes modifications made by New Vector Limited] # # +import base64 import errno import hashlib import hmac @@ -1650,7 +1651,9 @@ class MediaRepository: # the nearest 5 minutes, so that a CDN/cache can always cache the # media for a little bit exp = self.clock.time_msec() + self.hs.config.media.redirect_ttl_ms - parameters_str = urlencode(parameters) + parameters_str = base64.urlsafe_b64encode( + urlencode(sorted(parameters.items())).encode("utf-8") + ).decode("ascii") key = self.thumbnail_media_key( media_id=media_id, parameters=parameters_str, diff --git a/synapse/rest/synapse/media/thumbnail.py b/synapse/rest/synapse/media/thumbnail.py index 0a8364de9f..8880462a1b 100644 --- a/synapse/rest/synapse/media/thumbnail.py +++ b/synapse/rest/synapse/media/thumbnail.py @@ -13,6 +13,7 @@ # # +import base64 import logging from typing import TYPE_CHECKING from urllib.parse import parse_qs @@ -43,16 +44,17 @@ class ThumbnailResource(DirectServeJsonResource): Serves thumbnails from the media repository, with a temporary signed URL which expires after a set amount of time. - GET /_synapse/media/thumbnail/{media_id}/height={height}&width={width}&...?exp={exp}&sig={sig} + GET /_synapse/media/thumbnail/{media_id}/{parameters}?exp={exp}&sig={sig} The intent of this resource is to allow the federation and client media APIs to issue redirects to a signed URL that can then be cached by a CDN. This endpoint doesn't require any extra header, and is authenticated using the signature in the URL parameters. - The reason the `height`, `width` and other parameters are not part of the - query string but instead part of the path is to ignoring the query string - when caching the URL, which is possible with some CDNs. + The parameters are encoded as a form-urlencoded then base64 encoded string. + This avoids any automatic url decoding Twisted might do. The reason they are + part of the URL and not the query string is to ignore the query string when + caching the URL, which is possible with some CDNs. """ isLeaf = True @@ -111,7 +113,7 @@ class ThumbnailResource(DirectServeJsonResource): raise NotFoundError() # Now parse and check the parameters - args = parse_qs(parameters) + args = parse_qs(base64.urlsafe_b64decode(parameters)) width = parse_integer_from_args(args, "width", required=True) height = parse_integer_from_args(args, "height", required=True) method = parse_string_from_args(args, "method", "scale") diff --git a/tests/federation/test_federation_media.py b/tests/federation/test_federation_media.py index ea78f762d1..5c38590fd4 100644 --- a/tests/federation/test_federation_media.py +++ b/tests/federation/test_federation_media.py @@ -415,10 +415,9 @@ class FederationThumbnailTest(unittest.FederatingHomeserverTestCase): # Part 2: Redirect URL self.assertEqual(lines[4], f"--{boundary}") # Boundary for the next part # The Location header contains dynamic parts (exp, sig), so use regex - # Note: thumbnail URL includes width, height, method, and type as path parameters self.assertRegex( lines[5], - rf"^Location: https://test/_synapse/media/thumbnail/{content_uri.media_id}/width=32&height=32&method=scale&type=image%2Fpng\?exp=\d+&sig=\w+$", + rf"^Location: https://test/_synapse/media/thumbnail/{content_uri.media_id}/[^?]+\?exp=\d+&sig=\w+$", ) self.assertEqual(lines[6], "") # First empty line after Location header self.assertEqual(lines[7], "") # Second empty line after Location header