diff options
-rw-r--r-- | actionpack/lib/action_dispatch/http/request.rb | 7 | ||||
-rw-r--r-- | actionpack/lib/action_dispatch/middleware/params_parser.rb | 4 | ||||
-rw-r--r-- | actionpack/test/controller/webservice_test.rb | 13 | ||||
-rw-r--r-- | actionpack/test/dispatch/request/json_params_parsing_test.rb | 15 | ||||
-rw-r--r-- | actionpack/test/dispatch/request/xml_params_parsing_test.rb | 17 | ||||
-rw-r--r-- | activerecord/test/cases/relation/where_test.rb | 6 | ||||
-rw-r--r-- | activesupport/CHANGELOG.md | 7 | ||||
-rw-r--r-- | activesupport/lib/active_support/core_ext/hash/conversions.rb | 27 | ||||
-rw-r--r-- | activesupport/test/core_ext/hash_ext_test.rb | 28 |
9 files changed, 15 insertions, 109 deletions
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb index 31b36eedb0..452809a689 100644 --- a/actionpack/lib/action_dispatch/http/request.rb +++ b/actionpack/lib/action_dispatch/http/request.rb @@ -276,14 +276,15 @@ module ActionDispatch LOCALHOST =~ remote_addr && LOCALHOST =~ remote_ip end + protected + # Remove nils from the params hash def deep_munge(hash) - hash.each do |k, v| + hash.each_value do |v| case v when Array v.grep(Hash) { |x| deep_munge(x) } v.compact! - hash[k] = nil if v.empty? when Hash deep_munge(v) end @@ -292,8 +293,6 @@ module ActionDispatch hash end - protected - def parse_query(qs) deep_munge(super) end diff --git a/actionpack/lib/action_dispatch/middleware/params_parser.rb b/actionpack/lib/action_dispatch/middleware/params_parser.rb index 951f28f535..2c98ca03a8 100644 --- a/actionpack/lib/action_dispatch/middleware/params_parser.rb +++ b/actionpack/lib/action_dispatch/middleware/params_parser.rb @@ -47,12 +47,12 @@ module ActionDispatch when Proc strategy.call(request.raw_post) when :xml_simple, :xml_node - data = request.deep_munge(Hash.from_xml(request.body.read) || {}) + data = Hash.from_xml(request.raw_post) || {} data.with_indifferent_access when :yaml YAML.load(request.raw_post) when :json - data = request.deep_munge ActiveSupport::JSON.decode(request.body) + data = ActiveSupport::JSON.decode(request.raw_post) data = {:_json => data} unless data.is_a?(Hash) data.with_indifferent_access else diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index 2602540fbe..c0b9833603 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -116,19 +116,6 @@ class WebServiceTest < ActionDispatch::IntegrationTest end end - def test_post_xml_using_a_disallowed_type_attribute - $stderr = StringIO.new - with_test_route_set do - post '/', '<foo type="symbol">value</foo>', 'CONTENT_TYPE' => 'application/xml' - assert_response 500 - - post '/', '<foo type="yaml">value</foo>', 'CONTENT_TYPE' => 'application/xml' - assert_response 500 - end - ensure - $stderr = STDERR - end - def test_register_and_use_yaml with_test_route_set do with_params_parsers Mime::YAML => Proc.new { |d| YAML.load(d) } do diff --git a/actionpack/test/dispatch/request/json_params_parsing_test.rb b/actionpack/test/dispatch/request/json_params_parsing_test.rb index 2c4a6c2147..c0c3147e37 100644 --- a/actionpack/test/dispatch/request/json_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/json_params_parsing_test.rb @@ -30,21 +30,6 @@ class JsonParamsParsingTest < ActionDispatch::IntegrationTest ) end - test "nils are stripped from collections" do - assert_parses( - {"person" => nil}, - "{\"person\":[null]}", { 'CONTENT_TYPE' => 'application/json' } - ) - assert_parses( - {"person" => ['foo']}, - "{\"person\":[\"foo\",null]}", { 'CONTENT_TYPE' => 'application/json' } - ) - assert_parses( - {"person" => nil}, - "{\"person\":[null, null]}", { 'CONTENT_TYPE' => 'application/json' } - ) - end - test "logs error if parsing unsuccessful" do with_test_routing do output = StringIO.new diff --git a/actionpack/test/dispatch/request/xml_params_parsing_test.rb b/actionpack/test/dispatch/request/xml_params_parsing_test.rb index f13b64a3c7..cb68667002 100644 --- a/actionpack/test/dispatch/request/xml_params_parsing_test.rb +++ b/actionpack/test/dispatch/request/xml_params_parsing_test.rb @@ -30,23 +30,6 @@ class XmlParamsParsingTest < ActionDispatch::IntegrationTest assert_equal "<ok>bar</ok>", resp.body end - def assert_parses(expected, xml) - with_test_routing do - post "/parse", xml, default_headers - assert_response :ok - assert_equal(expected, TestController.last_request_parameters) - end - end - - test "nils are stripped from collections" do - assert_parses( - {"hash" => { "person" => nil} }, - "<hash><person type=\"array\"><person nil=\"true\"/></person></hash>") - assert_parses( - {"hash" => { "person" => ['foo']} }, - "<hash><person type=\"array\"><person>foo</person><person nil=\"true\"/></person>\n</hash>") - end - test "parses hash params" do with_test_routing do xml = "<person><name>David</name></person>" diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb index d1c3690478..297e865308 100644 --- a/activerecord/test/cases/relation/where_test.rb +++ b/activerecord/test/cases/relation/where_test.rb @@ -90,12 +90,6 @@ module ActiveRecord [[], {}, nil, ""].each do |blank| assert_equal 4, Edge.where(blank).order("sink_id").to_a.size end - def test_where_with_table_name_and_empty_array - assert_equal 0, Post.where(:id => []).count - end - - def test_where_with_empty_hash_and_no_foreign_key - assert_equal 0, Edge.where(:sink => {}).count end end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 5848f9712f..08bec2f4ae 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,12 +1,5 @@ ## Rails 4.0.0 (unreleased) ## -* Hash.from_xml raises when it encounters type="symbol" or type="yaml". - Use Hash.from_trusted_xml to parse this XML. - - CVE-2013-0156 - - *Jeremy Kemper* - * Deprecate `assert_present` and `assert_blank` in favor of `assert object.blank?` and `assert object.present?` diff --git a/activesupport/lib/active_support/core_ext/hash/conversions.rb b/activesupport/lib/active_support/core_ext/hash/conversions.rb index 8930376ac8..6cb7434e5f 100644 --- a/activesupport/lib/active_support/core_ext/hash/conversions.rb +++ b/activesupport/lib/active_support/core_ext/hash/conversions.rb @@ -101,33 +101,17 @@ class Hash # # hash = Hash.from_xml(xml) # # => {"hash"=>{"foo"=>1, "bar"=>2}} - # - # DisallowedType is raise if the XML contains attributes with <tt>type="yaml"</tt> or - # <tt>type="symbol"</tt>. Use <tt>Hash.from_trusted_xml</tt> to parse this XML. - def from_xml(xml, disallowed_types = nil) - ActiveSupport::XMLConverter.new(xml, disallowed_types).to_h + def from_xml(xml) + ActiveSupport::XMLConverter.new(xml).to_h end - # Builds a Hash from XML just like <tt>Hash.from_xml</tt>, but also allows Symbol and YAML. - def from_trusted_xml(xml) - from_xml xml, [] - end end end module ActiveSupport class XMLConverter # :nodoc: - class DisallowedType < StandardError - def initialize(type) - super "Disallowed type attribute: #{type.inspect}" - end - end - - DISALLOWED_TYPES = %w(symbol yaml) - - def initialize(xml, disallowed_types = nil) + def initialize(xml) @xml = normalize_keys(XmlMini.parse(xml)) - @disallowed_types = disallowed_types || DISALLOWED_TYPES end def to_h @@ -135,6 +119,7 @@ module ActiveSupport end private + def normalize_keys(params) case params when Hash @@ -160,10 +145,6 @@ module ActiveSupport end def process_hash(value) - if value.include?('type') && !value['type'].is_a?(Hash) && @disallowed_types.include?(value['type']) - raise DisallowedType, value['type'] - end - if become_array?(value) _, entries = Array.wrap(value.detect { |k,v| not v.is_a?(String) }) if entries.nil? || value['__content__'].try(:empty?) diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb index 30d95b75bc..84dd9bc983 100644 --- a/activesupport/test/core_ext/hash_ext_test.rb +++ b/activesupport/test/core_ext/hash_ext_test.rb @@ -1015,10 +1015,12 @@ class HashToXmlTest < ActiveSupport::TestCase <replies-close-in type="integer">2592000000</replies-close-in> <written-on type="date">2003-07-16</written-on> <viewed-at type="datetime">2003-07-16T09:28:00+0000</viewed-at> + <content type="yaml">--- \n1: should be an integer\n:message: Have a nice day\narray: \n- should-have-dashes: true\n should_have_underscores: true\n</content> <author-email-address>david@loudthinking.com</author-email-address> <parent-id></parent-id> <ad-revenue type="decimal">1.5</ad-revenue> <optimum-viewing-angle type="float">135</optimum-viewing-angle> + <resident type="symbol">yes</resident> </topic> EOT @@ -1031,10 +1033,12 @@ class HashToXmlTest < ActiveSupport::TestCase :replies_close_in => 2592000000, :written_on => Date.new(2003, 7, 16), :viewed_at => Time.utc(2003, 7, 16, 9, 28), + :content => { :message => "Have a nice day", 1 => "should be an integer", "array" => [{ "should-have-dashes" => true, "should_have_underscores" => true }] }, :author_email_address => "david@loudthinking.com", :parent_id => nil, :ad_revenue => BigDecimal("1.50"), :optimum_viewing_angle => 135.0, + :resident => :yes }.stringify_keys assert_equal expected_topic_hash, Hash.from_xml(topic_xml)["topic"] @@ -1048,6 +1052,7 @@ class HashToXmlTest < ActiveSupport::TestCase <approved type="boolean"></approved> <written-on type="date"></written-on> <viewed-at type="datetime"></viewed-at> + <content type="yaml"></content> <parent-id></parent-id> </topic> EOT @@ -1058,6 +1063,7 @@ class HashToXmlTest < ActiveSupport::TestCase :approved => nil, :written_on => nil, :viewed_at => nil, + :content => nil, :parent_id => nil }.stringify_keys @@ -1284,28 +1290,6 @@ class HashToXmlTest < ActiveSupport::TestCase assert_equal expected_product_hash, Hash.from_xml(product_xml)["product"] end - def test_from_xml_raises_on_disallowed_type_attributes - assert_raise ActiveSupport::XMLConverter::DisallowedType do - Hash.from_xml '<product><name type="foo">value</name></product>', %w(foo) - end - end - - def test_from_xml_disallows_symbol_and_yaml_types_by_default - assert_raise ActiveSupport::XMLConverter::DisallowedType do - Hash.from_xml '<product><name type="symbol">value</name></product>' - end - - assert_raise ActiveSupport::XMLConverter::DisallowedType do - Hash.from_xml '<product><name type="yaml">value</name></product>' - end - end - - def test_from_trusted_xml_allows_symbol_and_yaml_types - expected = { 'product' => { 'name' => :value }} - assert_equal expected, Hash.from_trusted_xml('<product><name type="symbol">value</name></product>') - assert_equal expected, Hash.from_trusted_xml('<product><name type="yaml">:value</name></product>') - end - def test_should_use_default_value_for_unknown_key hash_wia = HashWithIndifferentAccess.new(3) assert_equal 3, hash_wia[:new_key] |