Deny operations from the client

[fjorgemota]

Hello!

I'm doing a bit of search about Meteor and some of it's packages and, while reading the README and code of this project, I found a question: How I can to deny operations from the client?

I tried adding

tx.checkPermission = function() { return false }

To the shared code of the application but, after the addition of that, I cannot do transactions emitted by the server, and to me seems better to do insert/update/delete operations just from Meteor methods.

Is there a good way to do that?

I'm awaiting for answers. And sorry for my (bad) english. :)

[JackAdams]

The only tx.checkPermission function that counts is the one defined on the server. If you're making instant writes on the client {tx: true, instant: true}, yes, those are subject only to your allow and deny rules. It's the developer's responsibility to open up / lock down the allow/deny rules anyway, otherwise people could open the console and start typing Posts.insert({whatever: "they like"}) whether the babrahams:transactions package was there or not.

If you really want to do instant updates in client side transaction code (not recommended, but possible), the way to secure this and achieve consistency is by having your allow/deny permission logic match your tx.checkPermission server logic very closely (ideally just share the logic).

I'd recommend doing most (if not all) db mutations by calling a method and having the transaction execute entirely in server side code.

[fjorgemota]

Great.

I'm really searching how to do operations just from the server, but now I have found that I can use something like

tx.checkPermission = function() { 
    return false; 
}

and add overridePermissionCheck: true to each transaction made in the server.

To me, seems like a good option for the moment.

Thanks for the answer. :)

[JackAdams]

Yes, that's one way of doing it. I have some generic permission checking logic that I put in the tx.checkPermission function so that all data mutation attempts via a transaction have to pass that first. Saves me from having to repeat myself in every method call.

I should also have mentioned that tx.checkPermission is completely permissive by default so that it "just works" when you first add the package but, for production, it should be overwritten to return false for every call except the ones you explicitly permit.

[fjorgemota]

I'm testing here and, even when tx.checkPermission returns false, the client can set overridePermissionCheck to true to update the database normally, even passing the check in the server

Is this intended behavior? To me, makes sense to allow the use of overridePermissionCheck just on the server, never on the client.

What you think? Any tips there? If you want, I may reopen the issue so we can check that.

[JackAdams]

Do you have:

tx.permissionCheck = function () {
  return false;
}

in server code?

If so, this is a major security hole.

[fjorgemota]

Yeah.

Actually, it's something like:

tx.checkPermission = function(action, collection, doc, modifier) {
    "use strict";
    return false; // We will always deny transactions from the client
};

Because..as I said, I don't want to allow operations from the client. But, even with that on the server, if I run the following code on the Firefox's console dev tools

tx.start("some update");
Items.update({"_id":new Mongo.ObjectID("56c33ece18d5d454b879a1c1")}, {"$set":{"favorited":true}}, {tx:true, overridePermissionCheck: true});
tx.commit();

I'm able to see in the server log a "Executed update" message, with the row updated in all the browsers, and without any other modifications anywhere (btw, the insecure package is already removed in the project)

[JackAdams]

Just had a look through the code and you may be right. I'll need to write some automated tests and test against those. If this is a bug, I'll be: 1) very grateful to you for bringing it up 2) fixing it and pushing a new version out later today

[JackAdams]

Oh boy. That's bad. I think you're absolutely right. Will close that (gaping) security hole tonight.

[fjorgemota]

I readed the code too and don't found anything against this problem.

By the way, i'm happy to found that bug. (I really think what a mess this can cause..)

I'm trying to imagine viable solutions here. When I come to some conclusion I will suggest here.

[JackAdams]

Don't worry, I've already got the solution. Just need to push a new release to atmosphere, but want to write some automated tests first, so this NEVER happens again!

My gosh ... I've been using this in production for years!

New release will be out in a few hours' time.

[fjorgemota]

Oh. Great that you already found a solution.

I'm awaiting for the update. :)

Good tests!

[JackAdams]

Yep, the update is ready -- tests are all passing. I'm just updating some dependencies at the moment. Will release the new version in an hour or so.

[fjorgemota]

Great. Thanks! :)

[JackAdams]

Okay the new version (0.7.6) is released on atmosphere. Let me know if something's not working as it should! Many thanks for taking the time to raise this issue. It was a seriously bad security hole.

[fjorgemota]

What do you think about updating CHANGELOG.md with that version?

By the way, i'll test here and i'll keep you informed. :)

Thanks and keep the good work! :+1:

[JackAdams]

Haha... I just updated the CHANGELOG.md, then 10 seconds later saw your comment. Let me know if you find anything alarming in the new release. :-)