Using `throw new Meteor.Error` to enforce permissions?

1

Would using throw new Meteor.Error be secure for enforcing permissions.

For example, we create a function for checking is the user is an admin:

checkAdmin = function (userId) {
	userId = userId || Meteor.userId()
	return !!Meteor.users.findOne(userId).admin
}

and then we run it inside of a method:

Meteor.methods({
	"removeUser": function (userId) {
		check(userId, String);
		checkAdmin();

		return Meteor.users.remove(userId);
	}
})

Figure it might be clean and unobtrusive.

1 Like
2

Throwing a Meteor.Error is definitely a solid option for halting your method execution. I’m guessing you meant to write something like this:

checkAdmin = function (userId) {
    userId = userId || Meteor.userId()
    if (!Meteor.users.findOne(userId).admin) {
        throw new Meteor.Error("not-authorized", "Not authorized.");
    }
}

There are a couple improvements I’d make before using this checkAdmin function a production application.

First, I wouldn’t assume that userId has been checked before passing it into checkAdmin. Imagine some scenario where you’re calling checkAdmin on a userId passed into a method (but never the current user’s _id; always user this.userId to get that). You might not check that userId in your method because you assume that checkAdmin will check it for you. In that case, a malicious user could pass up a query selector that always selects an admin, bypassing your checkAdmin assertion.

Assume that userId is { admin: true }. Calling checkAdmin({ admin: true }); will always pass, as long as an admin user exists in your database.

Even worse, because you’re dropping the unchecked userId directly into your findOne call, a malicious user could DOS your entire application by passing up a userId of { $where: "d = new Date; do {c = new Date;} while (c - d < 10000);" }.

That said, definitely check your userId in checkAdmin:

checkAdmin = function (userId) {
    check(userId, Match.Optional(String));
    userId = userId || Meteor.userId();
    if (!Meteor.users.findOne(userId).admin) {
        throw new Meteor.Error("not-authorized", "Not authorized.");
    }
}

Lastly, if a user isn’t found for a given userId, your checkAdmin will throw a "can’t get field admin of undefined" error, rather than your “Not authorized” error. This still works in securing your method, but it probably isn’t as helpful. I’d check that your user exists before checking if admin is truthy.

checkAdmin = function (userId) {
    check(userId, Match.Optional(String));
    userId = userId || Meteor.userId();
    let user = Meteor.users.findOne(userId);
    if (!user || !user.admin) {
        throw new Meteor.Error("not-authorized", "Not authorized.");
    }
}

Or a bit cleaner with some help from Lodash:

checkAdmin = function (userId) {
    check(userId, Match.Optional(String));
    userId = userId || Meteor.userId();
    if (!_.get(Meteor.users.findOne(userId), 'admin') {
        throw new Meteor.Error("not-authorized", "Not authorized.");
    }
}

P.S. I’m working on a project called Secure Meteor, which will be a book/guide on securing your Meteor application with a heavy emphasis on explaining the nuts and bolts of Meteor-specific vulnerabilities, how they work, and how to prevent them. If you’re interested in that kind of thing, sign up and I’ll let you know when it’s finished!

3 Likes
3

Great points. I’m wrapping up a 6 month project… so my apologies for that embarrassing code snippet. Looking forward to your book.

2 Likes