Skip to content

Exception on unexpected params when enabled. #75

Closed
wants to merge 0 commits into from

7 participants

@thomasfedb

Added the ability for developers to enable reporting of unexpected parameters.

To enable add config.raise_on_unexpected_params = true to whatever environment file required.

This would satisfy the request in #66.

@mariovisic

+1 would be very useful.

@thomasfedb

That should fix the failing travis builds.

@sevenseacat

\o/

@thomasfedb

Rebased to remove changes to .gitignore.

@dhh
Ruby on Rails member
dhh commented Jan 4, 2013

Good stuff. But let's stick the option on config.action_controller.raise_on_unexpected_params -- it's weird just floating off config itself.

@thomasfedb

@dhh sure, I'll change that up right now.

@rafaelfranca
Ruby on Rails member

Also, before adding things here is better to add it to rails itself.

@dhh
Ruby on Rails member
dhh commented Jan 4, 2013
@thomasfedb

@rafaelfranca OK. I'll get that done.

@thomasfedb thomasfedb referenced this pull request in rails/rails Jan 4, 2013
Merged

Exception on unexpected params when enabled. #8752

@thomasfedb

Pull request has been submitted against rails/rails here: rails/rails#8752

@thomasfedb

I'll be porting the changes requested at rails/rails#8752 back to this pull request.

@thomasfedb

This should now be in like with what was accepted into rails here: rails/rails#8752.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jan 5, 2013
lib/strong_parameters/railtie.rb
@@ -7,5 +7,10 @@ class Railtie < ::Rails::Railtie
else
config.generators.scaffold_controller = :strong_parameters_controller
end
+
+ config.after_initialize do
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note Jan 5, 2013

This options should be set before action_controller.set_configs and deleted from app.config.action_controller or Rails will raise an exception.

See https://github.com/rails/protected_attributes/blob/master/lib/protected_attributes/railtie.rb#L10 to an example

@thomasfedb
thomasfedb added a note Jan 5, 2013

Cheers for the example, needed that. Will fix.

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

@rafaelfranca OK, made that change. Let me know if that's OK now.

@rafaelfranca rafaelfranca commented on an outdated diff Jan 5, 2013
lib/strong_parameters/railtie.rb
@@ -7,5 +7,13 @@ class Railtie < ::Rails::Railtie
else
config.generators.scaffold_controller = :strong_parameters_controller
end
+
+ config.before_configuration do |app|
+ config.action_controller.raise_on_unexpected_params = (Rails.env.test? || Rails.env.development?)
+ end
+
+ initializer "strong_parameters.config", :before => "active_controller.set_configs" do |app|
+ ActionController::Parameters.raise_on_unexpected = app.config.action_controller.delete(:raise_on_unexpected_params)
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note Jan 5, 2013

Just

ActionController::Parameters.raise_on_unexpected = app.config.action_controller.delete(:raise_on_unexpected_params) { Rails.env.test? || Rails.env.development? }

will work fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

We renamed the option to raise_on_unpermitted_parameters

See rails/rails@1401f96

@thomasfedb

@rafaelfranca I'm away from my computer for a week starting in 5 minutes. I will get this done immediately on my return.

Thanks for the feedback and guiding.

@dhh
Ruby on Rails member
dhh commented Jan 8, 2013

This isn't working out well in practice for me. This case shows it well:

    if person = authenticate(params.permit(:username, :password, :sig))
      sign_in person, params.permit(:remember_me)

First of all, I'll get a raise on action and controller parameters because they're present at the root. Second, I will get raises on the first permit call because :remember_me is present, and on the second one because the others are present.

I don't see a way around this at the moment, so I think we're going to have to revert.

@jeremy
Ruby on Rails member
jeremy commented Jan 8, 2013

We could track which params were touched with #permit or #require and have #tainted return those that were not. Then report on unexpected params after the action:

after_action do |c|
  if c.params.tainted.any?
    raise "You have some params that you didn't #permit or #require: #{c.params.tainted.keys.to_sentence}"
  end
end
@thomasfedb
@thomasfedb

In response to the example by @dhh, in such a situation I would write something like:

def create
  if person = authenticate(auth_params.select(:username, :password, :sig))
    sign_in person, auth_params.select(:remember_me)
end

private

def auth_params
  params.permit(:username, :password, :remember_me)
end

Alternately the authenticate and sign_in methods could be written such that unexpected keys aren't a problem and in that case the .select statements could be dropped.

As I said before in the vast majority of cases in what I see as a typical rails application the params being handled relate directly to a resource and so I think the example above will be reasonably rare.

Please give me your thoughts @rafaelfranca, @jeremy and @dhh.

@dhh
Ruby on Rails member
dhh commented Jan 14, 2013
@rubys
rubys commented Jan 14, 2013

The previous/current implementation had:

options.raise_on_unexpected_params ||= (Rails.env.test? || Rails.env.development?)

Logging would be a useful addition to the production environment.

@dhh
Ruby on Rails member
dhh commented Jan 14, 2013
@thomasfedb

@dhh Very happy to make the default a log entry. Should I make such changes against https://github.com/rails/rails first?

Would you prefer two configuration toggles, one for raising an exception and one for logging, or one config options which takes a symbol of :log or :raise.

@dhh
Ruby on Rails member
dhh commented Jan 15, 2013
@thomasfedb thomasfedb referenced this pull request in rails/rails Jan 19, 2013
Merged

Raise or log unpermitted params. #8999

@thomasfedb

I changed branch so any discussion should continue here: #85

@thomasfedb thomasfedb closed this Jan 19, 2013
@sgerrand sgerrand pushed a commit to sgerrand/rails that referenced this pull request Nov 2, 2013
@dhh dhh Revert "unpermitted params" exception -- it's just not going to work.…
… See the discussion on rails/strong_parameters#75.
cc1c3c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.