Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Stop fetch from mutating when default is a hash #102

Open
wants to merge 1 commit into from

4 participants

Brendon Murphy Geoffrey Hichborn Rafael Mendonça França Doug Cole
Brendon Murphy

See gh-56 for an issue on this

Brendon Murphy

This line makes me uneasy. It's needed for the "sticky" tests to pass but I feel like the Parameters were permitted then we dropped new data via a default hash, which hasn't been checked. Maybe that's OK because you are in control since they are defaults?

Geoffrey Hichborn

Huge +1 This has been a really annoying bug.

Geoffrey Hichborn

@bemurphy Could you get the tests passing again?

Rafael Mendonça França
Owner

First, we need to fix this issue on rails/rails, after this we can merge it on the gem.

Doug Cole

I resubmitted this to rails, it's merged here: rails/rails#12656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 19, 2013
  1. Brendon Murphy

    Stop fetch from mutating when default is a hash

    bemurphy authored
    See gh-56 for an issue on this
This page is out of date. Refresh to see the latest.
9 lib/action_controller/parameters.rb
View
@@ -79,7 +79,14 @@ def [](key)
end
def fetch(key, *args)
- convert_hashes_to_parameters(key, super)
+ value = super
+ # Don't rely on +convert_hashes_to_parameters+
+ # so as to not mutate via a +fetch+
+ if value.is_a?(Hash)
+ value = self.class.new(value)
+ value.permit! if permitted?
+ end
+ value
rescue KeyError, IndexError
raise ActionController::ParameterMissing.new(key)
end
7 test/parameters_taint_test.rb
View
@@ -22,6 +22,13 @@ class ParametersTaintTest < ActiveSupport::TestCase
end
end
+ test "fetch where the default is a hash will not mutate the instance" do
+ @params.fetch :foo, {}
+ assert !@params.has_key?(:foo)
+ @params.fetch :foo, {:fizz => :buzz}
+ assert !@params.has_key?(:foo)
+ end
+
test "not permitted is sticky on accessors" do
assert !@params.slice(:person).permitted?
assert !@params[:person][:name].permitted?
Something went wrong with that request. Please try again.