From 0ce38bd7bce862db251b882613ab7053ca777382 Mon Sep 17 00:00:00 2001 From: Selase Krakani <849886+s3lase@users.noreply.github.com> Date: Thu, 1 Dec 2022 10:26:35 +0000 Subject: [PATCH] SECURITY: Restrict unlisted topic creation (#19259) --- config/locales/server.en.yml | 1 + lib/guardian/topic_guardian.rb | 2 + lib/topic_creator.rb | 17 +++++- spec/lib/guardian/topic_guardian_spec.rb | 16 ++++++ spec/lib/topic_creator_spec.rb | 22 ++++++++ spec/requests/posts_controller_spec.rb | 71 ++++++++++++++++++++++++ 6 files changed, 128 insertions(+), 1 deletion(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8d29f5af5fe..6591d85b24a 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -589,6 +589,7 @@ en: target_user_not_found: "One of the users you are sending this message to could not be found." unable_to_update: "There was an error updating that topic." unable_to_tag: "There was an error tagging the topic." + unable_to_unlist: "Sorry, you cannot create an unlisted topic." featured_link: invalid: "is invalid. URL should include http:// or https://." user: diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb index 66df380baf4..c487de68978 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -181,6 +181,8 @@ module TopicGuardian can_moderate?(topic) || can_perform_action_available_to_group_moderators?(topic) end + alias :can_create_unlisted_topic? :can_toggle_topic_visibility? + def can_convert_topic?(topic) return false unless @user.in_any_groups?(SiteSetting.personal_message_enabled_groups_map) return false if topic.blank? diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index ca98d6a71a7..12ae6114a04 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -24,6 +24,8 @@ class TopicCreator # this allows us to add errors valid = topic.valid? + validate_visibility(topic) + category = find_category if category.present? && guardian.can_tag?(topic) tags = @opts[:tags].presence || [] @@ -46,6 +48,8 @@ class TopicCreator def create topic = Topic.new(setup_topic_params) + + validate_visibility!(topic) setup_tags(topic) if fields = @opts[:custom_fields] @@ -67,6 +71,18 @@ class TopicCreator private + def validate_visibility(topic) + if !@opts[:skip_validations] && !topic.visible && !guardian.can_create_unlisted_topic?(topic) + topic.errors.add(:base, :unable_to_unlist) + end + end + + def validate_visibility!(topic) + validate_visibility(topic) + + rollback_from_errors!(topic) if topic.errors.full_messages.present? + end + def create_shared_draft(topic) return if @opts[:shared_draft].blank? || @opts[:shared_draft] == 'false' @@ -302,5 +318,4 @@ class TopicCreator user end - end diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb index bfe003457c3..e0144821203 100644 --- a/spec/lib/guardian/topic_guardian_spec.rb +++ b/spec/lib/guardian/topic_guardian_spec.rb @@ -132,6 +132,22 @@ RSpec.describe TopicGuardian do end end + describe "#can_create_unlisted_topic?" do + it "returns true for moderators" do + expect(Guardian.new(moderator).can_create_unlisted_topic?(topic)).to eq(true) + end + + it "returns true for TL4 users" do + tl4_user = Fabricate(:user, trust_level: TrustLevel[4]) + + expect(Guardian.new(tl4_user).can_create_unlisted_topic?(topic)).to eq(true) + end + + it "returns false for regular users" do + expect(Guardian.new(user).can_create_unlisted_topic?(topic)).to eq(false) + end + end + # The test cases here are intentionally kept brief because majority of the cases are already handled by # `TopicGuardianCanSeeConsistencyCheck` which we run to ensure that the implementation between `TopicGuardian#can_see_topic_ids` # and `TopicGuardian#can_see_topic?` is consistent. diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb index b1536b8bec8..d815a6292a3 100644 --- a/spec/lib/topic_creator_spec.rb +++ b/spec/lib/topic_creator_spec.rb @@ -518,5 +518,27 @@ RSpec.describe TopicCreator do expect(topic.external_id).to eq('external_id') end end + + context "when invisible/unlisted" do + let(:unlisted_attrs) { valid_attrs.merge(visible: false) } + + it "throws an exception for a non-staff user" do + expect do + TopicCreator.create(user, Guardian.new(user), unlisted_attrs) + end.to raise_error(ActiveRecord::Rollback) + end + + it "is invalid for a non-staff user" do + expect(TopicCreator.new(user, Guardian.new(user), unlisted_attrs).valid?).to eq(false) + end + + it "creates unlisted topic for an admin" do + expect(TopicCreator.create(admin, Guardian.new(admin), unlisted_attrs)).to be_valid + end + + it "is valid for an admin" do + expect(TopicCreator.new(admin, Guardian.new(admin), unlisted_attrs).valid?).to eq(true) + end + end end end diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index a7ffbdb8bee..c2bc9181519 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -923,6 +923,34 @@ RSpec.describe PostsController do expect(response.status).to eq(422) end + + it "creates unlisted topic with admin master key" do + master_key = Fabricate(:api_key).key + + expect do + post "/posts.json", + params: { raw: "this is a test title", title: "this is test body", unlist_topic: true }, + headers: { HTTP_API_USERNAME: admin.username, HTTP_API_KEY: master_key } + end.to change { Topic.count }.by(1) + + expect(response.status).to eq(200) + expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false) + end + + it "prevents creation of unlisted topic with non-admin key" do + user_key = ApiKey.create!(user: user).key + + expect do + post "/posts.json", + params: { raw: "this is a test title", title: "this is test body", unlist_topic: true }, + headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: user_key } + end.not_to change { Topic.count } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include( + I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist") + ) + end end describe "when logged in" do @@ -1250,6 +1278,7 @@ RSpec.describe PostsController do expect(topic.title).to eq('This is the test title for the topic') expect(topic.category).to eq(category) expect(topic.meta_data).to eq("xyz" => 'abc') + expect(topic.visible).to eq(true) end it 'can create an uncategorized topic' do @@ -1411,6 +1440,48 @@ RSpec.describe PostsController do end end + context "with topic unlisting" do + context "when logged in as staff" do + before do + sign_in(admin) + end + + it "creates an unlisted topic" do + expect do + post "/posts.json", params: { + raw: "this is the test content", + title: "this is the test title for the topic", + unlist_topic: true + } + end.to change { Topic.count }.by(1) + + expect(response.status).to eq(200) + expect(Topic.find(response.parsed_body["topic_id"]).visible).to eq(false) + end + end + + context "when logged in as a non-staff user" do + before do + sign_in(user) + end + + it "prevents creation of an unlisted topic" do + expect do + post "/posts.json", params: { + raw: "this is the test content", + title: "this is the test title for the topic", + unlist_topic: true + } + end.not_to change { Topic.count } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to include( + I18n.t("activerecord.errors.models.topic.attributes.base.unable_to_unlist") + ) + end + end + end + describe 'shared draft' do fab!(:destination_category) { Fabricate(:category) }