From 5e4c0e2caadd6800c3ebf6b90334a7c70ffac2bf Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 7 Apr 2021 12:51:19 -0300 Subject: [PATCH] FEATURE: Treat site settings as plain text and add a new HTML type. (#12618) To add an extra layer of security, we sanitize settings before shipping them to the client. We don't sanitize those that have the "html" type. The CookedPostProcessor already uses Loofah for sanitization, so I chose to also use it for this. I added it to our gemfile since we installed it as a transitive dependency. --- Gemfile | 1 + Gemfile.lock | 1 + .../components/site-settings/html.hbs | 2 + .../admin/site_settings_controller.rb | 5 ++- app/services/site_settings_task.rb | 2 +- app/services/user_merger.rb | 2 +- config/site_settings.yml | 1 + lib/site_setting_extension.rb | 22 ++++++++-- lib/site_settings/type_supervisor.rb | 3 +- .../components/site_setting_extension_spec.rb | 40 ++++++++++++++++++- .../site_settings/type_supervisor_spec.rb | 3 ++ 11 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs diff --git a/Gemfile b/Gemfile index f446e63b3e1..e4963029f72 100644 --- a/Gemfile +++ b/Gemfile @@ -96,6 +96,7 @@ gem 'discourse_image_optim', require: 'image_optim' gem 'multi_json' gem 'mustache' gem 'nokogiri' +gem 'loofah' gem 'css_parser', require: false gem 'omniauth' diff --git a/Gemfile.lock b/Gemfile.lock index c542933c73f..075b4e0142d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -525,6 +525,7 @@ DEPENDENCIES logstash-event logstash-logger logster + loofah lru_redux lz4-ruby mail! diff --git a/app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs b/app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs new file mode 100644 index 00000000000..0da61a84e1f --- /dev/null +++ b/app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs @@ -0,0 +1,2 @@ +{{text-field value=(html-safe value) classNames="input-setting-string"}} +
{{html-safe setting.description}}
diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 6a1b7bda3d3..b909adf06a7 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -6,7 +6,10 @@ class Admin::SiteSettingsController < Admin::AdminController end def index - render_json_dump(site_settings: SiteSetting.all_settings, diags: SiteSetting.diags) + render_json_dump( + site_settings: SiteSetting.all_settings(sanitize_plain_text_settings: true), + diags: SiteSetting.diags + ) end def update diff --git a/app/services/site_settings_task.rb b/app/services/site_settings_task.rb index f9d185cdf2f..64c80486ffe 100644 --- a/app/services/site_settings_task.rb +++ b/app/services/site_settings_task.rb @@ -2,7 +2,7 @@ class SiteSettingsTask def self.export_to_hash(include_defaults: false, include_hidden: false) - site_settings = SiteSetting.all_settings(include_hidden) + site_settings = SiteSetting.all_settings(include_hidden: include_hidden) h = {} site_settings.each do |site_setting| next if site_setting[:default] == site_setting[:value] if !include_defaults diff --git a/app/services/user_merger.rb b/app/services/user_merger.rb index c0a6b6b826b..c322d5f3638 100644 --- a/app/services/user_merger.rb +++ b/app/services/user_merger.rb @@ -148,7 +148,7 @@ class UserMerger def update_site_settings ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.updating_site_settings") }, user_ids: [@acting_user.id] if @acting_user - SiteSetting.all_settings(true).each do |setting| + SiteSetting.all_settings(include_hidden: true).each do |setting| if setting[:type] == "username" && setting[:value] == @source_user.username SiteSetting.set_and_log(setting[:setting], @target_user.username) end diff --git a/config/site_settings.yml b/config/site_settings.yml index 04d7de530d6..327b1bee977 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -19,6 +19,7 @@ # type: username - Must match the username of an existing user. # type: list - A list of values, chosen from a set of valid values defined in the choices option. # type: enum - A single value, chosen from a set of valid values in the choices option. +# type: html - A single value. It could contain HTML. # # A type:list setting with the word 'colors' in its name will make color values have a bold line of the corresponding color # diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 6b02292c6d3..3196e2a97b5 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -208,14 +208,20 @@ module SiteSettingExtension def client_settings_json_uncached MultiJson.dump(Hash[*@client_settings.map do |name| value = self.public_send(name) - value = value.to_s if type_supervisor.get_type(name) == :upload - value = value.map(&:to_s).join("|") if type_supervisor.get_type(name) == :uploaded_image_list + type = type_supervisor.get_type(name) + value = value.to_s if type == :upload + value = value.map(&:to_s).join("|") if type == :uploaded_image_list + + if should_sanitize?(value, type) + value = sanitize(value) + end + [name, value] end.flatten]) end # Retrieve all settings - def all_settings(include_hidden = false) + def all_settings(include_hidden: false, sanitize_plain_text_settings: false) locale_setting_hash = { @@ -244,6 +250,8 @@ module SiteSettingExtension default.to_i < Upload::SEEDED_ID_THRESHOLD default = default_uploads[default.to_i] + elsif sanitize_plain_text_settings && should_sanitize?(value, type_hash[:type].to_s) + value = sanitize(value) end opts = { @@ -574,6 +582,14 @@ module SiteSettingExtension end end + def should_sanitize?(value, type) + value.is_a?(String) && type.to_s != 'html' + end + + def sanitize(value) + CGI.unescapeHTML(Loofah.scrub_fragment(value, :strip).to_s) + end + def logger Rails.logger end diff --git a/lib/site_settings/type_supervisor.rb b/lib/site_settings/type_supervisor.rb index ee78697ebf2..b33030c713b 100644 --- a/lib/site_settings/type_supervisor.rb +++ b/lib/site_settings/type_supervisor.rb @@ -36,7 +36,8 @@ class SiteSettings::TypeSupervisor tag_list: 21, color: 22, simple_list: 23, - emoji_list: 24 + emoji_list: 24, + html: 25 ) end diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index 2db0f2c494c..0180122715e 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -631,7 +631,7 @@ describe SiteSettingExtension do end it "is present in all_settings when we ask for hidden" do - expect(settings.all_settings(true).find { |s| s[:setting] == :superman_identity }).to be_present + expect(settings.all_settings(include_hidden: true).find { |s| s[:setting] == :superman_identity }).to be_present end end @@ -842,6 +842,23 @@ describe SiteSettingExtension do expect(setting[:default]).to eq(system_upload.url) end end + + it 'should sanitize html in the site settings' do + settings.setting(:with_html, 'rest') + settings.refresh! + + setting = settings.all_settings(sanitize_plain_text_settings: true).last + + expect(setting[:value]).to eq('rest') + end + + it 'settings with html type are not sanitized' do + settings.setting(:with_html, 'rest', type: :html) + + setting = settings.all_settings(sanitize_plain_text_settings: true).last + + expect(setting[:value]).to eq('rest') + end end describe '.client_settings_json_uncached' do @@ -855,6 +872,27 @@ describe SiteSettingExtension do %Q|{"default_locale":"#{SiteSetting.default_locale}","upload_type":"#{upload.url}","string_type":"haha"}| ) end + + it 'should sanitize html in the site settings' do + settings.setting(:with_html, 'rest', client: true) + settings.setting(:with_symbols, '<>rest', client: true) + settings.setting(:with_unknown_tag, 'rest', client: true) + settings.refresh! + + client_settings = JSON.parse settings.client_settings_json_uncached + + expect(client_settings['with_html']).to eq('rest') + expect(client_settings['with_symbols']).to eq('<>rest') + expect(client_settings['with_unknown_tag']).to eq('rest') + end + + it 'settings with html type are not sanitized' do + settings.setting(:with_html, 'rest', type: :html, client: true) + + client_settings = JSON.parse settings.client_settings_json_uncached + + expect(client_settings['with_html']).to eq('rest') + end end describe '.setup_methods' do diff --git a/spec/components/site_settings/type_supervisor_spec.rb b/spec/components/site_settings/type_supervisor_spec.rb index 36c6f82a197..f565b86eb05 100644 --- a/spec/components/site_settings/type_supervisor_spec.rb +++ b/spec/components/site_settings/type_supervisor_spec.rb @@ -94,6 +94,9 @@ describe SiteSettings::TypeSupervisor do it "'emoji_list' should be at the right position" do expect(SiteSettings::TypeSupervisor.types[:emoji_list]).to eq(24) end + it "'html' should be at the right position" do + expect(SiteSettings::TypeSupervisor.types[:html]).to eq(25) + end end end