From 1e1657e6bc3e3f99f6fbb275d4b29c5cf94eb072 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Thu, 19 Aug 2021 10:21:36 +0100 Subject: [PATCH 1/8] Apply limit to per-room display names when setting them Attempting to set a display name to something longer than the limit just sets it to the default profile name (which is the same as what happens when shadow-banned users try to change name) --- synapse/handlers/room_member.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index ba131962185f..3ca38cf998be 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -49,6 +49,7 @@ from synapse.util.distributor import user_left_room from ._base import BaseHandler +from .profile import MAX_DISPLAYNAME_LEN if TYPE_CHECKING: from synapse.server import HomeServer @@ -548,8 +549,10 @@ async def update_membership_locked( ) if ( - not self.allow_per_room_profiles and not is_requester_server_notices_user - ) or requester.shadow_banned: + (not self.allow_per_room_profiles and not is_requester_server_notices_user) + or requester.shadow_banned + or len(content.get("displayname", "")) > MAX_DISPLAYNAME_LEN + ): # Strip profile data, knowing that new profile data will be added to the # event's content in event_creation_handler.create_event() using the target's # global profile. From f6bb4a1f8306f80bbc599ba2c13fb5eb5c8f2d9a Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Thu, 19 Aug 2021 10:29:03 +0100 Subject: [PATCH 2/8] Added changelog --- changelog.d/10654.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10654.bugfix diff --git a/changelog.d/10654.bugfix b/changelog.d/10654.bugfix new file mode 100644 index 000000000000..c1b22896cd83 --- /dev/null +++ b/changelog.d/10654.bugfix @@ -0,0 +1 @@ +Apply display name length limit to per-room display names. \ No newline at end of file From e12d9ec4d33971944df2bd6f00c095bb92a07e8e Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Thu, 19 Aug 2021 11:13:04 +0100 Subject: [PATCH 3/8] Added type hint for member_linearizer (fixed mypy error) Not sure why this was neccessary! Without it it produces an error for line 811 of `synapse/handlers/room.py` --- synapse/handlers/room_member.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 3ca38cf998be..64cb085b5461 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -80,7 +80,7 @@ def __init__(self, hs: "HomeServer"): self.account_data_handler = hs.get_account_data_handler() self.event_auth_handler = hs.get_event_auth_handler() - self.member_linearizer = Linearizer(name="member") + self.member_linearizer: Linearizer = Linearizer(name="member") self.clock = hs.get_clock() self.spam_checker = hs.get_spam_checker() @@ -558,6 +558,8 @@ async def update_membership_locked( # global profile. content.pop("displayname", None) content.pop("avatar_url", None) + # content may now be emtpy + content_specified = len(content) == 0 effective_membership_state = action if action in ["kick", "unban"]: From f59a0291b632f79401f0417f9d4cde737628abcf Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 20 Aug 2021 14:09:57 +0100 Subject: [PATCH 4/8] Applied suggestions from code review Full import paths Check avatar url length as well don't wipe profile if just some things too long don't correct contents_specified --- synapse/handlers/room_member.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 64cb085b5461..93d51a6f9670 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -36,6 +36,7 @@ from synapse.event_auth import get_named_level, get_power_level_event from synapse.events import EventBase from synapse.events.snapshot import EventContext +from synapse.handlers.profile import MAX_AVATAR_URL_LEN, MAX_DISPLAYNAME_LEN from synapse.types import ( JsonDict, Requester, @@ -49,7 +50,6 @@ from synapse.util.distributor import user_left_room from ._base import BaseHandler -from .profile import MAX_DISPLAYNAME_LEN if TYPE_CHECKING: from synapse.server import HomeServer @@ -549,17 +549,19 @@ async def update_membership_locked( ) if ( - (not self.allow_per_room_profiles and not is_requester_server_notices_user) - or requester.shadow_banned - or len(content.get("displayname", "")) > MAX_DISPLAYNAME_LEN - ): + not self.allow_per_room_profiles and not is_requester_server_notices_user + ) or requester.shadow_banned: # Strip profile data, knowing that new profile data will be added to the # event's content in event_creation_handler.create_event() using the target's # global profile. content.pop("displayname", None) content.pop("avatar_url", None) - # content may now be emtpy - content_specified = len(content) == 0 + + if len(content.get("displayname", "")) > MAX_DISPLAYNAME_LEN: + content.pop("displayname", None) + + if len(content.get("avatar_url", None)) > MAX_AVATAR_URL_LEN: + content.pop("avatar_url", None) effective_membership_state = action if action in ["kick", "unban"]: From cd94d1028baa8b54a53aa0afa62eeca23ae95cfe Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 20 Aug 2021 14:21:34 +0100 Subject: [PATCH 5/8] Wrong default value - now empty string --- synapse/handlers/room_member.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 93d51a6f9670..e5310d6c1fec 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -560,7 +560,7 @@ async def update_membership_locked( if len(content.get("displayname", "")) > MAX_DISPLAYNAME_LEN: content.pop("displayname", None) - if len(content.get("avatar_url", None)) > MAX_AVATAR_URL_LEN: + if len(content.get("avatar_url", "")) > MAX_AVATAR_URL_LEN: content.pop("avatar_url", None) effective_membership_state = action From f326c3c647f177878b5cfdf5a949a70d1954bd03 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 20 Aug 2021 14:56:15 +0100 Subject: [PATCH 6/8] Throw synapse errors instead of stripping --- synapse/handlers/room_member.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index e5310d6c1fec..5918ae4a6a05 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -557,11 +557,11 @@ async def update_membership_locked( content.pop("displayname", None) content.pop("avatar_url", None) - if len(content.get("displayname", "")) > MAX_DISPLAYNAME_LEN: - content.pop("displayname", None) + if len(content.get("displayname") or "") > MAX_DISPLAYNAME_LEN: + raise SynapseError(403, "Display name too long") - if len(content.get("avatar_url", "")) > MAX_AVATAR_URL_LEN: - content.pop("avatar_url", None) + if len(content.get("avatar_url") or "") > MAX_AVATAR_URL_LEN: + raise SynapseError(403, "Avatar URL too long") effective_membership_state = action if action in ["kick", "unban"]: From e6fd3a7cbd07942a738721690367cbdf8bad9910 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 20 Aug 2021 14:59:09 +0100 Subject: [PATCH 7/8] Use same errors as when exceed limit on NON per-room displayname/url --- synapse/handlers/room_member.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 5918ae4a6a05..1b977dbe2e48 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -558,10 +558,10 @@ async def update_membership_locked( content.pop("avatar_url", None) if len(content.get("displayname") or "") > MAX_DISPLAYNAME_LEN: - raise SynapseError(403, "Display name too long") + raise SynapseError(403, "Displayname is too long (max 256)") if len(content.get("avatar_url") or "") > MAX_AVATAR_URL_LEN: - raise SynapseError(403, "Avatar URL too long") + raise SynapseError(403, "Avatar URL is too long (max 1000)") effective_membership_state = action if action in ["kick", "unban"]: From 7d93b1956b642c3338a6fb72d80c58db3ec442b8 Mon Sep 17 00:00:00 2001 From: Azrenbeth <7782548+Azrenbeth@users.noreply.github.com> Date: Fri, 20 Aug 2021 17:37:41 +0100 Subject: [PATCH 8/8] Applied suggestions from code review --- changelog.d/10654.bugfix | 2 +- synapse/handlers/room_member.py | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/changelog.d/10654.bugfix b/changelog.d/10654.bugfix index c1b22896cd83..b0bd78453fab 100644 --- a/changelog.d/10654.bugfix +++ b/changelog.d/10654.bugfix @@ -1 +1 @@ -Apply display name length limit to per-room display names. \ No newline at end of file +Enforce the maximum length for per-room display names and avatar URLs. \ No newline at end of file diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 1b977dbe2e48..401b84aad1eb 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -558,10 +558,18 @@ async def update_membership_locked( content.pop("avatar_url", None) if len(content.get("displayname") or "") > MAX_DISPLAYNAME_LEN: - raise SynapseError(403, "Displayname is too long (max 256)") + raise SynapseError( + 400, + f"Displayname is too long (max {MAX_DISPLAYNAME_LEN})", + errcode=Codes.BAD_JSON, + ) if len(content.get("avatar_url") or "") > MAX_AVATAR_URL_LEN: - raise SynapseError(403, "Avatar URL is too long (max 1000)") + raise SynapseError( + 400, + f"Avatar URL is too long (max {MAX_AVATAR_URL_LEN})", + errcode=Codes.BAD_JSON, + ) effective_membership_state = action if action in ["kick", "unban"]: