How to ensure security in Meteor methods

[minfawang]

In the example, the Meteor methods doesn't check the type of the argument passed in:

Meteor.methods({
  '/user/save': function(user) {
    if (user.validate()) {
      user.save();
      return user;
    }
  }
};

In such case, I feel the client could easily trick the system by defining the custom user object. Of course this is just a toy example that is not full-fledged, but I'm wondering if there is a way to ensure that the user object passed in is in desired format.

I'm now thinking of check the argument with user instanceof User. Is that sufficient?

Thanks.

[minfawang]

It looks like the instanceof check is insufficient.

I just did an experiment:

Foo = Astro.Class({
  name: 'foo'
};

var foo = new Foo();
foo instanceof Foo; // true
foo.validate = function () { console.log('Some bad things happen'); }
foo instanceof Foo; // true (but is dangerous)

So how can we ensure the data integrity? One solution I can think of is to only pass in the raw object, and initiate the object in the method. However, this usually causes redundant object destruction and construction. Is there a best practice for handling this issue?

[lukejagodzinski]

You have to think how would you do it without Astronomy. You would pass plain JavaScript object and check if all the values are valid, you would probably pick only certain values etc. Astronomy doesn't do it for you. Right now I'm working for Useful.io and I've noticed that teammates are using Astronomy methods for that checks what seems to be a very nice idea.

Post = Astro.Class({
  /* ... */
  methods: {
    canSave: function() {
      var user = Meteor.user();
      return user.isAdmin;
    }
  }
});

Meteor.methods({
  '/post/save': function(post) {
    if (!post.canSave) {
      throw new Meteor.Error(403, 'Not allowed!');
    }
    if (post.validate()) {
      post.save();
      return post;
    }
  }
};

If it goes about further checks. You can just check if a passed document is an instance of a given class and it should be enough.

[minfawang]

@jagi Thanks for the prompt response!

However, I think the example I wrote above shows exactly why your definition of Meteor methods is still insecure. Now on the client, if the user just enter:

var post = new Post();
post.canSave = function () { return true; // or even execute any arbitrary commands }
post.validate = function () { return true; // or even execute any arbitrary commands }
post.save = function () { // Do whatever }

Meteor.call('/post/save', post);

Then doesn't it make the system so fragile? I noticed this thread on Meteor Forum. I think my question is similar to the second and third question of the author, but I didn't see a very relevant response on that question.

And what I think a solution (although might not be perfect) is to still pass in an plain object to the meteor method, and initialize the Astro instance inside the method and do all the necessary validation steps. The drawback of this method I can think of is redundant deconstruction and reconstruction of Astro instances sometimes.

Meteor.methods({
  '/post/save': function(postRaw) {

    // Notice I'm passing the raw object and initialize the instance inside the method body
    // On the server, the definition of Post cannot be altered.
    var post = new Post(postRaw);

    if (!post.canSave) {
      throw new Meteor.Error(403, 'Not allowed!');
    }
    if (post.validate()) {
      post.save();
      return post;
    }
  }
};

What do you think?

[lukejagodzinski]

It's not possible to override the method on the client. Even if a user does it the server version of this document will still have that method. Why? To allow passing a document over the DDP protocol you have to register Astronomy class as a new type. How exactly does it work? It's deconstructing Astronomy document to plain values sending over the wire and constructing it again on the server. It's not possible to send methods (functions) over DDP. You can only send plain data. So when you think that you are sending Astronomy document... no you are not sending the whole document. It's not possible.

So this method is safe.

[minfawang]

@jagi Thank you for the explanation! Now it makes sense.

I'm going to close this issue.

[lukejagodzinski]

No problem :). I think that I will create special security section in the documentation for Astronomy 2.0. It's very important to make apps secure so it would dispel any doubts.

[minfawang]

Yay it will definitely help a lot of people! I'm excited to see v2.0 :+1: