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

Need routing option for filtering params and preventing mass-assignment #905

Closed
postmodern opened this Issue Aug 9, 2012 · 29 comments

Comments

Projects
None yet
8 participants
@postmodern
Contributor

postmodern commented Aug 9, 2012

There doesn't appear to be an option for specifying params accepted by a post route. This is needed to stop mass-assignment at the routing layer.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Aug 9, 2012

Member

Just as a note: this can be easily implemented using conditions.

http://www.sinatrarb.com/intro#Conditions

I think its worth adding.

Member

skade commented Aug 9, 2012

Just as a note: this can be easily implemented using conditions.

http://www.sinatrarb.com/intro#Conditions

I think its worth adding.

@ghost ghost assigned skade Aug 9, 2012

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Aug 9, 2012

Contributor

@skade Ah I like that idea. Then you could have multiple post routes, which accept different param sets.

Contributor

postmodern commented Aug 9, 2012

@skade Ah I like that idea. Then you could have multiple post routes, which accept different param sets.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Aug 9, 2012

Member

As proposed on Twitter:

allow %w(foo bar batz)
post ....

expect %w(foo bar bats)
post ...

deny %w(foo bar batz)
post ...

I am not sure whether deny is worth it, I don't like blacklisting that much.

Member

skade commented Aug 9, 2012

As proposed on Twitter:

allow %w(foo bar batz)
post ....

expect %w(foo bar bats)
post ...

deny %w(foo bar batz)
post ...

I am not sure whether deny is worth it, I don't like blacklisting that much.

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Aug 9, 2012

Contributor

I'd prefer an option syntax:

post :update, :allow => [:foo, :bar, :baz] do
end

post :update, :accept => [:foo, :bar, :baz] do
end

post :update, :params => [:foo, :bar, :baz] do
end

Also we should cross-out deny. Whitelisting is always preferred to selective blacklisting.

Contributor

postmodern commented Aug 9, 2012

I'd prefer an option syntax:

post :update, :allow => [:foo, :bar, :baz] do
end

post :update, :accept => [:foo, :bar, :baz] do
end

post :update, :params => [:foo, :bar, :baz] do
end

Also we should cross-out deny. Whitelisting is always preferred to selective blacklisting.

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Aug 9, 2012

Contributor

Or, we could specify allowed params globally in the controller, like proposed for Rails:

allow_params :user => {:name, :email}

get :show, :with => :id do
end

post :update do
end

post :sort_of_like_update do
end
Contributor

postmodern commented Aug 9, 2012

Or, we could specify allowed params globally in the controller, like proposed for Rails:

allow_params :user => {:name, :email}

get :show, :with => :id do
end

post :update do
end

post :sort_of_like_update do
end
@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Aug 9, 2012

Contributor

@skade I might try to toss together a small sinatra plugin using Sinatra Conditions, unless we want to build this into padrino-core?

Contributor

postmodern commented Aug 9, 2012

@skade I might try to toss together a small sinatra plugin using Sinatra Conditions, unless we want to build this into padrino-core?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Aug 9, 2012

Member

Great idea, personally I could see this any of the three: a sinatra plugin that works in Padrino, in padrino-contrib or even in core. I like this:

post :update, :allow => [:foo, :bar, :baz] do
end

seems pretty clean. I think my vote would be in padrino-contrib or as a sinatra/padrino plugin (i.e padrino-params-protection). What do you think @achiu @DAddYE

Member

nesquena commented Aug 9, 2012

Great idea, personally I could see this any of the three: a sinatra plugin that works in Padrino, in padrino-contrib or even in core. I like this:

post :update, :allow => [:foo, :bar, :baz] do
end

seems pretty clean. I think my vote would be in padrino-contrib or as a sinatra/padrino plugin (i.e padrino-params-protection). What do you think @achiu @DAddYE

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Aug 9, 2012

Member

@postmodern
The way conditions are set up in Padrino, the following 2 should be equivalent:

allow .....
post ....

post :allow => [....] do

Sinatra plugin would be much preferred as always, including it by standard isn't touched by this.

Member

skade commented Aug 9, 2012

@postmodern
The way conditions are set up in Padrino, the following 2 should be equivalent:

allow .....
post ....

post :allow => [....] do

Sinatra plugin would be much preferred as always, including it by standard isn't touched by this.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Aug 19, 2012

Member

Mmm, love it, we should finally setup a wiki or a google docs with routing changes. No?

Member

DAddYE commented Aug 19, 2012

Mmm, love it, we should finally setup a wiki or a google docs with routing changes. No?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 5, 2013

Contributor

Should we schedule this for v1.0?

Contributor

dariocravero commented Jan 5, 2013

Should we schedule this for v1.0?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 5, 2013

Member

I agree. If It is fine to u lets move to 1.0

On Jan 5, 2013, at 6:45 AM, "Darío Javier Cravero" notifications@github.com
wrote:

Should we schedule this for v1.0?


Reply to this email directly or view it on
GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-11910465.

Member

DAddYE commented Jan 5, 2013

I agree. If It is fine to u lets move to 1.0

On Jan 5, 2013, at 6:45 AM, "Darío Javier Cravero" notifications@github.com
wrote:

Should we schedule this for v1.0?


Reply to this email directly or view it on
GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-11910465.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 5, 2013

Contributor

Moved.

Contributor

dariocravero commented Jan 5, 2013

Moved.

@graudeejs

This comment has been minimized.

Show comment
Hide comment
@graudeejs

graudeejs Jan 21, 2014

Contributor

What about nested resources?

Also it would be much better if you could do this some method, to share common params for actions, instead of writing them all over again for each action that needs them.

I like how it's done in Rails with strong_parameters

Contributor

graudeejs commented Jan 21, 2014

What about nested resources?

Also it would be much better if you could do this some method, to share common params for actions, instead of writing them all over again for each action that needs them.

I like how it's done in Rails with strong_parameters

ujifgc added a commit that referenced this issue May 4, 2014

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 4, 2014

Member

@padrino/core-members please look at c1221fa and say if the following syntax is okay:

post :update, :allow => [:name, :email]
post :update, :allow => [:name, :id => Integer]
post :update, :allow => [:name => proc{ |v| v.reverse }]
post :update, :allow => [:name, :parent => [:name, :position]]

allow :name, :email, :password => proc{ |v| v.reverse }
post :update

If it's okay I will write tests and include this little filter in padrino-core routing. If it's not we can try to mimic rails strong_parameters API.

Member

ujifgc commented May 4, 2014

@padrino/core-members please look at c1221fa and say if the following syntax is okay:

post :update, :allow => [:name, :email]
post :update, :allow => [:name, :id => Integer]
post :update, :allow => [:name => proc{ |v| v.reverse }]
post :update, :allow => [:name, :parent => [:name, :position]]

allow :name, :email, :password => proc{ |v| v.reverse }
post :update

If it's okay I will write tests and include this little filter in padrino-core routing. If it's not we can try to mimic rails strong_parameters API.

@Ortuna

This comment has been minimized.

Show comment
Hide comment
@Ortuna

Ortuna May 4, 2014

Member

👍 I like it, and I don't see anything wrong with it.

Member

Ortuna commented May 4, 2014

👍 I like it, and I don't see anything wrong with it.

ujifgc added a commit that referenced this issue May 5, 2014

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 5, 2014

Member

There's a thing about common params for similar actions (example: :create and :update) I wanted to ask: is it wise to add another helper class method to inject a hash of allowed parameters to several routes or it's better to educate a programmer to do something like this:

MyFabulous::App.controllers :accounts do
  clean_params = [ :id, :name, :position ]
  post :create, :allow => clean_params do
    something
  end
  post :update, :with => :id, :allow => clean_params do
    something
  end
end

Or even this:

class Account
  include Database::Resource
  def self.put_it_in_the_right_place(file)
    something
  end
  def self.clean_params
    [ :id, :account => [ :name, :position, :file => &put_it_in_the_right_place ] ]
  end
end

MyFabulous::App.controllers :accounts do
  post :create, :allow => Account.clean_params do
    something
  end
  post :update, :with => :id, :allow => Account.clean_params do
    something
  end
end
Member

ujifgc commented May 5, 2014

There's a thing about common params for similar actions (example: :create and :update) I wanted to ask: is it wise to add another helper class method to inject a hash of allowed parameters to several routes or it's better to educate a programmer to do something like this:

MyFabulous::App.controllers :accounts do
  clean_params = [ :id, :name, :position ]
  post :create, :allow => clean_params do
    something
  end
  post :update, :with => :id, :allow => clean_params do
    something
  end
end

Or even this:

class Account
  include Database::Resource
  def self.put_it_in_the_right_place(file)
    something
  end
  def self.clean_params
    [ :id, :account => [ :name, :position, :file => &put_it_in_the_right_place ] ]
  end
end

MyFabulous::App.controllers :accounts do
  post :create, :allow => Account.clean_params do
    something
  end
  post :update, :with => :id, :allow => Account.clean_params do
    something
  end
end
@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 5, 2014

Contributor

I like this approach. 👍 @ujifgc.
I'd say it's better to educate the user to do that instead of forcing them to use a helper.

Also, with this syntax, I reckon it would be useful if the allow option would be available on the controller too and the local definition would override the controller one. E.g.:

MyFabulous::App.controllers :accounts, allow: [ :name, :position ] do
  post :create do
    something
  end
  post :update, :with => :id, :allow => [ :id, :name, :position ] do
    something
  end
end

Thoughts?

Contributor

dariocravero commented May 5, 2014

I like this approach. 👍 @ujifgc.
I'd say it's better to educate the user to do that instead of forcing them to use a helper.

Also, with this syntax, I reckon it would be useful if the allow option would be available on the controller too and the local definition would override the controller one. E.g.:

MyFabulous::App.controllers :accounts, allow: [ :name, :position ] do
  post :create do
    something
  end
  post :update, :with => :id, :allow => [ :id, :name, :position ] do
    something
  end
end

Thoughts?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 5, 2014

Member

👍 I think this could be an interesting addition to the controller. I agree we could allow it at the controller-level or the route-level as other options have been before. Also, I think those params if shared across actions would work the way you outlined with a variable or method that lists the attribute names. If the "allow" key isn't specified, I assume then it would still allow all keys.

Member

nesquena commented May 5, 2014

👍 I think this could be an interesting addition to the controller. I agree we could allow it at the controller-level or the route-level as other options have been before. Also, I think those params if shared across actions would work the way you outlined with a variable or method that lists the attribute names. If the "allow" key isn't specified, I assume then it would still allow all keys.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 6, 2014

Member

I patched some things to configure with controller options, play nicely with :with option and GET requests.

    mock_app do
      controller :persons, :allow => [ :name ] do
        post :create, :allow => [ :name, :position ] do
          # allows [ :name, :position ]
        end
        post :update, :with => [ :id ] do
          # allows [ :id, :name ]
        end
        get :show, :with => [ :id ] do
          # allows everything because it's GET
        end
        get :search, :allow => [ :q ] do
          # allows only :q because said specifically
        end
      end
    end

I would like to see more suggestions.

Member

ujifgc commented May 6, 2014

I patched some things to configure with controller options, play nicely with :with option and GET requests.

    mock_app do
      controller :persons, :allow => [ :name ] do
        post :create, :allow => [ :name, :position ] do
          # allows [ :name, :position ]
        end
        post :update, :with => [ :id ] do
          # allows [ :id, :name ]
        end
        get :show, :with => [ :id ] do
          # allows everything because it's GET
        end
        get :search, :allow => [ :q ] do
          # allows only :q because said specifically
        end
      end
    end

I would like to see more suggestions.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 6, 2014

Contributor

I'm not sure about allowing everything on gets when a controller-level allow is applied (or used in conjunction with with).

Maybe another suggestion would be :allow => true to override local allows and allow anything? The opposite might be valid too and :allow => false would forbid any params to be sent to the route.

Thoughts?

Contributor

dariocravero commented May 6, 2014

I'm not sure about allowing everything on gets when a controller-level allow is applied (or used in conjunction with with).

Maybe another suggestion would be :allow => true to override local allows and allow anything? The opposite might be valid too and :allow => false would forbid any params to be sent to the route.

Thoughts?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 6, 2014

Member

What is the point of filtering get parameters?
On May 6, 2014 2:57 PM, "Darío Javier Cravero" notifications@github.com
wrote:

I'm not sure about allowing everything on gets when a controller-level
allow is applied (or used in conjunction with with).

Maybe another suggestion would be :allow => true to override local allows
and allow anything? The opposite might be valid too and :allow => falsewould forbid any params to be sent to the route.

Thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-42288550
.

Member

ujifgc commented May 6, 2014

What is the point of filtering get parameters?
On May 6, 2014 2:57 PM, "Darío Javier Cravero" notifications@github.com
wrote:

I'm not sure about allowing everything on gets when a controller-level
allow is applied (or used in conjunction with with).

Maybe another suggestion would be :allow => true to override local allows
and allow anything? The opposite might be valid too and :allow => falsewould forbid any params to be sent to the route.

Thoughts?


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-42288550
.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 6, 2014

Contributor

I get you point, i.e., that it's supposed to be a read only action. But if you allowed a specific set of params in the controller, I believe that it should honour that constrain until it's explicitly disabled so it's easier to infer by reading the code.

Contributor

dariocravero commented May 6, 2014

I get you point, i.e., that it's supposed to be a read only action. But if you allowed a specific set of params in the controller, I believe that it should honour that constrain until it's explicitly disabled so it's easier to infer by reading the code.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 6, 2014

Member

Another issue with the API. If we have booleans for route option :allow, :allow => true fails to explain itself to me while :allow => [ :id, :name ] was pretty self-explanatory. Maybe we should name this option :allow_params => true, :allow_params => false, :allow_params => [ :id, :name ]?

Member

ujifgc commented May 6, 2014

Another issue with the API. If we have booleans for route option :allow, :allow => true fails to explain itself to me while :allow => [ :id, :name ] was pretty self-explanatory. Maybe we should name this option :allow_params => true, :allow_params => false, :allow_params => [ :id, :name ]?

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern May 7, 2014

Contributor

Filtering get parameters would prevent injecting arbitrary find options (ex: Model.find(params[:foo])).

Contributor

postmodern commented May 7, 2014

Filtering get parameters would prevent injecting arbitrary find options (ex: Model.find(params[:foo])).

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 7, 2014

Member

I removed the special treatment of GET, added true/false syntax and renamed allow to allow_params.

    mock_app do
      controller :persons, :allow_params => [ :name ] do
        post :create, :allow_params => [ :name, :position ] do
          # allows [ :name, :position ]
        end
        post :update, :with => [ :id ] do
          # allows [ :id, :name ]
        end
        post :update, :with => [ :id ], :allow_params => false do
          # allows [ :id ]
        end
        post :update, :with => [ :id ], :allow_params => true do
          # allows [ :id, :name, :everything_else ]
        end
      end
    end
Member

ujifgc commented May 7, 2014

I removed the special treatment of GET, added true/false syntax and renamed allow to allow_params.

    mock_app do
      controller :persons, :allow_params => [ :name ] do
        post :create, :allow_params => [ :name, :position ] do
          # allows [ :name, :position ]
        end
        post :update, :with => [ :id ] do
          # allows [ :id, :name ]
        end
        post :update, :with => [ :id ], :allow_params => false do
          # allows [ :id ]
        end
        post :update, :with => [ :id ], :allow_params => true do
          # allows [ :id, :name, :everything_else ]
        end
      end
    end

@ujifgc ujifgc removed the db-orm label May 7, 2014

@ujifgc ujifgc added the feature label May 7, 2014

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 7, 2014

Contributor

Looks good to me! :)

On Wed, May 7, 2014 at 5:39 PM, Igor Bochkariov notifications@github.comwrote:

I removed special treatment of GET, added true/false syntax and renamed
allow to allow_params.

mock_app do
  controller :persons, :allow_params => [ :name ] do
    post :create, :allow_params => [ :name, :position ] do


      # allows [ :name, :position ]
    end
    post :update, :with => [ :id ] do
      # allows [ :id, :name ]
    end

    post :update, :with => [ :id ], :allow_params => false do
      # allows [ :id ]
    end
    post :update, :with => [ :id ], :allow_params => true do
      # allows [ :id, :name, :everything_else ]
    end
  end
end


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-42451048
.

Contributor

dariocravero commented May 7, 2014

Looks good to me! :)

On Wed, May 7, 2014 at 5:39 PM, Igor Bochkariov notifications@github.comwrote:

I removed special treatment of GET, added true/false syntax and renamed
allow to allow_params.

mock_app do
  controller :persons, :allow_params => [ :name ] do
    post :create, :allow_params => [ :name, :position ] do


      # allows [ :name, :position ]
    end
    post :update, :with => [ :id ] do
      # allows [ :id, :name ]
    end

    post :update, :with => [ :id ], :allow_params => false do
      # allows [ :id ]
    end
    post :update, :with => [ :id ], :allow_params => true do
      # allows [ :id, :name, :everything_else ]
    end
  end
end


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/issues/905#issuecomment-42451048
.

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern May 7, 2014

Contributor

Could we rename allow_params to just params?

Contributor

postmodern commented May 7, 2014

Could we rename allow_params to just params?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc May 8, 2014

Member

Yes, I'm okay with it. @padrino/core-members ?

Member

ujifgc commented May 8, 2014

Yes, I'm okay with it. @padrino/core-members ?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 8, 2014

Contributor

Yeah, good recommendation @postmodern, make sense and it's snappier :)

Contributor

dariocravero commented May 8, 2014

Yeah, good recommendation @postmodern, make sense and it's snappier :)

@ujifgc ujifgc modified the milestones: 0.12.2, 1.0.0 May 8, 2014

@ujifgc ujifgc closed this in ca27d00 May 9, 2014

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