SECURITY: Prevent arbitrary topic custom fields from being set

Why this change?

The `PostsController#create` action allows arbitrary topic custom fields
to be set by any user that can create a topic. Without any restrictions,
this opens us up to potential security issues where plugins may be using
topic custom fields in security sensitive areas.

What does this change do?

1. This change introduces the `register_editable_topic_custom_field` plugin
API which allows plugins to register topic custom fields that are
editable either by staff users only or all users. The registered
editable topic custom fields are stored in `DiscoursePluginRegistry` and
is called by a new method `Topic#editable_custom_fields` which is then
used in the `PostsController#create` controller action. When an unpermitted custom fields is present in the `meta_data` params,
a 400 response code is returned.

2. Removes all reference to `meta_data` on a topic as it is confusing
   since we actually mean topic custom fields instead.

app/controllers/posts_controller.rb

@@ -850,8 +850,22 @@ class PostsController < ApplicationController
         .permit(*permitted)
         .tap do |allowed|
           allowed[:image_sizes] = params[:image_sizes]
-          # TODO this does not feel right, we should name what meta_data is allowed
-          allowed[:meta_data] = params[:meta_data]
+
+          if params.has_key?(:meta_data)
+            Discourse.deprecate(
+              "the :meta_data param is deprecated, use the :topic_custom_fields param instead",
+              since: "3.2",
+              drop_from: "3.3",
+            )
+          end
+
+          topic_custom_fields = {}
+          topic_custom_fields.merge!(editable_topic_custom_fields(:meta_data))
+          topic_custom_fields.merge!(editable_topic_custom_fields(:topic_custom_fields))
+
+          if topic_custom_fields.present?
+            allowed[:topic_opts] = { custom_fields: topic_custom_fields }
+          end
         end
 
     # Staff are allowed to pass `is_warning`
@@ -913,6 +927,25 @@ class PostsController < ApplicationController
     result.to_h
   end
 
+  def editable_topic_custom_fields(params_key)
+    if (topic_custom_fields = params[params_key]).present?
+      editable_topic_custom_fields = Topic.editable_custom_fields(guardian)
+
+      if (
+           unpermitted_topic_custom_fields =
+             topic_custom_fields.except(*editable_topic_custom_fields)
+         ).present?
+        raise Discourse::InvalidParameters.new(
+                "The following keys in :#{params_key} are not permitted: #{unpermitted_topic_custom_fields.keys.join(", ")}",
+              )
+      end
+
+      topic_custom_fields.permit(*editable_topic_custom_fields).to_h
+    else
+      {}
+    end
+  end
+
   def signature_for(args)
     +"post##" << Digest::SHA1.hexdigest(
       args

app/models/topic.rb

@@ -295,7 +295,6 @@ class Topic < ActiveRecord::Base
   attr_accessor :participants
   attr_accessor :participant_groups
   attr_accessor :topic_list
-  attr_accessor :meta_data
   attr_accessor :include_last_poster
   attr_accessor :import_mode # set to true to optimize creation and save for imports
 
@@ -621,19 +620,6 @@ class Topic < ActiveRecord::Base
     topics
   end
 
-  def meta_data=(data)
-    custom_fields.replace(data)
-  end
-
-  def meta_data
-    custom_fields
-  end
-
-  def update_meta_data(data)
-    custom_fields.update(data)
-    save
-  end
-
   def reload(options = nil)
     @post_numbers = nil
     @public_topic_timer = nil
@@ -2068,6 +2054,13 @@ class Topic < ActiveRecord::Base
     tags.reject { |tag| guardian.hidden_tag_names.include?(tag[:name]) }
   end
 
+  def self.editable_custom_fields(guardian)
+    fields = []
+    fields.push(*DiscoursePluginRegistry.public_editable_topic_custom_fields)
+    fields.push(*DiscoursePluginRegistry.staff_editable_topic_custom_fields) if guardian.is_staff?
+    fields
+  end
+
   private
 
   def invite_to_private_message(invited_by, target_user, guardian)

lib/discourse_plugin_registry.rb

@@ -77,6 +77,9 @@ class DiscoursePluginRegistry
   define_filtered_register :staff_user_custom_fields
   define_filtered_register :public_user_custom_fields
 
+  define_filtered_register :staff_editable_topic_custom_fields
+  define_filtered_register :public_editable_topic_custom_fields
+
   define_filtered_register :self_editable_user_custom_fields
   define_filtered_register :staff_editable_user_custom_fields
 

lib/plugin/instance.rb

@@ -197,6 +197,14 @@ class Plugin::Instance
     DiscoursePluginRegistry.register_public_user_custom_field(field, self)
   end
 
+  def register_editable_topic_custom_field(field, staff_only: false)
+    if staff_only
+      DiscoursePluginRegistry.register_staff_editable_topic_custom_field(field, self)
+    else
+      DiscoursePluginRegistry.register_public_editable_topic_custom_field(field, self)
+    end
+  end
+
   def register_editable_user_custom_field(field, staff_only: false)
     if staff_only
       DiscoursePluginRegistry.register_staff_editable_user_custom_field(field, self)

lib/post_creator.rb

@@ -50,7 +50,6 @@ class PostCreator
   #     category              - Category to assign to topic
   #     target_usernames      - comma delimited list of usernames for membership (private message)
   #     target_group_names    - comma delimited list of groups for membership (private message)
-  #     meta_data             - Topic meta data hash
   #     created_at            - Topic creation time (optional)
   #     pinned_at             - Topic pinned time (optional)
   #     pinned_globally       - Is the topic pinned globally (optional)

lib/topic_creator.rb

@@ -48,7 +48,7 @@ class TopicCreator
     setup_tags(topic)
 
     if fields = @opts[:custom_fields]
-      topic.custom_fields.merge!(fields)
+      topic.custom_fields = fields
     end
 
     DiscourseEvent.trigger(:before_create_topic, topic, self)
@@ -122,7 +122,7 @@ class TopicCreator
       visible: @opts[:visible],
     }
 
-    %i[subtype archetype meta_data import_mode advance_draft].each do |key|
+    %i[subtype archetype import_mode advance_draft].each do |key|
       topic_params[key] = @opts[key] if @opts[key].present?
     end
 

spec/lib/post_creator_spec.rb

@@ -23,9 +23,6 @@ RSpec.describe PostCreator do
     let(:creator_with_category) do
       PostCreator.new(user, basic_topic_params.merge(category: category.id))
     end
-    let(:creator_with_meta_data) do
-      PostCreator.new(user, basic_topic_params.merge(meta_data: { hello: "world" }))
-    end
     let(:creator_with_image_sizes) do
       PostCreator.new(user, basic_topic_params.merge(image_sizes: image_sizes))
     end
@@ -96,6 +93,7 @@ RSpec.describe PostCreator do
           user,
           basic_topic_params.merge(topic_opts: { custom_fields: { hello: "world" } }),
         )
+
       expect(post.topic.custom_fields).to eq("hello" => "world")
     end
 
@@ -330,10 +328,6 @@ RSpec.describe PostCreator do
         expect(creator_with_category.create.topic.category).to eq(category)
       end
 
-      it "adds  meta data from the post" do
-        expect(creator_with_meta_data.create.topic.meta_data["hello"]).to eq("world")
-      end
-
       it "passes the image sizes through" do
         Post.any_instance.expects(:image_sizes=).with(image_sizes)
         creator_with_image_sizes.create

spec/lib/topic_creator_spec.rb

@@ -40,20 +40,11 @@ RSpec.describe TopicCreator do
         expect(TopicCreator.create(moderator, Guardian.new(moderator), valid_attrs)).to be_valid
       end
 
-      it "supports both meta_data and custom_fields" do
-        opts =
-          valid_attrs.merge(
-            meta_data: {
-              import_topic_id: "foo",
-            },
-            custom_fields: {
-              import_id: "bar",
-            },
-          )
+      it "supports custom_fields that has been registered to the DiscoursePluginRegistry" do
+        opts = valid_attrs.merge(custom_fields: { import_id: "bar" })
 
         topic = TopicCreator.create(admin, Guardian.new(admin), opts)
 
-        expect(topic.custom_fields["import_topic_id"]).to eq("foo")
         expect(topic.custom_fields["import_id"]).to eq("bar")
       end
 

spec/models/topic_spec.rb

@@ -1705,48 +1705,6 @@ RSpec.describe Topic do
     end
   end
 
-  describe "meta data" do
-    fab!(:topic) { Fabricate(:topic, meta_data: { "hello" => "world" }) }
-
-    it "allows us to create a topic with meta data" do
-      expect(topic.meta_data["hello"]).to eq("world")
-    end
-
-    context "when updating" do
-      context "with existing key" do
-        before { topic.update_meta_data("hello" => "bane") }
-
-        it "updates the key" do
-          expect(topic.meta_data["hello"]).to eq("bane")
-        end
-      end
-
-      context "with a new key" do
-        before { topic.update_meta_data("city" => "gotham") }
-
-        it "adds the new key" do
-          expect(topic.meta_data["city"]).to eq("gotham")
-          expect(topic.meta_data["hello"]).to eq("world")
-        end
-      end
-
-      context "with a new key" do
-        before_all do
-          topic.update_meta_data("other" => "key")
-          topic.save!
-        end
-
-        it "can be loaded" do
-          expect(Topic.find(topic.id).meta_data["other"]).to eq("key")
-        end
-
-        it "is in sync with custom_fields" do
-          expect(Topic.find(topic.id).custom_fields["other"]).to eq("key")
-        end
-      end
-    end
-  end
-
   describe "after create" do
     fab!(:topic) { Fabricate(:topic) }
 

