Error thrown inside an async rule are not sent to the fallbackError function

[Sytten]

Describe the bug

If an async rule throws inside a chain, the error is not sent to the fallbackError function. The value is null.

Example

const editorIsEventOwner= chain(
  isPromoterStaff,
  rule({ cache: PermissionCache.STRICT })(
    async (_parent: {}, args: { id: string }, ctx: Context) => {
      const { id } = args;
      const event = await findEvent(toInt(id), ctx); // This can throw an Error
      if (!event) return false;
      return event.ownerId === ctx.user?.id;
    }
  )
);

This will correctly not allow the user to proceed if the findEvent fails, but it will not put the Error in the first parameter of the the function.

[open-collective-bot[bot]]

Hey @Sytten :wave:,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/graphql-shield

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing :smile:

[Sytten]

I created a work around in the mean time:

type AsyncIRuleFunction = (
  parent: any,
  args: any,
  ctx: any,
  info: any
) => Promise<IRuleResult>;

export const safe = (func: AsyncIRuleFunction): IRuleFunction => {
  return (parent: any, args: any, ctx: any, info: any) => {
    return func(parent, args, ctx, info).catch(
      (_) => new InternalServerError() // This inherits from ApolloError
    );
  };
};

[maticzav]

Could you create a reproduction CodeSandbox?

[Sytten]

@marktani here it is: https://codesandbox.io/s/example-graphl-shield-throw-x5g0h?file=/index.js

[Sytten]

Ok tracked down the issue. Basically here https://github.com/maticzav/graphql-shield/blob/master/src/rules.ts#L74 we return false for any error thrown so there is no way for the generator calling it to have this error. I will send a proposal that is not a breaking change (was hard to find, but I got one).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[zhouzi]

The workaround for us is to set debug to true in development. This way we can find out errors that are thrown instead of returned.

https://codesandbox.io/s/example-graphl-shield-throw-mfgle?file=/index.js

[Sytten]

I created a wrapper for the rule function. I would be willing to rewrite part of the lib to support more native feature if @maticzav let me. I basically did it with nexus-shield. I think the shift to throw would be better.

[maticzav]

That would be awesome @Sytten

[Sytten]

@maticzav I currently do not have a lot of time, but I would be happy to hop on a call so we can design the API and prepare for this v2 interface. Ping me on the prisma slack maybe?

[guiaramos]

Any update on this issue?

[kamikazePT]

options.fallbackError is never executed with the thrown error, there isn't a code flow that reaches that statement.

if debug = false && allowExternalRules = false, rule returns false and fallbackError is called with null

if debug = true, rule re-throws and then generator re-throws again, never calling fallbackError with the error.

we need a different flag to support fallbackError with error as an argument, which is a use case I was in need for logging purposes while having custom errors in shield configuration

in rules.ts

async resolve(
    parent: object,
    args: object,
    ctx: IShieldContext,
    info: GraphQLResolveInfo,
    options: IOptions,
  ): Promise<IRuleResult> {
    try {
      /* Resolve */
      const res = await this.executeRule(parent, args, ctx, info, options)

      if (res instanceof Error) {
        return res
      } else if (typeof res === 'string') {
        return new Error(res)
      } else if (res === true) {
        return true
      } else {
        return false
      }
    } catch (err) {
      if (options.debug) {
        throw err
      } else {
        return false
      }
    }
  }

in generator.ts

// Execution
    try {
      const res = await rule.resolve(parent, args, ctx, info, options)

      if (res === true) {
        return await resolve(parent, args, ctx, info)
      } else if (res === false) {
        if (typeof options.fallbackError === 'function') {
          return await options.fallbackError(null, parent, args, ctx, info)
        }
        return options.fallbackError
      } else {
        return res
      }
    } catch (err) {
      if (options.debug) {
        throw err
      } else if (options.allowExternalErrors) {
        return err
      } else {
        if (typeof options.fallbackError === 'function') {
          return await options.fallbackError(err, parent, args, ctx, info)
        }
        return options.fallbackError
      }
    }

[kamikazePT]

in generator.ts, the following else if is never reached as well in any case

else if (options.allowExternalErrors) {
        return err

[Sytten]

Note that would require a major version IMO since I rely on the current behaviour, see my comment above https://github.com/maticzav/graphql-shield/issues/739#issuecomment-616843767

[kamikazePT]

@Sytten not entirely sure we would need a major version, it can be an addition instead of a rewrite

to reach the last 2 elses on the generator catch, we can just add a new flag, that throws on rules.ts, but skips the if(debug) on the generator.ts

this could be fixed in 2 phases

• 1 minor version adding a new flag
• 1 major version in the future rewriting this logic and re-thinking the flags

[DevLugo]

Hello guys. Any workaround example?

I was trying with this example but I was unable to get it. https://github.com/dimatill/graphql-shield/issues/739#issuecomment-616843767