graphql-prisma-typescript icon indicating copy to clipboard operation
graphql-prisma-typescript copied to clipboard

Examples leak data - i can see what other users booked - or is this on purpose?

Open nickluger opened this issue 7 years ago • 11 comments

For example, if i have a booking, i can see who else booked the same place and when. I can fetch their email, personal data etc.

{
  viewer {
    me {
      lastName
    }
    bookings {
      place {
        name
        bookings {
        	bookee {
            firstName
            lastName
            email
          }
        }
      }
    }
  }
}

Response

{
  "data": {
    "viewer": {
      "me": {
        "lastName": "Smith"
      },
      "bookings": [
        {
          "place": {
            "name": "Mushroom Dome Cabin: #1 on airbnb in the world",
            "bookings": [
              {
                "bookee": {
                  "firstName": "Karl",
                  "lastName": "Marx",
                  "email": "[email protected]"
                }
              },
              {
                "bookee": {
                  "firstName": "Adam",
                  "lastName": "Smith",
                  "email": "[email protected]"
                }
              }
            ]
          }
        }
      ]
    }
  }
}

Hmm interesting, Karl will also be at Mushroom Dome!

This is a problem i ran into, not only in this server example, but also in my own and other peoples server code. As soon as we have cyclic model references, hiding data becomes very complex.

nickluger avatar Feb 22 '18 18:02 nickluger

Hi @nickluger. Thanks for bringing this up. You're right, the permission setup is not yet fully implemented.

Would be great if someone would like to take a stab at this - maybe based on graphql-shield? /cc @maticzav

schickling avatar Feb 23 '18 09:02 schickling

@schickling, I am on it!

maticzav avatar Feb 23 '18 14:02 maticzav

Hey @schickling, @maticzav,

Has there been any progress with this one? I've encountered a similar issue with a project I'm working on.

It'd be super useful for us GraphQLNoobs to see how to mask certain fields at arbitrary query depths given some permissions. In this example it would involve removing the "bookings" field from place or perhaps limiting the selection on the relation to specified public fields.

I'm using graphql-shield but don't see an obvious way to filter the incoming selection or verify it given the incoming user's permissions without hard coding the selection passed to prisma-binding. I imagine one possible solution might be to create a masking type but again not sure how to implement this.

Any help appreciated!

daveols avatar Jul 25 '18 10:07 daveols

Hey 👋,

I started playing with this a bit but came to a strange conclusion. GraphQL Shield serves as means to prevent unwanted requests. Data leaks don't happen because of bad queries but rather because of wrong data manipulation.

I think it could be beneficial to add GraphQL Shield to the example but hardly believe it will solve the data leaking problem.

maticzav avatar Aug 28 '18 22:08 maticzav

I found a solution to this, there are a couple of options. The first is more flexible, allowing you to get dynamic or filtered results for your resolvers. The second just returns null for the resolver when certain conditions are not met.

  1. Write a Place resolver which, for the bookings resolver, returns only bookings for the current user or all for the host:
export const Place = {
  bookings: {
    description: "The current user's bookings at this place, or all if the current user owns it",
    resolve: async ({ id }, args, ctx: Context, info) => {
      const place = await ctx.db.query.place({ where: { id } }, `{ host { id }, bookings { id } }`)
      if (place.host.id === ctx.user.id) {
        return place.bookings
      }
      return ctx.db.query.bookings({ where: { AND: [{ place: { id } }, { hostee: { id: ctx.user.id } }] } })
    }
  }
}

The resolver could probably be written better to improve performance but you get the idea.

  1. Use graphql-shield and write a rule which blocks access to the bookings key on the Place type if the current user is not an owner of that place:
const rules = {
  Query: {
    // ...
  },
  Mutation: {
    // ...
  },
  Place: {
    bookings: and(isAuthenticated, isPlaceOwner)
  },
}

I hope this helps someone!

daveols avatar Aug 29 '18 01:08 daveols

One can also introduce two different data models PublicBooking and Booking, where the first one hides certain fields, among those, bokee. So i can use PublicBookings to e.g. determine availability of a place and use Booking for example in my own profile or an admin view. Then you use graphql-shield to restrict access to those queries and mutation that use the Booking type.

We are using a similar approach currently with PrivateProfile and PublicProfile for our user data model, which both map to the same underlying User model but expose different fields.

You just need to take care, that, if you use Booking (or PrivateBooking) as a reference field type on another model, queries and mutations on this model also need to be protected by graphql-shield.

nickluger avatar Aug 29 '18 04:08 nickluger

I considered that approach too @nickluger. It is beneficial in terms of being a more accurate representation to the client via introspection. My thoughts on the potential downsides:

  • You duplicate schema definitions possibly multiple times for different access levels
  • You need to rewrite custom resolvers for the common fields between the types you are duplicating
  • You are limited to exposing just one of the access levels per query/mutation, instead of showing relevant data depending on the user's access level
  • It's harder to scale when implementing more advanced, granular privacy control

There is no 'right' approach here I think and it really depends on the use case and how you expect that resolver tree to behave throughout the schema lifespan.

daveols avatar Aug 29 '18 05:08 daveols

I strongly support the 2nd approach suggested by @daveols, using graphql-shield. The multi-model approach has another disadvantage, if you use Prisma - you cannot use fragments on the client any longer!

See here: https://github.com/graphql-binding/graphql-binding/issues/67

The 1st approach has the disadvantage of mingling resolvers with imperative authorization code, as described here https://www.prisma.io/blog/graphql-directive-permissions-authorization-made-easy-54c076b5368e/.

nickluger avatar Sep 15 '18 02:09 nickluger

Just a very blant remark, this does mean that you cannot currently implement prisma for a production application... or is there a concrete solution soon to be released. I ask this question since im evaluating this project vs other solutions like graphile.

voordev avatar Oct 06 '18 23:10 voordev

We're using the solution suggested by @daveols now, succesfully for a production app. You could try that, too.

nickluger avatar Oct 07 '18 05:10 nickluger

@voordev this by no means that prisma is not ready for a production application. Just means we need to add this to this implementation. We’ll add some shield soon

abhiaiyer91 avatar Oct 07 '18 07:10 abhiaiyer91