Graphql-shield: Consider allowing permissions to be set on a per-field basis

[artemzakharov]

Although I'm an avid user of GraphQL Shield - thank you for creating such a simple means of implementing permissions! - something that has always irked me is that when you implement permissions on a by-type basis, you effectively lose the _relational_ power of GraphQL. Perhaps I'm wrong on this, but this is the issue I see, illustrated with an example:

1. Developer of a social & eCommerce website implements a query to allow fetching user details for a user profile page , something like this:

{
  user (input: UserWhereUniqueInput) {
    name
    age
  }
}

1. Developer sets a basic isAuthenticated permission on the query to ensure that only logged in users can run this query, but adds no permissions beyond that

2. A malicious user pokes around the publicly available schema, and finds out that the website stores payment information as CreditCard nodes that have one-to-one relations with User nodes

3. Malicious user simply edits the query to the following, and can now steal people's credit cards:

{
  user (input: UserWhereUniqueInput) {
    name
    age
    creditCard {
      number
      securityCode
    }
  }
}

I see two solutions to this:

1. Forcibly set the info of the query to return a pre-determined result
2. Create a permissions system that parses info, compares it to one of several templates based on the user's permissions level, and either throws an error or lets the query through

Since the first solution effectively turns GraphQL into a regular REST endpoint, thereby defeating the purpose, I ended up rolling my own solution that implements #2 using graphql-fields. It even allows for functions to be passed in that can evaluate the input at runtime, and determine whether to authorize the operation or not based on the actual query input. Here's an example with a mutation:

export default {
  Mutation: {
    createCollectionAsPartner: rule()(
      async (parent, { collectionCreateInput }, ctx: Context, info) => {

        const permissionFunction: PermissionFunction = (): boolean => {
          return collectionCreateInput.collectionVersions.create.deleted !== false;
        };

        const permissionDefinition: PermissionDefinition = {
          collectionVersions: {
            create: {
              name: true,
              deleted: permissionFunction,
              externalFiles: {
                create: {
                  applicationType: true,
                },
              },
            },
          },
        };

        const verified = verifyMutationPermissions(
          collectionCreateInput,
          permissionDefinition,
          'createCollectionAsPartner'
        );

        return verified;
      }
    ),
  },
};

Using this definition, this mutation would work, because name is defined as true in the PermissionDefinition object:

mutation {
  createCollectionAsPartner(collectionCreateInput: {collectionVersions: {create: { name: "12345"}}})
}

While this one would not, because approvedManually is not defined in the PermissionDefinition object:

mutation {
  createCollectionAsPartner(collectionCreateInput: {collectionVersions: {create: { approvedManually:true}}})
}

This example is based on typings generated by Prisma, and limits what kind of input can be passed in when creating this type to the ones defined and equal to true in the PermissionDefinition object. If a field is not defined in the object, or is defined but as either false or a sync/async function that resolves to false at runtime, the operation is denied. The end result is extremely granular control over what is and isn't allowed, without sacrificing the ability to query/mutate by relations, the greatest strength in GraphQL.

Although I wouldn't necessarily expect this to make it into the native library, I'd love to hear everyone's thoughts on this approach! I was surprised to see how little attention this critical security issue is getting within the GraphQL community, but it's possible that I'm just doing something totally wrong and this isn't a problem with a correct setup.

[maticzav]

Hey @artemzakharov 👋!

First, thank you for such a warm feedback. It's one of the things that drive me when making open source projects.

I think your question is very relevant. Therefore I think we should consider posting it in Prisma Forum as well!

It would be beneficial if you could provide your schema as well, as we can better understand your issue with such context.

Could you provide that? 🙂

[artemzakharov]

Hi @maticzav,

Unfortunately I can't provide my full schema since it's (ironically) a proprietary project that will have to deal with issues just like the one above, and I don't want to compromise security, but I would be happy to post snippets to clarify any questions about why I took the approach that I did.

I also can't exactly post the full schema even if I wanted to, because I don't _have_ one per se... it's broken down by query/mutation into individual files, and assembled at runtime for a more modular development experience 🙂

My organizational structure looks like this:

And the GraphQL schema for this particular mutation looks like this:

type Mutation {
  createCollectionAsPartner(
    collectionCreateInput: CollectionCreateInput!
  ): Boolean
}

As you can see, I'm a big fan of leveraging types generated by Prisma - like CollectionCreateInput - over fixed inputs - like (name: string, userId: string, etc...) - which is probably where the concern to regulate what the shape of these inputs can be came from.

I wouldn't mind crossposting this to the official Prisma forum, but I'm not sure if I would have the time to oversee it. I'll consider doing it later this week, I just wanted to post it here first since it's a more thematic place to do so, but I'm glad (or not? 😅 ) to hear that this is in fact a real issue.

[maticzav]

