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

Always allow the empty string as an avatar_url. #12261

Merged
merged 5 commits into from
Mar 25, 2022
Merged

Always allow the empty string as an avatar_url. #12261

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d6830b6
Test case for the issue at large.
Mar 21, 2022
94e8a5f
Targetted test case
Mar 21, 2022
ba5fd0f
Don't reject an empty string as an invalid avatar
Mar 21, 2022
b0c5365
Changelog
Mar 21, 2022
e130f44
Typo fix
Mar 25, 2022
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
6 changes: 6 additions & 0 deletions synapse/handlers/profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,18 @@ 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.

If the given `mxc` is empty, no checks are performed. (Users are always able to
unset their avatar.)

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 mxc == "":
Copy link
Member

Choose a reason for hiding this comment

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

I think that you might be able to get a non-string value into here via RoomMemberHandler.update_membership_locked -- does that seem correct or am I missing something?

Copy link
Contributor Author

@DMRobertson DMRobertson Mar 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The avatar_url must have a len:

synapse/synapse/handlers/room_member.py

Line 618 in 8533c8b

if len(content.get("avatar_url") or "") > MAX_AVATAR_URL_LEN:

And given that it comes from JSON, that means that avatar_url is either a dict, list or string. So yes, I agree that we could pass non-strings into this function.

But this isn't the first unverified body parameter. Frankly I'd just toss this one in the #8445 bucket.

Copy link
Member

Choose a reason for hiding this comment

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

Previoulsy a JSON null (or false) also would work fine, since it would reduce to the empty string, I think the code in check_avatar_size_and_mime_type will choke on it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh sorry, you're quite right (I didn't spot the or ""). So that's every JSON value other than integers and true then!

Copy link
Member

Choose a reason for hiding this comment

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

I think the proper solution might be to do avatar_url = content.get("avatar_url") or "" and then use the value directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sympathise, but I'm mainly interested in fixing #12257 here. The worst that happens today when going via the RoomMember path is an internal server error when a client gives us a garbage avatar_url. I'd rather see a systematic approach to validation rather than an ad-hoc point fix.

A little more context: when trying to deactivate+erase a user, we set their avatar_url to "" here:

synapse/synapse/handlers/deactivate_account.py

Lines 149 to 151 in 15382b1

await self._profile_handler.set_avatar_url(
user, requester, "", by_admin, deactivation=True
)

And that ends up failing if avatar restrictions are turned on.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should handle blank strings properly, but it seems silly to not also handle the other data while we're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not following you here---what other data you have in mind? Would you prefer check_avatar_size_and_mime_type to take mxc: Optional[str] instead of mxc: str? Or maybe mxc: Any?

Copy link
Member

@clokep clokep Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm suggesting to add additional handling to raise errors if the value is not a string and not None, similar to what we do at

synapse/synapse/handlers/profile.py

Lines 296 to 304 in 300ed0b

if not isinstance(new_avatar_url, str):
raise SynapseError(
400, "'avatar_url' must be a string", errcode=Codes.INVALID_PARAM
)
if len(new_avatar_url) > MAX_AVATAR_URL_LEN:
raise SynapseError(
400, "Avatar URL is too long (max %i)" % (MAX_AVATAR_URL_LEN,)
)

Although that case seems to be rejecting strings... /me sighs. I'm not sure what the behavior of this code is supposed to be (should it accept nulls or not). Maybe we should just accept this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the behavior of this code is supposed to be (should it accept nulls or not).

Nor I; if it were up to me we'd reject nulls here.

return True

if not self.max_avatar_size and not self.allowed_avatar_mimetypes:
return True

Expand Down
6 changes: 6 additions & 0 deletions tests/handlers/test_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,12 @@ def test_avatar_constraints_no_config(self) -> None:
)
self.assertTrue(res)

@unittest.override_config({"max_avatar_size": 50})
def test_avatar_constraints_allow_empty_avatar_url(self) -> None:
"""An empty avatar is always permitted."""
res = self.get_success(self.handler.check_avatar_size_and_mime_type(""))
self.assertTrue(res)

@unittest.override_config({"max_avatar_size": 50})
def test_avatar_constraints_missing(self) -> None:
"""Tests that an avatar isn't allowed if the file at the given MXC URI couldn't
Expand Down
19 changes: 19 additions & 0 deletions tests/rest/admin/test_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,25 @@ def test_deactivate_user_erase_true(self) -> None:

self._is_erased("@user:test", True)

@override_config({"max_avatar_size": 1234})
def test_deactivate_user_erase_true_avatar_nonnull_but_empty(self) -> None:
"""Check we can erase a user whose avatar is the empty string.

Reproduces #12257.
"""
# Patch `self.other_user` to have an empty string as their avatar.
self.get_success(self.store.set_profile_avatar_url("user", ""))

# Check we can still erase them.
channel = self.make_request(
"POST",
self.url,
access_token=self.admin_user_tok,
content={"erase": True},
)
self.assertEqual(HTTPStatus.OK, channel.code, msg=channel.json_body)
self._is_erased("@user:test", True)

def test_deactivate_user_erase_false(self) -> None:
"""
Test deactivating a user and set `erase` to `false`
Expand Down