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

@@ -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'

Gemfile.lock

@@ -525,6 +525,7 @@ DEPENDENCIES
   logstash-event
   logstash-logger
   logster
+  loofah
   lru_redux
   lz4-ruby
   mail!

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>

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

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

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

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
 #

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

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
 

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

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