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

Library struggles with non-200 status codes which have valid JSON bodies #50

Open
keyneston opened this issue Nov 20, 2019 · 1 comment
Open

Comments

@keyneston
Copy link

@keyneston keyneston commented Nov 20, 2019

We noticed an issue when using this library against AWS API Gateway with a WAF, although this issue is not limited to these technologies.

When hitting a WAF if you don't pass the rules you will get a 403 response with the body of {"message":"Forbidden"}. When graphql parses this it it chokes.

graphql/graphql.go

Lines 140 to 145 in 3a92531

if err := json.NewDecoder(&buf).Decode(&gr); err != nil {
if res.StatusCode != http.StatusOK {
return fmt.Errorf("graphql: server returned a non-200 status code: %v", res.StatusCode)
}
return errors.Wrap(err, "decoding response")
}

If you read the above code it attempts to decode the JSON body. Only if this fails does it check the status code. This causes bugs because the status code is 403, and a non-graphql layer is returning a JSON output.

This ends up with a graphql response that thinks it is successful, but in reality failed. The only sign that it failed is that the object that gets returned doesn't have any decoded data in it (unless it by chance has a "message" field).

My suggested fix would be to check the status code first, and error if it is a non-200. While this is not an official API definition APIs-guru points out this issue:

Note: For any non-2XX response, the client should not rely on the body to be in GraphQL format since the source of the response may not be the GraphQL server but instead some intermediary such as API gateways, firewalls, etc.

My concern would be that some non-compliant graphql instance would be returning 403 and similar error codes with valid graphql errors. In which case you encounter a new bug. Potentially we could do both, check if status code is non-200 and return an error. Then also parse the JSON?

@a-h

This comment has been minimized.

Copy link

@a-h a-h commented Nov 20, 2019

This looks like it might be resolved in #47 - but it's not been merged yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.