From dfcfecae45e4db7da7a0cfb9b1af7f34732e6366 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 27 Jan 2022 16:02:09 +0000 Subject: [PATCH] Add tests --- synapse/handlers/profile.py | 8 +- synapse/handlers/room_member.py | 4 +- tests/handlers/test_profile.py | 106 +++++++++++++++++- tests/rest/client/test_profile.py | 172 ++++++++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 6b48de2026..33f8678e50 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -293,7 +293,7 @@ class ProfileHandler: 400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,) ) - await self.check_avatar_size_and_content_type(new_avatar_url) + await self.check_avatar_size_and_mime_type(new_avatar_url) avatar_url_to_set: Optional[str] = new_avatar_url if new_avatar_url == "": @@ -316,7 +316,7 @@ class ProfileHandler: await self._update_join_states(requester, target_user) - async def check_avatar_size_and_content_type(self, mxc: str) -> None: + async def check_avatar_size_and_mime_type(self, mxc: str) -> None: """Check that the size and content type of the avatar at the given MXC URI are within the configured limits. @@ -327,11 +327,11 @@ class ProfileHandler: SynapseError with an M_FORBIDDEN error code if the avatar doesn't fit within the limits allowed by the configuration. """ - if not await self._check_avatar_size_and_content_type(mxc): + if not await self._check_avatar_size_and_mime_type(mxc): raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN) @cached() - async def _check_avatar_size_and_content_type(self, mxc: str) -> bool: + async def _check_avatar_size_and_mime_type(self, mxc: str) -> bool: """Check that the size and content type of the avatar at the given MXC URI are within the configured limits. diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 3bbd8ffd9a..4e979dd8e2 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -591,8 +591,8 @@ class RoomMemberHandler(metaclass=abc.ABCMeta): ) if "avatar_url" in content: - await self.profile_handler.check_avatar_size_and_content_type( - content["avatar_url"], + await self.profile_handler.check_avatar_size_and_mime_type( + content["avatar_url"] ) # The event content should *not* include the authorising user as diff --git a/tests/handlers/test_profile.py b/tests/handlers/test_profile.py index c153018fd8..b7d6187a21 100644 --- a/tests/handlers/test_profile.py +++ b/tests/handlers/test_profile.py @@ -11,12 +11,13 @@ # 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 Any, Dict from unittest.mock import Mock import synapse.types from synapse.api.errors import AuthError, SynapseError from synapse.rest import admin +from synapse.server import HomeServer from synapse.types import UserID from tests import unittest @@ -46,7 +47,7 @@ class ProfileTestCase(unittest.HomeserverTestCase): ) return hs - def prepare(self, reactor, clock, hs): + def prepare(self, reactor, clock, hs: HomeServer): self.store = hs.get_datastore() self.frank = UserID.from_string("@1234abcd:test") @@ -248,3 +249,104 @@ class ProfileTestCase(unittest.HomeserverTestCase): ), SynapseError, ) + + def test_avatar_constraints_no_config(self): + """Tests that the method to check an avatar against configured constraints skips + all of its check if no constraint is configured. + """ + # The first check that's done by this method is whether the file exists; if we + # don't get an error on a non-existing file then it means all of the checks were + # successfully skipped. + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/unknown_file") + ) + self.assertTrue(allowed) + + @unittest.override_config( + { + "max_avatar_size": 50, + } + ) + def test_avatar_constraints_missing(self): + """Tests that an avatar isn't allowed if the file at the given MXC URI couldn't + be found. + """ + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/unknown_file") + ) + self.assertFalse(allowed) + + @unittest.override_config( + { + "max_avatar_size": 50, + } + ) + def test_avatar_constraints_file_size(self): + """Tests that a file that's above the allowed file size is forbidden but one + that's below it is allowed. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/small") + ) + self.assertTrue(allowed) + + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/big") + ) + self.assertFalse(allowed) + + @unittest.override_config( + { + "allowed_avatar_mimetypes": ["image/png"], + } + ) + def test_avatar_constraint_mime_type(self): + """Tests that a file with an unauthorised MIME type is forbidden but one with + an authorised content type is allowed. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/good") + ) + self.assertTrue(allowed) + + allowed = self.get_success( + self.handler._check_avatar_size_and_mime_type("mxc://test/bad") + ) + self.assertFalse(allowed) + + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): + """Stores metadata about files in the database. + + Args: + names_and_props: A dictionary with one entry per file, with the key being the + file's name, and the value being a dictionary of properties. Supported + properties are "mimetype" (for the file's type) and "size" (for the + file's size). + """ + store = self.hs.get_datastore() + + for name, props in names_and_props.items(): + self.get_success( + store.store_local_media( + media_id=name, + media_type=props.get("mimetype", "image/png"), + time_now_ms=self.clock.time_msec(), + upload_name=None, + media_length=props.get("size", 50), + user_id=UserID.from_string("@rin:test"), + ) + ) diff --git a/tests/rest/client/test_profile.py b/tests/rest/client/test_profile.py index 2860579c2e..140e62a46a 100644 --- a/tests/rest/client/test_profile.py +++ b/tests/rest/client/test_profile.py @@ -13,8 +13,12 @@ # limitations under the License. """Tests REST events for /profile paths.""" +from typing import Any, Dict + +from synapse.api.errors import Codes from synapse.rest import admin from synapse.rest.client import login, profile, room +from synapse.types import UserID from tests import unittest @@ -25,6 +29,7 @@ class ProfileTestCase(unittest.HomeserverTestCase): admin.register_servlets_for_client_rest_resource, login.register_servlets, profile.register_servlets, + room.register_servlets, ] def make_homeserver(self, reactor, clock): @@ -150,6 +155,173 @@ class ProfileTestCase(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200, channel.result) return channel.json_body.get("avatar_url") + @unittest.override_config( + { + "max_avatar_size": 50, + } + ) + def test_avatar_size_limit_global(self): + """Tests that the maximum size limit for avatars is enforced when updating a + global profile. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + channel = self.make_request( + "PUT", + "/profile/%s/avatar_url" % (self.owner,), + content={"avatar_url": "mxc://test/big"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + "/profile/%s/avatar_url" % (self.owner,), + content={"avatar_url": "mxc://test/small"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config( + { + "max_avatar_size": 50, + } + ) + def test_avatar_size_limit_per_room(self): + """Tests that the maximum size limit for avatars is enforced when updating a + per-room profile. + """ + self._setup_local_files( + { + "small": {"size": 40}, + "big": {"size": 60}, + } + ) + + room_id = self.helper.create_room_as(tok=self.owner_tok) + + channel = self.make_request( + "PUT", + "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner), + content={"membership": "join", "avatar_url": "mxc://test/big"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner), + content={"membership": "join", "avatar_url": "mxc://test/small"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config( + { + "allowed_avatar_mimetypes": ["image/png"], + } + ) + def test_avatar_allowed_mime_type_global(self): + """Tests that the MIME type whitelist for avatars is enforced when updating a + global profile. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + channel = self.make_request( + "PUT", + "/profile/%s/avatar_url" % (self.owner,), + content={"avatar_url": "mxc://test/bad"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + "/profile/%s/avatar_url" % (self.owner,), + content={"avatar_url": "mxc://test/good"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + @unittest.override_config( + { + "allowed_avatar_mimetypes": ["image/png"], + } + ) + def test_avatar_allowed_mime_type_per_room(self): + """Tests that the MIME type whitelist for avatars is enforced when updating a + per-room profile. + """ + self._setup_local_files( + { + "good": {"mimetype": "image/png"}, + "bad": {"mimetype": "application/octet-stream"}, + } + ) + + room_id = self.helper.create_room_as(tok=self.owner_tok) + + channel = self.make_request( + "PUT", + "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner), + content={"membership": "join", "avatar_url": "mxc://test/bad"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 403, channel.result) + self.assertEqual( + channel.json_body["errcode"], Codes.FORBIDDEN, channel.json_body + ) + + channel = self.make_request( + "PUT", + "/rooms/%s/state/m.room.member/%s" % (room_id, self.owner), + content={"membership": "join", "avatar_url": "mxc://test/good"}, + access_token=self.owner_tok, + ) + self.assertEqual(channel.code, 200, channel.result) + + def _setup_local_files(self, names_and_props: Dict[str, Dict[str, Any]]): + """Stores metadata about files in the database. + + Args: + names_and_props: A dictionary with one entry per file, with the key being the + file's name, and the value being a dictionary of properties. Supported + properties are "mimetype" (for the file's type) and "size" (for the + file's size). + """ + store = self.hs.get_datastore() + + for name, props in names_and_props.items(): + self.get_success( + store.store_local_media( + media_id=name, + media_type=props.get("mimetype", "image/png"), + time_now_ms=self.clock.time_msec(), + upload_name=None, + media_length=props.get("size", 50), + user_id=UserID.from_string("@rin:test"), + ) + ) + class ProfilesRestrictedTestCase(unittest.HomeserverTestCase):