Skip to content
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

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

Closed
psmir opened this issue Aug 9, 2018 · 10 comments
Closed

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

psmir opened this issue Aug 9, 2018 · 10 comments

Comments

@psmir
Copy link

psmir commented Aug 9, 2018

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
Copy link
Owner

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

@psmir
Copy link
Author

psmir commented Aug 20, 2018

@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
Copy link
Owner

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
Copy link
Author

psmir commented Aug 21, 2018

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
Copy link
Owner

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
Copy link

ACPK commented Oct 29, 2018

@psmir - Thank you for finding the bug!

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

@itmammoth
Copy link
Owner

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

@collimarco
Copy link

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

@itmammoth
Copy link
Owner

itmammoth commented Jul 24, 2019

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 added a commit that referenced this issue Aug 11, 2019
[#30] Use message_verifier to prevent html from tampering
@itmammoth
Copy link
Owner

1.3.0 has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants