SECURITY: Restrict display of topic titles associated with user badges (#18768)

Before this commit, we did not have guardian checks in place to determine if a
topic's title associated with a user badge should be displayed or not.
This means that the topic title of topics with restricted access
could be leaked to anon and users without access if certain conditions
are met. While we will not specify the conditions required, we have internally
assessed that the odds of meeting such conditions are low.

With this commit, we will now apply a guardian check to ensure that the
current user is able to see a topic before the topic's title is included
in the serialized object of a `UserBadge`.

app/controllers/user_badges_controller.rb

@@ -24,11 +24,19 @@ class UserBadgesController < ApplicationController
       user_badges = user_badges.offset(offset.to_i)
     end
 
+    user_badges_topic_ids = user_badges.map { |user_badge| user_badge.post&.topic_id }.compact
+
     user_badges = UserBadges.new(user_badges: user_badges,
                                  username: params[:username],
                                  grant_count: grant_count)
 
-    render_serialized(user_badges, UserBadgesSerializer, root: :user_badge_info, include_long_description: true)
+    render_serialized(
+      user_badges,
+      UserBadgesSerializer,
+      root: :user_badge_info,
+      include_long_description: true,
+      allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: user_badges_topic_ids)
+    )
   end
 
   def username
@@ -46,9 +54,12 @@ class UserBadgesController < ApplicationController
       .includes(post: :topic)
       .includes(:granted_by)
 
+    user_badges_topic_ids = user_badges.map { |user_badge| user_badge.post&.topic_id }.compact
+
     render_serialized(
       user_badges,
       DetailedUserBadgeSerializer,
+      allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: user_badges_topic_ids),
       root: :user_badges,
     )
   end

app/serializers/concerns/user_badge_post_and_topic_attributes_mixin.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+# Post and topic attributes are only included when the `allowed_user_badge_topic_ids` option is provided. The caller of the
+# serializer is responsible for ensuring that the topic ids in the options can be seen by the scope of the user by passing
+# the result of `Guardian#can_see_topic_ids` to the `allowed_user_badge_topic_ids` option.
+module UserBadgePostAndTopicAttributesMixin
+  private
+
+  def include_post_attributes?
+    return false if !object.badge.show_posts || !object.post
+    return true if scope.is_admin?
+
+    allowed_user_badge_topic_ids = options[:allowed_user_badge_topic_ids]
+
+    return false if allowed_user_badge_topic_ids.blank?
+
+    topic_id = object.post.topic_id
+
+    return false if topic_id.blank?
+
+    allowed_user_badge_topic_ids.include?(topic_id)
+  end
+
+  def include_topic_attributes?
+    include_post_attributes? && object.post.topic
+  end
+end

app/serializers/detailed_user_badge_serializer.rb

@@ -1,27 +1,34 @@
 # frozen_string_literal: true
 
 class DetailedUserBadgeSerializer < BasicUserBadgeSerializer
+  include UserBadgePostAndTopicAttributesMixin
+
   has_one :granted_by, serializer: UserBadgeSerializer::UserSerializer
 
   attributes :post_number, :topic_id, :topic_title, :is_favorite, :can_favorite
 
-  def include_post_number?
-    object.post
+  def post_number
+    object.post.post_number
   end
 
-  alias :include_topic_id? :include_post_number?
-  alias :include_topic_title? :include_post_number?
-
-  def post_number
-    object.post.post_number if object.post
+  def include_post_number?
+    include_post_attributes?
   end
 
   def topic_id
-    object.post.topic_id if object.post
+    object.post.topic_id
+  end
+
+  def include_topic_id?
+    include_topic_attributes?
   end
 
   def topic_title
-    object.post.topic.title if object.post && object.post.topic
+    object.post.topic.title
+  end
+
+  def include_topic_title?
+    include_topic_id?
   end
 
   def can_favorite

app/serializers/user_badge_serializer.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class UserBadgeSerializer < ApplicationSerializer
+  include UserBadgePostAndTopicAttributesMixin
 
   class UserSerializer < BasicUserSerializer
     include UserPrimaryGroupMixin
@@ -22,10 +23,9 @@ class UserBadgeSerializer < ApplicationSerializer
   end
 
   def include_post_id?
-    object.badge.show_posts && object.post_id && object.post
+    include_post_attributes?
   end
 
-  alias :include_topic? :include_post_id?
   alias :include_post_number? :include_post_id?
 
   def post_number
@@ -35,4 +35,8 @@ class UserBadgeSerializer < ApplicationSerializer
   def topic
     object.post.topic
   end
+
+  def include_topic?
+    include_topic_attributes?
+  end
 end

spec/fabricators/user_badge_fabricator.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+Fabricator(:user_badge) do
+  user
+  badge
+  granted_at { Time.zone.now }
+  granted_by(fabricator: :admin)
+end

spec/requests/user_badges_controller_spec.rb

@@ -28,7 +28,18 @@ RSpec.describe UserBadgesController do
   end
 
   describe '#index' do
-    let!(:user_badge) { UserBadge.create(badge: badge, user: user, granted_by: Discourse.system_user, granted_at: Time.now) }
+    fab!(:post) { Fabricate(:post) }
+    fab!(:private_message_post) { Fabricate(:private_message_post) }
+    let(:topic) { post.topic }
+    let(:private_message_topic) { private_message_post.topic }
+    fab!(:group) { Fabricate(:group) }
+    fab!(:private_category) { Fabricate(:private_category, group: group) }
+    fab!(:restricted_topic) { Fabricate(:topic, category: private_category) }
+    fab!(:restricted_post) { Fabricate(:post, topic: restricted_topic) }
+    fab!(:badge) { Fabricate(:badge, show_posts: true) }
+    fab!(:user_badge) { Fabricate(:user_badge, user: user, badge: badge, post: post) }
+    fab!(:user_badge_2) { Fabricate(:user_badge, badge: badge, post: private_message_post) }
+    fab!(:user_badge_3) { Fabricate(:user_badge, badge: badge, post: restricted_post) }
 
     it 'requires username or badge_id to be specified' do
       get "/user_badges.json"
@@ -57,7 +68,7 @@ RSpec.describe UserBadgesController do
 
       expect(response.status).to eq(200)
       parsed = response.parsed_body
-      expect(parsed["user_badge_info"]["user_badges"].length).to eq(1)
+      expect(parsed["user_badge_info"]["user_badges"].length).to eq(3)
     end
 
     it 'includes counts when passed the aggregate argument' do
@@ -70,6 +81,71 @@ RSpec.describe UserBadgesController do
       expect(parsed["user_badges"].first.has_key?('count')).to eq(true)
     end
 
+    context 'for post and topic attributes associated with user badge' do
+      it 'does not include the attributes for the private topic when user is anon' do
+        get "/user_badges.json", params: { badge_id: badge.id }
+
+        expect(response.status).to eq(200)
+
+        parsed = response.parsed_body
+
+        expect(parsed["topics"].map { |t| t["id"] }).to contain_exactly(post.topic_id)
+
+        parsed_user_badges = parsed["user_badge_info"]["user_badges"]
+
+        expect(parsed_user_badges.map { |ub| ub["post_id"] }.compact).to contain_exactly(post.id)
+        expect(parsed_user_badges.map { |ub| ub["post_number"] }.compact).to contain_exactly(post.post_number)
+      end
+
+      it 'does not include the attributes for topics which the current user cannot see' do
+        get "/user_badges.json", params: { badge_id: badge.id }
+
+        expect(response.status).to eq(200)
+
+        parsed = response.parsed_body
+
+        expect(parsed["topics"].map { |t| t["id"] }).to contain_exactly(post.topic_id)
+
+        parsed_user_badges = parsed["user_badge_info"]["user_badges"]
+
+        expect(parsed_user_badges.map { |ub| ub["post_id"] }.compact).to contain_exactly(post.id)
+        expect(parsed_user_badges.map { |ub| ub["post_number"] }.compact).to contain_exactly(post.post_number)
+      end
+
+      it 'includes the attributes for regular topic, private messages and restricted topics which the current user can see' do
+        group.add(user)
+        private_message_topic.allowed_users << user
+
+        sign_in(user)
+
+        get "/user_badges.json", params: { badge_id: badge.id }
+
+        expect(response.status).to eq(200)
+
+        parsed = response.parsed_body
+
+        expect(parsed["topics"].map { |t| t["id"] }).to contain_exactly(
+          post.topic_id,
+          private_message_post.topic_id,
+          restricted_post.topic_id
+        )
+
+        parsed_user_badges = parsed["user_badge_info"]["user_badges"]
+
+        expect(parsed_user_badges.map { |ub| ub["post_id"] }.compact).to contain_exactly(
+          post.id,
+          private_message_post.id,
+          restricted_post.id
+        )
+
+        expect(parsed_user_badges.map { |ub| ub["post_number"] }.compact).to contain_exactly(
+          post.post_number,
+          private_message_post.post_number,
+          restricted_post.post_number
+        )
+      end
+    end
+
     context 'with hidden profiles' do
       before do
         user.user_option.update_columns(hide_profile_and_presence: true)

spec/serializers/detailed_user_badge_serializer_spec.rb