@artemzakharov I see! I am currently on a short vacation and will be posting an in detail answer to your question in a couple of days.

Hope it's not urgent! 🙂

[maticzav]

Hey @artemzakharov 👋, I finally have some time to reply to your question! Here's the idea;

GraphQL works recursively. I suppose you are using Prisma as a database which often abstracts the query process to the point where it seems like responses are not processed on your server thoroughly. As you might have guessed from this, they still are - all of them!

If you have some spare time, I highly recommend reading this article because it very well explains the idea behind GraphQL execution.

Back to the point. Let's assume that your data model looks similar to this;

type User {
  name: String!
  age: Int!
  creditCard: CreditCard!
}

type CreditCard {
  number: String!
  securityCode: String!
}

and that our schema is similar to the following one;

type Query {
  user(id: ID!): User
}

Now, when you query the user, you first ask Prisma for a user with such and such id. All good! We also provide the info object with all the fields we want to access, and Prisma recursively obtains them for us. Up to this point, it seems like we have no direct control over the return values. The following part is the crucial one.

GraphQL (GraphQL Yoga + GraphQL Tools, in your case) define resolvers for every single field in our schema, even if we haven't explicitly told it to do so. Therefore, Query execution looks like this:

1. We want to access Query.user field. GraphQL executes the resolver which returns a nested object obtained from Prisma.

2. The result is forwarded to User type resolver, which has been auto-generated by graphql-yoga.

3. Each field that we have forwarded is also resolved by resolvers in User type and forwarded on until we reach final - scalar level. __NOTE__: creditCard portion of User is forwarded to CreditCard resolver type.

4. All of the values are composed together into a single response.

Finally, the solution! 🎉

You might have guessed where this was going before, otherwise, here's the idea. Instead of wrapping and modifying info object, we just apply rules to subfields of the nested types. In a case of the credit card, you could do something like this:

const permissions = shield({
  Query: {
    user: allow,
  },
  User: {
    name: allow,
    age: allow,
    creditCard: deny
   }
})

// or

const permissions = shield({
  Query: {
    user: allow,
  },
  CreditCard: {
    securityCode: deny
  }
})

Besides allow and deny, you could have used a more complex rule, such as isAdmin or something similar for example, and only partially limit the access.

---

I agree that relations are in a way still lost due to this approach. Nevertheless, such approach prevents many edge cases and reduces the number of tests needed for a genuinely secure codebase.

I hope this solves your situation at least to a certain degree or sparks some ideas in your mind. In any case, let me know! 🙂

PS.: If you are using Prisma, I highly recommend you copy all the generated types to your schema and remove the fields that shouldn't be exposed to the client. I would say this is one of the most overlooked security issues when using Prisma. __Your server schema doesn't need to match the generated one, not even on the type level. GraphQL should figure all of this by itself out of the box!__

I would also like to stress, that in a well-structured project, graphql-shield should become entirely obsolete. All of its functionality can be achieved using schema manipulation. Nevertheless, the usual approach is far from ideal and rather hasty. That's also the reason why I created graphql-shield - to ease the creation of permission layer.

[artemzakharov]

Thanks for the highly detailed answer, @maticzav!

I wanted to clarify a couple of things:

1. My model doesn't actually look like that, that was more of an example 😄
2. I'm not actually modifying the info object in any way - I'm only interpreting it to see what information is being requested, and then comparing to a predefined list of allowed fields

Looking at your approach, however, it almost seems like an inversion of my way; instead of defining a template for a query - something like, in written form, "If you are running a User query, and are an Admin, you can access fields x, y, and z" - you instead say "if your query is asking for field x, you must satisfy criteria a, b, and c". It's a very interesting comparison, and I'm not sure which is better, or more applicable in more situations. I think mine is definitely simpler to implement, but I can see some future issues with scaling if the number of queries and mutations grows substantially while the number of types remains relatively fixed.

Definitely something to think about some more 😄

[colinhacks]

@maticzav Thanks for the great explanation. I was with you right up until "relations are in a way still lost due to this approach". I'm not sure I understand how/why this is the case.

If I'm using Prisma and on my "application server" I implement resolvers for every field of every schema type (most of which just forward to the Prisma Docker container), relations remain intact, no?

Or are you just saying that a permissions scheme that blocks access to creditCard essentially breaks the client-server contract defined by your schema?

[maticzav]

Hey @colinmcd94, 👋

Sorry for the confusion. What I had in mind with "losing relations" is that there's no "direct" connection between values. Every field gets resolved on its own in a way.

A follow up to your second point; GraphQL always finds a way to stay true to your schema. Therefore, I wouldn't say permission could break the contract. It just changes the result.

Is that what you had in mind? 🙂

[nolandg]

@maticzav Been reading a bunch of issues lately, thanks for your awesome clarity! you're good at explaining graphql.