CVE-2022-46159: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Discourse is an open-source discussion platform. In version 2.8.13 and prior on the `stable` branch and version 2.9.0.beta14 and prior on the `beta` and `tests-passed` branches, any authenticated user can create an unlisted topic. These topics, which are not readily available to other users, can take up unnecessary site resources. A patch for this issue is available in the `main` branch of Discourse. There are no known workarounds available.

CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 4.3
Published: 2022-12-02T15:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: config/locales/server.en.yml, lib/guardian/topic_guardian.rb, lib/topic_creator.rb, spec/lib/guardian/topic_guardian_spec.rb, spec/lib/topic_creator_spec.rb, spec/requests/posts_controller_spec.rb
Commit date: 2022-12-01 10:26:35 +0000
Commit message: SECURITY: Restrict unlisted topic creation (#19259)


Diff: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 8d29f5af5fea..6591d85b24a8 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -587,10 +587,11 @@ en:
               reply_by_email_disabled: "Reply by email has been disabled."
               send_to_email_disabled: "Sorry, you cannot send personal messages to email."
               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:
           attributes:
             password:
diff --git a/lib/guardian/topic_guardian.rb b/lib/guardian/topic_guardian.rb
index 66df380baf4c..c487de68978f 100644
--- a/lib/guardian/topic_guardian.rb
+++ b/lib/guardian/topic_guardian.rb
@@ -179,10 +179,12 @@ module TopicGuardian
 
   def can_toggle_topic_visibility?(topic)
     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?
     return false if topic.trashed?
     return false if topic.is_category_topic?
diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb
index ca98d6a71a75..12ae6114a043 100644
--- a/lib/topic_creator.rb
+++ b/lib/topic_creator.rb
@@ -22,10 +22,12 @@ class TopicCreator
     # validate? will clear the error hash
     # so we fire the validation event after
     # 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 || []
       existing_tags = tags.present? ? Tag.where(name: tags) : []
       valid_tags = guardian.can_create_tag? ? tags : existing_tags
@@ -44,10 +46,12 @@ class TopicCreator
     valid
   end
 
   def create
     topic = Topic.new(setup_topic_params)
+
+    validate_visibility!(topic)
     setup_tags(topic)
 
     if fields = @opts[:custom_fields]
       topic.custom_fields.merge!(fields)
     end
@@ -65,10 +69,22 @@ class TopicCreator
     topic
   end
 
   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'
 
     category_id = @opts[:category].blank? ? SiteSetting.shared_drafts_category.to_i : @opts[:category]
     SharedDraft.create(topic_id: topic.id, category_id: category_id)
@@ -300,7 +316,6 @@ class TopicCreator
       )
     end
 
     user
   end
-
 end
diff --git a/spec/lib/guardian/topic_guardian_spec.rb b/spec/lib/guardian/topic_guardian_spec.rb
index bfe003457c3c..e01448212034 100644
--- a/spec/lib/guardian/topic_guardian_spec.rb
+++ b/spec/lib/guardian/topic_guardian_spec.rb
@@ -130,10 +130,26 @@ RSpec.describe TopicGuardian do
 
       expect(Guardian.new(tl4_user).can_review_topic?(topic)).to eq(false)
     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.
   describe '#can_see_topic_ids' do
     it 'returns the topic ids for the topics which a user is allowed to see' do
diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb
index b1536b8bec84..d815a6292a36 100644
--- a/spec/lib/topic_creator_spec.rb
+++ b/spec/lib/topic_creator_spec.rb
@@ -516,7 +516,29 @@ 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 a7ffbdb8bee6..c2bc91815193 100644
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@@ -921,10 +921,38 @@ RSpec.describe PostsController do
           params: { title: 'this is a test title', raw: 'this is test body', embed_url: '/test.txt' },
           headers: { HTTP_API_USERNAME: user.username, HTTP_API_KEY: master_key }
 
         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
       fab!(:user) { Fabricate(:user) }
 
@@ -1248,10 +1276,11 @@ RSpec.describe PostsController do
         expect(new_post.user).to eq(user)
         expect(new_post.raw).to eq('this is the test content')
         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
         title = 'this is the test title for the topic'
 
@@ -1409,10 +1438,52 @@ RSpec.describe PostsController do
           end
         end
       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) }
 
       it "will raise an error for regular users" do
         post "/posts.json", params: {

Fixed code:               unable_to_unlist: "Sorry, you cannot create an unlisted topic."
  alias :can_create_unlisted_topic? :can_toggle_topic_visibility?

    validate_visibility(topic)


    validate_visibility!(topic)
  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

  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


    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

      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
        expect(topic.visible).to eq(true)
    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