DEV: improve approach to removing JS from house ads (#37739)

We only really intended to remove JS so people use other patterns
this was not a security feature

plugins/discourse-adplugin/app/models/ad_plugin/house_ad.rb

@@ -2,12 +2,16 @@
 
 module AdPlugin
   class HouseAd < ActiveRecord::Base
-    include HasSanitizableFields
-
     self.table_name = "ad_plugin_house_ads"
 
     NAME_REGEX = /\A[[:alnum:]\s\.,'!@#$%&\*\-\+\=:]*\z/i
 
+    URI_ATTRIBUTES =
+      Set.new(%w[action cite data formaction href longdesc poster src xlink:href]).freeze
+    DANGEROUS_TAGS = Set.new(%w[script noscript base]).freeze
+    JAVASCRIPT_PROTOCOL_RE = /\Ajavascript:/i
+    PROTOCOL_SEPARATOR_RE = /[\x00-\x20\x7f-\xa0]+/n
+
     has_many :impressions,
              class_name: "AdPlugin::AdImpression",
              foreign_key: "ad_plugin_house_ad_id",
@@ -76,8 +80,33 @@ module AdPlugin
 
     private
 
+    # Hygiene: strip JS from house ads so admins use proper patterns for scripting.
+    # This is not a security boundary — admins can already inject JS via themes
+    # and components, and CSP blocks inline JS anyway.
     def sanitize_html
-      self.html = sanitize_field(self.html) if html_changed?
+      return unless html_changed?
+
+      fragment = Loofah.html5_fragment(self.html)
+
+      scrubber =
+        Loofah::Scrubber.new do |node|
+          if DANGEROUS_TAGS.include?(node.name)
+            node.remove
+            next
+          end
+
+          node.attribute_nodes.each do |attr|
+            if attr.name.start_with?("on")
+              attr.remove
+            elsif URI_ATTRIBUTES.include?(attr.name)
+              cleaned = attr.value.gsub(PROTOCOL_SEPARATOR_RE, "")
+              attr.remove if cleaned.match?(JAVASCRIPT_PROTOCOL_RE)
+            end
+          end
+        end
+
+      fragment.scrub!(scrubber)
+      self.html = fragment.to_html
     end
 
     def clear_cache

plugins/discourse-adplugin/spec/models/house_ad_setting_spec.rb

@@ -73,7 +73,7 @@ describe AdPlugin::HouseAdSetting do
       Fabricate(
         :house_ad,
         name: "anon-ad",
-        html: "<div>anon ad</div>",
+        html: '<div id="anon-banner" style="color: red" data-campaign="spring">anon ad</div>',
         visible_to_anons: true,
         visible_to_logged_in_users: false,
       )
@@ -83,7 +83,8 @@ describe AdPlugin::HouseAdSetting do
       Fabricate(
         :house_ad,
         name: "logged-in-ad",
-        html: "<div>logged-in ad</div>",
+        html:
+          '<section id="logged-in-banner" data-track="true"><a href="https://example.com" target="_blank" rel="noopener">logged-in ad</a></section>',
         visible_to_anons: false,
         visible_to_logged_in_users: true,
       )
@@ -100,7 +101,7 @@ describe AdPlugin::HouseAdSetting do
 
       expect(anon_message.data[:creatives]).to match(
         "anon-ad" => {
-          html: "<div>anon ad</div>",
+          html: '<div id="anon-banner" style="color: red" data-campaign="spring">anon ad</div>',
           category_ids: [],
           id: a_kind_of(Integer),
           routes: [],
@@ -111,7 +112,8 @@ describe AdPlugin::HouseAdSetting do
 
       expect(logged_in_message.data[:creatives]).to match(
         "logged-in-ad" => {
-          html: "<div>logged-in ad</div>",
+          html:
+            '<section id="logged-in-banner" data-track="true"><a href="https://example.com" target="_blank" rel="noopener">logged-in ad</a></section>',
           category_ids: [],
           id: a_kind_of(Integer),
           routes: [],

plugins/discourse-adplugin/spec/models/house_ad_spec.rb

@@ -191,6 +191,143 @@ describe AdPlugin::HouseAd do
     end
   end
 
+  describe "sanitize_html" do
+    it "removes script tags" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: "<div>Hello</div><script>alert(1)</script>"),
+        )
+      expect(ad.html).to eq("<div>Hello</div>")
+    end
+
+    it "removes noscript tags" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<div>Hello</div><noscript><img src="x"></noscript>'),
+        )
+      expect(ad.html).to eq("<div>Hello</div>")
+    end
+
+    it "removes base tags" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<base href="https://evil.com"><div>Hello</div>'),
+        )
+      expect(ad.html).to eq("<div>Hello</div>")
+    end
+
+    it "removes on* event handler attributes" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(
+            html:
+              '<img src="x.png" onerror="alert(1)"><a onclick="alert(1)" href="https://example.com">Click</a>',
+          ),
+        )
+      expect(ad.html).not_to include("onerror")
+      expect(ad.html).not_to include("onclick")
+      expect(ad.html).to include('href="https://example.com"')
+      expect(ad.html).to include('src="x.png"')
+    end
+
+    it "removes javascript: protocol in href" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<a href="javascript:alert(1)">Click</a>'),
+        )
+      expect(ad.html).not_to include("javascript:")
+    end
+
+    it "removes javascript: protocol with mixed case" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<a href="JaVaScRiPt:alert(1)">Click</a>'),
+        )
+      expect(ad.html).not_to include("JaVaScRiPt:")
+    end
+
+    it "removes javascript: protocol with control character evasion" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: "<a href=\"java\tscript:alert(1)\">Click</a>"),
+        )
+      expect(ad.html).not_to include("javascript:")
+    end
+
+    it "removes javascript: protocol in src attributes" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<iframe src="javascript:alert(1)"></iframe>'),
+        )
+      expect(ad.html).not_to include("javascript:")
+    end
+
+    it "preserves id attributes" do
+      ad = AdPlugin::HouseAd.create!(valid_attrs.merge(html: '<div id="my-ad">Hello</div>'))
+      expect(ad.html).to include('id="my-ad"')
+    end
+
+    it "preserves data-* attributes" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<div data-campaign="spring">Hello</div>'),
+        )
+      expect(ad.html).to include('data-campaign="spring"')
+    end
+
+    it "preserves style attributes" do
+      ad = AdPlugin::HouseAd.create!(valid_attrs.merge(html: '<div style="color: red">Hello</div>'))
+      expect(ad.html).to include('style="color: red"')
+    end
+
+    it "preserves target and rel attributes" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(
+            html: '<a href="https://example.com" target="_blank" rel="noopener">Link</a>',
+          ),
+        )
+      expect(ad.html).to include('target="_blank"')
+      expect(ad.html).to include('rel="noopener"')
+    end
+
+    it "preserves table elements" do
+      html =
+        "<table><thead><tr><th>Header</th></tr></thead><tbody><tr><td>Cell</td></tr></tbody></table>"
+      ad = AdPlugin::HouseAd.create!(valid_attrs.merge(html: html))
+      expect(ad.html).to include("<table>")
+      expect(ad.html).to include("<th>Header</th>")
+      expect(ad.html).to include("<td>Cell</td>")
+    end
+
+    it "preserves iframe with non-JS src" do
+      ad =
+        AdPlugin::HouseAd.create!(
+          valid_attrs.merge(html: '<iframe src="https://example.com/embed" width="100%"></iframe>'),
+        )
+      expect(ad.html).to include("<iframe")
+      expect(ad.html).to include('src="https://example.com/embed"')
+    end
+
+    it "preserves video/audio/source elements" do
+      html = '<video controls><source src="video.mp4" type="video/mp4"></video>'
+      ad = AdPlugin::HouseAd.create!(valid_attrs.merge(html: html))
+      expect(ad.html).to include("<video")
+      expect(ad.html).to include("<source")
+    end
+
+    it "preserves semantic elements" do
+      html =
+        "<section><header><nav>Menu</nav></header><article><footer>Footer</footer></article></section>"
+      ad = AdPlugin::HouseAd.create!(valid_attrs.merge(html: html))
+      expect(ad.html).to include("<section>")
+      expect(ad.html).to include("<header>")
+      expect(ad.html).to include("<nav>")
+      expect(ad.html).to include("<article>")
+      expect(ad.html).to include("<footer>")
+    end
+  end
+
   describe "routes" do
     let(:ad) { AdPlugin::HouseAd.create(valid_attrs) }