Always allow the empty string as an avatar_url.

[DMRobertson]

Fixes #12257; see also #11846.

[DMRobertson]

I've tested this as follows:

• checkout 94e8a5f (#12261).
• run the two new tests. They fail.
• checkout ba5fd0f (#12261).
• Re run those tests. They pass.

One test is probably sufficient here. I'll leave the reviewer to decide which they prefer.

[clokep]

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?

[DMRobertson]

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.

[clokep]

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.

[DMRobertson]

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

[clokep]

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

[DMRobertson]

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.

[clokep]

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

[DMRobertson]

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?

[clokep]

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.

[DMRobertson]

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.

[clokep]

I think having both tests is fine, if you want to remove one I think removing the admin one is the way to go.