Merge pull request #12459 from mkllnk/description-html

Sanitise HTML in long description of enterprise

app/models/enterprise.rb

@@ -247,11 +247,6 @@ def self.distinct_count
     count(distinct: true)
   end
 
-  # Remove any unsupported HTML.
-  def long_description
-    HtmlSanitizer.sanitize(super)
-  end
-
   # Remove any unsupported HTML.
   def long_description=(html)
     super(HtmlSanitizer.sanitize(html))

db/migrate/20240510033206_sanitize_enterprise_long_description.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class SanitizeEnterpriseLongDescription < ActiveRecord::Migration[7.0]
+  class Enterprise < 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
+  end
+
+  def up
+    Enterprise.where.not(long_description: [nil, ""]).find_each do |enterprise|
+      long_description = HtmlSanitizer.sanitize(enterprise.long_description)
+      enterprise.update!(long_description:)
+    end
+  end
+
+  private
+
+  def sanitize(html)
+    HtmlSanitizer.sanitize(html)
+  end
+end

spec/migrations/20240510033206_sanitize_enterprise_long_description_spec.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../db/migrate/20240510033206_sanitize_enterprise_long_description'
+
+RSpec.describe SanitizeEnterpriseLongDescription do
+  describe "#up" do
+    let!(:enterprise_nil_desc) { create(:enterprise, long_description: nil) }
+    let!(:enterprise_empty_desc) { create(:enterprise, long_description: "") }
+    let!(:enterprise_normal) { create(:enterprise, long_description: normal_desc) }
+    let!(:enterprise_bad) {
+      # The attribute is sanitised at assignment. So we need to inject into the
+      # database differently:
+      create(:enterprise).tap do |enterprise|
+        enterprise.update_columns(long_description: bad_desc)
+      end
+    }
+    let(:normal_desc) {
+      <<~HTML.squish
+        <p>𝐂𝐎̛𝐌 𝐓𝐀̂́𝐌 𝐂𝐇𝐔́ 𝐁𝐄́ is now available in Melbourne, everyone. 😂<br>
+        <>>> The story is this is a person who loves to eat...</p>
+      HTML
+    }
+    let(:normal_desc_sanitised) {
+      <<~HTML.squish
+        <p>𝐂𝐎̛𝐌 𝐓𝐀̂́𝐌 𝐂𝐇𝐔́ 𝐁𝐄́ is now available in Melbourne, everyone. 😂<br>
+        &lt;&gt;&gt;&gt; The story is this is a person who loves to eat...</p>
+      HTML
+    }
+    let(:bad_desc) {
+      <<~HTML.squish
+        <p data-controller="load->payMe">Fred Farmer is a certified organic
+        <script>alert("Gotcha!")</script>...</p>
+      HTML
+    }
+    let(:bad_desc_sanitised) {
+      "<p>Fred Farmer is a certified organic alert(\"Gotcha!\")...</p>"
+    }
+
+    it "sanitises the long description" do
+      expect { subject.up }.to change {
+        enterprise_bad.reload.attributes["long_description"]
+      }.from(bad_desc).to(bad_desc_sanitised)
+
+      expect(enterprise_nil_desc.reload.long_description).to eq nil
+      expect(enterprise_empty_desc.reload.long_description).to eq ""
+      expect(enterprise_normal.reload.attributes["long_description"])
+        .to eq normal_desc_sanitised
+    end
+  end
+end

spec/models/enterprise_spec.rb

@@ -404,11 +404,6 @@
       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
 
   describe "callbacks" do