filter-collections icon indicating copy to clipboard operation
filter-collections copied to clipboard

whitelist query parameters (instead of passing client's object)

Open testbird opened this issue 12 years ago • 7 comments

The publish function should only publish on a whitelist basis.

Instead of executing client supplied finds:

collection.find(query.selector, query.options);

Only _.pick out whitelisted field: parameters, and use the other parts only as explicit sort and limit objects in the find:

collection.find(query.selector, {sort: query.options.sort, limit: query.options.limit, fields: query.options.fields});

testbird avatar Jan 25 '14 16:01 testbird

This looks great but honestly I didn't fully understand the benefits of this (please consider that I've stated with meteor two months ago :P).

Are you considering possible security problems? Performance? What would be the improvements of modify publisher this way?

Thanks a lot!

julpod avatar Jan 25 '14 19:01 julpod

Yes, security. It's to consider that the query object comes from the client. Someone may be trying to send a query to access information that should not be accessible to him. Thus, publish function have to check client input, and make sure it is only used in a save way.

testbird avatar Jan 25 '14 19:01 testbird

With fields: you can restrict what gets published, but it would not help if the client could just send its own field: to override this. (Therfore the idea to have a "whitelist" used to publish just a selection of fields, and also to only consider fields from the whitelist in the client requests.)

testbird avatar Jan 25 '14 19:01 testbird

I've been working in a first approach about security. I've added a new method "allow" for the publisher that will let developers do some custom validation before any publisher run anything.

The approach is at https://github.com/julianmontagna/filter-collections/tree/0.1.2 (with updated README.md)

If you are available, let me know your thoughts. Thanks,

julpod avatar Jan 27 '14 19:01 julpod

I see, the "allow" is usefull for example to block all access for non-admin users etc. As an undefined function allows writes, maybe rather call it deny? http://docs.meteor.com/#allow

To return only specified whitelisting fields, it may be possible to take a list of whitelisted fields and add those fields to the fields: dictionary that are in the whitelist AND present in the user's query, but if the result is an empty list, add all the whitelisted fields. http://docs.meteor.com/#fieldspecifiers

The other part would be to restrict the maximal possible "scope" (https://github.com/julianmontagna/filter-collections/issues/9#issuecomment-33315585) for the user. Maybe by defining a minimal set of query selectors that are always added?

Overall, maybe there is a more universal way out of this, that may also make c-f more flexible to use, if the approach could be turned around. Rather than adding special parameters to make c-f more flexible (but harder and peculiar to configure), maybe c-f features could be added to custom publish() functions (#10). Maybe by adding some c-f supplied sort, filter, skip, limit objects?

testbird avatar Jan 27 '14 20:01 testbird

Yes, 'allow' will be 'deny' and is not 'fields'. I've pushed that since I needed on the project that I'm working for as a first approach of "security"? :P

Getting back to 'fields:' topic, I'm asking myself if a possible implementation could be:

Meteor.FilterCollections.publish(People, {
  name: 'someName',
  fields: {_id: 1, name: 1, "contacts.phone": 0, "contacts.email": 0}
});

and/or support also something like:

Meteor.FilterCollections.publish(People, {
  name: 'someName',
  fields: function(){

    var fields = {};

    fields._id = 1;
    fields.name = 1;

    if(!someRestrictionRule)
      fields.contacts = {
        phone: 0,
        email: 0
      }

  }
});

Then, the publisher will attach this to the query before building the cursor (.find) and this way we should have a way to restrict the document fields sent back to the client. Also, if no 'fields:' value was configured, all fields will be sent.

That's sounds good to you? Please, let me know your thoughts. Thanks!

julpod avatar Jan 28 '14 14:01 julpod

In js I am really a bloody newby. Still, that api would look quite flexible to me. :)

When "attaching" the field dictionary from the client, just make sure only whitelisted fields are added (those also present in the whitelist).

The docs left me in the impession that a someField: 1 implies all others are omitted, so in the example above you may have to also use contacts: 1 to then selectively omit the phone and email.

testbird avatar Jan 28 '14 15:01 testbird