From 25215d7285db10e2c04d903f251b791342e4dd6a Mon Sep 17 00:00:00 2001 From: wycats Date: Sun, 27 Jun 2010 21:12:10 -0700 Subject: Fix several known web encoding issues: * Specify accept-charset on all forms. All recent browsers, as well as IE5+, will use the encoding specified for form parameters * Unfortunately, IE5+ will not look at accept-charset unless at least one character in the form's values is not in the page's charset. Since the user can override the default charset (which Rails sets to UTF-8), we provide a hidden input containing a unicode character, forcing IE to look at the accept-charset. * Now that the vast majority of web input is UTF-8, we set the inbound parameters to UTF-8. This will eliminate many cases of incompatible encodings between ASCII-8BIT and UTF-8. * You can safely ignore params[:_snowman_] TODO: * Validate inbound text to confirm it is UTF-8 * Combine the whole_form implementations in form_helper_test and form_tag_helper_test --- actionpack/lib/action_dispatch/http/parameters.rb | 31 +- .../lib/action_view/helpers/form_tag_helper.rb | 15 +- .../request/url_encoded_params_parsing_test.rb | 23 ++ actionpack/test/template/erb/form_for_test.rb | 2 +- actionpack/test/template/erb/tag_helper_test.rb | 4 +- actionpack/test/template/form_helper_test.rb | 385 +++++++++++---------- actionpack/test/template/form_tag_helper_test.rb | 56 ++- 7 files changed, 318 insertions(+), 198 deletions(-) diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb index 0a37bd7fc1..add8cab2ab 100644 --- a/actionpack/lib/action_dispatch/http/parameters.rb +++ b/actionpack/lib/action_dispatch/http/parameters.rb @@ -6,7 +6,11 @@ module ActionDispatch module Parameters # Returns both GET and POST \parameters in a single hash. def parameters - @env["action_dispatch.request.parameters"] ||= request_parameters.merge(query_parameters).update(path_parameters).with_indifferent_access + @env["action_dispatch.request.parameters"] ||= begin + params = request_parameters.merge(query_parameters) + params.merge!(path_parameters) + encode_params(params).with_indifferent_access + end end alias :params :parameters @@ -32,6 +36,31 @@ module ActionDispatch end private + + # TODO: Validate that the characters are UTF-8. If they aren't, + # you'll get a weird error down the road, but our form handling + # should really prevent that from happening + def encode_params(params) + return params unless "ruby".encoding_aware? + + if params.is_a?(String) + return params.force_encoding("UTF-8").encode! + elsif !params.is_a?(Hash) + return params + end + + params.each do |k, v| + case v + when Hash + encode_params(v) + when Array + v.map! {|el| encode_params(el) } + else + encode_params(v) + end + end + end + # Convert nested Hash to HashWithIndifferentAccess def normalize_parameters(value) case value diff --git a/actionpack/lib/action_view/helpers/form_tag_helper.rb b/actionpack/lib/action_view/helpers/form_tag_helper.rb index ea491b2db8..0e9cb2349f 100644 --- a/actionpack/lib/action_view/helpers/form_tag_helper.rb +++ b/actionpack/lib/action_view/helpers/form_tag_helper.rb @@ -530,22 +530,31 @@ module ActionView returning options.stringify_keys do |html_options| html_options["enctype"] = "multipart/form-data" if html_options.delete("multipart") html_options["action"] = url_for(url_for_options, *parameters_for_url) + html_options["accept-encoding"] = "UTF-8" html_options["data-remote"] = true if html_options.delete("remote") end end def extra_tags_for_form(html_options) - case method = html_options.delete("method").to_s + snowman_tag = tag(:input, :type => "hidden", + :name => "_snowman_", :value => "☃") + + method = html_options.delete("method").to_s + + method_tag = case method when /^get$/i # must be case-insensitive, but can't use downcase as might be nil html_options["method"] = "get" '' when /^post$/i, "", nil html_options["method"] = "post" - protect_against_forgery? ? content_tag(:div, token_tag, :style => 'margin:0;padding:0;display:inline') : '' + token_tag else html_options["method"] = "post" - content_tag(:div, tag(:input, :type => "hidden", :name => "_method", :value => method) + token_tag, :style => 'margin:0;padding:0;display:inline') + tag(:input, :type => "hidden", :name => "_method", :value => method) + token_tag end + + tags = snowman_tag << method_tag + content_tag(:div, tags, :style => 'margin:0;padding:0;display:inline') end def form_tag_html(html_options) diff --git a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb index 69dbd7f528..0bcef81534 100644 --- a/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb @@ -141,6 +141,29 @@ class UrlEncodedParamsParsingTest < ActionController::IntegrationTest post "/parse", actual assert_response :ok assert_equal(expected, TestController.last_request_parameters) + assert_utf8(TestController.last_request_parameters) + end + end + + def assert_utf8(object) + return unless "ruby".encoding_aware? + + correct_encoding = Encoding.default_internal + + unless object.is_a?(Hash) + assert_equal correct_encoding, object.encoding, "#{object.inspect} should have been UTF-8" + return + end + + object.each do |k,v| + case v + when Hash + assert_utf8(v) + when Array + v.each {|el| assert_utf8(el) } + else + assert_utf8(v) + end end end end diff --git a/actionpack/test/template/erb/form_for_test.rb b/actionpack/test/template/erb/form_for_test.rb index ec6e872735..e722b40a9a 100644 --- a/actionpack/test/template/erb/form_for_test.rb +++ b/actionpack/test/template/erb/form_for_test.rb @@ -5,7 +5,7 @@ module ERBTest class TagHelperTest < BlockTestCase test "form_for works" do output = render_content "form_for(:staticpage, :url => {:controller => 'blah', :action => 'update'})", "" - assert_equal "
", output + assert_match %r{.*}, output end end end diff --git a/actionpack/test/template/erb/tag_helper_test.rb b/actionpack/test/template/erb/tag_helper_test.rb index 64a88bde1d..d073100986 100644 --- a/actionpack/test/template/erb/tag_helper_test.rb +++ b/actionpack/test/template/erb/tag_helper_test.rb @@ -28,8 +28,8 @@ module ERBTest end test "percent equals works with form tags" do - expected_output = "
hello
" - maybe_deprecated { assert_equal expected_output, render_content("form_tag('foo')", "<%= 'hello' %>") } + expected_output = %r{.*hello*} + maybe_deprecated { assert_match expected_output, render_content("form_tag('foo')", "<%= 'hello' %>") } end test "percent equals works with fieldset tags" do diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index 6ba407e230..eb6cf92805 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -583,7 +583,8 @@ class FormHelperTest < ActionView::TestCase end expected = - "
" + + "" + + snowman + "" + "" + "" + @@ -604,15 +605,14 @@ class FormHelperTest < ActionView::TestCase concat f.submit('Create post') end - expected = - "" + - "
" + + expected = whole_form("/posts/123", "create-post", "other_name_edit", :method => "put") do "" + "" + "" + "" + "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -626,14 +626,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + - "
" + + expected = whole_form("http://www.example.com", "create-post", nil, "put") do "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -647,14 +645,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + - "
" + + expected = whole_form("http://www.example.com", "create-post", nil, :method => "put", :remote => true) do "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -668,13 +664,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + + expected = whole_form("http://www.example.com", nil, nil, :remote => true) do "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -686,13 +681,12 @@ class FormHelperTest < ActionView::TestCase concat f.check_box(:secret) end - expected = - "
" + + expected = whole_form("http://www.example.com", "create-post") do "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -707,14 +701,13 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + + expected = whole_form do "" + "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -728,13 +721,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + + expected = whole_form do "" + "" + "" + - "" + - "
" + "" + end assert_dom_equal expected, output_buffer end @@ -749,9 +741,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end + assert_dom_equal expected, output_buffer ensure I18n.locale = old_locale @@ -766,9 +759,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end + assert_dom_equal expected, output_buffer ensure I18n.locale = old_locale @@ -781,9 +775,10 @@ class FormHelperTest < ActionView::TestCase concat f.submit :class => "extra" end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end + assert_dom_equal expected, output_buffer ensure I18n.locale = old_locale @@ -798,9 +793,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end + assert_dom_equal expected, output_buffer ensure I18n.locale = old_locale @@ -815,9 +811,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -832,10 +828,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "" + - "
" + expected = whole_form do + "" + + "" + end assert_dom_equal expected, output_buffer end @@ -850,10 +846,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "" + - "
" + expected = whole_form do + "" + + "" + end assert_dom_equal expected, output_buffer end @@ -867,9 +863,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -883,9 +879,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -899,9 +895,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -915,9 +911,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -931,9 +927,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = "
" + - "" + - "
" + expected = whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -952,12 +948,11 @@ class FormHelperTest < ActionView::TestCase } end - expected = "
" + - "" + - "
" + - "
" + - "" + - "
" + expected = whole_form do + "" + end + whole_form do + "" + end assert_dom_equal expected, output_buffer end @@ -975,10 +970,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1006,11 +1001,11 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1028,11 +1023,11 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1051,13 +1046,13 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1077,13 +1072,13 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1102,11 +1097,11 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1125,12 +1120,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1145,9 +1140,9 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '
' + expected = whole_form do + '' + end assert_dom_equal expected, output_buffer end @@ -1164,13 +1159,13 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1188,13 +1183,13 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1213,12 +1208,12 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer assert_equal yielded_comments, @post.comments @@ -1235,10 +1230,10 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1273,20 +1268,20 @@ class FormHelperTest < ActionView::TestCase end end - expected = '
' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '' + - '
' + expected = whole_form do + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + + '' + end assert_dom_equal expected, output_buffer end @@ -1426,7 +1421,8 @@ class FormHelperTest < ActionView::TestCase end expected = - "
" + + "" + + snowman + "" + "" + "" + @@ -1449,11 +1445,11 @@ class FormHelperTest < ActionView::TestCase end expected = - "" + - "" + - "" + - "" + - "
" + whole_form("http://www.example.com", "create-post") do + "" + + "" + + "" + end assert_dom_equal expected, output_buffer end @@ -1477,16 +1473,42 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + - "
" + - "
" + - "
" + - "
" + expected = whole_form do + "
" + + "
" + + "
" + end assert_dom_equal expected, output_buffer end + def snowman(method = nil) + txt = %{
} + txt << %{} + txt << %{} if method + txt << %{
} + end + + def form_text(action = "http://www.example.com", id = nil, html_class = nil, remote = nil) + txt = %{
} + end + + def whole_form(action = "http://www.example.com", id = nil, html_class = nil, options = nil) + contents = block_given? ? yield : "" + + if options.is_a?(Hash) + method, remote = options.values_at(:method, :remote) + else + method = options + end + + form_text(action, id, html_class, remote) + snowman(method) + contents + "
" + end + def test_default_form_builder old_default_form_builder, ActionView::Base.default_form_builder = ActionView::Base.default_form_builder, LabelledFormBuilder @@ -1499,12 +1521,11 @@ class FormHelperTest < ActionView::TestCase end end - expected = - "
" + + expected = whole_form do "
" + "
" + - "
" + - "
" + "
" + end assert_dom_equal expected, output_buffer ensure @@ -1577,7 +1598,7 @@ class FormHelperTest < ActionView::TestCase assert_deprecated do form_for(:post, @post, :html => {:id => 'some_form', :class => 'some_class'}) do |f| end end - expected = "
" + expected = whole_form("http://www.example.com", "some_form", "some_class") assert_dom_equal expected, output_buffer end @@ -1587,7 +1608,8 @@ class FormHelperTest < ActionView::TestCase form_for(:post, @post, :url => 'http://www.otherdomain.com') do |f| end end - assert_equal '
', output_buffer + assert_equal whole_form("http://www.otherdomain.com"), output_buffer + # assert_equal '
', output_buffer end def test_form_for_with_hash_url_option @@ -1604,14 +1626,15 @@ class FormHelperTest < ActionView::TestCase form_for(:post, @post, :url => @post) do |f| end end - expected = "
" + expected = whole_form("/posts/123") + # expected = "
" assert_equal expected, output_buffer end def test_form_for_with_existing_object form_for(@post) do |f| end - expected = "
" + expected = whole_form("/posts/123", "edit_post_123", "edit_post", "put") assert_equal expected, output_buffer end @@ -1622,7 +1645,7 @@ class FormHelperTest < ActionView::TestCase form_for(post) do |f| end - expected = "
" + expected = whole_form("/posts", "new_post", "new_post") assert_equal expected, output_buffer end @@ -1630,14 +1653,14 @@ class FormHelperTest < ActionView::TestCase @comment.save form_for([@post, @comment]) {} - expected = %(
) + expected = whole_form(comment_path(@post, @comment), "edit_comment_1", "edit_comment", "put") assert_dom_equal expected, output_buffer end def test_form_for_with_new_object_in_list form_for([@post, @comment]) {} - expected = %(
) + expected = whole_form(comments_path(@post), "new_comment", "new_comment") assert_dom_equal expected, output_buffer end @@ -1645,21 +1668,21 @@ class FormHelperTest < ActionView::TestCase @comment.save form_for([:admin, @post, @comment]) {} - expected = %(
) + expected = whole_form(admin_comment_path(@post, @comment), "edit_comment_1", "edit_comment", "put") assert_dom_equal expected, output_buffer end def test_form_for_with_new_object_and_namespace_in_list form_for([:admin, @post, @comment]) {} - expected = %(
) + expected = whole_form(admin_comments_path(@post), "new_comment", "new_comment") assert_dom_equal expected, output_buffer end def test_form_for_with_existing_object_and_custom_url form_for(@post, :url => "/super_posts") do |f| end - expected = "
" + expected = whole_form("/super_posts", "edit_post_123", "edit_post", "put") assert_equal expected, output_buffer end diff --git a/actionpack/test/template/form_tag_helper_test.rb b/actionpack/test/template/form_tag_helper_test.rb index b75863bb6a..75d2773c0a 100644 --- a/actionpack/test/template/form_tag_helper_test.rb +++ b/actionpack/test/template/form_tag_helper_test.rb @@ -8,6 +8,36 @@ class FormTagHelperTest < ActionView::TestCase @controller = BasicController.new end + def snowman(options = {}) + method = options[:method] + + txt = %{
} + txt << %{} + txt << %{} if method + txt << %{
} + end + + def form_text(action = "http://www.example.com", options = {}) + remote, enctype, html_class, id = options.values_at(:remote, :enctype, :html_class, :id) + + txt = %{
} + end + + def whole_form(action = "http://www.example.com", options = {}) + out = form_text(action, options) + snowman(options) + + if block_given? + out << yield << "
" + end + + out + end + def url_for(options) if options.is_a?(Hash) "http://www.example.com" @@ -31,51 +61,57 @@ class FormTagHelperTest < ActionView::TestCase def test_form_tag actual = form_tag - expected = %(
) + expected = whole_form assert_dom_equal expected, actual end def test_form_tag_multipart actual = form_tag({}, { 'multipart' => true }) - expected = %() + expected = whole_form("http://www.example.com", :enctype => true) assert_dom_equal expected, actual end def test_form_tag_with_method_put actual = form_tag({}, { :method => :put }) - expected = %(
) + expected = whole_form("http://www.example.com", :method => :put) assert_dom_equal expected, actual end def test_form_tag_with_method_delete actual = form_tag({}, { :method => :delete }) - expected = %(
) + + expected = whole_form("http://www.example.com", :method => :delete) assert_dom_equal expected, actual end def test_form_tag_with_remote actual = form_tag({}, :remote => true) - expected = %() + + expected = whole_form("http://www.example.com", :remote => true) assert_dom_equal expected, actual end def test_form_tag_with_remote_false actual = form_tag({}, :remote => false) - expected = %() + + expected = whole_form assert_dom_equal expected, actual end def test_form_tag_with_block_in_erb - output_buffer = form_tag("http://example.com") { concat "Hello world!" } + output_buffer = form_tag("http://www.example.com") { concat "Hello world!" } - expected = %(Hello world!
) + expected = whole_form { "Hello world!" } assert_dom_equal expected, output_buffer end def test_form_tag_with_block_and_method_in_erb - output_buffer = form_tag("http://example.com", :method => :put) { concat "Hello world!" } + output_buffer = form_tag("http://www.example.com", :method => :put) { concat "Hello world!" } + + expected = whole_form("http://www.example.com", :method => "put") do + "Hello world!" + end - expected = %(
Hello world!
) assert_dom_equal expected, output_buffer end -- cgit v1.2.3