Fixed CSS injection with width and height options

ankane · Aug 4, 2020 · ba67ab5 · ba67ab5
1 parent c1437a2
commit ba67ab5
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,3 +1,7 @@
+## 3.4.0 (unreleased)
+
+- Fixed CSS injection with `width` and `height` options
+
 ## 3.3.2 (2020-07-23)
 
 - Updated Chartkick.js to 3.2.1

diff --git a/lib/chartkick/helper.rb b/lib/chartkick/helper.rb
@@ -41,8 +41,8 @@ def chartkick_chart(klass, data_source, **options)
       @chartkick_chart_id ||= 0
       options = chartkick_deep_merge(Chartkick.options, options)
       element_id = options.delete(:id) || "chart-#{@chartkick_chart_id += 1}"
-      height = options.delete(:height) || "300px"
-      width = options.delete(:width) || "100%"
+      height = (options.delete(:height) || "300px").to_s
+      width = (options.delete(:width) || "100%").to_s
       defer = !!options.delete(:defer)
       # content_for: nil must override default
       content_for = options.key?(:content_for) ? options.delete(:content_for) : Chartkick.content_for
@@ -63,14 +63,27 @@ def chartkick_chart(klass, data_source, **options)
 
       # html vars
       html_vars = {
-        id: element_id,
-        height: height,
-        width: width
+        id: element_id
       }
       html_vars.each_key do |k|
         html_vars[k] = ERB::Util.html_escape(html_vars[k])
       end
-      html = (options.delete(:html) || %(<div id="%{id}" style="height: %{height}; width: %{width}; text-align: center; color: #999; line-height: %{height}; font-size: 14px; font-family: 'Lucida Grande', 'Lucida Sans Unicode', Verdana, Arial, Helvetica, sans-serif;">Loading...</div>)) % html_vars
+
+      # css vars
+      css_vars = {
+        height: height,
+        width: width
+      }
+      css_vars.each_key do |k|
+        # limit to alphanumeric and % for simplicity
+        # this prevents things like calc() but safety is the priority
+        raise ArgumentError, "Invalid #{k}" unless css_vars[k] =~ /\A[a-zA-Z0-9%]*\z/
+        # we limit above, but escape for safety as fail-safe
+        # to prevent XSS injection in worse-case scenario
+        css_vars[k] = ERB::Util.html_escape(css_vars[k])
+      end
+
+      html = (options.delete(:html) || %(<div id="%{id}" style="height: %{height}; width: %{width}; text-align: center; color: #999; line-height: %{height}; font-size: 14px; font-family: 'Lucida Grande', 'Lucida Sans Unicode', Verdana, Arial, Helvetica, sans-serif;">Loading...</div>)) % html_vars.merge(css_vars)
 
       # js vars
       js_vars = {

diff --git a/test/chartkick_test.rb b/test/chartkick_test.rb
@@ -80,20 +80,48 @@ def test_id_escaped
     assert_match "id=\"test-123&quot;\"", line_chart(@data, id: "test-123\"")
   end
 
-  def test_height
-    assert_match "height: 150px;", line_chart(@data, height: "150px")
+  def test_height_pixels
+    assert_match "height: 100px;", line_chart(@data, height: "100px")
   end
 
-  def test_height_escaped
-    assert_match "height: 150px&quot;;", line_chart(@data, height: "150px\"")
+  def test_height_percent
+    assert_match "height: 100%;", line_chart(@data, height: "100%")
   end
 
-  def test_width
-    assert_match "width: 80%;", line_chart(@data, width: "80%")
+  def test_height_quote
+    error = assert_raises(ArgumentError) do
+      line_chart(@data, height: "150px\"")
+    end
+    assert_equal "Invalid height", error.message
+  end
+
+  def test_height_semicolon
+    error = assert_raises(ArgumentError) do
+      line_chart(@data, height: "150px;background:123")
+    end
+    assert_equal "Invalid height", error.message
+  end
+
+  def test_width_pixels
+    assert_match "width: 100px;", line_chart(@data, width: "100px")
   end
 
-  def test_width_escaped
-    assert_match "width: 80%&quot;;", line_chart(@data, width: "80%\"")
+  def test_width_percent
+    assert_match "width: 100%;", line_chart(@data, width: "100%")
+  end
+
+  def test_width_quote
+    error = assert_raises(ArgumentError) do
+      line_chart(@data, width: "80%\"")
+    end
+    assert_equal "Invalid width", error.message
+  end
+
+  def test_width_semicolon
+    error = assert_raises(ArgumentError) do
+      line_chart(@data, width: "80%;background:123")
+    end
+    assert_equal "Invalid width", error.message
   end
 
   def test_nonce