SECURITY: Restrict unlisted topic creation (#19259)

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:

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?

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

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.

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

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) }