FEATURE: Do not allow inline JS in house ads (#37701)

Admin ads the script here, but the intent was never to allow JS here, we
have other patterns for adding JS we should follow.
This commit is contained in:
Sam
2026-02-12 07:48:01 +11:00
committed by GitHub
parent 3e4b4ff5c2
commit af731daa52
3 changed files with 78 additions and 4 deletions
@@ -2,6 +2,8 @@
module AdPlugin
class HouseAd < ActiveRecord::Base
include HasSanitizableFields
self.table_name = "ad_plugin_house_ads"
NAME_REGEX = /\A[[:alnum:]\s\.,'!@#$%&\*\-\+\=:]*\z/i
@@ -29,6 +31,8 @@ module AdPlugin
validates :name, presence: true, uniqueness: true, format: { with: NAME_REGEX }
validates :html, presence: true
before_save :sanitize_html
scope :for_anons, -> { where(visible_to_anons: true) }
scope :for_logged_in, -> { where(visible_to_logged_in_users: true) }
@@ -72,6 +76,10 @@ module AdPlugin
private
def sanitize_html
self.html = sanitize_field(self.html) if html_changed?
end
def clear_cache
Site.clear_anon_cache!
self.class.publish_if_ads_enabled
@@ -73,7 +73,7 @@ describe AdPlugin::HouseAdSetting do
Fabricate(
:house_ad,
name: "anon-ad",
html: "<whatever-anon>",
html: "<div>anon ad</div>",
visible_to_anons: true,
visible_to_logged_in_users: false,
)
@@ -83,7 +83,7 @@ describe AdPlugin::HouseAdSetting do
Fabricate(
:house_ad,
name: "logged-in-ad",
html: "<whatever-logged-in>",
html: "<div>logged-in ad</div>",
visible_to_anons: false,
visible_to_logged_in_users: true,
)
@@ -100,7 +100,7 @@ describe AdPlugin::HouseAdSetting do
expect(anon_message.data[:creatives]).to match(
"anon-ad" => {
html: "<whatever-anon>",
html: "<div>anon ad</div>",
category_ids: [],
id: a_kind_of(Integer),
routes: [],
@@ -111,7 +111,7 @@ describe AdPlugin::HouseAdSetting do
expect(logged_in_message.data[:creatives]).to match(
"logged-in-ad" => {
html: "<whatever-logged-in>",
html: "<div>logged-in ad</div>",
category_ids: [],
id: a_kind_of(Integer),
routes: [],
@@ -19,10 +19,76 @@ describe AdPlugin::HouseAdsController do
before { enable_current_plugin }
before { SiteSetting.ad_plugin_routes_enabled = true }
describe "#create" do
context "when used by admins" do
before { sign_in(admin) }
it "strips script tags from html on create" do
post "/admin/plugins/pluginad/house_creatives.json",
params: {
name: "XSS Ad",
html: '<div>Ad</div><script>alert("xss")</script>',
visible_to_anons: "true",
visible_to_logged_in_users: "true",
}
expect(response.status).to eq(200)
created_ad = AdPlugin::HouseAd.find_by(name: "XSS Ad")
expect(created_ad.html).not_to include("<script>")
expect(created_ad.html).to include("<div>Ad</div>")
end
it "strips event handler attributes from html on create" do
post "/admin/plugins/pluginad/house_creatives.json",
params: {
name: "Event Ad",
html:
'<img src="x" onerror="alert(1)"><a onclick="alert(1)" href="https://example.com">Click</a>',
visible_to_anons: "true",
visible_to_logged_in_users: "true",
}
expect(response.status).to eq(200)
created_ad = AdPlugin::HouseAd.find_by(name: "Event Ad")
expect(created_ad.html).not_to include("onerror")
expect(created_ad.html).not_to include("onclick")
end
end
end
describe "#update" do
context "when used by admins" do
before { sign_in(admin) }
it "strips script tags from html on update" do
put "/admin/plugins/pluginad/house_creatives/#{ad.id}.json",
params: {
name: ad.name,
html: '<div>Safe</div><script>fetch("/admin/users/1/grant_admin")</script>',
visible_to_anons: "true",
visible_to_logged_in_users: "false",
}
expect(response.status).to eq(200)
ad.reload
expect(ad.html).not_to include("<script>")
expect(ad.html).to include("<div>Safe</div>")
end
it "strips event handler attributes from html on update" do
put "/admin/plugins/pluginad/house_creatives/#{ad.id}.json",
params: {
name: ad.name,
html: '<img src="x" onerror="alert(document.cookie)">',
visible_to_anons: "true",
visible_to_logged_in_users: "false",
}
expect(response.status).to eq(200)
ad.reload
expect(ad.html).not_to include("onerror")
end
it "updates an existing ad" do
put "/admin/plugins/pluginad/house_creatives/#{ad.id}.json",
params: {