SECURITY: Prevent dismissal of topics that user can't see (#22086)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>

app/controllers/topics_controller.rb

@@ -1046,20 +1046,6 @@ class TopicsController < ApplicationController
 
   def reset_new
     topic_scope =
-      if params[:category_id].present?
-        category_ids = [params[:category_id]]
-        if params[:include_subcategories] == "true"
-          category_ids =
-            category_ids.concat(Category.where(parent_category_id: params[:category_id]).pluck(:id))
-        end
-
-        scope = Topic.where(category_id: category_ids)
-        scope = scope.joins(:tags).where(tags: { name: params[:tag_id] }) if params[:tag_id]
-        scope
-      elsif params[:tag_id].present?
-        Topic.joins(:tags).where(tags: { name: params[:tag_id] })
-      else
-        new_results =
       if current_user.new_new_view_enabled?
         if (params[:dismiss_topics] && params[:dismiss_posts])
           TopicQuery.new(current_user).new_and_unread_results(limit: false)
@@ -1073,11 +1059,33 @@ class TopicsController < ApplicationController
       else
         TopicQuery.new(current_user).new_results(limit: false)
       end
+    if tag_name = params[:tag_id]
+      tag_name = DiscourseTagging.visible_tags(guardian).where(name: tag_name).pluck(:name).first
+    end
+    topic_scope =
+      if params[:category_id].present?
+        category_ids = [params[:category_id].to_i]
+        if ActiveModel::Type::Boolean.new.cast(params[:include_subcategories])
+          category_ids =
+            category_ids.concat(Category.where(parent_category_id: params[:category_id]).pluck(:id))
+        end
+
+        category_ids &= guardian.allowed_category_ids
+        if category_ids.blank?
+          scope = topic_scope.none
+        else
+          scope = topic_scope.where(category_id: category_ids)
+          scope = scope.joins(:tags).where(tags: { name: tag_name }) if tag_name
+        end
+        scope
+      elsif tag_name.present?
+        topic_scope.joins(:tags).where(tags: { name: tag_name })
+      else
         if params[:tracked].to_s == "true"
-          TopicQuery.tracked_filter(new_results, current_user.id)
+          TopicQuery.tracked_filter(topic_scope, current_user.id)
         else
           current_user.user_stat.update_column(:new_since, Time.zone.now)
-          new_results
+          topic_scope
         end
       end
 
@@ -1086,7 +1094,7 @@ class TopicsController < ApplicationController
         raise Discourse::InvalidParameters.new("Expecting topic_ids to contain a list of topic ids")
       end
 
-      topic_ids = params[:topic_ids].map { |t| t.to_i }
+      topic_ids = params[:topic_ids].map(&:to_i)
       topic_scope = topic_scope.where(id: topic_ids)
     end
 
@@ -1097,7 +1105,6 @@ class TopicsController < ApplicationController
         TopicsBulkAction.new(current_user, topic_scope.pluck(:id), type: "dismiss_topics").perform!
       TopicTrackingState.publish_dismiss_new(current_user.id, topic_ids: dismissed_topic_ids)
     end
-
     if params[:dismiss_posts]
       if params[:untrack]
         dismissed_post_topic_ids =

lib/topics_bulk_action.rb

@@ -98,7 +98,7 @@ class TopicsBulkAction
   end
 
   def dismiss_topics
-    rows =
+    ids =
       Topic
         .where(id: @topic_ids)
         .joins(
@@ -108,9 +108,15 @@ class TopicsBulkAction
         .where("topic_users.last_read_post_number IS NULL")
         .order("topics.created_at DESC")
         .limit(SiteSetting.max_new_topics)
-        .map { |topic| { topic_id: topic.id, user_id: @user.id, created_at: Time.zone.now } }
-    DismissedTopicUser.insert_all(rows) if rows.present?
-    @changed_ids = rows.map { |row| row[:topic_id] }
+        .filter { |t| guardian.can_see?(t) }
+        .map(&:id)
+
+    if ids.present?
+      now = Time.zone.now
+      rows = ids.map { |id| { topic_id: id, user_id: @user.id, created_at: now } }
+      DismissedTopicUser.insert_all(rows)
+    end
+    @changed_ids = ids
   end
 
   def destroy_post_timing

spec/lib/topics_bulk_action_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe TopicsBulkAction do
     before { topic.destroy! }
 
     it "dismisses private messages" do
-      pm = Fabricate(:private_message_topic)
+      pm = Fabricate(:private_message_topic, recipient: user)
 
       TopicsBulkAction.new(user, [pm.id], type: "dismiss_topics").perform!
 
@@ -73,6 +73,29 @@ RSpec.describe TopicsBulkAction do
       expect(dismissed_topic_user.topic_id).to eq(topic2.id)
       expect(dismissed_topic_user.created_at).not_to be_nil
     end
+
+    it "doesn't dismiss topics the user can't see" do
+      group = Fabricate(:group)
+      private_category = Fabricate(:private_category, group: group)
+      topic2.update!(category_id: private_category.id)
+
+      expect do
+        TopicsBulkAction.new(user, [topic2.id, topic3.id], type: "dismiss_topics").perform!
+      end.to change { DismissedTopicUser.count }.by(1)
+
+      expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([topic3.id])
+
+      group.add(user)
+
+      expect do
+        TopicsBulkAction.new(user, [topic2.id, topic3.id], type: "dismiss_topics").perform!
+      end.to change { DismissedTopicUser.count }.by(1)
+
+      expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly(
+        topic2.id,
+        topic3.id,
+      )
+    end
   end
 
   describe "dismiss_posts" do

spec/requests/topics_controller_spec.rb

@@ -3975,6 +3975,113 @@ RSpec.describe TopicsController do
             [category_topic.id, subcategory_topic.id].sort,
           )
         end
+
+        context "when the category has private child categories" do
+          fab!(:category) { Fabricate(:category) }
+          fab!(:group) { Fabricate(:group) }
+          fab!(:private_child_category) do
+            Fabricate(:private_category, parent_category: category, group: group)
+          end
+          fab!(:public_child_category) { Fabricate(:category, parent_category: category) }
+          fab!(:topic_in_private_child_category) do
+            Fabricate(:topic, category: private_child_category)
+          end
+          fab!(:topic_in_public_child_category) do
+            Fabricate(:topic, category: public_child_category)
+          end
+
+          it "doesn't dismiss topics in private child categories that the user can't see" do
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json",
+                    params: {
+                      category_id: category.id,
+                      include_subcategories: true,
+                    }
+              end
+            expect(response.status).to eq(200)
+            expect(messages.size).to eq(1)
+            expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id))
+            expect(messages[0].user_ids).to eq([user.id])
+            expect(messages[0].data["message_type"]).to eq(
+              TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE,
+            )
+            expect(messages[0].data["payload"]["topic_ids"]).to eq(
+              [topic_in_public_child_category.id],
+            )
+            expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq(
+              [topic_in_public_child_category.id],
+            )
+          end
+
+          it "dismisses topics in private child categories that the user can see" do
+            group.add(user)
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json",
+                    params: {
+                      category_id: category.id,
+                      include_subcategories: true,
+                    }
+              end
+            expect(response.status).to eq(200)
+            expect(messages.size).to eq(1)
+            expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id))
+            expect(messages[0].user_ids).to eq([user.id])
+            expect(messages[0].data["message_type"]).to eq(
+              TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE,
+            )
+            expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly(
+              topic_in_public_child_category.id,
+              topic_in_private_child_category.id,
+            )
+            expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly(
+              topic_in_public_child_category.id,
+              topic_in_private_child_category.id,
+            )
+          end
+        end
+
+        context "when the category is private" do
+          fab!(:group) { Fabricate(:group) }
+          fab!(:private_category) { Fabricate(:private_category, group: group) }
+          fab!(:topic_in_private_category) { Fabricate(:topic, category: private_category) }
+
+          it "doesn't dismiss topics or publish topic IDs via MessageBus if the user can't access the category" do
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json", params: { category_id: private_category.id }
+              end
+            expect(response.status).to eq(200)
+            expect(messages.size).to eq(1)
+            expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id))
+            expect(messages[0].user_ids).to eq([user.id])
+            expect(messages[0].data["message_type"]).to eq(
+              TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE,
+            )
+            expect(messages[0].data["payload"]["topic_ids"]).to eq([])
+            expect(DismissedTopicUser.where(user_id: user.id).count).to eq(0)
+          end
+
+          it "dismisses topics and publishes the dismissed topic IDs if the user can access the category" do
+            group.add(user)
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json", params: { category_id: private_category.id }
+              end
+            expect(response.status).to eq(200)
+            expect(messages.size).to eq(1)
+            expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id))
+            expect(messages[0].user_ids).to eq([user.id])
+            expect(messages[0].data["message_type"]).to eq(
+              TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE,
+            )
+            expect(messages[0].data["payload"]["topic_ids"]).to eq([topic_in_private_category.id])
+            expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq(
+              [topic_in_private_category.id],
+            )
+          end
+        end
       end
 
       context "with tag" do
@@ -3986,6 +4093,54 @@ RSpec.describe TopicsController do
           put "/topics/reset-new.json?tag_id=#{tag.name}"
           expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([tag_topic.id])
         end
+
+        context "when the tag is restricted" do
+          fab!(:restricted_tag) { Fabricate(:tag, name: "restricted-tag") }
+          fab!(:topic_with_restricted_tag) { Fabricate(:topic, tags: [restricted_tag]) }
+          fab!(:group) { Fabricate(:group) }
+          fab!(:topic_without_tag) { Fabricate(:topic) }
+          fab!(:tag_group) do
+            Fabricate(
+              :tag_group,
+              name: "Restricted Tag Group",
+              tag_names: ["restricted-tag"],
+              permissions: [[group, TagGroupPermission.permission_types[:full]]],
+            )
+          end
+
+          it "respects the tag param and only dismisses topics tagged with this tag if the user can see it" do
+            group.add(user)
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json", params: { tag_id: restricted_tag.name }
+              end
+            expect(messages.size).to eq(1)
+            expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly(
+              topic_with_restricted_tag.id,
+            )
+            expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly(
+              topic_with_restricted_tag.id,
+            )
+          end
+
+          it "ignores the tag param and dismisses all topics if the user can't see the tag" do
+            messages =
+              MessageBus.track_publish do
+                put "/topics/reset-new.json", params: { tag_id: restricted_tag.name }
+              end
+            expect(messages.size).to eq(1)
+            expect(messages[0].data["payload"]["topic_ids"]).to contain_exactly(
+              topic_with_restricted_tag.id,
+              tag_topic.id,
+              topic_without_tag.id,
+            )
+            expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to contain_exactly(
+              topic_with_restricted_tag.id,
+              tag_topic.id,
+              topic_without_tag.id,
+            )
+          end
+        end
       end
 
       context "with tag and category" do
@@ -4034,6 +4189,24 @@ RSpec.describe TopicsController do
           expect(response.parsed_body["errors"]).to eq(nil)
         end
 
+        it "doesn't dismiss topics that the user can't see" do
+          private_category = Fabricate(:private_category, group: Fabricate(:group))
+          topic2.update!(category_id: private_category.id)
+
+          messages =
+            MessageBus.track_publish do
+              put "/topics/reset-new.json", params: { topic_ids: [topic2.id, topic3.id] }
+            end
+          expect(messages.size).to eq(1)
+          expect(messages[0].channel).to eq(TopicTrackingState.unread_channel_key(user.id))
+          expect(messages[0].user_ids).to eq([user.id])
+          expect(messages[0].data["message_type"]).to eq(
+            TopicTrackingState::DISMISS_NEW_MESSAGE_TYPE,
+          )
+          expect(messages[0].data["payload"]["topic_ids"]).to eq([topic3.id])
+          expect(DismissedTopicUser.where(user_id: user.id).pluck(:topic_id)).to eq([topic3.id])
+        end
+
         describe "when tracked param is true" do
           it "does not update user_stat.new_since and does not dismiss untracked topics" do
             put "/topics/reset-new.json?tracked=true",