From fbd6c300d2c116082403ff0f40365ca288ca9ece Mon Sep 17 00:00:00 2001 From: Selase Krakani <849886+s3lase@users.noreply.github.com> Date: Fri, 2 Dec 2022 15:55:17 +0000 Subject: [PATCH] SECURITY: Restrict unlisted topic creation (#19258) --- config/locales/server.en.yml | 1 + lib/guardian/topic_guardian.rb | 2 + lib/topic_creator.rb | 17 ++++- .../guardian/topic_guardian_spec.rb | 16 +++++ spec/components/post_creator_spec.rb | 14 +++- spec/components/topic_creator_spec.rb | 22 ++++++ spec/requests/posts_controller_spec.rb | 71 +++++++++++++++++++ 7 files changed, 140 insertions(+), 3 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index cb05e87cf31..b8be93b81c0 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -580,6 +580,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 904dc3de272..e7dac7258fe 100644 --- a/lib/guardian/topic_guardian.rb +++ b/lib/guardian/topic_guardian.rb @@ -163,6 +163,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 SiteSetting.enable_personal_messages? return false if topic.blank? diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 1211c84fe78..4f12779d098 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 || [] @@ -45,6 +47,8 @@ class TopicCreator def create topic = Topic.new(setup_topic_params) + + validate_visibility!(topic) setup_tags(topic) if fields = @opts[:custom_fields] @@ -66,6 +70,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' @@ -298,5 +314,4 @@ class TopicCreator user end - end diff --git a/spec/components/guardian/topic_guardian_spec.rb b/spec/components/guardian/topic_guardian_spec.rb index c6da52b9ee4..b89b8f26236 100644 --- a/spec/components/guardian/topic_guardian_spec.rb +++ b/spec/components/guardian/topic_guardian_spec.rb @@ -130,6 +130,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/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 484ff8140aa..93d918c72ba 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -43,15 +43,25 @@ describe PostCreator do expect(post.wiki).to eq(true) end - it "can be created with a hidden reason" do + it "creates post with a hidden reason for staff user" do hri = Post.hidden_reasons[:flag_threshold_reached] - post = PostCreator.create(user, basic_topic_params.merge(hidden_reason_id: hri)) + post = PostCreator.create(admin, basic_topic_params.merge(hidden_reason_id: hri)) expect(post.hidden).to eq(true) expect(post.hidden_at).to be_present expect(post.hidden_reason_id).to eq(hri) expect(post.topic.visible).to eq(false) end + it "fails to create post with a hidden reason for non-staff user" do + hri = Post.hidden_reasons[:flag_threshold_reached] + + expect do + post = PostCreator.create(user, basic_topic_params.merge(hidden_reason_id: hri)) + + expect(post).to be_nil + end.not_to change { Post.count } + end + it "ensures the user can create the topic" do Guardian.any_instance.expects(:can_create?).with(Topic, nil).returns(false) expect { creator.create }.to raise_error(Discourse::InvalidAccess) diff --git a/spec/components/topic_creator_spec.rb b/spec/components/topic_creator_spec.rb index 433462a4c21..5cae61efa46 100644 --- a/spec/components/topic_creator_spec.rb +++ b/spec/components/topic_creator_spec.rb @@ -286,5 +286,27 @@ describe TopicCreator do expect(topic.pinned_at).to eq_time(time2) 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 883eef44b4d..1b089bc4c81 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -850,6 +850,34 @@ 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 @@ -1173,6 +1201,7 @@ 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 @@ -1333,6 +1362,48 @@ 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) }