The problem is exactly that. “Check your allow/deny code” is a hard to do when it’s something you wrote yesterday, very hard when it’s something from last week. You’ll be a hero fixing others allow/deny rules.
1 Like
Yeah I mean the workflow for securing an allow-deny app is something like:
- Predict all possible updates that could be done from the client
- Write a function that somehow secures all possible forms of input that could come in
- Hope that none of your client code is dynamically generating mongo update queries such that they might fail in certain cases
- Cry?
Sure, if you use client updates in a very principled way and stick to only $set for example, then maybe it’s not so bad. But in that case, why give yourself all of the rope of sending Mongo queries over the wire if you could just send pre-agreed upon arguments, AKA method calls?
Plus, good luck ever changing your database schema if all of your client operations assume a specific set of fields for their updates.
I haven’t faced the issues you are describing. I think it’s because we are very principled in our design and in our usage, so our allow / deny mostly look at permissions.
Writing methods does work, but it’s cumbersome. Not only that, but dev time is so much shorter when you can have real client-side updates.
1 Like
I’ve heard this argument, but I’d really love to have some sort of head-to-head competition where two people try to build a relatively complex yet secure app, one with methods and one with allow/deny. I’d put my money on the method approach resulting in a secure, functional, and well-tested app faster. But I could be wrong, of course.
1 Like
OK, just modified my boilerplate code to use ValidatedMethod. Reminds me of JEE programming where we hide direct database access and wrap everything in a DAO. Well that’s what I am more familiar with to begin with so I think that’s fine if I am wearing my security conscious hat, rather than let’s-get-the-bloody-product-out-the-door-first hat. 
I looked at the guide on how it is implemented, sort of left a bit of a bad taste in my mouth though. Even the way the secret server code was implemented in the guide didn’t really seem right. So after a few iterations, I came up with something else. (I only show the example of one method, but the concept is the same for other methods)
import { Mongo } from 'meteor/mongo'
import { ValidatedMethod } from 'meteor/mdg:validated-method'
import { LoggedInMixin } from 'meteor/tunifight:loggedin-mixin'
import { SimpleSchema } from 'meteor/aldeed:simple-schema'
class TasksCollection extends Mongo.Collection {
constructor () {
super('tasks')
this.insertTaskMethod =
new ValidatedMethod({
name: 'tasks.insert',
mixins: [LoggedInMixin],
checkLoggedInError: {
error: 'notLoggedin'
},
validate: new SimpleSchema({
text: {
type: String,
optional: false
}
}).validator(),
run: (params) => {
super.insert(params)
}
})
}
get insertTask () {
return this.insertTaskMethod
}
}
export const Tasks = new TasksCollection()
When invoking, I used
Tasks.insertTask.call(this.entry, (err) => {
if (err) {
this.error = err
}
this.state.go('^.list')
})
For @Urigo This pattern should be closer to what you would need in socially.
@sashko the one thing that is annoying in the pattern is the need for .call, becauseI can’t seem to get away with return new ValidatedMethod({..}).call to just get the call function, I wonder if there’s a way around it, maybe Function.prototype.apply() or something.
Also for those who are thinking of using the same pattern I have above, if you use insert, update, delete … you won’t be able to easily change the run function on the server side like the following example of secret sever code
Tasks.insertTask.run = (params) => {
Tasks.insert({
text: params.text,
createdOn: new Date()
})
}
1 Like
Yeah we probably need to add something like this.call.bind(this) in the constructor here: https://github.com/meteor/validated-method/blob/master/validated-method.js
Glad to take a PR!
Also, ValidatedMethod doesn’t have to be the end-user API - you could easily think of some nice wrapper packages to make it a lot more concise.
The main point here is, we’re using a real programming language. Don’t like the tools you have? Make new tools!
1 Like
Problem with making new tools is I need to support them, I can’t just tell the next guy just go follow @sashko on github and the forums.
But honestly with the exception of the mixins and name and the fact that I have to keep on saying new SimpleSchema every time it looks good already.
1 Like
Well, we did just that and found three lines of allow / deny secured it for us. The client writes directly to the Collection. If we used methods, each type of write / update (and there are plenty) would require a new method, that needs to be secured etc. vs securing those three lines of code.
Now, admittedly, a malicious (or rather stupid) user can try to write to the Collection directly, but they will only mess up their own experience, the three lines of allow / deny secure the server (and other users).
1 Like
Also, Meteor is both fun and efficient. By starting to slowly chip away on the functions / functionalities that made it so, we are ending up with a more complex framework that really looses a major competitive edge.
The whole Blaze concern is part of this conversation, it’s about ease of use. No one is married to one approach, it’s just that the initial design made things so much faster and smoother. I hope we don’t loose this as we complexify things (or as Apollo is launched).
1 Like
Happy to work together to fix this!
1 Like
Thanks, I already started though because I got annoyed at the repetition.
import { ValidatedMethod } from 'meteor/mdg:validated-method'
import { LoggedInMixin } from 'meteor/tunifight:loggedin-mixin'
import { SimpleSchema } from 'meteor/aldeed:simple-schema'
import _ from 'lodash'
/**
* Extends validated method to require the user to be logged in.
*/
export class LoggedInValidatedMethod extends ValidatedMethod {
/**
* @param {string} name DDP method name
* @param {Object} schema SimpleSchema schema
* @param {Object} options other options for validated method
*/
constructor (name, schema, options) {
super(_.extend({}, options, {
name: name,
mixins: [LoggedInMixin],
checkLoggedInError: {
error: 'notLoggedin'
},
validate: new SimpleSchema(schema).validator()
}))
}
}
Usage:
get insertTask () {
return new LoggedInValidatedMethod('tasks.insert',
{
text: {
type: String,
optional: false
}
}, {
run: (params) => {
super.insert(params)
}
})
}
Apparently EcmaScript does not allow me to do arguments.callee or arguments.caller so I can get the name of the function so I don’t have to type in the DDP name.
1 Like
I think we need a package called disableAllowDeny that will shame the developer in the console if they ever use. My opinion: from a business perspective, it’d be well worth $99.
3 Likes
Allow/deny made sense at the time I was developing the app, then moved on to methods. So its something I know I have to remove soonish.
With this being the case, is MDG going to remove it from Meteor at some point?
Also, the one legitimate case to use deny that I’ve heard is on the profile field of Meteor.users since that is default writeable. Other than that, it isn’t needed.
Most production apps disable the ability to use allow/deny altogether, even on the profile. If there’s a way the database can be made vulnerable, it should be removed/prevented.
Methods are much more secure and practical.
1 Like
You have deny rules in the todo app?
This is because it’s not possible to globally disable allow/deny without that set of rules:
@sashko regarding the Todo app – I thought everything was denied by default. Is this incorrect?
Related:
I just loaded up my app, and performed the query:
//this works
var myId = Meteor.userId();
var huge = _.range(100000).join("let's attack the server/db and waste some bytes! ")
Meteor.users.update(myId, {$set:{profile:huge}});
I remember learning about this loophole back in the day. I am surprised to see this flaw STILL exists! I thought that was patched up in Meteor 1.1…
What’s the recommended way to fix all of these loopholes?
I would rather not be forced to do this:
collectionsList.forEach(function(collection){
collection.deny({
insert(){return true}
update(){return true}
remove(){return true}
})
})
3 Likes
I think it’s worth deprecating allow/deny for the next release of Meteor.
MDG is taking the right steps and should continue to make opinionated stances on development with Meteor - really for the sake of users who don’t know better or are being pushed to learn all of this in a short amount of time (as we all know many companies do this with employees).
Less security flaws = better.
2 Likes
Right unfortunately that’s currently the only way. If anyone has a design or pr to make it easy to turn off allow deny please let’s make it happen!
1 Like