Merge pull request #12943 from mkllnk/sanitise

Sanitise HTML attributes in the database

app/models/custom_tab.rb

@@ -5,11 +5,6 @@ class CustomTab < ApplicationRecord
 
   validates :title, presence: true, length: { maximum: 20 }
 
-  # Remove any unsupported HTML.
-  def content
-    HtmlSanitizer.sanitize(super)
-  end
-
   # Remove any unsupported HTML.
   def content=(html)
     super(HtmlSanitizer.sanitize(html))

app/models/enterprise_group.rb

@@ -74,11 +74,6 @@ class EnterpriseGroup < ApplicationRecord
     permalink
   end
 
-  # Remove any unsupported HTML.
-  def long_description
-    HtmlSanitizer.sanitize_and_enforce_link_target_blank(super)
-  end
-
   # Remove any unsupported HTML.
   def long_description=(html)
     super(HtmlSanitizer.sanitize_and_enforce_link_target_blank(html))

app/models/spree/product.rb

@@ -279,11 +279,6 @@ module Spree
     end
     # rubocop:enable Metrics/AbcSize
 
-    # Remove any unsupported HTML.
-    def description
-      HtmlSanitizer.sanitize(super)
-    end
-
     # Remove any unsupported HTML.
     def description=(html)
       super(HtmlSanitizer.sanitize(html))

db/migrate/20241023054951_sanitize_html_attributes.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+class SanitizeHtmlAttributes < ActiveRecord::Migration[7.0]
+  class CustomTab < ApplicationRecord
+  end
+
+  class EnterpriseGroup < ApplicationRecord
+  end
+
+  class SpreeProduct < ApplicationRecord
+  end
+
+  # This is a copy from our application code at the time of writing.
+  # We prefer to keep migrations isolated and not affected by changing
+  # application code in the future.
+  # If we need to change the sanitizer in the future we may need a new
+  # migration (not change the old one) to sanitise the data properly.
+  class HtmlSanitizer
+    ALLOWED_TAGS = %w[h1 h2 h3 h4 div p br b i u a strong em del pre blockquote ul ol li hr
+                      figure].freeze
+    ALLOWED_ATTRIBUTES = %w[href target].freeze
+    ALLOWED_TRIX_DATA_ATTRIBUTES = %w[data-trix-attachment].freeze
+
+    def self.sanitize(html)
+      @sanitizer ||= Rails::HTML5::SafeListSanitizer.new
+      @sanitizer.sanitize(
+        html, tags: ALLOWED_TAGS, attributes: (ALLOWED_ATTRIBUTES + ALLOWED_TRIX_DATA_ATTRIBUTES)
+      )
+    end
+
+    def self.sanitize_and_enforce_link_target_blank(html)
+      sanitize(enforce_link_target_blank(html))
+    end
+
+    def self.enforce_link_target_blank(html)
+      return if html.nil?
+
+      Nokogiri::HTML::DocumentFragment.parse(html).tap do |document|
+        document.css("a").each { |link| link["target"] = "_blank" }
+      end.to_s
+    end
+  end
+
+  def up
+    CustomTab.where.not(content: [nil, ""]).find_each do |row|
+      sane = HtmlSanitizer.sanitize(row.content)
+      row.update_column(:content, sane)
+    end
+    EnterpriseGroup.where.not(long_description: [nil, ""]).find_each do |row|
+      sane = HtmlSanitizer.sanitize_and_enforce_link_target_blank(row.long_description)
+      row.update_column(:long_description, sane)
+    end
+    SpreeProduct.where.not(description: [nil, ""]).find_each do |row|
+      sane = HtmlSanitizer.sanitize(row.description)
+      row.update_column(:description, sane)
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2024_10_02_014059) do
+ActiveRecord::Schema[7.0].define(version: 2024_10_23_054951) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_stat_statements"
   enable_extension "plpgsql"

spec/migrations/20241023054951_sanitize_html_attributes_spec.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../db/migrate/20241023054951_sanitize_html_attributes'
+
+RSpec.describe SanitizeHtmlAttributes do
+  describe "#up" do
+    # Let's hack some bad data:
+    let!(:tab) {
+      create(:custom_tab).tap do |row|
+        row.update_columns(content: bad_html)
+      end
+    }
+    let!(:enterprise_group) {
+      create(:enterprise_group).tap do |row|
+        row.update_columns(long_description: bad_html)
+      end
+    }
+    let!(:product) {
+      create(:product).tap do |row|
+        row.update_columns(description: bad_html)
+      end
+    }
+    let(:bad_html) {
+      <<~HTML.squish
+        <p data-controller="load->payMe">Fred Farmer is a certified
+        <a href="https://example.net/">organic</a>
+        <script>alert("Gotcha!")</script>...</p>
+      HTML
+    }
+    let(:good_html) {
+      <<~HTML.squish
+        <p>Fred Farmer is a certified
+        <a href="https://example.net/">organic</a>
+        alert("Gotcha!")...</p>
+      HTML
+    }
+    let(:good_html_external_link) {
+      <<~HTML.squish
+        <p>Fred Farmer is a certified
+        <a href="https://example.net/" target="_blank">organic</a>
+        alert("Gotcha!")...</p>
+      HTML
+    }
+
+    it "sanitises HTML attributes" do
+      expect { subject.up }.to change {
+        tab.reload.attributes["content"]
+      }
+        .from(bad_html).to(good_html)
+        .and change {
+               enterprise_group.reload.attributes["long_description"]
+             }
+        .from(bad_html).to(good_html_external_link)
+        .and change {
+               product.reload.attributes["description"]
+             }
+        .from(bad_html).to(good_html)
+    end
+  end
+end

spec/models/custom_tab_spec.rb

@@ -18,10 +18,5 @@ RSpec.describe CustomTab do
       subject.content = "Hello <script>alert</script> dearest <b>monster</b>."
       expect(subject.content).to eq "Hello alert dearest <b>monster</b>."
     end
-
-    it "sanitises existing HTML in content" do
-      subject[:content] = "Hello <script>alert</script> dearest <b>monster</b>."
-      expect(subject.content).to eq "Hello alert dearest <b>monster</b>."
-    end
   end
 end

spec/models/enterprise_group_spec.rb

@@ -124,10 +124,5 @@ RSpec.describe EnterpriseGroup do
       subject.long_description = "Hello <script>alert</script> dearest <b>monster</b>."
       expect(subject.long_description).to eq "Hello alert dearest <b>monster</b>."
     end
-
-    it "sanitises existing HTML in long_description" do
-      subject[:long_description] = "Hello <script>alert</script> dearest <b>monster</b>."
-      expect(subject.long_description).to eq "Hello alert dearest <b>monster</b>."
-    end
   end
 end

spec/models/spree/product_spec.rb

@@ -707,11 +707,6 @@ module Spree
         subject.description = "Hello <script>alert</script> dearest <b>monster</b>."
         expect(subject.description).to eq "Hello alert dearest <b>monster</b>."
       end
-
-      it "sanitises existing HTML in description" do
-        subject[:description] = "Hello <script>alert</script> dearest <b>monster</b>."
-        expect(subject.description).to eq "Hello alert dearest <b>monster</b>."
-      end
     end
   end