diff options
| -rw-r--r-- | changelog.d/15680.bugfix | 1 | ||||
| -rw-r--r-- | synapse/media/_base.py | 15 | ||||
| -rw-r--r-- | tests/media/test_base.py | 12 | ||||
| -rw-r--r-- | tests/media/test_media_storage.py | 20 |
4 files changed, 29 insertions, 19 deletions
diff --git a/changelog.d/15680.bugfix b/changelog.d/15680.bugfix new file mode 100644 index 0000000000..04ac19b4ec --- /dev/null +++ b/changelog.d/15680.bugfix @@ -0,0 +1 @@ +Fix a long-standing bug where media files were served in an unsafe manner. Contributed by @joshqou. diff --git a/synapse/media/_base.py b/synapse/media/_base.py index ef8334ae25..20cb8b9010 100644 --- a/synapse/media/_base.py +++ b/synapse/media/_base.py @@ -152,6 +152,9 @@ def add_file_headers( content_type = media_type request.setHeader(b"Content-Type", content_type.encode("UTF-8")) + + # Use a Content-Disposition of attachment to force download of media. + disposition = "attachment" if upload_name: # RFC6266 section 4.1 [1] defines both `filename` and `filename*`. # @@ -173,11 +176,17 @@ def add_file_headers( # correctly interpret those as of 0.99.2 and (b) they are a bit of a pain and we # may as well just do the filename* version. if _can_encode_filename_as_token(upload_name): - disposition = "inline; filename=%s" % (upload_name,) + disposition = "%s; filename=%s" % ( + disposition, + upload_name, + ) else: - disposition = "inline; filename*=utf-8''%s" % (_quote(upload_name),) + disposition = "%s; filename*=utf-8''%s" % ( + disposition, + _quote(upload_name), + ) - request.setHeader(b"Content-Disposition", disposition.encode("ascii")) + request.setHeader(b"Content-Disposition", disposition.encode("ascii")) # cache for at least a day. # XXX: we might want to turn this off for data we don't want to diff --git a/tests/media/test_base.py b/tests/media/test_base.py index 66498c744d..4728c80969 100644 --- a/tests/media/test_base.py +++ b/tests/media/test_base.py @@ -20,12 +20,12 @@ from tests import unittest class GetFileNameFromHeadersTests(unittest.TestCase): # input -> expected result TEST_CASES = { - b"inline; filename=abc.txt": "abc.txt", - b'inline; filename="azerty"': "azerty", - b'inline; filename="aze%20rty"': "aze%20rty", - b'inline; filename="aze"rty"': 'aze"rty', - b'inline; filename="azer;ty"': "azer;ty", - b"inline; filename*=utf-8''foo%C2%A3bar": "foo£bar", + b"attachment; filename=abc.txt": "abc.txt", + b'attachment; filename="azerty"': "azerty", + b'attachment; filename="aze%20rty"': "aze%20rty", + b'attachment; filename="aze"rty"': 'aze"rty', + b'attachment; filename="azer;ty"': "azer;ty", + b"attachment; filename*=utf-8''foo%C2%A3bar": "foo£bar", } def tests(self) -> None: diff --git a/tests/media/test_media_storage.py b/tests/media/test_media_storage.py index f0f2da65db..ea0051dde4 100644 --- a/tests/media/test_media_storage.py +++ b/tests/media/test_media_storage.py @@ -317,7 +317,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): def test_handle_missing_content_type(self) -> None: channel = self._req( - b"inline; filename=out" + self.test_image.extension, + b"attachment; filename=out" + self.test_image.extension, include_content_type=False, ) headers = channel.headers @@ -331,7 +331,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): If the filename is filename=<ascii> then Synapse will decode it as an ASCII string, and use filename= in the response. """ - channel = self._req(b"inline; filename=out" + self.test_image.extension) + channel = self._req(b"attachment; filename=out" + self.test_image.extension) headers = channel.headers self.assertEqual( @@ -339,7 +339,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): ) self.assertEqual( headers.getRawHeaders(b"Content-Disposition"), - [b"inline; filename=out" + self.test_image.extension], + [b"attachment; filename=out" + self.test_image.extension], ) def test_disposition_filenamestar_utf8escaped(self) -> None: @@ -350,7 +350,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): """ filename = parse.quote("\u2603".encode()).encode("ascii") channel = self._req( - b"inline; filename*=utf-8''" + filename + self.test_image.extension + b"attachment; filename*=utf-8''" + filename + self.test_image.extension ) headers = channel.headers @@ -359,13 +359,13 @@ class MediaRepoTests(unittest.HomeserverTestCase): ) self.assertEqual( headers.getRawHeaders(b"Content-Disposition"), - [b"inline; filename*=utf-8''" + filename + self.test_image.extension], + [b"attachment; filename*=utf-8''" + filename + self.test_image.extension], ) def test_disposition_none(self) -> None: """ - If there is no filename, one isn't passed on in the Content-Disposition - of the request. + If there is no filename, Content-Disposition should only + be a disposition type. """ channel = self._req(None) @@ -373,7 +373,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): self.assertEqual( headers.getRawHeaders(b"Content-Type"), [self.test_image.content_type] ) - self.assertEqual(headers.getRawHeaders(b"Content-Disposition"), None) + self.assertEqual(headers.getRawHeaders(b"Content-Disposition"), [b"attachment"]) def test_thumbnail_crop(self) -> None: """Test that a cropped remote thumbnail is available.""" @@ -612,7 +612,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): Tests that the `X-Robots-Tag` header is present, which informs web crawlers to not index, archive, or follow links in media. """ - channel = self._req(b"inline; filename=out" + self.test_image.extension) + channel = self._req(b"attachment; filename=out" + self.test_image.extension) headers = channel.headers self.assertEqual( @@ -625,7 +625,7 @@ class MediaRepoTests(unittest.HomeserverTestCase): Test that the Cross-Origin-Resource-Policy header is set to "cross-origin" allowing web clients to embed media from the downloads API. """ - channel = self._req(b"inline; filename=out" + self.test_image.extension) + channel = self._req(b"attachment; filename=out" + self.test_image.extension) headers = channel.headers |