From 72a65426a0489e5bb87b64050d6f1c1d9796b95c Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 16 Nov 2020 16:05:43 -0800 Subject: [PATCH 1/7] allow '&' and spaces --- app/serializers/api/product_serializer.rb | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 9223a797d8..1d1443ede4 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -16,16 +16,30 @@ class Api::ProductSerializer < ActiveModel::Serializer has_many :images, serializer: Api::ImageSerializer has_one :supplier, serializer: Api::IdSerializer + ALLOWED_CHARACTERS = { + "&" => "&", + " " => " " + }.freeze + # return an unformatted descripton def description - strip_tags object.description&.strip + return unless d = strip_tags(object.description&.strip) + + ALLOWED_CHARACTERS.each do |character, sub| + d = d.gsub(character, sub) + end + d end # return a sanitized html description def description_html d = sanitize(object.description, tags: ["p", "b", "strong", "em", "i", "a", "u"], attributes: ["href", "target"]) - d.to_s.html_safe + d = d.to_s.html_safe + ALLOWED_CHARACTERS.each do |character, sub| + d = d.gsub(character, sub) + end + d end def properties_with_values From 59527ab38aa1d9daac8fe3cb5b2be20242aa141b Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Fri, 8 Jan 2021 13:36:17 -0800 Subject: [PATCH 2/7] refactor filter to a service --- app/serializers/api/product_serializer.rb | 21 +++++---------------- app/services/product_description_filter.rb | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 16 deletions(-) create mode 100644 app/services/product_description_filter.rb diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 1d1443ede4..e22edaf9e1 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -16,30 +16,19 @@ class Api::ProductSerializer < ActiveModel::Serializer has_many :images, serializer: Api::ImageSerializer has_one :supplier, serializer: Api::IdSerializer - ALLOWED_CHARACTERS = { - "&" => "&", - " " => " " - }.freeze - # return an unformatted descripton def description return unless d = strip_tags(object.description&.strip) - ALLOWED_CHARACTERS.each do |character, sub| - d = d.gsub(character, sub) - end - d + ProductDescriptionFilter.filter(d) end # return a sanitized html description def description_html - d = sanitize(object.description, tags: ["p", "b", "strong", "em", "i", "a", "u"], - attributes: ["href", "target"]) - d = d.to_s.html_safe - ALLOWED_CHARACTERS.each do |character, sub| - d = d.gsub(character, sub) - end - d + ProductDescriptionFilter.filter( + sanitize(object.description, tags: ["p", "b", "strong", "em", "i", "a", "u"], + attributes: ["href", "target"]).to_s.html_safe + ) end def properties_with_values diff --git a/app/services/product_description_filter.rb b/app/services/product_description_filter.rb new file mode 100644 index 0000000000..da66ae23cc --- /dev/null +++ b/app/services/product_description_filter.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class ProductDescriptionFilter + FILTERED_CHARACTERS = { + "&amp;" => "&", + "&" => "&", + " " => " " + }.freeze + + def self.filter(descripton) + FILTERED_CHARACTERS.each do |character, sub| + descripton = descripton.gsub(character, sub) + end + descripton + end +end From b7ecf4791a9d633079fd6a194f7a68a6bd0ea584 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 9 Jan 2021 01:07:11 +0000 Subject: [PATCH 3/7] Extract more sanitizing logic from Api::ProductSerializer and make service more generic/re-usable. --- app/serializers/api/product_serializer.rb | 17 +++++----- app/services/content_sanitizer.rb | 36 ++++++++++++++++++++++ app/services/product_description_filter.rb | 16 ---------- 3 files changed, 44 insertions(+), 25 deletions(-) create mode 100644 app/services/content_sanitizer.rb delete mode 100644 app/services/product_description_filter.rb diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index e22edaf9e1..33c8ba4430 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -1,8 +1,6 @@ require "open_food_network/scope_variant_to_hub" class Api::ProductSerializer < ActiveModel::Serializer - include ActionView::Helpers::SanitizeHelper - attributes :id, :name, :permalink, :meta_keywords attributes :group_buy, :notes, :description, :description_html attributes :properties_with_values, :price @@ -18,17 +16,12 @@ class Api::ProductSerializer < ActiveModel::Serializer # return an unformatted descripton def description - return unless d = strip_tags(object.description&.strip) - - ProductDescriptionFilter.filter(d) + sanitizer.strip_content(object.description) end # return a sanitized html description def description_html - ProductDescriptionFilter.filter( - sanitize(object.description, tags: ["p", "b", "strong", "em", "i", "a", "u"], - attributes: ["href", "target"]).to_s.html_safe - ) + sanitizer.sanitize_content(object.description).html_safe end def properties_with_values @@ -50,4 +43,10 @@ class Api::ProductSerializer < ActiveModel::Serializer object.master.price_with_fees(options[:current_distributor], options[:current_order_cycle]) end end + + private + + def sanitizer + @sanitizer ||= ContentSanitizer.new + end end diff --git a/app/services/content_sanitizer.rb b/app/services/content_sanitizer.rb new file mode 100644 index 0000000000..4b2c7d3046 --- /dev/null +++ b/app/services/content_sanitizer.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# Sanitizes and cleans up user-provided content that may contain tags, special characters, etc. + +class ContentSanitizer + include ActionView::Helpers::SanitizeHelper + + ALLOWED_TAGS = ["p", "b", "strong", "em", "i", "a", "u"].freeze + ALLOWED_ATTRIBUTES = ["href", "target"].freeze + FILTERED_CHARACTERS = { + "&amp;" => "&", + "&" => "&", + " " => " " + }.freeze + + def strip_content(content) + content = strip_tags(content.to_s.strip) + + filter_characters(content) if content.length + end + + def sanitize_content(content) + content = sanitize(content.to_s, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES) + + filter_characters(content) if content.length + end + + private + + def filter_characters(content) + FILTERED_CHARACTERS.each do |character, sub| + content = content.gsub(character, sub) + end + content + end +end diff --git a/app/services/product_description_filter.rb b/app/services/product_description_filter.rb deleted file mode 100644 index da66ae23cc..0000000000 --- a/app/services/product_description_filter.rb +++ /dev/null @@ -1,16 +0,0 @@ -# frozen_string_literal: true - -class ProductDescriptionFilter - FILTERED_CHARACTERS = { - "&amp;" => "&", - "&" => "&", - " " => " " - }.freeze - - def self.filter(descripton) - FILTERED_CHARACTERS.each do |character, sub| - descripton = descripton.gsub(character, sub) - end - descripton - end -end From 5723a79409ecedab83fe9f3c1c1d051d1129239a Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 11 Jan 2021 21:21:24 -0800 Subject: [PATCH 4/7] check for `content.present?` --- app/services/content_sanitizer.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/content_sanitizer.rb b/app/services/content_sanitizer.rb index 4b2c7d3046..fc28e6c1dd 100644 --- a/app/services/content_sanitizer.rb +++ b/app/services/content_sanitizer.rb @@ -16,13 +16,13 @@ class ContentSanitizer def strip_content(content) content = strip_tags(content.to_s.strip) - filter_characters(content) if content.length + filter_characters(content) if content.present? end def sanitize_content(content) content = sanitize(content.to_s, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES) - filter_characters(content) if content.length + filter_characters(content) if content.present? end private From 67e58257390f8506e92ed964dbf5552f9b5072ca Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 11 Jan 2021 21:34:59 -0800 Subject: [PATCH 5/7] add specs for content sanitizer --- spec/services/content_sanitizer.rb | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 spec/services/content_sanitizer.rb diff --git a/spec/services/content_sanitizer.rb b/spec/services/content_sanitizer.rb new file mode 100644 index 0000000000..f00a9e8db6 --- /dev/null +++ b/spec/services/content_sanitizer.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ContentSanitizer do + let(:service) { described_class.new } + + context "#strip_content" do + it "strips disallowed tags" do + expect(service.strip_content("I'm friendly!")).to eq("I'm friendly!") + end + + it "replaces spaces" do + expect(service.strip_content("swiss chard")).to eq("swiss chard") + end + + it "replaces ampersands" do + expect(service.strip_content("pb & j")).to eq("pb & j") + end + + it "replaces double escaped ampersands" do + expect(service.strip_content("pb &amp; j")).to eq("pb & j") + end + end + + context "#sanitize_content" do + it "leaves bold tags" do + bold = "I'm bold" + expect(service.sanitize_content(bold)).to eq(bold) + end + + it "leaves links intact" do + link = "Bar" + expect(service.sanitize_content(link)).to eq(link) + end + + it "replaces spaces" do + expect(service.sanitize_content("swiss chard")).to eq("swiss chard") + end + + it "replaces ampersands" do + expect(service.sanitize_content("pb & j")).to eq("pb & j") + end + + it "replaces double escaped ampersands" do + expect(service.sanitize_content("pb &amp; j")).to eq("pb & j") + end + end +end From bbd7fd0350890eb28bb6c859a653cde06e93b02c Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 13 Jan 2021 20:57:27 -0800 Subject: [PATCH 6/7] handle nil product descriptions --- app/serializers/api/product_serializer.rb | 2 +- app/services/content_sanitizer.rb | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/serializers/api/product_serializer.rb b/app/serializers/api/product_serializer.rb index 33c8ba4430..d97f9967b2 100644 --- a/app/serializers/api/product_serializer.rb +++ b/app/serializers/api/product_serializer.rb @@ -21,7 +21,7 @@ class Api::ProductSerializer < ActiveModel::Serializer # return a sanitized html description def description_html - sanitizer.sanitize_content(object.description).html_safe + sanitizer.sanitize_content(object.description)&.html_safe end def properties_with_values diff --git a/app/services/content_sanitizer.rb b/app/services/content_sanitizer.rb index fc28e6c1dd..66db2879fd 100644 --- a/app/services/content_sanitizer.rb +++ b/app/services/content_sanitizer.rb @@ -14,15 +14,19 @@ class ContentSanitizer }.freeze def strip_content(content) + return unless content.present? + content = strip_tags(content.to_s.strip) - filter_characters(content) if content.present? + filter_characters(content) end def sanitize_content(content) + return unless content.present? + content = sanitize(content.to_s, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES) - filter_characters(content) if content.present? + filter_characters(content) end private From 08d9ef583279d7684f2f5e21fab56c7ea6dc2d56 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 13 Jan 2021 21:01:12 -0800 Subject: [PATCH 7/7] rename spec file; add test for nil content --- .../{content_sanitizer.rb => content_sanitizer_spec.rb} | 8 ++++++++ 1 file changed, 8 insertions(+) rename spec/services/{content_sanitizer.rb => content_sanitizer_spec.rb} (87%) diff --git a/spec/services/content_sanitizer.rb b/spec/services/content_sanitizer_spec.rb similarity index 87% rename from spec/services/content_sanitizer.rb rename to spec/services/content_sanitizer_spec.rb index f00a9e8db6..8e2117c1e8 100644 --- a/spec/services/content_sanitizer.rb +++ b/spec/services/content_sanitizer_spec.rb @@ -21,6 +21,10 @@ describe ContentSanitizer do it "replaces double escaped ampersands" do expect(service.strip_content("pb &amp; j")).to eq("pb & j") end + + it "echos nil if given nil" do + expect(service.strip_content(nil)).to be(nil) + end end context "#sanitize_content" do @@ -45,5 +49,9 @@ describe ContentSanitizer do it "replaces double escaped ampersands" do expect(service.sanitize_content("pb &amp; j")).to eq("pb & j") end + + it "echos nil if given nil" do + expect(service.sanitize_content(nil)).to be(nil) + end end end