fix(MSC3911): Handle profile avatars that are unknown gracefully (#148)

Related: famedly/product-management#3448

Be more selective about what avatar url's are allowed to be set to the
profile of a given user. Particularly around remote media, but any media
that is suddenly "missing" could have this error.

Enhance the pre-flight validation to ensure that the media exists, with
selective conditions during the transition away from unrestricted media
to either ignore the error or forbid the operation.

Gracefully handle updating membership events during a profile avatar
change propagation. Most of the potential errors that can be raised here
should now be blocked by the pre-flight validation when setting the
profile. Additionally, outgoing remote invites and room creator join
membership events conditionally drop the avatar url if the media does
not exist and legacy unrestricted media is disabled

Author: jason-famedly

synapse/handlers/profile.py

@@ -327,8 +327,22 @@ async def validate_avatar_url(self, avatar_url: str, requester: Requester) -> No
             media_info = await self.hs.get_datastores().main.get_cached_remote_media(
                 mxc_uri.server_name, mxc_uri.media_id
             )
+            # When our local user has attempted to set a profile avatar to a remote
+            # piece of media, but the local server has not actually seen it, only
+            # complain if unrestricted media is disabled, otherwise just allow it. Later
+            # when/if the new profile data is propagated to each room's membership
+            # event, it will either be copied/passed along/dropped depending on the
+            # above circumstances
             if not media_info:
-                # There is no data on this, not much can be done until it comes along
+                if self.disable_unrestricted_media:
+                    # The user should have done a COPY on this media previous to this
+                    # attempt to set
+                    raise SynapseError(
+                        HTTPStatus.NOT_FOUND,
+                        "Profile request to update avatar to remote media can not proceed, a /copy request should have happened first",
+                        errcode=Codes.NOT_FOUND,
+                    )
+                # For backwards compatible behavior, treat the media as unrestricted
                 return
 
         if self.disable_unrestricted_media and not media_info.restricted:

synapse/handlers/room_member.py

@@ -110,6 +110,9 @@ def __init__(self, hs: "HomeServer"):
         self.event_auth_handler = hs.get_event_auth_handler()
         self._worker_lock_handler = hs.get_worker_locks_handler()
         self.enable_restricted_media = hs.config.experimental.msc3911_enabled
+        self.allow_legacy_media = (
+            not hs.config.experimental.msc3911_unrestricted_media_upload_disabled
+        )
 
         self._membership_types_to_include_profile_data_in = {
             Membership.JOIN,
@@ -863,21 +866,83 @@ async def update_membership_locked(
                 logger.info("Failed to get profile information for %r: %s", target, e)
 
             if self.enable_restricted_media and not media_info_for_attachment:
-                # Other than membership
-                avatar_url = content.get("avatar_url")
+                # This code path should only be taken for memberships updating an
+                # avatar url
+                avatar_url = content.get(EventContentFields.MEMBERSHIP_AVATAR_URL)
                 if avatar_url:
                     # Something about the MediaRepository does not like being part of
                     # the initialization code of the RoomMemberHandler, so just import
                     # it on the spot instead.
                     media_repo = self.hs.get_media_repository()
 
-                    new_mxc_uri = await media_repo.copy_media(
-                        MXCUri.from_str(avatar_url), requester.user, 20_000
-                    )
-                    media_object = await media_repo.get_media_info(new_mxc_uri)
-                    assert isinstance(media_object, LocalMedia)
-                    media_info_for_attachment = {media_object}
-                    content[EventContentFields.MEMBERSHIP_AVATAR_URL] = str(new_mxc_uri)
+                    try:
+                        # Run a preflight that this media exists. The http replication
+                        # call should not be a thundering herd of guaranteed http fails.
+                        existing_mxc_uri = MXCUri.from_str(avatar_url)
+                        # The actual info is irrelevant at this stage, just ignore. This
+                        # will raise if the media does not exist
+                        _ = await media_repo.get_media_info(existing_mxc_uri)
+
+                        new_mxc_uri = await media_repo.copy_media(
+                            existing_mxc_uri, requester.user, 20_000
+                        )
+                    except SynapseError:
+                        # The only kind of media copying that should fail is from a
+                        # remote item that has not been seen and cached previously. This
+                        # should already be guarded from in the only circumstances we
+                        # could think of: setting a profile avatar.
+                        #
+                        # If legacy unrestricted media is allowed, this can still be
+                        # triggered. If this happens, ignore it. This allows the avatar
+                        # url to be faithfully set to the given url. All we can do is
+                        # hope that it is not restricted when it finally shows up.
+                        # Guard against other potentials escaping by raising if it
+                        # should occur when unrestricted media begins to be disallowed.
+                        if self.allow_legacy_media:
+                            logger.debug(
+                                "Ignoring media copy request; the media is unknown and "
+                                "will not be treated as restricted"
+                            )
+
+                        else:
+                            # Don't fail the new membership event just because the media
+                            # was not found. Specifically, for:
+                            # * Outgoing remote invites
+                            # * Joins in the context of a new room
+                            # Just remove it, and let the client deal with the lack of
+                            # hint
+                            if effective_membership_state == Membership.INVITE or (
+                                effective_membership_state == Membership.JOIN
+                                and new_room
+                            ):
+                                logger.warning(
+                                    "Unknown media (%s) can not be restricted to "
+                                    "membership event for '%s', dropping avatar from "
+                                    "event",
+                                    avatar_url,
+                                    target,
+                                )
+                                del content[EventContentFields.MEMBERSHIP_AVATAR_URL]
+
+                            else:
+                                # All other types of membership should raise. For
+                                # whatever reason, the media is missing so this should
+                                # not continue
+                                logger.warning(
+                                    "Unknown media (%s) can not be restricted to "
+                                    "membership event for '%s'",
+                                    avatar_url,
+                                    target,
+                                )
+                                raise
+
+                    else:
+                        media_object = await media_repo.get_media_info(new_mxc_uri)
+                        assert isinstance(media_object, LocalMedia)
+                        media_info_for_attachment = {media_object}
+                        content[EventContentFields.MEMBERSHIP_AVATAR_URL] = str(
+                            new_mxc_uri
+                        )
 
         # if this is a join with a 3pid signature, we may need to turn a 3pid
         # invite into a normal invite before we can handle the join.

synapse/media/media_repository.py

@@ -153,9 +153,13 @@ async def get_media_info(self, mxc_uri: MXCUri) -> Union[LocalMedia, RemoteMedia
                 mxc_uri.server_name, mxc_uri.media_id
             )
         if not media_info:
-            raise SynapseError(404, "Media not found", errcode="M_NOT_FOUND")
+            raise SynapseError(
+                HTTPStatus.NOT_FOUND, "Media not found", errcode=Codes.NOT_FOUND
+            )
         if media_info.quarantined_by:
-            raise SynapseError(404, "Media not found", errcode="M_NOT_FOUND")
+            raise SynapseError(
+                HTTPStatus.NOT_FOUND, "Media not found", errcode=Codes.NOT_FOUND
+            )
         return media_info
 
     async def create_or_update_content(

tests/rest/client/test_profile.py

@@ -1015,7 +1015,7 @@ def test_can_attach_media_to_profile_update(self) -> None:
         assert restrictions.event_id is None
         assert restrictions.profile_user_id == UserID.from_string(self.user)
 
-    def test_attaching_nonexistent_media_to_profile_fails(self) -> None:
+    def test_attaching_nonexistent_local_media_to_profile_fails(self) -> None:
         """
         Test that media that does not exist is not allowed to be attached to a user profile.
         """
@@ -1032,6 +1032,42 @@ def test_attaching_nonexistent_media_to_profile_fails(self) -> None:
         assert channel.json_body["errcode"] == Codes.INVALID_PARAM
         assert "does not exist" in channel.json_body["error"]
 
+    def test_attaching_unreachable_remote_media_to_profile_might_succeed(self) -> None:
+        """
+        Test that media that can not be retrieved can be attached to a user profile, if
+        legacy unrestricted media is allowed.
+        """
+        # Generate non-existing media.
+        nonexistent_mxc_uri = MXCUri.from_str("mxc://remote/fakeMediaId")
+        channel = self.make_request(
+            "PUT",
+            f"/_matrix/client/v3/profile/{self.user}/avatar_url",
+            access_token=self.tok,
+            content={"avatar_url": str(nonexistent_mxc_uri)},
+        )
+
+        assert channel.code == HTTPStatus.OK, channel.json_body
+
+    @override_config(
+        {"experimental_features": {"msc3911_unrestricted_media_upload_disabled": True}}
+    )
+    def test_attaching_unreachable_remote_media_to_profile_fails(self) -> None:
+        """
+        Test that media that can not be retrieved will fail to be attached to a user
+        profile when legacy unrestricted media is disabled.
+        """
+        # Generate non-existing media.
+        nonexistent_mxc_uri = MXCUri.from_str("mxc://remote/fakeMediaId_2")
+        channel = self.make_request(
+            "PUT",
+            f"/_matrix/client/v3/profile/{self.user}/avatar_url",
+            access_token=self.tok,
+            content={"avatar_url": str(nonexistent_mxc_uri)},
+        )
+
+        assert channel.code == HTTPStatus.NOT_FOUND, channel.json_body
+        assert channel.json_body["errcode"] == Codes.NOT_FOUND
+
     def test_attaching_unrestricted_media_to_profile(self) -> None:
         """
         Test that attaching unrestricted media to user profile also works

tests/rest/client/test_rooms.py

@@ -5269,6 +5269,53 @@ def test_create_room_fails_with_malformed_room_avatar_url(self) -> None:
         room_id = self.create_room_with_avatar(avatar_mxc="junk", expected_code=400)
         assert room_id is None
 
+    @override_config(
+        {"experimental_features": {"msc3911_unrestricted_media_upload_disabled": True}}
+    )
+    def test_create_room_with_missing_profile_avatar_media_succeeds(self) -> None:
+        """
+        Test that a profile avatar that should automatically be included in a room
+        creator's join event does not break the room when the actual media of the avatar
+        is missing.
+        """
+        # First inject a profile avatar url directly into the database. The handler
+        # functions for such can not be used as they do validation, and it would fail as
+        # the media does not actually exist.
+        avatar_mxc_uri = MXCUri.from_str("mxc://fake-domain/whatever")
+        # Make sure to add the restrictions too
+        self.get_success_or_raise(
+            self.hs.get_datastores().main.set_media_restricted_to_user_profile(
+                avatar_mxc_uri.server_name,
+                avatar_mxc_uri.media_id,
+                self.user,
+            )
+        )
+        self.get_success_or_raise(
+            self.store.set_profile_avatar_url(
+                UserID.from_string(self.user), str(avatar_mxc_uri)
+            )
+        )
+
+        # try and create room. This should succeed, but the avatar will have been
+        # stripped from the join event of the creator
+        room_id = self.helper.create_room_as(
+            self.user,
+            tok=self.tok,
+        )
+        assert room_id is not None
+        # Make sure the avatar is not on the event
+        membership_as_set = self.get_success_or_raise(
+            self.store.get_membership_event_ids_for_user(self.user, room_id)
+        )
+        join_event_id = membership_as_set.pop()
+        join_event = self.get_success_or_raise(self.store.get_event(join_event_id))
+        assert join_event.content["membership"] == Membership.JOIN
+        assert "avatar_url" not in join_event.content
+
+        # The display name would have been added, see if that is still there
+        assert "displayname" in join_event.content
+        assert join_event.content["displayname"] == "david"
+
 
 class RoomMemberEventMediaAttachmentTestCase(unittest.HomeserverTestCase):
     servlets = [
@@ -5489,8 +5536,6 @@ def test_invite_with_media_get_copied_and_attached_to_event(self) -> None:
         assert channel.code == 200, channel.result["body"]
 
         # Get the member event of the invite just occurred.
-        # creator_event_ids = self.get_success(self.store.get_membership_event_ids_for_user(self.user, room_id))
-        # assert len(creator_event_ids) == 1
         invitee_event_ids = self.get_success(
             self.store.get_membership_event_ids_for_user(self.other_user, room_id)
         )
@@ -5502,7 +5547,7 @@ def test_invite_with_media_get_copied_and_attached_to_event(self) -> None:
         assert event.type == EventTypes.Member
 
         # Verify that this event a different mxc
-        assert event.content.get(EventContentFields.MEMBERSHIP_DISPLAYNAME) != str(
+        assert event.content.get(EventContentFields.MEMBERSHIP_AVATAR_URL) != str(
             invitee_avatar_mxc_uri
         )