SECURITY: Don't allow moderators to list PMs of all groups.

* Also return 404 when a user is trying to list PMs of a group that
cannot be accessed by the user.

app/controllers/list_controller.rb

@@ -143,22 +143,37 @@ class ListController < ApplicationController
   end
 
   def self.generate_message_route(action)
-    define_method("#{action}") do
+    case action
-      if action == :private_messages_tag && !guardian.can_tag_pms?
+    when :private_messages_tag
-        raise Discourse::NotFound
+      define_method("#{action}") do
+        raise Discourse::NotFound if !guardian.can_tag_pms?
+        message_route(action)
       end
+    when :private_messages_group, :private_messages_group_archive
+      define_method("#{action}") do
+        group = Group.find_by(name: params[:group_name])
+        raise Discourse::NotFound unless guardian.can_see_group_messages?(group)
 
-      list_opts = build_topic_list_options
+        message_route(action)
-      target_user = fetch_user_from_params({ include_inactive: current_user.try(:staff?) }, [:user_stat, :user_option])
+      end
-      guardian.ensure_can_see_private_messages!(target_user.id)
+    else
-      list = generate_list_for(action.to_s, target_user, list_opts)
+      define_method("#{action}") do
-      url_prefix = "topics"
+        message_route(action)
-      list.more_topics_url = construct_url_with(:next, list_opts, url_prefix)
+      end
-      list.prev_topics_url = construct_url_with(:prev, list_opts, url_prefix)
-      respond_with_list(list)
     end
   end
 
+  def message_route(action)
+    target_user = fetch_user_from_params({ include_inactive: current_user.try(:staff?) }, [:user_stat, :user_option])
+    guardian.ensure_can_see_private_messages!(target_user.id)
+    list_opts = build_topic_list_options
+    list = generate_list_for(action.to_s, target_user, list_opts)
+    url_prefix = "topics"
+    list.more_topics_url = construct_url_with(:next, list_opts, url_prefix)
+    list.prev_topics_url = construct_url_with(:prev, list_opts, url_prefix)
+    respond_with_list(list)
+  end
+
   %i{
     private_messages
     private_messages_sent

lib/topic_query.rb

@@ -532,22 +532,15 @@ class TopicQuery
     result = Topic.includes(:tags)
 
     if type == :group
-      result = result.includes(:allowed_users)
+      result = result
-      result = result.where("
+        .includes(:allowed_users)
-        topics.id IN (
+        .joins("INNER JOIN topic_allowed_groups tag ON tag.topic_id = topics.id AND tag.group_id IN (SELECT id FROM groups WHERE name ilike '#{sanitize_sql_array([@options[:group_name]])}')")
-          SELECT topic_id FROM topic_allowed_groups
+
-          WHERE (
+      unless user.admin?
-            group_id IN (
+        result = result.joins("INNER JOIN group_users gu ON gu.group_id = tag.group_id AND gu.user_id = #{user.id.to_i}")
-              SELECT group_id
+      end
-              FROM group_users
+
-              WHERE user_id = #{user.id.to_i}
+      result
-              OR #{user.staff?}
-            )
-          )
-          AND group_id IN (SELECT id FROM groups WHERE name ilike ?)
-        )",
-        @options[:group_name]
-      )
     elsif type == :user
       result = result.includes(:allowed_users)
       result = result.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = #{user.id.to_i})")

spec/components/topic_query_spec.rb

@@ -1097,7 +1097,15 @@ describe TopicQuery do
       expect(topics).to contain_exactly(group_message)
     end
 
-    it 'should return the right list for a user not part of the group' do
+    it "should not allow a moderator not part of the group to view the group's messages" do
+      topics = TopicQuery.new(nil, group_name: group.name)
+        .list_private_messages_group(Fabricate(:moderator))
+        .topics
+
+      expect(topics).to eq([])
+    end
+
+    it "should not allow a user not part of the group to view the group's messages" do
       topics = TopicQuery.new(nil, group_name: group.name)
         .list_private_messages_group(Fabricate(:user))
         .topics

spec/requests/list_controller_spec.rb

@@ -183,8 +183,17 @@ RSpec.describe ListController do
     describe 'with unicode_usernames' do
       before { SiteSetting.unicode_usernames = false }
 
+      it 'should return the right response when user does not belong to group' do
+        Fabricate(:private_message_topic, allowed_groups: [group])
+
+        group.remove(user)
+
+        get "/topics/private-messages-group/#{user.username}/#{group.name}.json"
+
+        expect(response.status).to eq(404)
+      end
+
       it 'should return the right response' do
-        group.add(user)
         topic = Fabricate(:private_message_topic, allowed_groups: [group])
         get "/topics/private-messages-group/#{user.username}/#{group.name}.json"