From 9415935902f120a9bac0bfce7129725a0db38ed3 Mon Sep 17 00:00:00 2001 From: Michael Koziarski Date: Thu, 8 Oct 2009 09:31:20 +1300 Subject: Switch to on-by-default XSS escaping for rails. This consists of: * String#html_safe! a method to mark a string as 'safe' * ActionView::SafeBuffer a string subclass which escapes anything unsafe which is concatenated to it * Calls to String#html_safe! throughout the rails helpers * a 'raw' helper which lets you concatenate trusted HTML from non-safety-aware sources (e.g. presantized strings in the DB) * New ERB implementation based on erubis which uses a SafeBuffer instead of a String Hat tip to Django for the inspiration. --- actionpack/test/controller/output_escaping_test.rb | 19 ++++++++++ actionpack/test/controller/render_test.rb | 2 +- actionpack/test/template/asset_tag_helper_test.rb | 12 +++++++ actionpack/test/template/erb_util_test.rb | 12 +++++++ actionpack/test/template/form_helper_test.rb | 2 +- actionpack/test/template/raw_output_helper_test.rb | 21 +++++++++++ actionpack/test/template/render_test.rb | 2 +- actionpack/test/template/sanitize_helper_test.rb | 11 +++++- actionpack/test/template/tag_helper_test.rb | 1 + actionpack/test/template/test_case_test.rb | 2 +- actionpack/test/template/url_helper_test.rb | 2 +- actionpack/test/view/safe_buffer_test.rb | 41 ++++++++++++++++++++++ 12 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 actionpack/test/controller/output_escaping_test.rb create mode 100644 actionpack/test/template/raw_output_helper_test.rb create mode 100644 actionpack/test/view/safe_buffer_test.rb (limited to 'actionpack/test') diff --git a/actionpack/test/controller/output_escaping_test.rb b/actionpack/test/controller/output_escaping_test.rb new file mode 100644 index 0000000000..7332f3f1e3 --- /dev/null +++ b/actionpack/test/controller/output_escaping_test.rb @@ -0,0 +1,19 @@ +require 'abstract_unit' + +class OutputEscapingTest < ActiveSupport::TestCase + + test "escape_html shouldn't die when passed nil" do + assert ERB::Util.h(nil).blank? + end + + test "escapeHTML should escape strings" do + assert_equal "<>"", ERB::Util.h("<>\"") + end + + test "escapeHTML shouldn't touch explicitly safe strings" do + # TODO this seems easier to compose and reason about, but + # this should be verified + assert_equal "<", ERB::Util.h("<".html_safe!) + end + +end diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index abcc8bf384..2db524ca4b 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -1050,7 +1050,7 @@ class RenderTest < ActionController::TestCase def test_action_talk_to_layout get :action_talk_to_layout - assert_equal "Talking to the layout\nAction was here!", @response.body + assert_equal "Talking to the layout\n\nAction was here!", @response.body end # :addressed: diff --git a/actionpack/test/template/asset_tag_helper_test.rb b/actionpack/test/template/asset_tag_helper_test.rb index 83fc6a282c..d94135b04b 100644 --- a/actionpack/test/template/asset_tag_helper_test.rb +++ b/actionpack/test/template/asset_tag_helper_test.rb @@ -231,6 +231,11 @@ class AssetTagHelperTest < ActionView::TestCase assert_dom_equal(%(\n\n\n\n), javascript_include_tag(:defaults)) end + def test_javascript_include_tag_is_html_safe + assert javascript_include_tag(:defaults).html_safe? + assert javascript_include_tag("prototype").html_safe? + end + def test_register_javascript_include_default ENV["RAILS_ASSET_ID"] = "" ActionView::Helpers::AssetTagHelper::register_javascript_include_default 'bank' @@ -285,6 +290,13 @@ class AssetTagHelperTest < ActionView::TestCase } end + def test_stylesheet_link_tag_is_html_safe + ENV["RAILS_ASSET_ID"] = "" + assert stylesheet_link_tag('dir/file').html_safe? + assert stylesheet_link_tag('dir/other/file', 'dir/file2').html_safe? + assert stylesheet_tag('dir/file', {}).html_safe? + end + def test_custom_stylesheet_expansions ENV["RAILS_ASSET_ID"] = '' ActionView::Helpers::AssetTagHelper::register_stylesheet_expansion :robbery => ["bank", "robber"] diff --git a/actionpack/test/template/erb_util_test.rb b/actionpack/test/template/erb_util_test.rb index 49f51c50c5..fa6b263965 100644 --- a/actionpack/test/template/erb_util_test.rb +++ b/actionpack/test/template/erb_util_test.rb @@ -15,6 +15,18 @@ class ErbUtilTest < Test::Unit::TestCase end end + def test_html_escape_is_html_safe + escaped = h("

") + assert_equal "<p>", escaped + assert escaped.html_safe? + end + + def test_html_escape_passes_html_escpe_unmodified + escaped = h("

".html_safe!) + assert_equal "

