Dot operator fields - deny security

Security
1

Hello,

I’m trying to prevent people from editing a list of fields (readonlyFields) using the following code

var readonlyFields= ["billing"];
People.deny({
	insert: function(){
		return true;
	},
	update: function(userId, doc, fields, modifier){
		if(!fields || _.intersection(fields,readonlyFields).length > 0)
		return true;
	},
	delete:function(){
		return true;
	}
})

However, if billing is an object, and has children such as an array called paymentHistory, etc., will this code prevent somebody from fiddling with their payment history? Could they update billing.paymentHistory and thus bypass the deny rule? Or is Meteor smart enough to pass billing in as a field even though they are updating billing.paymentHistory?

Thank you!

2

I think generally for something you need a high level of security on - like billing data - you don’t want to let clients make those modifications on the client. You want to do those changes via very controlled Methods. Methods don’t use the allow/deny rules, so you need to build the security into the Method function itself. You can have shared methods (in /lib) which exposes the code to the client. I typically put secure stuff like this in the /server directory only to prevent clients from seeing what is going on in the server during the call.

edit - sorry I know I didn’t answer your question. I don’t use allow/deny rules, so don’t want to tell you the wrong thing. Just an observation!

1 Like
3

Note that the Meteor guide strongly suggests avoiding Allow/Deny: http://guide.meteor.com/security.html#allow-deny

4

So is Meteor going to get rid of them entirely someday? Maybe they were added because it seems like a good idea but then after time it was decided that they are insecure.

5

Yeah that’s about right. I put in a bit of work a while ago to start to make it removable. But getting rid of it entirely would be a breaking change so the best we can do is recommend against it. Perhaps we should remove it from the docs as well.

Also to be clear allow/deny isn’t broken or anything, it’s just really really hard to write correct code using that API.

6

Makes sense. Maybe a warning in the docs would be even better because people are bound to see tutorials that tell them how to use allow/deny. If its not in the docs they may just find other sources. But if it is in the docs, then they will see that it’s not recommended.

2 Likes
7

How about a console.log() warning which only appears when running in development mode, pops up in console warning users and pointing them to the guide? This way it doesn’t impact people moving forward if they want to use it. there could even be a variable in settings.json which suppresses the warning for those who really want to keep using it and don’t want to be bothered.

3 Likes