Fix and add protections for XSS in names.

Add the method ERB::Util.xml_name_escape to escape dangerous characters in names of tags and names of attributes, following the specification of XML. Use that method in the tag helpers of ActionView::Helpers. Add a deprecation warning to the option :escape_attributes mentioning the new behavior and the transition to :escape, to simplify by applying the option to the whole tag.
rails · Apr 12, 2022 · 5c7dae5 · 5c7dae5
1 parent 8198d7c
commit 5c7dae5
Show file tree

Hide file tree

Showing 6 changed files with 238 additions and 19 deletions.
diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`.
+
+    Escape dangerous characters in names of tags and names of attributes in the
+    tag helpers, following the XML specification. Rename the option
+    `:escape_attributes` to `:escape`, to simplify by applying the option to the
+    whole tag.
+
+    *Álvaro Martín Fraguas*
+
 ## Rails 7.0.2.3 (March 08, 2022) ##
 
 *   No changes.

diff --git a/actionview/lib/action_view/helpers/tag_helper.rb b/actionview/lib/action_view/helpers/tag_helper.rb
@@ -65,18 +65,25 @@ def p(*arguments, **options, &block)
           tag_string(:p, *arguments, **options, &block)
         end
 
-        def tag_string(name, content = nil, escape_attributes: true, **options, &block)
+        def tag_string(name, content = nil, **options, &block)
+          escape = handle_deprecated_escape_options(options)
+
           content = @view_context.capture(self, &block) if block_given?
           if (HTML_VOID_ELEMENTS.include?(name) || SVG_VOID_ELEMENTS.include?(name)) && content.nil?
-            "<#{name.to_s.dasherize}#{tag_options(options, escape_attributes)}>".html_safe
+            "<#{name.to_s.dasherize}#{tag_options(options, escape)}>".html_safe
           else
-            content_tag_string(name.to_s.dasherize, content || "", options, escape_attributes)
+            content_tag_string(name.to_s.dasherize, content || "", options, escape)
           end
         end
 
         def content_tag_string(name, content, options, escape = true)
           tag_options = tag_options(options, escape) if options
-          content     = ERB::Util.unwrapped_html_escape(content) if escape
+
+          if escape
+            name = ERB::Util.xml_name_escape(name)
+            content = ERB::Util.unwrapped_html_escape(content)
+          end
+
           "<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name]}#{content}</#{name}>".html_safe
         end
 
@@ -127,6 +134,8 @@ def boolean_tag_option(key)
         end
 
         def tag_option(key, value, escape)
+          key = ERB::Util.xml_name_escape(key) if escape
+
           case value
           when Array, Hash
             value = TagHelper.build_tag_values(value) if key.to_s == "class"
@@ -137,6 +146,7 @@ def tag_option(key, value, escape)
             value = escape ? ERB::Util.unwrapped_html_escape(value) : value.to_s
           end
           value = value.gsub('"', "&quot;") if value.include?('"')
+
           %(#{key}="#{value}")
         end
 
@@ -153,6 +163,27 @@ def respond_to_missing?(*args)
             true
           end
 
+          def handle_deprecated_escape_options(options)
+            # The option :escape_attributes has been merged into the options hash to be
+            # able to warn when it is used, so we need to handle default values here.
+            escape_option_provided = options.has_key?(:escape)
+            escape_attributes_option_provided = options.has_key?(:escape_attributes)
+
+            if escape_attributes_option_provided
+              ActiveSupport::Deprecation.warn(<<~MSG)
+                Use of the option :escape_attributes is deprecated. It currently \
+                escapes both names and values of tags and attributes and it is \
+                equivalent to :escape. If any of them are enabled, the escaping \
+                is fully enabled.
+              MSG
+            end
+
+            return true unless escape_option_provided || escape_attributes_option_provided
+            escape_option = options.delete(:escape)
+            escape_attributes_option = options.delete(:escape_attributes)
+            escape_option || escape_attributes_option
+          end
+
           def method_missing(called, *args, **options, &block)
             tag_string(called, *args, **options, &block)
           end
@@ -216,13 +247,13 @@ def method_missing(called, *args, **options, &block)
       #   tag.div data: { city_state: %w( Chicago IL ) }
       #   # => <div data-city-state="[&quot;Chicago&quot;,&quot;IL&quot;]"></div>
       #
-      # The generated attributes are escaped by default. This can be disabled using
-      # +escape_attributes+.
+      # The generated tag names and attributes are escaped by default. This can be disabled using
+      # +escape+.
       #
       #   tag.img src: 'open & shut.png'
       #   # => <img src="open &amp; shut.png">
       #
-      #   tag.img src: 'open & shut.png', escape_attributes: false
+      #   tag.img src: 'open & shut.png', escape: false
       #   # => <img src="open & shut.png">
       #
       # The tag builder respects
@@ -300,6 +331,7 @@ def tag(name = nil, options = nil, open = false, escape = true)
         if name.nil?
           tag_builder
         else
+          name = ERB::Util.xml_name_escape(name) if escape
           "<#{name}#{tag_builder.tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
         end
       end
@@ -308,7 +340,7 @@ def tag(name = nil, options = nil, open = false, escape = true)
       # HTML attributes by passing an attributes hash to +options+.
       # Instead of passing the content as an argument, you can also use a block
       # in which case, you pass your +options+ as the second parameter.
-      # Set escape to false to disable attribute value escaping.
+      # Set escape to false to disable escaping.
       # Note: this is legacy syntax, see +tag+ method description for details.
       #
       # ==== Options

diff --git a/actionview/test/template/tag_helper_test.rb b/actionview/test/template/tag_helper_test.rb
@@ -7,6 +7,8 @@ class TagHelperTest < ActionView::TestCase
 
   tests ActionView::Helpers::TagHelper
 
+  COMMON_DANGEROUS_CHARS = "&<>\"' %*+,/;=^|"
+
   def test_tag
     assert_equal "<br />", tag("br")
     assert_equal "<br clear=\"left\" />", tag(:br, clear: "left")
@@ -111,6 +113,77 @@ def test_tag_builder_do_not_modify_html_safe_options
     assert html_safe_str.html_safe?
   end
 
+  def test_tag_with_dangerous_name
+    assert_equal "<#{"_" * COMMON_DANGEROUS_CHARS.size} />",
+                 tag(COMMON_DANGEROUS_CHARS)
+
+    assert_equal "<#{COMMON_DANGEROUS_CHARS} />",
+                 tag(COMMON_DANGEROUS_CHARS, nil, false, false)
+  end
+
+  def test_tag_builder_with_dangerous_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<#{escaped_dangerous_chars}></#{escaped_dangerous_chars}>",
+                 tag.public_send(COMMON_DANGEROUS_CHARS.to_sym)
+
+    assert_equal "<#{COMMON_DANGEROUS_CHARS}></#{COMMON_DANGEROUS_CHARS}>",
+                 tag.public_send(COMMON_DANGEROUS_CHARS.to_sym, nil, escape: false)
+  end
+
+  def test_tag_with_dangerous_aria_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name aria-#{escaped_dangerous_chars}=\"the value\" />",
+                 tag("the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
+
+    assert_equal "<the-name aria-#{COMMON_DANGEROUS_CHARS}=\"the value\" />",
+                 tag("the-name", { aria: { COMMON_DANGEROUS_CHARS => "the value" } }, false, false)
+  end
+
+  def test_tag_builder_with_dangerous_aria_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name aria-#{escaped_dangerous_chars}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
+
+    assert_equal "<the-name aria-#{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" }, escape: false)
+  end
+
+  def test_tag_with_dangerous_data_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name data-#{escaped_dangerous_chars}=\"the value\" />",
+                 tag("the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
+
+    assert_equal "<the-name data-#{COMMON_DANGEROUS_CHARS}=\"the value\" />",
+                 tag("the-name", { data: { COMMON_DANGEROUS_CHARS => "the value" } }, false, false)
+  end
+
+  def test_tag_builder_with_dangerous_data_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name data-#{escaped_dangerous_chars}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
+
+    assert_equal "<the-name data-#{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", data: { COMMON_DANGEROUS_CHARS => "the value" }, escape: false)
+  end
+
+  def test_tag_with_dangerous_unknown_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\" />",
+                 tag("the-name", COMMON_DANGEROUS_CHARS => "the value")
+
+    assert_equal "<the-name #{COMMON_DANGEROUS_CHARS}=\"the value\" />",
+                 tag("the-name", { COMMON_DANGEROUS_CHARS => "the value" }, false, false)
+  end
+
+  def test_tag_builder_with_dangerous_unknown_attribute_name
+    escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
+    assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")
+
+    assert_equal "<the-name #{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
+                 tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value", escape: false)
+  end
+
   def test_content_tag
     assert_equal "<a href=\"create\">Create</a>", content_tag("a", "Create", "href" => "create")
     assert_predicate content_tag("a", "Create", "href" => "create"), :html_safe?
@@ -130,10 +203,54 @@ def test_tag_builder_with_content
     assert_equal "<p>&lt;script&gt;evil_js&lt;/script&gt;</p>",
                  tag.p("<script>evil_js</script>")
     assert_equal "<p><script>evil_js</script></p>",
-                 tag.p("<script>evil_js</script>", escape_attributes: false)
+                 tag.p("<script>evil_js</script>", escape: false)
     assert_equal '<input pattern="\w+">', tag.input(pattern: /\w+/)
   end
 
+  def test_deprecated_option_escape_attributes
+    raw_content = "<script>evil_js</script>"
+    escaped_content = "&lt;script&gt;evil_js&lt;/script&gt;"
+
+    expected_deprecation_warning = <<~MSG
+      Use of the option :escape_attributes is deprecated. It currently \
+      escapes both names and values of tags and attributes and it is \
+      equivalent to :escape. If any of them are enabled, the escaping \
+      is fully enabled.
+    MSG
+
+    assert_equal "<p>#{escaped_content}</p>",
+                 tag.p(raw_content)
+    assert_equal "<p>#{escaped_content}</p>",
+                 tag.p(raw_content, escape: true)
+    assert_equal "<p>#{raw_content}</p>",
+                 tag.p(raw_content, escape: false)
+
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{escaped_content}</p>",
+                   tag.p(raw_content, escape_attributes: true)
+    end
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{raw_content}</p>",
+                   tag.p(raw_content, escape_attributes: false)
+    end
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{escaped_content}</p>",
+                   tag.p(raw_content, escape_attributes: true, escape: true)
+    end
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{raw_content}</p>",
+                   tag.p(raw_content, escape_attributes: false, escape: false)
+    end
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{escaped_content}</p>",
+                   tag.p(raw_content, escape_attributes: true, escape: false)
+    end
+    assert_deprecated(expected_deprecation_warning) do
+      assert_equal "<p>#{escaped_content}</p>",
+                   tag.p(raw_content, escape_attributes: false, escape: true)
+    end
+  end
+
   def test_tag_builder_nested
     assert_equal "<div>content</div>",
                  tag.div { "content" }
@@ -246,10 +363,10 @@ def test_content_tag_with_unescaped_array_class
   end
 
   def test_tag_builder_with_unescaped_array_class
-    str = tag.p "limelight", class: ["song", "play>"], escape_attributes: false
+    str = tag.p "limelight", class: ["song", "play>"], escape: false
     assert_equal "<p class=\"song play>\">limelight</p>", str
 
-    str = tag.p "limelight", class: ["song", ["play>"]], escape_attributes: false
+    str = tag.p "limelight", class: ["song", ["play>"]], escape: false
     assert_equal "<p class=\"song play>\">limelight</p>", str
   end
 
@@ -268,7 +385,7 @@ def test_content_tag_with_unescaped_empty_array_class
   end
 
   def test_tag_builder_with_unescaped_empty_array_class
-    str = tag.p "limelight", class: [], escape_attributes: false
+    str = tag.p "limelight", class: [], escape: false
     assert_equal '<p class="">limelight</p>', str
   end
 
@@ -339,10 +456,10 @@ def test_content_tag_with_unescaped_conditional_hash_classes
   end
 
   def test_tag_builder_with_unescaped_conditional_hash_classes
-    str = tag.p "limelight", class: { "song": true, "play>": true }, escape_attributes: false
+    str = tag.p "limelight", class: { "song": true, "play>": true }, escape: false
     assert_equal "<p class=\"song play>\">limelight</p>", str
 
-    str = tag.p "limelight", class: ["song", { "play>": true }], escape_attributes: false
+    str = tag.p "limelight", class: ["song", { "play>": true }], escape: false
     assert_equal "<p class=\"song play>\">limelight</p>", str
   end
 
@@ -456,11 +573,11 @@ def test_disable_escaping
   end
 
   def test_tag_builder_disable_escaping
-    assert_equal '<a href="&amp;"></a>', tag.a(href: "&amp;", escape_attributes: false)
-    assert_equal '<a href="&amp;">cnt</a>', tag.a(href: "&amp;", escape_attributes: false) { "cnt" }
-    assert_equal '<br data-hidden="&amp;">', tag.br("data-hidden": "&amp;", escape_attributes: false)
-    assert_equal '<a href="&amp;">content</a>', tag.a("content", href: "&amp;", escape_attributes: false)
-    assert_equal '<a href="&amp;">content</a>', tag.a(href: "&amp;", escape_attributes: false) { "content" }
+    assert_equal '<a href="&amp;"></a>', tag.a(href: "&amp;", escape: false)
+    assert_equal '<a href="&amp;">cnt</a>', tag.a(href: "&amp;", escape: false) { "cnt" }
+    assert_equal '<br data-hidden="&amp;">', tag.br("data-hidden": "&amp;", escape: false)
+    assert_equal '<a href="&amp;">content</a>', tag.a("content", href: "&amp;", escape: false)
+    assert_equal '<a href="&amp;">content</a>', tag.a(href: "&amp;", escape: false) { "content" }
   end
 
   def test_data_attributes

diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`.
+
+    Add the method `ERB::Util.xml_name_escape` to escape dangerous characters
+    in names of tags and names of attributes, following the specification of XML.
+
+    *Álvaro Martín Fraguas*
+
 ## Rails 7.0.2.3 (March 08, 2022) ##
 
 *   No changes.

diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb
@@ -11,6 +11,14 @@ module Util
     HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+)|(#[xX][\dA-Fa-f]+));)/
     JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u
 
+    # Following XML requirements: https://www.w3.org/TR/REC-xml/#NT-Name
+    TAG_NAME_START_REGEXP_SET = ":A-Z_a-z\u{C0}-\u{D6}\u{D8}-\u{F6}\u{F8}-\u{2FF}\u{370}-\u{37D}\u{37F}-\u{1FFF}" \
+                                "\u{200C}-\u{200D}\u{2070}-\u{218F}\u{2C00}-\u{2FEF}\u{3001}-\u{D7FF}\u{F900}-\u{FDCF}" \
+                                "\u{FDF0}-\u{FFFD}\u{10000}-\u{EFFFF}"
+    TAG_NAME_START_REGEXP = /[^#{TAG_NAME_START_REGEXP_SET}]/
+    TAG_NAME_FOLLOWING_REGEXP = /[^#{TAG_NAME_START_REGEXP_SET}\-.0-9\u{B7}\u{0300}-\u{036F}\u{203F}-\u{2040}]/
+    TAG_NAME_REPLACEMENT_CHAR = "_"
+
     # A utility method for escaping HTML tag characters.
     # This method is also aliased as <tt>h</tt>.
     #
@@ -115,6 +123,26 @@ def json_escape(s)
     end
 
     module_function :json_escape
+
+    # A utility method for escaping XML names of tags and names of attributes.
+    #
+    #   xml_name_escape('1 < 2 & 3')
+    #   # => "1___2___3"
+    #
+    # It follows the requirements of the specification: https://www.w3.org/TR/REC-xml/#NT-Name
+    def xml_name_escape(name)
+      name = name.to_s
+      return "" if name.blank?
+
+      starting_char = name[0].gsub(TAG_NAME_START_REGEXP, TAG_NAME_REPLACEMENT_CHAR)
+
+      return starting_char if name.size == 1
+
+      following_chars = name[1..-1].gsub(TAG_NAME_FOLLOWING_REGEXP, TAG_NAME_REPLACEMENT_CHAR)
+
+      starting_char + following_chars
+    end
+    module_function :xml_name_escape
   end
 end