fix: allow CSS properties to have quoted string values

This was broken in v2.9.0 by bf13d48 Fixes #202
flavorjones · Apr 7, 2021 · 1565bfe · 1565bfe
1 parent cc5ce1c
commit 1565bfe
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,22 @@
 # Changelog
 
-### 2.9.0 / 2021-01-14
+### next / unreleased
+
+### Bug fixes
+
+* Fix a regression in v2.9.0 which inappropriately removed CSS properties with quoted string values. [[#202](https://github.com/flavorjones/loofah/issues/202)]
+
+
+## 2.9.0 / 2021-01-14
+
+### Features
 
 * Handle CSS functions in a CSS shorthand property (like `background`). [[#199](https://github.com/flavorjones/loofah/issues/199), [#200](https://github.com/flavorjones/loofah/issues/200)]
 
 
-### 2.8.0 / 2020-11-25
+## 2.8.0 / 2020-11-25
+
+### Features
 
 * Allow CSS properties `order`, `flex-direction`, `flex-grow`, `flex-wrap`, `flex-shrink`, `flex-flow`, `flex-basis`, `flex`, `justify-content`, `align-self`, `align-items`, and `align-content`. [[#197](https://github.com/flavorjones/loofah/issues/197)] (Thanks, [@miguelperez](https://github.com/miguelperez)!)
 

diff --git a/lib/loofah/html5/scrub.rb b/lib/loofah/html5/scrub.rb
@@ -9,6 +9,7 @@ module Scrub
       CSS_KEYWORDISH = /\A(#[0-9a-fA-F]+|rgb\(\d+%?,\d*%?,?\d*%?\)?|-?\d{0,3}\.?\d{0,10}(ch|cm|r?em|ex|in|lh|mm|pc|pt|px|Q|vmax|vmin|vw|vh|%|,|\))?)\z/
       CRASS_SEMICOLON = { node: :semicolon, raw: ";" }
       CSS_IMPORTANT = '!important'
+      CSS_PROPERTY_STRING_WITHOUT_EMBEDDED_QUOTES = /\A(["'])?[^"']+\1\z/
 
       class << self
         def allowed_element?(element_name)
@@ -92,7 +93,11 @@ def scrub_css(style)
               when :whitespace
                 nil
               when :string
-                nil
+                if child[:raw] =~ CSS_PROPERTY_STRING_WITHOUT_EMBEDDED_QUOTES
+                  Crass::Parser.stringify(child)
+                else
+                  nil
+                end
               when :function
                 if SafeList::ALLOWED_CSS_FUNCTIONS.include?(child[:name].downcase)
                   Crass::Parser.stringify(child)

diff --git a/test/html5/test_scrub_css.rb b/test/html5/test_scrub_css.rb
@@ -29,4 +29,33 @@ class UnitHTML5Scrub < Loofah::TestCase
                    Loofah::HTML5::Scrub.scrub_css("background: linear-gradient(transparent 50%, #ffff66 50%);")
     end
   end
+
+  describe "property string values" do
+    it "allows hypenated values" do
+      text = %q(font-family:'AvenirNext-Regular';)
+      assert_equal(text, Loofah::HTML5::Scrub.scrub_css(text))
+
+      text = %q(font-family:"AvenirNext-Regular";)
+      assert_equal(text, Loofah::HTML5::Scrub.scrub_css(text))
+    end
+
+    it "allows embedded spaces in values" do
+      text = %q(font-family:'Avenir Next';)
+      assert_equal(text, Loofah::HTML5::Scrub.scrub_css(text))
+
+      text = %q(font-family:"Avenir Next";)
+      assert_equal(text, Loofah::HTML5::Scrub.scrub_css(text))
+    end
+
+    it "does not allow values with embedded or irregular quotes" do
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:'AvenirNext"-Regular';)))
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:"AvenirNext'-Regular";)))
+
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:'AvenirNext-Regular;)))
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:'AvenirNext-Regular";)))
+
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:"AvenirNext-Regular;)))
+      assert_empty(Loofah::HTML5::Scrub.scrub_css(%q(font-family:"AvenirNext-Regular';)))
+    end
+  end
 end
diff --git a/test/integration/test_ad_hoc.rb b/test/integration/test_ad_hoc.rb
@@ -260,5 +260,25 @@ def test_dont_remove_whitespace_between_tags
         end
       end
     end
+
+    it "handles property string values" do
+      input = <<~EOF
+        <span style="font-size: 36px; font-family: 'AvenirNext-Regular';">variation 1a</span>
+        <span style="font-size: 36px; font-family: AvenirNext-Regular;">variation 1b</span>
+        <span style="font-size: 36px; font-family: 'Avenir Next';">variation 2a</span>
+        <span style="font-size: 36px; font-family: Avenir Next;">variation 2b</span>
+      EOF
+
+      expected = <<~EOF
+        <span style="font-size:36px;font-family:'AvenirNext-Regular';">variation 1a</span>
+        <span style="font-size:36px;font-family:AvenirNext-Regular;">variation 1b</span>
+        <span style="font-size:36px;font-family:'Avenir Next';">variation 2a</span>
+        <span style="font-size:36px;font-family:Avenir Next;">variation 2b</span>
+      EOF
+
+      actual = Loofah.scrub_fragment(input, :escape)
+
+      assert_equal(expected, actual.to_html)
+    end
   end
 end