Security Feature

[chibipirate]

Any updates on this?

[Lepozepo]

Not yet but soon I hope, I'll try to get something before the hackathon for everyone to enjoy

[a-mummey]

For the time being, is it possible to just completely disable delete from the client? I was able to pop open console and do Cloudinary.delete('my_image')

and poof it was gone, seems a bit too easy for a person to mess with my media library. My strategy was to do this: delete Meteor.default_server.method_handlers['c.delete_by_public_id'];

but I feel a determined attacker could still find a way to pop open the console and start deleting images. Any thoughts on this?

[Lepozepo]

I see 2 paths you could take. You can overwrite and rewrite the delete function on your app and hard code a password/auth-function for the delete function to pass or you can create a auth function for this package and create a merge request. ^_^

[chibipirate]

@Lepozepo Is there a specific way to override a package method? I've looked around and came up empty. Whenever I try to do it I get this error:

Error: A method named 'c.delete_by_public_id' is already defined

[Lepozepo]

There might be a way to overwrite meteor methods but I'm not sure if that's something you should do. What are you trying to accomplish?

[chibipirate]

@Lepozepo I was just working off of your previous reply: 'You can overwrite and rewrite the delete function on your app'. I was going to do a role check or something similar to the following before the deletion happens: if (! Meteor.userId()) { throw new Meteor.Error("not-authorized"); }

[chibipirate]

@Lepozepo Using: Meteor.default_server.method_handlers["c.delete_by_public_id"] seems to bypass the error. I'm going to test it out.

[Lepozepo]

Ah, I see what you mean, lol, sorry. Damn, I guess with the current implementation it is a pain to add security to the delete function. Let me see if I can patch it quickly right now, it's going to break everyone's delete though >_<.

[Lepozepo]

I have an idea for a dirty fix while I get time to make a better implementation that won't break people.

[chibipirate]

@Lepozepo Good news. Overriding it works like that. Here's is how I did it in my methods.js file:

var Future; Future = Npm.require("fibers/future"); Meteor.default_server.method_handlers["c.delete_by_public_id"] = function(public_id) {

if (!this.userId && !Roles.userIsInRole(this.userId, ['admin'])) {
  throw new Meteor.Error(403, "Access denied");
}

var future;
this.unblock();
if (check) {
  check(public_id, String);
}
future = new Future();
Cloudinary.api.delete_resources([public_id], function(result) {
  return future["return"](result);
});
return future.wait();

}

[Lepozepo]

Sweet! I'm going to push an update in a minute though so you won't have to do that ^_^

[Lepozepo]

    "c.delete_by_public_id": (public_id,auth_function) ->
        @unblock()
        if auth_function and not auth_function(public_id)
            throw new Meteor.Error "Unauthorized","Delete not allowed"

        if check
            check public_id, String

        future = new Future()

        Cloudinary.api.delete_resources [public_id], (result) ->
            future.return result

        return future.wait()

[chibipirate]

@Lepozepo Sounds good. I'll update once it's up. Thanks for the fast responses! :) +1

[chibipirate]

@Lepozepo Could you add some details about "auth_function"?

[Lepozepo]

Nvm, that doesn't work. There would have to be a function that builds the delete function together with the auth function for it to work :/.

[Lepozepo]

Got it, but it only works with a global authorization function for now.

    "c.delete_by_public_id": (public_id) ->
        @unblock()
        if Cloudinary.rules.delete
            auth_function = _.bind Cloudinary.rules.delete,this
            if not auth_function()
                throw new Meteor.Error "Unauthorized", "Delete not allowed"

        if check
            check public_id, String

        future = new Future()

        Cloudinary.api.delete_resources [public_id], (result) ->
            future.return result

        return future.wait()

[Lepozepo]

So what you'll do is set an authorization function on the server side under Cloudinary.rules.delete and tap into the Meteor method through there. Does that make sense?

[Lepozepo]

Fixed with 502c83a2bdc50024ba7b25e1268f2c00bce7910a

[chibipirate]

@Lepozepo Makes sense. I'll give it a go. Thanks again!

[Lepozepo]

No problem ^_^ I had a bit of time on my hands tonight :D.

On October 5, 2015 at 9:56:51 PM, ats (notifications@github.com) wrote:

@Lepozepo Makes sense. I'll give it a go. Thanks again!

— Reply to this email directly or view it on GitHub.

[danielparas]

@Lepozepo, thanks for this plugin! - really helpful :)

Is there a way I can pass data to the auth_function ? As it doesn't take any arguments. Reason I need to pass data is because I need to perform more checks other than just confirming that if a user is logged on. Eg. if the user owns the image that is going to be deleted or if they are deleting someone else's image for example.

Thanks Dan

[Lepozepo]

Mmm, I could append the information passed to the method to the context of the authorization function. This would definitely make things easier. I'll do some open source in a couple of minutes and see if I can tackle this quickly right now.

[danielparas]

Thanks @Lepozepo !

[danielparas]

Hey @Lepozepo, any luck ?

[fabyeah]

I want to allow users to only delete their own images (that they uploaded). Is that possible? How do I do that?

[fabyeah]

Nevermind, I forgot I'm already using methods...

[fabyeah]

Actually forget that, please tell me :)

[danielparas]

I didn't manage to find a way with the current setup as there's no way to link the cloudinary signature with a specific file. As it stands the only check you can perform is if a user is logged in and if that user has permission to delete in general. but this means they can delete any file i.e. its either all or nothing so to speak, nothing in between.

[fabyeah]

@danielparas If I'm not mistaken, it's possible to set the filename when uploading a file. So you could prefix the filename with the user's id. It's not possible to check when deleting, if a user's id matches the filename prefix?

[danielparas]

@fabyeah - did not manage to find a way to perform this check. If you find a way let me know :) thats is why I requested a small adjustment to the rules functions

[fabyeah]

Can you maybe just not make the delete method available on the client? I understand that upload should be available from client to save server resources, but deleting doesn't really have any impact on performance, does it? As the cloudinary config is also only server side, a client wouldn't be able to make just any delete requests anymore, because he doesn't have the API Key and API Secret?

[Lepozepo]

Hey guys! Damn, sorry I never mentioned this @danielparas but I did make the public_id available on the delete rule function. You should be able to use this to properly check for ownership now (it's been up for a while too, lol). @fabyeah the delete function is really just a convenience function but you have to make sure to protect it with a rule. If you are deleting from the server only without a client facing interface I can see how a client side function isn't necessary. Does this answer your question?