@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+RSpec.describe DetailedUserBadgeSerializer do
+  describe '#topic_id and #topic_title attributes' do
+    fab!(:user) { Fabricate(:user) }
+    fab!(:admin) { Fabricate(:admin) }
+    fab!(:post) { Fabricate(:post) }
+    fab!(:badge) { Fabricate(:badge, show_posts: true) }
+    fab!(:user_badge) { Fabricate(:user_badge, badge: badge, post_id: post.id) }
+    let(:guardian) { Guardian.new(user_badge.user) }
+
+    it 'does not include attributes in serialized object when badge has not been configured to show posts' do
+      badge.update!(show_posts: false)
+
+      guardian = Guardian.new
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id]),
+        root: false,
+      ).as_json
+
+      expect(serialized[:topic_id]).to eq(nil)
+      expect(serialized[:topic_title]).to eq(nil)
+    end
+
+    it 'does not include attributes in serialized object when user badge is not associated with a post' do
+      user_badge.update!(post_id: nil)
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id]),
+        root: false
+      ).as_json
+
+      expect(serialized[:topic_id]).to eq(nil)
+      expect(serialized[:topic_title]).to eq(nil)
+    end
+
+    it 'does not include attributes in serialized object when user badge is not associated with a topic' do
+      post.topic.destroy!
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id]),
+        root: false
+      ).as_json
+
+      expect(serialized[:topic_id]).to eq(nil)
+      expect(serialized[:topic_title]).to eq(nil)
+    end
+
+    it 'does not include attributes in serialized object when allowed_user_badge_topic_ids option is not provided' do
+      serialized = described_class.new(user_badge, scope: guardian, root: false).as_json
+
+      expect(serialized[:topic_id]).to eq(nil)
+      expect(serialized[:topic_title]).to eq(nil)
+    end
+
+    it 'does not included attributes in serialized object when topic id is not present in allowed_user_badge_topic_ids option' do
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id + 1]),
+        root: false
+      ).as_json
+
+      expect(serialized[:topic_id]).to eq(nil)
+      expect(serialized[:topic_title]).to eq(nil)
+    end
+
+    it 'includes attributes in serialized object for admin scope even if allowed_user_badge_topic_ids option is not provided' do
+      serialized = described_class.new(user_badge, scope: Guardian.new(admin), root: false).as_json
+
+      expect(serialized[:topic_id]).to eq(post.topic_id)
+      expect(serialized[:topic_title]).to eq(post.topic.title)
+    end
+
+    it 'includes attributes in serialized object when topic id is present in allowed_user_badge_topic_ids option' do
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id]),
+        root: false
+      ).as_json
+
+      expect(serialized[:topic_id]).to eq(post.topic_id)
+      expect(serialized[:topic_title]).to eq(post.topic.title)
+    end
+  end
+end

spec/serializers/user_badge_serializer_spec.rb

@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+RSpec.describe UserBadgeSerializer do
+  describe '#topic' do
+    fab!(:user) { Fabricate(:user) }
+    fab!(:admin) { Fabricate(:admin) }
+    fab!(:post) { Fabricate(:post) }
+    fab!(:badge) { Fabricate(:badge, show_posts: true) }
+    fab!(:user_badge) { Fabricate(:user_badge, badge: badge, post_id: post.id) }
+    let(:guardian) { Guardian.new(user_badge.user) }
+
+    it 'is not included in serialized object when badge has not been configured to show posts' do
+      badge.update!(show_posts: false)
+
+      guardian = Guardian.new
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id])
+      ).as_json
+
+      expect(serialized[:topics]).to eq(nil)
+    end
+
+    it 'is not included in serialized object when user badge is not associated with a post' do
+      user_badge.update!(post_id: nil)
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id])
+      ).as_json
+
+      expect(serialized[:topics]).to eq(nil)
+    end
+
+    it 'is not included in serialized object when user badge is not associated with a topic' do
+      post.topic.destroy!
+
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id])
+      ).as_json
+
+      expect(serialized[:topics]).to eq(nil)
+    end
+
+    it 'is not included in serialized object when allowed_user_badge_topic_ids option is not provided' do
+      serialized = described_class.new(user_badge, scope: guardian).as_json
+
+      expect(serialized[:topics]).to eq(nil)
+    end
+
+    it 'is not included in serialized object when topic id is not present in allowed_user_badge_topic_ids option' do
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id + 1])
+      ).as_json
+
+      expect(serialized[:topics]).to eq(nil)
+    end
+
+    it 'is included in serialized object for admin scope even if allowed_user_badge_topic_ids option is not provided' do
+      serialized = described_class.new(user_badge, scope: Guardian.new(admin)).as_json
+
+      serialized_topic = serialized[:topics][0]
+
+      expect(serialized_topic[:id]).to eq(post.topic_id)
+      expect(serialized_topic[:title]).to eq(post.topic.title)
+      expect(serialized_topic[:fancy_title]).to eq(post.topic.fancy_title)
+      expect(serialized_topic[:slug]).to eq(post.topic.slug)
+      expect(serialized_topic[:posts_count]).to eq(post.topic.reload.posts_count)
+    end
+
+    it 'is included in serialized object when topic id is present in allowed_user_badge_topic_ids option' do
+      serialized = described_class.new(user_badge,
+        scope: guardian,
+        allowed_user_badge_topic_ids: guardian.can_see_topic_ids(topic_ids: [post.topic_id])
+      ).as_json
+
+      serialized_topic = serialized[:topics][0]
+
+      expect(serialized_topic[:id]).to eq(post.topic_id)
+      expect(serialized_topic[:title]).to eq(post.topic.title)
+      expect(serialized_topic[:fancy_title]).to eq(post.topic.fancy_title)
+      expect(serialized_topic[:slug]).to eq(post.topic.slug)
+      expect(serialized_topic[:posts_count]).to eq(post.topic.reload.posts_count)
+    end
+  end
+end