CVE-2021-41082: Incorrect Authorization (CWE-863)
value: Discourse is a platform for community discussion. In affected versions any private message that includes a group had its title and participating user exposed to users that do not have access to the private messages. However, access control for the private messages was not compromised as users were not able to view the posts in the leaked private message despite seeing it in their inbox. The problematic commit was reverted around 32 minutes after it was made. Users are encouraged to upgrade to the latest commit if they are running Discourse against the `tests-passed` branch.
CWE description: The product performs an authorization check when an actor attempts to access a resource or perform an action, but it does not correctly perform the check. This allows attackers to bypass intended access restrictions.
CVSS v3 base score: 7.5
Published: 2021-09-20T21:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/topics_controller.rb, lib/topic_query/private_message_lists.rb, spec/lib/topic_query/private_message_lists_spec.rb
Commit date: 2021-09-15 10:29:42 +0800
Commit message: PERF: Improve query performance all inbox private messages. (#14304)

First reported in https://meta.discourse.org/t/-/202482/19

There are two optimizations being applied here:

1. Fetch a user's group ids in a seperate query instead of including it
   as a sub-query. When I tried a subquery, the query plan becomes very
inefficient.

1. Join against the `topic_allowed_users` and `topic_allowed_groups`
   table instead of doing an IN against a subquery where we UNION the
`topic_id`s from the two tables. From my profiling, this enables PG to
do a backwards index scan on the `index_topics_on_timestamps_private`
index.

This commit fixes a bug where listing all messages was incorrectly
excluding topics if a topic has been archived by a group even if the
user did not belong to the group.

This commit also fixes another bug where dismissing private messages
selectively was subjected to the default limit of 30.
Diff: diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 710632bae0b..4de9618a941 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -950,11 +950,11 @@ class TopicsController < ApplicationController
     changed_topic_ids = operator.perform!
     render_json_dump topic_ids: changed_topic_ids
   end
 
   def private_message_reset_new
-    topic_query = TopicQuery.new(current_user)
+    topic_query = TopicQuery.new(current_user, limit: false)
 
     if params[:topic_ids].present?
       unless Array === params[:topic_ids]
         raise Discourse::InvalidParameters.new(
           "Expecting topic_ids to contain a list of topic ids"
@@ -966,17 +966,16 @@ class TopicsController < ApplicationController
         .where("topics.id IN (?)", params[:topic_ids].map(&:to_i))
     else
       params.require(:inbox)
       inbox = params[:inbox].to_s
       filter = private_message_filter(topic_query, inbox)
-      topic_query.options[:limit] = false
       topic_scope = topic_query.filter_private_message_new(current_user, filter)
     end
 
     topic_ids = TopicsBulkAction.new(
       current_user,
-      topic_scope.pluck(:id),
+      topic_scope.distinct(false).pluck(:id),
       type: "dismiss_topics"
     ).perform!
 
     render json: success_json.merge(topic_ids: topic_ids)
   end
@@ -1244,11 +1243,15 @@ class TopicsController < ApplicationController
     topic_query = TopicQuery.new(current_user)
 
     if inbox = params[:private_message_inbox]
       filter = private_message_filter(topic_query, inbox)
       topic_query.options[:limit] = false
-      topics = topic_query.filter_private_messages_unread(current_user, filter)
+
+      topic_query
+        .filter_private_messages_unread(current_user, filter)
+        .distinct(false)
+        .pluck(:id)
     else
       topics = TopicQuery.unread_filter(topic_query.joined_topic_user, staff: guardian.is_staff?).listable_topics
       topics = TopicQuery.tracked_filter(topics, current_user.id) if params[:tracked].to_s == "true"
 
       if params[:category_id]
@@ -1263,13 +1266,13 @@ class TopicsController < ApplicationController
       end
 
       if params[:tag_name].present?
         topics = topics.joins(:tags).where("tags.name": params[:tag_name])
       end
-    end
 
-    topics.pluck(:id)
+      topics.pluck(:id)
+    end
   end
 
   def private_message_filter(topic_query, inbox)
     case inbox
     when "group"
diff --git a/lib/topic_query/private_message_lists.rb b/lib/topic_query/private_message_lists.rb
index 9aaadbabbe6..1a0c770a730 100644
--- a/lib/topic_query/private_message_lists.rb
+++ b/lib/topic_query/private_message_lists.rb
@@ -141,20 +141,24 @@ class TopicQuery
           result = result.joins("INNER JOIN group_users gu ON gu.group_id = tag.group_id AND gu.user_id = #{user.id.to_i}")
         end
       elsif type == :user
         result = result.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = #{user.id.to_i})")
       elsif type == :all
-        result = result.where("topics.id IN (
-              SELECT topic_id
-              FROM topic_allowed_users
-              WHERE user_id = #{user.id.to_i}
-              UNION ALL
-              SELECT topic_id FROM topic_allowed_groups
-              WHERE group_id IN (
-                SELECT group_id FROM group_users WHERE user_id = #{user.id.to_i}
-              )
-      )")
+        group_ids = group_with_messages_ids(user)
+
+        result = result.joins(<<~SQL)
+        LEFT JOIN topic_allowed_users tau
+          ON tau.topic_id = topics.id
+          AND tau.user_id = #{user.id.to_i}
+        LEFT JOIN topic_allowed_groups tag
+          ON tag.topic_id = topics.id
+          #{group_ids.present? ? "AND tag.group_id IN (#{group_ids.join(",")})" : ""}
+        SQL
+
+        result = result
+          .where("tag.topic_id IS NOT NULL OR tau.topic_id IS NOT NULL")
+          .distinct
       end
 
       result = result.joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})")
         .order("topics.bumped_at DESC")
         .private_messages
@@ -227,12 +231,19 @@ class TopicQuery
         .joins("LEFT OUTER JOIN topic_groups tg ON topics.id = tg.topic_id AND tg.group_id = #{group_id}")
         .select(*selected_values)
     end
 
     def filter_archived(list, user, archived: true)
+      # Executing an extra query instead of a sub-query because it is more
+      # efficient for the PG planner. Caution should be used when changing the
+      # query here as it can easily lead to an inefficient query.
+      group_ids = group_with_messages_ids(user)
+
       list = list.joins(<<~SQL)
-      LEFT JOIN group_archived_messages gm ON gm.topic_id = topics.id
+      LEFT JOIN group_archived_messages gm
+        ON gm.topic_id = topics.id
+        #{group_ids.present? ? "AND gm.group_id IN (#{group_ids.join(",")})" : ""}
       LEFT JOIN user_archived_messages um
         ON um.user_id = #{user.id.to_i}
         AND um.topic_id = topics.id
       SQL
 
@@ -262,7 +273,17 @@ class TopicQuery
     end
 
     def user_first_unread_pm_at(user)
       UserStat.where(user: user).pluck_first(:first_unread_pm_at)
     end
+
+    def group_with_messages_ids(user)
+      @group_with_messages_ids ||= {}
+
+      if ids = @group_with_messages_ids[user.id]
+        return ids
+      end
+
+      @group_with_messages_ids[user.id] = user.groups.where(has_messages: true).pluck(:id)
+    end
   end
 end
diff --git a/spec/lib/topic_query/private_message_lists_spec.rb b/spec/lib/topic_query/private_message_lists_spec.rb
index 6c02589c158..681a4b887a7 100644
--- a/spec/lib/topic_query/private_message_lists_spec.rb
+++ b/spec/lib/topic_query/private_message_lists_spec.rb
@@ -41,11 +41,11 @@ describe TopicQuery::PrivateMessageLists do
 
       topics = TopicQuery.new(nil).list_private_messages_all(user).topics
 
       expect(topics).to eq([])
 
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all(user_2).topics
 
       expect(topics).to contain_exactly(private_message)
     end
@@ -73,22 +73,22 @@ describe TopicQuery::PrivateMessageLists do
     it 'does not include user or group archived messages' do
       create_post(user: user_2, topic: private_message)
       create_post(user: user_2, topic: group_message)
 
       UserArchivedMessage.archive!(user_2.id, private_message)
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all_sent(user_2).topics
 
       expect(topics).to eq([])
     end
   end
 
   describe '#list_private_messages_all_archive' do
     it 'returns a list of all private messages that has been archived' do
       UserArchivedMessage.archive!(user_2.id, private_message)
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all_archive(user_2).topics
 
       expect(topics).to contain_exactly(private_message, group_message)
     end

Vulnerable code:     topic_query = TopicQuery.new(current_user)
      topic_query.options[:limit] = false
      topic_scope.pluck(:id),
      topics = topic_query.filter_private_messages_unread(current_user, filter)
    end
    topics.pluck(:id)
        result = result.where("topics.id IN (
              SELECT topic_id
              FROM topic_allowed_users
              WHERE user_id = #{user.id.to_i}
              UNION ALL
              SELECT topic_id FROM topic_allowed_groups
              WHERE group_id IN (
                SELECT group_id FROM group_users WHERE user_id = #{user.id.to_i}
              )
      )")
      LEFT JOIN group_archived_messages gm ON gm.topic_id = topics.id
      GroupArchivedMessage.archive!(user_2.id, group_message)
      GroupArchivedMessage.archive!(user_2.id, group_message)
      GroupArchivedMessage.archive!(user_2.id, group_message)
Fixed code:     topic_query = TopicQuery.new(current_user, limit: false)
      topic_scope.distinct(false).pluck(:id),

      topic_query
        .filter_private_messages_unread(current_user, filter)
        .distinct(false)
        .pluck(:id)
      topics.pluck(:id)
    end
        group_ids = group_with_messages_ids(user)

        result = result.joins(<<~SQL)
        LEFT JOIN topic_allowed_users tau
          ON tau.topic_id = topics.id
          AND tau.user_id = #{user.id.to_i}
        LEFT JOIN topic_allowed_groups tag
          ON tag.topic_id = topics.id
          #{group_ids.present? ? "AND tag.group_id IN (#{group_ids.join(",")})" : ""}
        SQL

        result = result
          .where("tag.topic_id IS NOT NULL OR tau.topic_id IS NOT NULL")
          .distinct
      # Executing an extra query instead of a sub-query because it is more
      # efficient for the PG planner. Caution should be used when changing the
      # query here as it can easily lead to an inefficient query.
      group_ids = group_with_messages_ids(user)

      LEFT JOIN group_archived_messages gm
        ON gm.topic_id = topics.id
        #{group_ids.present? ? "AND gm.group_id IN (#{group_ids.join(",")})" : ""}

    def group_with_messages_ids(user)
      @group_with_messages_ids ||= {}

      if ids = @group_with_messages_ids[user.id]
        return ids
      end

      @group_with_messages_ids[user.id] = user.groups.where(has_messages: true).pluck(:id)
    end
      GroupArchivedMessage.archive!(group.id, group_message)
      GroupArchivedMessage.archive!(group.id, group_message)
      GroupArchivedMessage.archive!(group.id, group_message)