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

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

Open
testbird opened this issue Jan 25, 2014 · 7 comments
Open

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

testbird opened this issue Jan 25, 2014 · 7 comments

Comments

@testbird
Copy link

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});
@julpod
Copy link
Owner

julpod commented Jan 25, 2014

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!

@testbird
Copy link
Author

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

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.)

@julpod
Copy link
Owner

julpod commented Jan 27, 2014

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,

@testbird
Copy link
Author

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" (#9 (comment)) 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?

@julpod
Copy link
Owner

julpod commented Jan 28, 2014

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!

@testbird
Copy link
Author

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.

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

No branches or pull requests

2 participants