FEATURE: Treat site settings as plain text and add a new HTML type. (#12618)

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

1 Like

This commit appears in #12618 which was approved by eviltrout. It was merged by romanrizzi.