From df3eb939732f86edc143445f267d1dcdd2e940ff Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 27 Oct 2021 11:33:07 -0300 Subject: [PATCH] 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](https://github.com/rails/rails-html-sanitizer/blob/018cf540737ae10ee1c673ce184408881852c479/lib/rails/html/sanitizer.rb#L108) * Make sure that the sanitization logic doesn't corrupt settings with special characters --- app/models/badge.rb | 8 +++++++ app/models/concerns/has_sanitizable_fields.rb | 23 +++++++++++++++++++ app/models/translation_override.rb | 10 ++++---- app/models/user_field.rb | 10 ++++++++ config/locales/client.en.yml | 8 +++---- lib/site_setting_extension.rb | 9 ++++++-- spec/models/badge_spec.rb | 9 ++++++++ spec/models/site_setting_spec.rb | 18 +++++++++++++++ spec/models/translation_override_spec.rb | 10 ++++++++ spec/models/user_field_spec.rb | 9 ++++++++ 10 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 app/models/concerns/has_sanitizable_fields.rb diff --git a/app/models/badge.rb b/app/models/badge.rb index 5e480aa2e57..af0c054d66a 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 00000000000..b0db07de00e --- /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 edd2b85084a..85b09c0e74a 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 b024574894c..e5c3caf86d3 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 67b081f45cb..58e2adf1e3e 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: "" - title: "HTML that will be inserted before the tag" + text: "Head" + title: "HTML that will be inserted before the head tag" body_tag: - text: "" - title: "HTML that will be inserted before the 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 dd529e8e71c..c86c67ff247 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 55cf3b51485..2125292364b 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 = "click me!" + badge = Fabricate(:badge) + + badge.update!(description: xss) + + expect(badge.description).to eq("click me!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 7a815848787..7d858a951df 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 = "click me!" + + SiteSetting.global_notice = xss + + expect(SiteSetting.global_notice).to eq("click me!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 diff --git a/spec/models/translation_override_spec.rb b/spec/models/translation_override_spec.rb index dddb037ba1f..9a05f30c28c 100644 --- a/spec/models/translation_override_spec.rb +++ b/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 = "setup wizard ✨" + + 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("setup wizard ✨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} }') diff --git a/spec/models/user_field_spec.rb b/spec/models/user_field_spec.rb index 7f545b3706d..e7385911463 100644 --- a/spec/models/user_field_spec.rb +++ b/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 = "click me!" + user_field = Fabricate(:user_field) + + user_field.update!(description: xss) + + expect(user_field.description).to eq("click me!alert('TEST');") + end end