diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md
index e86ebdb36eb8d..5612aeae989e4 100644
--- 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*
+
* Ensure models passed to `form_for` attempt to call `to_model`.
*Sean Doyle*
diff --git a/actionview/lib/action_view/helpers/tag_helper.rb b/actionview/lib/action_view/helpers/tag_helper.rb
index 265619ffb025c..d37dc6cfb2682 100644
--- a/actionview/lib/action_view/helpers/tag_helper.rb
+++ b/actionview/lib/action_view/helpers/tag_helper.rb
@@ -65,19 +65,24 @@ 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, escape: true, **options, &block)
content = @view_context.capture(self, &block) if block_given?
self_closing = SVG_SELF_CLOSING_ELEMENTS.include?(name)
if (HTML_VOID_ELEMENTS.include?(name) || self_closing) && content.nil?
- "<#{name.to_s.dasherize}#{tag_options(options, escape_attributes)}#{self_closing ? " />" : ">"}".html_safe
+ "<#{name.to_s.dasherize}#{tag_options(options, escape)}#{self_closing ? " />" : ">"}".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
@@ -128,6 +133,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"
@@ -138,6 +145,7 @@ def tag_option(key, value, escape)
value = escape ? ERB::Util.unwrapped_html_escape(value) : value.to_s
end
value = value.gsub('"', """) if value.include?('"')
+
%(#{key}="#{value}")
end
@@ -217,13 +225,13 @@ def method_missing(called, *args, **options, &block)
# tag.div data: { city_state: %w( Chicago IL ) }
# # =>
#
- # 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'
# # =>
#
- # tag.img src: 'open & shut.png', escape_attributes: false
+ # tag.img src: 'open & shut.png', escape: false
# # =>
#
# The tag builder respects
@@ -301,6 +309,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
@@ -309,7 +318,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
index e7b506fa1b48c..f401dfb699fd8 100644
--- 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 "
", tag("br")
assert_equal "
", tag(:br, clear: "left")
@@ -119,6 +121,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 "",
+ tag("the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
+
+ assert_equal "",
+ 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 "",
+ tag.public_send(:"the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
+
+ assert_equal "",
+ 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 "",
+ tag("the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
+
+ assert_equal "",
+ 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 "",
+ tag.public_send(:"the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
+
+ assert_equal "",
+ 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 "",
+ tag("the-name", COMMON_DANGEROUS_CHARS => "the value")
+
+ assert_equal "",
+ 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 "",
+ tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")
+
+ assert_equal "",
+ tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value", escape: false)
+ end
+
def test_content_tag
assert_equal "Create", content_tag("a", "Create", "href" => "create")
assert_predicate content_tag("a", "Create", "href" => "create"), :html_safe?
@@ -138,7 +211,7 @@ def test_tag_builder_with_content
assert_equal "<script>evil_js</script>
",
tag.p("")
assert_equal "",
- tag.p("", escape_attributes: false)
+ tag.p("", escape: false)
assert_equal '', tag.input(pattern: /\w+/)
end
@@ -254,10 +327,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 "\">limelight
", str
- str = tag.p "limelight", class: ["song", ["play>"]], escape_attributes: false
+ str = tag.p "limelight", class: ["song", ["play>"]], escape: false
assert_equal "\">limelight
", str
end
@@ -276,7 +349,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 'limelight
', str
end
@@ -347,10 +420,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 "\">limelight
", str
- str = tag.p "limelight", class: ["song", { "play>": true }], escape_attributes: false
+ str = tag.p "limelight", class: ["song", { "play>": true }], escape: false
assert_equal "\">limelight
", str
end
@@ -468,11 +541,11 @@ def test_disable_escaping
end
def test_tag_builder_disable_escaping
- assert_equal '', tag.a(href: "&", escape_attributes: false)
- assert_equal 'cnt', tag.a(href: "&", escape_attributes: false) { "cnt" }
- assert_equal '
', tag.br("data-hidden": "&", escape_attributes: false)
- assert_equal 'content', tag.a("content", href: "&", escape_attributes: false)
- assert_equal 'content', tag.a(href: "&", escape_attributes: false) { "content" }
+ assert_equal '', tag.a(href: "&", escape: false)
+ assert_equal 'cnt', tag.a(href: "&", escape: false) { "cnt" }
+ assert_equal '
', tag.br("data-hidden": "&", escape: false)
+ assert_equal 'content', tag.a("content", href: "&", escape: false)
+ assert_equal 'content', tag.a(href: "&", escape: false) { "content" }
end
def test_data_attributes
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 0587735977835..33a6ecd55b4c9 100644
--- 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*
+
* `Pathname.blank?` only returns true for `Pathname.new("")`
Previously it would end up calling `Pathname#empty?` which returned true
diff --git a/activesupport/lib/active_support/core_ext/string/output_safety.rb b/activesupport/lib/active_support/core_ext/string/output_safety.rb
index 98bc9ffbd2d09..d4ffff58965b0 100644
--- 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 h.
#
@@ -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
diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb
index efbe7fb607b04..f7cb841e82db5 100644
--- a/activesupport/test/core_ext/string_ext_test.rb
+++ b/activesupport/test/core_ext/string_ext_test.rb
@@ -1049,6 +1049,32 @@ def to_s
expected = "© <"
assert_equal expected, ERB::Util.html_escape_once(string)
end
+
+ test "ERB::Util.xml_name_escape should escape unsafe characters for XML names" do
+ unsafe_char = ">"
+ safe_char = "Á"
+ safe_char_after_start = "3"
+
+ assert_equal "_", ERB::Util.xml_name_escape(unsafe_char)
+ assert_equal "_#{safe_char}", ERB::Util.xml_name_escape(unsafe_char + safe_char)
+ assert_equal "__", ERB::Util.xml_name_escape(unsafe_char * 2)
+
+ assert_equal "__#{safe_char}_",
+ ERB::Util.xml_name_escape("#{unsafe_char * 2}#{safe_char}#{unsafe_char}")
+
+ assert_equal safe_char + safe_char_after_start,
+ ERB::Util.xml_name_escape(safe_char + safe_char_after_start)
+
+ assert_equal "_#{safe_char}",
+ ERB::Util.xml_name_escape(safe_char_after_start + safe_char)
+
+ assert_equal "img_src_nonexistent_onerror_alert_1_",
+ ERB::Util.xml_name_escape("img src=nonexistent onerror=alert(1)")
+
+ common_dangerous_chars = "&<>\"' %*+,/;=^|"
+ assert_equal "_" * common_dangerous_chars.size,
+ ERB::Util.xml_name_escape(common_dangerous_chars)
+ end
end
class StringExcludeTest < ActiveSupport::TestCase