Graphql-shield: Errors thrown in resolver result in "Insufficient Permissions" error

[delantai]

First of all, just wanted to add that I love the simplicity of this module, it's certainly made setting up auth in my latest graphql project simpler. Quick question:

I noticed that any errors raised in within my resolvers result in a PermissionError -- is this the intended behavior?

I'm not currently doing anything to handle the errors, so perhaps I should be doing something to deal with them more gracefully; but regardless, I was expecting to receive/log/crash with the original error.

[ghost]

Well, after looking into the source code I've found this https://github.com/maticzav/graphql-shield/blob/30575cf3d5f1cc0c3de6c077c9574bbcf314e683/src/index.ts#L142

Seems like in order to not have Insufficient Permissions message where you don't want it to be you need to do something like this:

import { PermissionError } from 'graphql-shield';

// some code ...
throw new PermissionError('your message')
// some code ...

The bad thing (to me) is that now you need to replace throw new Error everywhere in resolvers((

I hope that thing will be fixed in the future as current approach does not seem great at all

[iangregsondev]

Yes, this is a problem for me too, as I am trying to use graphql-errors which is a library that allows me to throw errors and it intercepts them and only exposes what i need.

Problem is that graphql-shield intercepts all errors, so I get this.

When it actually wasn't a permission error.

{
  "data": {
    "user": null
  },
  "errors": [
    {
      "message": "Insufficient Permissions.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "user"
      ]
    }
  ]
}

[iangregsondev]

Hi @maticzav ,

I thought I would give a little more context, so when I throw an error, the graphql-errors library (https://github.com/thebigredgeek/apollo-errors) picks it up and outputs the client

Let me explain, I basically create a new type of error - let's call it FooError like so

const FooError = createError('FooError', {
  message: 'A foo error has occurred'
});

and then inside my resolver I can actually throw it like so, whats important to know is that the data is returned to the client but the internals are actually just being logged into the console of server. Thus protecting errors for leaking to the client.

      throw new FooError({
        data: {
          something: 'important'
        },
        internalData: {
          error: `The SQL server died.`
        }
      });

Here is an example what is displayed in the client

{
  "data": {
    "user": null
  },
  "errors": [
    {
      "message": "A foo error has occurred WOW OW ",
      "name": "FooError",
      "time_thrown": "2018-05-21T07:57:54.796Z",
      "data": {
        "something": "important"
      }
    }
  ]
}

So the SQL error was never leaked, but it was displayed in the server logs.

Now, when graphql shield is enabled, it seems to rethrow a permissions error, I was wondering if it's not a permissions error can we just rethrow the original error and graphql-error will take care of the rest.

Here is what is displayed when graphql shield is enabled for the same error above (which a permissions error wasn't thrown)

{
  "data": {
    "user": null
  },
  "errors": [
    {
      "message": "Insufficient Permissions.",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "user"
      ]
    }
  ]
}

[maticzav]

@appsolutegeek thanks for the detailed report! I'll include it in the new version coming very soon and make this part of the evaluation more flexible. You should be then able to customise error handler to your needs!

[iangregsondev]

Thanks. I am loving the library, by the way.

Excellent. It makes life so much easier :-)

[maticzav]

Hey @almostprogrammer, I wanted to gain some more input on this issue. Let me explain you my point of view. (Everyone is welcome to contribute, I only addressed @almostprogrammer because he gave the most opposing opinion and I want to know more.)

The bad thing (to me) is that now you need to replace throw new Error everywhere in resolvers

The idea behind Shield is that it prevents any data leaks of any sort to the client. Atop of ensuring that no data is leaked if permissions don't pass, I also wanted to prevent any unpredicted Error leaks which could expose the server architecture. Because of this, we catch all the errors. Only the ones which are explicitly said that can be leaked (custom error messages) are leaked to the client.

I hope this clears out the idea why we catch all the errors, not just the ones thrown by permission execution. Could you maybe explain how or why would you tackle this problem?

[iangregsondev]

I was using graphql-errors https://github.com/thebigredgeek/apollo-errors which i suppose kinds of duplicates what you are doing.

I was only using graphql-errros as I thought it supported exactly what I needed.

If graphql-shield can support this - then great.

Although my personal opinion is that if an error is thrown that is NON permission related, it should be left to bubble up and we can use other tools to support that..

Maybe we can have an OPT IN / OUT of this feature so that people can opt out to errors being thrown and graphql would just ignore them / rethrow them without changing anything.

And also people can opt in to say that any uncaught errors inside of a permission would automatically get thrown as a permission error.

What I do like about graphql-errors is that you can create an error to choose what you want to be exposed to the client and what is emitted on the server.

  throw new FooError({
    data: {
      something: 'important'
    },
    internalData: {
      error: `The SQL server died.`
    }
  });

If there is an error, maybe i want to inform the client that something bad has gone wrong but i don't want to leak specifics.

Just my 2 cents.

I personally would like the library could be easily used with other libraries so have an OPT OUT would be really beneficial to me.

Thanks again!

[tms1337]

@maticzav This seems not to be fixed yet, is there any parameter I can send to opt out of wrapping in-resolver error in AuthenticationError ?

Edit: my bad I found a way but was not so clear right away so writing down a solution for others with the similar problem. The snippet below just returns the original error.

const permissions = shield(
  {
    Query: {...},
    Mutation: {...},
  },
  {
    fallbackError: (err) => err,
  }
);

[maticzav]

@tms1337 have you tried returning an error? That's the way of telling shield that everything is under control, but you still want to show the error.

[tms1337]

@tms1337 have you tried returning an error? That's the way of telling shield that everything is under control, but you still want to show the error.

This works, but I have a specific usecase where I do not want to return an error.