From 31448f2b7fa6f3920485229e5710d9fcf87f190d Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Mon, 1 Aug 2016 21:41:46 +0200 Subject: Make Parameters support legacy YAML encodings. By changing ActionController::Parameter's superclass, Rails 5 also changed the YAML serialization format. Since YAML doesn't know how to handle parameters it would fallback to its routine for the superclass, which in Rails 4.2 was Hash while just Object in Rails 5. As evident in the tags YAML would spit out: 4.2: !ruby/hash-with-ivars:ActionController::Parameters 5.0: !ruby/object:ActionController::Parameters Thus when loading parameters YAML from 4.2 in Rails 5, it would parse a hash dump as it would an Object class. To fix this we have to provide our own `init_with` to be aware of the past format as well as the new one. Then we add a `load_tags` mapping, such that when the YAML parser sees `!ruby/hash-with-ivars:ActionController::Parameters`, it knows to call our `init_with` function and not try to instantiate it as a normal hash subclass. --- .../action_controller/metal/strong_parameters.rb | 19 +++++++++++++ .../controller/parameters/serialization_test.rb | 32 ++++++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 actionpack/test/controller/parameters/serialization_test.rb (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 26794c67b7..98624ba103 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -7,6 +7,12 @@ require 'action_dispatch/http/upload' require 'rack/test' require 'stringio' require 'set' +require 'yaml' + +# Wire up YAML format compatibility with Rails 4.2. Makes the YAML parser call +# `init_with` when it encounters `!ruby/hash-with-ivars:ActionController::Parameters`, +# instead of trying to parse it as a regular hash subclass. +YAML.load_tags['!ruby/hash-with-ivars:ActionController::Parameters'] = 'ActionController::Parameters' module ActionController # Raised when a required parameter is missing. @@ -591,6 +597,19 @@ module ActionController "<#{self.class} #{@parameters} permitted: #{@permitted}>" end + def init_with(coder) # :nodoc: + if coder.map['elements'] + # YAML's Hash subclass format from Rails 4.2, where keys and values + # were stored under an elements hash and `permitted` within an ivars hash. + @parameters = coder.map['elements'].with_indifferent_access + @permitted = coder.map['ivars'][:@permitted] + else + # YAML's Object format. Only needed because of the format + # backwardscompability above, otherwise equivalent to YAML's initialization. + @parameters, @permitted = coder.map['parameters'], coder.map['permitted'] + end + end + def method_missing(method_sym, *args, &block) if @parameters.respond_to?(method_sym) message = <<-DEPRECATE.squish diff --git a/actionpack/test/controller/parameters/serialization_test.rb b/actionpack/test/controller/parameters/serialization_test.rb new file mode 100644 index 0000000000..5d389f5a4c --- /dev/null +++ b/actionpack/test/controller/parameters/serialization_test.rb @@ -0,0 +1,32 @@ +require 'abstract_unit' +require 'action_controller/metal/strong_parameters' +require 'active_support/core_ext/string/strip' + +class ParametersSerializationTest < ActiveSupport::TestCase + test 'yaml serialization' do + assert_equal <<-end_of_yaml.strip_heredoc, YAML.dump(ActionController::Parameters.new(key: :value)) + --- !ruby/object:ActionController::Parameters + parameters: !ruby/hash:ActiveSupport::HashWithIndifferentAccess + key: :value + permitted: false + end_of_yaml + end + + test 'yaml deserialization' do + params = ActionController::Parameters.new(key: :value) + roundtripped = YAML.load(YAML.dump(params)) + + assert_equal params, roundtripped + assert_not roundtripped.permitted? + end + + test 'yaml backwardscompatible with hash inheriting parameters' do + assert_equal ActionController::Parameters.new(key: :value), YAML.load(<<-end_of_yaml.strip_heredoc) + --- !ruby/hash-with-ivars:ActionController::Parameters + elements: + key: :value + ivars: + :@permitted: false + end_of_yaml + end +end -- cgit v1.2.3 From 6eb978234cdce57e75c36a096fa76a634705c7c8 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Tue, 2 Aug 2016 14:54:25 +0200 Subject: Let Psych 2.0.9+ deserialize 2.0.8 serialized parameters. If we were to serialize an `ActionController::Parameters` on Psych 2.0.8, we'd get: ```yaml --- !ruby/hash:ActionController::Parameters key: :value ``` Because 2.0.8 didn't store instance variables, while 2.0.9 did: https://github.com/tenderlove/psych/commit/8f84ad0fc711a82a1040def861cb121e8985fd4c That, coupled with 2.0.8 calling `new` instead of `allocate` meant parameters was deserialized just fine: https://github.com/tenderlove/psych/commit/af308f8307899cb9e1c0fffea4bce3110a1c3926 However, if users have 2.0.8 serialized parameters, then upgrade to Psych 2.0.9+ and Rails 5, it would start to blow up because `initialize` will never be called, and thus `@parameters` will never be assigned. Hello, `NoMethodErrors` on `NilClass`! :) To fix this we register another variant of the previous serialization format and take it into account in `init_with`. I've tested this in our app and previously raising code now deserializes like a champ. I'm unsure how to test this in our suite because we use Psych 2.0.8 and don't know how to make us use 2.0.9+ for just one test. --- .../lib/action_controller/metal/strong_parameters.rb | 11 +++++++++-- .../test/controller/parameters/serialization_test.rb | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 98624ba103..d2db982a6e 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -12,7 +12,10 @@ require 'yaml' # Wire up YAML format compatibility with Rails 4.2. Makes the YAML parser call # `init_with` when it encounters `!ruby/hash-with-ivars:ActionController::Parameters`, # instead of trying to parse it as a regular hash subclass. +# Second `load_tags` is for compatibility with Psych prior to 2.0.9 where hashes +# were dumped without instance variables. YAML.load_tags['!ruby/hash-with-ivars:ActionController::Parameters'] = 'ActionController::Parameters' +YAML.load_tags['!ruby/hash:ActionController::Parameters'] = 'ActionController::Parameters' module ActionController # Raised when a required parameter is missing. @@ -599,14 +602,18 @@ module ActionController def init_with(coder) # :nodoc: if coder.map['elements'] - # YAML's Hash subclass format from Rails 4.2, where keys and values + # YAML 2.0.9's Hash subclass format from Rails 4.2, where keys and values # were stored under an elements hash and `permitted` within an ivars hash. @parameters = coder.map['elements'].with_indifferent_access @permitted = coder.map['ivars'][:@permitted] - else + elsif coder.map['parameters'] # YAML's Object format. Only needed because of the format # backwardscompability above, otherwise equivalent to YAML's initialization. @parameters, @permitted = coder.map['parameters'], coder.map['permitted'] + else + # YAML 2.0.8's format where hash instance variables weren't stored. + @parameters = coder.map.with_indifferent_access + @permitted = false end end diff --git a/actionpack/test/controller/parameters/serialization_test.rb b/actionpack/test/controller/parameters/serialization_test.rb index 5d389f5a4c..84193e304a 100644 --- a/actionpack/test/controller/parameters/serialization_test.rb +++ b/actionpack/test/controller/parameters/serialization_test.rb @@ -20,13 +20,26 @@ class ParametersSerializationTest < ActiveSupport::TestCase assert_not roundtripped.permitted? end - test 'yaml backwardscompatible with hash inheriting parameters' do - assert_equal ActionController::Parameters.new(key: :value), YAML.load(<<-end_of_yaml.strip_heredoc) + test 'yaml backwardscompatible with psych 2.0.8 format' do + params = YAML.load <<-end_of_yaml.strip_heredoc + --- !ruby/hash:ActionController::Parameters + key: :value + end_of_yaml + + assert_equal :value, params[:key] + assert_not params.permitted? + end + + test 'yaml backwardscompatible with psych 2.0.9+ format' do + params = YAML.load(<<-end_of_yaml.strip_heredoc) --- !ruby/hash-with-ivars:ActionController::Parameters elements: key: :value ivars: :@permitted: false end_of_yaml + + assert_equal :value, params[:key] + assert_not params.permitted? end end -- cgit v1.2.3 From 0e0cff0f27eb76e84fd2226396f16bb52fe754bd Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Tue, 2 Aug 2016 15:53:45 +0200 Subject: Replace implicit formats with a case statement. The coder that Psych passes in has a `tag` method we can use to detect which serialization format we're reviving for. Use it and make it clearer alongside the `load_tags` fiddling. --- .../lib/action_controller/metal/strong_parameters.rb | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index d2db982a6e..13e46789e9 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -601,19 +601,20 @@ module ActionController end def init_with(coder) # :nodoc: - if coder.map['elements'] - # YAML 2.0.9's Hash subclass format from Rails 4.2, where keys and values + case coder.tag + when '!ruby/hash:ActionController::Parameters' + # YAML 2.0.8's format where hash instance variables weren't stored. + @parameters = coder.map.with_indifferent_access + @permitted = false + when '!ruby/hash-with-ivars:ActionController::Parameters' + # YAML 2.0.9's Hash subclass format where keys and values # were stored under an elements hash and `permitted` within an ivars hash. @parameters = coder.map['elements'].with_indifferent_access @permitted = coder.map['ivars'][:@permitted] - elsif coder.map['parameters'] + when '!ruby/object:ActionController::Parameters' # YAML's Object format. Only needed because of the format # backwardscompability above, otherwise equivalent to YAML's initialization. @parameters, @permitted = coder.map['parameters'], coder.map['permitted'] - else - # YAML 2.0.8's format where hash instance variables weren't stored. - @parameters = coder.map.with_indifferent_access - @permitted = false end end -- cgit v1.2.3 From b71732c84a4bb5f8f7cbf65c5193db97c7e31eca Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Tue, 2 Aug 2016 16:20:08 +0200 Subject: Move the YAML hook closer to `init_with`. Looked odd, so completely detached from the other necessary part of the implementation. --- .../lib/action_controller/metal/strong_parameters.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 13e46789e9..f101c7b836 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -9,14 +9,6 @@ require 'stringio' require 'set' require 'yaml' -# Wire up YAML format compatibility with Rails 4.2. Makes the YAML parser call -# `init_with` when it encounters `!ruby/hash-with-ivars:ActionController::Parameters`, -# instead of trying to parse it as a regular hash subclass. -# Second `load_tags` is for compatibility with Psych prior to 2.0.9 where hashes -# were dumped without instance variables. -YAML.load_tags['!ruby/hash-with-ivars:ActionController::Parameters'] = 'ActionController::Parameters' -YAML.load_tags['!ruby/hash:ActionController::Parameters'] = 'ActionController::Parameters' - module ActionController # Raised when a required parameter is missing. # @@ -600,6 +592,15 @@ module ActionController "<#{self.class} #{@parameters} permitted: #{@permitted}>" end + def self.hook_into_yaml_loading # :nodoc: + # Wire up YAML format compatibility with Rails 4.2 and Psych 2.0.8 and 2.0.9+. + # Makes the YAML parser call `init_with` when it encounters the keys below + # instead of trying its own parsing routines. + YAML.load_tags['!ruby/hash-with-ivars:ActionController::Parameters'] = name + YAML.load_tags['!ruby/hash:ActionController::Parameters'] = name + end + hook_into_yaml_loading + def init_with(coder) # :nodoc: case coder.tag when '!ruby/hash:ActionController::Parameters' -- cgit v1.2.3 From 1d542e47d15d0f29cf301dcb883143716c06552c Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Tue, 2 Aug 2016 16:22:57 +0200 Subject: Set `always_permitted_parameters`. The tests were written with the common false value seen in Rails apps, show that intent in the code. Should also fix the build on 5-0-stable. --- actionpack/test/controller/parameters/serialization_test.rb | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'actionpack') diff --git a/actionpack/test/controller/parameters/serialization_test.rb b/actionpack/test/controller/parameters/serialization_test.rb index 84193e304a..352dd8ffb7 100644 --- a/actionpack/test/controller/parameters/serialization_test.rb +++ b/actionpack/test/controller/parameters/serialization_test.rb @@ -3,6 +3,15 @@ require 'action_controller/metal/strong_parameters' require 'active_support/core_ext/string/strip' class ParametersSerializationTest < ActiveSupport::TestCase + setup do + @old_permitted_parameters = ActionController::Parameters.always_permitted_parameters + ActionController::Parameters.always_permitted_parameters = true + end + + teardown do + ActionController::Parameters.always_permitted_parameters = @old_permitted_parameters + end + test 'yaml serialization' do assert_equal <<-end_of_yaml.strip_heredoc, YAML.dump(ActionController::Parameters.new(key: :value)) --- !ruby/object:ActionController::Parameters -- cgit v1.2.3 From 70b995a77f3e47c24ae88509066d0cdc4b2d779e Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Tue, 2 Aug 2016 16:52:18 +0200 Subject: Fix wrong assignment. Screwed up both the left and right hand sides! --- actionpack/test/controller/parameters/serialization_test.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/test/controller/parameters/serialization_test.rb b/actionpack/test/controller/parameters/serialization_test.rb index 352dd8ffb7..c9d38c1f48 100644 --- a/actionpack/test/controller/parameters/serialization_test.rb +++ b/actionpack/test/controller/parameters/serialization_test.rb @@ -4,16 +4,17 @@ require 'active_support/core_ext/string/strip' class ParametersSerializationTest < ActiveSupport::TestCase setup do - @old_permitted_parameters = ActionController::Parameters.always_permitted_parameters - ActionController::Parameters.always_permitted_parameters = true + @old_permitted_parameters = ActionController::Parameters.permit_all_parameters + ActionController::Parameters.permit_all_parameters = false end teardown do - ActionController::Parameters.always_permitted_parameters = @old_permitted_parameters + ActionController::Parameters.permit_all_parameters = @old_permitted_parameters end test 'yaml serialization' do - assert_equal <<-end_of_yaml.strip_heredoc, YAML.dump(ActionController::Parameters.new(key: :value)) + params = ActionController::Parameters.new(key: :value) + assert_equal <<-end_of_yaml.strip_heredoc, YAML.dump(params) --- !ruby/object:ActionController::Parameters parameters: !ruby/hash:ActiveSupport::HashWithIndifferentAccess key: :value -- cgit v1.2.3