spec/requests/posts_controller_spec.rb

@@ -1333,9 +1333,6 @@ RSpec.describe PostsController do
                raw: "this is the test content",
                title: "this is the test title for the topic",
                category: category.id,
-               meta_data: {
-                 xyz: "abc",
-               },
              }
 
         expect(response.status).to eq(403)
@@ -1407,15 +1404,12 @@ RSpec.describe PostsController do
         expect(Post.last.topic.tags.count).to eq(1)
       end
 
-      it "creates the post" do
+      it "creates the topic and post with the right attributes" do
         post "/posts.json",
              params: {
                raw: "this is the test content",
                title: "this is the test title for the topic",
                category: category.id,
-               meta_data: {
-                 xyz: "abc",
-               },
              }
 
         expect(response.status).to eq(200)
@@ -1427,10 +1421,112 @@ RSpec.describe PostsController do
         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
 
+      context "when adding custom fields to topic via the `topic_custom_fields` param" do
+        it "should return a 400 response code when no custom fields has been permitted" do
+          sign_in(user)
+
+          post "/posts.json",
+               params: {
+                 raw: "this is the test content",
+                 title: "this is the test title for the topic",
+                 category: category.id,
+                 topic_custom_fields: {
+                   xyz: "abc",
+                   abc: "xyz",
+                 },
+               }
+
+          expect(response.status).to eq(400)
+          expect(Topic.last.custom_fields).to eq({})
+        end
+
+        context "when custom fields has been permitted" do
+          fab!(:plugin) do
+            plugin = Plugin::Instance.new
+            plugin.register_editable_topic_custom_field(:xyz)
+            plugin.register_editable_topic_custom_field(:abc, staff_only: true)
+            plugin
+          end
+
+          it "should return a 400 response when trying to add a staff ony custom field for a non-staff user" do
+            sign_in(user)
+
+            post "/posts.json",
+                 params: {
+                   raw: "this is the test content",
+                   title: "this is the test title for the topic",
+                   category: category.id,
+                   topic_custom_fields: {
+                     abc: "xyz",
+                   },
+                 }
+
+            expect(response.status).to eq(400)
+            expect(Topic.last.custom_fields).to eq({})
+          end
+
+          it "should add custom fields to topic that is permitted for a non-staff user" do
+            sign_in(user)
+
+            post "/posts.json",
+                 params: {
+                   raw: "this is the test content",
+                   title: "this is the test title for the topic",
+                   category: category.id,
+                   topic_custom_fields: {
+                     xyz: "abc",
+                   },
+                 }
+
+            expect(response.status).to eq(200)
+            expect(Topic.last.custom_fields).to eq({ "xyz" => "abc" })
+          end
+
+          it "should add custom fields to topic that is permitted for a non-staff user via the deprecated `meta_data` param" do
+            sign_in(user)
+
+            Discourse.expects(:deprecate).with(
+              "the :meta_data param is deprecated, use the :topic_custom_fields param instead",
+              anything,
+            )
+
+            post "/posts.json",
+                 params: {
+                   raw: "this is the test content",
+                   title: "this is the test title for the topic",
+                   category: category.id,
+                   meta_data: {
+                     xyz: "abc",
+                   },
+                 }
+
+            expect(response.status).to eq(200)
+            expect(Topic.last.custom_fields).to eq({ "xyz" => "abc" })
+          end
+
+          it "should add custom fields to topic that is permitted for a staff user and public user" do
+            sign_in(Fabricate(:admin))
+
+            post "/posts.json",
+                 params: {
+                   raw: "this is the test content",
+                   title: "this is the test title for the topic",
+                   category: category.id,
+                   topic_custom_fields: {
+                     xyz: "abc",
+                     abc: "xyz",
+                   },
+                 }
+
+            expect(response.status).to eq(200)
+            expect(Topic.last.custom_fields).to eq({ "xyz" => "abc", "abc" => "xyz" })
+          end
+        end
+      end
+
       it "can create an uncategorized topic" do
         title = "this is the test title for the topic"
 
@@ -1562,9 +1658,6 @@ RSpec.describe PostsController do
                  raw:
                    "this is the test content http://fakespamwebsite.com http://fakespamwebsite.com/spam http://fakespamwebsite.com/spammy",
                  title: "this is the test title for the topic",
-                 meta_data: {
-                   xyz: "abc",
-                 },
                }
 
           expect(response.parsed_body["errors"]).to include(I18n.t(:spamming_host))