From c31cc963daac55f6a3bca9da99b619276911dbd7 Mon Sep 17 00:00:00 2001 From: Jeremy Kemper Date: Sat, 5 Jan 2013 17:46:26 -0700 Subject: Revert "Merge branch 'master-sec'" This reverts commit 88cc1688d0cb828c17706b41a8bd27870f2a2beb, reversing changes made to f049016cd348627bf8db0d72382d7580bf802a79. --- actionpack/lib/action_dispatch/http/request.rb | 7 +++--- .../action_dispatch/middleware/params_parser.rb | 4 ++-- actionpack/test/controller/webservice_test.rb | 13 ---------- .../dispatch/request/json_params_parsing_test.rb | 15 ------------ .../dispatch/request/xml_params_parsing_test.rb | 17 ------------- activerecord/test/cases/relation/where_test.rb | 6 ----- activesupport/CHANGELOG.md | 7 ------ .../active_support/core_ext/hash/conversions.rb | 27 ++++----------------- 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 '/', 'value', 'CONTENT_TYPE' => 'application/xml' - assert_response 500 - - post '/', 'value', '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 "bar", 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} }, - "") - assert_parses( - {"hash" => { "person" => ['foo']} }, - "foo\n") - end - test "parses hash params" do with_test_routing do xml = "David" 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 type="yaml" or - # type="symbol". Use Hash.from_trusted_xml 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 Hash.from_xml, 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 2592000000 2003-07-16 2003-07-16T09:28:00+0000 + --- \n1: should be an integer\n:message: Have a nice day\narray: \n- should-have-dashes: true\n should_have_underscores: true\n david@loudthinking.com 1.5 135 + yes 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 + 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 'value', %w(foo) - end - end - - def test_from_xml_disallows_symbol_and_yaml_types_by_default - assert_raise ActiveSupport::XMLConverter::DisallowedType do - Hash.from_xml 'value' - end - - assert_raise ActiveSupport::XMLConverter::DisallowedType do - Hash.from_xml 'value' - end - end - - def test_from_trusted_xml_allows_symbol_and_yaml_types - expected = { 'product' => { 'name' => :value }} - assert_equal expected, Hash.from_trusted_xml('value') - assert_equal expected, Hash.from_trusted_xml(':value') - end - def test_should_use_default_value_for_unknown_key hash_wia = HashWithIndifferentAccess.new(3) assert_equal 3, hash_wia[:new_key] -- cgit v1.2.3