DEV: Remove HTML setting type and sanitization logic. (#14440)

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.
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 0da61a8..0000000
--- 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"}}
-<div class="desc">{{html-safe setting.description}}</div>
diff --git a/app/assets/javascripts/discourse/app/components/global-notice.js b/app/assets/javascripts/discourse/app/components/global-notice.js
index 9dc041b..82936c7 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 08ff366..96aee8f 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}}
-      <span class="text">{{html-safe notice.text}}</span>
+      <span class="text">{{notice.text}}</span>
 
       {{#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 b909adf..a17f9c7 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 2c98aad..42f3c7a 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 0000000..de64efd
--- /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 28c7a9c..dd529e8 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 88d07a3..99170c3 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 c345887..5be49b0 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, '<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
@@ -862,19 +845,6 @@ describe SiteSettingExtension do
       )
     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)
 
diff --git a/spec/components/site_settings/type_supervisor_spec.rb b/spec/components/site_settings/type_supervisor_spec.rb
index f565b86..36c6f82 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

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

GitHub sha: 90a3fbc07b79ea019de8cb93126db4e26eda6fde

This commit appears in #14440 which was approved by CvX and tgxworld. It was merged by romanrizzi.

This commit has been mentioned on Discourse Meta. There might be relevant details there: