New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration option to optionally disable deep_munge #13188

Merged
merged 1 commit into from Dec 20, 2013

Conversation

Projects
None yet
6 participants
@imanel
Contributor

imanel commented Dec 5, 2013

In relation to #13157 I tried to add option to disable deep_munge if developer is willing to do so. Deep munge is enabled by default to protect those who are unaware of risk, while allowing to disable it if advanced parameter parsing is required in given application.

@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Dec 5, 2013

Contributor

/cc @jeremy

Contributor

imanel commented Dec 5, 2013

/cc @jeremy

@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Dec 5, 2013

Contributor

I'm aware that perform_deep_munge might not be best configuration option name, but alternatives like strip_nil_in_json might be confusing and in result might lead to enabling this option without knowing the risk. Any better name will be welcome.

Contributor

imanel commented Dec 5, 2013

I'm aware that perform_deep_munge might not be best configuration option name, but alternatives like strip_nil_in_json might be confusing and in result might lead to enabling this option without knowing the risk. Any better name will be welcome.

@pftg

View changes

Show outdated Hide outdated actionpack/CHANGELOG.md
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Dec 5, 2013

Member

I'm 👍 on a config option as a pressure-release valve for users who are affected by this issue and explicitly choose to opt out of security protection.

The option can be deprecated if/when we have a superior solution to the root issue.

Member

jeremy commented Dec 5, 2013

I'm 👍 on a config option as a pressure-release valve for users who are affected by this issue and explicitly choose to opt out of security protection.

The option can be deprecated if/when we have a superior solution to the root issue.

@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Dec 6, 2013

Contributor

That was the intention - right now a lot of users are still on 3.2.10 or are patching Rails 4 because of this issue (I'm doing so). Should I add/modify something in order to push this forward? I also can't find test result on Travis - how to rerun pull request testing?

Contributor

imanel commented Dec 6, 2013

That was the intention - right now a lot of users are still on 3.2.10 or are patching Rails 4 because of this issue (I'm doing so). Should I add/modify something in order to push this forward? I also can't find test result on Travis - how to rerun pull request testing?

@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Dec 20, 2013

Contributor

Any update on this one?

Contributor

imanel commented Dec 20, 2013

Any update on this one?

@imanel imanel referenced this pull request Dec 20, 2013

Closed

Remove deep munge #13157

jeremy added a commit that referenced this pull request Dec 20, 2013

Merge pull request #13188 from imanel/skip_deep_munge
Add configuration option to optionally disable deep_munge

Conflicts:
	actionpack/CHANGELOG.md

@jeremy jeremy closed this Dec 20, 2013

@jeremy jeremy merged commit e8572cf into rails:master Dec 20, 2013

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Dec 20, 2013

Member

Thanks @imanel ! 👍

Member

jeremy commented Dec 20, 2013

Thanks @imanel ! 👍

@monfresh

This comment has been minimized.

Show comment
Hide comment
@monfresh

monfresh Jun 26, 2014

Maybe I'm not understanding how this works, but this doesn't seem to work for me in Rails 4.1.1. I'm developing a JSON API and I want clients to be able to update Array fields by passing in an empty array in the JSON. Here's what I have:

# In config/application.rb
config.action_dispatch.perform_deep_munge = false
# In the model
serialize :emails, Array
# In the controller
def update
  location = Location.find(params[:id])
  location.update!(location_params)
  render json: location, status: 200
end

private

def location_params
  params.permit(emails: [])
end
# In the spec
it 'allows empty array for serialized array fields' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] },
        'HTTP_X_API_TOKEN' => @token
  expect(json['emails']).to eq([])
end

With this setup, the spec does not pass. To make it pass, I need to explicitly set the emails param to an empty array, like this:

def location_params
  params[:emails] ||= []
  params.permit(emails: [])
end

This works regardless of the deep munge setting, but is not ideal since I would have to do the ||= [] dance for every parameter I don't want to be deep munged, which would make my location_params method turn ugly.

Am I misunderstanding the deep munge setting or is it not working as expected?

