diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md
index f2be93e1d6c82..c41e74f36cedb 100644
--- a/actionview/CHANGELOG.md
+++ b/actionview/CHANGELOG.md
@@ -1,3 +1,13 @@
+* 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 6.0.4.7 (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
index fc2654a6842d2..7b51d18730f78 100644
--- a/actionview/lib/action_view/helpers/tag_helper.rb
+++ b/actionview/lib/action_view/helpers/tag_helper.rb
@@ -41,18 +41,25 @@ def initialize(view_context)
@view_context = view_context
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 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
@@ -85,6 +92,8 @@ def boolean_tag_option(key)
end
def tag_option(key, value, escape)
+ key = ERB::Util.xml_name_escape(key) if escape
+
if value.is_a?(Array)
value = escape ? safe_join(value, " ") : value.join(" ")
else
@@ -107,6 +116,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
@@ -237,6 +267,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
diff --git a/actionview/test/template/tag_helper_test.rb b/actionview/test/template/tag_helper_test.rb
index 6626f78678566..2f53826c9cfa7 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")
@@ -86,6 +88,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 "
<script>evil_js</script>
", tag.p("") assert_equal "", - tag.p("", escape_attributes: false) + tag.p("", escape: false) end def test_tag_builder_nested @@ -220,10 +293,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 @@ -242,7 +315,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 @@ -313,11 +386,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 '