From 90a3fbc07b79ea019de8cb93126db4e26eda6fde Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Mon, 4 Oct 2021 15:40:35 -0300 Subject: [PATCH] DEV: Remove HTML setting type and sanitization logic. (#14440) * DEV: Remove HTML setting type and sanitization logic. We concluded that we don't want settings to contain HTML, so I'm removing the setting type and sanitization logic. Additionally, we no longer allow the global-notice text to contain HTML. I searched for usages of this setting type in the `all-the-plugins` repo and found none, so I haven't added a migration for existing settings. * Mark Global notices containing links as HTML Safe. --- .../components/site-settings/html.hbs | 2 -- .../discourse/app/components/global-notice.js | 7 +++-- .../templates/components/global-notice.hbs | 2 +- .../admin/site_settings_controller.rb | 2 +- config/site_settings.yml | 1 - ...12_migrate_deprecated_html_setting_type.rb | 15 ++++++++++ lib/site_setting_extension.rb | 16 +--------- lib/site_settings/type_supervisor.rb | 2 +- .../components/site_setting_extension_spec.rb | 30 ------------------- .../site_settings/type_supervisor_spec.rb | 3 -- 10 files changed, 24 insertions(+), 56 deletions(-) delete mode 100644 app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs create mode 100644 db/migrate/20210928161912_migrate_deprecated_html_setting_type.rb 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 deleted file mode 100644 index 0da61a84e1f..00000000000 --- a/app/assets/javascripts/admin/addon/templates/components/site-settings/html.hbs +++ /dev/null @@ -1,2 +0,0 @@ -{{text-field value=(html-safe value) classNames="input-setting-string"}} -
{{html-safe setting.description}}
diff --git a/app/assets/javascripts/discourse/app/components/global-notice.js b/app/assets/javascripts/discourse/app/components/global-notice.js index 9dc041b8636..82936c7a59d 100644 --- a/app/assets/javascripts/discourse/app/components/global-notice.js +++ b/app/assets/javascripts/discourse/app/components/global-notice.js @@ -5,6 +5,7 @@ import I18n from "I18n"; import LogsNotice from "discourse/services/logs-notice"; import { bind } from "discourse-common/utils/decorators"; import getURL from "discourse-common/lib/get-url"; +import { htmlSafe } from "@ember/template"; const _pluginNotices = []; @@ -111,7 +112,9 @@ export default Component.extend({ const requiredText = I18n.t("wizard_required", { url: getURL("/wizard"), }); - notices.push(Notice.create({ text: requiredText, id: "alert-wizard" })); + notices.push( + Notice.create({ text: htmlSafe(requiredText), id: "alert-wizard" }) + ); } if ( @@ -214,7 +217,7 @@ export default Component.extend({ @bind _handleLogsNoticeUpdate() { const logNotice = Notice.create({ - text: LogsNotice.currentProp("message"), + text: htmlSafe(LogsNotice.currentProp("message")), id: "alert-logs-notice", options: { dismissable: true, diff --git a/app/assets/javascripts/discourse/app/templates/components/global-notice.hbs b/app/assets/javascripts/discourse/app/templates/components/global-notice.hbs index 08ff36603fc..96aee8f1771 100644 --- a/app/assets/javascripts/discourse/app/templates/components/global-notice.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/global-notice.hbs @@ -4,7 +4,7 @@ {{#if notice.options.html}} {{html-safe notice.options.html}} {{/if}} - {{html-safe notice.text}} + {{notice.text}} {{#if notice.options.dismissable}} {{d-button diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index b909adf06a7..a17f9c70de6 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -7,7 +7,7 @@ class Admin::SiteSettingsController < Admin::AdminController def index render_json_dump( - site_settings: SiteSetting.all_settings(sanitize_plain_text_settings: true), + site_settings: SiteSetting.all_settings, diags: SiteSetting.diags ) end diff --git a/config/site_settings.yml b/config/site_settings.yml index 2c98aad72f9..42f3c7ab07d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -19,7 +19,6 @@ # 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/db/migrate/20210928161912_migrate_deprecated_html_setting_type.rb b/db/migrate/20210928161912_migrate_deprecated_html_setting_type.rb new file mode 100644 index 00000000000..de64efd108d --- /dev/null +++ b/db/migrate/20210928161912_migrate_deprecated_html_setting_type.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class MigrateDeprecatedHtmlSettingType < ActiveRecord::Migration[6.1] + def up + execute <<~SQL + UPDATE site_settings + SET data_type = 1 + WHERE data_type = 25 + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 28c7a9cd45c..dd529e8e71c 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -212,16 +212,12 @@ module SiteSettingExtension 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, sanitize_plain_text_settings: false) + def all_settings(include_hidden: false) locale_setting_hash = { @@ -250,8 +246,6 @@ 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 = { @@ -582,14 +576,6 @@ 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 88d07a34740..99170c3dc0d 100644 --- a/lib/site_settings/type_supervisor.rb +++ b/lib/site_settings/type_supervisor.rb @@ -37,7 +37,7 @@ class SiteSettings::TypeSupervisor color: 22, simple_list: 23, emoji_list: 24, - html: 25 + html_deprecated: 25, ) end diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index c34588725f1..5be49b0ff03 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -831,23 +831,6 @@ 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, '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, 'rest', type: :html) - - setting = settings.all_settings(sanitize_plain_text_settings: true).last - - expect(setting[:value]).to eq('rest') - end end describe '.client_settings_json_uncached' do @@ -862,19 +845,6 @@ describe SiteSettingExtension do ) end - it 'should sanitize html in the site settings' do - settings.setting(:with_html, 'rest', client: true) - settings.setting(:with_symbols, '<>rest', client: true) - settings.setting(:with_unknown_tag, '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, 'rest', type: :html, client: true) diff --git a/spec/components/site_settings/type_supervisor_spec.rb b/spec/components/site_settings/type_supervisor_spec.rb index f565b86eb05..36c6f82a197 100644 --- a/spec/components/site_settings/type_supervisor_spec.rb +++ b/spec/components/site_settings/type_supervisor_spec.rb @@ -94,9 +94,6 @@ 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