From b6ab4417720e03f1551abda2f1e4bd0a392dd04e Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 31 Jul 2012 22:25:54 -0300 Subject: [PATCH] html_escape should escape single quotes 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 --- .../test/controller/new_base/render_template_test.rb | 2 +- actionpack/test/controller/render_test.rb | 4 ++-- actionpack/test/template/erb_util_test.rb | 12 ++++++------ actionpack/test/template/form_options_helper_test.rb | 2 +- actionpack/test/template/form_tag_helper_test.rb | 2 +- actionpack/test/template/template_test.rb | 2 +- actionpack/test/template/text_helper_test.rb | 8 ++++---- actionpack/test/template/url_helper_test.rb | 18 +++++++++--------- .../active_support/core_ext/string/output_safety.rb | 6 +++--- activesupport/test/core_ext/string_ext_test.rb | 4 ++-- 10 files changed, 30 insertions(+), 30 deletions(-) diff --git a/actionpack/test/controller/new_base/render_template_test.rb b/actionpack/test/controller/new_base/render_template_test.rb index 156d87c..d0be4f6 100644 --- a/actionpack/test/controller/new_base/render_template_test.rb +++ b/actionpack/test/controller/new_base/render_template_test.rb @@ -126,7 +126,7 @@ module RenderTemplate test "rendering a template with error properly excerts the code" do get :with_error assert_status 500 - assert_match "undefined local variable or method `idontexist'", response.body + assert_match "undefined local variable or method `idontexist", response.body end end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 6bebe7e..3f047fc 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -186,7 +186,7 @@ class TestController < ActionController::Base # :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 @@ -844,7 +844,7 @@ class RenderTest < ActionController::TestCase # :ported: def test_do_with_render_text_and_layout get :render_text_hello_world_with_layout - assert_equal "hello world, I'm here!", @response.body + assert_equal "hello world, I am here!", @response.body end # :ported: diff --git a/actionpack/test/template/erb_util_test.rb b/actionpack/test/template/erb_util_test.rb index ca2710e..3d51024 100644 --- a/actionpack/test/template/erb_util_test.rb +++ b/actionpack/test/template/erb_util_test.rb @@ -8,11 +8,11 @@ class ErbUtilTest < ActiveSupport::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 @@ -40,13 +40,13 @@ class ErbUtilTest < ActiveSupport::TestCase def test_rest_in_ascii (0..127).to_a.map {|int| int.chr }.each do |chr| - next if chr.in?('&"<>') + next if chr.in?('&"<>\'') assert_equal chr, html_escape(chr) end end def test_html_escape_once - assert_equal '1 < 2 & 3', html_escape_once('1 < 2 & 3') + assert_equal '1 <>&"' 2 & 3', html_escape_once('1 <>&"\' 2 & 3') end def test_html_escape_once_returns_unsafe_strings_when_passed_unsafe_strings diff --git a/actionpack/test/template/form_options_helper_test.rb b/actionpack/test/template/form_options_helper_test.rb index bfc7317..43d4937 100644 --- a/actionpack/test/template/form_options_helper_test.rb +++ b/actionpack/test/template/form_options_helper_test.rb @@ -1125,7 +1125,7 @@ class FormOptionsHelperTest < ActionView::TestCase def test_options_for_select_with_element_attributes assert_dom_equal( - "\n\n\n", + "\n\n\n", options_for_select([ [ "", { :class => 'bold' } ], [ "USA", { :onclick => "alert('Hello World');" } ], [ "Sweden" ], "Germany" ]) ) end diff --git a/actionpack/test/template/form_tag_helper_test.rb b/actionpack/test/template/form_tag_helper_test.rb index 9afa4a2..6c791e0 100644 --- a/actionpack/test/template/form_tag_helper_test.rb +++ b/actionpack/test/template/form_tag_helper_test.rb @@ -374,7 +374,7 @@ class FormTagHelperTest < ActionView::TestCase def test_submit_tag assert_dom_equal( - %(), + %(), submit_tag("Save", :onclick => "alert('hello!')", :data => { :disable_with => "Saving..." }) ) end diff --git a/actionpack/test/template/template_test.rb b/actionpack/test/template/template_test.rb index 322bea3..061f5bb 100644 --- a/actionpack/test/template/template_test.rb +++ b/actionpack/test/template/template_test.rb @@ -84,7 +84,7 @@ class TestERBTemplate < ActiveSupport::TestCase def test_locals @template = new_template("<%= my_local %>") @template.locals = [: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 diff --git a/actionpack/test/template/text_helper_test.rb b/actionpack/test/template/text_helper_test.rb index a3ab091..75ec1d8 100644 --- a/actionpack/test/template/text_helper_test.rb +++ b/actionpack/test/template/text_helper_test.rb @@ -107,8 +107,8 @@ class TextHelperTest < ActionView::TestCase end def test_truncate_with_link_options - assert_equal "Here's a long test and I...Continue", - truncate("Here's a long test and I need a continue to read link", :length => 27) { link_to 'Continue', '#' } + assert_equal "Here is a long test and ...Continue", + truncate("Here is a long test and I need a continue to read link", :length => 27) { link_to 'Continue', '#' } end def test_truncate_should_be_html_safe @@ -149,8 +149,8 @@ class TextHelperTest < ActionView::TestCase end def test_truncate_with_block_should_escape_the_block - assert_equal "Here's a long test and I...<script>alert('foo');</script>", - truncate("Here's a long test and I need a continue to read link", :length => 27) { "" } + assert_equal "Here is a long test and ...<script>" + ERB::Util.html_escape("alert('foo');") + "</script>", + truncate("Here is a long test and I need a continue to read link", :length => 27) { "" } end def test_highlight_should_be_html_safe diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index cb6f378..2c67b22 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -244,7 +244,7 @@ class UrlHelperTest < ActiveSupport::TestCase def test_link_tag_with_custom_onclick link = link_to("Hello", "http://www.example.com", :onclick => "alert('yay!')") - expected = %{Hello} + expected = %{Hello} assert_dom_equal expected, link end @@ -254,12 +254,12 @@ class UrlHelperTest < ActiveSupport::TestCase link_to("Hello", "http://www.example.com", :data => { :confirm => "Are you sure?" }) ) assert_dom_equal( - "Hello", - link_to("Hello", "http://www.example.com", :data => { :confirm => "You can't possibly be sure, can you?" }) + "Hello", + link_to("Hello", "http://www.example.com", :data => { :confirm => "You cant possibly be sure, can you?" }) ) assert_dom_equal( - "Hello", - link_to("Hello", "http://www.example.com", :data => { :confirm => "You can't possibly be sure,\n can you?" }) + "Hello", + link_to("Hello", "http://www.example.com", :data => { :confirm => "You cant possibly be sure,\n can you?" }) ) end @@ -272,14 +272,14 @@ class UrlHelperTest < ActiveSupport::TestCase end assert_deprecated ":confirm option is deprecated and will be removed from Rails 4.1. Use ':data => { :confirm => \'Text\' }' instead" do assert_dom_equal( - "Hello", - link_to("Hello", "http://www.example.com", :confirm => "You can't possibly be sure, can you?") + "Hello", + link_to("Hello", "http://www.example.com", :confirm => "You cant possibly be sure, can you?") ) end assert_deprecated ":confirm option is deprecated and will be removed from Rails 4.1. Use ':data => { :confirm => \'Text\' }' instead" do assert_dom_equal( - "Hello", - link_to("Hello", "http://www.example.com", :confirm => "You can't possibly be sure,\n can you?") + "Hello", + link_to("Hello", "http://www.example.com", :confirm => "You cant possibly be sure,\n can you?") ) end end 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 5226ff0..c17d695 100644 --- a/activesupport/lib/active_support/core_ext/string/output_safety.rb +++ b/activesupport/lib/active_support/core_ext/string/output_safety.rb @@ -3,9 +3,9 @@ require 'active_support/core_ext/kernel/singleton_class' class ERB module Util - HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"' } + HTML_ESCAPE = { '&' => '&', '>' => '>', '<' => '<', '"' => '"', "'" => ''' } JSON_ESCAPE = { '&' => '\u0026', '>' => '\u003E', '<' => '\u003C' } - HTML_ESCAPE_ONCE_REGEXP = /[\"><]|&(?!([a-zA-Z]+|(#\d+));)/ + HTML_ESCAPE_ONCE_REGEXP = /["><']|&(?!([a-zA-Z]+|(#\d+));)/ JSON_ESCAPE_REGEXP = /[&"><]/ # A utility method for escaping HTML tag characters. @@ -21,7 +21,7 @@ class ERB if s.html_safe? s else - s.encode(s.encoding, :xml => :attr)[1...-1].html_safe + s.gsub(/[&"'><]/, HTML_ESCAPE).html_safe end end diff --git a/activesupport/test/core_ext/string_ext_test.rb b/activesupport/test/core_ext/string_ext_test.rb index e5b7744..3b08ebe 100644 --- a/activesupport/test/core_ext/string_ext_test.rb +++ b/activesupport/test/core_ext/string_ext_test.rb @@ -498,8 +498,8 @@ class OutputSafetyTest < ActiveSupport::TestCase end test "ERB::Util.html_escape should escape unsafe characters" do - string = '<>&"' - expected = '<>&"' + string = '<>&"\'' + expected = '<>&"'' assert_equal expected, ERB::Util.html_escape(string) end -- 1.7.11.1