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.
diff --git a/Gemfile b/Gemfile
index f446e63..e496302 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 c542933..075b4e0 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 0000000..0da61a8
--- /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"}}
+<div class="desc">{{html-safe setting.description}}</div>
diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb
index 6a1b7bd..b909adf 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 f9d185c..64c8048 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 c0a6b6b..c322d5f 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 04d7de5..327b1be 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 6b02292..3196e2a 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 ee78697..b33030c 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 2db0f2c..0180122 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, '<script></script>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, '<script></script>rest', type: :html)
+
+ setting = settings.all_settings(sanitize_plain_text_settings: true).last
+
+ expect(setting[:value]).to eq('<script></script>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, '<script></script>rest', client: true)
+ settings.setting(:with_symbols, '<>rest', client: true)
+ settings.setting(:with_unknown_tag, '<rest>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, '<script></script>rest', type: :html, client: true)
+
+ client_settings = JSON.parse settings.client_settings_json_uncached
+
+ expect(client_settings['with_html']).to eq('<script></script>rest')
+ end
end
describe '.setup_methods' do
[... diff too long, it was truncated ...]
GitHub sha: 5e4c0e2c