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

Collection.update(... {multi:true}) can still be run on the client #3271

Closed
dandv opened this Issue Dec 11, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@dandv
Contributor

dandv commented Dec 11, 2014

Normally,

Players.update({}, {$inc: {score: 1}}, {multi: true});

will trigger this exception:

Not permitted. Untrusted code may only update documents by ID.

But performing the update via the collection property of a cursor will work:

Players.find().collection.update({}, {$inc: {score: 1}}, {multi: true});

Thankfully this only works on the client, but I thought I should report it since collection isn't marked as a private property of Cursor.

@dandv

This comment has been minimized.

Show comment
Hide comment
@dandv

dandv Dec 11, 2014

Contributor

By the way, the security this check provides is easy to bypass by just looping through the results of .find() and calling update on each _id, so what if it was removed altogether?

Contributor

dandv commented Dec 11, 2014

By the way, the security this check provides is easy to bypass by just looping through the results of .find() and calling update on each _id, so what if it was removed altogether?

@steph643

This comment has been minimized.

Show comment
Hide comment
@steph643

steph643 Dec 11, 2014

+1
Several times in the past, for debugging purpose, I had to bypass this security check using a loop. This made me mumble about this strange feature. Is it really necessary?

steph643 commented Dec 11, 2014

+1
Several times in the past, for debugging purpose, I had to bypass this security check using a loop. This made me mumble about this strange feature. Is it really necessary?

@dandv

This comment has been minimized.

Show comment
Hide comment
@dandv

dandv Dec 11, 2014

Contributor

I've been thinking about this some more, and I believe now that it may lead to an unintended consequence of worse security for use cases when an arbitrary collection must be modified.

This is actually a pretty important use case for Meteor: integrating with widget libraries that operate on multiple records from a collection they are passed. For example, a grid component may want to delete a batch of records as a response to a user action, or a reorderable list component may need to update multiple indexes for its items at once.

{{grid items=players}}

Right now we have two solutions:

  1. collection.find({...}).forEach and issue updates by _id. This is obviously inefficient because it sends one update message for each document, instead of the server running only one multi-update query.
  1. Define a Meteor.method to update the collection on the server with {multi: true}. The problem is you'll have to pass a collection name as a parameter, and the server will have to look it up in the global namespace. This is what @mizzao's autocomplete does:
collection = global[collName]
...
collection.find(...)

This is bad for three reasons:

  1. Via such a method, the server offers arbitrary access to its collections and has to resort to odd workarounds to keep some collections private (use random identifiers for them?)
  2. The recordset that's being modified on the client is a subset of the recordset on the server. Telling the server via Meteor.methods to update based on a selector may select more records than was intended on the client.
  3. The records on the client might have arrived from multiple collections. This makes it even more complicated to tell the server what to update.

I'm sure there were good reasons for denying the client the update(...{multi: true}) feature. What were they? Can this decision be revisited?

Right now it seems that the security advantage can very easily be bypassed from the console, and the downside is a drastic reduction in update performance on the server.

Contributor

dandv commented Dec 11, 2014

I've been thinking about this some more, and I believe now that it may lead to an unintended consequence of worse security for use cases when an arbitrary collection must be modified.

This is actually a pretty important use case for Meteor: integrating with widget libraries that operate on multiple records from a collection they are passed. For example, a grid component may want to delete a batch of records as a response to a user action, or a reorderable list component may need to update multiple indexes for its items at once.

{{grid items=players}}

Right now we have two solutions:

  1. collection.find({...}).forEach and issue updates by _id. This is obviously inefficient because it sends one update message for each document, instead of the server running only one multi-update query.
  1. Define a Meteor.method to update the collection on the server with {multi: true}. The problem is you'll have to pass a collection name as a parameter, and the server will have to look it up in the global namespace. This is what @mizzao's autocomplete does:
collection = global[collName]
...
collection.find(...)

