Consider setting `action_on_unpermitted_parameters` to `:raise` in development and/or test

[G-Rath]

strong_parameters uses action_on_unpermitted_parameters to control what happens when an unpermitted param is found, which can be set to false, :log, or :raise.

The default for production is "false" whereas in development and test it's :log but maybe we should set it to :raise for the latter?

It makes sense for it to be silent in production because ultimately anything can be passed to our endpoints, but that's also why I think it would make sense to be very loud in dev and test as we should only be getting expected params.

I don't think historically we've had any significant issues or bugs around params that this would catch, but I have come across some instances of unpermitted params being filtered in apps (most commonly with the CSRF token) which raises my eyebrows as I'm usually hunting down a niche bug, so I think it could be a way of improving our hygiene a bit

[G-Rath]

Ok I applied it to my test suite and was immediately reminded that I've got cases where I explicitly pass in an unpermitted param as I'm checking that you can't e.g. pass in entity_id to have the app create a relationship on a nested entity (as that id should be from the route), but maybe it would still make sense to enable it for development?

[eoinkelly]

I temporarily turned this on in my most recent client project in dev and test. It created a number of test failures and errors which I have grouped into categories below.

Category 1: :raise is a bit worse

Here we are overriding parts of a Devise controller to adjust the sign-up process. The method below permits only the param it cares about and then extracts that value. It ignores the other params because devise handles them elsewhere. Fixing this would mean permitting :password, :password_confirmation, :current_password which arguably makes the code below less clear. A future reader would naturally ask why permit 3 things when you clearly only use 1.

module Users
  class RegistrationsController < Devise::RegistrationsController

	# ... 
	def subscribed_param
	  params.require(:user).permit(:subscribed)[:subscribed]
	end
  end
end

Category 2: :raise is better

I found actual bugs by turning this on in test!

I found 3 test cases which were not actually testing what we expect because the param passed to the tests no longer matches the param expected by the controller.

Category 3: it's a tie

It found a few request specs where we were sending just enough params to test the code path rather than the full set of params. This helps keep the test code concise and "on-topic" but but does make the test as documentation of how to call that endpoint.

Evaluating the categories

The issues in category 1 could be worked around pretty easily with a comment or by refactoring the code into 2 well named methods e.g.

module Users
  class RegistrationsController < Devise::RegistrationsController

	# ... 
	def subscribed_param
	  permitted_params[:subscribed]
	end

	def permitted_param
	  params.require(:user).permit(:subscribed, :foo, :bar, :whatever)
	end
  end
end

The win in category 2 above is quite strong. This flag found an actual bug in our tests.

I think category 3 is fine either way.

Conclusion

Overall I support setting this to :raise in development and test environment for new applications.

[hmaddocks]

the param passed to the tests no longer matches the param expected by the controller

Even worse. There's no guarantee that your test params match the form fields.