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
Commit date: 2021-09-15 11:32:10 +0800
Commit message: Partially revert "PERF: Improve query performance all inbox private messages. (#14304)" (#14344)

This partially reverts commit ddb458343dc39a7a8c99467dcd809b444514fe2c.

Seeing performance degrade on larger sites so back to drawing board on
this one. Instead of the DISTINCT LEFT JOIN, we switch back to
IN(subquery).
Diff: diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 4de9618a941..b23245cc1be 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -971,11 +971,11 @@ class TopicsController < ApplicationController
       topic_scope = topic_query.filter_private_message_new(current_user, filter)
     end
 
     topic_ids = TopicsBulkAction.new(
       current_user,
-      topic_scope.distinct(false).pluck(:id),
+      topic_scope.pluck(:id),
       type: "dismiss_topics"
     ).perform!
 
     render json: success_json.merge(topic_ids: topic_ids)
   end
@@ -1243,15 +1243,11 @@ 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
-
-      topic_query
-        .filter_private_messages_unread(current_user, filter)
-        .distinct(false)
-        .pluck(:id)
+      topics = topic_query.filter_private_messages_unread(current_user, filter)
     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]
@@ -1266,13 +1262,13 @@ class TopicsController < ApplicationController
       end
 
       if params[:tag_name].present?
         topics = topics.joins(:tags).where("tags.name": params[:tag_name])
       end
-
-      topics.pluck(:id)
     end
+
+    topics.pluck(:id)
   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 1a0c770a730..c056d5946bf 100644
--- a/lib/topic_query/private_message_lists.rb
+++ b/lib/topic_query/private_message_lists.rb
@@ -143,22 +143,29 @@ class TopicQuery
       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
         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
+        result =
+        if group_ids.present?
+          result.where(<<~SQL)
+            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 (#{group_ids.join(",")})
+            )
+          SQL
+        else
+          result.joins(<<~SQL)
+          INNER JOIN topic_allowed_users tau
+            ON tau.topic_id = topics.id
+            AND tau.user_id = #{user.id.to_i}
+          SQL
+        end
       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
@@ -236,27 +243,34 @@ class TopicQuery
       # 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
-        #{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
+      if group_ids.present?
+        list = list.joins(<<~SQL)
+          LEFT JOIN group_archived_messages gm
+            ON gm.topic_id = topics.id
+            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
 
-      list =
         if archived
           list.where("um.user_id IS NOT NULL OR gm.topic_id IS NOT NULL")
         else
           list.where("um.user_id IS NULL AND gm.topic_id IS NULL")
         end
+      else
+        list = list.joins(<<~SQL)
+          LEFT JOIN user_archived_messages um
+          ON um.user_id = #{user.id.to_i}
+          AND um.topic_id = topics.id
+        SQL
 
-      list
+        list.where("um.user_id IS #{archived ? "NOT NULL" : "NULL"}")
+      end
     end
 
     def not_archived(list, user)
       list.joins("LEFT JOIN user_archived_messages um
                          ON um.user_id = #{user.id.to_i} AND um.topic_id = topics.id")

Vulnerable code:       topic_scope.distinct(false).pluck(:id),

      topic_query
        .filter_private_messages_unread(current_user, filter)
        .distinct(false)
        .pluck(:id)

      topics.pluck(:id)
        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
      list = list.joins(<<~SQL)
      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
      list =
      list
Fixed code:       topic_scope.pluck(:id),
      topics = topic_query.filter_private_messages_unread(current_user, filter)

    topics.pluck(:id)
        result =
        if group_ids.present?
          result.where(<<~SQL)
            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 (#{group_ids.join(",")})
            )
          SQL
        else
          result.joins(<<~SQL)
          INNER JOIN topic_allowed_users tau
            ON tau.topic_id = topics.id
            AND tau.user_id = #{user.id.to_i}
          SQL
        end
      if group_ids.present?
        list = list.joins(<<~SQL)
          LEFT JOIN group_archived_messages gm
            ON gm.topic_id = topics.id
            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
      else
        list = list.joins(<<~SQL)
          LEFT JOIN user_archived_messages um
          ON um.user_id = #{user.id.to_i}
          AND um.topic_id = topics.id
        SQL
        list.where("um.user_id IS #{archived ? "NOT NULL" : "NULL"}")
      end