This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add config option to prevent media downloads from listed domains. #15197
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
aa7c7af
Prevent downloads from configured domains
turt2live 5215707
Changelog
turt2live 960650b
Define a variable that we use
turt2live f7fa9f7
Remove dollar signs
turt2live 5c6e2ec
Merge branch 'develop' into travis/block-media-by-domain
turt2live 0d2ea49
Try writing the file too?
turt2live c4855c2
I guess await doesn't work
turt2live d9daa63
Access properties that might work
turt2live 33f3107
Appease CI
turt2live b292cd1
Merge branch 'develop' into travis/block-media-by-domain
erikjohnston 298c96c
Fix up tests
erikjohnston bd13239
Apply suggestions from code review
turt2live 2dd8d5d
Finish field rename
turt2live e339fa9
Merge branch 'develop' into travis/block-media-by-domain
turt2live 673e4fc
Fix imports
turt2live 9c6e0b5
Fix code style
turt2live 2987d9a
Clean up imports
turt2live File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add an option to prevent media downloads from configured domains. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
# Copyright 2023 The Matrix.org Foundation C.I.C. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
from typing import Dict | ||
|
||
from twisted.test.proto_helpers import MemoryReactor | ||
from twisted.web.resource import Resource | ||
|
||
from synapse.media._base import FileInfo | ||
from synapse.server import HomeServer | ||
from synapse.util import Clock | ||
|
||
from tests import unittest | ||
from tests.test_utils import SMALL_PNG | ||
from tests.unittest import override_config | ||
|
||
|
||
class MediaDomainBlockingTests(unittest.HomeserverTestCase): | ||
remote_media_id = "doesnotmatter" | ||
remote_server_name = "evil.com" | ||
erikjohnston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: | ||
self.store = hs.get_datastores().main | ||
|
||
# Inject a piece of media. We'll use this to ensure we're returning a sane | ||
# response when we're not supposed to block it, distinguishing a media block | ||
# from a regular 404. | ||
file_id = "abcdefg12345" | ||
file_info = FileInfo(server_name=self.remote_server_name, file_id=file_id) | ||
with hs.get_media_repository().media_storage.store_into_file(file_info) as ( | ||
f, | ||
fname, | ||
finish, | ||
): | ||
f.write(SMALL_PNG) | ||
self.get_success(finish()) | ||
|
||
self.get_success( | ||
self.store.store_cached_remote_media( | ||
origin=self.remote_server_name, | ||
media_id=self.remote_media_id, | ||
media_type="image/png", | ||
media_length=1, | ||
time_now_ms=clock.time_msec(), | ||
upload_name="test.png", | ||
filesystem_id=file_id, | ||
) | ||
) | ||
|
||
def create_resource_dict(self) -> Dict[str, Resource]: | ||
# We need to manually set the resource tree to include media, the | ||
# default only does `/_matrix/client` APIs. | ||
return {"/_matrix/media": self.hs.get_media_repository_resource()} | ||
|
||
@override_config( | ||
{ | ||
# Disable downloads from the domain we'll be trying to download from. | ||
# Should result in a 404. | ||
"prevent_media_downloads_from": ["evil.com"] | ||
} | ||
) | ||
def test_cannot_download_blocked_media(self) -> None: | ||
""" | ||
Tests to ensure that remote media which is blocked cannot be downloaded. | ||
""" | ||
response = self.make_request( | ||
"GET", | ||
f"/_matrix/media/v3/download/evil.com/{self.remote_media_id}", | ||
shorthand=False, | ||
) | ||
self.assertEqual(response.code, 404) | ||
|
||
@override_config( | ||
{ | ||
# 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"] | ||
} | ||
) | ||
def test_remote_media_normally_unblocked(self) -> None: | ||
""" | ||
Tests to ensure that remote media is normally able to be downloaded | ||
when no domain block is in place. | ||
erikjohnston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
response = self.make_request( | ||
"GET", | ||
f"/_matrix/media/v3/download/evil.com/{self.remote_media_id}", | ||
shorthand=False, | ||
) | ||
self.assertEqual(response.code, 200) | ||
|
||
@override_config( | ||
{ | ||
# Disable downloads from the domain we'll be trying to download from. | ||
# Should result in a 404. | ||
"prevent_media_downloads_from": ["evil.com"], | ||
"dynamic_thumbnails": True, | ||
} | ||
) | ||
def test_cannot_download_blocked_media_thumbnail(self) -> None: | ||
""" | ||
Same test as test_cannot_download_blocked_media but for thumbnails. | ||
""" | ||
response = self.make_request( | ||
"GET", | ||
f"/_matrix/media/v3/thumbnail/evil.com/{self.remote_media_id}?width=100&height=100", | ||
shorthand=False, | ||
content={"width": 100, "height": 100}, | ||
) | ||
self.assertEqual(response.code, 404) | ||
|
||
@override_config( | ||
{ | ||
# 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"], | ||
"dynamic_thumbnails": True, | ||
} | ||
) | ||
def test_remote_media_thumbnail_normally_unblocked(self) -> None: | ||
""" | ||
Same test as test_remote_media_normally_unblocked but for thumbnails. | ||
""" | ||
response = self.make_request( | ||
"GET", | ||
f"/_matrix/media/v3/thumbnail/evil.com/{self.remote_media_id}?width=100&height=100", | ||
shorthand=False, | ||
) | ||
self.assertEqual(response.code, 200) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking about the name. This is a block list, if you later want to use the concept with allow lists, you have to rename the parameter.
Wouldn't it be better to adapt the name to the current parameters for allowing and disallowing federation or url previews?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow lists aren't currently planned for this feature - I'd favour a config migration (deprecation) later on if we decide to go that route.