", escaped + assert escaped.html_safe? + end + def test_rest_in_ascii (0..127).to_a.map {|int| int.chr }.each do |chr| next if %w(& " < >).include?(chr) diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index 6a08c99619..04c635e770 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -974,7 +974,7 @@ class FormHelperTest < ActionView::TestCase (field_helpers - %w(hidden_field)).each do |selector| src = <<-END_SRC def #{selector}(field, *args, &proc) - " " + super + "
" + (" " + super + "
").html_safe! end END_SRC class_eval src, __FILE__, __LINE__ diff --git a/actionpack/test/template/raw_output_helper_test.rb b/actionpack/test/template/raw_output_helper_test.rb new file mode 100644 index 0000000000..598aa5b1d8 --- /dev/null +++ b/actionpack/test/template/raw_output_helper_test.rb @@ -0,0 +1,21 @@ +require 'abstract_unit' +require 'testing_sandbox' + +class RawOutputHelperTest < ActionView::TestCase + tests ActionView::Helpers::RawOutputHelper + include TestingSandbox + + def setup + @string = "hello" + end + + test "raw returns the safe string" do + result = raw(@string) + assert_equal @string, result + assert result.html_safe? + end + + test "raw handles nil values correctly" do + assert_equal "", raw(nil) + end +end \ No newline at end of file diff --git a/actionpack/test/template/render_test.rb b/actionpack/test/template/render_test.rb index 3c192906ae..35c51ca7ea 100644 --- a/actionpack/test/template/render_test.rb +++ b/actionpack/test/template/render_test.rb @@ -229,7 +229,7 @@ module RenderTestCases end def test_render_with_nested_layout - assert_equal %(title\n

column
\n
content
\n), + assert_equal %(title\n\n\n
column
\n
content
\n), @view.render(:file => "test/nested_layout.erb", :layout => "layouts/yield") end diff --git a/actionpack/test/template/sanitize_helper_test.rb b/actionpack/test/template/sanitize_helper_test.rb index f715071bbc..222d4dbf4c 100644 --- a/actionpack/test/template/sanitize_helper_test.rb +++ b/actionpack/test/template/sanitize_helper_test.rb @@ -39,7 +39,16 @@ class SanitizeHelperTest < ActionView::TestCase %{This is a test.\n\n\nIt no longer contains any HTML.\n}, strip_tags( %{This is <b>a <a href="" target="_blank">test</a></b>.\n\n\n\n

It no longer contains any HTML.

\n})) assert_equal "This has a here.", strip_tags("This has a here.") - [nil, '', ' '].each { |blank| assert_equal blank, strip_tags(blank) } + [nil, '', ' '].each do |blank| + stripped = strip_tags(blank) + assert_equal blank, stripped + assert stripped.html_safe? unless blank.nil? + end + assert strip_tags("").html_safe? end def assert_sanitized(text, expected = nil) diff --git a/actionpack/test/template/tag_helper_test.rb b/actionpack/test/template/tag_helper_test.rb index 2aa3d5b5fa..433f6514cf 100644 --- a/actionpack/test/template/tag_helper_test.rb +++ b/actionpack/test/template/tag_helper_test.rb @@ -34,6 +34,7 @@ class TagHelperTest < ActionView::TestCase def test_content_tag assert_equal "Create", content_tag("a", "Create", "href" => "create") + assert content_tag("a", "Create", "href" => "create").html_safe? assert_equal content_tag("a", "Create", "href" => "create"), content_tag("a", "Create", :href => "create") end diff --git a/actionpack/test/template/test_case_test.rb b/actionpack/test/template/test_case_test.rb index 5db42c4d68..ca72c13ffa 100644 --- a/actionpack/test/template/test_case_test.rb +++ b/actionpack/test/template/test_case_test.rb @@ -155,7 +155,7 @@ module ActionView class AssertionsTest < ActionView::TestCase def render_from_helper form_tag('/foo') do - concat render(:text => '') + concat render(:text => '').html_safe! end end helper_method :render_from_helper diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb index ce99482078..7f6ebc56b7 100644 --- a/actionpack/test/template/url_helper_test.rb +++ b/actionpack/test/template/url_helper_test.rb @@ -139,7 +139,7 @@ class UrlHelperTest < ActionView::TestCase end def test_link_tag_with_img - assert_dom_equal "", link_to("", "http://www.example.com") + assert_dom_equal "\"Favicon\"", link_to(image_tag("/favicon.jpg"), "http://www.example.com") end def test_link_with_nil_html_options diff --git a/actionpack/test/view/safe_buffer_test.rb b/actionpack/test/view/safe_buffer_test.rb new file mode 100644 index 0000000000..2236709627 --- /dev/null +++ b/actionpack/test/view/safe_buffer_test.rb @@ -0,0 +1,41 @@ +require 'abstract_unit' + +class SafeBufferTest < ActionView::TestCase + def setup + @buffer = ActionView::SafeBuffer.new + end + + test "Should look like a string" do + assert @buffer.is_a?(String) + assert_equal "", @buffer + end + + test "Should escape a raw string which is passed to them" do + @buffer << "