Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throwing an error from a plugin result in Unexpected response #2984

Open
Tracked by #2700 ...
j0k3r opened this issue Sep 4, 2023 · 4 comments
Open
Tracked by #2700 ...

Throwing an error from a plugin result in Unexpected response #2984

j0k3r opened this issue Sep 4, 2023 · 4 comments
Assignees
Labels
good first issue kind/docs Improvements or additions to documentation

Comments

@j0k3r
Copy link

j0k3r commented Sep 4, 2023

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:

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 EmrysMyrddin added the stage/2-failing-test A failing test was created that describes the issue label Sep 22, 2023
@EmrysMyrddin EmrysMyrddin self-assigned this Sep 22, 2023
@EmrysMyrddin
Copy link
Collaborator

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
Copy link
Author

j0k3r commented Sep 22, 2023

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
Copy link

peterklingelhofer commented Jan 9, 2024

I believe this may have been fixed: #3149

@j0k3r j0k3r closed this as completed Jan 9, 2024
@EmrysMyrddin
Copy link
Collaborator

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.

@EmrysMyrddin EmrysMyrddin reopened this Jan 10, 2024
@EmrysMyrddin EmrysMyrddin added kind/docs Improvements or additions to documentation good first issue and removed stage/2-failing-test A failing test was created that describes the issue labels Jan 10, 2024
This was referenced May 7, 2024
This was referenced May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue kind/docs Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants