Merge pull request #200 from flavorjones/199-css-functions-not-handled-properly

fix: handle CSS functions in a CSS shorthand property

Author: flavorjones

lib/loofah/html5/scrub.rb

@@ -7,22 +7,22 @@ module HTML5 # :nodoc:
     module Scrub
       CONTROL_CHARACTERS = /[`\u0000-\u0020\u007f\u0080-\u0101]/
       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 => ";" }
+      CRASS_SEMICOLON = { node: :semicolon, raw: ";" }
       CSS_IMPORTANT = '!important'
 
       class << self
         def allowed_element?(element_name)
-          ::Loofah::HTML5::SafeList::ALLOWED_ELEMENTS_WITH_LIBXML2.include? element_name
+          ::Loofah::HTML5::SafeList::ALLOWED_ELEMENTS_WITH_LIBXML2.include?(element_name)
         end
 
         #  alternative implementation of the html5lib attribute scrubbing algorithm
         def scrub_attributes(node)
           node.attribute_nodes.each do |attr_node|
             attr_name = if attr_node.namespace
-                          "#{attr_node.namespace.prefix}:#{attr_node.node_name}"
-                        else
-                          attr_node.node_name
-                        end
+              "#{attr_node.namespace.prefix}:#{attr_node.node_name}"
+            else
+              attr_node.node_name
+            end
 
             if attr_name =~ /\Adata-[\w-]+\z/
               next
@@ -58,13 +58,13 @@ def scrub_attributes(node)
             end
           end
 
-          scrub_css_attribute node
+          scrub_css_attribute(node)
 
           node.attribute_nodes.each do |attr_node|
             node.remove_attribute(attr_node.name) if attr_node.value !~ /[^[:space:]]/
           end
 
-          force_correct_attribute_escaping! node
+          force_correct_attribute_escaping!(node)
         end
 
         def scrub_css_attribute(node)
@@ -73,33 +73,50 @@ def scrub_css_attribute(node)
         end
 
         def scrub_css(style)
-          style_tree = Crass.parse_properties style
+          style_tree = Crass.parse_properties(style)
           sanitized_tree = []
 
           style_tree.each do |node|
             next unless node[:node] == :property
             next if node[:children].any? do |child|
-              [:url, :bad_url].include?(child[:node]) || (child[:node] == :function && !SafeList::ALLOWED_CSS_FUNCTIONS.include?(child[:name].downcase))
+              [:url, :bad_url].include?(child[:node])
             end
+
             name = node[:name].downcase
-            if SafeList::ALLOWED_CSS_PROPERTIES.include?(name) || SafeList::ALLOWED_SVG_PROPERTIES.include?(name)
-              sanitized_tree << node << CRASS_SEMICOLON
-            elsif SafeList::SHORTHAND_CSS_PROPERTIES.include?(name.split("-").first)
-              value = node[:value].split.map do |keyword|
-                if SafeList::ALLOWED_CSS_KEYWORDS.include?(keyword) || keyword =~ CSS_KEYWORDISH
+            next unless SafeList::ALLOWED_CSS_PROPERTIES.include?(name) ||
+                SafeList::ALLOWED_SVG_PROPERTIES.include?(name) ||
+                SafeList::SHORTHAND_CSS_PROPERTIES.include?(name.split("-").first)
+
+            value = node[:children].map do |child|
+              case child[:node]
+              when :whitespace
+                nil
+              when :string
+                nil
+              when :function
+                if SafeList::ALLOWED_CSS_FUNCTIONS.include?(child[:name].downcase)
+                  Crass::Parser.stringify(child)
+                end
+              when :ident
+                keyword = child[:value]
+                if !SafeList::SHORTHAND_CSS_PROPERTIES.include?(name.split("-").first) ||
+                   SafeList::ALLOWED_CSS_KEYWORDS.include?(keyword) ||
+                   (keyword =~ CSS_KEYWORDISH)
                   keyword
                 end
-              end.compact
-              unless value.empty?
-                value << CSS_IMPORTANT if node[:important]
-                propstring = sprintf "%s:%s", name, value.join(" ")
-                sanitized_node = Crass.parse_properties(propstring).first
-                sanitized_tree << sanitized_node << CRASS_SEMICOLON
+              else
+                child[:raw]
               end
-            end
+            end.compact
+
+            next if value.empty?
+            value << CSS_IMPORTANT if node[:important]
+            propstring = format("%s:%s", name, value.join(" "))
+            sanitized_node = Crass.parse_properties(propstring).first
+            sanitized_tree << sanitized_node << CRASS_SEMICOLON
           end
 
-          Crass::Parser.stringify sanitized_tree
+          Crass::Parser.stringify(sanitized_tree)
         end
 
         #

test/assets/testdata_sanitizer_tests1.dat

@@ -458,31 +458,31 @@
     "name": "style_attr_end_with_nothing",
     "input": "<div style=\"color: blue\" />",
     "output": "<div style='color: blue;'/>",
-    "xhtml": "<div style='color: blue;'></div>",
+    "xhtml": "<div style='color:blue;'></div>",
     "rexml": "<div style='color: blue;'></div>"
   },
 
   {
     "name": "style_attr_end_with_space",
     "input": "<div style=\"color: blue \" />",
     "output": "<div style='color: blue ;'/>",
-    "xhtml": "<div style='color: blue ;'></div>",
+    "xhtml": "<div style='color:blue;'></div>",
     "rexml": "<div style='color: blue ;'></div>"
   },
 
   {
     "name": "style_attr_end_with_semicolon",
     "input": "<div style=\"color: blue;\" />",
     "output": "<div style='color: blue;'/>",
-    "xhtml": "<div style='color: blue;'></div>",
+    "xhtml": "<div style='color:blue;'></div>",
     "rexml": "<div style='color: blue;'></div>"
   },
 
   {
     "name": "style_attr_end_with_semicolon_space",
     "input": "<div style=\"color: blue; \" />",
     "output": "<div style='color: blue;'/>",
-    "xhtml": "<div style='color: blue;'></div>",
+    "xhtml": "<div style='color:blue;'></div>",
     "rexml": "<div style='color: blue;'></div>"
   },
 

test/html5/test_scrub.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+require "helper"
+
+class UnitHTML5Scrub < Loofah::TestCase
+  include Loofah
+
+  describe "hex values" do
+    it "handles upper case" do
+      assert_equal "background:#ABC012;", Loofah::HTML5::Scrub.scrub_css("background: #ABC012")
+    end
+    it "handles lower case" do
+      assert_equal "background:#abc012;", Loofah::HTML5::Scrub.scrub_css("background: #abc012")
+    end
+  end
+
+  describe "css functions" do
+    it "allows safe functions" do
+      assert_equal "background-color:linear-gradient(transparent 50%, #ffff66 50%);",
+                   Loofah::HTML5::Scrub.scrub_css("background-color: linear-gradient(transparent 50%, #ffff66 50%);")
+    end
+
+    it "disallows unsafe functions" do
+      assert_equal "", Loofah::HTML5::Scrub.scrub_css("background-color: haxxor-fun(transparent 50%, #ffff66 50%);")
+    end
+
+    # see #199 for the bug we're testing here
+    it "allows safe functions in shorthand css properties" do
+      assert_equal "background:linear-gradient(transparent 50%, #ffff66 50%);",
+                   Loofah::HTML5::Scrub.scrub_css("background: linear-gradient(transparent 50%, #ffff66 50%);")
+    end
+  end
+end

test/html5/test_scrub_css.rb

@@ -226,8 +226,7 @@ def test_dont_remove_whitespace_between_tags
 
         it "Loofah.document removes the comment" do
           sanitized = Loofah.document(html)
-          sanitized_html = sanitized.to_html
-          refute_match(/--/, sanitized_html)
+          refute(sanitized.children.any? { |node| node.comment? } )
         end
 
         it "Loofah.scrub_document removes the comment" do