Skip to content

Commit 5c7dae5

Browse files
amartinfraguastenderlove
amartinfraguas
authored and
tenderlove
committed
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.
1 parent 8198d7c commit 5c7dae5

File tree

6 files changed

+238
-19
lines changed

6 files changed

+238
-19
lines changed

‎actionview/CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
* Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`.
2+
3+
Escape dangerous characters in names of tags and names of attributes in the
4+
tag helpers, following the XML specification. Rename the option
5+
`:escape_attributes` to `:escape`, to simplify by applying the option to the
6+
whole tag.
7+
8+
*Álvaro Martín Fraguas*
9+
110
## Rails 7.0.2.3 (March 08, 2022) ##
211

312
* No changes.

‎actionview/lib/action_view/helpers/tag_helper.rb

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -65,18 +65,25 @@ def p(*arguments, **options, &block)
6565
tag_string(:p, *arguments, **options, &block)
6666
end
6767

68-
def tag_string(name, content = nil, escape_attributes: true, **options, &block)
68+
def tag_string(name, content = nil, **options, &block)
69+
escape = handle_deprecated_escape_options(options)
70+
6971
content = @view_context.capture(self, &block) if block_given?
7072
if (HTML_VOID_ELEMENTS.include?(name) || SVG_VOID_ELEMENTS.include?(name)) && content.nil?
71-
"<#{name.to_s.dasherize}#{tag_options(options, escape_attributes)}>".html_safe
73+
"<#{name.to_s.dasherize}#{tag_options(options, escape)}>".html_safe
7274
else
73-
content_tag_string(name.to_s.dasherize, content || "", options, escape_attributes)
75+
content_tag_string(name.to_s.dasherize, content || "", options, escape)
7476
end
7577
end
7678

7779
def content_tag_string(name, content, options, escape = true)
7880
tag_options = tag_options(options, escape) if options
79-
content = ERB::Util.unwrapped_html_escape(content) if escape
81+
82+
if escape
83+
name = ERB::Util.xml_name_escape(name)
84+
content = ERB::Util.unwrapped_html_escape(content)
85+
end
86+
8087
"<#{name}#{tag_options}>#{PRE_CONTENT_STRINGS[name]}#{content}</#{name}>".html_safe
8188
end
8289

@@ -127,6 +134,8 @@ def boolean_tag_option(key)
127134
end
128135

129136
def tag_option(key, value, escape)
137+
key = ERB::Util.xml_name_escape(key) if escape
138+
130139
case value
131140
when Array, Hash
132141
value = TagHelper.build_tag_values(value) if key.to_s == "class"
@@ -137,6 +146,7 @@ def tag_option(key, value, escape)
137146
value = escape ? ERB::Util.unwrapped_html_escape(value) : value.to_s
138147
end
139148
value = value.gsub('"', "&quot;") if value.include?('"')
149+
140150
%(#{key}="#{value}")
141151
end
142152

@@ -153,6 +163,27 @@ def respond_to_missing?(*args)
153163
true
154164
end
155165

166+
def handle_deprecated_escape_options(options)
167+
# The option :escape_attributes has been merged into the options hash to be
168+
# able to warn when it is used, so we need to handle default values here.
169+
escape_option_provided = options.has_key?(:escape)
170+
escape_attributes_option_provided = options.has_key?(:escape_attributes)
171+
172+
if escape_attributes_option_provided
173+
ActiveSupport::Deprecation.warn(<<~MSG)
174+
Use of the option :escape_attributes is deprecated. It currently \
175+
escapes both names and values of tags and attributes and it is \
176+
equivalent to :escape. If any of them are enabled, the escaping \
177+
is fully enabled.
178+
MSG
179+
end
180+
181+
return true unless escape_option_provided || escape_attributes_option_provided
182+
escape_option = options.delete(:escape)
183+
escape_attributes_option = options.delete(:escape_attributes)
184+
escape_option || escape_attributes_option
185+
end
186+
156187
def method_missing(called, *args, **options, &block)
157188
tag_string(called, *args, **options, &block)
158189
end
@@ -216,13 +247,13 @@ def method_missing(called, *args, **options, &block)
216247
# tag.div data: { city_state: %w( Chicago IL ) }
217248
# # => <div data-city-state="[&quot;Chicago&quot;,&quot;IL&quot;]"></div>
218249
#
219-
# The generated attributes are escaped by default. This can be disabled using
220-
# +escape_attributes+.
250+
# The generated tag names and attributes are escaped by default. This can be disabled using
251+
# +escape+.
221252
#
222253
# tag.img src: 'open & shut.png'
223254
# # => <img src="open &amp; shut.png">
224255
#
225-
# tag.img src: 'open & shut.png', escape_attributes: false
256+
# tag.img src: 'open & shut.png', escape: false
226257
# # => <img src="open & shut.png">
227258
#
228259
# The tag builder respects
@@ -300,6 +331,7 @@ def tag(name = nil, options = nil, open = false, escape = true)
300331
if name.nil?
301332
tag_builder
302333
else
334+
name = ERB::Util.xml_name_escape(name) if escape
303335
"<#{name}#{tag_builder.tag_options(options, escape) if options}#{open ? ">" : " />"}".html_safe
304336
end
305337
end
@@ -308,7 +340,7 @@ def tag(name = nil, options = nil, open = false, escape = true)
308340
# HTML attributes by passing an attributes hash to +options+.
309341
# Instead of passing the content as an argument, you can also use a block
310342
# in which case, you pass your +options+ as the second parameter.
311-
# Set escape to false to disable attribute value escaping.
343+
# Set escape to false to disable escaping.
312344
# Note: this is legacy syntax, see +tag+ method description for details.
313345
#
314346
# ==== Options

‎actionview/test/template/tag_helper_test.rb

Lines changed: 128 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ class TagHelperTest < ActionView::TestCase
77

88
tests ActionView::Helpers::TagHelper
99

10+
COMMON_DANGEROUS_CHARS = "&<>\"' %*+,/;=^|"
11+
1012
def test_tag
1113
assert_equal "<br />", tag("br")
1214
assert_equal "<br clear=\"left\" />", tag(:br, clear: "left")
@@ -111,6 +113,77 @@ def test_tag_builder_do_not_modify_html_safe_options
111113
assert html_safe_str.html_safe?
112114
end
113115

116+
def test_tag_with_dangerous_name
117+
assert_equal "<#{"_" * COMMON_DANGEROUS_CHARS.size} />",
118+
tag(COMMON_DANGEROUS_CHARS)
119+
120+
assert_equal "<#{COMMON_DANGEROUS_CHARS} />",
121+
tag(COMMON_DANGEROUS_CHARS, nil, false, false)
122+
end
123+
124+
def test_tag_builder_with_dangerous_name
125+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
126+
assert_equal "<#{escaped_dangerous_chars}></#{escaped_dangerous_chars}>",
127+
tag.public_send(COMMON_DANGEROUS_CHARS.to_sym)
128+
129+
assert_equal "<#{COMMON_DANGEROUS_CHARS}></#{COMMON_DANGEROUS_CHARS}>",
130+
tag.public_send(COMMON_DANGEROUS_CHARS.to_sym, nil, escape: false)
131+
end
132+
133+
def test_tag_with_dangerous_aria_attribute_name
134+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
135+
assert_equal "<the-name aria-#{escaped_dangerous_chars}=\"the value\" />",
136+
tag("the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
137+
138+
assert_equal "<the-name aria-#{COMMON_DANGEROUS_CHARS}=\"the value\" />",
139+
tag("the-name", { aria: { COMMON_DANGEROUS_CHARS => "the value" } }, false, false)
140+
end
141+
142+
def test_tag_builder_with_dangerous_aria_attribute_name
143+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
144+
assert_equal "<the-name aria-#{escaped_dangerous_chars}=\"the value\"></the-name>",
145+
tag.public_send(:"the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" })
146+
147+
assert_equal "<the-name aria-#{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
148+
tag.public_send(:"the-name", aria: { COMMON_DANGEROUS_CHARS => "the value" }, escape: false)
149+
end
150+
151+
def test_tag_with_dangerous_data_attribute_name
152+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
153+
assert_equal "<the-name data-#{escaped_dangerous_chars}=\"the value\" />",
154+
tag("the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
155+
156+
assert_equal "<the-name data-#{COMMON_DANGEROUS_CHARS}=\"the value\" />",
157+
tag("the-name", { data: { COMMON_DANGEROUS_CHARS => "the value" } }, false, false)
158+
end
159+
160+
def test_tag_builder_with_dangerous_data_attribute_name
161+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
162+
assert_equal "<the-name data-#{escaped_dangerous_chars}=\"the value\"></the-name>",
163+
tag.public_send(:"the-name", data: { COMMON_DANGEROUS_CHARS => "the value" })
164+
165+
assert_equal "<the-name data-#{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
166+
tag.public_send(:"the-name", data: { COMMON_DANGEROUS_CHARS => "the value" }, escape: false)
167+
end
168+
169+
def test_tag_with_dangerous_unknown_attribute_name
170+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
171+
assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\" />",
172+
tag("the-name", COMMON_DANGEROUS_CHARS => "the value")
173+
174+
assert_equal "<the-name #{COMMON_DANGEROUS_CHARS}=\"the value\" />",
175+
tag("the-name", { COMMON_DANGEROUS_CHARS => "the value" }, false, false)
176+
end
177+
178+
def test_tag_builder_with_dangerous_unknown_attribute_name
179+
escaped_dangerous_chars = "_" * COMMON_DANGEROUS_CHARS.size
180+
assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\"></the-name>",
181+
tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")
182+
183+
assert_equal "<the-name #{COMMON_DANGEROUS_CHARS}=\"the value\"></the-name>",
184+
tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value", escape: false)
185+
end
186+
114187
def test_content_tag
115188
assert_equal "<a href=\"create\">Create</a>", content_tag("a", "Create", "href" => "create")
116189
assert_predicate content_tag("a", "Create", "href" => "create"), :html_safe?
@@ -130,10 +203,54 @@ def test_tag_builder_with_content
130203
assert_equal "<p>&lt;script&gt;evil_js&lt;/script&gt;</p>",
131204
tag.p("<script>evil_js</script>")
132205
assert_equal "<p><script>evil_js</script></p>",
133-
tag.p("<script>evil_js</script>", escape_attributes: false)
206+
tag.p("<script>evil_js</script>", escape: false)
134207
assert_equal '<input pattern="\w+">', tag.input(pattern: /\w+/)
135208
end
136209

210+
def test_deprecated_option_escape_attributes
211+
raw_content = "<script>evil_js</script>"
212+
escaped_content = "&lt;script&gt;evil_js&lt;/script&gt;"
213+
214+
expected_deprecation_warning = <<~MSG
215+
Use of the option :escape_attributes is deprecated. It currently \
216+
escapes both names and values of tags and attributes and it is \
217+
equivalent to :escape. If any of them are enabled, the escaping \
218+
is fully enabled.
219+
MSG
220+
221+
assert_equal "<p>#{escaped_content}</p>",
222+
tag.p(raw_content)
223+
assert_equal "<p>#{escaped_content}</p>",
224+
tag.p(raw_content, escape: true)
225+
assert_equal "<p>#{raw_content}</p>",
226+
tag.p(raw_content, escape: false)
227+
228+
assert_deprecated(expected_deprecation_warning) do
229+
assert_equal "<p>#{escaped_content}</p>",
230+
tag.p(raw_content, escape_attributes: true)
231+
end
232+
assert_deprecated(expected_deprecation_warning) do
233+
assert_equal "<p>#{raw_content}</p>",
234+
tag.p(raw_content, escape_attributes: false)
235+
end
236+
assert_deprecated(expected_deprecation_warning) do
237+
assert_equal "<p>#{escaped_content}</p>",
238+
tag.p(raw_content, escape_attributes: true, escape: true)
239+
end
240+
assert_deprecated(expected_deprecation_warning) do
241+
assert_equal "<p>#{raw_content}</p>",
242+
tag.p(raw_content, escape_attributes: false, escape: false)
243+
end
244+
assert_deprecated(expected_deprecation_warning) do
245+
assert_equal "<p>#{escaped_content}</p>",
246+
tag.p(raw_content, escape_attributes: true, escape: false)
247+
end
248+
assert_deprecated(expected_deprecation_warning) do
249+
assert_equal "<p>#{escaped_content}</p>",
250+
tag.p(raw_content, escape_attributes: false, escape: true)
251+
end
252+
end
253+
137254
def test_tag_builder_nested
138255
assert_equal "<div>content</div>",
139256
tag.div { "content" }
@@ -246,10 +363,10 @@ def test_content_tag_with_unescaped_array_class
246363
end
247364

248365
def test_tag_builder_with_unescaped_array_class
249-
str = tag.p "limelight", class: ["song", "play>"], escape_attributes: false
366+
str = tag.p "limelight", class: ["song", "play>"], escape: false
250367
assert_equal "<p class=\"song play>\">limelight</p>", str
251368

252-
str = tag.p "limelight", class: ["song", ["play>"]], escape_attributes: false
369+
str = tag.p "limelight", class: ["song", ["play>"]], escape: false
253370
assert_equal "<p class=\"song play>\">limelight</p>", str
254371
end
255372

@@ -268,7 +385,7 @@ def test_content_tag_with_unescaped_empty_array_class
268385
end
269386

270387
def test_tag_builder_with_unescaped_empty_array_class
271-
str = tag.p "limelight", class: [], escape_attributes: false
388+
str = tag.p "limelight", class: [], escape: false
272389
assert_equal '<p class="">limelight</p>', str
273390
end
274391

@@ -339,10 +456,10 @@ def test_content_tag_with_unescaped_conditional_hash_classes
339456
end
340457

341458
def test_tag_builder_with_unescaped_conditional_hash_classes
342-
str = tag.p "limelight", class: { "song": true, "play>": true }, escape_attributes: false
459+
str = tag.p "limelight", class: { "song": true, "play>": true }, escape: false
343460
assert_equal "<p class=\"song play>\">limelight</p>", str
344461

345-
str = tag.p "limelight", class: ["song", { "play>": true }], escape_attributes: false
462+
str = tag.p "limelight", class: ["song", { "play>": true }], escape: false
346463
assert_equal "<p class=\"song play>\">limelight</p>", str
347464
end
348465

@@ -456,11 +573,11 @@ def test_disable_escaping
456573
end
457574

458575
def test_tag_builder_disable_escaping
459-
assert_equal '<a href="&amp;"></a>', tag.a(href: "&amp;", escape_attributes: false)
460-
assert_equal '<a href="&amp;">cnt</a>', tag.a(href: "&amp;", escape_attributes: false) { "cnt" }
461-
assert_equal '<br data-hidden="&amp;">', tag.br("data-hidden": "&amp;", escape_attributes: false)
462-
assert_equal '<a href="&amp;">content</a>', tag.a("content", href: "&amp;", escape_attributes: false)
463-
assert_equal '<a href="&amp;">content</a>', tag.a(href: "&amp;", escape_attributes: false) { "content" }
576+
assert_equal '<a href="&amp;"></a>', tag.a(href: "&amp;", escape: false)
577+
assert_equal '<a href="&amp;">cnt</a>', tag.a(href: "&amp;", escape: false) { "cnt" }
578+
assert_equal '<br data-hidden="&amp;">', tag.br("data-hidden": "&amp;", escape: false)
579+
assert_equal '<a href="&amp;">content</a>', tag.a("content", href: "&amp;", escape: false)
580+
assert_equal '<a href="&amp;">content</a>', tag.a(href: "&amp;", escape: false) { "content" }
464581
end
465582

466583
def test_data_attributes

‎activesupport/CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,10 @@
1+
* Fix and add protections for XSS in `ActionView::Helpers` and `ERB::Util`.
2+
3+
Add the method `ERB::Util.xml_name_escape` to escape dangerous characters
4+
in names of tags and names of attributes, following the specification of XML.
5+
6+
*Álvaro Martín Fraguas*
7+
18
## Rails 7.0.2.3 (March 08, 2022) ##
29

310
* No changes.

‎activesupport/lib/active_support/core_ext/string/output_safety.rb

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,14 @@ module Util
1111
HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+)|(#[xX][\dA-Fa-f]+));)/
1212
JSON_ESCAPE_REGEXP = /[\u2028\u2029&><]/u
1313

14+
# Following XML requirements: https://www.w3.org/TR/REC-xml/#NT-Name
15+
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}" \
16+
"\u{200C}-\u{200D}\u{2070}-\u{218F}\u{2C00}-\u{2FEF}\u{3001}-\u{D7FF}\u{F900}-\u{FDCF}" \
17+
"\u{FDF0}-\u{FFFD}\u{10000}-\u{EFFFF}"
18+
TAG_NAME_START_REGEXP = /[^#{TAG_NAME_START_REGEXP_SET}]/
19+
TAG_NAME_FOLLOWING_REGEXP = /[^#{TAG_NAME_START_REGEXP_SET}\-.0-9\u{B7}\u{0300}-\u{036F}\u{203F}-\u{2040}]/
20+
TAG_NAME_REPLACEMENT_CHAR = "_"
21+
1422
# A utility method for escaping HTML tag characters.
1523
# This method is also aliased as <tt>h</tt>.
1624
#
@@ -115,6 +123,26 @@ def json_escape(s)
115123
end
116124

117125
module_function :json_escape
126+
127+
# A utility method for escaping XML names of tags and names of attributes.
128+
#
129+
# xml_name_escape('1 < 2 & 3')
130+
# # => "1___2___3"
131+
#
132+
# It follows the requirements of the specification: https://www.w3.org/TR/REC-xml/#NT-Name
133+
def xml_name_escape(name)
134+
name = name.to_s
135+
return "" if name.blank?
136+
137+
starting_char = name[0].gsub(TAG_NAME_START_REGEXP, TAG_NAME_REPLACEMENT_CHAR)
138+
139+
return starting_char if name.size == 1
140+
141+
following_chars = name[1..-1].gsub(TAG_NAME_FOLLOWING_REGEXP, TAG_NAME_REPLACEMENT_CHAR)
142+
143+
starting_char + following_chars
144+
end
145+
module_function :xml_name_escape
118146
end
119147
end
120148

0 commit comments

Comments
 (0)