search

rails / protected_attributes

Protect attributes from mass-assignment in ActiveRecord models.
MIT License
228 stars 92 forks source link

Better strong parameters integration #73

Closed rafaelfranca closed 9 years ago

rafaelfranca commented 9 years ago

After this change we only fallback to strong parameters if model macros are not called.

Before we were fallbacking to mass assignment sanitization even if macros were not being called. Now we only check the strong parameter protection.

Also it changes the default behavior when including the gem.

Before we were permitting all the parameters and forcing to whitelist all the attributes. This make harder to upgrade to strong parameters because you can't slowly change the models to not use the attribute macros.

Since now we are fallbacking to strong parameters if the macro is not defined on the model we should not permit all the parameters neither call the macro on ActiveRecord::Base.

Models with macros will use the mass assignment sanitization and models without the macros will raise if parameters are not permitted.

This will make easier to slowly upgrade to use only strong parameters.

Review to see if I'm not missing anything @rails/security @byroot @senny.

twmills commented 9 years ago

This actually broke our app because we didn't have didn't have the config specified, so a patch level for this seems inappropriate.

rafaelfranca commented 9 years ago

@twmills could you explain more which config you didn't have specified? I can release a new patch level to aid your problem.

twmills commented 9 years ago

It's this:

config.action_controller.permit_all_parameters = true

twmills commented 9 years ago

Just feels like anything changing default behavior should be more than a patch level version bump.

rafaelfranca commented 9 years ago

Ah, got it. So this feature was released as a minor version. The 1.1 version was exactly to fallback to strong parameters when attr_accessible and attr_protected are not used, but there were a bug in the original implementation that was exactly removing this configuration. So 1.1.1 fixed this bug. That was the reason that made you application to break.

rafaelfranca commented 9 years ago

BTW, you should not set this configuration anymore. You should either filter the parameters or use the model macros.

rafaelfranca commented 9 years ago

As you can see the CHANGELOG show that it was released in a minor version https://github.com/rails/protected_attributes/blob/master/CHANGELOG.md#110

joshuaflanagan commented 9 years ago

The breaking change was not released in the minor version, even if that was the intention. The breaking change (removing the default config) was made in a patch release. The "fix" to the 1.1.0 release should have been 1.2.0 to properly indicate a breaking change.

rafaelfranca commented 9 years ago

Like I said, the removal of the default config were to be made in the minor release but was forgotten. So I considered a bug of the minor release and did a patch release to fix that bug.