DEV: Sanitize HTML admin inputs (#14681)

* DEV: Sanitize HTML admin inputs

This PR adds on-save HTML sanitization for:

Client site settings
translation overrides
badges descriptions
user fields descriptions

I used Rails's SafeListSanitizer, which [accepts the following HTML tags and attributes](018cf54073/lib/rails/html/sanitizer.rb (L108))

* Make sure that the sanitization logic doesn't corrupt settings with special characters

app/models/badge.rb

@@ -5,6 +5,7 @@ class Badge < ActiveRecord::Base
   self.ignored_columns = %w{image}
 
   include GlobalPath
+  include HasSanitizableFields
 
   # NOTE: These badge ids are not in order! They are grouped logically.
   #       When picking an id, *search* for it.
@@ -116,6 +117,7 @@ class Badge < ActiveRecord::Base
   scope :enabled, -> { where(enabled: true) }
 
   before_create :ensure_not_system
+  before_save :sanitize_description
 
   after_commit do
     SvgSprite.expire_cache
@@ -314,6 +316,12 @@ class Badge < ActiveRecord::Base
   def ensure_not_system
     self.id = [Badge.maximum(:id) + 1, 100].max unless id
   end
+
+  def sanitize_description
+    if description_changed?
+      self.description = sanitize_field(self.description)
+    end
+  end
 end
 
 # == Schema Information

app/models/concerns/has_sanitizable_fields.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+module HasSanitizableFields
+  extend ActiveSupport::Concern
+
+  def sanitize_field(field, additional_attributes: [])
+    if field
+      sanitizer = Rails::Html::SafeListSanitizer.new
+      allowed_attributes = Rails::Html::SafeListSanitizer.allowed_attributes
+
+      if additional_attributes.present?
+        allowed_attributes = allowed_attributes.merge(additional_attributes)
+      end
+
+      field = CGI.unescape_html(sanitizer.sanitize(field, attributes: allowed_attributes))
+      # Just replace the characters that our translations use for interpolation.
+      # Calling CGI.unescape removes characters like '+', which will corrupt the original value.
+      field = field.gsub('%7B', '{').gsub('%7D', '}')
+    end
+
+    field
+  end
+end

app/models/translation_override.rb

@@ -39,6 +39,7 @@ class TranslationOverride < ActiveRecord::Base
     }
   }
 
+  include HasSanitizableFields
   include ActiveSupport::Deprecation::DeprecatedConstantAccessor
   deprecate_constant 'CUSTOM_INTERPOLATION_KEYS_WHITELIST', 'TranslationOverride::ALLOWED_CUSTOM_INTERPOLATION_KEYS'
 
@@ -50,13 +51,15 @@ class TranslationOverride < ActiveRecord::Base
   def self.upsert!(locale, key, value)
     params = { locale: locale, translation_key: key }
 
-    data = { value: value }
+    translation_override = find_or_initialize_by(params)
+    sanitized_value = translation_override.sanitize_field(value, additional_attributes: ['data-auto-route'])
+
+    data = { value: sanitized_value }
     if key.end_with?('_MF')
       _, filename = JsLocaleHelper.find_message_format_locale([locale], fallback_to_english: false)
-      data[:compiled_js] = JsLocaleHelper.compile_message_format(filename, locale, value)
+      data[:compiled_js] = JsLocaleHelper.compile_message_format(filename, locale, sanitized_value)
     end
 
-    translation_override = find_or_initialize_by(params)
     params.merge!(data) if translation_override.new_record?
     i18n_changed(locale, [key]) if translation_override.update(data)
     translation_override
@@ -125,7 +128,6 @@ class TranslationOverride < ActiveRecord::Base
     if original_text
       original_interpolation_keys = I18nInterpolationKeysFinder.find(original_text)
       new_interpolation_keys = I18nInterpolationKeysFinder.find(value)
-
       custom_interpolation_keys = []
 
       ALLOWED_CUSTOM_INTERPOLATION_KEYS.select do |keys, value|

app/models/user_field.rb

@@ -3,6 +3,7 @@
 class UserField < ActiveRecord::Base
 
   include AnonCacheInvalidator
+  include HasSanitizableFields
 
   validates_presence_of :description, :field_type
   validates_presence_of :name, unless: -> { field_type == "confirm" }
@@ -10,6 +11,7 @@ class UserField < ActiveRecord::Base
   has_one :directory_column, dependent: :destroy
   accepts_nested_attributes_for :user_field_options
 
+  before_save :sanitize_description
   after_save :queue_index_search
 
   def self.max_length
@@ -19,6 +21,14 @@ class UserField < ActiveRecord::Base
   def queue_index_search
     SearchIndexer.queue_users_reindex(UserCustomField.where(name: "user_field_#{self.id}").pluck(:user_id))
   end
+
+  private
+
+  def sanitize_description
+    if description_changed?
+      self.description = sanitize_field(self.description)
+    end
+  end
 end
 
 # == Schema Information

config/locales/client.en.yml

@@ -4555,11 +4555,11 @@ en:
 
               Prefixing the property names is highly recommended to avoid conflicts with plugins and/or core.
           head_tag:
-            text: "</head>"
+            text: "Head"
-            title: "HTML that will be inserted before the </head> tag"
+            title: "HTML that will be inserted before the head tag"
           body_tag:
-            text: "</body>"
+            text: "Body"
-            title: "HTML that will be inserted before the </body> tag"
+            title: "HTML that will be inserted before the body tag"
           yaml:
             text: "YAML"
             title: "Define theme settings in YAML format"

lib/site_setting_extension.rb

@@ -2,6 +2,7 @@
 
 module SiteSettingExtension
   include SiteSettings::DeprecatedSettings
+  include HasSanitizableFields
 
   # support default_locale being set via global settings
   # this also adds support for testing the extension and global settings
@@ -362,8 +363,12 @@ module SiteSettingExtension
   def add_override!(name, val)
     old_val = current[name]
     val, type = type_supervisor.to_db_value(name, val)
-    provider.save(name, val, type)
+
-    current[name] = type_supervisor.to_rb_value(name, val)
+    sanitize_override = val.is_a?(String) && client_settings.include?(name)
+
+    sanitized_val = sanitize_override ? sanitize_field(val) : val
+    provider.save(name, sanitized_val, type)
+    current[name] = type_supervisor.to_rb_value(name, sanitized_val)
     clear_uploads_cache(name)
     notify_clients!(name) if client_settings.include? name
     clear_cache!

spec/models/badge_spec.rb

@@ -51,6 +51,15 @@ describe Badge do
     expect(b.grant_count).to eq(1)
   end
 
+  it 'sanitizes the description' do
+    xss = "<b onmouseover=alert('Wufff!')>click me!</b><script>alert('TEST');</script>"
+    badge = Fabricate(:badge)
+
+    badge.update!(description: xss)
+
+    expect(badge.description).to eq("<b>click me!</b>alert('TEST');")
+  end
+
   describe '#manually_grantable?' do
     fab!(:badge) { Fabricate(:badge, name: 'Test Badge') }
     subject { badge.manually_grantable? }

spec/models/site_setting_spec.rb

@@ -204,4 +204,22 @@ describe SiteSetting do
       expect(SiteSetting.blocked_attachment_filenames_regex).to eq(/foo|bar/)
     end
   end
+
+  it 'sanitizes the client settings when they are overridden' do
+    xss = "<b onmouseover=alert('Wufff!')>click me!</b><script>alert('TEST');</script>"
+
+    SiteSetting.global_notice = xss
+
+    expect(SiteSetting.global_notice).to eq("<b>click me!</b>alert('TEST');")
+  end
+
+  it "doesn't corrupt site settings with special characters" do
+    value = 'OX5y3Oljb+Qt9Bu809vsBQ==<>!%{}*&!@#$%..._-A'
+    settings = new_settings(SiteSettings::LocalProcessProvider.new)
+    settings.setting(:test_setting, '', client: true)
+
+    settings.test_setting = value
+
+    expect(settings.test_setting).to eq(value)
+  end
 end

spec/models/translation_override_spec.rb

@@ -115,6 +115,16 @@ describe TranslationOverride do
     expect(ovr.value).to eq('some value')
   end
 
+  it 'sanitizes values before upsert' do
+    xss = "<a href='%{url}' data-auto-route='true'>setup wizard</a> ✨<script>alert('TEST');</script>"
+
+    TranslationOverride.upsert!('en', 'js.wizard_required', xss)
+
+    ovr = TranslationOverride.where(locale: 'en', translation_key: 'js.wizard_required').first
+    expect(ovr).to be_present
+    expect(ovr.value).to eq("<a href=\"%{url}\" data-auto-route=\"true\">setup wizard</a> ✨alert('TEST');")
+  end
+
   it "stores js for a message format key" do
     TranslationOverride.upsert!('ru', 'some.key_MF', '{NUM_RESULTS, plural, one {1 result} other {many} }')
 

spec/models/user_field_spec.rb

@@ -12,4 +12,13 @@ describe UserField do
     subject { described_class.new(field_type: 'dropdown') }
     it { is_expected.to validate_presence_of :name }
   end
+
+  it 'sanitizes the description' do
+    xss = "<b onmouseover=alert('Wufff!')>click me!</b><script>alert('TEST');</script>"
+    user_field = Fabricate(:user_field)
+
+    user_field.update!(description: xss)
+
+    expect(user_field.description).to eq("<b>click me!</b>alert('TEST');")
+  end
 end