This is bad for three reasons:

  1. Via such a method, the server offers arbitrary access to its collections and has to resort to odd workarounds to keep some collections private (use random identifiers for them?)
  2. The recordset that's being modified on the client is a subset of the recordset on the server. Telling the server via Meteor.methods to update based on a selector may select more records than was intended on the client.
  3. The records on the client might have arrived from multiple collections. This makes it even more complicated to tell the server what to update.

I'm sure there were good reasons for denying the client the update(...{multi: true}) feature. What were they? Can this decision be revisited?

Right now it seems that the security advantage can very easily be bypassed from the console, and the downside is a drastic reduction in update performance on the server.

@n1mmy

This comment has been minimized.

Show comment
Hide comment
@n1mmy

n1mmy Dec 11, 2014

Member

It is very important not to allow running arbitrary selectors from clients. By running carefully constructed selectors, a client can actually read unpublished data, including secret tokens! See https://groups.google.com/forum/#!searchin/meteor-talk/0.5.8/meteor-talk/qUa3clv4g5E/N7F8IaAX5AgJ for details.

Member

n1mmy commented Dec 11, 2014

It is very important not to allow running arbitrary selectors from clients. By running carefully constructed selectors, a client can actually read unpublished data, including secret tokens! See https://groups.google.com/forum/#!searchin/meteor-talk/0.5.8/meteor-talk/qUa3clv4g5E/N7F8IaAX5AgJ for details.

@dandv

This comment has been minimized.

Show comment
Hide comment
@dandv

dandv Dec 11, 2014

Contributor

Thanks @n1mmy; nifty attack. I've posted in that thread - is there a better way to update multiple records in arbitrary collections from the client than a collection.find().forEach loop or a Meteor.method that takes the collection name as a parameter?

Contributor

dandv commented Dec 11, 2014

Thanks @n1mmy; nifty attack. I've posted in that thread - is there a better way to update multiple records in arbitrary collections from the client than a collection.find().forEach loop or a Meteor.method that takes the collection name as a parameter?

@glasser

This comment has been minimized.

Show comment
Hide comment
@glasser

glasser Dec 11, 2014

Member

Additionally, that collection that you found (which probably should have a more underscored name) is an internal detail of the Mongo.Collection. The modification that you are doing does not affect the server.

Member

glasser commented Dec 11, 2014

Additionally, that collection that you found (which probably should have a more underscored name) is an internal detail of the Mongo.Collection. The modification that you are doing does not affect the server.

@glasser glasser closed this Dec 11, 2014

@n1mmy

This comment has been minimized.

Show comment
Hide comment
@n1mmy

n1mmy Dec 12, 2014

Member

Well, you have to get the collection name from the client to the server somehow. Your options are basically to have it as an argument to the method (as you mention), or to have it in the name of the method (like the default methods controlled by allow/deny).

They're pretty equivalent, really. Just a question of how you want to structure your code. Either way, you probably want to structure things so collections opt in to being modified (like allow/deny). This is how you can avoid the 'client can modify arbitrary collections' issue you mention. And you want to avoid running arbitrary selectors from the client. For example, you could pass a list of ids instead of a selector.

Member

n1mmy commented Dec 12, 2014

Well, you have to get the collection name from the client to the server somehow. Your options are basically to have it as an argument to the method (as you mention), or to have it in the name of the method (like the default methods controlled by allow/deny).

They're pretty equivalent, really. Just a question of how you want to structure your code. Either way, you probably want to structure things so collections opt in to being modified (like allow/deny). This is how you can avoid the 'client can modify arbitrary collections' issue you mention. And you want to avoid running arbitrary selectors from the client. For example, you could pass a list of ids instead of a selector.

@dandv

This comment has been minimized.

Show comment
Hide comment
@dandv

dandv Jan 21, 2015

Contributor

Thanks @n1mmy. I ended up passing a list of IDs.

Contributor

dandv commented Jan 21, 2015

Thanks @n1mmy. I ended up passing a list of IDs.

@vsivsi vsivsi referenced this issue Jul 13, 2015

Open

Sortable #2

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