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
|
