From 4cb7472376b24ddb9581a20af6dc7534a1c63e8f Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 10 Oct 2023 11:23:56 +0800 Subject: [PATCH] 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 | 37 +++++++- app/models/topic.rb | 21 ++--- lib/discourse_plugin_registry.rb | 3 + lib/plugin/instance.rb | 8 ++ lib/post_creator.rb | 1 - lib/topic_creator.rb | 4 +- spec/lib/post_creator_spec.rb | 8 +- spec/lib/topic_creator_spec.rb | 13 +-- spec/models/topic_spec.rb | 42 --------- spec/requests/posts_controller_spec.rb | 115 ++++++++++++++++++++++--- 10 files changed, 162 insertions(+), 90 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 43c95ba46c4..538413b777a 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index 4ed8755aa65..9457847360a 100644 --- a/app/models/topic.rb +++ b/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) diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index 53406bf0f96..cb1e8e22ba2 100644 --- a/lib/discourse_plugin_registry.rb +++ b/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 diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index af82b6411a1..17241f75005 100644 --- a/lib/plugin/instance.rb +++ b/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) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 787787b2edc..62451a4694f 100644 --- a/lib/post_creator.rb +++ b/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) diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 24245291c54..3a685b1d452 100644 --- a/lib/topic_creator.rb +++ b/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 diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index b2209df8e2d..66068953b76 100644 --- a/spec/lib/post_creator_spec.rb +++ b/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 diff --git a/spec/lib/topic_creator_spec.rb b/spec/lib/topic_creator_spec.rb index 6b128267452..3add4cd33ac 100644 --- a/spec/lib/topic_creator_spec.rb +++ b/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 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 4cdefea3625..c285c25d43a 100644 --- a/spec/models/topic_spec.rb +++ b/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) } diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 6959615dadb..485d954324e 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/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))