From 101ec21bc9a7f6a2c673d3d643df5825dba019e6 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 27 Oct 2022 11:26:14 +0800 Subject: [PATCH] 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 | 13 ++- ...r_badge_post_and_topic_attributes_mixin.rb | 27 ++++++ .../detailed_user_badge_serializer.rb | 25 ++++-- app/serializers/user_badge_serializer.rb | 8 +- spec/fabricators/user_badge_fabricator.rb | 8 ++ spec/requests/user_badges_controller_spec.rb | 80 ++++++++++++++++- .../detailed_user_badge_serializer_spec.rb | 89 +++++++++++++++++++ .../serializers/user_badge_serializer_spec.rb | 89 +++++++++++++++++++ 8 files changed, 325 insertions(+), 14 deletions(-) create mode 100644 app/serializers/concerns/user_badge_post_and_topic_attributes_mixin.rb create mode 100644 spec/fabricators/user_badge_fabricator.rb create mode 100644 spec/serializers/detailed_user_badge_serializer_spec.rb create mode 100644 spec/serializers/user_badge_serializer_spec.rb diff --git a/app/controllers/user_badges_controller.rb b/app/controllers/user_badges_controller.rb index abf2b9f054f..d87504d9a28 100644 --- a/app/controllers/user_badges_controller.rb +++ b/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 diff --git a/app/serializers/concerns/user_badge_post_and_topic_attributes_mixin.rb b/app/serializers/concerns/user_badge_post_and_topic_attributes_mixin.rb new file mode 100644 index 00000000000..3710dc5f23c --- /dev/null +++ b/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 diff --git a/app/serializers/detailed_user_badge_serializer.rb b/app/serializers/detailed_user_badge_serializer.rb index b924fffb26d..e31d8f8e8a7 100644 --- a/app/serializers/detailed_user_badge_serializer.rb +++ b/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 diff --git a/app/serializers/user_badge_serializer.rb b/app/serializers/user_badge_serializer.rb index 6f904b01b51..4b2b536ef5a 100644 --- a/app/serializers/user_badge_serializer.rb +++ b/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 diff --git a/spec/fabricators/user_badge_fabricator.rb b/spec/fabricators/user_badge_fabricator.rb new file mode 100644 index 00000000000..b1d8f259a48 --- /dev/null +++ b/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 diff --git a/spec/requests/user_badges_controller_spec.rb b/spec/requests/user_badges_controller_spec.rb index 119c6de0fae..e4452d12081 100644 --- a/spec/requests/user_badges_controller_spec.rb +++ b/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) diff --git a/spec/serializers/detailed_user_badge_serializer_spec.rb b/spec/serializers/detailed_user_badge_serializer_spec.rb new file mode 100644 index 00000000000..6c4a713f10b --- /dev/null +++ b/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 diff --git a/spec/serializers/user_badge_serializer_spec.rb b/spec/serializers/user_badge_serializer_spec.rb new file mode 100644 index 00000000000..941c3445ef4 --- /dev/null +++ b/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