Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Configurable limits on avatars #11846

Merged
merged 5 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11846.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow configuring a maximum file size as well as a list of allowed content types for avatars.
14 changes: 14 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,20 @@ limit_remote_rooms:
#
#allow_per_room_profiles: false

# The largest allowed file size for a user avatar. Defaults to no restriction.
#
# Note that user avatar changes will not work if this is set without
# using Synapse's media repository.
#
#max_avatar_size: 10M

# The MIME types allowed for user avatars. Defaults to no restriction.
#
# Note that user avatar changes will not work if this is set without
# using Synapse's media repository.
#
#allowed_avatar_mimetypes: ["image/png", "image/jpeg", "image/gif"]

# How long to keep redacted events in unredacted form in the database. After
# this period redacted events get replaced with their redacted form in the DB.
#
Expand Down
27 changes: 27 additions & 0 deletions synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,19 @@ def read_config(self, config, **kwargs):
# events with profile information that differ from the target's global profile.
self.allow_per_room_profiles = config.get("allow_per_room_profiles", True)

# The maximum size an avatar can have, in bytes.
self.max_avatar_size = config.get("max_avatar_size")
if self.max_avatar_size is not None:
self.max_avatar_size = self.parse_size(self.max_avatar_size)

# The MIME types allowed for an avatar.
self.allowed_avatar_mimetypes = config.get("allowed_avatar_mimetypes")
if self.allowed_avatar_mimetypes and not isinstance(
self.allowed_avatar_mimetypes,
list,
):
raise ConfigError("allowed_avatar_mimetypes must be a list")

self.listeners = [parse_listener_def(x) for x in config.get("listeners", [])]

# no_tls is not really supported any more, but let's grandfather it in
Expand Down Expand Up @@ -1168,6 +1181,20 @@ def generate_config_section(
#
#allow_per_room_profiles: false

# The largest allowed file size for a user avatar. Defaults to no restriction.
#
# Note that user avatar changes will not work if this is set without
# using Synapse's media repository.
babolivier marked this conversation as resolved.
Show resolved Hide resolved
#
#max_avatar_size: 10M

# The MIME types allowed for user avatars. Defaults to no restriction.
#
# Note that user avatar changes will not work if this is set without
# using Synapse's media repository.
#
#allowed_avatar_mimetypes: ["image/png", "image/jpeg", "image/gif"]

# How long to keep redacted events in unredacted form in the database. After
# this period redacted events get replaced with their redacted form in the DB.
#
Expand Down
67 changes: 67 additions & 0 deletions synapse/handlers/profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
create_requester,
get_domain_from_id,
)
from synapse.util.caches.descriptors import cached
from synapse.util.stringutils import parse_and_validate_mxc_uri

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand Down Expand Up @@ -64,6 +66,11 @@ def __init__(self, hs: "HomeServer"):
self.user_directory_handler = hs.get_user_directory_handler()
self.request_ratelimiter = hs.get_request_ratelimiter()

self.max_avatar_size = hs.config.server.max_avatar_size
self.allowed_avatar_mimetypes = hs.config.server.allowed_avatar_mimetypes

self.server_name = hs.config.server.server_name

if hs.config.worker.run_background_tasks:
self.clock.looping_call(
self._update_remote_profile_cache, self.PROFILE_UPDATE_MS
Expand Down Expand Up @@ -286,6 +293,9 @@ async def set_avatar_url(
400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,)
)

if not await self.check_avatar_size_and_mime_type(new_avatar_url):
raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN)

avatar_url_to_set: Optional[str] = new_avatar_url
if new_avatar_url == "":
avatar_url_to_set = None
Expand All @@ -307,6 +317,63 @@ async def set_avatar_url(

await self._update_join_states(requester, target_user)

@cached()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a hug deal, but do we expect this to be called often enough with the same mxc that we should cache it?

Copy link
Contributor Author

@babolivier babolivier Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because this is called both when setting your global profile and when setting a per-room one. Except setting a global profile then updates your per-room profile in every room you're in. So if I'm in hundreds of rooms then a single global avatar change is going to trigger this function to be called hundreds of times with the same mxc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Pretty much trying to make #1297 not worse!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep exactly!

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.

Args:
mxc: The MXC URI at which the avatar can be found.

Returns:
A boolean indicating whether the file can be allowed to be set as an avatar.
"""
if not self.max_avatar_size and not self.allowed_avatar_mimetypes:
return True

server_name, _, media_id = parse_and_validate_mxc_uri(mxc)

if server_name == self.server_name:
media_info = await self.store.get_local_media(media_id)
else:
media_info = await self.store.get_cached_remote_media(server_name, media_id)

if media_info is None:
# Both configuration options need to access the file's metadata, and
# retrieving remote avatars just for this becomes a bit of a faff, especially
# if e.g. the file is too big. It's also generally safe to assume most files
# used as avatar are uploaded locally, or if the upload didn't happen as part
# of a PUT request on /avatar_url that the file was at least previewed by the
# user locally (and therefore downloaded to the remote media cache).
logger.warning("Forbidding avatar change to %s: avatar not on server", mxc)
return False

if self.max_avatar_size:
# Ensure avatar does not exceed max allowed avatar size
if media_info["media_length"] > self.max_avatar_size:
logger.warning(
"Forbidding avatar change to %s: %d bytes is above the allowed size "
"limit",
mxc,
media_info["media_length"],
)
return False

if self.allowed_avatar_mimetypes:
# Ensure the avatar's file type is allowed
if (
self.allowed_avatar_mimetypes
and media_info["media_type"] not in self.allowed_avatar_mimetypes
babolivier marked this conversation as resolved.
Show resolved Hide resolved
):
logger.warning(
"Forbidding avatar change to %s: mimetype %s not allowed",
mxc,
media_info["media_type"],
)
return False

return True

async def on_profile_query(self, args: JsonDict) -> JsonDict:
"""Handles federation profile query requests."""

Expand Down
6 changes: 6 additions & 0 deletions synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,12 @@ async def update_membership_locked(
errcode=Codes.BAD_JSON,
)

if "avatar_url" in content:
if not await self.profile_handler.check_avatar_size_and_mime_type(
content["avatar_url"],
):
raise SynapseError(403, "This avatar is not allowed", Codes.FORBIDDEN)

# The event content should *not* include the authorising user as
# it won't be properly signed. Strip it out since it might come
# back from a client updating a display name / avatar.
Expand Down
94 changes: 92 additions & 2 deletions tests/handlers/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -46,7 +47,7 @@ def register_query_handler(query_type, handler):
)
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")
Expand Down Expand Up @@ -248,3 +249,92 @@ def test_set_my_avatar_if_disabled(self):
),
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.
res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/unknown_file")
)
self.assertTrue(res)

@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.
"""
res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/unknown_file")
)
self.assertFalse(res)

@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},
}
)

res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/small")
)
self.assertTrue(res)

res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/big")
)
self.assertFalse(res)

@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"},
}
)

res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/good")
)
self.assertTrue(res)

res = self.get_success(
self.handler.check_avatar_size_and_mime_type("mxc://test/bad")
)
self.assertFalse(res)

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"),
)
)
Loading