Better interaction with strong parameters #15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The 'resource_params' method in some mixins here will invoke
'#{resource_name}_params', if the controller explicitly defines it.
This patch allows the controller to explicitly define 'resource_params'
as well, which ... ahem ... some peoples' abstract base classes may have
already been doing.
The default 'resource_params' included with the original code here
(permitting updates to all attributes unless something else is
specified) is a convenient default, and was (in effect) default
Rails behavior in Rails 3 or below if you didn't specify anything else.
And even if you did try to give an explicit whitelist, by defining a
"#{resource_name}_params" method, that list could be bypassed in some
cases. (If you supplied a list with *no* parameters on the whitelist,
new_resource would get an empty ActionController::Parameters -- and
since that answers true to 'empty?', it would dig around in 'params'
on its own. For example, CommentsController defined 'comment_params'
to permit only :user_id -- but there were specs that supplied only
:name, and passed.)
But requiring explicit whitelists was an explicit design decision that
Rails core made after they determined that this default was unsafe.
FWIW, what forced this on them was a demonstration that this led to
a subtle but absolutely deadly vulnerability in Github, which was and
is a Rails app. Briefly, Egor Homakov tried to raise the issue with
Rails core. When they didn't seem to be taking it seriously, he
forced the issue by adding a 'public_key[user_id]' input to one of
their forms via Firebug. He used this "augmented" form to update the
user_id of one of his own public keys to assign it to a member of Rails
core, and then used that key to make a commit onto Rails master itself.
(It's still there, as b8396578.) See this discussion from the immediate
aftermath (before strong params were available):
https://gist.github.com/peternixey/1978249
The upshot is that each controller is required to have a one-line
definition of resource_params (or #{resource_name}_params). Having
to write them may be a slight annoyance -- but it's there because
experience has shown that a "set anything" default is unsafe.
(People daring/foolish enough to want it can still arrange it with a
def resource_params; params[resource_name].try(&:permit!); end
in ApplicationController -- but at least that's where tools like
Brakeman can see it!)
Redundant with the definition in RC::ResourceMethods (which is where this properly belongs).
The ':rubygems' source is deprecated because it was synonymous with 'http://rubygems.org', which permits the code to be altered by a man-in-the-middle.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request deals with #14 by eliminating the default behavior of synthesizing a whitelist (which likely includes attributes that a client isn't meant to be able to set directly, including foreign keys and sensitive stuff like
is_adminon aUserobject). It also updates the docs to describe usage with strong parameters (in Rails 4+, or Rails 3 with thestrong_parametersgem.See discussion on the ticket for more particulars...