Improper authorisation of /members discloses room membership to non-members

@@ -332,6 +332,13 @@ async def get_rooms_in_group(
             requester_user_id, group_id
         )
 
+        # Note! room_results["is_public"] is about whether the room is considered
+        # public from the group's point of view. (i.e. whether non-group members
+        # should be able to see the room is in the group).
+        # This is not the same as whether the room itself is public (in the sense
+        # of being visible in the room directory).
+        # As such, room_results["is_public"] itself is not sufficient to determine
+        # whether any given user is permitted to see the room's metadata.
         room_results = await self.store.get_rooms_in_group(
             group_id, include_private=is_user_in_group
         )
@@ -341,8 +348,15 @@ async def get_rooms_in_group(
             room_id = room_result["room_id"]
 
             joined_users = await self.store.get_users_in_room(room_id)
+
+            # check the user is actually allowed to see the room before showing it to them
+            allow_private = requester_user_id in joined_users
+
             entry = await self.room_list_handler.generate_room_entry(
-                room_id, len(joined_users), with_alias=False, allow_private=True
+                room_id,
+                len(joined_users),
+                with_alias=False,
+                allow_private=allow_private,
             )
 
             if not entry:
@@ -354,7 +368,7 @@ async def get_rooms_in_group(
 
         chunk.sort(key=lambda e: -e["num_joined_members"])
 
-        return {"chunk": chunk, "total_room_count_estimate": len(room_results)}
+        return {"chunk": chunk, "total_room_count_estimate": len(chunk)}
 
 
 class GroupsServerHandler(GroupsServerWorkerHandler):

@@ -0,0 +1,43 @@
+from synapse.rest.client.v1 import room
+from synapse.rest.client.v2_alpha import groups
+
+from tests import unittest
+from tests.unittest import override_config
+
+
+class GroupsTestCase(unittest.HomeserverTestCase):
+    user_id = "@alice:test"
+    room_creator_user_id = "@bob:test"
+
+    servlets = [room.register_servlets, groups.register_servlets]
+
+    @override_config({"enable_group_creation": True})
+    def test_rooms_limited_by_visibility(self):
+        group_id = "+spqr:test"
+
+        # Alice creates a group
+        channel = self.make_request("POST", "/create_group", {"localpart": "spqr"})
+        self.assertEquals(channel.code, 200, msg=channel.text_body)
+        self.assertEquals(channel.json_body, {"group_id": group_id})
+
+        # Bob creates a private room
+        room_id = self.helper.create_room_as(self.room_creator_user_id, is_public=False)
+        self.helper.auth_user_id = self.room_creator_user_id
+        self.helper.send_state(
+            room_id, "m.room.name", {"name": "bob's secret room"}, tok=None
+        )
+        self.helper.auth_user_id = self.user_id
+
+        # Alice adds the room to her group.
+        channel = self.make_request(
+            "PUT", f"/groups/{group_id}/admin/rooms/{room_id}", {}
+        )
+        self.assertEquals(channel.code, 200, msg=channel.text_body)
+        self.assertEquals(channel.json_body, {})
+
+        # Alice now tries to retrieve the room list of the space.
+        channel = self.make_request("GET", f"/groups/{group_id}/rooms")
+        self.assertEquals(channel.code, 200, msg=channel.text_body)
+        self.assertEquals(
+            channel.json_body, {"chunk": [], "total_room_count_estimate": 0}
+        )