machinebox / graphql

Simple low-level GraphQL HTTP client for Go
https://blog.machinebox.io/a-graphql-client-library-for-go-5bffd0455878
Apache License 2.0
933 stars 217 forks source link

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

Open keyneston opened 4 years ago

keyneston commented 4 years ago

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.

https://github.com/machinebox/graphql/blob/3a92531802258604bd12793465c2e28bc4b2fc85/graphql.go#L140-L145

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 commented 4 years ago

This looks like it might be resolved in https://github.com/machinebox/graphql/pull/47 - but it's not been merged yet.