ruby-hyperloop / hyper-mesh

The project has moved to Hyperstack!! - Synchronization of active record models across multiple clients using Pusher, ActionCable, or Polling
https://hyperstack.org/
MIT License
22 stars 12 forks source link

Security hole - it might be possible to get a list of all id's for a model #45

Open catmando opened 6 years ago

catmando commented 6 years ago

this https://github.com/ruby-hyperloop/hyper-mesh/blob/ebf661eecc592799b7723bb666c1d42d84551918/lib/reactive_record/server_data_cache.rb#L182

reads every id, but is not protected by the check_permission_with_acting_user method

it should read:

collection = cache_item.value.collect do |record| 
  record.check_permission_with_acting_user(@acting_user, :view_permitted?, :id)
  record.id
end
cache_item.build_new_cache_item(collection, method, method)

Similar to https://github.com/ruby-hyperloop/hyper-mesh/issues/43 .

Hopefully adding that line will not kill performance!

catmando commented 6 years ago

Be aware however that any record that has ANY attribute accessible to a channel will automatically be grant access of the id to that channel. This is to allow push synchronization. So for example if my User instance channel has access to the user's name (for example) then each user will also have access to the :id of each record that belongs to them.

I don't think this is an issue FYI... but if it is then we would have to implement an optional encryption of the id before transmission (and decryption after reception.) I would make optional so that if you are not worried about it you don't have to pay for the encrypt/decrypt overhead. Also would probably be handy during development not to encyrpt/decrypt the ids.

catmando commented 6 years ago

FYI aggregations might have a similar problem, however i think we should just Make a caveat, as those things are so hard to deal with. I would rather we just had a config switch that turned them off.

janbiedermann commented 6 years ago

Thanks for generously assigning this to me. I will immediately put it, in bold, to make sure of its importance, on my hyperloop todo list. To fix it ASAP, i put it at the end of the list, so it will be the first item after all the other items on my list.

catmando commented 6 years ago

:-)

catmando commented 6 years ago

I'll try to get done as well

catmando commented 6 years ago

@janbiedermann sorry abt assigning problem. Fix should be just as shown, but I am concerned about perf impact. You r kind of master if that. If you check it out perf wise I will do test cases etc.

janbiedermann commented 6 years ago

in progress. So far:

lib/active_record_base.rb:

lib/reactive_record/permissions.rb

(hyper-mesh.rb modified accordingly)

lib/hyperloop/internal_policy.rb added:

added in lib/reactive_record/proxy

Proxy Interface may change slightly with further progress, maybe.

added lib/reactive_record/graph.rb class ReactiveRecord::Graph < BasicObject class Node < BasicObject internal to Graph interface still in flux

Ill update this to keep track.