Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Falsey in Collection#deny and Collection#allow #5442

Merged

Conversation

martijndeh
Copy link
Contributor

This minor change will help prevent potential security issues.

Currently, the following snippet gets silently ignored:

Articles.allow({
  insert: false
});

Instead, the above snippet should throw an error.

The code currently does check truthy values, but doesn't check falsey values. Instead, it should check all defined values. This pull request fixes the check for truthy values with defined values.

I decided to use Object#hasOwnProperty instead of e.g. options[name] !== undefined because in some situation undefined might also be passed to one of the keys.

This issue came to light after a team member wrote the following piece of code:

var isAdmin = function() {
    return Roles.userIsInRole(Meteor.userId(), ['admin']);
};

Articles.deny({
    insert: !isAdmin
});

Luckily this was flagged during a code review. But to my surprise, Meteor didn't throw any errors. After investigating, I found the truthy check to be the issue.

I hope to get this request merged!

@stubailo
Copy link
Contributor

Looks awesome! Wow, that code snippet is the best - it's one of those things that kind of makes sense semantically but doesn't do the right thing at all. Thanks for the improvement!

stubailo pushed a commit that referenced this pull request Oct 20, 2015
…ow-deny

Falsey in Collection#deny and Collection#allow
@stubailo stubailo merged commit 53cc021 into meteor:devel Oct 20, 2015
sneakertack added a commit to sneakertack/Telescope that referenced this pull request Oct 27, 2015
Fixes the following error on startup while using Meteor v1.2.1.

```sh
Error: deny: Value for `update` must be a function
    at packages/mongo/collection.js:755:1
    at Array.forEach (packages/es5-shim/.npm/package/node_modules/es5-shim/es5-shim.js:417:1)
    at Function._.each._.forEach (packages/underscore/underscore.js:105:1)
    at [object Object].addValidator (packages/mongo/collection.js:752:1)
    at [object Object].Mongo.Collection.deny (packages/mongo/collection.js:804:1)
    at Posts.getNotificationProperties.properties.postAuthorName (lib/herald.js:7:21)
    at /home/sneakertack/work/gh/gh-purple/.meteor/local/build/programs/server/boot.js:249:
```

Prior to meteor/meteor#5442 this error in
the Telescope codebase wasn't being properly caught.
@x5engine
Copy link

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants