From a7dc243db9871cc9791fea40b8f85e492fe585df Mon Sep 17 00:00:00 2001 From: Ana Nunes da Silva Date: Fri, 24 May 2024 12:50:57 +0100 Subject: [PATCH 1/4] Sanitize product description using rails default sanitizer --- app/models/spree/product.rb | 10 ++++++++++ spec/models/spree/product_spec.rb | 12 ++++++++++++ 2 files changed, 22 insertions(+) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index d760c329ec..e51f23ac7c 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -304,6 +304,16 @@ module Spree ) end + # Remove any unsupported HTML. + def description + Rails::HTML::SafeListSanitizer.new.sanitize(super) + end + + # # Remove any unsupported HTML. + def description=(html) + super(Rails::HTML::SafeListSanitizer.new.sanitize(html)) + end + private def update_units diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 15667125c3..993b1cb833 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -748,6 +748,18 @@ module Spree expect(e.variants.reload).to be_empty end end + + describe "serialisation" do + it "sanitises HTML in description" do + subject.description = "Hello dearest monster." + expect(subject.description).to eq "Hello alert dearest monster." + end + + it "sanitises existing HTML in description" do + subject[:description] = "Hello dearest monster." + expect(subject.description).to eq "Hello alert dearest monster." + end + end end RSpec.describe "product import" do From 5f54ea3877acc06b5d6fbbba38f45acf4f879a47 Mon Sep 17 00:00:00 2001 From: Ana Nunes da Silva Date: Fri, 31 May 2024 17:51:52 +0100 Subject: [PATCH 2/4] Add safe trix tags to html sanitizer; Use custom html sanitizer in product description. --- app/models/spree/product.rb | 6 +- app/services/html_sanitizer.rb | 6 +- spec/services/html_sanitizer_spec.rb | 94 +++++++++++++++++++++------- 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index e51f23ac7c..1f79ebf95c 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -306,12 +306,12 @@ module Spree # Remove any unsupported HTML. def description - Rails::HTML::SafeListSanitizer.new.sanitize(super) + HtmlSanitizer.sanitize(super) end - # # Remove any unsupported HTML. + # Remove any unsupported HTML. def description=(html) - super(Rails::HTML::SafeListSanitizer.new.sanitize(html)) + super(HtmlSanitizer.sanitize(html)) end private diff --git a/app/services/html_sanitizer.rb b/app/services/html_sanitizer.rb index df3c608219..e6d81d7ecd 100644 --- a/app/services/html_sanitizer.rb +++ b/app/services/html_sanitizer.rb @@ -6,10 +6,14 @@ # We offer an editor which supports certain tags but you can't insert just any # HTML, which would be dangerous. class HtmlSanitizer + ALLOWED_TAGS = %w[h1 h2 h3 h4 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: %w[h1 h2 h3 h4 p br b i u a], attributes: %w[href target], + html, tags: ALLOWED_TAGS, attributes: (ALLOWED_ATTRIBUTES + ALLOWED_TRIX_DATA_ATTRIBUTES) ) end end diff --git a/spec/services/html_sanitizer_spec.rb b/spec/services/html_sanitizer_spec.rb index c98d695b35..090d4bd3e9 100644 --- a/spec/services/html_sanitizer_spec.rb +++ b/spec/services/html_sanitizer_spec.rb @@ -5,33 +5,85 @@ require 'spec_helper' RSpec.describe HtmlSanitizer do subject { described_class } - it "removes dangerous tags" do - html = "Hello !" - expect(subject.sanitize(html)) - .to eq "Hello alert!" + context "when HTML has supported tags" do + it "keeps supported tags" do + html = "Hello alert!
How are you?" + expect(subject.sanitize(html)) + .to eq "Hello alert!
How are you?" + end + + it "handles nested tags" do + html = '' + expect(subject.sanitize(html)).to eq(html) + end end - it "keeps supported tags" do - html = "Hello alert!
How are you?" - expect(subject.sanitize(html)) - .to eq "Hello alert!
How are you?" + context "when HTML has dangerous tags" do + it "removes script tags" do + html = "Hello !" + expect(subject.sanitize(html)).to eq "Hello alert!" + end + + it "removes iframe tags" do + html = "Content " + expect(subject.sanitize(html)).to eq "Content " + end + + it "removes object tags" do + html = "" + expect(subject.sanitize(html)).to eq "" + end + + it "removes embed tags" do + html = "" + expect(subject.sanitize(html)).to eq "" + end + + it "removes link tags" do + html = "" + expect(subject.sanitize(html)).to eq "" + end + + it "removes link tags" do + html = "" + expect(subject.sanitize(html)).to eq "" + end + + it "removes form tags" do + html = "
...
" + expect(subject.sanitize(html)).to eq "..." + end + + it "removes combined dangerous tags" do + html = "" + expect(subject.sanitize(html)).to eq "alert" + end end - it "keeps supported attributes" do - html = 'Hello alert!' - expect(subject.sanitize(html)) - .to eq 'Hello alert!' + context "when HTML has supported attributes" do + it "keeps supported attributes" do + html = 'Hello alert!' + expect(subject.sanitize(html)) + .to eq 'Hello alert!' + end end - it "removes unsupported attributes" do - html = 'Hello alert!' - expect(subject.sanitize(html)) - .to eq 'Hello alert!' - end + context "when HTML has dangerous attributes" do + it "removes unsupported attributes" do + html = 'Hello alert!' + expect(subject.sanitize(html)) + .to eq 'Hello alert!' + end - it "removes dangerous attribute values" do - html = 'Hello you!' - expect(subject.sanitize(html)) - .to eq 'Hello you!' + it "removes dangerous attribute values" do + html = 'Hello you!' + expect(subject.sanitize(html)) + .to eq 'Hello you!' + end + + it "keeps only Trix-specific data attributes" do + html = '
...
' + expect(subject.sanitize(html)).to eq('
...
') + end end end From 205c7dafd2ca500af3e7887da5ec0c1b9738fe80 Mon Sep 17 00:00:00 2001 From: Ana Nunes da Silva Date: Mon, 3 Jun 2024 11:35:25 +0100 Subject: [PATCH 3/4] Add div to sanitizer supported tags --- app/services/html_sanitizer.rb | 3 ++- spec/services/html_sanitizer_spec.rb | 24 ++++++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/app/services/html_sanitizer.rb b/app/services/html_sanitizer.rb index e6d81d7ecd..73660d8829 100644 --- a/app/services/html_sanitizer.rb +++ b/app/services/html_sanitizer.rb @@ -6,7 +6,8 @@ # We offer an editor which supports certain tags but you can't insert just any # HTML, which would be dangerous. class HtmlSanitizer - ALLOWED_TAGS = %w[h1 h2 h3 h4 p br b i u a strong em del pre blockquote ul ol li hr figure].freeze + 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 diff --git a/spec/services/html_sanitizer_spec.rb b/spec/services/html_sanitizer_spec.rb index 090d4bd3e9..bda9eb2188 100644 --- a/spec/services/html_sanitizer_spec.rb +++ b/spec/services/html_sanitizer_spec.rb @@ -6,14 +6,26 @@ RSpec.describe HtmlSanitizer do subject { described_class } context "when HTML has supported tags" do - it "keeps supported tags" do - html = "Hello alert!
How are you?" - expect(subject.sanitize(html)) - .to eq "Hello alert!
How are you?" + it "keeps supported regular tags" do + supported_tags = %w[h1 h2 h3 h4 div p b i u a strong em del pre blockquote ul ol li figure] + supported_tags.each do |tag| + html = "<#{tag}>Content" + sanitized_html = subject.sanitize(html) + expect(sanitized_html).to eq(html), "Expected '#{tag}' to be preserved." + end + end + + it "keeps supported void tags" do + supported_tags = %w[br hr] + supported_tags.each do |tag| + html = "<#{tag}>" + sanitized_html = subject.sanitize(html) + expect(sanitized_html).to eq(html), "Expected '#{tag}' to be preserved." + end end it "handles nested tags" do - html = '
  • Item 1
  • Item 2
' + html = '
  • Item 1
  • Item 2
' expect(subject.sanitize(html)).to eq(html) end end @@ -44,7 +56,7 @@ RSpec.describe HtmlSanitizer do expect(subject.sanitize(html)).to eq "" end - it "removes link tags" do + it "removes base tags" do html = "" expect(subject.sanitize(html)).to eq "" end From 5a8eea398ef6c8724b265329adb2427f9fd95100 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 4 Jun 2024 10:24:40 +1000 Subject: [PATCH 4/4] Add comment --- app/services/html_sanitizer.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/html_sanitizer.rb b/app/services/html_sanitizer.rb index 73660d8829..84d78f6f5c 100644 --- a/app/services/html_sanitizer.rb +++ b/app/services/html_sanitizer.rb @@ -6,6 +6,7 @@ # We offer an editor which supports certain tags but you can't insert just any # HTML, which would be dangerous. class HtmlSanitizer + # div is required by Trix editor 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