Throwing an error from a plugin result in `Unexpected response`

[j0k3r]

Describe the bug

When a plugin is throwing a GraphQLError, the result is Unexpected response.

I created a custom plugin which throw a GraphQLError in case of a problem (to make it short) and when that error happen, the server respond Unexpected response refering to an Internal Server Error.

I was thinking of a problem on my side then I tested using the useJWT plugin from Yoga (using the Hello world as base template) and I got the same problem.

I sent a request to the server using these headers (which obvisouly throw an error):

{
  "authorization": "oups"
}

The response:

{
  "errors": [
    {
      "message": "Unexpected response: \"GraphQLError: Unsupported token type provided: \\\"oups\\\"\\n    at createGraphQLError (/private/tmp/jwt/examples/hello-world/node_modules/@graphql-tools/utils/cjs/errors.js:9:12)\\n    at unauthorizedError (/private/tmp/jwt/examples/hello-world/node_modules/@graphql-yoga/plugin-jwt/cjs/index.js:54:50)\\n    at defaultGetToken (/private/tmp/jwt/examples/hello-world/node_modules/@graphql-yoga/plugin-jwt/cjs/index.js:97:15)\\n    at onRequest (/private/tmp/jwt/examples/hello-world/node_modules/@graphql-yoga/plugin-jwt/cjs/index.js:29:27)\\n    at /private/tmp/jwt/examples/hello-world/node_modules/@whatwg-node/server/cjs/createServerAdapter.js:67:125\\n    at iterate (/private/tmp/jwt/examples/hello-world/node_modules/@whatwg-node/server/cjs/utils.js:306:25)\\n    at iterate (/private/tmp/jwt/examples/hello-world/node_modules/@whatwg-node/server/cjs/utils.js:318:16)\\n    at /private/tmp/jwt/examples/hello-world/node_modules/@whatwg-node/server/cjs/utils.js:312:24\"",
      "extensions": {
        "requestBody": {
          "query": "{\n  hello\n}"
        },
        "responseDetails": {
          "status": 500,
          "statusText": "Internal Server Error"
        }
      }
    }
  ]
}

Your Example Website or App

https://codesandbox.io/p/sandbox/magical-nobel-5283sf

Steps to Reproduce the Bug or Issue

1. The app should run on codesandbox
2. Open GraphiQL
3. Select Header
4. Define that header:

   {
     "authorization": "oups"
   }

5. Run the default query
6. The app will crash

Expected behavior

The user should get a 401 response from the server as per the GraphQLError status code: https://github.com/dotansimha/graphql-yoga/blob/d143af175f2c816fdd094c9f76fde6c75a2f5872/packages/plugins/jwt/src/index.ts#L117

Screenshots or Videos

No response

Platform

• OS: macOS
• NodeJS: v18.16.0
• @graphql-yoga/*:

    "@graphql-yoga/plugin-jwt": "^1.0.1",
    "@graphql-yoga/render-graphiql": "4.0.4",
    "graphql": "16.6.0",
    "graphql-yoga": "4.0.4"

Additional context

No response

[EmrysMyrddin]

Hi, thank you for you detailed report !

The problem here probably comes both form your plugin implementation and a lack of documentation from our part.

Your plugin is probably using onRequest or onResponse hooks. Exceptions thrown from this hooks are not catch by Yoga automatically, meaning it will bubble up to node's http server, which crashes if any exception occurs during request handling.

You actually found a bug in the implementation of useJwt, which shouldn't rely on onRequest but on onRequestParse (since it is actually extracting data from the request).

To avoid errors not being catch automatically, you should avoid using onRequest and onResponse in favour of onRequestParse and onResultProcess. onRequest and onResponse are very low level hooks coming from the HTTP layer and knowing nothing about GraphQL, this is why it doesn't automatically handles error here, since it doesn't know what to do with it.

If you have any question or need help, you can provide more information about your plugin implementation.

[j0k3r]

You're right, my plugin was based on something like the useJwt plugin, which was using onRequest. Switching to onRequestParse and the exception is properly handled 👍🏼

[peterklingelhofer]

I believe this may have been fixed: #3149

[EmrysMyrddin]

It's in part fixed, but actually I think we have to really make it stand in the documentation that onRequest and onResponse should be used in last resort and with a lot of caution.