relay-rails-blog icon indicating copy to clipboard operation
relay-rails-blog copied to clipboard

Doesn't this allow someone to delete anything?

Open nuclearspike opened this issue 7 years ago • 5 comments

There's a pretty serious hole in site security with this code.

Here: https://github.com/gauravtiwari/relay-rails-blog/blob/master/app/api/mutations/comment_mutations.rb#L52

comment = RelaySchema.object_from_id(inputs[:id], ctx)
comment.destroy

I could pass in the global id for an admin's User record and it would load then destroy it. There are no protections around whether it actually loaded a Comment or whether the User has permissions to delete that Comment. I can destroy any object in the DB with that code.

nuclearspike avatar Mar 03 '17 19:03 nuclearspike

Hey Paul, Thanks for pointing out. You know, I intended to do another series of post on authorisation, but never got chance (apart from keeping the project up-to-date). Will fix this and let you know.

gauravtiwari avatar Mar 06 '17 16:03 gauravtiwari

@nuclearspike - made some small updates. I might do this differently, but for now this is good enough - https://github.com/gauravtiwari/relay-rails-blog/blob/master/app/api/relay_schema.rb#L35

https://github.com/gauravtiwari/relay-rails-blog/blob/master/app/api/mutations/comment_mutations.rb#L50

What do you think?

gauravtiwari avatar Mar 06 '17 18:03 gauravtiwari

Yeah, that looks like it would protect the objects from malicious deletion.

What I've done in my own project is have a function that wraps all 3 actions, it loads the object by id, validates its type and checks the permission of what you're about to do. It throws appropriate errors. Use is something like:

comment = GraphLoader.load(args[:commentId], Comment, ctx, :destroy)

You could make a "load_and_authorize" method.

nuclearspike avatar Mar 06 '17 19:03 nuclearspike

@nuclearspike Yeah, that sounds like a good idea. However, have you checked graphql-ruby pro version, it provides authorisation and some other stuff if you pay for it. So, I was thinking perhaps I can patch the basic version of the Gem to include a authorise and validate method just like pro version so, we can do load_and_authorize on fields and even on schemas

https://github.com/rmosolgo/graphql-ruby/tree/master/guides/pro

gauravtiwari avatar Mar 07 '17 05:03 gauravtiwari

@gauravtiwari I like the graphql-ruby project, I have not looked at the pro version before. However, I still feel like I'm doing way too much wiring between GraphQL and ActiveRecord, stuff that should be way more automatic. I've written my own class that helps me a little where you just feed it a model class and an array of attributes to expose and it looks up their types for you. It's rather basic though.

I recently came across GraphAPI. Which is much more the direction I'd like to see people moving... however, it doesn't seem to have a ton of recent activity and it doesn't support cancan or similar yet. But it offers automatically generated mutations and queries of your models with very little effort, but also allows you to extend what it's doing.

nuclearspike avatar Mar 07 '17 18:03 nuclearspike