monfresh commented Jun 26, 2014

Maybe I'm not understanding how this works, but this doesn't seem to work for me in Rails 4.1.1. I'm developing a JSON API and I want clients to be able to update Array fields by passing in an empty array in the JSON. Here's what I have:

# In config/application.rb
config.action_dispatch.perform_deep_munge = false
# In the model
serialize :emails, Array
# In the controller
def update
  location = Location.find(params[:id])
  location.update!(location_params)
  render json: location, status: 200
end

private

def location_params
  params.permit(emails: [])
end
# In the spec
it 'allows empty array for serialized array fields' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] },
        'HTTP_X_API_TOKEN' => @token
  expect(json['emails']).to eq([])
end

With this setup, the spec does not pass. To make it pass, I need to explicitly set the emails param to an empty array, like this:

def location_params
  params[:emails] ||= []
  params.permit(emails: [])
end

This works regardless of the deep munge setting, but is not ideal since I would have to do the ||= [] dance for every parameter I don't want to be deep munged, which would make my location_params method turn ugly.

Am I misunderstanding the deep munge setting or is it not working as expected?

@imanel

This comment has been minimized.

Show comment
Hide comment
@imanel

imanel Jun 26, 2014

Contributor

It looks like you're using it correctly, so it's very strange. Could you prepare example app confirming this error? I will try to check what's happening.

Contributor

imanel commented Jun 26, 2014

It looks like you're using it correctly, so it's very strange. Could you prepare example app confirming this error? I will try to check what's happening.

@monfresh

This comment has been minimized.

Show comment
Hide comment
@monfresh

monfresh Jun 26, 2014

I did some more research and found some interesting things. First, it turns out that to test an empty array, you have to specify a few more things in the spec. The hash needs .to_json and a Content-Type needs to be specified:

it 'sets emails field to empty array when value is empty array' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end

Second, the behavior varies depending on whether you're using strong_parameters or protected_attributes. With protected_attributes, the following specs both pass, regardless of whether or not deep munging is turned on:

it 'sets emails field to empty array when value is empty array' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end
it 'sets emails field to empty array when value is nil' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: nil }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end

With strong_parameters and location_params defined as in my previous comment, the when value is nil test always fails, whether or not deep munging is turned on, which makes sense because emails is not defined as a scalar value. The when value is empty array test only passes if deep munging is turned off.

monfresh commented Jun 26, 2014

I did some more research and found some interesting things. First, it turns out that to test an empty array, you have to specify a few more things in the spec. The hash needs .to_json and a Content-Type needs to be specified:

it 'sets emails field to empty array when value is empty array' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end

Second, the behavior varies depending on whether you're using strong_parameters or protected_attributes. With protected_attributes, the following specs both pass, regardless of whether or not deep munging is turned on:

it 'sets emails field to empty array when value is empty array' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: [] }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end
it 'sets emails field to empty array when value is nil' do
  patch api_endpoint(path: "/locations/#{@loc.id}"),
        { emails: nil }.to_json,
        'HTTP_X_API_TOKEN' => @token,
        'Content-Type' => 'application/json'
  expect(json['emails']).to eq([])
end

With strong_parameters and location_params defined as in my previous comment, the when value is nil test always fails, whether or not deep munging is turned on, which makes sense because emails is not defined as a scalar value. The when value is empty array test only passes if deep munging is turned off.

@mehulkar

This comment has been minimized.

Show comment
Hide comment
@mehulkar

mehulkar Mar 27, 2015

What's the earliest release where this config option is available? I'm not sure how to check. Currently using 4.0.4 and would like to be able to turn off deep munge.

mehulkar commented Mar 27, 2015

What's the earliest release where this config option is available? I'm not sure how to check. Currently using 4.0.4 and would like to be able to turn off deep munge.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Mar 27, 2015

Member

4.1.0. You can see this info in the commit page, just below at the commit message e8572cf

Member

rafaelfranca commented Mar 27, 2015

4.1.0. You can see this info in the commit page, just below at the commit message e8572cf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment