DEV: Sanitize HTML admin inputs (#14681)

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

  • Make sure that the sanitization logic doesn’t corrupt settings with special characters
diff --git a/app/models/badge.rb b/app/models/badge.rb
index 5e480aa..af0c054 100644
--- a/app/models/badge.rb
+++ b/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
diff --git a/app/models/concerns/has_sanitizable_fields.rb b/app/models/concerns/has_sanitizable_fields.rb
new file mode 100644
index 0000000..b0db07d
--- /dev/null
+++ b/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
diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb
index edd2b85..85b09c0 100644
--- a/app/models/translation_override.rb
+++ b/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|
diff --git a/app/models/user_field.rb b/app/models/user_field.rb
index b024574..e5c3caf 100644
--- a/app/models/user_field.rb
+++ b/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
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 67b081f..58e2adf 100644
--- a/config/locales/client.en.yml
+++ b/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>"
-            title: "HTML that will be inserted before the </head> tag"
+            text: "Head"
+            title: "HTML that will be inserted before the head tag"
           body_tag:
-            text: "</body>"
-            title: "HTML that will be inserted before the </body> tag"
+            text: "Body"
+            title: "HTML that will be inserted before the body tag"
           yaml:
             text: "YAML"
             title: "Define theme settings in YAML format"
diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb
index dd529e8..c86c67f 100644
--- a/lib/site_setting_extension.rb
+++ b/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!
diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb
index 55cf3b5..2125292 100644
--- a/spec/models/badge_spec.rb
+++ b/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? }
diff --git a/spec/models/site_setting_spec.rb b/spec/models/site_setting_spec.rb
index 7a81584..7d858a9 100644
--- a/spec/models/site_setting_spec.rb
+++ b/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
+

[... diff too long, it was truncated ...]

GitHub sha: df3eb939732f86edc143445f267d1dcdd2e940ff

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