Take our Allow & Deny Security Challenge

announce
1

Allow & Deny vs Methods is one of the big debates within the Meteor community. Personally I tend to fall more on the side of Methods, simply because Allow & Deny is very hard to get right.

To illustrate this, I decided to hold a simple coding challenge:

https://www.discovermeteor.com/blog/allow-deny-security-challenge/

Fork the pad, post links to your answers here, and I’ll try to go through them to see if I can find any security holes ; )

2 Likes
2

Alright. I’ve updated my answer again. This should really be NSA proof!

Messages.allow 
  update: (userId, doc, fields, modifier) ->
    true
    # change this!
Messages.deny
  update: (userId, doc, fields, modifier) ->
    return true if !userId or 'createdAt' in fields or 'userId' in fields
    return true if modifier.$addToSet?.likes and doc.likes.indexOf(userId) != -1
    if 'likesCount' in fields
      return true if !modifier.$addToSet?.likes
      return true if !modifier.$inc?.likesCount
      return true if modifier.$inc.likesCount != 1
    return true if userId != doc.userId and 'body' in fields
    return false
3

I have a more “refined” opinion on this topic now, I wonder what you think:

  • Allow/Deny is fine for insert, remove, and a hypothetical read; there’s only one concern that you might want computed properties on create, but collection hooks take care of that.
  • It is not fine for update

Thoughts?

4

Can you create a MeteorPad with your answer?

5

It can be tricky for insert as well, depending on what you want to do. Unless you combine it with collection hooks, that can work too.

Another rule I came up with: allow/deny is ok when you’re updating your own documents, but not a good idea when you want to give users permission to change something on other user’s documents.

6

Sure can. Here:
http://meteorpad.com/pad/krqwC2tFfcb2R2xdg/Chatroom%20Security%20Challenge

7

You still need to be careful about, for example, a user reassigning their document to some other user by modifying the user ID foreign key.

8

Sorry, not secure :frowning: I was able to set likesCount to any value.

9

Can you try again with the same meteorpad?

10

Did we kill meteorpad? Takes forever for the server instance to load up and then once it does, it doesn’t update when I save my changes…

1 Like
11

My submission (also posted in comments on discover meteor post): http://meteorpad.com/pad/NEv9y3WsMgugHmBwg/copleykj’s%20Chatroom%20Security%20Challenge

I modified this a bit to use the socialize:likeable package which I authored but it still does the same basic thing and I believe satisfies all of the requirements.

12

Here’s my submission.

http://meteorpad.com/pad/uSrCGEWX87YiRBtax/Copy%20of%20Chatroom%20Security%20Challenge

13

@khamoud Sorry, not secure: Messages.update(Messages.findOne()._id, {$set: {body: "foo", likesCount: 9999}})

1 Like
14

Man. I can’t believe I missed that. It’s pretty obvious now that I think about my implementation (on mobile can’t see it). I’m partial to meteor methods for stuff like this for that exact reason.

1 Like
15

Results: https://www.discovermeteor.com/blog/allow-deny-challenge-results/

5 Likes
16

This is probably my favorite post ever written about Meteor. Awesome.

2 Likes
17

That means a lot! Glad you enjoyed it :slight_smile:

1 Like