Skip to content

Commit

Permalink
html_escape should escape single quotes
Browse files Browse the repository at this point in the history 
https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.231_-_HTML_Escape_Before_Inserting_Untrusted_Data_into_HTML_Element_Content
Closes #7215

Conflicts:
	actionpack/test/controller/new_base/render_template_test.rb
	actionpack/test/template/asset_tag_helper_test.rb
	actionpack/test/template/erb_util_test.rb
	actionpack/test/template/javascript_helper_test.rb
	actionpack/test/template/template_test.rb
	activesupport/lib/active_support/core_ext/string/output_safety.rb
	activesupport/test/core_ext/string_ext_test.rb
	railties/test/application/assets_test.rb
  • Loading branch information
@spastorino @tenderlove
spastorino authored and tenderlove committed Aug 8, 2012
1 parent f07c708 commit 780a718
Show file tree
Hide file tree
Showing 10 changed files with 64 additions and 35 deletions.
4 changes: 2 additions & 2 deletions actionpack/test/controller/render_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def render_text_hello_world

# :ported:
def render_text_hello_world_with_layout
@variable_for_layout = ", I'm here!"
@variable_for_layout = ", I am here!"
render :text => "hello world", :layout => true
end

Expand Down Expand Up @@ -776,7 +776,7 @@ def test_render_text
# :ported:
def test_do_with_render_text_and_layout
get :render_text_hello_world_with_layout
assert_equal "<html>hello world, I'm here!</html>", @response.body
assert_equal "<html>hello world, I am here!</html>", @response.body
end

# :ported:
Expand Down
32 changes: 22 additions & 10 deletions actionpack/test/template/asset_tag_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,11 @@ def url_for(*args)
%(image_tag("slash..png")) => %(<img alt="Slash." src="/images/slash..png" />),
%(image_tag(".pdf.png")) => %(<img alt=".pdf" src="/images/.pdf.png" />),
%(image_tag("http://www.rubyonrails.com/images/rails.png")) => %(<img alt="Rails" src="http://www.rubyonrails.com/images/rails.png" />),
%(image_tag("mouse.png", :mouseover => "/images/mouse_over.png")) => %(<img alt="Mouse" onmouseover="this.src='/images/mouse_over.png'" onmouseout="this.src='/images/mouse.png'" src="/images/mouse.png" />),
%(image_tag("mouse.png", :mouseover => image_path("mouse_over.png"))) => %(<img alt="Mouse" onmouseover="this.src='/images/mouse_over.png'" onmouseout="this.src='/images/mouse.png'" src="/images/mouse.png" />),
%(image_tag("mouse.png", :alt => nil)) => %(<img src="/images/mouse.png" />)
%(image_tag("//www.rubyonrails.com/images/rails.png")) => %(<img alt="Rails" src="//www.rubyonrails.com/images/rails.png" />),
%(image_tag("mouse.png", :mouseover => "/images/mouse_over.png")) => %(<img alt="Mouse" onmouseover="this.src=&#x27;/images/mouse_over.png&#x27;" onmouseout="this.src=&#x27;/images/mouse.png&#x27;" src="/images/mouse.png" />),
%(image_tag("mouse.png", :mouseover => image_path("mouse_over.png"))) => %(<img alt="Mouse" onmouseover="this.src=&#x27;/images/mouse_over.png&#x27;" onmouseout="this.src=&#x27;/images/mouse.png&#x27;" src="/images/mouse.png" />),
%(image_tag("mouse.png", :alt => nil)) => %(<img src="/images/mouse.png" />),
%(image_tag("data:image/gif;base64,R0lGODlhAQABAID/AMDAwAAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==", :alt => nil)) => %(<img src="data:image/gif;base64,R0lGODlhAQABAID/AMDAwAAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==" />),
}

FaviconLinkToTag = {
Expand Down Expand Up @@ -1007,8 +1009,8 @@ def test_should_compute_proper_path
assert_dom_equal(%(/collaboration/hieraki/javascripts/xmlhr.js), javascript_path("xmlhr"))
assert_dom_equal(%(/collaboration/hieraki/stylesheets/style.css), stylesheet_path("style"))
assert_dom_equal(%(/collaboration/hieraki/images/xml.png), image_path("xml.png"))
assert_dom_equal(%(<img alt="Mouse" onmouseover="this.src='/collaboration/hieraki/images/mouse_over.png'" onmouseout="this.src='/collaboration/hieraki/images/mouse.png'" src="/collaboration/hieraki/images/mouse.png" />), image_tag("mouse.png", :mouseover => "/images/mouse_over.png"))
assert_dom_equal(%(<img alt="Mouse2" onmouseover="this.src='/collaboration/hieraki/images/mouse_over2.png'" onmouseout="this.src='/collaboration/hieraki/images/mouse2.png'" src="/collaboration/hieraki/images/mouse2.png" />), image_tag("mouse2.png", :mouseover => image_path("mouse_over2.png")))
assert_dom_equal(%(<img alt="Mouse" onmouseover="this.src=&#x27;/collaboration/hieraki/images/mouse_over.png&#x27;" onmouseout="this.src=&#x27;/collaboration/hieraki/images/mouse.png&#x27;" src="/collaboration/hieraki/images/mouse.png" />), image_tag("mouse.png", :mouseover => "/images/mouse_over.png"))
assert_dom_equal(%(<img alt="Mouse2" onmouseover="this.src=&#x27;/collaboration/hieraki/images/mouse_over2.png&#x27;" onmouseout="this.src=&#x27;/collaboration/hieraki/images/mouse2.png&#x27;" src="/collaboration/hieraki/images/mouse2.png" />), image_tag("mouse2.png", :mouseover => image_path("mouse_over2.png")))
end

def test_should_ignore_relative_root_path_on_complete_url
Expand All @@ -1018,11 +1020,21 @@ def test_should_ignore_relative_root_path_on_complete_url
def test_should_compute_proper_path_with_asset_host
@controller.config.asset_host = "http://assets.example.com"
assert_dom_equal(%(<link href="http://www.example.com/collaboration/hieraki" rel="alternate" title="RSS" type="application/rss+xml" />), auto_discovery_link_tag)
assert_dom_equal(%(http://assets.example.com/collaboration/hieraki/javascripts/xmlhr.js), javascript_path("xmlhr"))
assert_dom_equal(%(http://assets.example.com/collaboration/hieraki/stylesheets/style.css), stylesheet_path("style"))
assert_dom_equal(%(http://assets.example.com/collaboration/hieraki/images/xml.png), image_path("xml.png"))
assert_dom_equal(%(<img alt="Mouse" onmouseover="this.src='http://assets.example.com/collaboration/hieraki/images/mouse_over.png'" onmouseout="this.src='http://assets.example.com/collaboration/hieraki/images/mouse.png'" src="http://assets.example.com/collaboration/hieraki/images/mouse.png" />), image_tag("mouse.png", :mouseover => "/images/mouse_over.png"))
assert_dom_equal(%(<img alt="Mouse2" onmouseover="this.src='http://assets.example.com/collaboration/hieraki/images/mouse_over2.png'" onmouseout="this.src='http://assets.example.com/collaboration/hieraki/images/mouse2.png'" src="http://assets.example.com/collaboration/hieraki/images/mouse2.png" />), image_tag("mouse2.png", :mouseover => image_path("mouse_over2.png")))
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/javascripts/xmlhr.js), javascript_path("xmlhr"))
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/stylesheets/style.css), stylesheet_path("style"))
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/images/xml.png), image_path("xml.png"))
assert_dom_equal(%(<img alt="Mouse" onmouseover="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse_over.png&#x27;" onmouseout="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse.png&#x27;" src="gopher://assets.example.com/collaboration/hieraki/images/mouse.png" />), image_tag("mouse.png", :mouseover => "/images/mouse_over.png"))
assert_dom_equal(%(<img alt="Mouse2" onmouseover="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse_over2.png&#x27;" onmouseout="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse2.png&#x27;" src="gopher://assets.example.com/collaboration/hieraki/images/mouse2.png" />), image_tag("mouse2.png", :mouseover => image_path("mouse_over2.png")))
end

def test_should_compute_proper_path_with_asset_host_and_default_protocol
@controller.config.asset_host = "assets.example.com"
@controller.config.default_asset_host_protocol = :request
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/javascripts/xmlhr.js), javascript_path("xmlhr"))
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/stylesheets/style.css), stylesheet_path("style"))
assert_dom_equal(%(gopher://assets.example.com/collaboration/hieraki/images/xml.png), image_path("xml.png"))
assert_dom_equal(%(<img alt="Mouse" onmouseover="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse_over.png&#x27;" onmouseout="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse.png&#x27;" src="gopher://assets.example.com/collaboration/hieraki/images/mouse.png" />), image_tag("mouse.png", :mouseover => "/images/mouse_over.png"))
assert_dom_equal(%(<img alt="Mouse2" onmouseover="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse_over2.png&#x27;" onmouseout="this.src=&#x27;gopher://assets.example.com/collaboration/hieraki/images/mouse2.png&#x27;" src="gopher://assets.example.com/collaboration/hieraki/images/mouse2.png" />), image_tag("mouse2.png", :mouseover => image_path("mouse_over2.png")))
end

def test_should_ignore_asset_host_on_complete_url
Expand Down
10 changes: 5 additions & 5 deletions actionpack/test/template/erb_util_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ class ErbUtilTest < Test::Unit::TestCase
define_method "test_html_escape_#{expected.gsub(/\W/, '')}" do
assert_equal expected, html_escape(given)
end
end

unless given == '"'
define_method "test_json_escape_#{expected.gsub(/\W/, '')}" do
assert_equal ERB::Util::JSON_ESCAPE[given], json_escape(given)
end
ERB::Util::JSON_ESCAPE.each do |given, expected|
define_method "test_json_escape_#{expected.gsub(/\W/, '')}" do
assert_equal ERB::Util::JSON_ESCAPE[given], json_escape(given)
end
end

Expand Down Expand Up @@ -39,7 +39,7 @@ def test_html_escape_passes_html_escpe_unmodified

def test_rest_in_ascii
(0..127).to_a.map {|int| int.chr }.each do |chr|
next if %w(& " < >).include?(chr)
next if %w(& " < > ').include?(chr)
assert_equal chr, html_escape(chr)
end
end
Expand Down
6 changes: 3 additions & 3 deletions actionpack/test/template/form_options_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ def test_time_zone_select_with_default_time_zone_and_value

def test_options_for_select_with_element_attributes
assert_dom_equal(
"<option value=\"&lt;Denmark&gt;\" class=\"bold\">&lt;Denmark&gt;</option>\n<option value=\"USA\" onclick=\"alert('Hello World');\">USA</option>\n<option value=\"Sweden\">Sweden</option>\n<option value=\"Germany\">Germany</option>",
"<option value=\"&lt;Denmark&gt;\" class=\"bold\">&lt;Denmark&gt;</option>\n<option value=\"USA\" onclick=\"alert(&#x27;Hello World&#x27;);\">USA</option>\n<option value=\"Sweden\">Sweden</option>\n<option value=\"Germany\">Germany</option>",
options_for_select([ [ "<Denmark>", { :class => 'bold' } ], [ "USA", { :onclick => "alert('Hello World');" } ], [ "Sweden" ], "Germany" ])
)
end
Expand Down Expand Up @@ -923,13 +923,13 @@ def test_option_html_attributes_with_single_element_hash
def test_option_html_attributes_with_multiple_element_hash
output = option_html_attributes([ 'foo', 'bar', { :class => 'fancy', 'onclick' => "alert('Hello World');" } ])
assert output.include?(" class=\"fancy\"")
assert output.include?(" onclick=\"alert('Hello World');\"")
assert output.include?(" onclick=\"alert(&#x27;Hello World&#x27;);\"")
end

def test_option_html_attributes_with_multiple_hashes
output = option_html_attributes([ 'foo', 'bar', { :class => 'fancy' }, { 'onclick' => "alert('Hello World');" } ])
assert output.include?(" class=\"fancy\"")
assert output.include?(" onclick=\"alert('Hello World');\"")
assert output.include?(" onclick=\"alert(&#x27;Hello World&#x27;);\"")
end

def test_option_html_attributes_with_special_characters
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/template/form_tag_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ def test_stringify_symbol_keys

def test_submit_tag
assert_dom_equal(
%(<input name='commit' data-disable-with="Saving..." onclick="alert('hello!')" type="submit" value="Save" />),
%(<input name='commit' data-disable-with="Saving..." onclick="alert(&#x27;hello!&#x27;)" type="submit" value="Save" />),
submit_tag("Save", :disable_with => "Saving...", :onclick => "alert('hello!')")
)
end
Expand Down
10 changes: 5 additions & 5 deletions actionpack/test/template/javascript_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def test_escape_javascript_with_safebuffer
end

def test_button_to_function
assert_dom_equal %(<input type="button" onclick="alert('Hello world!');" value="Greeting" />),
assert_dom_equal %(<input type="button" onclick="alert(&#x27;Hello world!&#x27;);" value="Greeting" />),
button_to_function("Greeting", "alert('Hello world!')")
end

Expand All @@ -60,7 +60,7 @@ def test_button_to_function_with_rjs_block_and_options
end

def test_button_to_function_with_onclick
assert_dom_equal "<input onclick=\"alert('Goodbye World :('); alert('Hello world!');\" type=\"button\" value=\"Greeting\" />",
assert_dom_equal "<input onclick=\"alert(&#x27;Goodbye World :(&#x27;); alert(&#x27;Hello world!&#x27;);\" type=\"button\" value=\"Greeting\" />",
button_to_function("Greeting", "alert('Hello world!')", :onclick => "alert('Goodbye World :(')")
end

Expand All @@ -70,12 +70,12 @@ def test_button_to_function_without_function
end

def test_link_to_function
assert_dom_equal %(<a href="#" onclick="alert('Hello world!'); return false;">Greeting</a>),
assert_dom_equal %(<a href="#" onclick="alert(&#x27;Hello world!&#x27;); return false;">Greeting</a>),
link_to_function("Greeting", "alert('Hello world!')")
end

def test_link_to_function_with_existing_onclick
assert_dom_equal %(<a href="#" onclick="confirm('Sanity!'); alert('Hello world!'); return false;">Greeting</a>),
assert_dom_equal %(<a href="#" onclick="confirm(&#x27;Sanity!&#x27;); alert(&#x27;Hello world!&#x27;); return false;">Greeting</a>),
link_to_function("Greeting", "alert('Hello world!')", :onclick => "confirm('Sanity!')")
end

Expand All @@ -94,7 +94,7 @@ def test_link_to_function_with_rjs_block_and_options
end

def test_link_to_function_with_href
assert_dom_equal %(<a href="http://example.com/" onclick="alert('Hello world!'); return false;">Greeting</a>),
assert_dom_equal %(<a href="http://example.com/" onclick="alert(&#x27;Hello world!&#x27;); return false;">Greeting</a>),
link_to_function("Greeting", "alert('Hello world!')", :href => 'http://example.com/')
end

Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/template/template_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def test_basic_template

def test_locals
@template = new_template("<%= my_local %>")
assert_equal "I'm a local", render(:my_local => "I'm a local")
assert_equal "I am a local", render(:my_local => "I am a local")
end

def test_restores_buffer
Expand Down
10 changes: 5 additions & 5 deletions actionpack/test/template/url_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def test_link_with_nil_html_options

def test_link_tag_with_custom_onclick
link = link_to("Hello", "http://www.example.com", :onclick => "alert('yay!')")
expected = %{<a href="http://www.example.com" onclick="alert('yay!')">Hello</a>}
expected = %{<a href="http://www.example.com" onclick="alert(&#x27;yay!&#x27;)">Hello</a>}
assert_dom_equal expected, link
end

Expand All @@ -198,12 +198,12 @@ def test_link_tag_with_javascript_confirm
link_to("Hello", "http://www.example.com", :confirm => "Are you sure?")
)
assert_dom_equal(
"<a href=\"http://www.example.com\" data-confirm=\"You can't possibly be sure, can you?\">Hello</a>",
link_to("Hello", "http://www.example.com", :confirm => "You can't possibly be sure, can you?")
"<a href=\"http://www.example.com\" data-confirm=\"You cant possibly be sure, can you?\">Hello</a>",
link_to("Hello", "http://www.example.com", :confirm => "You cant possibly be sure, can you?")
)
assert_dom_equal(
"<a href=\"http://www.example.com\" data-confirm=\"You can't possibly be sure,\n can you?\">Hello</a>",
link_to("Hello", "http://www.example.com", :confirm => "You can't possibly be sure,\n can you?")
"<a href=\"http://www.example.com\" data-confirm=\"You cant possibly be sure,\n can you?\">Hello</a>",
link_to("Hello", "http://www.example.com", :confirm => "You cant possibly be sure,\n can you?")
)
end

Expand Down
6 changes: 3 additions & 3 deletions activesupport/lib/active_support/core_ext/string/output_safety.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

class ERB
module Util
HTML_ESCAPE = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;' }
HTML_ESCAPE = { '&' => '&amp;', '>' => '&gt;', '<' => '&lt;', '"' => '&quot;', "'" => '&#x27;' }

This comment has been minimized.

Sign in to view

Copy link
@jshraibman-mdsol

jshraibman-mdsol Jan 30, 2013

Was this necessary to fix CVE-2013-0333? All of a sudden strings with single quotes stuck into input boxes are being doubly escaped, like so:

<input id="regime_name" name="regime[name]" size="30" type="text" value="with an &amp;mpersand and a &amp;#x27;quote&amp;#x27;" />

This comment has been minimized.

Sign in to view

Copy link
@jshraibman-mdsol

jshraibman-mdsol Jan 30, 2013

Existing unit tests fail. When I run rails/actionpack/test/template/form_helper_test.rb I get:


  1) Failure:
test_default_form_builder_no_instance_variable(FormHelperTest) [/Users/jshraibman/work/rails/actionpack/lib/action_controller/test_case.rb:119]:
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can't be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can't be empty</li></ul></div></form>"> expected but was
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can&#x27;t be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can&#x27;t be empty</li></ul></div></form>">.

  2) Failure:
test_default_form_builder_with_active_record_helpers(FormHelperTest) [/Users/jshraibman/work/rails/actionpack/lib/action_controller/test_case.rb:119]:
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can't be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can't be empty</li></ul></div></form>"> expected but was
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can&#x27;t be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can&#x27;t be empty</li></ul></div></form>">.

  3) Failure:
test_default_form_builder_without_object(FormHelperTest) [/Users/jshraibman/work/rails/actionpack/lib/action_controller/test_case.rb:119]:
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can't be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can't be empty</li></ul></div></form>"> expected but was
<"<form action=\"http://www.example.com\" method=\"post\"><div class=\"formError\">can&#x27;t be empty</div><div class=\"errorExplanation\" id=\"errorExplanation\"><h2>1 error prohibited this post from being saved</h2><p>There were problems with the following fields:</p><ul><li>Author name can&#x27;t be empty</li></ul></div></form>">.

96 tests, 134 assertions, 3 failures, 0 errors, 0 skips
JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' }

# A utility method for escaping HTML tag characters.
# This method is also aliased as <tt>h</tt>.
#
# In your ERb templates, use this method to escape any unsafe content. For example:
# In your ERB templates, use this method to escape any unsafe content. For example:
# <%=h @person.name %>
#
# ==== Example:
Expand All @@ -20,7 +20,7 @@ def html_escape(s)
if s.html_safe?
s
else
s.to_s.gsub(/&/, "&amp;").gsub(/\"/, "&quot;").gsub(/>/, "&gt;").gsub(/</, "&lt;").html_safe
s.gsub(/[&"'><]/n) { |special| HTML_ESCAPE[special] }.html_safe
end
end

Expand Down
17 changes: 17 additions & 0 deletions activesupport/test/core_ext/string_ext_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,23 @@ def to_s
assert string.html_safe?
assert !string.to_param.html_safe?
end

test "ERB::Util.html_escape should escape unsafe characters" do
string = '<>&"\''
expected = '&lt;&gt;&amp;&quot;&#x27;'
assert_equal expected, ERB::Util.html_escape(string)
end

test "ERB::Util.html_escape should correctly handle invalid UTF-8 strings" do
string = [192, 60].pack('CC')
expected = 192.chr + "&lt;"
assert_equal expected, ERB::Util.html_escape(string)
end

test "ERB::Util.html_escape should not escape safe strings" do
string = "<b>hello</b>".html_safe
assert_equal string, ERB::Util.html_escape(string)
end
end

class StringExcludeTest < ActiveSupport::TestCase
Expand Down

0 comments on commit 780a718

Please sign in to comment.