itmammoth / rails_sortable

Easy drag & drop sorting with persisting the arranged order for rails
MIT License
142 stars 37 forks source link

Security issue. There is no way to authorize the sortable resources. #30

Closed psmir closed 5 years ago

psmir commented 6 years ago

It is possible to change IDs in the markup and sort arbitrary sortable models. For example, a user can change such code

<li id="Question_88">
<li id="Question_89">

to

<li id="User_1">
<li id="User_2">

and sort. After that the users will be reordered.

itmammoth commented 6 years ago

Thanks @psmir Do you have any ideas to solve this problem?

psmir commented 6 years ago

@itmammoth I think you can ask users to define an authorization method with certain name in their ApplicationController. For example

def authorize_rails_sortable(model)
  # here they can use some authorization gem such as cancancan
  authorize! :update, model
end

In SortableController you can do something like this:

 def find_model(klass_to_id)
    klass, id = klass_to_id.values_at('klass', 'id')
    model =  klass.constantize.find(id.to_i)
    authorize_rails_sortable(model) if self.respond_to? :authorize_rails_sortable
    model
 end
itmammoth commented 6 years ago

Thanks @psmir I understand it, but I don't think depending on other libraries is the best way. I'll try to find the better way...

psmir commented 6 years ago

I don't think depending on other libraries is the best way

authorize_rails_sortable does not depend on other libraries. It is a wrapper. You could demand this wrapper to return a boolean value. In order to authorize sortable resources, developers should define the wrapper something like this

class ApplicationController < ActionController::Base
  ...
  # this name is more suitable
  def rails_sortable?(model)
     # just check owner
     model.user_id == current_user.id
  end

  # for cancancan
  def rails_sortable?(model)
     begin
       authorize! :update, model 
     rescue CanCan::AccessDenied
       return false
     end
     true
  end
  ...
end

You can use rails_sortable? method in SortableController to check records before sorting. If it returns false for some record you can skip one or raise an exception and stop sorting entirely.

I don't think this is the best solution. I just wanted to clarify my thoughts.

itmammoth commented 6 years ago

I understand your thoughts. Currently, SortableController does not intended to be inherited by controllers, so it is necessary to give an interception to controllers (or models) in another way.

ACPK commented 5 years ago

@psmir - Thank you for finding the bug!

@itmammoth - Due to the security implications, has there been any progress on this?

itmammoth commented 5 years ago

Sorry for no progress. I cannot get around to it now.

collimarco commented 5 years ago

It is not possible to use this library in production until this security bug is fixed :( Any workaround?

itmammoth commented 5 years ago

I made a PR to fix the issue. See the #40.

That might be NOT the best way, but I think good enough. Do you guys have any thoughts on this?

itmammoth commented 5 years ago

1.3.0 has been released.