@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 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).
The allow/deny() are function callbacks, so somewhere in the code there would be something that calls such callbacks. We just need to find the one that calls the deny() callback for the collection and make it assume it is always true.
//populate collectionList with a pointer to each collection
var deny = function(){return true}
var all = {insert:deny, update:deny, remove:deny}
collectionList.forEach(function(c){
c.deny(all);
})
But this should only be necessary for Meteor.users collection, I think every action to other collections are auto-denied by default.
Otherwise, I think @trajano is on the right track… the entire API should just go.
Ease of use is one thing, but knowing every client-side entry point to your db (and being able to predict the ways in which those entry points could be hacked/manipulated) seems too important to me not to use methods 100%.
Which is partially why I created rlivingston:simple-schema-mixin. Include that mixin and the validate option can be replaced by a schema option. There are also techniques that can allow you to reuse your existing schema for most parameters via a combination of SimpleSchema’s pick and schema functions. See the README.md for examples.
Honestly, in my own code I also wrote a package with a class wrapper for ValidatedMethod that I call SimplyValidatedMethod. It automatically includes the SimpleSchemaMixin so that I don’t have to specify mixins on every ValidatedMethod definition. I’ve thought about developing that into a general purpose package that allows someone to specify whatever mixin list they choose and any other options that might be the same in all of their ValidateMethod definitions. But, I think that would undermine ValidatedMethod. It would be better if an extension were made within the ValidatedMethod package to help eliminate that type of duplication.
I feel you’re misunderstanding the fetch rule. This only limits the database fields fetched and made available for the code that you place in your insert, update, and delete rules. From the meteor docs:
When calling update or remove Meteor will by default fetch the entire document doc from the database. If you have large documents you may wish to fetch only the fields that are actually used by your functions. Accomplish this by setting fetch to an array of field names to retrieve.
So, my understanding is that the fetch within allow/deny rules doesn’t limit what your collection find operations can access. Only the publication can do that.
This * 10,000. With allow/deny there’s one place that decides whether that user can edit it the document or not. With 50 methods that can modify a collection, it’s a lot more likely that someone forgot to do that check.
You’ll typically only ever have one or two insert, update, and remove methods per collection. If you have 50 then you might want to consider rewriting your application in a more concise and efficient manner.
Allow/deny is basically just a wrapper for methods - because allow/deny ultimately uses/transpiles into methods.
Also, the check package is great for letting you know when you haven’t done a check in a method.
Okay, 50 was purposely excessive. But one or two is not realistic either (outside of a basic TODO app), you’ll end up with just as much of a mess as you would if you just used allow/deny in the first place.
The check package is used for checking methods, audit-argument-checks is what you’re thinking of. Which is definitely a good package. Unfortunately it doesn’t allow you to use ES6 property destructing or rest operator.
Hey, I know what’s the problem is The framework needs 2-3 (or even 20-30) ‘reference apps’ so that it is really stays up to its purpose. Probably it already has behind the scenes (high paying customers on Galaxy).
Otherwise all this theoretical talk is similar to “how to educate a kid” - with single truth “allow/deny list” or with explaining that in each of methods he or she has… what are the users he/she will interact with? are they good and will not try to abuse him/her? But we have only simple to-do kid, zygote, not even embryo and trying to predict his/her behaviour in university.
I’m on the method side of this debate and yet I don’t buy one or two. I’m probably about half-way in between these two extremes.
I find that the number of methods tends to be directly related to the number of fields, and meteor development tends to drive me to a relatively flat schema. I write methods that update single fields much of the time. The exception might be when fields are highly related and tend to change in concert with each other.
I think that reactivity is the largest factor driving this approach. The old approach of holding onto changes and sending them all at once with a submit is mostly dead. It still throws me sometimes as a user that very few applications have me apply or submit option, setup, and profile changes for example. So, a field change usually results in a method call that updates a single field at the server - reactively driving immediate response to the change back to the client.
But, I don’t consider this a factor in why I threw away allow/deny. I threw them away because the decisions as to what to allow or deny are complex. In my app they are very often per field and depend on the user’s relationship to the data (owner, contributor, stakeholder, disseminator, etc.) and even sometimes the exact nature of the change. Whether to allow or deny an update is just not as simple as whether or not the user is an admin or logged in anymore.
So, I feel that I need to carefully consider the rights for the data being updated by each method, not for each collection.
Of course, if you implement allow/deny at the method level, you’ve essentially eliminated this argument. At that point, it’s just terminology.
I’m on the method side when I have to work with a team and I am responsible for working on the server side. Primarily because I would like to know how things are controlled. That’s from my experience on larger scale enterprise applications.
However, when the UI is still not finalized and it’s more of a collaboration/shared documents app installed on an intranet, I would use use allow/deny and just have a simple security check to ensure that someone is logged in and in the proper role. This simplifies the development on both sides of the fence. Mind you the security and data set of those systems are a bit more lax since its a limited number of users that know each other and it’s more of helping them manage their workflow rather than putting things out to the public.
Agree. It’s sometimes nice to keep things simple while prototyping or for an intranet, and you often can because your exposure is less.
In my case, I’m a team of one but, because my apps tend to be large scale, I don’t trust myself to think of all angles all of the time. So I impose the same structure on myself I used to impose on my team. I deny everything and carefully punch through with methods.
It’s entirely possible. And may be evidence that the allow/deny documentation was never clear to begin with. I’ve understood the following line to be equivalent of an allow all or allow * firewall rule for a few years now. Maybe that’s incorrect. But based on how it was written up on docs.meteor.com, that’s how some people have been interpreting it.
I’m sympathetic to this view, and think it’s useful to trace the history of how allow/deny came to be. Originally, it was an extension of the livedata system right? And livedata is a pub/sub system. Why did Meteor choose to implement pub/sub? Because it was a javascript version of the pubsubhubbub protocol. And atom/rss style publication feeds were the early aughts real-time alternative to prefetching data with polling intervals.
So with basic stateless HTTP Ajax calls, data gets fetched and that’s it. Stateless, scalable, secure, and all that. But because it’s stateless, there’s no mechanism for real-time updates; so people resort to polling, which leads to prefetching, and then publications/subscriptions. As that happens, security becomes a concern, and somebody along the way slaps together an app-level firewall. The firewall has allow() and deny() rules. And since we’re implementing subscription/prefetching functionality, lets just assume that the firewall is going to have an implicit allow all rule on fetching data; because it would sort of defeat the purpose of livedata to firewall the outbound updates, right? And then, hey, lets do some refactoring, and move the firewall stuff to it’s own package; and the prefetching stuff to it’s own package.
I could be completely wrong about that sequence of events. But it seems pretty obvious to me that something like that is what happened, more or less.
So, just a couple questions:
Would Firewall.allow() and Firewall.deny() be a good namespace to move the functionality under? Does it behoove Apollo to have a Firewall object of some sort? Semantically and educationally, this is a coherent API that people are familiar with.
As we move to ValidatedMethods, is Apollo going to re-address the use-cases of prefetching data? There’s this tiny sliver of functionality in allow/deny that acts as an allow all rule for the rest of livedata. Maybe other people don’t see that. But it seems obvious to me that there are remnants of that era of design. Simply curious to know if we have any idea yet what the new mechanism for publications/prefetching/livedata is going to be in a stateless method-centric Apollo world. (Last I heard, livedata was going to live side-by-side with Apollo; and that polling was going to be the recommend strategy???)
@rlivingston@Volodymyr@workman Just to clarify, when I said one or two of each per collection, I meant one or two inserts, updates, and removes, per collection.
For example:
Theoretical Posts Collection
Insert method
Insert method
Update method
Update method
Remove method
Remove method
That’s usually how it plays out per collection. I’m running a production app right now that literally has a single insert, update, and remove method per collection, with a few rare cases where I need more than one of each.
Really, the argument can’t be made against methods being bad for apps with lots of inserts, updates, and removes. If anything, methods are better when your app has more inserts, updates, and removes - you can have one place for all of your method logic as opposed to having twenty places in the UI code where an insert, update, or remove is called.
Regardless of the arguments that will ensue, it’s good that we’re having this conversation.
@repsonsive, @sashko
That’s a good point – being new dev friendly. Apps grow and change and get hardened. Making it simple to start with is NOT a bad idea. No one starts with a 1k lines of code day one to start testing and prototyping.
