From 9441acff1a867f672449fef0ecf8908016652d51 Mon Sep 17 00:00:00 2001 From: Andrew Carpenter Date: Thu, 28 Jul 2016 16:12:21 -0700 Subject: [PATCH] ensure tag/content_tag escapes " in attribute vals Many helpers mark content as HTML-safe without escaping double quotes -- including `sanitize`. Regardless of whether or not the attribute values are HTML-escaped, we want to be sure they don't include double quotes, as that can cause XSS issues. For example: `content_tag(:div, "foo", title: sanitize('" onmouseover="alert(1);//'))` CVE-2016-6316 --- actionpack/lib/action_view/helpers/tag_helper.rb | 16 ++++++++++++---- actionpack/test/template/tag_helper_test.rb | 10 ++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/actionpack/lib/action_view/helpers/tag_helper.rb b/actionpack/lib/action_view/helpers/tag_helper.rb index 7f58a27..6b659a7 100644 --- a/actionpack/lib/action_view/helpers/tag_helper.rb +++ b/actionpack/lib/action_view/helpers/tag_helper.rb @@ -10,6 +10,7 @@ module ActionView module TagHelper extend ActiveSupport::Concern include CaptureHelper + include OutputSafetyHelper BOOLEAN_ATTRIBUTES = %w(disabled readonly multiple checked autobuffer autoplay controls loop selected hidden scoped async @@ -141,20 +142,27 @@ module ActionView unless v.is_a?(String) || v.is_a?(Symbol) || v.is_a?(BigDecimal) v = v.to_json end - v = ERB::Util.html_escape(v) if escape - attrs << %(data-#{k.to_s.dasherize}="#{v}") + attrs << tag_option("data-#{k.to_s.dasherize}", v, escape) end elsif BOOLEAN_ATTRIBUTES.include?(key) attrs << %(#{key}="#{key}") if value elsif !value.nil? final_value = value.is_a?(Array) ? value.join(" ") : value - final_value = ERB::Util.html_escape(final_value) if escape - attrs << %(#{key}="#{final_value}") + attrs << tag_option(key, value, escape) end end " #{attrs.sort * ' '}".html_safe unless attrs.empty? end end + + def tag_option(key, value, escape) + if value.is_a?(Array) + value = escape ? safe_join(value, " ") : value.join(" ") + else + value = escape ? ERB::Util.html_escape(value) : value + end + %(#{key}="#{value.gsub(/"/, '"'.freeze)}") + end end end end diff --git a/actionpack/test/template/tag_helper_test.rb b/actionpack/test/template/tag_helper_test.rb index e362955..9c3d636 100644 --- a/actionpack/test/template/tag_helper_test.rb +++ b/actionpack/test/template/tag_helper_test.rb @@ -101,6 +101,16 @@ class TagHelperTest < ActionView::TestCase end end + def test_tag_does_not_honor_html_safe_double_quotes_as_attributes + assert_dom_equal '

content

', + content_tag('p', "content", title: '"'.html_safe) + end + + def test_data_tag_does_not_honor_html_safe_double_quotes_as_attributes + assert_dom_equal '

content

', + content_tag('p', "content", data: { title: '"'.html_safe }) + end + def test_skip_invalid_escaped_attributes ['&1;', 'dfa3;', '& #123;'].each do |escaped| assert_equal %(), tag('a', :href => escaped) -- 2.8.1